Re: [PATCH 1/1] hw: aspeed_gpio: Fix pin I/O type declarations

[Peter Delevoryas]

> On Sep 30, 2021, at 5:05 AM, Rashmica Gupta <rashmica.g@gmail.com> wrote:
> 
> On Thu, 2021-09-30 at 00:45 +0000, Peter Delevoryas wrote:
>> 
>>> On Sep 28, 2021, at 3:53 AM, Damien Hedde
>>> <damien.hedde@greensocs.com> wrote:
>>> 
>>> 
>>> 
>>> On 9/28/21 05:24, pdel@fb.com wrote:
>>>> From: Peter Delevoryas <pdel@fb.com>
>>>> Some of the pin declarations in the Aspeed GPIO module were
>>>> incorrect,
>>>> probably because of confusion over which bits in the input and
>>>> output
>>>> uint32_t's correspond to which groups in the label array. Since
>>>> the
>>>> uint32_t literals are in big endian, it's sort of the opposite of
>>>> what
>>>> would be intuitive. The least significant bit in
>>>> ast2500_set_props[6]
>>>> corresponds to GPIOY0, not GPIOAB7.
> 
> Looks like I didn't think about endianness! I remember there was
> conflicting information about which groups of GPIOs were input only -
> the input/output table says groups W and X for ast2600 while the info
> in direction registers says T and U. I don't recall why I went with the
> former over the latter but the latter seems to be correct as this is
> what is in the kernel driver.

Oh I see, thanks for checking.

> 
>>>> GPIOxx indicates input and output capabilities, GPIxx indicates
>>>> only
>>>> input, GPOxx indicates only output.
>>>> AST2500:
>>>> - Previously had GPIW0..GPIW7 and GPIX0..GPIX7, that's correct.
>>>> - Previously had GPIOY0..GPIOY3, should have been GPIOY0..GPIOY7.
>>>> - Previously had GPIOAB0..GPIOAB3 and GPIAB4..GPIAB7, should only
>>>> have
>>>>   been GPIOAB0..GPIOAB3.
>>>> AST2600:
>>>> - GPIOT0..GPIOT7 should have been GPIT0..GPIT7.
>>>> - GPIOU0..GPIOU7 should have been GPIU0..GPIU7.
>>>> - GPIW0..GPIW7 should have been GPIOW0..GPIOW7.
>>>> - GPIOY0..GPIOY7 and GPIOZ0...GPIOZ7 were disabled.
>>>> Fixes: 4b7f956862dc2db4c5c ("hw/gpio: Add basic Aspeed GPIO model
>>>> for AST2400 and AST2500")
>>>> Fixes: 36d737ee82b2972167e ("hw/gpio: Add in AST2600 specific
>>>> implementation")
>>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> 
>>> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
>> 
> 
> Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>

Thanks Rashmica!

Peter

> 
>> cc’ing Dan
>> 
>>> 
>>>> ---
>>>>  hw/gpio/aspeed_gpio.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
>>>> index dfa6d6cb40..33a40a624a 100644
>>>> --- a/hw/gpio/aspeed_gpio.c
>>>> +++ b/hw/gpio/aspeed_gpio.c
>>>> @@ -796,7 +796,7 @@ static const GPIOSetProperties
>>>> ast2500_set_props[] = {
>>>>      [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>>>>      [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>>>>      [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>>>> -    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
>>>> +    [6] = {0x0fffffff,  0x0fffffff,  {"Y", "Z", "AA", "AB"} },
>>>>      [7] = {0x000000ff,  0x000000ff,  {"AC"} },
>>>>  };
>>>>  @@ -805,9 +805,9 @@ static GPIOSetProperties
>>>> ast2600_3_3v_set_props[] = {
>>>>      [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
>>>>      [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
>>>>      [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
>>>> -    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
>>>> -    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
>>>> -    [6] = {0xffff0000,  0x0fff0000,  {"Y", "Z", "", ""} },
>>>> +    [4] = {0xffffffff,  0x00ffffff,  {"Q", "R", "S", "T"} },
>>>> +    [5] = {0xffffffff,  0xffffff00,  {"U", "V", "W", "X"} },
>>>> +    [6] = {0x0000ffff,  0x0000ffff,  {"Y", "Z"} },
>>>>  };
>>>>    static GPIOSetProperties ast2600_1_8v_set_props[] = {