On Tue, 2015-08-18 at 10:45 +0200, Ard Biesheuvel wrote:
> On 18 August 2015 at 10:39, Haojian Zhuang <haojian.zhu...@linaro.org> 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 <haojian.zhu...@linaro.org>
> > ---
> > 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..e8a2094 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))) {
> > *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));
>
> This should be ~GPIO_PIN_MASK(Gpio)
OK. I'll fix it.
>
> > 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),
> > GPIO_PIN_MASK(Gpio));
>
> You should write the value 0x0 here: this will only affect the GPIO
> that you selected, so it will clear only a single bit
>
All these are already fixed in v3 patches. But I miss to merge them into
the 3rd patch. I'll reformat the patches.
And these other 2 patches of v3 are sent separately since I met the
issue of sending email. I have to resend them again.
Regards
Haojian
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
