[PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[Gabor Juhos]

There is a long standing bug which causes I2C communication not to
work on the Armada 3700 based boards. This small series restores
that functionality.

Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
  - collect offered tags
  - rebase and retest on tip of i2c/for-current
  - Link to v1: https://lore.kernel.org/r/20250511-i2c-pxa-fix-i2c-communication-v1-0-e9097d09a015@gmail.com

---
Gabor Juhos (3):
      i2c: add init_recovery() callback
      i2c: pxa: prevent calling of the generic recovery init code
      i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

 drivers/i2c/busses/i2c-pxa.c | 25 +++++++++++++++++++------
 drivers/i2c/i2c-core-base.c  |  8 +++++++-
 include/linux/i2c.h          |  4 ++++
 3 files changed, 30 insertions(+), 7 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250510-i2c-pxa-fix-i2c-communication-3e6de1e3d0c6

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

[PATCH v2 1/3] i2c: add init_recovery() callback

[Gabor Juhos]

Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
and modify the i2c_init_recovery() function to call that if specified
instead of the generic i2c_gpio_init_recovery() function.

This allows controller drivers to skip calling the generic code by
implementing a dummy callback function, or alternatively to run a
fine tuned custom implementation.

This is needed for the 'i2c-pxa' driver in order to be able to fix
a long standing bug for which the fix will be implemented in a
followup patch.

Cc: stable@vger.kernel.org # 6.3+
Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
  - add 'Reviewed-by' tag from Linus Walleij
  - rebase on tip of i2c/for-current
---
 drivers/i2c/i2c-core-base.c | 8 +++++++-
 include/linux/i2c.h         | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ecca8c006b020379fb53293b20ab09ba25314609..e38be81dbcc17dd15947a189fd3b89def8529d1e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -427,12 +427,18 @@ static int i2c_init_recovery(struct i2c_adapter *adap)
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	bool is_error_level = true;
 	char *err_str;
+	int ret;
 
 	if (!bri)
 		return 0;
 
-	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
+	if (bri->init_recovery) {
+		ret = bri->init_recovery(adap);
+		if (ret)
+			return ret;
+	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
+	}
 
 	if (!bri->recover_bus) {
 		err_str = "no suitable method provided";
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 20fd41b51d5c85ee1665395c07345faafd8e2fca..4cefdd4c730fc9dc4655f6581f3dea64435d7124 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -636,6 +636,9 @@ struct i2c_timings {
  *	for generic GPIO recovery.
  * @get_bus_free: Returns the bus free state as seen from the IP core in case it
  *	has a more complex internal logic than just reading SDA. Optional.
+ * @init_recovery: If specified, it will be called instead of the generic GPIO
+ *	recovery initialization code. Platform may use a dummy callback to skip
+ *	calling the generic code, or it may use a custom implementation.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
@@ -660,6 +663,7 @@ struct i2c_bus_recovery_info {
 	void (*set_sda)(struct i2c_adapter *adap, int val);
 	int (*get_bus_free)(struct i2c_adapter *adap);
 
+	int (*init_recovery)(struct i2c_adapter *adap);
 	void (*prepare_recovery)(struct i2c_adapter *adap);
 	void (*unprepare_recovery)(struct i2c_adapter *adap);
 

-- 
2.50.1

Re: [PATCH v2 1/3] i2c: add init_recovery() callback

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
> and modify the i2c_init_recovery() function to call that if specified
> instead of the generic i2c_gpio_init_recovery() function.
> 
> This allows controller drivers to skip calling the generic code by
> implementing a dummy callback function, or alternatively to run a
> fine tuned custom implementation.
> 
> This is needed for the 'i2c-pxa' driver in order to be able to fix
> a long standing bug for which the fix will be implemented in a

> followup patch.

"...next change."

...

The first traditional question is why the generic recovery is not working.

...

> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> +	if (bri->init_recovery) {
> +		ret = bri->init_recovery(adap);
> +		if (ret)
> +			return ret;

> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
>  		return -EPROBE_DEFER;
> +	}

If the above stays, I think we would drop the last and always have
init_recovery to be assigned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] i2c: add init_recovery() callback

[Gabor Juhos]

2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
>> Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
>> and modify the i2c_init_recovery() function to call that if specified
>> instead of the generic i2c_gpio_init_recovery() function.
>>
>> This allows controller drivers to skip calling the generic code by
>> implementing a dummy callback function, or alternatively to run a
>> fine tuned custom implementation.
>>
>> This is needed for the 'i2c-pxa' driver in order to be able to fix
>> a long standing bug for which the fix will be implemented in a
> 
>> followup patch.
> 
> "...next change."

Ok.

> 
> ...
> 
> The first traditional question is why the generic recovery is not working.


The details are in the driver specific patches. Should I write it all down here too?

> 
> ...
> 
>> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> +	if (bri->init_recovery) {
>> +		ret = bri->init_recovery(adap);
>> +		if (ret)
>> +			return ret;
> 
>> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
>>  		return -EPROBE_DEFER;
>> +	}
> 
> If the above stays, I think we would drop the last and always have
> init_recovery to be assigned.
> 

In that case we would have something like this:

    if (!bri->init_recovery)
        bri->init_recovery = i2c_gpio_init_recovery;

    ret = bri->init_recovery(adap);
    if (ret)
        return ret;

Since the callback is used only once, and within the same fuction where it is
assigned, I don't really see the advantage of the assignment. Although it
definitely looks cleaner as far as error handling is concerned.

Originally, I have used the following solution:

    if (bri->init_recovery)
        ret = bri->init_recovery(adap);
    else
        ret = i2c_gpio_init_recovery(adap);

    if (ret)
        return ret;

However the existing code ignores errors from i2c_gpio_init_recovery() except
EPROBE_DEFER, so I changed this to the code proposed in the patch in order to
keep the existing behaviour.

Regards,
Gabor

Re: [PATCH v2 1/3] i2c: add init_recovery() callback

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 12:24:22PM +0200, Gabor Juhos wrote:
> 2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
> > On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:

...

> >> This is needed for the 'i2c-pxa' driver in order to be able to fix
> >> a long standing bug for which the fix will be implemented in a

The above left for some context for the below discussion.

...

> > The first traditional question is why the generic recovery is not working.
> 
> The details are in the driver specific patches. Should I write it all down here too?

Instead of the above paragraph, give a summary of your use case to answer 'why'
it can not be done differently.

...

> >> -	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> >> +	if (bri->init_recovery) {
> >> +		ret = bri->init_recovery(adap);
> >> +		if (ret)
> >> +			return ret;
> > 
> >> +	} else if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) {
> >>  		return -EPROBE_DEFER;
> >> +	}
> > 
> > If the above stays, I think we would drop the last and always have
> > init_recovery to be assigned.
> 
> In that case we would have something like this:
> 
>     if (!bri->init_recovery)
>         bri->init_recovery = i2c_gpio_init_recovery;
> 
>     ret = bri->init_recovery(adap);
>     if (ret)
>         return ret;
> 
> Since the callback is used only once, and within the same fuction where it is
> assigned, I don't really see the advantage of the assignment. Although it
> definitely looks cleaner as far as error handling is concerned.
> 
> Originally, I have used the following solution:
> 
>     if (bri->init_recovery)
>         ret = bri->init_recovery(adap);
>     else
>         ret = i2c_gpio_init_recovery(adap);

> 

Without this blank line...

>     if (ret)
>         return ret;

...this looks like the best compromise among proposed implementations.

> However the existing code ignores errors from i2c_gpio_init_recovery() except
> EPROBE_DEFER, so I changed this to the code proposed in the patch in order to
> keep the existing behaviour.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] i2c: add init_recovery() callback

[Gabor Juhos]

2025. 08. 13. 15:06 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:24:22PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 22:17 keltezéssel, Andy Shevchenko írta:
>>> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> 
> ...
> 
>>>> This is needed for the 'i2c-pxa' driver in order to be able to fix
>>>> a long standing bug for which the fix will be implemented in a
> 
> The above left for some context for the below discussion.
> 
> ...
> 
>>> The first traditional question is why the generic recovery is not working.
>>
>> The details are in the driver specific patches. Should I write it all down here too?
> 
> Instead of the above paragraph, give a summary of your use case to answer 'why'
> it can not be done differently.

Ok.

> 
> ...
> 

...

>>
>> Originally, I have used the following solution:
>>
>>     if (bri->init_recovery)
>>         ret = bri->init_recovery(adap);
>>     else
>>         ret = i2c_gpio_init_recovery(adap);
> 
>>
> 
> Without this blank line...
> 
>>     if (ret)
>>         return ret;
> 
> ...this looks like the best compromise among proposed implementations.

Ok, I will use this in the next version.

Regards,
Gabor

Re: [PATCH v2 1/3] i2c: add init_recovery() callback

[Russell King (Oracle)]

On Mon, Aug 11, 2025 at 11:17:41PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 11, 2025 at 09:49:55PM +0200, Gabor Juhos wrote:
> > Add a new init_recovery() callback to struct 'i2c_bus_recovery_info'
> > and modify the i2c_init_recovery() function to call that if specified
> > instead of the generic i2c_gpio_init_recovery() function.
> > 
> > This allows controller drivers to skip calling the generic code by
> > implementing a dummy callback function, or alternatively to run a
> > fine tuned custom implementation.
> > 
> > This is needed for the 'i2c-pxa' driver in order to be able to fix
> > a long standing bug for which the fix will be implemented in a
> 
> > followup patch.
> 
> "...next change."
> 
> ...
> 
> The first traditional question is why the generic recovery is not working.

I opposed the conversion of my recovery code to generic recovery when
it was first mooted for this driver, and was over-ruled. Lo and behold,
as I predicted, generic recovery fails with i2c-pxa. I now don't
remember the details, but there has been a regression reported...

Given that I was over-ruled, I am not minded to go back and try and
find either the previous discussion (google can be exceedingly
difficult now to find such history) nor to try and work it out again
(I've other things, including meetings today.)

Nevertheless, all I now remember is that generic recovery breaks
i2c-pxa, whereas my recovery works.

If I had to guess, I suspect it's something to do with how careful I
was to ensure a glitchless transition between i2c mode and GPIO mode
on the pinmux, and the generic recovery probably isn't as careful, but
I could be wrong there.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Gabor Juhos]

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 conrtoller is getting enabled in i2c_pxa_reset(),
whereas in the case of the generic initialization it happens after that.

To resolve the problem, provide an empty init_recovery() callback
function thus preventing the I2C core to call the generic recovery
initialization code.

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.305277] i2c i2c-0:  PXA I2C adapter
  [    7.310198] i2c i2c-1:  PXA I2C adapter
  [    7.315012] sfp sfp-eth1: Host maximum power 3.0W
  [    7.324061] lm75 1-0048: supply vs not found, using dummy regulator
  [    7.331738] sfp sfp-eth0: Host maximum power 3.0W
  [    7.337000] hwmon hwmon0: temp1_input not attached to any thermal zone
  [    7.343593] lm75 1-0048: hwmon0: sensor 'tmp75c'
  [    7.348526] lm75 1-0049: supply vs not found, using dummy regulator
  [    7.356858] hwmon hwmon1: temp1_input not attached to any thermal zone
  [    7.363463] lm75 1-0049: hwmon1: sensor 'tmp75c'
  ...
  [    7.730315] sfp sfp-eth1: module Mikrotik         S-RJ01           rev 1.0  sn 61B103C55C58     dc 201022
  [    7.840318] sfp sfp-eth0: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
  [    7.850083] mvneta d0030000.ethernet eth0: unsupported SFP module: no common interface modes
  [    7.990335] hwmon hwmon2: temp1_input not attached to any thermal zone

