| Message ID | CAOMZO5DvGF5OW6fGQocZcFf+6103OhOyUCRdWGLBKbewWOOLHw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pca953x: Probing too early | expand |
On Tue, Aug 20, 2024 at 11:05 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? So, it should return deferred probe when adapter is not available? At least this is how we solve such an issue in ACPI case. I believe you are talking DT here?
On Tue, Aug 20, 2024 at 05:02:28PM -0300, Fabio Estevam 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 what cause probe failure? I think it should be defer probe if some resource are not ready yet. Frank > [ 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? > > Thanks, > > 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().
On Tue, Aug 20, 2024 at 5:45 PM Frank Li <Frank.li@nxp.com> wrote: > what cause probe failure? I think it should be defer probe if some resource > are not ready yet. The failure happens because i2c-2 is probed after the pca953x driver: [ 1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11 [ 1.905575] i2c i2c-2: IMX I2C adapter registered PCA953x tries to write to a register, but the I2C bus is not ready yet. At which point in the driver I should check for the I2C controller presence and return -EPROBE_DEFER if it is not ready? Thanks
On Tue, Aug 20, 2024 at 05:50:38PM -0300, Fabio Estevam wrote: > On Tue, Aug 20, 2024 at 5:45 PM Frank Li <Frank.li@nxp.com> wrote: > > > what cause probe failure? I think it should be defer probe if some resource > > are not ready yet. > > The failure happens because i2c-2 is probed after the pca953x driver: > > [ 1.898258] pca953x 2-0020: probe with driver pca953x failed with error -11 > [ 1.905575] i2c i2c-2: IMX I2C adapter registered > > PCA953x tries to write to a register, but the I2C bus is not ready yet. I am not sure how this happen. i2c device should be probed by i2c bus driver. Only after i2c bus probe, which scan i2c device, then i2c device driver probe can be called. Frank > > At which point in the driver I should check for the I2C controller > presence and return -EPROBE_DEFER if it is not ready? > > Thanks
On Tue, Aug 20, 2024 at 6:04 PM Frank Li <Frank.li@nxp.com> wrote: > I am not sure how this happen. i2c device should be probed by i2c bus > driver. Only after i2c bus probe, which scan i2c device, then i2c device > driver probe can be called. Exactly. This is my understanding as well. I don't understand how the PCA953X driver can probe before its I2C parent. For now, I am registering imx-i2c like this: --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -1639,17 +1639,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); No errors are seen so far. What do you think?
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
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.
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?
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
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.
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
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.
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
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
--- 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.