| Message ID | 1476324020-57155-1-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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? Regards, Leif > > -- > 1.9.1 >
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.
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
On 13 October 2016 at 11:54, Leif Lindholm <leif.lindholm@linaro.org> wrote: > 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? > Yes, absolutely
在 10/13/2016 4:33 PM, Leif Lindholm 写道: > 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? This is because the original setting in Pv660.dsc.inc is under [LibraryClasses.AARCH64], so I think if I just use a common section here, it might not be able to override the one in Pv660.dsc.inc, according to Library Instance precedence rule. The reason why it was put under [LibraryClasses.AARCH64] in Pv660.dsc.inc is unclear now; probably it was just copied from other platform. So we may need another patch to change it in Pv660.dsc.inc first, if we want to fix the one in D02 dsc file. Do you think it is necessary to do that? Thanks. Heyi > >> + 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? > > Regards, > > Leif > >> >> -- >> 1.9.1 >>
On Thu, Oct 13, 2016 at 08:27:15PM +0800, Heyi Guo wrote: > >>+## 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? > This is because the original setting in Pv660.dsc.inc is under > [LibraryClasses.AARCH64], so I think if I just use a common section here, it > might not be able to override the one in Pv660.dsc.inc, according to Library > Instance precedence rule. OK, that makes sense. > The reason why it was put under [LibraryClasses.AARCH64] in Pv660.dsc.inc is > unclear now; probably it was just copied from other platform. > > So we may need another patch to change it in Pv660.dsc.inc first, if we want > to fix the one in D02 dsc file. Do you think it is necessary to do that? No. But it would be nice to have a short coment: "# Overriding original defined in LibraryClasses.AARCH64 in Pc660.dsc.inc" Regards, Leif > Thanks. > > Heyi > > > > > >>+ 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? > > > >Regards, > > > >Leif > > > >>-- > >>1.9.1 > >> >
Suggest changing 'one by one' in subject to 'bytewise'. On Thu, Oct 13, 2016 at 10:00:14AM +0800, Heyi Guo wrote: > From: Peicong Li <lipeicong@huawei.com> > > We would get incorrect data when accessing multiple bytes at one time > and across EEPROM page boundary, so we change to access EEPROM by > single byte. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Peicong Li <lipeicong@huawei.com> > --- > .../D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c > index 994ed6a..0760140 100644 > --- a/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c > +++ b/Platforms/Hisilicon/D03/Drivers/OemNicConfig2PHi1610/OemNicConfig2P.c > @@ -106,10 +106,11 @@ UINT16 make_crc_checksum(UINT8 *buf, UINT32 len) > EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr) > { > I2C_DEVICE stI2cDev = {0}; > - EFI_STATUS Status; > + EFI_STATUS Status = EFI_SUCCESS; > UINT16 I2cOffset; > UINT16 crc16; > NIC_MAC_ADDRESS stMacDesc = {0}; > + UINT32 i; > > Status = I2CInit(0, EEPROM_I2C_PORT, Normal); > if (EFI_ERROR(Status)) > @@ -119,12 +120,14 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr) > } > > I2cOffset = I2C_OFFSET_EEPROM_ETH0 + (Port * sizeof(NIC_MAC_ADDRESS)); > - Unrelated whitespace change. > stI2cDev.DeviceType = DEVICE_TYPE_E2PROM; > stI2cDev.Port = EEPROM_I2C_PORT; > stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM; > stI2cDev.Socket = 0; > - Status = I2CRead(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc); > + for(i = 0; i < sizeof(NIC_MAC_ADDRESS); i++) > + { for (...) { (missing space, keep '{' on same line.) > + Status |= I2CRead(&stI2cDev, I2cOffset+i, 1, (UINT8 *)&stMacDesc+i); Spaces around '+'. > + } > if (EFI_ERROR(Status)) > { Keep '{' with 'if'. > DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cRead failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status)); > @@ -147,9 +150,10 @@ EFI_STATUS OemGetMacE2prom(IN UINT32 Port, OUT UINT8 *pucAddr) > EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr) > { > I2C_DEVICE stI2cDev = {0}; > - EFI_STATUS Status; > + EFI_STATUS Status = EFI_SUCCESS; > UINT16 I2cOffset; > NIC_MAC_ADDRESS stMacDesc = {0}; > + UINT32 i; > > > stMacDesc.MacLen = MAC_ADDR_LEN; > @@ -170,7 +174,11 @@ EFI_STATUS OemSetMacE2prom(IN UINT32 Port, IN UINT8 *pucAddr) > stI2cDev.Port = EEPROM_I2C_PORT; > stI2cDev.SlaveDeviceAddress = I2C_SLAVEADDR_EEPROM; > stI2cDev.Socket = 0; > - Status = I2CWrite(&stI2cDev, I2cOffset, sizeof(NIC_MAC_ADDRESS), (UINT8 *)&stMacDesc); > + for(i = 0; i < sizeof(NIC_MAC_ADDRESS); i++) > + { > + Status |= I2CWrite(&stI2cDev, I2cOffset + i, 1, (UINT8 *)&stMacDesc+i); > + } > + Identical comments as for previous for loop. > if (EFI_ERROR(Status)) > { > DEBUG((EFI_D_ERROR, "[%a]:[%dL] Call I2cWrite failed! p1=0x%x.\n", __FUNCTION__, __LINE__, Status)); > -- > 1.9.1 >
Subject should describe the behavioral change, not make a comment that it changes a particular bit of code. Also, needs a message body. Which needs to describe why this change is made, and why there is now two buffers to fill, and a new "socket" parameter. On Thu, Oct 13, 2016 at 10:00:16AM +0800, Heyi Guo wrote: > From: Peicong Li <lipeicong@huawei.com> > > Change-Id: I99009939b55de787c66fb48840a23c8a56b76a12 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Peicong Li <lipeicong@huawei.com> > --- > .../Smbios/SmbiosMiscDxe/Type09/MiscSystemSlotDesignationFunction.c | 3 ++- > Chips/Hisilicon/Hi1610/Include/Library/SerdesLib.h | 2 +- > Chips/Hisilicon/Pv660/Include/Library/SerdesLib.h | 2 +- > Platforms/Hisilicon/D02/Library/OemMiscLibD02/BoardFeatureD02.c | 2 +- > Platforms/Hisilicon/D03/Library/OemMiscLib2P/BoardFeature2PHi1610.c | 2 +- > 5 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/Chips/Hisilicon/Drivers/Smbios/SmbiosMiscDxe/Type09/MiscSystemSlotDesignationFunction.c b/Chips/Hisilicon/Drivers/Smbios/SmbiosMiscDxe/Type09/MiscSystemSlotDesignationFunction.c > index a0e3de3..79f5625 100644 > --- a/Chips/Hisilicon/Drivers/Smbios/SmbiosMiscDxe/Type09/MiscSystemSlotDesignationFunction.c > +++ b/Chips/Hisilicon/Drivers/Smbios/SmbiosMiscDxe/Type09/MiscSystemSlotDesignationFunction.c > @@ -74,8 +74,9 @@ UpdateSlotUsage( > { > EFI_STATUS Status; > serdes_param_t sSerdesParam; > + serdes_param_t sSerdesParamB; If the new one is called B, should not the old one be renamed A? This is how the new D05 code refers to it at its end, and it would be more consistent. > > - Status = OemGetSerdesParam (&sSerdesParam); > + Status = OemGetSerdesParam (&sSerdesParam, &sSerdesParamB, 0); > if(EFI_ERROR(Status)) > { > DEBUG((EFI_D_ERROR, "[%a]:[%dL] OemGetSerdesParam failed %r\n", __FUNCTION__, __LINE__, Status)); > diff --git a/Chips/Hisilicon/Hi1610/Include/Library/SerdesLib.h b/Chips/Hisilicon/Hi1610/Include/Library/SerdesLib.h > index 700d40e..3bd5a0f 100755 > --- a/Chips/Hisilicon/Hi1610/Include/Library/SerdesLib.h > +++ b/Chips/Hisilicon/Hi1610/Include/Library/SerdesLib.h > @@ -82,7 +82,7 @@ typedef struct { > UINT32 DsCfg; > } SERDES_POLARITY_INVERT; > > -EFI_STATUS OemGetSerdesParam (serdes_param_t *Param); > +EFI_STATUS OemGetSerdesParam (serdes_param_t *ParamA, serdes_param_t *ParamB, UINT32 SocketId); > extern SERDES_POLARITY_INVERT gSerdesPolarityTxDesc[]; > extern SERDES_POLARITY_INVERT gSerdesPolarityRxDesc[]; > UINT32 GetEthType(UINT8 EthChannel); > diff --git a/Chips/Hisilicon/Pv660/Include/Library/SerdesLib.h b/Chips/Hisilicon/Pv660/Include/Library/SerdesLib.h > index 070934b..b6c7e20 100644 > --- a/Chips/Hisilicon/Pv660/Include/Library/SerdesLib.h > +++ b/Chips/Hisilicon/Pv660/Include/Library/SerdesLib.h > @@ -76,7 +76,7 @@ typedef struct { > } SERDES_POLARITY_INVERT; > > > -EFI_STATUS OemGetSerdesParam (serdes_param_t *Param); > +EFI_STATUS OemGetSerdesParam (serdes_param_t *ParamA, serdes_param_t *ParamB, UINT32 SocketId); > extern SERDES_POLARITY_INVERT gSerdesPolarityTxDesc[]; > extern SERDES_POLARITY_INVERT gSerdesPolarityRxDesc[]; > UINT32 GetEthType(UINT8 EthChannel); > diff --git a/Platforms/Hisilicon/D02/Library/OemMiscLibD02/BoardFeatureD02.c b/Platforms/Hisilicon/D02/Library/OemMiscLibD02/BoardFeatureD02.c > index d4aa84a..873eb4f 100644 > --- a/Platforms/Hisilicon/D02/Library/OemMiscLibD02/BoardFeatureD02.c > +++ b/Platforms/Hisilicon/D02/Library/OemMiscLibD02/BoardFeatureD02.c > @@ -59,7 +59,7 @@ serdes_param_t gSerdesParam = { > .hilink5_mode = EM_HILINK5_SAS1_4LANE, > }; > > -EFI_STATUS OemGetSerdesParam (serdes_param_t *Param) > +EFI_STATUS OemGetSerdesParam (serdes_param_t *Param, serdes_param_t *ParamB, UINT32 SocketId) > { > if (NULL == Param) > { > diff --git a/Platforms/Hisilicon/D03/Library/OemMiscLib2P/BoardFeature2PHi1610.c b/Platforms/Hisilicon/D03/Library/OemMiscLib2P/BoardFeature2PHi1610.c > index 23c55e1..d25c50e 100644 > --- a/Platforms/Hisilicon/D03/Library/OemMiscLib2P/BoardFeature2PHi1610.c > +++ b/Platforms/Hisilicon/D03/Library/OemMiscLib2P/BoardFeature2PHi1610.c > @@ -75,7 +75,7 @@ serdes_param_t gSerdesParam1 = { > .use_ssc = 0, > }; > > -EFI_STATUS OemGetSerdesParam (serdes_param_t *Param) > +EFI_STATUS OemGetSerdesParam (serdes_param_t *Param, serdes_param_t *ParamB, UINT32 SocketId) > { > if (NULL == Param) > { > -- > 1.9.1 >
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. +## This library instance is to override the one in Pv660.dsc.inc. +[LibraryClasses.AARCH64] + 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. +## This library instance is to override the one in Pv660.dsc.inc. +[LibraryClasses.AARCH64] + 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
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. 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(-)