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>

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

diff --git a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
index 38458f484476360f2ad31644f493e53f342d9289..8fde2bb5efe71f77e349ccb54d192a501691d86c 100644 (file)
--- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
+++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
@@ -19,6 +19,7 @@
 #include <Protocol/EmbeddedGpio.h>\r
 \r
 // PL061 GPIO Registers\r
+#define PL061_GPIO_DATA_REG_OFFSET      ((UINTN) 0x000)\r
 #define PL061_GPIO_DATA_REG             ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x000)\r
 #define PL061_GPIO_DIR_REG              ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x400)\r
 #define PL061_GPIO_IS_REG               ((UINT32)PcdGet32 (PcdPL061GpioBase) + 0x404)\r
@@ -46,9 +47,5 @@
 \r
 // All bits low except one bit high, native bit length\r
 #define GPIO_PIN_MASK(Pin)              (1UL << ((UINTN)(Pin)))\r
-// All bits low except one bit high, restricted to 8 bits (i.e. ensures zeros above 8bits)\r
-#define GPIO_PIN_MASK_HIGH_8BIT(Pin)    (GPIO_PIN_MASK(Pin) && 0xFF)\r
-// All bits high except one bit low, restricted to 8 bits (i.e. ensures zeros above 8bits)\r
-#define GPIO_PIN_MASK_LOW_8BIT(Pin)     ((~GPIO_PIN_MASK(Pin)) && 0xFF)\r
 \r
 #endif  // __PL061_GPIO_H__\r