On 02/10/16 16:00, Ryan Harkin wrote: > On 19 August 2015 at 08:56, 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..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 "+"?
Not knowing anything about what's going on: I don't think so. The above does not manipulate a register value, it calculates a register offset. > I realise that they probably achieve the same thing here, but using a > "+" for masking is unusual. It's not (value) masking, it's offset calculation. (Whether the offset calculation is *correct* is of course over my head.) > > The same comment applied to the other changes below: Those changes are different: >> *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)); MmioAnd8() reads the register at the given offset, masks the value read, and then writes back the result. >> 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); This replaces the read-mask-write triplet (using a constant offset) with a simple write (to a dynamically calculated offset). >> // 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; read-set-write, only the or-mask changes >> >> 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); replaces read-set-write to a constant register offset with a write to a dynamically calculated offset etc Thanks Laszlo >> // 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 >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel