On Thu, Sep 28, 2023 at 11:11 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@xxxxxxxxxx> 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 -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@xxxxxxxxxx Web: www.sartura.hr