[1] https://lore.kernel.org/r/20230926160255.330417-1-robert.marko@sartura.hr

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>
Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
---
Changes in v2:
  - rebase and retest on tip of i2c/for-current
---
 drivers/i2c/busses/i2c-pxa.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 968a8b8794dac3398a68d827c567aa5bb73ae3d7..f4cb36e22077753e21b0260df52b8bdbb85fa308 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1330,6 +1330,12 @@ static void i2c_pxa_unprepare_recovery(struct i2c_adapter *adap)
 	i2c_pxa_enable(i2c);
 }
 
+static int i2c_pxa_init_recovery_cb(struct i2c_adapter *adap)
+{
+	/* We have initialized everything already, so nothing to do here. */
+	return 0;
+}
+
 static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
 {
 	struct i2c_bus_recovery_info *bri = &i2c->recovery;
@@ -1398,6 +1404,7 @@ static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
 		return 0;
 	}
 
+	bri->init_recovery = i2c_pxa_init_recovery_cb;
 	bri->prepare_recovery = i2c_pxa_prepare_recovery;
 	bri->unprepare_recovery = i2c_pxa_unprepare_recovery;
 	bri->recover_bus = i2c_generic_scl_recovery;

-- 
2.50.1

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

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos 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 conrtoller is getting enabled in i2c_pxa_reset(),
> whereas in the case of the generic initialization it happens after that.
> 
> To resolve the problem, provide an empty init_recovery() callback
> function thus preventing the I2C core to call the generic recovery
> initialization code.
> 
> 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.305277] i2c i2c-0:  PXA I2C adapter
>   [    7.310198] i2c i2c-1:  PXA I2C adapter
>   [    7.315012] sfp sfp-eth1: Host maximum power 3.0W
>   [    7.324061] lm75 1-0048: supply vs not found, using dummy regulator
>   [    7.331738] sfp sfp-eth0: Host maximum power 3.0W
>   [    7.337000] hwmon hwmon0: temp1_input not attached to any thermal zone
>   [    7.343593] lm75 1-0048: hwmon0: sensor 'tmp75c'
>   [    7.348526] lm75 1-0049: supply vs not found, using dummy regulator
>   [    7.356858] hwmon hwmon1: temp1_input not attached to any thermal zone
>   [    7.363463] lm75 1-0049: hwmon1: sensor 'tmp75c'
>   ...
>   [    7.730315] sfp sfp-eth1: module Mikrotik         S-RJ01           rev 1.0  sn 61B103C55C58     dc 201022
>   [    7.840318] sfp sfp-eth0: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>   [    7.850083] mvneta d0030000.ethernet eth0: unsupported SFP module: no common interface modes
>   [    7.990335] hwmon hwmon2: temp1_input not attached to any thermal zone

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.

