| Message ID | TYCP286MB089577B47D70F0AB25ABA6F5BCD52@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mmio: do not calculate bgpio_bits via "ngpios" | expand |
Hi Shiji, thanks for your patch! On Tue, Jun 25, 2024 at 3:22 AM Shiji Yang <yangshiji66@outlook.com> wrote: > bgpio_bits must be aligned with the data bus width. For example, on a > 32 bit big endian system and we only have 16 GPIOs. If we only assume > bgpio_bits=16 we can never control the GPIO because the base address > is the lowest address. > > low address high address > ------------------------------------------------- > | byte3 | byte2 | byte1 | byte0 | > ------------------------------------------------- > | NaN | NaN | gpio8-15 | gpio0-7 | > ------------------------------------------------- > > Fixes: 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") > Fixes: https://github.com/openwrt/openwrt/issues/15739 > Reported-by: Mark Mentovai <mark@mentovai.com> > Signed-off-by: Shiji Yang <yangshiji66@outlook.com> > Suggested-By: Mark Mentovai <mark@mentovai.com> > Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com> > Tested-by: Lóránd Horváth <lorand.horvath82@gmail.com> Commit 55b2395e4e92 also contains this: @@ -614,10 +616,15 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, gc->parent = dev; gc->label = dev_name(dev); gc->base = -1; - gc->ngpio = gc->bgpio_bits; gc->request = bgpio_request; After this patch gc->ngpio will be unset for any GPIO chip that provides a ngpios property, so restore the above line too. But maybe a better fix is: + #include <linux/types.h> (...) + else + gc->bgpio_bits = round_up(gc->ngpio, sizeof(phys_addr_t) * 8); ? Yours, Linus Walleij
Hi Linus, On Wed, 26 Jun 2024 at 14:06, Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Shiji, > > thanks for your patch! > > On Tue, Jun 25, 2024 at 3:22 AM Shiji Yang <yangshiji66@outlook.com> wrote: > > > bgpio_bits must be aligned with the data bus width. For example, on a > > 32 bit big endian system and we only have 16 GPIOs. If we only assume > > bgpio_bits=16 we can never control the GPIO because the base address > > is the lowest address. > > > > low address high address > > ------------------------------------------------- > > | byte3 | byte2 | byte1 | byte0 | > > ------------------------------------------------- > > | NaN | NaN | gpio8-15 | gpio0-7 | > > ------------------------------------------------- > > > > Fixes: 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") > > Fixes: https://github.com/openwrt/openwrt/issues/15739 > > Reported-by: Mark Mentovai <mark@mentovai.com> > > Signed-off-by: Shiji Yang <yangshiji66@outlook.com> > > Suggested-By: Mark Mentovai <mark@mentovai.com> > > Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com> > > Tested-by: Lóránd Horváth <lorand.horvath82@gmail.com> > > Commit 55b2395e4e92 also contains this: > > @@ -614,10 +616,15 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, > gc->parent = dev; > gc->label = dev_name(dev); > gc->base = -1; > - gc->ngpio = gc->bgpio_bits; > gc->request = bgpio_request; > > After this patch gc->ngpio will be unset for any GPIO chip that > provides a ngpios property, so restore the above line too. The patch only removes a line changing gc->bgpio_bits, not gc->ngpio. gc->ngpio is untouched. gc->ngpio will still be set by gpiochip_get_ngpios() if there is a ngpio property. See the context of the patch: > @@ -619,8 +619,6 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, > ret = gpiochip_get_ngpios(gc, dev); <-- > if (ret) > gc->ngpio = gc->bgpio_bits; <- and if it fails, fallback to bgpio_bits > > But maybe a better fix is: > > + #include <linux/types.h> > (...) > + else > + gc->bgpio_bits = round_up(gc->ngpio, sizeof(phys_addr_t) * 8); > > ? bgpio only supports a single register worth of gpios, so the limit is 1 * sizeof(phys_addr_t) * 8. So this would force force bgpio_bits to sizeof(phys_addr_t) * 8. And this will break any bgpio users where the gpio registers are less than phys_addr_t wide. Like 32 bit registers on a 64 bit system, or 16 bit registers on 32 bit. Therefore I think the most sane thing is to keep gc->bgpio_bits at sz * 8. The only potentially reasonable thing that could be added here is a check that gc->ngpio is at most bgpio_bits. But that would be an additional check, not a fix per se. Best Regards, Jonas
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? Thanks. Asmaa > drivers/gpio/gpio-mmio.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index > 71e1af7c2184..d89e78f0ead3 100644 > --- a/drivers/gpio/gpio-mmio.c > +++ b/drivers/gpio/gpio-mmio.c > @@ -619,8 +619,6 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, > ret = gpiochip_get_ngpios(gc, dev); > if (ret) > gc->ngpio = gc->bgpio_bits; > - else > - gc->bgpio_bits = roundup_pow_of_two(round_up(gc->ngpio, > 8)); > > ret = bgpio_setup_io(gc, dat, set, clr, flags); > if (ret) > -- > 2.45.1
On Wed, 26 Jun 2024 at 17:00, Asmaa Mnebhi <asmaa@nvidia.com> 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. Best Regards, Jonas
> -----Original Message----- > From: Jonas Gorski <jonas.gorski@gmail.com> > Sent: Wednesday, June 26, 2024 11:21 AM > To: Asmaa Mnebhi <asmaa@nvidia.com> > Cc: Shiji Yang <yangshiji66@outlook.com>; linux-gpio@vger.kernel.org; Linus > Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>; > Andy Shevchenko <andy.shevchenko@gmail.com>; linux- > kernel@vger.kernel.org; Mark Mentovai <mark@mentovai.com>; Lóránd > Horváth <lorand.horvath82@gmail.com> > 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@nvidia.com> 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. 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." My v1->v3 patches were only changing ngpio, not bgpio_bit. Please check my v3 patch: https://lore.kernel.org/lkml/202303050354.HH9DhJsr-lkp@intel.com/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); 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.
On Wed, 26 Jun 2024 at 21:59, Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > > > > -----Original Message----- > > From: Jonas Gorski <jonas.gorski@gmail.com> > > Sent: Wednesday, June 26, 2024 11:21 AM > > To: Asmaa Mnebhi <asmaa@nvidia.com> > > Cc: Shiji Yang <yangshiji66@outlook.com>; linux-gpio@vger.kernel.org; Linus > > Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>; > > Andy Shevchenko <andy.shevchenko@gmail.com>; linux- > > kernel@vger.kernel.org; Mark Mentovai <mark@mentovai.com>; Lóránd > > Horváth <lorand.horvath82@gmail.com> > > 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@nvidia.com> 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@intel.com/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
> > >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. > LGTM then 😊. Thanks!
On Tue, Jun 25, 2024 at 3:22 AM Shiji Yang <yangshiji66@outlook.com> wrote: > bgpio_bits must be aligned with the data bus width. For example, on a > 32 bit big endian system and we only have 16 GPIOs. If we only assume > bgpio_bits=16 we can never control the GPIO because the base address > is the lowest address. > > low address high address > ------------------------------------------------- > | byte3 | byte2 | byte1 | byte0 | > ------------------------------------------------- > | NaN | NaN | gpio8-15 | gpio0-7 | > ------------------------------------------------- > > Fixes: 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") > Fixes: https://github.com/openwrt/openwrt/issues/15739 > Reported-by: Mark Mentovai <mark@mentovai.com> > Signed-off-by: Shiji Yang <yangshiji66@outlook.com> > Suggested-By: Mark Mentovai <mark@mentovai.com> > Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com> > Tested-by: Lóránd Horváth <lorand.horvath82@gmail.com> I'm convinced this is the right thing to do. (By Jonas Gorski.) Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Bart, can you apply it for fixes? (Or for -next if you wanna be really careful.) Yours, Linus Walleij
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> On Tue, 25 Jun 2024 09:19:49 +0800, Shiji Yang wrote: > bgpio_bits must be aligned with the data bus width. For example, on a > 32 bit big endian system and we only have 16 GPIOs. If we only assume > bgpio_bits=16 we can never control the GPIO because the base address > is the lowest address. > > low address high address > ------------------------------------------------- > | byte3 | byte2 | byte1 | byte0 | > ------------------------------------------------- > | NaN | NaN | gpio8-15 | gpio0-7 | > ------------------------------------------------- > > [...] Applied, thanks! [1/1] gpio: mmio: do not calculate bgpio_bits via "ngpios" commit: f07798d7bb9c46d17d80103fb772fd2c75d47919 Best regards,
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index 71e1af7c2184..d89e78f0ead3 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -619,8 +619,6 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev, ret = gpiochip_get_ngpios(gc, dev); if (ret) gc->ngpio = gc->bgpio_bits; - else - gc->bgpio_bits = roundup_pow_of_two(round_up(gc->ngpio, 8)); ret = bgpio_setup_io(gc, dat, set, clr, flags); if (ret)
bgpio_bits must be aligned with the data bus width. For example, on a 32 bit big endian system and we only have 16 GPIOs. If we only assume bgpio_bits=16 we can never control the GPIO because the base address is the lowest address. low address high address ------------------------------------------------- | byte3 | byte2 | byte1 | byte0 | ------------------------------------------------- | NaN | NaN | gpio8-15 | gpio0-7 | ------------------------------------------------- Fixes: 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()") Fixes: https://github.com/openwrt/openwrt/issues/15739 Reported-by: Mark Mentovai <mark@mentovai.com> Signed-off-by: Shiji Yang <yangshiji66@outlook.com> Suggested-By: Mark Mentovai <mark@mentovai.com> Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com> Tested-by: Lóránd Horváth <lorand.horvath82@gmail.com> --- drivers/gpio/gpio-mmio.c | 2 -- 1 file changed, 2 deletions(-)