Re: pca953x: Probing too early

Re: pca953x: Probing too early

[Fabio Estevam]

Adding the i2c-folks on Cc.

On Tue, Aug 20, 2024 at 5:02 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi,
>
> I am seeing an issue with the PCA935X driver in 6.6.41 and
> 6.11.0-rc4-next-20240820.
>
> The pca953x is getting probed before its I2C parent (i2c-2):
>
> [    1.872917] pca953x 2-0020: supply vcc not found, using dummy regulator
> [    1.889195] pca953x 2-0020: using no AI
> [    1.893260] pca953x 2-0020: failed writing register
> [    1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11
> [    1.905575] i2c i2c-2: IMX I2C adapter registered
>
> This problem is seen on a custom imx8mp board.
> I am not able to reproduce it on an imx8mm-evk.
>
> If I select the pca953x as a module or insert a delay inside its
> probe() function, it probes successfully.
>
> The drivers/gpio/gpio-pca953x.c has the following comments:
>
> /* register after i2c postcore initcall and before
>  * subsys initcalls that may rely on these GPIOs
>  */
> subsys_initcall(pca953x_init);
>
> but it seems this is not happening.
>
> I have also tried to register it like this:
>
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -1369,21 +1369,7 @@ static struct i2c_driver pca953x_driver = {
>         .remove         = pca953x_remove,
>         .id_table       = pca953x_id,
>  };
> -
> -static int __init pca953x_init(void)
> -{
> -       return i2c_add_driver(&pca953x_driver);
> -}
> -/* register after i2c postcore initcall and before
> - * subsys initcalls that may rely on these GPIOs
> - */
> -subsys_initcall(pca953x_init);
> -
> -static void __exit pca953x_exit(void)
> -{
> -       i2c_del_driver(&pca953x_driver);
> -}
> -module_exit(pca953x_exit);
> +module_i2c_driver(pca953x_driver);
> )
>
> but this did not help either.
>
> Does anyone have any suggestions on how to fix this problem when the
> pca953x driver is built-in?

If I register the i2c-imx driver like this:

--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1638,18 +1638,7 @@ static struct platform_driver i2c_imx_driver = {
        },
        .id_table = imx_i2c_devtype,
 };
-
-static int __init i2c_adap_imx_init(void)
-{
-       return platform_driver_register(&i2c_imx_driver);
-}
-subsys_initcall(i2c_adap_imx_init);
-
-static void __exit i2c_adap_imx_exit(void)
-{
-       platform_driver_unregister(&i2c_imx_driver);
-}
-module_exit(i2c_adap_imx_exit);
+module_platform_driver(i2c_imx_driver);

then the pca953x driver probes correctly.

:~/stable/linux$ git grep subsys_initcall drivers/i2c/ | wc -l
15

:~/stable/linux$ git grep module_platform_driver drivers/i2c/ | wc -l
75

Most of the I2C drivers are registered as module_platform_driver().

Re: pca953x: Probing too early

[Andrew Lunn]

