On Thu, Oct 13, 2016 at 11:19:01AM +0100, Ard Biesheuvel wrote:
On 13 October 2016 at 09:33, Leif Lindholm leif.lindholm@linaro.org wrote:
Please change WA to workaround in subject (there are too many possible things it could be an abbreviation of to make it simple to read :)
On Thu, Oct 13, 2016 at 10:00:10AM +0800, Heyi Guo wrote:
Timer interrupt cannot be reported after EDK2 commit 7989300, for GIC on D02/D03 is not fully ARM GIC compliant. The issue has been fixed on newer chips so we use a WA for D02 and D03 only.
workaround
On D02 and D03, IRQ will be latched in GIC logic except virutal timer interrupt IRQ #27, so we change to use virtual timer instead of physical in UEFI.
Change-Id: Ie8eca7e4dea45a3a318ee32783ddaa15363065e2 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Heyi Guo heyi.guo@linaro.org
Platforms/Hisilicon/D02/Pv660D02.dsc | 10 ++++++++-- Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +- Platforms/Hisilicon/D03/D03.dsc | 10 +++++++++- Platforms/Hisilicon/D03/D03.fdf | 2 +- 4 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc b/Platforms/Hisilicon/D02/Pv660D02.dsc index d025bdd..4b4a0b7 100644 --- a/Platforms/Hisilicon/D02/Pv660D02.dsc +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc @@ -72,6 +72,13 @@ PlatformBdsLib|OpenPlatformPkg/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
+## GIC on D02/D03 is not fully ARM GIC compatible: IRQ cannot be cancelled when +## input signal is deasserted, except for virtual timer interrupt IRQ #27. So +## we choose to use virutal timer instead of physical one as a workaround.
Typo: virutal -> virtual
+## This library instance is to override the one in Pv660.dsc.inc.
This whole block is a very useful comment to describe an otherwise non-obvious change, thanks.
+[LibraryClasses.AARCH64]
Out of interest - why is this added for AARCH64 only? Does the platform build and function properly for ARM, and is this workaround then not needed there?
- ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.inf
[LibraryClasses.common.SEC] ArmPlatformLib|OpenPlatformPkg/Chips/Hisilicon/Library/ArmPlatformLibPv660/ArmPlatformLibSec.inf
@@ -341,8 +348,7 @@
ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
- #ArmPkg/Drivers/TimerDxe/TimerDxe
- ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf
ArmPkg/Drivers/TimerDxe/TimerDxe.inf ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
IntelFrameworkModulePkg/Universal/StatusCode/RuntimeDxe/StatusCodeRuntimeDxe.inf
diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf b/Platforms/Hisilicon/D02/Pv660D02.fdf index 69be1f1..fa0dc2d 100644 --- a/Platforms/Hisilicon/D02/Pv660D02.fdf +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf @@ -195,7 +195,7 @@ READ_LOCK_STATUS = TRUE #INF EmbeddedPkg/SimpleTextInOutSerial/SimpleTextInOutSerial.inf
INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
- INF ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf
INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf INF ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
#
diff --git a/Platforms/Hisilicon/D03/D03.dsc b/Platforms/Hisilicon/D03/D03.dsc index 83a18b1..ccf16a2 100644 --- a/Platforms/Hisilicon/D03/D03.dsc +++ b/Platforms/Hisilicon/D03/D03.dsc @@ -81,6 +81,14 @@
LpcLib|OpenPlatformPkg/Chips/Hisilicon/Binary/Hi1610/Library/LpcLib/LpcLib.inf SerialPortLib|OpenPlatformPkg/Chips/Hisilicon/Binary/Hi1610/Library/Uart/LpcSerialPortLib/LpcSerialPortLib.inf
+## GIC on D02/D03 is not fully ARM GIC compatible: IRQ cannot be cancelled when +## input signal is deasserted, except for virtual timer interrupt IRQ #27. So +## we choose to use virutal timer instead of physical one as a workaround.
Typo: virutal -> virtual
+## This library instance is to override the one in Pv660.dsc.inc. +[LibraryClasses.AARCH64]
Out of interest - why is this added for AARCH64 only? Does the platform build and function properly for ARM, and is this workaround then not needed there?
- ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.inf
[LibraryClasses.common.SEC] ArmPlatformLib|OpenPlatformPkg/Chips/Hisilicon/Library/ArmPlatformLibPv660/ArmPlatformLibSec.inf
@@ -396,7 +404,7 @@
ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
- ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf
ArmPkg/Drivers/TimerDxe/TimerDxe.inf
ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf IntelFrameworkModulePkg/Universal/StatusCode/RuntimeDxe/StatusCodeRuntimeDxe.inf
diff --git a/Platforms/Hisilicon/D03/D03.fdf b/Platforms/Hisilicon/D03/D03.fdf index 8144151..8ba3bd0 100644 --- a/Platforms/Hisilicon/D03/D03.fdf +++ b/Platforms/Hisilicon/D03/D03.fdf @@ -187,7 +187,7 @@ READ_LOCK_STATUS = TRUE # Simple TextIn/TextOut for UEFI Terminal
INF ArmPkg/Drivers/ArmGic/ArmGicDxe.inf
- INF ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf
INF ArmPkg/Drivers/TimerDxe/TimerDxe.inf
INF ArmPlatformPkg/Drivers/SP805WatchdogDxe/SP805WatchdogDxe.inf
For my part, with these minor fixes, I'm happy with this patch. Ard: do you agree that this is the best fix for the problem?
Yes, as I stated before, I think this is reasonable given that this code owns EL2 and so it can ensure that any uses of the virtual timer are consistent with the physical timer. I don't think we care about the actual virtual offset here, so the value of CNTVOFF_EL2 can be ignored.
Can I take that as a Reviewed-by once the nitpicks have been addressed?
/ Leif