And I didn't get how recovery code affects the initialisation (enumeration). Do we
set pin control state back and forth during probe? May be this is a root cause?

...

> [1] https://lore.kernel.org/r/20230926160255.330417-1-robert.marko@sartura.hr
> 

Can you make this a Link tag?
Link: $URL #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>
> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>

...

>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
>  {
>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;

>  		return 0;
>  	}
>  
> +	bri->init_recovery = i2c_pxa_init_recovery_cb;

This is unfortunate. I would keep the naming schema consistent, i.e. rename
existing function and use its original name for the new callback.

>  	bri->prepare_recovery = i2c_pxa_prepare_recovery;
>  	bri->unprepare_recovery = i2c_pxa_unprepare_recovery;
>  	bri->recover_bus = i2c_generic_scl_recovery;

-- 
With Best Regards,
Andy Shevchenko

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

[Gabor Juhos]

2025. 08. 11. 22:26 keltezéssel, Andy Shevchenko írta:
> On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos 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 conrtoller is getting enabled in i2c_pxa_reset(),
>> whereas in the case of the generic initialization it happens after that.
>>
>> To resolve the problem, provide an empty init_recovery() callback
>> function thus preventing the I2C core to call the generic recovery
>> initialization code.
>>
>> 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.305277] i2c i2c-0:  PXA I2C adapter
>>   [    7.310198] i2c i2c-1:  PXA I2C adapter
>>   [    7.315012] sfp sfp-eth1: Host maximum power 3.0W
>>   [    7.324061] lm75 1-0048: supply vs not found, using dummy regulator
>>   [    7.331738] sfp sfp-eth0: Host maximum power 3.0W
>>   [    7.337000] hwmon hwmon0: temp1_input not attached to any thermal zone
>>   [    7.343593] lm75 1-0048: hwmon0: sensor 'tmp75c'
>>   [    7.348526] lm75 1-0049: supply vs not found, using dummy regulator
>>   [    7.356858] hwmon hwmon1: temp1_input not attached to any thermal zone
>>   [    7.363463] lm75 1-0049: hwmon1: sensor 'tmp75c'
>>   ...
>>   [    7.730315] sfp sfp-eth1: module Mikrotik         S-RJ01           rev 1.0  sn 61B103C55C58     dc 201022
>>   [    7.840318] sfp sfp-eth0: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>>   [    7.850083] mvneta d0030000.ethernet eth0: unsupported SFP module: no common interface modes
>>   [    7.990335] hwmon hwmon2: temp1_input not attached to any thermal zone
> 
> 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.