On Tue, Aug 20, 2024 at 05:47:27PM -0300, Fabio Estevam wrote:
> Adding the i2c-folks on Cc.
> 
> On Tue, Aug 20, 2024 at 5:02 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi,
> >
> > I am seeing an issue with the PCA935X driver in 6.6.41 and
> > 6.11.0-rc4-next-20240820.
> >
> > The pca953x is getting probed before its I2C parent (i2c-2):
> >
> > [    1.872917] pca953x 2-0020: supply vcc not found, using dummy regulator
> > [    1.889195] pca953x 2-0020: using no AI
> > [    1.893260] pca953x 2-0020: failed writing register
> > [    1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11

-11 is EAGAIN, which is a bit odd. Given your description, i would of
expected ENODEV. My guess is, it needs another resource, a GPIO,
regulator, or interrupt controller. That resources might not of probed
yet. If that is true, you want the pca953x_probe() to return
-EPROBE_DEFER. The driver core will then try the probe again sometime
later, hopefully when all the needed resources are available.

Track down where the EAGAIN is coming from.

      Andrew

Re: pca953x: Probing too early

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 12:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Aug 20, 2024 at 05:47:27PM -0300, Fabio Estevam wrote:
> > Adding the i2c-folks on Cc.
> > On Tue, Aug 20, 2024 at 5:02 PM Fabio Estevam <festevam@gmail.com> wrote:

> > > I am seeing an issue with the PCA935X driver in 6.6.41 and
> > > 6.11.0-rc4-next-20240820.
> > >
> > > The pca953x is getting probed before its I2C parent (i2c-2):
> > >
> > > [    1.872917] pca953x 2-0020: supply vcc not found, using dummy regulator
> > > [    1.889195] pca953x 2-0020: using no AI
> > > [    1.893260] pca953x 2-0020: failed writing register
> > > [    1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11
>
> -11 is EAGAIN, which is a bit odd. Given your description, i would of
> expected ENODEV. My guess is, it needs another resource, a GPIO,
> regulator, or interrupt controller. That resources might not of probed
> yet. If that is true, you want the pca953x_probe() to return
> -EPROBE_DEFER. The driver core will then try the probe again sometime
> later, hopefully when all the needed resources are available.
>
> Track down where the EAGAIN is coming from.

I bet on __i2c_smbus_xfer(). Which comes from
https://elixir.bootlin.com/linux/v6.11-rc4/source/drivers/i2c/busses/i2c-imx.c#L549.
Very easy to check is to add at the top of i2c-imx.c the following

#undef EAGAIN
#define EAGAIN __LINE__

and search for the respective line in suspicious files.

-- 
With Best Regards,
Andy Shevchenko

Re: pca953x: Probing too early

[Fabio Estevam]

Hi Andy,

On Tue, Aug 20, 2024 at 7:07 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> I bet on __i2c_smbus_xfer(). Which comes from
> https://elixir.bootlin.com/linux/v6.11-rc4/source/drivers/i2c/busses/i2c-imx.c#L549.

Yes, this is correct.

> Very easy to check is to add at the top of i2c-imx.c the following
>
> #undef EAGAIN
> #define EAGAIN __LINE__
>
> and search for the respective line in suspicious files.

That's an useful hint, thanks.

Re: pca953x: Probing too early

[Russell King (Oracle)]

On Tue, Aug 20, 2024 at 11:29:07PM +0200, Andrew Lunn wrote:
> On Tue, Aug 20, 2024 at 05:47:27PM -0300, Fabio Estevam wrote:
> > Adding the i2c-folks on Cc.
> > 
> > On Tue, Aug 20, 2024 at 5:02 PM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > I am seeing an issue with the PCA935X driver in 6.6.41 and
> > > 6.11.0-rc4-next-20240820.
> > >
> > > The pca953x is getting probed before its I2C parent (i2c-2):
> > >
> > > [    1.872917] pca953x 2-0020: supply vcc not found, using dummy regulator
> > > [    1.889195] pca953x 2-0020: using no AI
> > > [    1.893260] pca953x 2-0020: failed writing register
> > > [    1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11
> 
> -11 is EAGAIN, which is a bit odd. Given your description, i would of
> expected ENODEV. My guess is, it needs another resource, a GPIO,
> regulator, or interrupt controller. That resources might not of probed
> yet. If that is true, you want the pca953x_probe() to return
> -EPROBE_DEFER. The driver core will then try the probe again sometime
> later, hopefully when all the needed resources are available.
> 
> Track down where the EAGAIN is coming from.

This is where:

        ret = regmap_bulk_write(chip->regmap, regaddr, value, NBANK(chip));
        if (ret < 0) {
                dev_err(&chip->client->dev, "failed writing register\n");

printing the error code in error messages would really help debugging.
Sadly, people don't do this. I don't know why we don't bulk replace
all error messages with just "Error!\n" to make them even more cryptic
and undebuggable!

It's likely that EAGAIN is coming from this - the probe function calls
one of the init functions, and propagates the error up, and as that
message is being printed... Tracing down, the I2C transfer function
returns -EAGAIN if it fails the transfer, and __i2c_smbus_xfer() will
itself retry it a number of times before propagating that -EAGAIN up.

EAGAIN is supposed to only be generated on arbitration loss - I'm
guessing that the I2C bus is in some kind of locked state, meaning
that devices on this bus are not accessible. Maybe the I2C bus
pull-ups aren't powered? Maybe there's a bad device on the bus
pulling the bus down?

Someone mentioned i2c-imx, maybe try enabling debug in that driver
to see why it's failing to access the device?

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

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

Re: pca953x: Probing too early

[Fabio Estevam]

Hi Russell,

On Tue, Aug 20, 2024 at 7:22 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:

> This is where:
>
>         ret = regmap_bulk_write(chip->regmap, regaddr, value, NBANK(chip));
>         if (ret < 0) {
>                 dev_err(&chip->client->dev, "failed writing register\n");

Yes, correct. This is where -EAGAIN is coming from:

[    1.745657] pca953x 2-0020: supply vcc not found, using dummy regulator
[    1.752502] pca953x 2-0020: using no AI
[    1.756579] pca953x 2-0020: **** failed writing register: -11
[    1.762510] pca953x: probe of 2-0020 failed with error -11
[    1.768298] i2c i2c-2: IMX I2C adapter registered

The pca953x driver tries to write to the i2c-2 bus before i2c-2 is registered.

This is the point I don't understand: how can the pca953x driver get
probed before its I2C bus parent?

Thanks

Re: pca953x: Probing too early

[Fabio Estevam]

On Tue, Aug 20, 2024 at 8:18 PM Fabio Estevam <festevam@gmail.com> wrote:

> The pca953x driver tries to write to the i2c-2 bus before i2c-2 is registered.
>
> This is the point I don't understand: how can the pca953x driver get
> probed before its I2C bus parent?

Disconsider what I wrote above.

I'm trying to recover from the arbitration lost like this:

--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -491,6 +491,8 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
*i2c_imx, int for_busy, bool a
                /* check for arbitration lost */
                if (temp & I2SR_IAL) {
                        i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
+                       pr_err("******* Arbitration lost\n");
+                       i2c_recover_bus(&i2c_imx->adapter);
                        return -EAGAIN;
                }

@@ -1487,6 +1489,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
        i2c_imx->adapter.nr             = pdev->id;
        i2c_imx->adapter.dev.of_node    = pdev->dev.of_node;
        i2c_imx->base                   = base;
+       i2c_imx->adapter.retries        = 5;
        ACPI_COMPANION_SET(&i2c_imx->adapter.dev, ACPI_COMPANION(&pdev->dev));

but still get pca953x probe failure:

[    1.756761] pca953x 2-0020: supply vcc not found, using dummy regulator
[    1.766564] pca953x 2-0020: using no AI
[    1.775333] ******* Arbitration lost
[    1.783811] ******* Arbitration lost
[    1.793701] ******* Arbitration lost
[    1.797455] ******* Arbitration lost
[    1.801209] ******* Arbitration lost
[    1.804964] ******* Arbitration lost
[    1.808562] pca953x 2-0020: failed writing register
[    1.813602] pca953x: probe of 2-0020 failed with error -11
[    1.819222] i2c i2c-2: IMX I2C adapter registered

Re: pca953x: Probing too early

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 3:50 AM Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Aug 20, 2024 at 8:18 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> > The pca953x driver tries to write to the i2c-2 bus before i2c-2 is registered.
> >
> > This is the point I don't understand: how can the pca953x driver get
> > probed before its I2C bus parent?

It's just messages. The clients are getting probed inside the
adapter's probe phase.

> Disconsider what I wrote above.
>
> I'm trying to recover from the arbitration lost like this:
>
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -491,6 +491,8 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> *i2c_imx, int for_busy, bool a
>                 /* check for arbitration lost */
>                 if (temp & I2SR_IAL) {
>                         i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> +                       pr_err("******* Arbitration lost\n");
> +                       i2c_recover_bus(&i2c_imx->adapter);
>                         return -EAGAIN;
>                 }
>
> @@ -1487,6 +1489,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>         i2c_imx->adapter.nr             = pdev->id;
>         i2c_imx->adapter.dev.of_node    = pdev->dev.of_node;
>         i2c_imx->base                   = base;
> +       i2c_imx->adapter.retries        = 5;
>         ACPI_COMPANION_SET(&i2c_imx->adapter.dev, ACPI_COMPANION(&pdev->dev));
>
> but still get pca953x probe failure:
>
> [    1.756761] pca953x 2-0020: supply vcc not found, using dummy regulator
> [    1.766564] pca953x 2-0020: using no AI
> [    1.775333] ******* Arbitration lost
> [    1.783811] ******* Arbitration lost
> [    1.793701] ******* Arbitration lost
> [    1.797455] ******* Arbitration lost
> [    1.801209] ******* Arbitration lost
> [    1.804964] ******* Arbitration lost
> [    1.808562] pca953x 2-0020: failed writing register
> [    1.813602] pca953x: probe of 2-0020 failed with error -11
> [    1.819222] i2c i2c-2: IMX I2C adapter registered

I think you should check that the adapter is powered on, to me it
sounds like you get 0xffffffff in temp or so.

-- 
With Best Regards,
Andy Shevchenko

Re: pca953x: Probing too early

[Oleksij Rempel]

Hi Fabio,

On Tue, Aug 20, 2024 at 09:50:04PM -0300, Fabio Estevam wrote:
> On Tue, Aug 20, 2024 at 8:18 PM Fabio Estevam <festevam@gmail.com> wrote:
> 
> > The pca953x driver tries to write to the i2c-2 bus before i2c-2 is registered.
> >
> > This is the point I don't understand: how can the pca953x driver get
> > probed before its I2C bus parent?
> 
> Disconsider what I wrote above.
> 
> I'm trying to recover from the arbitration lost like this:
> 
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -491,6 +491,8 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> *i2c_imx, int for_busy, bool a
>                 /* check for arbitration lost */
>                 if (temp & I2SR_IAL) {
>                         i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> +                       pr_err("******* Arbitration lost\n");
> +                       i2c_recover_bus(&i2c_imx->adapter);
>                         return -EAGAIN;
>                 }
> 
> @@ -1487,6 +1489,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>         i2c_imx->adapter.nr             = pdev->id;
>         i2c_imx->adapter.dev.of_node    = pdev->dev.of_node;
>         i2c_imx->base                   = base;
> +       i2c_imx->adapter.retries        = 5;
>         ACPI_COMPANION_SET(&i2c_imx->adapter.dev, ACPI_COMPANION(&pdev->dev));
> 
> but still get pca953x probe failure:
> 
> [    1.756761] pca953x 2-0020: supply vcc not found, using dummy regulator
> [    1.766564] pca953x 2-0020: using no AI
> [    1.775333] ******* Arbitration lost
> [    1.783811] ******* Arbitration lost
> [    1.793701] ******* Arbitration lost
> [    1.797455] ******* Arbitration lost
> [    1.801209] ******* Arbitration lost
> [    1.804964] ******* Arbitration lost
> [    1.808562] pca953x 2-0020: failed writing register
> [    1.813602] pca953x: probe of 2-0020 failed with error -11
> [    1.819222] i2c i2c-2: IMX I2C adapter registered

Do you have a multi master i2c bus? If not, can you please test
following patch:
https://lore.kernel.org/all/20240715151824.90033-2-eichest@gmail.com/

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: pca953x: Probing too early

[Fabio Estevam]

Hi Oleksij,

On Wed, Aug 21, 2024 at 3:05 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Do you have a multi master i2c bus? If not, can you please test
> following patch:
> https://lore.kernel.org/all/20240715151824.90033-2-eichest@gmail.com/

Yes, this fixes the problem, thanks!

I will reply in that thread with my Tested-by.

Thanks everyone for the help.

Cheers