Re: [PATCH v3 1/2] i2c: pxa: defer reset on Armada 3700 when recovery is used

[Robert Marko <robert.marko@sartura.hr>]

On Wed, Aug 27, 2025 at 7:14 PM Gabor Juhos <j4g8y7@gmail.com> wrote:
>
> The I2C communication is completely broken on the Armada 3700 platform
> since commit 0b01392c18b9 ("i2c: pxa: move to generic GPIO recovery").
>
> For example, on the Methode uDPU board, probing of the two onboard
> temperature sensors fails ...
>
>   [    7.271713] i2c i2c-0: using pinctrl states for GPIO recovery
>   [    7.277503] i2c i2c-0:  PXA I2C adapter
>   [    7.282199] i2c i2c-1: using pinctrl states for GPIO recovery
>   [    7.288241] i2c i2c-1:  PXA I2C adapter
>   [    7.292947] sfp sfp-eth1: Host maximum power 3.0W
>   [    7.299614] sfp sfp-eth0: Host maximum power 3.0W
>   [    7.308178] lm75 1-0048: supply vs not found, using dummy regulator
>   [   32.489631] lm75 1-0048: probe with driver lm75 failed with error -121
>   [   32.496833] lm75 1-0049: supply vs not found, using dummy regulator
>   [   82.890614] lm75 1-0049: probe with driver lm75 failed with error -121
>
> ... and accessing the plugged-in SFP modules also does not work:
>
>   [  511.298537] sfp sfp-eth1: please wait, module slow to respond
>   [  536.488530] sfp sfp-eth0: please wait, module slow to respond
>   ...
>   [ 1065.688536] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
>   [ 1090.888532] sfp sfp-eth0: failed to read EEPROM: -EREMOTEIO
>
> After a discussion [1], there was an attempt to fix the problem by
> reverting the offending change by commit 7b211c767121 ("Revert "i2c:
> pxa: move to generic GPIO recovery""), but that only helped to fix
> the issue in the 6.1.y stable tree. The reason behind the partial succes
> is that there was another change in commit 20cb3fce4d60 ("i2c: Set i2c
> pinctrl recovery info from it's device pinctrl") in the 6.3-rc1 cycle
> which broke things further.
>
> The cause of the problem is the same in case of both offending commits
> mentioned above. Namely, the I2C core code changes the pinctrl state to
> GPIO while running the recovery initialization code. Although the PXA
> specific initialization also does this, but the key difference is that
> it happens before the controller is getting enabled in i2c_pxa_reset(),
> whereas in the case of the generic initialization it happens after that.
>
> Change the code to reset the controller only before the first transfer
> instead of before registering the controller. This ensures that the
> controller is not enabled at the time when the generic recovery code
> performs the pinctrl state changes, thus avoids the problem described
> above.
>
> As the result this change restores the original behaviour, which in
> turn makes the I2C communication to work again as it can be seen from
> the following log:
>
>   [    7.363250] i2c i2c-0: using pinctrl states for GPIO recovery
>   [    7.369041] i2c i2c-0:  PXA I2C adapter
>   [    7.373673] i2c i2c-1: using pinctrl states for GPIO recovery
>   [    7.379742] i2c i2c-1:  PXA I2C adapter
>   [    7.384506] sfp sfp-eth1: Host maximum power 3.0W
>   [    7.393013] sfp sfp-eth0: Host maximum power 3.0W
>   [    7.399266] lm75 1-0048: supply vs not found, using dummy regulator
>   [    7.407257] hwmon hwmon0: temp1_input not attached to any thermal zone
>   [    7.413863] lm75 1-0048: hwmon0: sensor 'tmp75c'
>   [    7.418746] lm75 1-0049: supply vs not found, using dummy regulator
>   [    7.426371] hwmon hwmon1: temp1_input not attached to any thermal zone
>   [    7.432972] lm75 1-0049: hwmon1: sensor 'tmp75c'
>   [    7.755092] sfp sfp-eth1: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>   [    7.764997] mvneta d0040000.ethernet eth1: unsupported SFP module: no common interface modes
>   [    7.785362] sfp sfp-eth0: module Mikrotik         S-RJ01           rev 1.0  sn 61B103C55C58     dc 201022
>   [    7.803426] hwmon hwmon2: temp1_input not attached to any thermal zone
>
> Link: https://lore.kernel.org/r/20230926160255.330417-1-robert.marko@sartura.hr #1
>
> Cc: stable@vger.kernel.org # 6.3+
> Fixes: 20cb3fce4d60 ("i2c: Set i2c pinctrl recovery info from it's device pinctrl")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---

Tested-by: Robert Marko <robert.marko@sartura.hr>

> Changes in v3:
>   - rebase on tip of i2c/for-current
>   - rework the patch and use a different approach which does not requires
>     modification in the I2C core code and update commit description
>     acccordingly
>   - remove Imre's SoB tag, it should have been a Reviewed-by tag, but due
>     to the rework this is an entirely different patch so that does not
>     apply anyway
>   - use Link tag for the URL of the referenced LKML thread
>   - Link to v2: https://lore.kernel.org/r/20250811-i2c-pxa-fix-i2c-communication-v2-2-ca42ea818dc9@gmail.com
>
> Changes in v2:
>   - rebase and retest on tip of i2c/for-current
>   - Link to v1: https://lore.kernel.org/r/20250511-i2c-pxa-fix-i2c-communication-v1-2-e9097d09a015@gmail.com
> ---
>  drivers/i2c/busses/i2c-pxa.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 968a8b8794dac3398a68d827c567aa5bb73ae3d7..70acf33e1d573231f84a1f09cffb376a8277351d 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -268,6 +268,7 @@ struct pxa_i2c {
>         struct pinctrl          *pinctrl;
>         struct pinctrl_state    *pinctrl_default;
>         struct pinctrl_state    *pinctrl_recovery;
> +       bool                    reset_before_xfer;
>  };
>
>  #define _IBMR(i2c)     ((i2c)->reg_ibmr)
> @@ -1144,6 +1145,11 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap,
>  {
>         struct pxa_i2c *i2c = adap->algo_data;
>
> +       if (i2c->reset_before_xfer) {
> +               i2c_pxa_reset(i2c);
> +               i2c->reset_before_xfer = false;
> +       }
> +
>         return i2c_pxa_internal_xfer(i2c, msgs, num, i2c_pxa_do_xfer);
>  }
>
> @@ -1521,7 +1527,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
>                 }
>         }
>
> -       i2c_pxa_reset(i2c);
> +       /*
> +        * Skip reset on Armada 3700 when recovery is used to avoid
> +        * controller hang due to the pinctrl state changes done by
> +        * the generic recovery initialization code. The reset will
> +        * be performed later, prior to the first transfer.
> +        */
> +       if (i2c_type == REGS_A3700 && i2c->adap.bus_recovery_info)
> +               i2c->reset_before_xfer = true;
> +       else
> +               i2c_pxa_reset(i2c);
>
>         ret = i2c_add_numbered_adapter(&i2c->adap);
>         if (ret < 0)
>
> --
> 2.50.1
>


-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura d.d.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr