Re: [PATCH] ArmPkg/ArmLib: remove indirection layer from timer register accessors

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 January 2017 at 15:50, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 20 January 2017 at 14:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 20 January 2017 at 14:32, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>>> Hi Ard,
>>>
>>> On 20 January 2017 at 12:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> The generic timer support libraries call the actual system register
>>>> accessor function via a single pair of functions ArmArchTimerReadReg()
>>>> and ArmArchTimerWriteReg(), which take an enum to argument to identify
>>>> the register, and return output values by pointer reference.
>>>>
>>>> Since these functions are never called with a non-immediate argument,
>>>> we can simply replace each invocation with the underlying system register
>>>> accessor instead. This is mostly functionally equivalent, with the
>>>> exception of the bounds check for the enum (which is pointless given the
>>>> fact that we never pass a variable), the check for the presence of the
>>>> architected timer (which only makes sense for ARMv7, but is highly unlikely
>>>> to vary between platforms that are similar enough to run the same firmware
>>>> image), and a check for enum values that refer to the HYP view of the timer,
>>>> which we never referred to anywhere in the code in the first place.
>>>>
>>>> So get rid of the middle man, and update the ArmGenericTimerPhyCounterLib
>>>> and ArmGenericTimerVirtCounterLib implementations to call the system
>>>> register accessors directly.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> Are there any other patches needed to get this working?
>>>
>>> I've just applied it to the head of EDK2 [1] and I get this error when
>>> building for TC2, FVP and Juno:
>>>
>>> Building ... /linaro/platforms/uefi/edk2/ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>>> [AARCH64]
>>> /linaro/platforms/uefi/edk2/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c:
>>> In function 'ArmGenericTimerSetTimerFreq':
>>> /linaro/platforms/uefi/edk2/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c:51:19:
>>> error: 'CntFrq' undeclared (first use in this function)
>>>    ArmWriteCntFrq (CntFrq);
>>>                    ^
>>> /linaro/platforms/uefi/edk2/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.c:51:19:
>>> note: each undeclared identifier is reported only once for each
>>> function it appears in
>>> GNUmakefile:304: recipe for target
>>> '/linaro/platforms/uefi/edk2/Build/ArmVExpress-FVP-AArch64/DEBUG_GCC5/AARCH64/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib/OUTPUT/ArmGenericTimerPhyCounterLib.obj'
>>> failed
>>> make: *** [/linaro/platforms/uefi/edk2/Build/ArmVExpress-FVP-AArch64/DEBUG_GCC5/AARCH64/ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib/OUTPUT/ArmGenericTimerPhyCounterLib.obj]
>>> Error 1
>>>
>>
>>
>> Oops! Poor testing on my part: that should be FreqInHz not CntFrq
>
> Naughty! ;-)
>
> Okay, so with that change, it works on TC2, FVP Foundation & AEMv8 and
> Juno R0/R1/R2:
>
> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

Splendid! Thanks