> Do we set pin control state back and forth during probe? May be this is a root cause?

Yes, basically. The state gets changed back and forth twice. Once in driver
probe before the controller gets initialized, then once again in
i2c_gpio_init_generic_recovery(). The problem is caused by the second state
change as it runs after the controller gets enabled which confuses the hardware.

> ...
> 
>> [1] https://lore.kernel.org/r/20230926160255.330417-1-robert.marko@sartura.hr
>>
> 
> Can you make this a Link tag?
> Link: $URL #1

Sure, I can change that.

>> 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>
>> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
> 
> ...
> 
>>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
>>  {
>>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;
> 
>>  		return 0;
>>  	}
>>  
>> +	bri->init_recovery = i2c_pxa_init_recovery_cb;
> 
> This is unfortunate. I would keep the naming schema consistent, i.e. rename
> existing function and use its original name for the new callback.

I agree, but since the change is targeted also to stable kernels, I wanted to
keep the change as minimal as possible.

> 
>>  	bri->prepare_recovery = i2c_pxa_prepare_recovery;
>>  	bri->unprepare_recovery = i2c_pxa_unprepare_recovery;
>>  	bri->recover_bus = i2c_generic_scl_recovery;
> 

Reards,
Gabor

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

[Andy Shevchenko]

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. Why is
recovery involved in probe? This is quite confusing...

> > Do we set pin control state back and forth during probe? May be this is a root cause?
> 
> Yes, basically. The state gets changed back and forth twice. Once in driver
> probe before the controller gets initialized, then once again in
> i2c_gpio_init_generic_recovery(). The problem is caused by the second state
> change as it runs after the controller gets enabled which confuses the hardware.

...

> >>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
> >>  {
> >>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;
> > 
> >>  		return 0;
> >>  	}
> >>  
> >> +	bri->init_recovery = i2c_pxa_init_recovery_cb;
> > 
> > This is unfortunate. I would keep the naming schema consistent, i.e. rename
> > existing function and use its original name for the new callback.
> 
> I agree, but since the change is targeted also to stable kernels, I wanted to
> keep the change as minimal as possible.

Renaming is not a big deal AFAICS, but leaving this _cb will be confusing in a
long term. I prefer name to be changed.

> >>  	bri->prepare_recovery = i2c_pxa_prepare_recovery;
> >>  	bri->unprepare_recovery = i2c_pxa_unprepare_recovery;
> >>  	bri->recover_bus = i2c_generic_scl_recovery;

-- 
With Best Regards,
Andy Shevchenko

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

[Gabor Juhos]

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.

> 
>>> Do we set pin control state back and forth during probe? May be this is a root cause?
>>
>> Yes, basically. The state gets changed back and forth twice. Once in driver
>> probe before the controller gets initialized, then once again in
>> i2c_gpio_init_generic_recovery(). The problem is caused by the second state
>> change as it runs after the controller gets enabled which confuses the hardware.
> 
> ...
> 
>>>>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
>>>>  {
>>>>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;
>>>
>>>>  		return 0;
>>>>  	}
>>>>  
>>>> +	bri->init_recovery = i2c_pxa_init_recovery_cb;
>>>
>>> This is unfortunate. I would keep the naming schema consistent, i.e. rename
>>> existing function and use its original name for the new callback.
>>
>> I agree, but since the change is targeted also to stable kernels, I wanted to
>> keep the change as minimal as possible.
> 
> Renaming is not a big deal AFAICS, but leaving this _cb will be confusing in a
> long term. I prefer name to be changed.

Ok, will change the name.

Regards,
Gabor

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

[Russell King (Oracle)]

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. 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Gabor Juhos]

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.


Regards,
Gabor

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Gabor Juhos]

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.

[PATCH v2 3/3] i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

[Gabor Juhos]

Under some circumstances I2C recovery fails on Armada 3700. At least
on the Methode uDPU board, removing and replugging an SFP module fails
often, like this:

  [   36.953127] sfp sfp-eth1: module removed
  [   38.468549] i2c i2c-1: i2c_pxa: timeout waiting for bus free
  [   38.486960] sfp sfp-eth1: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
  [   38.496867] mvneta d0040000.ethernet eth1: unsupported SFP module: no common interface modes
  [   38.521448] hwmon hwmon2: temp1_input not attached to any thermal zone
  [   39.249196] sfp sfp-eth1: module removed
  ...
  [  292.568799] sfp sfp-eth1: please wait, module slow to respond
  ...
  [  625.208814] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO

Note that the 'unsupported SFP module' messages are not relevant. The
module is used only for testing the I2C recovery funcionality, because
the error can be triggered easily with this specific one.

