Hi,
It seems GPIOs on OMAP1 boards are somewhat broken after:
commit 92bf78b33b0b463b00c6b0203b49aea845daecc8
Author: Andreas Kemnade <[email protected]>
Date: Fri Jan 13 21:59:22 2023 +0100
gpio: omap: use dynamic allocation of base
E.g. on OSK1 the ethernet IRQ cannot (omap_gpio.0) no longer be requested:
[ 0.277252] Error requesting gpio 0 for smc91x irq
Also the tps65010 (still using static allocation) will now conflict:
[ 0.400726] gpio gpiochip5: Static allocation of GPIO base is deprecated, use dynamic allocation.
[ 0.400848] gpio gpiochip5: (tps65010): GPIO integer space overlap, cannot add chip
[ 0.400970] gpiochip_add_data_with_key: GPIOs 208..214 (tps65010) failed to register, -16
[ 0.401092] tps65010 i2c-tps65010: can't add gpiochip, err -16
I think this change should be reverted until the board files and other
gpiochips are fixed accordingly.
A.
Hi,
On Tue, 25 Apr 2023 20:32:41 +0300
Aaro Koskinen <[email protected]> wrote:
> Hi,
>
> It seems GPIOs on OMAP1 boards are somewhat broken after:
>
> commit 92bf78b33b0b463b00c6b0203b49aea845daecc8
> Author: Andreas Kemnade <[email protected]>
> Date: Fri Jan 13 21:59:22 2023 +0100
>
> gpio: omap: use dynamic allocation of base
>
> E.g. on OSK1 the ethernet IRQ cannot (omap_gpio.0) no longer be requested:
>
> [ 0.277252] Error requesting gpio 0 for smc91x irq
>
> Also the tps65010 (still using static allocation) will now conflict:
>
> [ 0.400726] gpio gpiochip5: Static allocation of GPIO base is deprecated, use dynamic allocation.
> [ 0.400848] gpio gpiochip5: (tps65010): GPIO integer space overlap, cannot add chip
> [ 0.400970] gpiochip_add_data_with_key: GPIOs 208..214 (tps65010) failed to register, -16
> [ 0.401092] tps65010 i2c-tps65010: can't add gpiochip, err -16
>
> I think this change should be reverted until the board files and other
> gpiochips are fixed accordingly.
>
well, then just fix that tps65010 thing.
that change is itself a regression fix for exactly the same kind of error.
twl4030 gpio registration conflicts with omap gpio registration.
Probably in former times, the dynamic allocation always started at 512,
so no conflicts between static and dynamic.
So I see two options: either fix the remaining static allocation
or fix allocation so there are no overlaps between static and dynamic.
Regards,
Andreas
Hi,
On Tue, Apr 25, 2023 at 08:11:17PM +0200, Andreas Kemnade wrote:
> On Tue, 25 Apr 2023 20:32:41 +0300
> Aaro Koskinen <[email protected]> wrote:
> > It seems GPIOs on OMAP1 boards are somewhat broken after:
> >
> > commit 92bf78b33b0b463b00c6b0203b49aea845daecc8
> > Author: Andreas Kemnade <[email protected]>
> > Date: Fri Jan 13 21:59:22 2023 +0100
> >
> > gpio: omap: use dynamic allocation of base
> >
> > E.g. on OSK1 the ethernet IRQ cannot (omap_gpio.0) no longer be requested:
> >
> > [ 0.277252] Error requesting gpio 0 for smc91x irq
> >
> > Also the tps65010 (still using static allocation) will now conflict:
> >
> > [ 0.400726] gpio gpiochip5: Static allocation of GPIO base is deprecated, use dynamic allocation.
> > [ 0.400848] gpio gpiochip5: (tps65010): GPIO integer space overlap, cannot add chip
> > [ 0.400970] gpiochip_add_data_with_key: GPIOs 208..214 (tps65010) failed to register, -16
> > [ 0.401092] tps65010 i2c-tps65010: can't add gpiochip, err -16
> >
> > I think this change should be reverted until the board files and other
> > gpiochips are fixed accordingly.
> >
> well, then just fix that tps65010 thing.
>
> that change is itself a regression fix for exactly the same kind of error.
Which commit introduced that regression? Also, the changelog mentions
it happens only with "unusual" probe order. Now, all the ordinary cases
for OMAP1 are broken.
And it's not just that tps65010 thing. E.g. 770 fails to boot as well
and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
well. AFAIK it's because all the gpio_request()s in OMAP1 board files
stopped now working.
A.
On Tue, 25 Apr 2023 21:38:57 +0300
Aaro Koskinen <[email protected]> wrote:
> Hi,
>
> On Tue, Apr 25, 2023 at 08:11:17PM +0200, Andreas Kemnade wrote:
> > On Tue, 25 Apr 2023 20:32:41 +0300
> > Aaro Koskinen <[email protected]> wrote:
> > > It seems GPIOs on OMAP1 boards are somewhat broken after:
> > >
> > > commit 92bf78b33b0b463b00c6b0203b49aea845daecc8
> > > Author: Andreas Kemnade <[email protected]>
> > > Date: Fri Jan 13 21:59:22 2023 +0100
> > >
> > > gpio: omap: use dynamic allocation of base
> > >
> > > E.g. on OSK1 the ethernet IRQ cannot (omap_gpio.0) no longer be requested:
> > >
> > > [ 0.277252] Error requesting gpio 0 for smc91x irq
> > >
> > > Also the tps65010 (still using static allocation) will now conflict:
> > >
> > > [ 0.400726] gpio gpiochip5: Static allocation of GPIO base is deprecated, use dynamic allocation.
> > > [ 0.400848] gpio gpiochip5: (tps65010): GPIO integer space overlap, cannot add chip
> > > [ 0.400970] gpiochip_add_data_with_key: GPIOs 208..214 (tps65010) failed to register, -16
> > > [ 0.401092] tps65010 i2c-tps65010: can't add gpiochip, err -16
> > >
> > > I think this change should be reverted until the board files and other
> > > gpiochips are fixed accordingly.
> > >
> > well, then just fix that tps65010 thing.
> >
> > that change is itself a regression fix for exactly the same kind of error.
>
> Which commit introduced that regression? Also, the changelog mentions
> it happens only with "unusual" probe order. Now, all the ordinary cases
> for OMAP1 are broken.
>
did not bisect that to an exact commit.
Unusual probe order: on the device where I tested it,
I did not see a completely successful probe.
> And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> stopped now working.
>
so we break every non-devicetree user of omap-gpio?
Regards,
Andreas
On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> Aaro Koskinen <[email protected]> wrote:
> > Which commit introduced that regression? Also, the changelog mentions
> > it happens only with "unusual" probe order. Now, all the ordinary cases
> > for OMAP1 are broken.
> >
> did not bisect that to an exact commit.
> Unusual probe order: on the device where I tested it,
> I did not see a completely successful probe.
If you cannot point out a working past commit, there was no regression. If
you fix something that hasn't worked before or has been long time broken,
it must not cause breakage to other current users.
> > And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> > and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> > well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> > stopped now working.
> >
> so we break every non-devicetree user of omap-gpio?
It seems so.
A.
On Tue, 25 Apr 2023 22:36:37 +0300
Aaro Koskinen <[email protected]> wrote:
> On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> > Aaro Koskinen <[email protected]> wrote:
> > > Which commit introduced that regression? Also, the changelog mentions
> > > it happens only with "unusual" probe order. Now, all the ordinary cases
> > > for OMAP1 are broken.
> > >
> > did not bisect that to an exact commit.
> > Unusual probe order: on the device where I tested it,
> > I did not see a completely successful probe.
>
> If you cannot point out a working past commit, there was no regression. If
> you fix something that hasn't worked before or has been long time broken,
> it must not cause breakage to other current users.
>
Well, I did not take the time for a bisect. As we need a less aggressive
fix, it seems to be worth doing it.
> > > And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> > > and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> > > well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> > > stopped now working.
> > >
> > so we break every non-devicetree user of omap-gpio?
>
> It seems so.
>
or maybe an if (not_using_devicetree())
Regards,
Andreas
Hi,
* Andreas Kemnade <[email protected]> [230425 19:58]:
> On Tue, 25 Apr 2023 22:36:37 +0300
> Aaro Koskinen <[email protected]> wrote:
>
> > On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> > > Aaro Koskinen <[email protected]> wrote:
> > > > Which commit introduced that regression? Also, the changelog mentions
> > > > it happens only with "unusual" probe order. Now, all the ordinary cases
> > > > for OMAP1 are broken.
> > > >
> > > did not bisect that to an exact commit.
> > > Unusual probe order: on the device where I tested it,
> > > I did not see a completely successful probe.
> >
> > If you cannot point out a working past commit, there was no regression. If
> > you fix something that hasn't worked before or has been long time broken,
> > it must not cause breakage to other current users.
> >
> Well, I did not take the time for a bisect. As we need a less aggressive
> fix, it seems to be worth doing it.
>
> > > > And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> > > > and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> > > > well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> > > > stopped now working.
> > > >
> > > so we break every non-devicetree user of omap-gpio?
> >
> > It seems so.
> >
> or maybe an if (not_using_devicetree())
Not sure what the best way to fix this might be, adding Linus W to Cc too.
Maybe using gpio line names in the legacy platform data instead of numbers?
Seems that we should just revert this patch for now and try again after
the issues have been fixed.
Regards,
Tony
Hi,
On Wed, 26 Apr 2023 10:19:10 +0300
Tony Lindgren <[email protected]> wrote:
> Hi,
>
> * Andreas Kemnade <[email protected]> [230425 19:58]:
> > On Tue, 25 Apr 2023 22:36:37 +0300
> > Aaro Koskinen <[email protected]> wrote:
> >
> > > On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> > > > Aaro Koskinen <[email protected]> wrote:
> > > > > Which commit introduced that regression? Also, the changelog mentions
> > > > > it happens only with "unusual" probe order. Now, all the ordinary cases
> > > > > for OMAP1 are broken.
> > > > >
> > > > did not bisect that to an exact commit.
> > > > Unusual probe order: on the device where I tested it,
> > > > I did not see a completely successful probe.
> > >
> > > If you cannot point out a working past commit, there was no regression. If
> > > you fix something that hasn't worked before or has been long time broken,
> > > it must not cause breakage to other current users.
> > >
> > Well, I did not take the time for a bisect. As we need a less aggressive
> > fix, it seems to be worth doing it.
> >
> > > > > And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> > > > > and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> > > > > well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> > > > > stopped now working.
> > > > >
> > > > so we break every non-devicetree user of omap-gpio?
> > >
> > > It seems so.
> > >
> > or maybe an if (not_using_devicetree())
>
> Not sure what the best way to fix this might be, adding Linus W to Cc too.
> Maybe using gpio line names in the legacy platform data instead of numbers?
>
> Seems that we should just revert this patch for now and try again after
> the issues have been fixed.
>
I think the reason for the patch (besides of cleaning up warnings) is that
dynamic allocation seems to start at 512, static at zero.
If both are there, like registering twl_gpio between omap gpiochip 4 and 5,
dynamic allocation seems just to start after the last static number,
calling for trouble.
If dynamic alloc would just start at 512 in that case too, no problem would appear.
As said I have not bisected it to an exact commit yet.
So if we need to move backward, we should IMHO first fix that allocation thing.
Regards,
Andreas
Wed, Apr 26, 2023 at 09:39:20AM +0200, Andreas Kemnade kirjoitti:
> On Wed, 26 Apr 2023 10:19:10 +0300
> Tony Lindgren <[email protected]> wrote:
> > * Andreas Kemnade <[email protected]> [230425 19:58]:
> > > On Tue, 25 Apr 2023 22:36:37 +0300
> > > Aaro Koskinen <[email protected]> wrote:
> > > > On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> > > > > Aaro Koskinen <[email protected]> wrote:
...
> > > > > > Which commit introduced that regression? Also, the changelog mentions
> > > > > > it happens only with "unusual" probe order. Now, all the ordinary cases
> > > > > > for OMAP1 are broken.
> > > > > >
> > > > > did not bisect that to an exact commit.
> > > > > Unusual probe order: on the device where I tested it,
> > > > > I did not see a completely successful probe.
> > > >
> > > > If you cannot point out a working past commit, there was no regression. If
> > > > you fix something that hasn't worked before or has been long time broken,
> > > > it must not cause breakage to other current users.
> > > >
> > > Well, I did not take the time for a bisect. As we need a less aggressive
> > > fix, it seems to be worth doing it.
> > >
> > > > > > And it's not just that tps65010 thing. E.g. 770 fails to boot as well
> > > > > > and it doesn't use it; and reverting 92bf78b33b0b fixes that one as
> > > > > > well. AFAIK it's because all the gpio_request()s in OMAP1 board files
> > > > > > stopped now working.
> > > > > >
> > > > > so we break every non-devicetree user of omap-gpio?
> > > >
> > > > It seems so.
> > > >
> > > or maybe an if (not_using_devicetree())
> >
> > Not sure what the best way to fix this might be, adding Linus W to Cc too.
> > Maybe using gpio line names in the legacy platform data instead of numbers?
> >
> > Seems that we should just revert this patch for now and try again after
> > the issues have been fixed.
> >
> I think the reason for the patch (besides of cleaning up warnings) is that
> dynamic allocation seems to start at 512, static at zero.
> If both are there, like registering twl_gpio between omap gpiochip 4 and 5,
> dynamic allocation seems just to start after the last static number,
> calling for trouble.
>
> If dynamic alloc would just start at 512 in that case too, no problem would appear.
> As said I have not bisected it to an exact commit yet.
> So if we need to move backward, we should IMHO first fix that allocation thing.
I agree.
As PoC can the reported add the following lines
if (gdev->base < GPIO_DYNAMIC_BASE)
continue;
after https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L190
and test your idea?
--
With Best Regards,
Andy Shevchenko
On Wed, 26 Apr 2023 13:52:51 +0300
[email protected] wrote:
> Wed, Apr 26, 2023 at 09:39:20AM +0200, Andreas Kemnade kirjoitti:
> > On Wed, 26 Apr 2023 10:19:10 +0300
> > Tony Lindgren <[email protected]> wrote:
> > > * Andreas Kemnade <[email protected]> [230425 19:58]:
> > > > On Tue, 25 Apr 2023 22:36:37 +0300
> > > > Aaro Koskinen <[email protected]> wrote:
> > > > > On Tue, Apr 25, 2023 at 09:20:40PM +0200, Andreas Kemnade wrote:
> > > > > > Aaro Koskinen <[email protected]> wrote:
>
> ...
[...]
> > I think the reason for the patch (besides of cleaning up warnings) is that
> > dynamic allocation seems to start at 512, static at zero.
> > If both are there, like registering twl_gpio between omap gpiochip 4 and 5,
> > dynamic allocation seems just to start after the last static number,
> > calling for trouble.
> >
> > If dynamic alloc would just start at 512 in that case too, no problem would appear.
> > As said I have not bisected it to an exact commit yet.
> > So if we need to move backward, we should IMHO first fix that allocation thing.
>
> I agree.
>
> As PoC can the reported add the following lines
>
> if (gdev->base < GPIO_DYNAMIC_BASE)
> continue;
>
> after https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L190
> and test your idea?
>
>
yes,
looking at git blame
7b61212f2a07a ("gpiolib: Get rid of ARCH_NR_GPIOS")
would probably have been the correct Fixes-tag for for that patch (and
will be for the new approach to fix it) and its intention was exactly
what we are talking about.
I will test.
Regards,
Andreas
On Tue, 25 Apr 2023 21:38:57 +0300
Aaro Koskinen <[email protected]> wrote:
> Hi,
>
> On Tue, Apr 25, 2023 at 08:11:17PM +0200, Andreas Kemnade wrote:
> > On Tue, 25 Apr 2023 20:32:41 +0300
> > Aaro Koskinen <[email protected]> wrote:
> > > It seems GPIOs on OMAP1 boards are somewhat broken after:
> > >
> > > commit 92bf78b33b0b463b00c6b0203b49aea845daecc8
> > > Author: Andreas Kemnade <[email protected]>
> > > Date: Fri Jan 13 21:59:22 2023 +0100
> > >
> > > gpio: omap: use dynamic allocation of base
> > >
> > > E.g. on OSK1 the ethernet IRQ cannot (omap_gpio.0) no longer be requested:
> > >
> > > [ 0.277252] Error requesting gpio 0 for smc91x irq
> > >
> > > Also the tps65010 (still using static allocation) will now conflict:
> > >
> > > [ 0.400726] gpio gpiochip5: Static allocation of GPIO base is deprecated, use dynamic allocation.
> > > [ 0.400848] gpio gpiochip5: (tps65010): GPIO integer space overlap, cannot add chip
> > > [ 0.400970] gpiochip_add_data_with_key: GPIOs 208..214 (tps65010) failed to register, -16
> > > [ 0.401092] tps65010 i2c-tps65010: can't add gpiochip, err -16
> > >
> > > I think this change should be reverted until the board files and other
> > > gpiochips are fixed accordingly.
> > >
> > well, then just fix that tps65010 thing.
> >
> > that change is itself a regression fix for exactly the same kind of error.
>
> Which commit introduced that regression? Also, the changelog mentions
> it happens only with "unusual" probe order. Now, all the ordinary cases
> for OMAP1 are broken.
>
Checked it:
7b61212f2a07a ("gpiolib: Get rid of ARCH_NR_GPIOS")
As said in another mail, I will now try the suggested less-invasive fix.
Regards,
Andreas
On Wed, Apr 26, 2023 at 9:19 AM Tony Lindgren <[email protected]> wrote:
> Not sure what the best way to fix this might be, adding Linus W to Cc too.
> Maybe using gpio line names in the legacy platform data instead of numbers?
I sent a fat invasive fix which, if it works, will fix the problem once and
for all on OSK1.
If it works, I can write the same fix for Nokia 770 or whatever.
I think it is best to just get rid of the static GPIO numbers from these
boards so I took a stab at that.
Yours,
Linus Walleij
* Linus Walleij <[email protected]> [230426 20:36]:
> On Wed, Apr 26, 2023 at 9:19 AM Tony Lindgren <[email protected]> wrote:
>
> > Not sure what the best way to fix this might be, adding Linus W to Cc too.
> > Maybe using gpio line names in the legacy platform data instead of numbers?
>
> I sent a fat invasive fix which, if it works, will fix the problem once and
> for all on OSK1.
>
> If it works, I can write the same fix for Nokia 770 or whatever.
>
> I think it is best to just get rid of the static GPIO numbers from these
> boards so I took a stab at that.
OK makes sense to me thanks!
Tony
On Thu, Apr 27, 2023 at 9:03 AM Tony Lindgren <[email protected]> wrote:
>
> * Linus Walleij <[email protected]> [230426 20:36]:
> > On Wed, Apr 26, 2023 at 9:19 AM Tony Lindgren <[email protected]> wrote:
> >
> > > Not sure what the best way to fix this might be, adding Linus W to Cc too.
> > > Maybe using gpio line names in the legacy platform data instead of numbers?
> >
> > I sent a fat invasive fix which, if it works, will fix the problem once and
> > for all on OSK1.
> >
> > If it works, I can write the same fix for Nokia 770 or whatever.
> >
> > I think it is best to just get rid of the static GPIO numbers from these
> > boards so I took a stab at that.
>
> OK makes sense to me thanks!
>
> Tony
Sorry, I'm late to the party, I was busy at Linaro Connect. Thanks
Linus for taking care of this.
Bartosz
* Tony Lindgren <[email protected]> [230426 07:20]:
> Seems that we should just revert this patch for now and try again after
> the issues have been fixed.
Looking at the proposed fixes being posted seems like they are quite
intrusive.. How about we partially revert this patch so omap1 still
uses static assigment of gpios?
This way the later SoCs can avoid the warnings and the legacy board
file and driver changes can get merged during the next merge window.
Regards,
Tony
On Thu, May 4, 2023 at 7:52 AM Tony Lindgren <[email protected]> wrote:
> * Tony Lindgren <[email protected]> [230426 07:20]:
> > Seems that we should just revert this patch for now and try again after
> > the issues have been fixed.
>
> Looking at the proposed fixes being posted seems like they are quite
> intrusive.. How about we partially revert this patch so omap1 still
> uses static assigment of gpios?
I think Andreas patch (commit 92bf78b33b0b463b00c6b0203b49aea845daecc8)
kind of describes the problem with that: the probe order is now unpredictable,
so if we revert the patch then that problem returns, but I don't know how
serious that problem is.
It's one of the reasons why we can't have static GPIO bases anymore FWIW.
The only fix that would actually fix the problem would be to undo deferred
probe and the ongoing work to determine probe order from the device
tree resource tree, and it is way too late for that, it's just not possible.
Yours,
Linus Walleij
Hi,
On Thu, 4 May 2023 14:13:32 +0200
Linus Walleij <[email protected]> wrote:
> On Thu, May 4, 2023 at 7:52 AM Tony Lindgren <[email protected]> wrote:
> > * Tony Lindgren <[email protected]> [230426 07:20]:
> > > Seems that we should just revert this patch for now and try again after
> > > the issues have been fixed.
> >
> > Looking at the proposed fixes being posted seems like they are quite
> > intrusive.. How about we partially revert this patch so omap1 still
> > uses static assigment of gpios?
>
> I think Andreas patch (commit 92bf78b33b0b463b00c6b0203b49aea845daecc8)
> kind of describes the problem with that: the probe order is now unpredictable,
> so if we revert the patch then that problem returns, but I don't know how
> serious that problem is.
>
well, I think we can even fully revert 92bf78b33b0b463b00c6b0203b49aea845daecc8
after my patch
gpiolib: fix allocation of mixed dynamic/static GPIOs
is in as a short time solution. That should only leave unpredictable
numbers of multiple dynamic gpio controllers.
Regards,
Andrea
* Andreas Kemnade <[email protected]> [230504 12:45]:
> Hi,
>
> On Thu, 4 May 2023 14:13:32 +0200
> Linus Walleij <[email protected]> wrote:
>
> > On Thu, May 4, 2023 at 7:52 AM Tony Lindgren <[email protected]> wrote:
> > > * Tony Lindgren <[email protected]> [230426 07:20]:
> > > > Seems that we should just revert this patch for now and try again after
> > > > the issues have been fixed.
> > >
> > > Looking at the proposed fixes being posted seems like they are quite
> > > intrusive.. How about we partially revert this patch so omap1 still
> > > uses static assigment of gpios?
> >
> > I think Andreas patch (commit 92bf78b33b0b463b00c6b0203b49aea845daecc8)
> > kind of describes the problem with that: the probe order is now unpredictable,
> > so if we revert the patch then that problem returns, but I don't know how
> > serious that problem is.
> >
> well, I think we can even fully revert 92bf78b33b0b463b00c6b0203b49aea845daecc8
> after my patch
>
> gpiolib: fix allocation of mixed dynamic/static GPIOs
>
> is in as a short time solution. That should only leave unpredictable
> numbers of multiple dynamic gpio controllers.
OK thanks sounds good to me.
Regards,
Tony