From 328d8cfa6278a5558ce510662df73f4c17086567 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Wed, 28 Oct 2015 18:11:49 +0000
Subject: [PATCH] ArmPlatformPkg: PL061 - rewrite the hardware interaction

The PL061 GPIO controller is a bit of an anachronism, and the existing
driver does nothing to hide this - leading to it being very tricky to
read.

Rewrite it to document (in comments and code) what is actually
happening, and fix some bugs in the process.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Ryan Harkin <ryan.harkin@linaro.org>
---
 .../Drivers/PL061GpioDxe/PL061Gpio.c          | 72 ++++++++++++++++---
 ArmPlatformPkg/Include/Drivers/PL061Gpio.h    |  5 +-
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
index 45897ca753..38341a3d05 100644
--- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
+++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
@@ -28,6 +28,56 @@
 #include <Protocol/EmbeddedGpio.h>
 #include <Drivers/PL061Gpio.h>
 
+//
+// The PL061 is a strange beast. The 8-bit data register is aliased across a
+// region 0x400 bytes in size, with bits [9:2] of the address operating as a
+// mask for both read and write operations:
+// For reads:
+//   - All bits where their corresponding mask bit is 1 return the current
+//     value of that bit in the GPIO_DATA register.
+//   - All bits where their corresponding mask bit is 0 return 0.
+// For writes:
+//   - All bits where their corresponding mask bit is 1 set the bit in the
+//     GPIO_DATA register to the written value.
+//   - All bits where their corresponding mask bit is 0 are left untouched
+//     in the GPIO_DATA register.
+//
+// To keep this driver intelligible, PL061EffectiveAddress, PL061GetPins and
+// Pl061SetPins provide an internal abstraction from this interface.
+
+STATIC
+UINTN
+EFIAPI
+PL061EffectiveAddress (
+  IN UINTN Address,
+  IN UINTN Mask
+  )
+{
+  return ((Address + PL061_GPIO_DATA_REG_OFFSET) + (Mask << 2));
+}
+
+STATIC
+UINTN
+EFIAPI
+PL061GetPins (
+  IN UINTN Address,
+  IN UINTN Mask
+  )
+{
+  return MmioRead8 (PL061EffectiveAddress (Address, Mask));
+}
+
+STATIC
+VOID
+EFIAPI
+PL061SetPins (
+  IN UINTN Address,
+  IN UINTN Mask,
+  IN UINTN Value
+  )
+{
+  MmioWrite8 (PL061EffectiveAddress (Address, Mask), Value);
+}
 
 /**
   Function implementations
@@ -88,7 +138,7 @@ Get (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) {
+  if (PL061GetPins (PL061_GPIO_DATA_REG, Gpio)) {
     *Value = 1;
   } else {
     *Value = 0;
@@ -135,22 +185,22 @@ Set (
   {
     case GPIO_MODE_INPUT:
       // Set the corresponding direction bit to LOW for input
-      MmioAnd8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK_LOW_8BIT(Gpio));
+      MmioAnd8 (PL061_GPIO_DIR_REG, ~GPIO_PIN_MASK(Gpio) & 0xFF);
       break;
 
     case GPIO_MODE_OUTPUT_0:
-      // Set the corresponding data bit to LOW for 0
-      MmioAnd8 (PL061_GPIO_DATA_REG, GPIO_PIN_MASK_LOW_8BIT(Gpio));
       // Set the corresponding direction bit to HIGH for output
-      MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK_HIGH_8BIT(Gpio));
+      MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+      // Set the corresponding data bit to LOW for 0
+      PL061SetPins (PL061_GPIO_DATA_REG, GPIO_PIN_MASK(Gpio), 0);
       break;
 
     case GPIO_MODE_OUTPUT_1:
-      // Set the corresponding data bit to HIGH for 1
-      MmioOr8 (PL061_GPIO_DATA_REG, GPIO_PIN_MASK_HIGH_8BIT(Gpio));
       // Set the corresponding direction bit to HIGH for output
-      MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK_HIGH_8BIT(Gpio));
-      break;
+      MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK(Gpio));
+      // Set the corresponding data bit to HIGH for 1
+      PL061SetPins (PL061_GPIO_DATA_REG, GPIO_PIN_MASK(Gpio), 0xff);
+    break;
 
     default:
       // Other modes are not supported
@@ -194,9 +244,9 @@ GetMode (
   }
 
   // Check if it is input or output
-  if (MmioRead8 (PL061_GPIO_DIR_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) {
+  if (MmioRead8 (PL061_GPIO_DIR_REG) & GPIO_PIN_MASK(Gpio)) {
     // Pin set to output
-    if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) {
+    if (PL061GetPins (PL061_GPIO_DATA_REG, GPIO_PIN_MASK(Gpio))) {
       *Mode = GPIO_MODE_OUTPUT_1;
     } else {
       *Mode = GPIO_MODE_OUTPUT_0;
diff --git a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
index 38458f4844..8fde2bb5ef 100644
--- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
+++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
@@ -19,6 +19,7 @@
 #include <Protocol/EmbeddedGpio.h>
 
 // PL061 GPIO Registers
+#define PL061_GPIO_DATA_REG_OFFSET      ((UINTN) 0x000)
 #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)
@@ -46,9 +47,5 @@
 
 // All bits low except one bit high, native bit length
 #define GPIO_PIN_MASK(Pin)              (1UL << ((UINTN)(Pin)))
-// All bits low except one bit high, restricted to 8 bits (i.e. ensures zeros above 8bits)
-#define GPIO_PIN_MASK_HIGH_8BIT(Pin)    (GPIO_PIN_MASK(Pin) && 0xFF)
-// All bits high except one bit low, restricted to 8 bits (i.e. ensures zeros above 8bits)
-#define GPIO_PIN_MASK_LOW_8BIT(Pin)     ((~GPIO_PIN_MASK(Pin)) && 0xFF)
 
 #endif  // __PL061_GPIO_H__