Re: [PATCH v2 2/3] i2c: pxa: prevent calling of the generic recovery init code

[Gabor Juhos <j4g8y7@gmail.com>]

2025. 08. 17. 17:53 keltezéssel, Russell King (Oracle) írta:
> On Sun, Aug 17, 2025 at 04:59:22PM +0200, Gabor Juhos wrote:
>> 2025. 08. 13. 17:28 keltezéssel, Russell King (Oracle) írta:
>>> On Wed, Aug 13, 2025 at 05:17:28PM +0200, Gabor Juhos wrote:
>>>> 2025. 08. 13. 15:10 keltezéssel, Andy Shevchenko írta:
>>>>> On Wed, Aug 13, 2025 at 12:36:45PM +0200, Gabor Juhos wrote:
>>>>>> 2025. 08. 11. 22:26 keltezéssel, Andy Shevchenko írta:
>>>>>>> On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>> TBH this sounds to me like trying to hack the solution and as you pointed out
>>>>>>> the problem is in pinctrl state changes. I think it may affect not only I2C case.
>>>>>>
>>>>>> It is not an error in the pinctrl code. I have checked and even fixed a few bugs
>>>>>> in that.
>>>>>>
>>>>>>> And I didn't get how recovery code affects the initialisation (enumeration).
>>>>>>
>>>>>> Without the fix, it is not possible to initiate a transaction on the bus, which
>>>>>> in turn prevents enumeration.
>>>>>
>>>>> But why? As you said below the first pin control state is changed during the
>>>>> probe, which is fine, and the culprit one happens on the recovery.
>>>>
>>>> Erm, no. Both happens during probe, before the I2C core tries to enumerate the
>>>> devices on the bus.
>>>>
>>>>> Why is recovery involved in probe? This is quite confusing...
>>>> Let me try to explain it differently. Here is the simplified call chain:
>>>>
>>>>   i2c_pxa_probe()
>>>>      ...
>>>>      i2c_pxa_init_recovery()
>>>>         pinctrl_select_state()                  <- selects GPIO state
>>>>         pinctrl_select_state()                  <- selects default (I2C) state
>>>>      ...
>>>>      i2c_add_numbered_adapter()
>>>>          i2c_register_adapter()
>>>>              ...
>>>>              i2c_init_recovery()
>>>>                  i2c_gpio_init_recovery()
>>>>                      i2c_gpio_init_generic_recovery()
>>>>                          pinctrl_select_state() <- selects GPIO state***
>>>>                          ...
>>>>                          pinctrl_select_state() <- selects default (I2C) state
>>>>              ...
>>>>              bus_for_each_drv()
>>>>                  __process_new_adapter()
>>>>                      i2c_do_add_adapter()
>>>>                          i2c_detect()           <- enumerates the devices
>>>>
>>>> The culprit is the first pinctrl_select_state() call in
>>>> i2c_gpio_init_generic_recovery() marked with '***'.
>>>>
>>>> That call causes the controller to go stuck, which makes it impossible to
>>>> transfer anything on the bus.
>>>
>>> Probably because when GPIO state is selected, the I2C bus pins end up
>>> being set low, which the I2C controller sees, so it thinks there's
>>> another device communicating on the bus.
>>
>> Yes, it seems so.
>>
>> When GPIO state is selected, the bits in the Bus Monitor register which are
>> continuously reflecting the value of the SCL and SDA pins contains zeros.
>>
>> Additionally, the Status register indicates an 'Early Bus Busy' condition, which
>> means that 'The SCL or SDA line is low, without a Start condition'.
>>
>>
>>> I could be wrong, as I don't have the hardware to hand to research
>>> the issue again.
>>>
>>> I have a vague memory that the GPIO state must _always_ reflect the
>>> actual pin state before switching to it to avoid glitches and avoid
>>> inadvertently changing the I2C controller state.
>>
>> Unfortunately, it only helps to avoid glitches on the external lines. At least,
>> in the current case the controller hungs no matter which value combination is
>> being set on the GPIO pins before switching to GPIO state.
> 
> Note that my original i2c-pxa recovery implementation was proven
> functional on the uDPU, both by myself and Telus.
> 

No doubt, and that is why we want to restore the original behaviour.

I just wanted to indicate, that the approach used during recovery, does not help
to avoid the current hung.

In other words, the i2c_gpio_init_generic_recovery() simply does this:

        pinctrl_select_state(bri->pinctrl, bri->pins_gpio);

But even if we replace that with the following code (copied from
i2c_pxa_prepare_recovery()) ...

        u32 ibmr = readl(_IBMR(i2c));

        /*
         * Program the GPIOs to reflect the current I2C bus state while
         * we transition to recovery; this avoids glitching the bus.
         */
        gpiod_set_value(i2c->recovery.scl_gpiod, ibmr & IBMR_SCLS);
        gpiod_set_value(i2c->recovery.sda_gpiod, ibmr & IBMR_SDAS);

        WARN_ON(pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery));

... the controller still hangs once the pinctrl state is changed.