Re: [edk2] [PATCH 3/3] ArmPlatformPkg: PL061: support multiple controller

2015-08-18 Thread Ard Biesheuvel 
On 18 August 2015 at 14:30, Haojian Zhuang haojian.zhu...@linaro.org wrote:
 On Tue, 2015-08-18 at 11:43 +0200, Ard Biesheuvel wrote:
 On 18 August 2015 at 11:31, Haojian Zhuang haojian.zhu...@linaro.org wrote:
  On Tue, 2015-08-18 at 11:22 +0200, Ard Biesheuvel wrote:
  On 18 August 2015 at 11:04, Haojian Zhuang haojian.zhu...@linaro.org 
  wrote:
   Support multiple PL061 controllers.
  
   Contributed-under: TianoCore Contribution Agreement 1.0
   Signed-off-by: Haojian Zhuang haojian.zhu...@linaro.org
   ---
ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c| 107 
   ++---
.../Drivers/PL061GpioDxe/PL061GpioDxe.inf  |   3 +-
ArmPlatformPkg/Include/Drivers/PL061Gpio.h |  40 
3 files changed, 96 insertions(+), 54 deletions(-)
  
   diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c 
   b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
   index e8a2094..3027656 100644
   --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
   +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
   @@ -28,6 +28,7 @@
#include Drivers/PL061Gpio.h
  
BOOLEAN mPL061Initialized = FALSE;
   +PLATFORM_GPIO_CONTROLLER *mPL061PlatformGpio;
  
