Re: [PATCH] gpio: mmio: do not calculate bgpio_bits via "ngpios"

[Jonas Gorski <jonas.gorski@xxxxxxxxx>]

On Wed, 26 Jun 2024 at 21:59, Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Jonas Gorski <jonas.gorski@xxxxxxxxx>
> > Sent: Wednesday, June 26, 2024 11:21 AM
> > To: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
> > Cc: Shiji Yang <yangshiji66@xxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; Linus
> > Walleij <linus.walleij@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>;
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; linux-
> > kernel@xxxxxxxxxxxxxxx; Mark Mentovai <mark@xxxxxxxxxxxx>; Lóránd
> > Horváth <lorand.horvath82@xxxxxxxxx>
> > Subject: Re: [PATCH] gpio: mmio: do not calculate bgpio_bits via "ngpios"
> > Importance: High
> >
> > On Wed, 26 Jun 2024 at 17:00, Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
> > >
> > > I am not sure this change is needed?
> > > When I initially submitted " gpio: mmio: handle "ngpios" properly in
> > bgpio_init() ", It was specifically because I have a 32bit reg access but only 16
> > gpios. Initially, I did not add the else and so Andy suggested to add it with the
> > roundup_pow_of_two to stay backward compatible. If your system is a 32 bit
> > arch and you only use 16 Gpio bits, why don't you configure that in your DTS?
> >
> > Because the registers in the datasheet are specified as 32 bit wide, so defining
> > them as 32 bit in the dts(i) is the most natural way of defining them, even if
> > they may use less than half of the register for gpios. And on big endian
> > systems you cannot just use smaller accessors, you also must shift the register
> > offsets. So this change broke existing devicetrees.
> >
> > And as other theoretical arguments against doing that, less than 32 bit
> > accesses may be inefficient, or the bus where the registers are may require 32
> > bit accesses. And finally, the caller explicitly passed a register width via the sz
> > argument, so we should listen to the caller and use that, and not trying to be
> > clever by changing the access width based on the number of gpios. At least
> > not unless the caller explicitly requested that. Like e.g. make 0 a special value
> > for automatically calculating the number of bits based on the number of
> > gpios.
> >
> > If you only use 16 bits of the 32 bit registers and you want to use 16 bit
> > accessors, IMHO it's up to you to pass appropriate values to bgpio_init(), and
> > not hope that bgpio_init() will fix this magically up for you.
> >
>
> It was definitely not my intention to change/hack a 32 bits reg access to 16.
> I agree with you that bgpio_bits should just not be changed. Maybe the else statement introduces a bug/hack in the case where ngpio is already a power of 2 such as 16 while the register access is a 32 bit access. In this case bgpio_bits would be 16 when it should be 32.

It definitely does, as currently ngpio 1 - 8 will cause bgpio_bits
will be set to 8, leading to 8 bit accesses, 9 - 16 will be 16 bit
accesses etc.

>
> However, Shiji's has a bug and will break other code. My very first patch for "gpio: mmio: handle "ngpios" properly in bgpio_init()" was meant to fix the following problem (that shiji's code will reintroduce):
> " bgpio_init uses "sz" argument to populate ngpio, which is not accurate.
> Instead, read the "ngpios" property from the DT and if it doesn't
> exist, use the "sz" argument. With this change, drivers no longer need
> to overwrite the ngpio variable after calling bgpio_init."

And this is a very nice thing to have! No objections here.

>
> My v1->v3 patches were only changing ngpio, not bgpio_bit. Please check my v3 patch: https://lore.kernel.org/lkml/202303050354.HH9DhJsr-lkp@xxxxxxxxx/t/
>
> After v3, Andy asked me to also change bgpio_bits. Please see the whole thread:
>
> > > > >+       ret = gpiochip_get_ngpios(gc, dev);
> > > > >+       if (ret)
> > > >> +               gc->ngpio = gc->bgpio_bits;
> > >>
> > > >But this doesn't update bgpio_bits in the success case. Can you explain why
> > >> it's not a problem (should be at least in the code as a comment).
> >>
> >> In the success rate, the bgpio_bits would also be equal to "sz * 8" anyways.
> >> The argument " unsigned long sz" passed in bgpio_init is specifically for this purpose. That tells the gpio library the gpio register access size.
> > >if (!is_power_of_2(sz))
> >>                  return -EINVAL;
> > > gc->bgpio_bits = sz * 8;
> >>
> > >If in the success case, we make it dependent on the ngpio value, we would need to round it up anyways to the closest (power of 2 && multiple of 8) which is the same as "sz * 8"
> >> I will add a comment in the code in my next patch.
>
> >I believe we should use only a single source for what we need. If we
> > rely on ngpios, the bgpio_bits should be recalculated based on it. The
> >expression doing this is very simple. Something like round_up(ngpios,
> >8);
>

Right, mistakes happen. Also I don't want to blame anyone, because it
happens to work fine on little endian systems, and big endian systems
are so rare it's easy to forget that they exist ;-)

>  Now, if we want to not modify bgpio_bits, we could go back to my v3 patch.
> ngpio is the number of gpio pins supported while bgpio_bits is the register access bit type. These are 2 different entities.

Exactly. And AFAICT this patch does exactly this, restoring the code
to the state of v3.

Best Regards,
Jonas