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.1On 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
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
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
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
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!
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
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!
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.
© 2016 - 2025 Red Hat, Inc.