[edk2,v2,08/24] ArmVirtPkg: implement ArmVirtTimerFdtClientLib

Message ID 1460108711-12122-9-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit ea62bb766fbbee87840751bcbdeb43f1adc43ec6
Headers show

Commit Message

Ard Biesheuvel April 8, 2016, 9:44 a.m.
This implements a library ArmVirtTimerFdtClientLib which is intended to
be incorporated into TimerDxe via NULL library class resolution. This
allows us to make TimerDxe depend on the FDT client protocol, and
discover the timer interrupts 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/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c   | 86 ++++++++++++++++++++
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf | 45 ++++++++++
 2 files changed, 131 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 8, 2016, 3:51 p.m. | #1
On 04/08/16 11:44, Ard Biesheuvel wrote:
> This implements a library ArmVirtTimerFdtClientLib which is intended to

> be incorporated into TimerDxe via NULL library class resolution. This

> allows us to make TimerDxe depend on the FDT client protocol, and

> discover the timer interrupts from the device tree directly rather than

> relying on VirtFdtDxe to set the dynamic PCDs.


I agree that the four PCDs massaged here are only consumed by
"ArmPkg/Drivers/TimerDxe".

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c   | 86 ++++++++++++++++++++

>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf | 45 ++++++++++

>  2 files changed, 131 insertions(+)

> 

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

> new file mode 100644

> index 000000000000..08e24fd82be5

> --- /dev/null

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

> @@ -0,0 +1,86 @@

