On 04/13/16 15:18, Ard Biesheuvel wrote:
> On 13 April 2016 at 14:16, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 04/13/16 10:02, Ard Biesheuvel wrote:
>>> This implements a library ArmVirtPL031FdtClientLib which is intended to
>>> be incorporated into RealTimeClockRuntimeDxe via NULL library class
>>> resolution. This allows us to make RealTimeClockRuntimeDxe depend on the
>>> FDT client protocol, and discover the PL031 base address from the device
>>> tree
>>> directly rather than relying on VirtFdtDxe to set the dynamic PCDs.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> ---
>>> ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf |
>>> 47 ++++++++++++
>>> ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c |
>>> 80 ++++++++++++++++++++
>>> 2 files changed, 127 insertions(+)
>>
>> Heh, did you adopt a git-diff-order file that prioritizes *.inf over
>> *.c? :) If so, thank you!
>>
>
> Only for ArmVirtPkg and OvmfPkg patches :-)
I'm sure your kernel patch reviewers would also appreciate if you put
*.h first! ;)
>>> + //
>>> + // UEFI takes ownership of the RTC hardware, and exposes its
>>> functionality
>>> + // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
>>> + // need to disable it in the device tree to prevent the OS from attaching
>>> + // its device driver as well.
>>> + //
>>> + Status = FdtClient->SetNodeProperty (FdtClient, Node, "status",
>>> + "disabled", sizeof ("disabled"));
>>> + if (EFI_ERROR (Status)) {
>>> + DEBUG ((EFI_D_WARN, "Failed to set PL031 status to 'disabled'\n"));
>>> + }
>>
>> (3) I believe you forgot that this last section should depend on
>>
>> !FeaturePcdGet (PcdPureAcpiBoot)
>>
>> There are three such actions; the chosen node thing, the config table
>> install, and the RTC node disablement. The first is handled well in
>> commit cd2178bb73b5, the second is handled well in patch v3 6/9, and the
>> third should be fixed here, I believe.
>>
>
> It does not depend on it, strictly speaking, there is just little
> point in manipulating the FDT if it is never going to be passed to the
> OS.
I agree about the dependency not being strict, but since this is a
conversion / refactoring, let's stick with the original logic (plus
let's remain consistent with the other manipulation sites.)
(I believe you didn't disagree with my request, just wanted to make it
more precise -- and now I wanted to make it even more precise. :))
>> Summary:
>> - please fix (1)
>> - please investigate (2)
>> - please fix (3)
>> - please update the commit message according to (4); i.e., please
>> explain why this solution is safe. I think adding (i) through (iii) --
>> as a single paragraph -- should suffice.
>>
>> ... Actually, I don't see any other users of
>>
>> ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.inf
>>
>> in the edk2 tree.
>>
>> So, how about relocating the library instance to ArmVirtPkg, removing
>> PcdPL031RtcBase for good (from "ArmPlatformPkg/ArmPlatformPkg.dec" and
>> everywhere else), and consuming the FDT directly in LibRtcInitialize()?
>>
>> (Xen would not be disturbed by this, because it links
>> RealTimeClockRuntimeDxe against XenRealTimeClockLib.)
>>
>> Or does something in Leif's platform tree use this library?
>>
>
> PL031 is a standard ARM RTC IP block, and QEMU happens to emulate it.
> So it really does not belong in ArmVirtPkg
Makes perfect sense, thanks.
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