/**
  Function implementations
   @@ -38,20 +39,31 @@ PL061Identify (
  VOID
  )
{
   -  // Check if this is a PrimeCell Peripheral
   -  if ((MmioRead8 (PL061_GPIO_PCELL_ID0) != 0x0D)
   -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID1) != 0xF0)
   -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID2) != 0x05)
   -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID3) != 0xB1)) {
   -return EFI_NOT_FOUND;
   +  UINTNIndex;
   +  UINT32   RegisterBase;
 
  Why is this a 32-bit value?
 
  All registers in PL061 are 32-bit. So we don't need to define UINTN at
  here.
 

 So what happens if the GPIO controller is mapped above 4 GB in physical 
 memory?


 OK. I'll use 64-bit register base at here. But the original PL061 driver
 is also using 32-bit register base. We could get the definition from
 PL061Gpio.h
 #define PL061_GPIO_DATA_REG ((UINT32)PcdGet32
 (PcdPL061GpioBase) + 0x000)


OK. We should also fix that at some point, but for new code, please
don't make any 32-bit assumptions

   @@ -329,6 +367,9 @@ PL061InstallProtocol (
  //
  ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, gEmbeddedGpioProtocolGuid);
  
   +  Status = gBS-LocateProtocol (gPlatformGpioProtocolGuid, NULL, 
   (VOID **)mPL061PlatformGpio);
   +  ASSERT_EFI_ERROR (Status);
   +
 
  We should fall back to using the PCD if the protocol cannot be found.
  This way, we won't break existing users.
 
 
  I was intended to use fallback. But I think it's not good enough.
 
  1. If anyone defines the PCD register base in dec file. The fallback
  mechanism will be broken.
 

 Well, you would only look at the PCD if the protocol is not installed,
 so I don't see how that would change anything compared to the current
 situation.

  2. Since every driver is built as module, we must create the driver
  dependency.
 

 Please try and use a BEFORE ... depex instead, you can put it in your
 platform GPIO DXE with the file GUID of the PL061 DXE driver.


 I tried to use BEFORE in inf. Here's the content in platform gpio inf.
 [Defines]
   INF_VERSION= 0x00010005
   BASE_NAME  = HiKeyGpio
   FILE_GUID  = b51a851c-7bf7-463f-b261-cfb158b7f699
   MODULE_TYPE= DXE_DRIVER
   VERSION_STRING = 1.0
   ENTRY_POINT= HiKeyGpioEntryPoint

 [Protocols]
   gPlatformGpioProtocolGuid

 [Depex]
   BEFORE gEmbeddedGpioProtocolGuid


You need to use the file GUID of PL061 DXE here. Perhaps you should
add it as a new GUID definition in the HiSiPkg .dec, with the value
5c1997d7-8d45-4f21-af3c-2206b8ed8bec


 And here's updated PL061 driver.

 PL061InstallProtocol (
   IN EFI_HANDLE ImageHandle,
   IN EFI_SYSTEM_TABLE   *SystemTable
   )
 {
   EFI_STATUSStatus;
   EFI_HANDLEHandle;
   GPIO_CONTROLLER   *GpioController;

   //
   // Make sure the Gpio protocol has not been installed in the system
 yet.
   //
   ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, gEmbeddedGpioProtocolGuid);

   Status = gBS-LocateProtocol (gPlatformGpioProtocolGuid, NULL, (VOID
 **)mPL061PlatformGpio);
   DEBUG ((EFI_D_ERROR, #%a, %d, Status:%r\n, __func__, __LINE__,
 Status));
   if (EFI_ERROR (Status)  (Status == EFI_NOT_FOUND)) {
 // Create the mPL061PlatformGpio
 mPL061PlatformGpio = (PLATFORM_GPIO_CONTROLLER *)AllocateZeroPool
 (sizeof (PLATFORM_GPIO_CONTROLLER) + sizeof (GPIO_CONTROLLER));
 if (mPL061PlatformGpio == NULL) {
   DEBUG ((EFI_D_ERROR, %a: failed to allocate
 PLATFORM_GPIO_CONTROLLER\n, __func__));
   return EFI_BAD_BUFFER_SIZE;
 }

 mPL061PlatformGpio-GpioCount = PL061_GPIO_PINS;
 mPL061PlatformGpio-GpioControllerCount = 1;
 mPL061PlatformGpio-GpioControllerSize = sizeof (GPIO_CONTROLLER);

[edk2] [PATCH 3/3] ArmPlatformPkg: PL061: support multiple controller

2015-08-18 Thread Haojian Zhuang 
Support multiple PL061 controllers.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang haojian.zhu...@linaro.org
---
 ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c| 107 ++---
 .../Drivers/PL061GpioDxe/PL061GpioDxe.inf  |   3 +-
 ArmPlatformPkg/Include/Drivers/PL061Gpio.h |  40 
 3 files changed, 96 insertions(+), 54 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c 
b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
index e8a2094..3027656 100644
--- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
+++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
@@ -28,6 +28,7 @@
 #include Drivers/PL061Gpio.h
 
 BOOLEAN mPL061Initialized = FALSE;
+PLATFORM_GPIO_CONTROLLER *mPL061PlatformGpio;
 
 /**
   Function implementations
@@ -38,20 +39,31 @@ PL061Identify (
   VOID
   )
 {
-  // Check if this is a PrimeCell Peripheral
-  if ((MmioRead8 (PL061_GPIO_PCELL_ID0) != 0x0D)
-  ||  (MmioRead8 (PL061_GPIO_PCELL_ID1) != 0xF0)
-  ||  (MmioRead8 (PL061_GPIO_PCELL_ID2) != 0x05)
-  ||  (MmioRead8 (PL061_GPIO_PCELL_ID3) != 0xB1)) {
-return EFI_NOT_FOUND;
+  UINTNIndex;
+  UINT32   RegisterBase;
+
+  if (   (mPL061PlatformGpio-GpioCount == 0)
+  || (mPL061PlatformGpio-GpioControllerCount == 0)) {
+ return EFI_NOT_FOUND;
   }
 
-  // Check if this PrimeCell Peripheral is the PL061 GPIO
-  if ((MmioRead8 (PL061_GPIO_PERIPH_ID0) != 0x61)
-  ||  (MmioRead8 (PL061_GPIO_PERIPH_ID1) != 0x10)
-  ||  ((MmioRead8 (PL061_GPIO_PERIPH_ID2)  0xF) != 0x04)
-  ||  (MmioRead8 (PL061_GPIO_PERIPH_ID3) != 0x00)) {
-return EFI_NOT_FOUND;
+  for (Index = 0; Index  mPL061PlatformGpio-GpioControllerCount; Index++) {
+RegisterBase = (UINT32) 
mPL061PlatformGpio-GpioController[Index].RegisterBase;
+// Check if this is a PrimeCell Peripheral
+if ((MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID0) != 0x0D)
+||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID1) != 0xF0)
+||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID2) != 0x05)
+||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID3) != 0xB1)) {
+  return EFI_NOT_FOUND;
+}
+   
+// Check if this PrimeCell Peripheral is the PL061 GPIO
+if ((MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID0) != 0x61)
+||  (MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID1) != 0x10)
+||  ((MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID2)  0xF) != 0x04)
+||  (MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID3) != 0x00)) {
+  return EFI_NOT_FOUND;
+}
   }
 
   return EFI_SUCCESS;
@@ -84,6 +96,30 @@ PL061Initialize (
   return Status;
 }
 
+EFI_STATUS
+EFIAPI
+PL061Locate (
+  IN  EMBEDDED_GPIO_PIN Gpio,
+  OUT UINT32*ControllerIndex,
+  OUT UINT32*ControllerOffset,
+  OUT UINT32*RegisterBase
+  )
+{
+  UINT32Index;
+
+  for (Index = 0; Index  mPL061PlatformGpio-GpioControllerCount; Index++) {
+if ((Gpio = mPL061PlatformGpio-GpioController[Index].GpioIndex)
+  (Gpio  mPL061PlatformGpio-GpioController[Index].GpioIndex
+ + mPL061PlatformGpio-GpioController[Index].InternalGpioCount)) {
+  *ControllerIndex = Index;
+  *ControllerOffset = Gpio % 
mPL061PlatformGpio-GpioController[Index].InternalGpioCount;
+  *RegisterBase = mPL061PlatformGpio-GpioController[Index].RegisterBase;
+  return EFI_SUCCESS;
+}
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
 /**
 
 Routine Description:
@@ -110,11 +146,15 @@ Get (
   )
 {
   EFI_STATUSStatus = EFI_SUCCESS;
+  UINT32Index, Offset, RegisterBase;
 
-  if ((Value == NULL)
-  ||  (Gpio  LAST_GPIO_PIN))
-  {
-return EFI_INVALID_PARAMETER;
+  Status = PL061Locate (Gpio, Index, Offset, RegisterBase);
+  if (EFI_ERROR (Status))
+goto EXIT;
+
+  if (Value == NULL) {
+Status = EFI_INVALID_PARAMETER;
+goto EXIT;
   }
 
   // Initialize the hardware if not already done
@@ -125,7 +165,7 @@ Get (
 }
   }
 
-  if (MmioRead8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio)  2))) {
+  if (MmioRead8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) 
 2))) {
 *Value = 1;
   } else {
 *Value = 0;
@@ -162,12 +202,11 @@ Set (
   )
 {
   EFI_STATUSStatus = EFI_SUCCESS;
+  UINT32Index, Offset, RegisterBase;
 
-  // Check for errors
-  if (Gpio  LAST_GPIO_PIN) {
-Status = EFI_INVALID_PARAMETER;
+  Status = PL061Locate (Gpio, Index, Offset, RegisterBase);
+  if (EFI_ERROR (Status))
 goto EXIT;
-  }
 
   // Initialize the hardware if not already done
   if (!mPL061Initialized) {
@@ -181,21 +220,21 @@ Set (
   {
 case GPIO_MODE_INPUT:
   // Set the corresponding direction bit to LOW for input
-  MmioAnd8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+  MmioAnd8 (RegisterBase + PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Offset));
   break;
 
 case GPIO_MODE_OUTPUT_0:
   // Set the corresponding data bit to LOW

Re: [edk2] [PATCH 3/3] ArmPlatformPkg: PL061: support multiple controller

2015-08-18 Thread Ard Biesheuvel 
On 18 August 2015 at 11:04, Haojian Zhuang haojian.zhu...@linaro.org wrote:
 Support multiple PL061 controllers.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Haojian Zhuang haojian.zhu...@linaro.org
 ---
  ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c| 107 
 ++---
  .../Drivers/PL061GpioDxe/PL061GpioDxe.inf  |   3 +-
  ArmPlatformPkg/Include/Drivers/PL061Gpio.h |  40 
  3 files changed, 96 insertions(+), 54 deletions(-)

 diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c 
 b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
 index e8a2094..3027656 100644
 --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
 +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
 @@ -28,6 +28,7 @@
  #include Drivers/PL061Gpio.h

  BOOLEAN mPL061Initialized = FALSE;
 +PLATFORM_GPIO_CONTROLLER *mPL061PlatformGpio;

  /**
Function implementations
 @@ -38,20 +39,31 @@ PL061Identify (
VOID
)
  {
 -  // Check if this is a PrimeCell Peripheral
 -  if ((MmioRead8 (PL061_GPIO_PCELL_ID0) != 0x0D)
 -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID1) != 0xF0)
 -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID2) != 0x05)
 -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID3) != 0xB1)) {
 -return EFI_NOT_FOUND;
 +  UINTNIndex;
 +  UINT32   RegisterBase;

Why is this a 32-bit value?

 +
 +  if (   (mPL061PlatformGpio-GpioCount == 0)
 +  || (mPL061PlatformGpio-GpioControllerCount == 0)) {
 + return EFI_NOT_FOUND;
}

 -  // Check if this PrimeCell Peripheral is the PL061 GPIO
 -  if ((MmioRead8 (PL061_GPIO_PERIPH_ID0) != 0x61)
 -  ||  (MmioRead8 (PL061_GPIO_PERIPH_ID1) != 0x10)
 -  ||  ((MmioRead8 (PL061_GPIO_PERIPH_ID2)  0xF) != 0x04)
 -  ||  (MmioRead8 (PL061_GPIO_PERIPH_ID3) != 0x00)) {
 -return EFI_NOT_FOUND;
 +  for (Index = 0; Index  mPL061PlatformGpio-GpioControllerCount; Index++) {
 +RegisterBase = (UINT32) 
 mPL061PlatformGpio-GpioController[Index].RegisterBase;
 +// Check if this is a PrimeCell Peripheral
 +if ((MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID0) != 0x0D)
 +||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID1) != 0xF0)
 +||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID2) != 0x05)
 +||  (MmioRead8 (RegisterBase + PL061_GPIO_PCELL_ID3) != 0xB1)) {
 +  return EFI_NOT_FOUND;
 +}
 +
 +// Check if this PrimeCell Peripheral is the PL061 GPIO
 +if ((MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID0) != 0x61)
 +||  (MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID1) != 0x10)
 +||  ((MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID2)  0xF) != 
 0x04)
 +||  (MmioRead8 (RegisterBase + PL061_GPIO_PERIPH_ID3) != 0x00)) {
 +  return EFI_NOT_FOUND;
 +}
}

return EFI_SUCCESS;
 @@ -84,6 +96,30 @@ PL061Initialize (
return Status;
  }

 +EFI_STATUS
 +EFIAPI
 +PL061Locate (
 +  IN  EMBEDDED_GPIO_PIN Gpio,
 +  OUT UINT32*ControllerIndex,
 +  OUT UINT32*ControllerOffset,
 +  OUT UINT32*RegisterBase
 +  )
 +{
 +  UINT32Index;
 +
 +  for (Index = 0; Index  mPL061PlatformGpio-GpioControllerCount; Index++) {
 +if ((Gpio = mPL061PlatformGpio-GpioController[Index].GpioIndex)
 +  (Gpio  mPL061PlatformGpio-GpioController[Index].GpioIndex
 + + mPL061PlatformGpio-GpioController[Index].InternalGpioCount)) 
 {
 +  *ControllerIndex = Index;
 +  *ControllerOffset = Gpio % 
 mPL061PlatformGpio-GpioController[Index].InternalGpioCount;

Since you are relying on the internal GPIO count to be the same for
all instances, you should probably check that the value is correct in
PL061Identify()

 +  *RegisterBase = mPL061PlatformGpio-GpioController[Index].RegisterBase;
 +  return EFI_SUCCESS;
 +}
 +  }
 +  return EFI_INVALID_PARAMETER;
 +}
 +
  /**

  Routine Description:
 @@ -110,11 +146,15 @@ Get (
)
  {
EFI_STATUSStatus = EFI_SUCCESS;
 +  UINT32Index, Offset, RegisterBase;

 -  if ((Value == NULL)
 -  ||  (Gpio  LAST_GPIO_PIN))
 -  {
 -return EFI_INVALID_PARAMETER;
 +  Status = PL061Locate (Gpio, Index, Offset, RegisterBase);
 +  if (EFI_ERROR (Status))
 +goto EXIT;
 +
 +  if (Value == NULL) {
 +Status = EFI_INVALID_PARAMETER;
 +goto EXIT;
}

// Initialize the hardware if not already done
 @@ -125,7 +165,7 @@ Get (
  }
}

 -  if (MmioRead8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio)  2))) {
 +  if (MmioRead8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) 
  2))) {
  *Value = 1;
} else {
  *Value = 0;
 @@ -162,12 +202,11 @@ Set (
)
  {
EFI_STATUSStatus = EFI_SUCCESS;
 +  UINT32Index, Offset, RegisterBase;

 -  // Check for errors
 -  if (Gpio  LAST_GPIO_PIN) {
 -Status = EFI_INVALID_PARAMETER;
 +  Status = PL061Locate (Gpio, Index, Offset, RegisterBase);
 +  if (EFI_ERROR (Status))
  goto EXIT;
 -  }

// Initialize the hardware if not already

Re: [edk2] [PATCH 3/3] ArmPlatformPkg: PL061: support multiple controller

2015-08-18 Thread Ard Biesheuvel 
On 18 August 2015 at 11:31, Haojian Zhuang haojian.zhu...@linaro.org wrote:
 On Tue, 2015-08-18 at 11:22 +0200, Ard Biesheuvel wrote:
 On 18 August 2015 at 11:04, Haojian Zhuang haojian.zhu...@linaro.org wrote:
  Support multiple PL061 controllers.
 
  Contributed-under: TianoCore Contribution Agreement 1.0
  Signed-off-by: Haojian Zhuang haojian.zhu...@linaro.org
  ---
   ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c| 107 
  ++---
   .../Drivers/PL061GpioDxe/PL061GpioDxe.inf  |   3 +-
   ArmPlatformPkg/Include/Drivers/PL061Gpio.h |  40 
   3 files changed, 96 insertions(+), 54 deletions(-)
 
  diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c 
  b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
  index e8a2094..3027656 100644
  --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
  +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
  @@ -28,6 +28,7 @@
   #include Drivers/PL061Gpio.h
 
   BOOLEAN mPL061Initialized = FALSE;
  +PLATFORM_GPIO_CONTROLLER *mPL061PlatformGpio;
 
   /**
 Function implementations
  @@ -38,20 +39,31 @@ PL061Identify (
 VOID
 )
   {
  -  // Check if this is a PrimeCell Peripheral
  -  if ((MmioRead8 (PL061_GPIO_PCELL_ID0) != 0x0D)
  -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID1) != 0xF0)
  -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID2) != 0x05)
  -  ||  (MmioRead8 (PL061_GPIO_PCELL_ID3) != 0xB1)) {
  -return EFI_NOT_FOUND;
  +  UINTNIndex;
  +  UINT32   RegisterBase;

 Why is this a 32-bit value?

 All registers in PL061 are 32-bit. So we don't need to define UINTN at
 here.


So what happens if the GPIO controller is mapped above 4 GB in physical memory?

  +EFI_STATUS
  +EFIAPI
  +PL061Locate (
  +  IN  EMBEDDED_GPIO_PIN Gpio,
  +  OUT UINT32*ControllerIndex,
  +  OUT UINT32*ControllerOffset,
  +  OUT UINT32*RegisterBase
  +  )
  +{
  +  UINT32Index;
  +
  +  for (Index = 0; Index  mPL061PlatformGpio-GpioControllerCount; 
  Index++) {
  +if ((Gpio = mPL061PlatformGpio-GpioController[Index].GpioIndex)
  +  (Gpio  mPL061PlatformGpio-GpioController[Index].GpioIndex
  + + 
  mPL061PlatformGpio-GpioController[Index].InternalGpioCount)) {
  +  *ControllerIndex = Index;
  +  *ControllerOffset = Gpio % 
  mPL061PlatformGpio-GpioController[Index].InternalGpioCount;

 Since you are relying on the internal GPIO count to be the same for
 all instances, you should probably check that the value is correct in
 PL061Identify()

 OK. I'll check whether it's 8 at here.

  @@ -329,6 +367,9 @@ PL061InstallProtocol (
 //
 ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, gEmbeddedGpioProtocolGuid);
 
  +  Status = gBS-LocateProtocol (gPlatformGpioProtocolGuid, NULL, (VOID 
  **)mPL061PlatformGpio);
  +  ASSERT_EFI_ERROR (Status);
  +

 We should fall back to using the PCD if the protocol cannot be found.
 This way, we won't break existing users.


 I was intended to use fallback. But I think it's not good enough.

 1. If anyone defines the PCD register base in dec file. The fallback
 mechanism will be broken.


Well, you would only look at the PCD if the protocol is not installed,
so I don't see how that would change anything compared to the current
situation.

 2. Since every driver is built as module, we must create the driver
 dependency.


Please try and use a BEFORE ... depex instead, you can put it in your
platform GPIO DXE with the file GUID of the PL061 DXE driver.

 // Install the Embedded GPIO Protocol onto a new handle
 Handle = NULL;
 Status = gBS-InstallMultipleProtocolInterfaces(
  diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf 
  b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
  index 9d9e4cd..971452c 100644
  --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
  +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
  @@ -45,6 +45,7 @@
 
   [Protocols]
 gEmbeddedGpioProtocolGuid
  +  gPlatformGpioProtocolGuid
 
   [Depex]
  -  TRUE
  +  gPlatformGpioProtocolGuid

 Likewise, this breaks existing users

 Withouth this dependency, LocateProtocol() will be invoked before
 InstallMultipleProtocols(). So I have to add the dependency at here.


Where is the InstallMultipleProtocols() call? Are you going to post it as well?

 I can't find that any driver is using PL061 in edk2 code repository. So
 I can't update the code to reference PL061 driver.


Well, that is not entirely the point. First of all, it may be used
outside of the edk2 tree. But we should also not complicate the common
case by forcing everyone to use the multiple GPIO protocol and install
it programmatically rather than simply setting a PCD

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