Enabling debug in the i2c-pxa driver reveals the following:

  [   82.034678] sfp sfp-eth1: module removed
  [   90.008654] i2c i2c-1: slave_0x50 error: timeout with active message
  [   90.015112] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
  [   90.020464] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
  [   90.027906] i2c i2c-1: log:
  [   90.030787]

This continues until the retries are exhausted ...

  [  110.192489] i2c i2c-1: slave_0x50 error: exhausted retries
  [  110.198012] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
  [  110.203323] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
  [  110.210810] i2c i2c-1: log:
  [  110.213633]

... then the whole sequence starts again ...

  [  115.368641] i2c i2c-1: slave_0x50 error: timeout with active message

... while finally the SFP core gives up:

  [  671.975258] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO

When we analyze the log, it can be seen that bit 1 and 11 is set in the
ISR (Interface Status Register). Bit 1 indicates the ACK/NACK status, but
the purpose of bit 11 is not documented in the driver code unfortunately.

The 'Functional Specification' document of the Armada 3700 SoCs family
however says that this bit indicates an 'Early Bus Busy' condition. The
document also notes that whenever this bit is set, it is not possible to
initiate a transaction on the I2C bus. The observed behaviour corresponds
to this statement.

Unfortunately, I2C recovery does not help as it never runs in this
special case. Although the driver checks the busyness of the bus at
several places, but since it does not consider the A3700 specific bit
in these checks it can't determine the actual status of the bus correctly
which results in the errors above.

In order to fix the problem, add a new member to struct 'i2c_pxa' to
store a controller specific bitmask containing the bits indicating the
busy status, and use that in the code while checking the actual status
of the bus. This ensures that the correct status can be determined on
the Armada 3700 based devices without causing functional changes on
devices based on other SoCs.

With the change applied, the driver detects the busy condition, and runs
the recovery process:

  [  742.617312] i2c i2c-1: state:i2c_pxa_wait_bus_not_busy:449: ISR=00000802, ICR=000007e0, IBMR=03
  [  742.626099] i2c i2c-1: i2c_pxa: timeout waiting for bus free
  [  742.631933] i2c i2c-1: recovery: resetting controller, ISR=0x00000802
  [  742.638421] i2c i2c-1: recovery: IBMR 0x00000003 ISR 0x00000000

This clears the EBB bit in the ISR register, so it makes it possible to
initiate transactions on the I2C bus again.

After this patch, the SFP module used for testing can be removed and
replugged numerous times without causing the error described at the
beginning. Previously, the error happened after a few such attempts.

The patch has been tested also with the following kernel versions:
5.10.237, 5.15.182, 6.1.138, 6.6.90, 6.12.28, 6.14.6. It improves
recoverabilty on all of them.

Cc: stable@vger.kernel.org # 5.8+
Fixes: 7c9ec2c52518 ("i2c: pxa: implement generic i2c bus recovery")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
---
Changes in v2:
  - rebase and retest on tip of i2c/for-current

Note: the patch is included in this series for completeness however
it can be applied independently from the preceding patches. On kernels
6.3+, it restores I2C functionality even in itself because it recovers
the controller from the bad state described in the previous patch.
---
 drivers/i2c/busses/i2c-pxa.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index f4cb36e22077753e21b0260df52b8bdbb85fa308..6239200631211b2b6c9919d27fec63704bd19e52 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -71,6 +71,7 @@
 #define ISR_GCAD	(1 << 8)	   /* general call address detected */
 #define ISR_SAD		(1 << 9)	   /* slave address detected */
 #define ISR_BED		(1 << 10)	   /* bus error no ACK/NAK */
+#define ISR_A3700_EBB	(1 << 11)	   /* early bus busy for armada 3700 */
 
 #define ILCR_SLV_SHIFT		0
 #define ILCR_SLV_MASK		(0x1FF << ILCR_SLV_SHIFT)
@@ -263,6 +264,7 @@ struct pxa_i2c {
 	bool			highmode_enter;
 	u32			fm_mask;
 	u32			hs_mask;
+	u32			busy_mask;
 
 	struct i2c_bus_recovery_info recovery;
 	struct pinctrl		*pinctrl;
@@ -429,7 +431,7 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
 
 	while (1) {
 		isr = readl(_ISR(i2c));
-		if (!(isr & (ISR_IBB | ISR_UB)))
+		if (!(isr & i2c->busy_mask))
 			return 0;
 
 		if (isr & ISR_SAD)
@@ -466,7 +468,7 @@ static int i2c_pxa_wait_master(struct pxa_i2c *i2c)
 		 * quick check of the i2c lines themselves to ensure they've
 		 * gone high...
 		 */
-		if ((readl(_ISR(i2c)) & (ISR_UB | ISR_IBB)) == 0 &&
+		if ((readl(_ISR(i2c)) & i2c->busy_mask) == 0 &&
 		    readl(_IBMR(i2c)) == (IBMR_SCLS | IBMR_SDAS)) {
 			if (i2c_debug > 0)
 				dev_dbg(&i2c->adap.dev, "%s: done\n", __func__);
@@ -487,7 +489,7 @@ static int i2c_pxa_set_master(struct pxa_i2c *i2c)
 	if (i2c_debug)
 		dev_dbg(&i2c->adap.dev, "setting to bus master\n");
 
-	if ((readl(_ISR(i2c)) & (ISR_UB | ISR_IBB)) != 0) {
+	if ((readl(_ISR(i2c)) & i2c->busy_mask) != 0) {
 		dev_dbg(&i2c->adap.dev, "%s: unit is busy\n", __func__);
 		if (!i2c_pxa_wait_master(i2c)) {
 			dev_dbg(&i2c->adap.dev, "%s: error: unit busy\n", __func__);
@@ -513,7 +515,7 @@ static int i2c_pxa_wait_slave(struct pxa_i2c *i2c)
 			dev_dbg(&i2c->adap.dev, "%s: %ld: ISR=%08x, ICR=%08x, IBMR=%02x\n",
 				__func__, (long)jiffies, readl(_ISR(i2c)), readl(_ICR(i2c)), readl(_IBMR(i2c)));
 
-		if ((readl(_ISR(i2c)) & (ISR_UB|ISR_IBB)) == 0 ||
+		if ((readl(_ISR(i2c)) & i2c->busy_mask) == 0 ||
 		    (readl(_ISR(i2c)) & ISR_SAD) != 0 ||
 		    (readl(_ICR(i2c)) & ICR_SCLE) == 0) {
 			if (i2c_debug > 1)
@@ -1171,7 +1173,7 @@ static int i2c_pxa_pio_set_master(struct pxa_i2c *i2c)
 	/*
 	 * Wait for the bus to become free.
 	 */
-	while (timeout-- && readl(_ISR(i2c)) & (ISR_IBB | ISR_UB))
+	while (timeout-- && readl(_ISR(i2c)) & i2c->busy_mask)
 		udelay(1000);
 
 	if (timeout < 0) {
@@ -1316,7 +1318,7 @@ static void i2c_pxa_unprepare_recovery(struct i2c_adapter *adap)
 	 * handing control of the bus back to avoid the bus changing state.
 	 */
 	isr = readl(_ISR(i2c));
-	if (isr & (ISR_UB | ISR_IBB)) {
+	if (isr & i2c->busy_mask) {
 		dev_dbg(&i2c->adap.dev,
 			"recovery: resetting controller, ISR=0x%08x\n", isr);
 		i2c_pxa_do_reset(i2c);
@@ -1480,6 +1482,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	i2c->fm_mask = pxa_reg_layout[i2c_type].fm;
 	i2c->hs_mask = pxa_reg_layout[i2c_type].hs;
 
+	i2c->busy_mask = ISR_UB | ISR_IBB;
+	if (i2c_type == REGS_A3700)
+		i2c->busy_mask |= ISR_A3700_EBB;
+
 	if (i2c_type != REGS_CE4100)
 		i2c->reg_isar = i2c->reg_base + pxa_reg_layout[i2c_type].isar;
 

-- 
2.50.1

Re: [PATCH v2 3/3] i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 09:49:57PM +0200, Gabor Juhos wrote:
> Under some circumstances I2C recovery fails on Armada 3700. At least
> on the Methode uDPU board, removing and replugging an SFP module fails
> often, like this:
> 
>   [   36.953127] sfp sfp-eth1: module removed
>   [   38.468549] i2c i2c-1: i2c_pxa: timeout waiting for bus free
>   [   38.486960] sfp sfp-eth1: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>   [   38.496867] mvneta d0040000.ethernet eth1: unsupported SFP module: no common interface modes
>   [   38.521448] hwmon hwmon2: temp1_input not attached to any thermal zone
>   [   39.249196] sfp sfp-eth1: module removed
>   ...
>   [  292.568799] sfp sfp-eth1: please wait, module slow to respond
>   ...
>   [  625.208814] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
> 
> Note that the 'unsupported SFP module' messages are not relevant. The
> module is used only for testing the I2C recovery funcionality, because
> the error can be triggered easily with this specific one.
> 
> Enabling debug in the i2c-pxa driver reveals the following:
> 
>   [   82.034678] sfp sfp-eth1: module removed
>   [   90.008654] i2c i2c-1: slave_0x50 error: timeout with active message
>   [   90.015112] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
>   [   90.020464] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
>   [   90.027906] i2c i2c-1: log:
>   [   90.030787]
> 
> This continues until the retries are exhausted ...
> 
>   [  110.192489] i2c i2c-1: slave_0x50 error: exhausted retries
>   [  110.198012] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
>   [  110.203323] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
>   [  110.210810] i2c i2c-1: log:
>   [  110.213633]
> 
> ... then the whole sequence starts again ...
> 
>   [  115.368641] i2c i2c-1: slave_0x50 error: timeout with active message
> 
> ... while finally the SFP core gives up:
> 
>   [  671.975258] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
> 
> When we analyze the log, it can be seen that bit 1 and 11 is set in the
> ISR (Interface Status Register). Bit 1 indicates the ACK/NACK status, but
> the purpose of bit 11 is not documented in the driver code unfortunately.
> 
> The 'Functional Specification' document of the Armada 3700 SoCs family
> however says that this bit indicates an 'Early Bus Busy' condition. The
> document also notes that whenever this bit is set, it is not possible to
> initiate a transaction on the I2C bus. The observed behaviour corresponds
> to this statement.
> 
> Unfortunately, I2C recovery does not help as it never runs in this
> special case. Although the driver checks the busyness of the bus at
> several places, but since it does not consider the A3700 specific bit
> in these checks it can't determine the actual status of the bus correctly
> which results in the errors above.
> 
> In order to fix the problem, add a new member to struct 'i2c_pxa' to
> store a controller specific bitmask containing the bits indicating the
> busy status, and use that in the code while checking the actual status
> of the bus. This ensures that the correct status can be determined on
> the Armada 3700 based devices without causing functional changes on
> devices based on other SoCs.
> 
> With the change applied, the driver detects the busy condition, and runs
> the recovery process:
> 
>   [  742.617312] i2c i2c-1: state:i2c_pxa_wait_bus_not_busy:449: ISR=00000802, ICR=000007e0, IBMR=03
>   [  742.626099] i2c i2c-1: i2c_pxa: timeout waiting for bus free
>   [  742.631933] i2c i2c-1: recovery: resetting controller, ISR=0x00000802
>   [  742.638421] i2c i2c-1: recovery: IBMR 0x00000003 ISR 0x00000000
> 
> This clears the EBB bit in the ISR register, so it makes it possible to
> initiate transactions on the I2C bus again.
> 
> After this patch, the SFP module used for testing can be removed and
> replugged numerous times without causing the error described at the
> beginning. Previously, the error happened after a few such attempts.
> 
> The patch has been tested also with the following kernel versions:
> 5.10.237, 5.15.182, 6.1.138, 6.6.90, 6.12.28, 6.14.6. It improves
> recoverabilty on all of them.

...

> Note: the patch is included in this series for completeness however
> it can be applied independently from the preceding patches. On kernels
> 6.3+, it restores I2C functionality even in itself because it recovers
> the controller from the bad state described in the previous patch.

Sounds to me like this one should be applied first independently on the
discussion / conclusion on the patch 1.

...

Code wise it looks reasonable to me, but I haven't reviewed it properly
and wouldn't probably have a time, that's why no tags.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

[Gabor Juhos]

2025. 08. 11. 22:31 keltezéssel, Andy Shevchenko írta:
> On Mon, Aug 11, 2025 at 09:49:57PM +0200, Gabor Juhos wrote:
>> Under some circumstances I2C recovery fails on Armada 3700. At least
>> on the Methode uDPU board, removing and replugging an SFP module fails
>> often, like this:
>>
>>   [   36.953127] sfp sfp-eth1: module removed
>>   [   38.468549] i2c i2c-1: i2c_pxa: timeout waiting for bus free
>>   [   38.486960] sfp sfp-eth1: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>>   [   38.496867] mvneta d0040000.ethernet eth1: unsupported SFP module: no common interface modes
>>   [   38.521448] hwmon hwmon2: temp1_input not attached to any thermal zone
>>   [   39.249196] sfp sfp-eth1: module removed
>>   ...
>>   [  292.568799] sfp sfp-eth1: please wait, module slow to respond
>>   ...
>>   [  625.208814] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
>>
>> Note that the 'unsupported SFP module' messages are not relevant. The
>> module is used only for testing the I2C recovery funcionality, because
>> the error can be triggered easily with this specific one.
>>
>> Enabling debug in the i2c-pxa driver reveals the following:
>>
>>   [   82.034678] sfp sfp-eth1: module removed
>>   [   90.008654] i2c i2c-1: slave_0x50 error: timeout with active message
>>   [   90.015112] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
>>   [   90.020464] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
>>   [   90.027906] i2c i2c-1: log:
>>   [   90.030787]
>>
>> This continues until the retries are exhausted ...
>>
>>   [  110.192489] i2c i2c-1: slave_0x50 error: exhausted retries
>>   [  110.198012] i2c i2c-1: msg_num: 2 msg_idx: 0 msg_ptr: 0
>>   [  110.203323] i2c i2c-1: IBMR: 00000003 IDBR: 000000a0 ICR: 000007e0 ISR: 00000802
>>   [  110.210810] i2c i2c-1: log:
>>   [  110.213633]
>>
>> ... then the whole sequence starts again ...
>>
>>   [  115.368641] i2c i2c-1: slave_0x50 error: timeout with active message
>>
>> ... while finally the SFP core gives up:
>>
>>   [  671.975258] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
>>
>> When we analyze the log, it can be seen that bit 1 and 11 is set in the
>> ISR (Interface Status Register). Bit 1 indicates the ACK/NACK status, but
>> the purpose of bit 11 is not documented in the driver code unfortunately.
>>
>> The 'Functional Specification' document of the Armada 3700 SoCs family
>> however says that this bit indicates an 'Early Bus Busy' condition. The
>> document also notes that whenever this bit is set, it is not possible to
>> initiate a transaction on the I2C bus. The observed behaviour corresponds
>> to this statement.
>>
>> Unfortunately, I2C recovery does not help as it never runs in this
>> special case. Although the driver checks the busyness of the bus at
>> several places, but since it does not consider the A3700 specific bit
>> in these checks it can't determine the actual status of the bus correctly
>> which results in the errors above.
>>
>> In order to fix the problem, add a new member to struct 'i2c_pxa' to
>> store a controller specific bitmask containing the bits indicating the
>> busy status, and use that in the code while checking the actual status
>> of the bus. This ensures that the correct status can be determined on
>> the Armada 3700 based devices without causing functional changes on
>> devices based on other SoCs.
>>
>> With the change applied, the driver detects the busy condition, and runs
>> the recovery process:
>>
>>   [  742.617312] i2c i2c-1: state:i2c_pxa_wait_bus_not_busy:449: ISR=00000802, ICR=000007e0, IBMR=03
>>   [  742.626099] i2c i2c-1: i2c_pxa: timeout waiting for bus free
>>   [  742.631933] i2c i2c-1: recovery: resetting controller, ISR=0x00000802
>>   [  742.638421] i2c i2c-1: recovery: IBMR 0x00000003 ISR 0x00000000
>>
>> This clears the EBB bit in the ISR register, so it makes it possible to
>> initiate transactions on the I2C bus again.
>>
>> After this patch, the SFP module used for testing can be removed and
>> replugged numerous times without causing the error described at the
>> beginning. Previously, the error happened after a few such attempts.
>>
>> The patch has been tested also with the following kernel versions:
>> 5.10.237, 5.15.182, 6.1.138, 6.6.90, 6.12.28, 6.14.6. It improves
>> recoverabilty on all of them.
> 
> ...
> 
>> Note: the patch is included in this series for completeness however
>> it can be applied independently from the preceding patches. On kernels
>> 6.3+, it restores I2C functionality even in itself because it recovers
>> the controller from the bad state described in the previous patch.
> 
> Sounds to me like this one should be applied first independently on the
> discussion / conclusion on the patch 1.

Yes. At least the users would have a working although not optimal solution in
the meantime.

Regards,
Gabor

Re: [PATCH v2 3/3] i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 12:50:35PM +0200, Gabor Juhos wrote:
> 2025. 08. 11. 22:31 keltezéssel, Andy Shevchenko írta:
> > On Mon, Aug 11, 2025 at 09:49:57PM +0200, Gabor Juhos wrote:

...

> >> Note: the patch is included in this series for completeness however
> >> it can be applied independently from the preceding patches. On kernels
> >> 6.3+, it restores I2C functionality even in itself because it recovers
> >> the controller from the bad state described in the previous patch.
> > 
> > Sounds to me like this one should be applied first independently on the
> > discussion / conclusion on the patch 1.
> 
> Yes. At least the users would have a working although not optimal solution in
> the meantime.

Then please reorder in the next version.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] i2c: pxa: handle 'Early Bus Busy' condition on Armada 3700

[Gabor Juhos]

2025. 08. 13. 15:11 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:50:35PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 22:31 keltezéssel, Andy Shevchenko írta:
>>> On Mon, Aug 11, 2025 at 09:49:57PM +0200, Gabor Juhos wrote:
> 

...

>>>
>>> Sounds to me like this one should be applied first independently on the
>>> discussion / conclusion on the patch 1.
>>
>> Yes. At least the users would have a working although not optimal solution in
>> the meantime.
> 
> Then please reorder in the next version.

Ok.

Regards,
Gabor

Re: [PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[Andy Shevchenko]

On Mon, Aug 11, 2025 at 09:49:54PM +0200, Gabor Juhos wrote:
> There is a long standing bug which causes I2C communication not to
> work on the Armada 3700 based boards. This small series restores
> that functionality.
> 
> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>

I didn't get this SoB chain. Who is Imre here and what is his role in all this?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[Gabor Juhos]

2025. 08. 11. 22:12 keltezéssel, Andy Shevchenko írta:
> On Mon, Aug 11, 2025 at 09:49:54PM +0200, Gabor Juhos wrote:
>> There is a long standing bug which causes I2C communication not to
>> work on the Armada 3700 based boards. This small series restores
>> that functionality.
>>
>> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> 
> I didn't get this SoB chain. Who is Imre here and what is his role in all this?
> 
> 

Imre reviewed the patches before publishing those, but we were unsure about that
the Reviewed-by tag can be used when it is offered privately before the
publicaton, so we decided to use the SoB tag instead.

It can be changed to the Rewieved-by tag if that is applicable in this case.

Regards,
Gabor

Re: [PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[Andy Shevchenko]

On Wed, Aug 13, 2025 at 12:13:26PM +0200, Gabor Juhos wrote:
> 2025. 08. 11. 22:12 keltezéssel, Andy Shevchenko írta:
> > On Mon, Aug 11, 2025 at 09:49:54PM +0200, Gabor Juhos wrote:

...

> >> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
> >> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > 
> > I didn't get this SoB chain. Who is Imre here and what is his role in all this?
> 
> Imre reviewed the patches before publishing those, but we were unsure about that
> the Reviewed-by tag can be used when it is offered privately before the
> publicaton, so we decided to use the SoB tag instead.

It's exactly what should have been done. As a such the SoB chain is confusing
(taking into account the authorship of the patches).

> It can be changed to the Rewieved-by tag if that is applicable in this case.

Please do so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] i2c: pxa: fix I2C communication on Armada 3700

[Gabor Juhos]

2025. 08. 13. 15:02 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:13:26PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 22:12 keltezéssel, Andy Shevchenko írta:
>>> On Mon, Aug 11, 2025 at 09:49:54PM +0200, Gabor Juhos wrote:
> 
> ...
> 
>>>> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>
>>>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>>>
>>> I didn't get this SoB chain. Who is Imre here and what is his role in all this?
>>
>> Imre reviewed the patches before publishing those, but we were unsure about that
>> the Reviewed-by tag can be used when it is offered privately before the
>> publicaton, so we decided to use the SoB tag instead.
> 
> It's exactly what should have been done. As a such the SoB chain is confusing
> (taking into account the authorship of the patches).

Thanks for confirming!

> 
>> It can be changed to the Rewieved-by tag if that is applicable in this case.
> 
> Please do so.

Ok.

Regards,
Gabor