[edk2,v3,1/9] ArmVirtPkg: implement ArmVirtPL031FdtClientLib

Message ID 1460534539-2169-2-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel April 13, 2016, 8:02 a.m.
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.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf | 47 ++++++++++++
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 80 ++++++++++++++++++++
 2 files changed, 127 insertions(+)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek April 13, 2016, 12:16 p.m. | #1
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.biesheuvel@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!

> 

> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf

> new file mode 100644

> index 000000000000..65ba8f356d1d

> --- /dev/null

> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf

> @@ -0,0 +1,47 @@

> +#/** @file

> +#  FDT client library for ARM's PL031 RTC driver

> +#

> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.

> +#

> +#  This program and the accompanying materials

> +#  are licensed and made available under the terms and conditions of the BSD License

> +#  which accompanies this distribution. The full text of the license may be found at

> +#  http://opensource.org/licenses/bsd-license.php

> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

> +#

> +#

> +#**/


(1) I think the empty lines could be distributed better. (Same comment
as for an earlier patch).

> +

> +[Defines]

> +  INF_VERSION                    = 0x00010005

> +  BASE_NAME                      = ArmVirtPL031FdtClientLib

> +  FILE_GUID                      = 13173319-B270-4669-8592-3BB2B31E9E29

> +  MODULE_TYPE                    = BASE

> +  VERSION_STRING                 = 1.0

> +  LIBRARY_CLASS                  = ArmVirtPL031FdtClientLib|DXE_DRIVER DXE_RUNTIME_DRIVER

> +  CONSTRUCTOR                    = ArmVirtPL031FdtClientLibConstructor

> +

> +[Sources]

> +  ArmVirtPL031FdtClientLib.c

> +

> +[Packages]

> +  ArmPkg/ArmPkg.dec


(2) What needs "ArmPkg/ArmPkg.dec"?

> +  ArmPlatformPkg/ArmPlatformPkg.dec

> +  ArmVirtPkg/ArmVirtPkg.dec

> +  MdePkg/MdePkg.dec

> +

> +[LibraryClasses]

> +  BaseLib

> +  DebugLib

> +  PcdLib

> +  UefiBootServicesTableLib

> +

> +[Protocols]

> +  gFdtClientProtocolGuid                                ## CONSUMES

> +

> +[Pcd]

> +  gArmPlatformTokenSpaceGuid.PcdPL031RtcBase

> +

> +[Depex]

> +  gFdtClientProtocolGuid


> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c

> new file mode 100644

> index 000000000000..02ab404d5763

> --- /dev/null

> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c

> @@ -0,0 +1,80 @@

> +/** @file

> +  FDT client library for ARM's PL031 RTC driver

> +

> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

> +

> +  This program and the accompanying materials

> +  are licensed and made available under the terms and conditions of the BSD License

> +  which accompanies this distribution.  The full text of the license may be found at

> +  http://opensource.org/licenses/bsd-license.php

> +

> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

> +

> +**/

> +

> +#include <Uefi.h>

> +

> +#include <Library/BaseLib.h>

> +#include <Library/DebugLib.h>

> +#include <Library/PcdLib.h>

> +#include <Library/UefiBootServicesTableLib.h>

> +

> +#include <Protocol/FdtClient.h>

> +

> +RETURN_STATUS

> +EFIAPI

> +ArmVirtPL031FdtClientLibConstructor (

> +  VOID

> +  )

> +{

> +  EFI_STATUS                    Status;

> +  FDT_CLIENT_PROTOCOL           *FdtClient;

> +  INT32                         Node;

> +  CONST UINT64                  *Reg;

> +  UINT32                        RegSize;

> +  UINT64                        RegBase;

> +

> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

> +                  (VOID **)&FdtClient);

> +  ASSERT_EFI_ERROR (Status);

> +

> +  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", &Node);

> +  if (EFI_ERROR (Status)) {

> +    DEBUG ((EFI_D_WARN, "%a: No 'arm,pl031' compatible DT node found\n",

> +      __FUNCTION__));

> +    return EFI_SUCCESS;

> +  }

> +

> +  Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",

> +                        (CONST VOID **)&Reg, &RegSize);

> +  if (EFI_ERROR (Status)) {

> +    DEBUG ((EFI_D_WARN,

> +      "%a: No 'reg' property found in 'arm,pl031' compatible DT node\n",

> +      __FUNCTION__));

> +    return EFI_SUCCESS;

> +  }

> +

> +  ASSERT (RegSize == 16);

> +

> +  RegBase = SwapBytes64 (Reg[0]);

> +  ASSERT (RegBase < MAX_UINT32);

> +

> +  PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);

> +

> +  DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));

> +

> +  //

> +  // 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.

> +

> +  return EFI_SUCCESS;

> +}

> 


(4) Let me see if the NULL library class resolution, predicted in the
commit message of this patch, and implemented in patch v3 2/9, is
safe... The direct user of PcdPL031RtcBase is the library instance

  ArmPlatformPkg/Library/PL031RealTimeClockLib

