On 11 August 2015 at 11:42, Haojian Zhuang haojian.zhuang@linaro.org wrote:

Both GPIO_PIN_MASK_HIGH_8BIT() and GPIO_PIN_MASK_LOW_8BIT() macro should return a 8-bit value. But it returned a boolean value (0 or 1). So replace "&&" with "&" instead.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org

---

ArmPlatformPkg/Include/Drivers/PL061Gpio.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h index 38458f4..cafa7b2 100644 --- a/ArmPlatformPkg/Include/Drivers/PL061Gpio.h +++ b/ArmPlatformPkg/Include/Drivers/PL061Gpio.h @@ -47,8 +47,8 @@ // 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) +#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) +#define GPIO_PIN_MASK_LOW_8BIT(Pin) ((~GPIO_PIN_MASK(Pin)) & 0xFF)

#endif // __PL061_GPIO_H__

OK, you were really quick with these patches :-)

I looked at the code and the PL061 documentation myself, and I would like to propose a more thorough way to fix this code.

In this patch, could you please remove the two macros completely? So in the .c file, GPIO_PIN_MASK_HIGH_8BIT() just becomes GPIO_PIN_MASK() and GPIO_PIN_MASK_LOW_8BIT() become ~GPIO_PIN_MASK(). Then, these 2 macros can be dropped. (If you are feeling pedantic, you can add a (UINT8) cast to the definition of GPIO_PIN_MASK(), but I don't think any of the compilers we support for ARM will complain if you don't)

The reason is that a) they are not needed, since they are only used inside MmioXxxx8() functions that ignore the top bits anyway, so they only make the C code more difficult to read b) their names are misleading -> 'mask low 8 bit' means something different to most programmers from what is being done here. c) your next patch will remove a couple of invocations anyway (see my comments there)

Thanks, Ard.

On 11 August 2015 at 11:42, Haojian Zhuang haojian.zhuang@linaro.org wrote:

The way of accessing PL061 GPIODATA register is wrong.

The 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.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Haojian Zhuang haojian.zhuang@linaro.org

---

ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c index ff05662..35418c9 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_HIGH_8BIT(Gpio) << 2))) { *Value = 1; } else { *Value = 0;

@@ -186,14 +186,14 @@ Set (

 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));
  

•  MmioAnd8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK_HIGH_8BIT(Gpio) << 2), GPIO_PIN_MASK_LOW_8BIT(Gpio));
  

There is no reason for a read/and/write sequence here: the mask will ensure that you can only affect the selected GPIO, so instead, you can simply use MmioWrite8(<addr>, 0x0)

   // Set the corresponding direction bit to HIGH for output
   MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK_HIGH_8BIT(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));
  

•  MmioOr8 (PL061_GPIO_DATA_REG + (GPIO_PIN_MASK_HIGH_8BIT(Gpio) << 2), GPIO_PIN_MASK_HIGH_8BIT(Gpio));
  

Likewise, but use MmioWrite8(addr, 0xff) in this case

   // Set the corresponding direction bit to HIGH for output
   MmioOr8 (PL061_GPIO_DIR_REG, GPIO_PIN_MASK_HIGH_8BIT(Gpio));
   break;

There is another place in the file that uses the same broken logic, inside GetMode(). Could you please fix that as well?