> +/** @file

> +  FDT client library for ARM's TimerDxe

> +

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


Please add UefiBootServicesTableLib to [LibraryClasses].

> +

> +#include <Protocol/FdtClient.h>

> +

> +typedef struct {

> +  UINT32  Type;

> +  UINT32  Number;

> +  UINT32  Flags;

> +} INTERRUPT_PROPERTY;

> +

> +RETURN_STATUS

> +EFIAPI

> +ArmVirtTimerFdtClientLibConstructor (

> +  VOID

> +  )

> +{

> +  EFI_STATUS                    Status;

> +  FDT_CLIENT_PROTOCOL           *FdtClient;

> +  CONST INTERRUPT_PROPERTY      *InterruptProp;


Ah, so it will point directly into the DTB. This is an opportunity to
document the source then: please bracket the INTERRUPT_PROPERTY typedef
with:

#pragma pack (1)
...
#pragma pack ()

> +  UINT32                        PropSize;

> +  INT32                         SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;

> +

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

> +                  (VOID **)&FdtClient);

> +  ASSERT_EFI_ERROR (Status);


This is right, but I'll have a note at the end.

> +

> +  Status = FdtClient->FindCompatibleNodeProperty (FdtClient, "arm,armv7-timer",

> +                        "interrupts", (CONST VOID **)&InterruptProp,

> +                        &PropSize);

> +  if (Status == EFI_NOT_FOUND) {

> +    Status = FdtClient->FindCompatibleNodeProperty (FdtClient,

> +                          "arm,armv8-timer", "interrupts",

> +                          (CONST VOID **)&InterruptProp,

> +                          &PropSize);

> +  }

> +

> +  if (EFI_ERROR (Status)) {

> +    return Status;

> +  }


This will again trigger the library construct assertion, but that's
okay; I guess we'd be busted without a timer anyway.

> +

> +  //

> +  // - interrupts : Interrupt list for secure, non-secure, virtual and

> +  //  hypervisor timers, in that order.

> +  //

> +  ASSERT (PropSize == 36 || PropSize == 48);

> +

> +  SecIntrNum = SwapBytes32 (InterruptProp[0].Number)

> +               + (InterruptProp[0].Type ? 16 : 0);

> +  IntrNum = SwapBytes32 (InterruptProp[1].Number)

> +            + (InterruptProp[1].Type ? 16 : 0);

> +  VirtIntrNum = SwapBytes32 (InterruptProp[2].Number)

> +                + (InterruptProp[2].Type ? 16 : 0);

> +  HypIntrNum = PropSize < 48 ? 0 : SwapBytes32 (InterruptProp[3].Number)

> +                                   + (InterruptProp[3].Type ? 16 : 0);

> +

> +  DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",

> +    SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));

> +

> +  PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);

> +  PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);

> +  PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);

> +  PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);

> +

> +  return EFI_SUCCESS;

> +}

> +

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

> new file mode 100644

> index 000000000000..e54c401b305e

> --- /dev/null

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

> @@ -0,0 +1,45 @@

> +#/** @file

> +#  FDT client library for ARM's TimerDxe

> +#

> +#  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                      = ArmVirtTimerFdtClientLib

> +  FILE_GUID                      = 77EA80CA-2EFB-4AB4-8567-230D968F6B37

> +  MODULE_TYPE                    = BASE

> +  VERSION_STRING                 = 1.0

> +  LIBRARY_CLASS                  = ArmVirtTimerFdtClientLib|DXE_DRIVER

> +  CONSTRUCTOR                    = ArmVirtTimerFdtClientLibConstructor

> +

> +[Sources]

> +  ArmVirtTimerFdtClientLib.c

> +

> +[Packages]

> +  ArmPkg/ArmPkg.dec

> +  ArmVirtPkg/ArmVirtPkg.dec

> +  MdePkg/MdePkg.dec

> +

> +[LibraryClasses]

> +  BaseLib

> +  DebugLib

> +  PcdLib

> +

> +[Protocols]

> +  gFdtClientProtocolGuid                                ## CONSUMES

> +

> +[Pcd]

> +  gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum

> +  gArmTokenSpaceGuid.PcdArmArchTimerIntrNum

> +  gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum

> +  gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum

> 


Since the stated goal of this library (class & instance alike) is to be
linked into TimerDxe, and to imbue the latter with a dependency on the
FDT client protocol, please move the DepEx hunk from the next patch into
this one.

(The DepEx also belongs together with the ASSERT_EFI_ERROR() above.)

With the above addressed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>


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/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
new file mode 100644
index 000000000000..08e24fd82be5
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
@@ -0,0 +1,86 @@ 
+/** @file
+  FDT client library for ARM's TimerDxe
+
+  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>
+
+typedef struct {
+  UINT32  Type;
+  UINT32  Number;
+  UINT32  Flags;
+} INTERRUPT_PROPERTY;
+
+RETURN_STATUS
+EFIAPI
+ArmVirtTimerFdtClientLibConstructor (
+  VOID
+  )
+{
+  EFI_STATUS                    Status;
+  FDT_CLIENT_PROTOCOL           *FdtClient;
+  CONST INTERRUPT_PROPERTY      *InterruptProp;
+  UINT32                        PropSize;
+  INT32                         SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
+
+  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+                  (VOID **)&FdtClient);
+  ASSERT_EFI_ERROR (Status);
+
+  Status = FdtClient->FindCompatibleNodeProperty (FdtClient, "arm,armv7-timer",
+                        "interrupts", (CONST VOID **)&InterruptProp,
+                        &PropSize);
+  if (Status == EFI_NOT_FOUND) {
+    Status = FdtClient->FindCompatibleNodeProperty (FdtClient,
+                          "arm,armv8-timer", "interrupts",
+                          (CONST VOID **)&InterruptProp,
+                          &PropSize);
+  }
+
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // - interrupts : Interrupt list for secure, non-secure, virtual and
+  //  hypervisor timers, in that order.
+  //
+  ASSERT (PropSize == 36 || PropSize == 48);
+
+  SecIntrNum = SwapBytes32 (InterruptProp[0].Number)
+               + (InterruptProp[0].Type ? 16 : 0);
+  IntrNum = SwapBytes32 (InterruptProp[1].Number)
+            + (InterruptProp[1].Type ? 16 : 0);
+  VirtIntrNum = SwapBytes32 (InterruptProp[2].Number)
+                + (InterruptProp[2].Type ? 16 : 0);
+  HypIntrNum = PropSize < 48 ? 0 : SwapBytes32 (InterruptProp[3].Number)
+                                   + (InterruptProp[3].Type ? 16 : 0);
+
+  DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
+    SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
+
+  PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
+  PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
+  PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
+  PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
+
+  return EFI_SUCCESS;
+}
+
diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
new file mode 100644
index 000000000000..e54c401b305e
--- /dev/null
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
@@ -0,0 +1,45 @@ 
+#/** @file
+#  FDT client library for ARM's TimerDxe
+#
+#  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                      = ArmVirtTimerFdtClientLib
+  FILE_GUID                      = 77EA80CA-2EFB-4AB4-8567-230D968F6B37
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmVirtTimerFdtClientLib|DXE_DRIVER
+  CONSTRUCTOR                    = ArmVirtTimerFdtClientLibConstructor
+
+[Sources]
+  ArmVirtTimerFdtClientLib.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  ArmVirtPkg/ArmVirtPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  PcdLib
+
+[Protocols]
+  gFdtClientProtocolGuid                                ## CONSUMES
+
+[Pcd]
+  gArmTokenSpaceGuid.PcdArmArchTimerSecIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum