[Linaro-uefi,01/11] D02/D03: WA to fix timer interrupt issue

Message ID 1476324020-57155-1-git-send-email-heyi.guo@linaro.org
State Superseded
Headers show

Commit Message

gary guo Oct. 13, 2016, 2 a.m.
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(-)

Comments

Leif Lindholm Oct. 13, 2016, 8:33 a.m. | #1
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
>
Ard Biesheuvel Oct. 13, 2016, 10:19 a.m. | #2
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.
Leif Lindholm Oct. 13, 2016, 10:54 a.m. | #3
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
Ard Biesheuvel Oct. 13, 2016, 11:13 a.m. | #4
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
gary guo Oct. 13, 2016, 12:27 p.m. | #5
在 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
>>
Leif Lindholm Oct. 13, 2016, 1:50 p.m. | #6
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
> >>
>
Leif Lindholm Oct. 13, 2016, 1:57 p.m. | #7
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
>
Leif Lindholm Oct. 13, 2016, 2:08 p.m. | #8
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
>

Patch

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