On Tue, 2015-08-11 at 09:42 +0200, Ard Biesheuvel wrote:
On 11 August 2015 at 09:11, 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))) {
Hello Haojian,
I have not looked at the PL061 spec, but I noticed a pretty awful bug in the PL061 header (ArmPlatformPkg/Include/Drivers/PL061Gpio.h):
50:#define GPIO_PIN_MASK_HIGH_8BIT(Pin) (GPIO_PIN_MASK(Pin) && 0xFF) 52:#define GPIO_PIN_MASK_LOW_8BIT(Pin) ((~GPIO_PIN_MASK(Pin)) && 0xFF)
I am pretty sure that the && is incorrect here, since the result of the expression can only be 0 or 1 then.
Could you perhaps fix this first, and only then figure out what you need to get the code to work for your hardware? It looks like you are using some kind of bit banding interface, whereas the existing code reads/writes all GPIO values from offset 0x0 and performs its own masking. Perhaps your approach is better, but I'd like to fix first and then optimize.
Thanks, Ard.
Actually this patch tried to fix both gpio input & gpio output. In my previous test, I only tested gpio input (MmioRead8()) above.
Yes, I need to replace "&&" symbol by "&" symbol. Otherwise, my patch can't make gpio output working well.
Since the original code is also trying to update one bit value in gpio register, my patch is just used to make it work as expected.
Regards Haojian