Discussion:
[edk2] [PATCH 3/3] ArmPlatformPkg: PL061: support multiple controller
Haojian Zhuang
2015-08-18 09:04:52 UTC
Permalink
Support multiple PL061 controllers.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Haojian Zhuang <***@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;
+ UINTN Index;
+ 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
+ )
+{
+ UINT32 Index;
+
+ 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_STATUS Status = EFI_SUCCESS;
+ UINT32 Index, 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_STATUS Status = EFI_SUCCESS;
+ UINT32 Index, 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 for 0
- MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), GPIO_PIN_MASK(Gpio));
+ MmioWrite8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) << 2), 0);
// Set the corresponding direction bit to HIGH for output
- MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+ MmioOr8 (RegisterBase + PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Offset));
break;

case GPIO_MODE_OUTPUT_1:
// Set the corresponding data bit to HIGH for 1
- MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), GPIO_PIN_MASK(Gpio));
+ MmioWrite8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) << 2), 0xff);
// Set the corresponding direction bit to HIGH for output
- MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+ MmioOr8 (RegisterBase + PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Offset));
break;

default:
@@ -234,12 +273,11 @@ GetMode (
)
{
EFI_STATUS Status;
+ UINT32 Index, Offset, RegisterBase;

- // Check for errors
- if ( (Mode == NULL)
- || (Gpio > LAST_GPIO_PIN)) {
- return EFI_INVALID_PARAMETER;
- }
+ Status = PL061Locate (Gpio, &Index, &Offset, &RegisterBase);
+ if (EFI_ERROR (Status))
+ return Status;

// Initialize the hardware if not already done
if (!mPL061Initialized) {
@@ -250,9 +288,9 @@ GetMode (
}

// Check if it is input or output
- if (MmioRead8 (PL061_GPIO_DIR_REG + (GPIO_PIN_MASK(Gpio) << 2))) {
+ if (MmioRead8 (RegisterBase + PL061_GPIO_DIR_REG) & GPIO_PIN_MASK(Offset)) {
// Pin set to output
- if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK(Gpio)) {
+ if (MmioRead8 (RegisterBase + PL061_GPIO_DATA_REG) & GPIO_PIN_MASK(Offset)) {
*Mode = GPIO_MODE_OUTPUT_1;
} else {
*Mode = GPIO_MODE_OUTPUT_0;
@@ -329,6 +367,9 @@ PL061InstallProtocol (
//
ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEmbeddedGpioProtocolGuid);

+ Status = gBS->LocateProtocol (&gPlatformGpioProtocolGuid, NULL, (VOID **)&mPL061PlatformGpio);
+ ASSERT_EFI_ERROR (Status);
+
// 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
diff --git a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
index d436fd4..98d7bc2 100644
--- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
+++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
@@ -19,26 +19,26 @@
#include <Protocol/EmbeddedGpio.h>

// PL061 GPIO Registers
-#define PL061_GPIO_DATA_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x000)
-#define PL061_GPIO_DIR_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x400)
-#define PL061_GPIO_IS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x404)
-#define PL061_GPIO_IBE_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x408)
-#define PL061_GPIO_IEV_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x40C)
-#define PL061_GPIO_IE_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x410)
-#define PL061_GPIO_RIS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x414)
-#define PL061_GPIO_MIS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x410)
-#define PL061_GPIO_IC_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x41C)
-#define PL061_GPIO_AFSEL_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x420)
-
-#define PL061_GPIO_PERIPH_ID0 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE0)
-#define PL061_GPIO_PERIPH_ID1 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE4)
-#define PL061_GPIO_PERIPH_ID2 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE8)
-#define PL061_GPIO_PERIPH_ID3 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFEC)
-
-#define PL061_GPIO_PCELL_ID0 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF0)
-#define PL061_GPIO_PCELL_ID1 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF4)
-#define PL061_GPIO_PCELL_ID2 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF8)
-#define PL061_GPIO_PCELL_ID3 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFFC)
+#define PL061_GPIO_DATA_REG 0x000
+#define PL061_GPIO_DIR_REG 0x400
+#define PL061_GPIO_IS_REG 0x404
+#define PL061_GPIO_IBE_REG 0x408
+#define PL061_GPIO_IEV_REG 0x40C
+#define PL061_GPIO_IE_REG 0x410
+#define PL061_GPIO_RIS_REG 0x414
+#define PL061_GPIO_MIS_REG 0x410
+#define PL061_GPIO_IC_REG 0x41C
+#define PL061_GPIO_AFSEL_REG 0x420
+
+#define PL061_GPIO_PERIPH_ID0 0xFE0
+#define PL061_GPIO_PERIPH_ID1 0xFE4
+#define PL061_GPIO_PERIPH_ID2 0xFE8
+#define PL061_GPIO_PERIPH_ID3 0xFEC
+
+#define PL061_GPIO_PCELL_ID0 0xFF0
+#define PL061_GPIO_PCELL_ID1 0xFF4
+#define PL061_GPIO_PCELL_ID2 0xFF8
+#define PL061_GPIO_PCELL_ID3 0xFFC


// GPIO pins are numbered 0..7
--
2.1.4


------------------------------------------------------------------------------
Ard Biesheuvel
2015-08-18 09:22:30 UTC
Permalink
Post by Haojian Zhuang
Support multiple PL061 controllers.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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;
+ UINTN Index;
+ UINT32 RegisterBase;
Why is this a 32-bit value?
Post by Haojian Zhuang
+
+ 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
+ )
+{
+ UINT32 Index;
+
+ 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()
Post by Haojian Zhuang
+ *RegisterBase = mPL061PlatformGpio->GpioController[Index].RegisterBase;
+ return EFI_SUCCESS;
+ }
+ }
+ return EFI_INVALID_PARAMETER;
+}
+
/**
@@ -110,11 +146,15 @@ Get (
)
{
EFI_STATUS Status = EFI_SUCCESS;
+ UINT32 Index, 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_STATUS Status = EFI_SUCCESS;
+ UINT32 Index, 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 (
{
// 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;
// Set the corresponding data bit to LOW for 0
- MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), GPIO_PIN_MASK(Gpio));
+ MmioWrite8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) << 2), 0);
// Set the corresponding direction bit to HIGH for output
- MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+ MmioOr8 (RegisterBase + PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Offset));
break;
// Set the corresponding data bit to HIGH for 1
- MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), GPIO_PIN_MASK(Gpio));
+ MmioWrite8 (RegisterBase + PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Offset) << 2), 0xff);
// Set the corresponding direction bit to HIGH for output
- MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+ MmioOr8 (RegisterBase + PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Offset));
break;
@@ -234,12 +273,11 @@ GetMode (
)
{
EFI_STATUS Status;
+ UINT32 Index, Offset, RegisterBase;
- // Check for errors
- if ( (Mode == NULL)
- || (Gpio > LAST_GPIO_PIN)) {
- return EFI_INVALID_PARAMETER;
- }
+ Status = PL061Locate (Gpio, &Index, &Offset, &RegisterBase);
+ if (EFI_ERROR (Status))
+ return Status;
// Initialize the hardware if not already done
if (!mPL061Initialized) {
@@ -250,9 +288,9 @@ GetMode (
}
// Check if it is input or output
- if (MmioRead8 (PL061_GPIO_DIR_REG + (GPIO_PIN_MASK(Gpio) << 2))) {
+ if (MmioRead8 (RegisterBase + PL061_GPIO_DIR_REG) & GPIO_PIN_MASK(Offset)) {
// Pin set to output
- if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK(Gpio)) {
+ if (MmioRead8 (RegisterBase + PL061_GPIO_DATA_REG) & GPIO_PIN_MASK(Offset)) {
*Mode = GPIO_MODE_OUTPUT_1;
} else {
*Mode = GPIO_MODE_OUTPUT_0;
@@ -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.
Post by Haojian Zhuang
// 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
Post by Haojian Zhuang
diff --git a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
index d436fd4..98d7bc2 100644
--- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
+++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
@@ -19,26 +19,26 @@
#include <Protocol/EmbeddedGpio.h>
// PL061 GPIO Registers
-#define PL061_GPIO_DATA_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x000)
-#define PL061_GPIO_DIR_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x400)
-#define PL061_GPIO_IS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x404)
-#define PL061_GPIO_IBE_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x408)
-#define PL061_GPIO_IEV_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x40C)
-#define PL061_GPIO_IE_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x410)
-#define PL061_GPIO_RIS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x414)
-#define PL061_GPIO_MIS_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x410)
-#define PL061_GPIO_IC_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x41C)
-#define PL061_GPIO_AFSEL_REG ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x420)
-
-#define PL061_GPIO_PERIPH_ID0 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE0)
-#define PL061_GPIO_PERIPH_ID1 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE4)
-#define PL061_GPIO_PERIPH_ID2 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFE8)
-#define PL061_GPIO_PERIPH_ID3 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFEC)
-
-#define PL061_GPIO_PCELL_ID0 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF0)
-#define PL061_GPIO_PCELL_ID1 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF4)
-#define PL061_GPIO_PCELL_ID2 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFF8)
-#define PL061_GPIO_PCELL_ID3 ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0xFFC)
+#define PL061_GPIO_DATA_REG 0x000
+#define PL061_GPIO_DIR_REG 0x400
+#define PL061_GPIO_IS_REG 0x404
+#define PL061_GPIO_IBE_REG 0x408
+#define PL061_GPIO_IEV_REG 0x40C
+#define PL061_GPIO_IE_REG 0x410
+#define PL061_GPIO_RIS_REG 0x414
+#define PL061_GPIO_MIS_REG 0x410
+#define PL061_GPIO_IC_REG 0x41C
+#define PL061_GPIO_AFSEL_REG 0x420
+
+#define PL061_GPIO_PERIPH_ID0 0xFE0
+#define PL061_GPIO_PERIPH_ID1 0xFE4
+#define PL061_GPIO_PERIPH_ID2 0xFE8
+#define PL061_GPIO_PERIPH_ID3 0xFEC
+
+#define PL061_GPIO_PCELL_ID0 0xFF0
+#define PL061_GPIO_PCELL_ID1 0xFF4
+#define PL061_GPIO_PCELL_ID2 0xFF8
+#define PL061_GPIO_PCELL_ID3 0xFFC
// GPIO pins are numbered 0..7
--
2.1.4
------------------------------------------------------------------------------
Haojian Zhuang
2015-08-18 09:31:11 UTC
Permalink
Post by Ard Biesheuvel
Post by Haojian Zhuang
Support multiple PL061 controllers.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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;
+ UINTN Index;
+ 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.
Post by Ard Biesheuvel
Post by Haojian Zhuang
+EFI_STATUS
+EFIAPI
+PL061Locate (
+ IN EMBEDDED_GPIO_PIN Gpio,
+ OUT UINT32 *ControllerIndex,
+ OUT UINT32 *ControllerOffset,
+ OUT UINT32 *RegisterBase
+ )
+{
+ UINT32 Index;
+
+ 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.
Post by Ard Biesheuvel
Post by Haojian Zhuang
@@ -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.

2. Since every driver is built as module, we must create the driver
dependency.
Post by Ard Biesheuvel
Post by Haojian Zhuang
// 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.

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.

Regards
Haojian


------------------------------------------------------------------------------
Ard Biesheuvel
2015-08-18 09:43:29 UTC
Permalink
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
Support multiple PL061 controllers.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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;
+ UINTN Index;
+ 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?
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
+EFI_STATUS
+EFIAPI
+PL061Locate (
+ IN EMBEDDED_GPIO_PIN Gpio,
+ OUT UINT32 *ControllerIndex,
+ OUT UINT32 *ControllerOffset,
+ OUT UINT32 *RegisterBase
+ )
+{
+ UINT32 Index;
+
+ 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.
Post by Ard Biesheuvel
Post by Haojian Zhuang
@@ -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.
Post by Haojian Zhuang
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.
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
// 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?
Post by Haojian Zhuang
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.

------------------------------------------------------------------------------
Haojian Zhuang
2015-08-18 12:30:23 UTC
Permalink
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
Support multiple PL061 controllers.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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;
+ UINTN Index;
+ 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)
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
@@ -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.
Post by Haojian Zhuang
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

And here's updated PL061 driver.

PL061InstallProtocol (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
EFI_HANDLE Handle;
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);
mPL061PlatformGpio->GpioController = (GPIO_CONTROLLER *)((UINTN)
mPL061PlatformGpio + sizeof (PLATFORM_GPIO_CONTROLLER));

GpioController = mPL061PlatformGpio->GpioController;
GpioController->RegisterBase = (UINTN) PcdGet32 (PcdPL061GpioBase);
GpioController->GpioIndex = 0;
GpioController->InternalGpioCount = PL061_GPIO_PINS;
}
...
}

The log of uefi is in below.

add-symbol-file /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
CH64/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe/DEBUG/PL061GpioDxe.dll 0x3D9B3260
Loading driver at 0x0003D9B3000 EntryPoint=0x0003D9B32A8
PL061GpioDxe.efi
#PL061InstallProtocol, 377, Status:Not
Found

add-symbol-file /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
CH64/MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe/DEBUG/En
glishDxe.dll
0x3D89E260
Loading driver at 0x0003D89E000 EntryPoint=0x0003D89E2A8
EnglishDxe.efi
Driver B51A851C-7BF7-463F-B261-CFB158B7F699 was discovered but not
loaded!!
HiKeyDetectJumper: failed to set jumper as gpio input

It seems that "BEFORE" can't make platform gpio driver loaded just
before PL061. It's not loaded at all.
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
// 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 didn't post it since it's in the platform gpio driver. And the
platform isn't created in edk2 code repository yet.



------------------------------------------------------------------------------
Ard Biesheuvel
2015-08-18 12:37:30 UTC
Permalink
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
Support multiple PL061 controllers.
Contributed-under: TianoCore Contribution Agreement 1.0
---
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;
+ UINTN Index;
+ 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
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
@@ -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.
Post by Haojian Zhuang
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
Post by Haojian Zhuang
And here's updated PL061 driver.
PL061InstallProtocol (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
EFI_HANDLE Handle;
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);
mPL061PlatformGpio->GpioController = (GPIO_CONTROLLER *)((UINTN)
mPL061PlatformGpio + sizeof (PLATFORM_GPIO_CONTROLLER));
GpioController = mPL061PlatformGpio->GpioController;
GpioController->RegisterBase = (UINTN) PcdGet32 (PcdPL061GpioBase);
GpioController->GpioIndex = 0;
GpioController->InternalGpioCount = PL061_GPIO_PINS;
}
...
}
The log of uefi is in below.
add-symbol-file /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
CH64/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe/DEBUG/PL061GpioDxe.dll 0x3D9B3260
Loading driver at 0x0003D9B3000 EntryPoint=0x0003D9B32A8
PL061GpioDxe.efi
#PL061InstallProtocol, 377, Status:Not
Found
add-symbol-file /opt/workspace/boot/uefi/linaro-edk2/Build/HiKey/DEBUG_GCC49/AAR
CH64/MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe/DEBUG/En
glishDxe.dll
0x3D89E260
Loading driver at 0x0003D89E000 EntryPoint=0x0003D89E2A8
EnglishDxe.efi
Driver B51A851C-7BF7-463F-B261-CFB158B7F699 was discovered but not
loaded!!
HiKeyDetectJumper: failed to set jumper as gpio input
It seems that "BEFORE" can't make platform gpio driver loaded just
before PL061. It's not loaded at all.
Post by Ard Biesheuvel
Post by Haojian Zhuang
Post by Ard Biesheuvel
Post by Haojian Zhuang
// 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 didn't post it since it's in the platform gpio driver. And the
platform isn't created in edk2 code repository yet.
OK that is fine. I was just curious

------------------------------------------------------------------------------
Loading...