Message-ID: <8cb62eb9-9137-44b4-86f6-82f69813e83f@gmail.com>
Date: Wed, 13 Aug 2025 12:36:45 +0200
From: Gabor Juhos <j4g8y7@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Wolfram Sang <wsa@...nel.org>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Andi Shyti <andi.shyti@...nel.org>, Russell King
 <rmk+kernel@...linux.org.uk>, Andrew Lunn <andrew@...n.ch>,
 Hanna Hawa <hhhawa@...zon.com>, Robert Marko <robert.marko@...tura.hr>,
 Linus Walleij <linus.walleij@...aro.org>, linux-i2c@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Imre Kaloz <kaloz@...nwrt.org>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/3] i2c: pxa: prevent calling of the generic recovery
 init code

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@...r.kernel.org # 6.3+
>> Fixes: 20cb3fce4d60 ("i2c: Set i2c pinctrl recovery info from it's device pinctrl")
>> Signed-off-by: Gabor Juhos <j4g8y7@...il.com>
>> Signed-off-by: Imre Kaloz <kaloz@...nwrt.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

Powered by blists - more mailing lists