Now, if that library instance had a constructor, and that constructor
consumed the PCD, then this solution would not be safe, because it would
not enforce any ordering between the constructors. In that case,
PL031RealTimeClockLib would have to be made dependent on this library class.

However, I'm noticing that the call chain is actually the following:

(i) The entry point function of "EmbeddedPkg/RealTimeClockRuntimeDxe",
namely InitializeRealTimeClock(), calls LibRtcInitialize() explicitly.

(ii) In
"ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c",
LibRtcInitialize() consumes PcdPL031RtcBase.

(iii) Given that our constructor here will be executed before the entry
point of RealTimeClockRuntimeDxe is called, this solution is safe after all.

I can't easily decide if I should call this "brittle" or not. After all,
the fact that LibRtcInitialize() needs to be called explicitly by its
consumers is part of that library class. I guess we can rely on that.

A problem would arise if there was another (independent) library
instance that called LibRtcInitialize() in its constructor. However,
that use case is broken in general (BaseTools cannot track such
dependencies); it is not specific to this patch.

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?

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 13, 2016, 1:18 p.m. | #2
On 13 April 2016 at 14:16, Laszlo Ersek <lersek@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.biesheuvel@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 :-)

>>

>> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf

>> new file mode 100644

>> index 000000000000..65ba8f356d1d

>> --- /dev/null

>> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf

>> @@ -0,0 +1,47 @@

>> +#/** @file

>> +#  FDT client library for ARM's PL031 RTC driver

>> +#

>> +#  Copyright (c) 2016, Linaro Ltd. All rights reserved.

>> +#

>> +#  This program and the accompanying materials

>> +#  are licensed and made available under the terms and conditions of the BSD License

>> +#  which accompanies this distribution. The full text of the license may be found at

>> +#  http://opensource.org/licenses/bsd-license.php

>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>> +#

>> +#

>> +#**/

>

> (1) I think the empty lines could be distributed better. (Same comment

> as for an earlier patch).

>

>> +

>> +[Defines]

>> +  INF_VERSION                    = 0x00010005

>> +  BASE_NAME                      = ArmVirtPL031FdtClientLib

>> +  FILE_GUID                      = 13173319-B270-4669-8592-3BB2B31E9E29

>> +  MODULE_TYPE                    = BASE

>> +  VERSION_STRING                 = 1.0

>> +  LIBRARY_CLASS                  = ArmVirtPL031FdtClientLib|DXE_DRIVER DXE_RUNTIME_DRIVER

>> +  CONSTRUCTOR                    = ArmVirtPL031FdtClientLibConstructor

>> +

>> +[Sources]

>> +  ArmVirtPL031FdtClientLib.c

>> +

>> +[Packages]

>> +  ArmPkg/ArmPkg.dec

>

> (2) What needs "ArmPkg/ArmPkg.dec"?

>


Probably nothing, will remove it I can ...

>> +  ArmPlatformPkg/ArmPlatformPkg.dec

>> +  ArmVirtPkg/ArmVirtPkg.dec

>> +  MdePkg/MdePkg.dec

>> +

>> +[LibraryClasses]

>> +  BaseLib

>> +  DebugLib

>> +  PcdLib

>> +  UefiBootServicesTableLib

>> +

>> +[Protocols]

>> +  gFdtClientProtocolGuid                                ## CONSUMES

>> +

>> +[Pcd]

>> +  gArmPlatformTokenSpaceGuid.PcdPL031RtcBase

>> +

>> +[Depex]

>> +  gFdtClientProtocolGuid

>

>> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c

>> new file mode 100644

>> index 000000000000..02ab404d5763

>> --- /dev/null

>> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c

>> @@ -0,0 +1,80 @@

>> +/** @file

>> +  FDT client library for ARM's PL031 RTC driver

>> +

>> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>

>> +

>> +  This program and the accompanying materials

>> +  are licensed and made available under the terms and conditions of the BSD License

>> +  which accompanies this distribution.  The full text of the license may be found at

>> +  http://opensource.org/licenses/bsd-license.php

>> +

>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,

>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>> +

>> +**/

>> +

>> +#include <Uefi.h>

>> +

>> +#include <Library/BaseLib.h>

>> +#include <Library/DebugLib.h>

>> +#include <Library/PcdLib.h>

>> +#include <Library/UefiBootServicesTableLib.h>

>> +

>> +#include <Protocol/FdtClient.h>

>> +

>> +RETURN_STATUS

>> +EFIAPI

>> +ArmVirtPL031FdtClientLibConstructor (

>> +  VOID

>> +  )

>> +{

>> +  EFI_STATUS                    Status;

>> +  FDT_CLIENT_PROTOCOL           *FdtClient;

>> +  INT32                         Node;

>> +  CONST UINT64                  *Reg;

>> +  UINT32                        RegSize;

>> +  UINT64                        RegBase;

>> +

>> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

>> +                  (VOID **)&FdtClient);

>> +  ASSERT_EFI_ERROR (Status);

>> +

>> +  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", &Node);

>> +  if (EFI_ERROR (Status)) {

>> +    DEBUG ((EFI_D_WARN, "%a: No 'arm,pl031' compatible DT node found\n",

>> +      __FUNCTION__));

>> +    return EFI_SUCCESS;

>> +  }

>> +

>> +  Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",

>> +                        (CONST VOID **)&Reg, &RegSize);

>> +  if (EFI_ERROR (Status)) {

>> +    DEBUG ((EFI_D_WARN,

>> +      "%a: No 'reg' property found in 'arm,pl031' compatible DT node\n",

>> +      __FUNCTION__));

>> +    return EFI_SUCCESS;

>> +  }

>> +

>> +  ASSERT (RegSize == 16);

>> +

>> +  RegBase = SwapBytes64 (Reg[0]);

>> +  ASSERT (RegBase < MAX_UINT32);

>> +

>> +  PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);

>> +

>> +  DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));

>> +

>> +  //

>> +  // 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.

>> +

>> +  return EFI_SUCCESS;

>> +}

>>

>

> (4) Let me see if the NULL library class resolution, predicted in the

> commit message of this patch, and implemented in patch v3 2/9, is

> safe... The direct user of PcdPL031RtcBase is the library instance

>

>   ArmPlatformPkg/Library/PL031RealTimeClockLib

>

> Now, if that library instance had a constructor, and that constructor

> consumed the PCD, then this solution would not be safe, because it would

> not enforce any ordering between the constructors. In that case,

> PL031RealTimeClockLib would have to be made dependent on this library class.

>

> However, I'm noticing that the call chain is actually the following:

>

> (i) The entry point function of "EmbeddedPkg/RealTimeClockRuntimeDxe",

> namely InitializeRealTimeClock(), calls LibRtcInitialize() explicitly.

>

> (ii) In

> "ArmPlatformPkg/Library/PL031RealTimeClockLib/PL031RealTimeClockLib.c",

> LibRtcInitialize() consumes PcdPL031RtcBase.

>

> (iii) Given that our constructor here will be executed before the entry

> point of RealTimeClockRuntimeDxe is called, this solution is safe after all.

>

> I can't easily decide if I should call this "brittle" or not. After all,

> the fact that LibRtcInitialize() needs to be called explicitly by its

> consumers is part of that library class. I guess we can rely on that.

>

> A problem would arise if there was another (independent) library

> instance that called LibRtcInitialize() in its constructor. However,

> that use case is broken in general (BaseTools cannot track such

> dependencies); it is not specific to this patch.

>

> 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek April 13, 2016, 1:35 p.m. | #3
On 04/13/16 15:18, Ard Biesheuvel wrote:
> On 13 April 2016 at 14:16, Laszlo Ersek <lersek@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.biesheuvel@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

Patch

diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
new file mode 100644
index 000000000000..65ba8f356d1d
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
@@ -0,0 +1,47 @@ 
+#/** @file
+#  FDT client library for ARM's PL031 RTC driver
+#
+#  Copyright (c) 2016, Linaro Ltd. All rights reserved.
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD License
+#  which accompanies this distribution. The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = ArmVirtPL031FdtClientLib
+  FILE_GUID                      = 13173319-B270-4669-8592-3BB2B31E9E29
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmVirtPL031FdtClientLib|DXE_DRIVER DXE_RUNTIME_DRIVER
+  CONSTRUCTOR                    = ArmVirtPL031FdtClientLibConstructor
+
+[Sources]
+  ArmVirtPL031FdtClientLib.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  ArmVirtPkg/ArmVirtPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  PcdLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gFdtClientProtocolGuid                                ## CONSUMES
+
+[Pcd]
+  gArmPlatformTokenSpaceGuid.PcdPL031RtcBase
+
+[Depex]
+  gFdtClientProtocolGuid
diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
new file mode 100644
index 000000000000..02ab404d5763
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
@@ -0,0 +1,80 @@ 
+/** @file
+  FDT client library for ARM's PL031 RTC driver
+
+  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/FdtClient.h>
+
+RETURN_STATUS
+EFIAPI
+ArmVirtPL031FdtClientLibConstructor (
+  VOID
+  )
+{
+  EFI_STATUS                    Status;
+  FDT_CLIENT_PROTOCOL           *FdtClient;
+  INT32                         Node;
+  CONST UINT64                  *Reg;
+  UINT32                        RegSize;
+  UINT64                        RegBase;
+
+  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+                  (VOID **)&FdtClient);
+  ASSERT_EFI_ERROR (Status);
+
+  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", &Node);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_WARN, "%a: No 'arm,pl031' compatible DT node found\n",
+      __FUNCTION__));
+    return EFI_SUCCESS;
+  }
+
+  Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",
+                        (CONST VOID **)&Reg, &RegSize);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_WARN,
+      "%a: No 'reg' property found in 'arm,pl031' compatible DT node\n",
+      __FUNCTION__));
+    return EFI_SUCCESS;
+  }
+
+  ASSERT (RegSize == 16);
+
+  RegBase = SwapBytes64 (Reg[0]);
+  ASSERT (RegBase < MAX_UINT32);
+
+  PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
+
+  DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
+
+  //
+  // 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"));
+  }
+
+  return EFI_SUCCESS;
+}