[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] hw: aspeed_gpio: Fix pin I/O type declarations
From: |
Rashmica Gupta |
Subject: |
Re: [PATCH 1/1] hw: aspeed_gpio: Fix pin I/O type declarations |
Date: |
Thu, 30 Sep 2021 22:05:04 +1000 |
User-agent: |
Evolution 3.38.4 (3.38.4-1.fc33) |
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.
> > > 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>
> 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[] = {
>