On 02/10/16 16:00, Ryan Harkin wrote:
> 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 "+"?
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
>> [email protected]
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> 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
