Re: [edk2] [PATCH v3 1/9] ArmVirtPkg: implement ArmVirtPL031FdtClientLib

[Laszlo Ersek]

On 04/13/16 15:18, Ard Biesheuvel wrote:
> On 13 April 2016 at 14:16, Laszlo Ersek  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 
>>> ---
>>>  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

Re: [edk2] [PATCH v3 1/9] ArmVirtPkg: implement ArmVirtPL031FdtClientLib

[Ard Biesheuvel]

On 13 April 2016 at 14:16, Laszlo Ersek  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 
>> ---
>>  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 ..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 ..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.
>> +
>> +  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 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +RETURN_STATUS
>> +EFIAPI
>> +ArmVirtPL031FdtClientLibConstructor (
>> +  VOID
>> +  )
>> +{
>> +  EFI_STATUSStatus;
>> +  FDT_CLIENT_PROTOCOL   *FdtClient;
>> +  INT32 Node;
>> +  CONST UINT64  *Reg;
>> +  UINT32RegSize;
>> +  UINT64RegBase;
>> +
>> +  Status = gBS->LocateProtocol (, NULL,
>> +  (VOID **));
>> +  ASSERT_EFI_ERROR (Status);
>> +
>> +  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", );
>> +  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",
>> + 

Re: [edk2] [PATCH v3 1/9] ArmVirtPkg: implement ArmVirtPL031FdtClientLib

[Laszlo Ersek]

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 
> ---
>  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 ..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 ..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.
> +
> +  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 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +RETURN_STATUS
> +EFIAPI
> +ArmVirtPL031FdtClientLibConstructor (
> +  VOID
> +  )
> +{
> +  EFI_STATUSStatus;
> +  FDT_CLIENT_PROTOCOL   *FdtClient;
> +  INT32 Node;
> +  CONST UINT64  *Reg;
> +  UINT32RegSize;
> +  UINT64RegBase;
> +
> +  Status = gBS->LocateProtocol (, NULL,
> +  (VOID **));
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", );
> +  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 **), );
> +  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]);
> + 

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

[Ard Biesheuvel]

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 
---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf | 47 

 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c   | 80 

 2 files changed, 127 insertions(+)

diff --git 
a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf 
b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.inf
new file mode 100644
index ..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 ..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.
+
+  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 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+RETURN_STATUS
+EFIAPI
+ArmVirtPL031FdtClientLibConstructor (
+  VOID
+  )
+{
+  EFI_STATUSStatus;
+  FDT_CLIENT_PROTOCOL   *FdtClient;
+  INT32 Node;
+  CONST UINT64  *Reg;
+  UINT32RegSize;
+  UINT64RegBase;
+
+  Status = gBS->LocateProtocol (, NULL,
+  (VOID **));
+  ASSERT_EFI_ERROR (Status);
+
+  Status = FdtClient->FindCompatibleNode (FdtClient, "arm,pl031", );
+  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 **), );
+  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