| Message ID | 20230926160255.330417-1-robert.marko@sartura.hr |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: core: dont change pinmux state to GPIO during recovery setup | expand |
On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@sartura.hr> wrote: > @@ -359,13 +359,6 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap) > if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery) > return 0; > > - /* > - * pins might be taken as GPIO, so we should inform pinctrl about > - * this and move the state to GPIO > - */ > - if (bri->pinctrl) > - pinctrl_select_state(bri->pinctrl, bri->pins_gpio); > - But this might be absolutely necessary for other i2c drivers and this is set in generic code. My first question is: why is this platform even defining the "gpio" pin control state for this i2c device if it is so dangerous to use? If it can't be used, you just give it too much rope, delete the "gpio" state for this group from the device tree: problem solved. (This can be done with the specific /delete-node/ directive if necessary, e.g. if you want to use the "gpio" state on other boards.) Second: do you even want to do recovery on this platform then? Is it necessary? What happens electronically in this case, if we don't switch the pins to GPIO mode? Is it something akin to the "strict" property in struct pinmux: that the GPIO block and the device can affect the same pins at the same time? That warrants an explanation and a comment. If you can't delete the "gpio" pin control state, I would add a bool pinctrl_stay_in_device_mode; to struct i2c_bus_recovery_info in include/linux/i2c.h And just: if (bri->pinctrl && !bri->pinctrl_stay_in_device_mode) pinctrl_select_state(bri->pinctrl, bri->pins_gpio); (Also the switch to the "default" state further down could be contitional !bri->pinctrl_stay_in_device_mode) But mostly I wonder why the "gpio" pin control state is defined, if it's not to be used. Yours, Linus Walleij
On Thu, Sep 28, 2023 at 11:11 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > @@ -359,13 +359,6 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap) > > if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery) > > return 0; > > > > - /* > > - * pins might be taken as GPIO, so we should inform pinctrl about > > - * this and move the state to GPIO > > - */ > > - if (bri->pinctrl) > > - pinctrl_select_state(bri->pinctrl, bri->pins_gpio); > > - > > But this might be absolutely necessary for other i2c drivers and this is > set in generic code. > > My first question is: why is this platform even defining the "gpio" pin > control state for this i2c device if it is so dangerous to use? > If it can't be used, you just give it too much rope, delete the "gpio" > state for this group from the device tree: problem solved. > > (This can be done with the specific /delete-node/ directive if > necessary, e.g. if you want to use the "gpio" state on other boards.) Hi, Sorry for the late reply. GPIO function is being defined as I2C recovery itself requires it, it will switch the pins to GPIO mode and then pulse SCL for a predefined number of pulses in order to try and recover the bus. So we cannot remove it from DTS and have I2C recovery working. Also, GPIO is a fully valid option for this pin group according to the datasheet, it is even the default until you switch them to I2C. > > Second: do you even want to do recovery on this platform then? > Is it necessary? What happens electronically in this case, if we don't > switch the pins to GPIO mode? Is it something akin to the "strict" > property in struct pinmux: that the GPIO block and the device can > affect the same pins at the same time? That warrants an explanation > and a comment. Yes, I2C recovery is required on this board as otherwise the I2C bus will get stuck after a certain number of SFP module plug/unplug events or sometimes even just randomly, I2C recovery allows the bus to recover and continue working. Maybe my commit message was confusing, so I will try and explain further. I2C recovery did work on Armada 3720 just fine until the driver was converted to use the generic I2C recovery which is now part of the I2C core. After it was converted to it, the I2C bus completely stopped working on Armada 3720 if I2C recovery is enabled by making the recovery pinctrl available in DTS. I then spent quite a while trying to bisect the exact change that causes this issue in the conversion as code is almost identical to what the driver was doing previously, and have bisected it down to pinctrl_select_state(bri->pinctrl, bri->pins_gpio) being called before SDA and SCL pins are obtained via devm_gpiod_get(). Then I thought that maybe there was a HW bug in the Armada 3720 as the pin function was being set to GPIO twice via register write so I made sure the register was being written to only if the desired value is different than the current one but that did not help at all. For whatever reason calling pinctrl_select_state(bri->pinctrl, bri->pins_gpio) causes the pins to basically lock up, but the weird thing is that calling devm_gpiod_get() will also end up with the kernel changing the pin function to GPIO and this works, hence this patch. This is basically the only change in the old driver behavior which would do: pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery); return pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default); After obtaining the SDA and SCL pins via devm_gpiod_get() and this somehow works while the generic I2C code will first call: pinctrl_select_state(bri->pinctrl, bri->pins_gpio) Then it obtains the SDA and SCL pins via devm_gpiod_get() and eventually at the end of i2c_gpio_init_generic_recovery() call pinctrl_select_state(bri->pinctrl, bri->pins_default); to return the pins back to I2C state. Hopefully, this all makes more sense. > > If you can't delete the "gpio" pin control state, I would add a > bool pinctrl_stay_in_device_mode; > to > struct i2c_bus_recovery_info > in include/linux/i2c.h > > And just: > > if (bri->pinctrl && !bri->pinctrl_stay_in_device_mode) > pinctrl_select_state(bri->pinctrl, bri->pins_gpio); > > (Also the switch to the "default" state further down could be > contitional !bri->pinctrl_stay_in_device_mode) > > But mostly I wonder why the "gpio" pin control state is defined, if it's > not to be used. Well, that is the thing, it is being used, it just somehow got broken with the move to generic I2C recovery. I hope to have explained the issue and symptoms more clearly now as I have ran out of ideas why a call to pinctrl_select_state() to change to GPIO state breaks things, but then devm_gpio_get() also eventually makes a call to the pinctrl driver to change the pin function to GPIO but that works. Regards, Robert > > Yours, > Linus Walleij
Hi Robert! Thanks for getting back on this issue. On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@sartura.hr> wrote: > Yes, I2C recovery is required on this board as otherwise the I2C bus will > get stuck after a certain number of SFP module plug/unplug events or > sometimes even just randomly, I2C recovery allows the bus to recover > and continue working. OK makes sense. > Maybe my commit message was confusing, so I will try and explain further. > I2C recovery did work on Armada 3720 just fine until the driver was converted > to use the generic I2C recovery which is now part of the I2C core. > > After it was converted to it, the I2C bus completely stopped working > on Armada 3720 > if I2C recovery is enabled by making the recovery pinctrl available in DTS. Shouldn't we just revert that patch until we can figure this out then? > I then spent quite a while trying to bisect the exact change that > causes this issue > in the conversion as code is almost identical to what the driver was > doing previously, > and have bisected it down to pinctrl_select_state(bri->pinctrl, > bri->pins_gpio) being > called before SDA and SCL pins are obtained via devm_gpiod_get(). > > Then I thought that maybe there was a HW bug in the Armada 3720 as the > pin function > was being set to GPIO twice via register write so I made sure the > register was being > written to only if the desired value is different than the current one > but that did not help > at all. > For whatever reason calling pinctrl_select_state(bri->pinctrl, > bri->pins_gpio) causes > the pins to basically lock up, but the weird thing is that calling > devm_gpiod_get() will > also end up with the kernel changing the pin function to GPIO and this > works, hence > this patch. That sounds like a race condition... such as if the pin control block is not clocked when the first pinctrl_select_state() comes in. Or maybe the i2c hardware needs to be initialized/clocked before touching it's mux? (Weird but could happen.) One thing that makes me a bit suspicious is this: i2c/busses/i2c-pxa.c: subsys_initcall(i2c_adap_pxa_init); While: pinctrl/mvebu/pinctrl-armada-37xx.c builtin_platform_driver_probe(armada_37xx_pinctrl_driver, armada_37xx_pinctrl_probe); If there is no probe reordering then the i2c driver will probe before pin control. What happens if you move the i2c_adap_pxa_init() call to module_init() instead of subsys_initcall()? Yours, Linus Walleij
On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote: > Hi Robert! > > Thanks for getting back on this issue. > > On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > Yes, I2C recovery is required on this board as otherwise the I2C bus will > > get stuck after a certain number of SFP module plug/unplug events or > > sometimes even just randomly, I2C recovery allows the bus to recover > > and continue working. > > OK makes sense. > > > Maybe my commit message was confusing, so I will try and explain further. > > I2C recovery did work on Armada 3720 just fine until the driver was converted > > to use the generic I2C recovery which is now part of the I2C core. > > > > After it was converted to it, the I2C bus completely stopped working > > on Armada 3720 > > if I2C recovery is enabled by making the recovery pinctrl available in DTS. > > Shouldn't we just revert that patch until we can figure this out then? Note that when I wrote the i2c-pxa recovery code (which was developed and tested on Armada 3720 - the uDPU) it had to work... when the suggestion came up to implement generic recovery, I stated: http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d > > I then spent quite a while trying to bisect the exact change that > > causes this issue > > in the conversion as code is almost identical to what the driver was > > doing previously, > > and have bisected it down to pinctrl_select_state(bri->pinctrl, > > bri->pins_gpio) being > > called before SDA and SCL pins are obtained via devm_gpiod_get(). Yes, indeed. That's because the pinctrl internals get confused. I sent you an email about it on 6th December 2019 "pinctrl states vs pinmux vs gpio (i2c bus recovery)" which is why i2c-pxa did things the way it did in my commit "i2c: pxa: implement generic i2c bus recovery".
On Thu, Nov 9, 2023 at 9:30 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote: > > > After it was converted to it, the I2C bus completely stopped working > > > on Armada 3720 > > > if I2C recovery is enabled by making the recovery pinctrl available in DTS. > > > > Shouldn't we just revert that patch until we can figure this out then? > > Note that when I wrote the i2c-pxa recovery code (which was developed > and tested on Armada 3720 - the uDPU) it had to work... when the > suggestion came up to implement generic recovery, I stated: > > http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d Makes me even more convinced that we should just revert this. i.e. commit 0b01392c18b9993a584f36ace1d61118772ad0ca i2c: pxa: move to generic GPIO recovery There is even: https://lore.kernel.org/linux-i2c/20201209204645.GF3499@kunai/ "In case we missed a glitch, we can still revert the patch later." Well this is later. Robert can you see if it possible to revert, that things work after a revert and send a revert patch? > > > I then spent quite a while trying to bisect the exact change that > > > causes this issue > > > in the conversion as code is almost identical to what the driver was > > > doing previously, > > > and have bisected it down to pinctrl_select_state(bri->pinctrl, > > > bri->pins_gpio) being > > > called before SDA and SCL pins are obtained via devm_gpiod_get(). > > Yes, indeed. That's because the pinctrl internals get confused. I sent > you an email about it on 6th December 2019 > > "pinctrl states vs pinmux vs gpio (i2c bus recovery)" I found it: https://lore.kernel.org/all/20191206173343.GX25745@shell.armlinux.org.uk/ Sadly I had no good advice for any simple elegant solutions to the problem, but the more complicated solution does work so let's go for that. > which is why i2c-pxa did things the way it did in my commit > "i2c: pxa: implement generic i2c bus recovery". I think we need to go back to this. It's nice with the ambition to create generic code of course, but sometimes it is better to just roll something IP-unique. Yours, Linus Walleij
On Thu, Nov 9, 2023 at 10:02 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Nov 9, 2023 at 9:30 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote: > > > > > After it was converted to it, the I2C bus completely stopped working > > > > on Armada 3720 > > > > if I2C recovery is enabled by making the recovery pinctrl available in DTS. > > > > > > Shouldn't we just revert that patch until we can figure this out then? > > > > Note that when I wrote the i2c-pxa recovery code (which was developed > > and tested on Armada 3720 - the uDPU) it had to work... when the > > suggestion came up to implement generic recovery, I stated: > > > > http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d > > Makes me even more convinced that we should just revert this. i.e. > commit 0b01392c18b9993a584f36ace1d61118772ad0ca > i2c: pxa: move to generic GPIO recovery > > There is even: > https://lore.kernel.org/linux-i2c/20201209204645.GF3499@kunai/ > > "In case we missed a glitch, we can still revert the patch later." > Well this is later. > > Robert can you see if it possible to revert, that things work after a > revert and send a revert patch? Hi, Yes, a revert still applies and "fixes" things so I2C starts working as before. I can send the revert tomorrow, I was just hoping that there was an bug that could be fixed instead of reverting, but seems its more complicated. Regards, Robert > > > > > I then spent quite a while trying to bisect the exact change that > > > > causes this issue > > > > in the conversion as code is almost identical to what the driver was > > > > doing previously, > > > > and have bisected it down to pinctrl_select_state(bri->pinctrl, > > > > bri->pins_gpio) being > > > > called before SDA and SCL pins are obtained via devm_gpiod_get(). > > > > Yes, indeed. That's because the pinctrl internals get confused. I sent > > you an email about it on 6th December 2019 > > > > "pinctrl states vs pinmux vs gpio (i2c bus recovery)" > > I found it: > https://lore.kernel.org/all/20191206173343.GX25745@shell.armlinux.org.uk/ > > Sadly I had no good advice for any simple elegant solutions > to the problem, but the more complicated solution does > work so let's go for that. > > > which is why i2c-pxa did things the way it did in my commit > > "i2c: pxa: implement generic i2c bus recovery". > > I think we need to go back to this. > > It's nice with the ambition to create generic code of course, but > sometimes it is better to just roll something IP-unique. > > Yours, > Linus Walleij
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 60746652fd52..b34d939078a1 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -359,13 +359,6 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap) if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery) return 0; - /* - * pins might be taken as GPIO, so we should inform pinctrl about - * this and move the state to GPIO - */ - if (bri->pinctrl) - pinctrl_select_state(bri->pinctrl, bri->pins_gpio); - /* * if there is incomplete or no recovery information, see if generic * GPIO recovery is available
Ever since PXA I2C driver was moved to the generic I2C recovery, I2C has stopped working completely on Armada 3720 if the pins are specified in DTS. After a while it was traced down to the only difference being that PXA driver did not change the pinmux state to GPIO before trying to acquire the GPIO pins. And indeed as soon as this call is removed I2C starts working. To me it seems that this call is not required at all as devm_gpiod_get() will result in the pinmux state being changed to GPIO via the pinmux set_mux() op. Fixes: 0b01392c18b9 ("i2c: pxa: move to generic GPIO recovery") Signed-off-by: Robert Marko <robert.marko@sartura.hr> --- drivers/i2c/i2c-core-base.c | 7 ------- 1 file changed, 7 deletions(-)