On 10 February 2016 at 16:55, Leif Lindholm <[email protected]> wrote:
> 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 <[email protected]>
> ---
> ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c | 70
> +++++++++++++++++++++----
> ArmPlatformPkg/Include/Drivers/PL061Gpio.h | 6 +--
> 2 files changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> index 7fa9ab6..4bfb8de 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
> @@ -113,7 +163,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;
> @@ -160,21 +210,21 @@ 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));
> + 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), 1);
Shouldn't you pass 0xff as value here? This way, you can only set pin 0 afaict
> break;
>
> default:
> @@ -219,9 +269,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 38458f4..9da68bd 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)
> @@ -40,15 +41,10 @@
> #define PL061_GPIO_PCELL_ID2 ((UINT32)PcdGet32 (PcdPL061GpioBase)
> + 0xFF8)
> #define PL061_GPIO_PCELL_ID3 ((UINT32)PcdGet32 (PcdPL061GpioBase)
> + 0xFFC)
>
> -
> // GPIO pins are numbered 0..7
> #define LAST_GPIO_PIN 7
>
> // 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__
> --
> 2.1.4
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
