On 19 August 2015 at 08:56, Haojian Zhuang <[email protected]> wrote:
> // All bits low except one bit high, restricted to 8 bits
> // (i.e. ensures zeros above 8bits)
>
> But '&&' is wrong at here. It'll only return 1 or 0 as the result
> of GPIO_PIN_MASK_HIGH_8BIT(Pin).
>
> Since PL061 spec said in below.
>
> In order to write to GPIODATA, the corresponding bits in the mask,
> resulting from the address bus, PADDR[9:2], must be HIGH. Otherwise
> the bit values remain unchanged by the write.
> Similarly, the values read from this register are determined for
> each bit, by the mask bit derived from the address used to access
> the data register, PADDR[9:2]. Bits that are 1 in the address mask
> cause the corresponding bits in GPIODATA to be read, and bits that
> are 0 in the address mask cause the corresponding bits in GPIODATA
> to be read as 0, regardless of their value.
>
> Now simplify the way to read/write bit of GPIO_DATA register.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <[email protected]>
> ---
> ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c | 16 ++++++++--------
> ArmPlatformPkg/Include/Drivers/PL061Gpio.h | 4 ----
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> index ff05662..042fc76 100644
> --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c
> @@ -125,7 +125,7 @@ Get (
> }
> }
>
> - if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) {
> + if (MmioRead8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2))) {
Shouldn't this be and OR "|", not a plus "+"?
I realise that they probably achieve the same thing here, but using a
"+" for masking is unusual.
The same comment applied to the other changes below:
> *Value = 1;
> } else {
> *Value = 0;
> @@ -181,21 +181,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));
> 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));
> + MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), 0);
> // 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));
> 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));
> + MmioWrite8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2), 0xff);
> // 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));
> break;
>
> default:
> @@ -250,9 +250,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 (MmioRead8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK(Gpio) << 2))) {
> *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..d436fd4 100644
> --- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
> +++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h
> @@ -46,9 +46,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__
> --
> 2.1.4
>
> _______________________________________________
> edk2-devel mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
