| Message ID | 20241127083818.2108201-1-carlos.song@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] i2c: imx: support DMA defer probing | expand |
Hello Carlos, On 27.11.24 09:38, carlos.song@nxp.com wrote: > From: Carlos Song <carlos.song@nxp.com> > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver > have not ready yet. > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > and "Documentation/driver-api/driver-model/driver.rst". > > Use CPU mode to avoid stuck registering i2c adapter when DMA resources > are unavailable. Please try to address open questions before sending new versions of the patch set. Otherwise, it's difficult to follow the conversation. Did you see my question[1] on your v2: | Wouldn't this break probe for all i2c-imx users who have CONFIG_IMX_SDMA | disabled? | | Also I am wondering on what kernel version and what configuration | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run into | this situation. | | I'd have expected that with fw_devlink enabled, the I2C controller wouldn't | be probed before the DMA provider is available. [1]: https://lore.kernel.org/all/19a43db4-db5c-4638-9778-d94fb571a206@pengutronix.de/ Thanks, Ahma > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > --- > Change for V4: > - Output "Only use PIO mode" log in debug level when no DMA configure. > Change for V3: > - According to Marc's comment, remove error print when defer probe. > Add info log when DMA has not been enabled and add error log when > DMA error, both won't stuck the i2c adapter register, just for reminding, > i2c adapter is working only in PIO mode. > Change for V2: > - According to Frank's comments, wrap at 75 char and Simplify fix code > at i2c_imx_dma_request(). > - Use strict patch check, fix alignment warning at i2c_imx_dma_request() > --- > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 5ed4cb61e262..b11d66d56c55 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx) > } > > /* Functions for DMA support */ > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > - dma_addr_t phy_addr) > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr) > { > struct imx_i2c_dma *dma; > struct dma_slave_config dma_sconfig; > - struct device *dev = &i2c_imx->adapter.dev; > + struct device *dev = i2c_imx->adapter.dev.parent; > int ret; > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); > if (!dma) > - return; > + return -ENOMEM; > > dma->chan_tx = dma_request_chan(dev, "tx"); > if (IS_ERR(dma->chan_tx)) { > @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", > dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx)); > > - return; > + return 0; > > fail_rx: > dma_release_channel(dma->chan_rx); > @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > dma_release_channel(dma->chan_tx); > fail_al: > devm_kfree(dev, dma); > + > + return ret; > } > > static void i2c_imx_dma_callback(void *arg) > @@ -1803,6 +1804,23 @@ static int i2c_imx_probe(struct platform_device *pdev) > if (ret == -EPROBE_DEFER) > goto clk_notifier_unregister; > > + /* > + * Init DMA config if supported, -ENODEV means DMA not enabled at > + * this platform, that is not a real error, so just remind "only > + * PIO mode is used". If DMA is enabled, but meet error when request > + * DMA channel, error should be showed in probe error log. PIO mode > + * should be available regardless of DMA. > + */ > + ret = i2c_imx_dma_request(i2c_imx, phy_addr); > + if (ret) { > + if (ret == -EPROBE_DEFER) > + goto clk_notifier_unregister; > + else if (ret == -ENODEV) > + dev_dbg(&pdev->dev, "Only use PIO mode\n"); > + else > + dev_err_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n"); > + } > + > /* Add I2C adapter */ > ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > if (ret < 0) > @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device *pdev) > i2c_imx->adapter.name); > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); > > - /* Init DMA config if supported */ > - i2c_imx_dma_request(i2c_imx, phy_addr); > - > return 0; /* Return OK */ > > clk_notifier_unregister:
On 27.11.2024 16:38:18, carlos.song@nxp.com wrote: > From: Carlos Song <carlos.song@nxp.com> > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver > have not ready yet. > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > and "Documentation/driver-api/driver-model/driver.rst". > > Use CPU mode to avoid stuck registering i2c adapter when DMA resources > are unavailable. > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > --- > Change for V4: > - Output "Only use PIO mode" log in debug level when no DMA configure. > Change for V3: > - According to Marc's comment, remove error print when defer probe. > Add info log when DMA has not been enabled and add error log when > DMA error, both won't stuck the i2c adapter register, just for reminding, > i2c adapter is working only in PIO mode. > Change for V2: > - According to Frank's comments, wrap at 75 char and Simplify fix code > at i2c_imx_dma_request(). > - Use strict patch check, fix alignment warning at i2c_imx_dma_request() > --- > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 5ed4cb61e262..b11d66d56c55 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx) > } > > /* Functions for DMA support */ > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > - dma_addr_t phy_addr) > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr) > { > struct imx_i2c_dma *dma; > struct dma_slave_config dma_sconfig; > - struct device *dev = &i2c_imx->adapter.dev; > + struct device *dev = i2c_imx->adapter.dev.parent; Why is this change needed? What's the purpose? > int ret; > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); This means you bind the allocated memory to the parent device, not the i2c device anymore. Marc
> -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: Wednesday, November 27, 2024 4:38 PM > To: Carlos Song <carlos.song@nxp.com>; mkl@pengutronix.de; Frank Li > <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; > andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com > Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > Hello Carlos, > > On 27.11.24 09:38, carlos.song@nxp.com wrote: > > From: Carlos Song <carlos.song@nxp.com> > > > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA > > driver have not ready yet. > > > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > > and "Documentation/driver-api/driver-model/driver.rst". > > > > Use CPU mode to avoid stuck registering i2c adapter when DMA resources > > are unavailable. > > Please try to address open questions before sending new versions of the patch > set. Otherwise, it's difficult to follow the conversation. > > Did you see my question[1] on your v2: > Hi, thank you so much! So sorry about it... I missed it yesterday. I will answer your question[1] in this mail. > | Wouldn't this break probe for all i2c-imx users who have > | CONFIG_IMX_SDMA disabled? > | I have tested i2c probe at IMX and LS platform when DMA disabled, it won't break i2c-imx probe. When require DMA channel in i2c_imx_dma_request, find no devices and return -ENODEV, as you see at V4 patch, it will continue to probe and work in PIO mode. I2C adapter should keep available whatever DMA mode is or isn't enabled. > | Also I am wondering on what kernel version and what configuration > | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run into > | this situation. > | I want to correct something, these code about DMA in i2c-imx.c is for eDMA not for SDMA. For eDMA mode, I have tested this patch at layerscape-1043 platform. My patch is based on cfba9f07a1d6 (tag: next-20241122, origin/master, origin/HEAD). Test log is : No apply this patch: CONFIG_I2C_IMX=y CONFIG_FSL_EDMA=y root@ls1043ardb:~# dmesg | grep i2c [ 1.162053] i2c i2c-0: IMX I2C adapter registered [ 1.166826] i2c i2c-0: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers [ 4.722057] i2c_dev: i2c /dev entries driver Not apply the patch: CONFIG_I2C_IMX=y CONFIG_FSL_EDMA=m root@ls1043ardb:~# dmesg | grep i2c [ 1.166381] i2c i2c-0: IMX I2C adapter registered [ 4.719226] i2c_dev: i2c /dev entries driver (result shows i2c not enabled the eDMA mode) root@ls1043ardb:~# i2cdetect -y -l i2c-0 i2c 2180000.i2c I2C adapter root@ls1043ardb:~# i2cdetect -y 0 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: 08 -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- After apply the patch: CONFIG_I2C_IMX=y CONFIG_FSL_EDMA=m root@ls1043ardb:~# root@ls1043ardb:~# dmesg | grep i2c [ 4.697046] i2c_dev: i2c /dev entries driver [ 7.304142] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers [ 7.313532] i2c i2c-0: IMX I2C adapter registered (result shows i2c probed after eDMA module installed) root@ls1043ardb:~# root@ls1043ardb:~# i2cdetect -y -l i2c-0 i2c 2180000.i2c I2C adapter root@ls1043ardb:~# i2cdetect -y 0 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: 08 -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- 70: -- -- -- -- -- -- -- -- > | I'd have expected that with fw_devlink enabled, the I2C controller > | wouldn't be probed before the DMA provider is available. > This is a legacy patch, it has been in our local tree for a long time. The related history is relatively vague. I reproduced the problem and found this patch is effective, so I referred the community patch and legacy patch to rewrite the commit log(I am not sure if this would happened in some cases so I kept this information). Now it seems that these descriptions are redundant. I should completely removed this in the commit log: Move i2c_imx_dma_request() before registering I2C adapter to avoid infinite loop of .probe() calls to the same driver, see "e8c220fac415 Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" and "Documentation/driver-api/driver-model/driver.rst". [1]: https://lore.kernel.org/all/19a43db4-db5c-4638-9778-d94fb571a206@pengutronix.de/ [2]:https://lore.kernel.org/all/153e8e36-7b0e-4379-9cc3-6dacb5d705be@pengutronix.de/ > > > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > --- > > Change for V4: > > - Output "Only use PIO mode" log in debug level when no DMA configure. > > Change for V3: > > - According to Marc's comment, remove error print when defer probe. > > Add info log when DMA has not been enabled and add error log when > > DMA error, both won't stuck the i2c adapter register, just for reminding, > > i2c adapter is working only in PIO mode. > > Change for V2: > > - According to Frank's comments, wrap at 75 char and Simplify fix code > > at i2c_imx_dma_request(). > > - Use strict patch check, fix alignment warning at > > i2c_imx_dma_request() > > --- > > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > > 1 file changed, 23 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct > > imx_i2c_struct *i2c_imx) } > > > > /* Functions for DMA support */ > > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > - dma_addr_t > phy_addr) > > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > +dma_addr_t phy_addr) > > { > > struct imx_i2c_dma *dma; > > struct dma_slave_config dma_sconfig; > > - struct device *dev = &i2c_imx->adapter.dev; > > + struct device *dev = i2c_imx->adapter.dev.parent; > > int ret; > > > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); > > if (!dma) > > - return; > > + return -ENOMEM; > > > > dma->chan_tx = dma_request_chan(dev, "tx"); > > if (IS_ERR(dma->chan_tx)) { > > @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct > imx_i2c_struct *i2c_imx, > > dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", > > dma_chan_name(dma->chan_tx), > > dma_chan_name(dma->chan_rx)); > > > > - return; > > + return 0; > > > > fail_rx: > > dma_release_channel(dma->chan_rx); > > @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct > imx_i2c_struct *i2c_imx, > > dma_release_channel(dma->chan_tx); > > fail_al: > > devm_kfree(dev, dma); > > + > > + return ret; > > } > > > > static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@ > > static int i2c_imx_probe(struct platform_device *pdev) > > if (ret == -EPROBE_DEFER) > > goto clk_notifier_unregister; > > > > + /* > > + * Init DMA config if supported, -ENODEV means DMA not enabled at > > + * this platform, that is not a real error, so just remind "only > > + * PIO mode is used". If DMA is enabled, but meet error when request > > + * DMA channel, error should be showed in probe error log. PIO mode > > + * should be available regardless of DMA. > > + */ > > + ret = i2c_imx_dma_request(i2c_imx, phy_addr); > > + if (ret) { > > + if (ret == -EPROBE_DEFER) > > + goto clk_notifier_unregister; > > + else if (ret == -ENODEV) > > + dev_dbg(&pdev->dev, "Only use PIO mode\n"); > > + else > > + dev_err_probe(&pdev->dev, ret, "Failed to setup > DMA, only use PIO mode\n"); > > + } > > + > > /* Add I2C adapter */ > > ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > > if (ret < 0) > > @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > i2c_imx->adapter.name); > > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); > > > > - /* Init DMA config if supported */ > > - i2c_imx_dma_request(i2c_imx, phy_addr); > > - > > return 0; /* Return OK */ > > > > clk_notifier_unregister: > > > -- > Pengutronix e.K. | > | > Steuerwalder Str. 21 | > http://www.pen/ > gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C1acf840d499f > 49a7872408dd0ebedc39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > %7C638682935131084746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&s > data=Y9Qn9XEk15yu4CespwsNu6hl3%2FqfNTvEeOn4ZvnGxbo%3D&reserved=0 > | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
> -----Original Message----- > From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: Wednesday, November 27, 2024 4:43 PM > To: Carlos Song <carlos.song@nxp.com> > Cc: Frank Li <frank.li@nxp.com>; o.rempel@pengutronix.de; > kernel@pengutronix.de; andi.shyti@kernel.org; shawnguo@kernel.org; > s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org; > imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing > > On 27.11.2024 16:38:18, carlos.song@nxp.com wrote: > > From: Carlos Song <carlos.song@nxp.com> > > > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA > > driver have not ready yet. > > > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > > and "Documentation/driver-api/driver-model/driver.rst". > > > > Use CPU mode to avoid stuck registering i2c adapter when DMA resources > > are unavailable. > > > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > --- > > Change for V4: > > - Output "Only use PIO mode" log in debug level when no DMA configure. > > Change for V3: > > - According to Marc's comment, remove error print when defer probe. > > Add info log when DMA has not been enabled and add error log when > > DMA error, both won't stuck the i2c adapter register, just for reminding, > > i2c adapter is working only in PIO mode. > > Change for V2: > > - According to Frank's comments, wrap at 75 char and Simplify fix code > > at i2c_imx_dma_request(). > > - Use strict patch check, fix alignment warning at > > i2c_imx_dma_request() > > --- > > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > > 1 file changed, 23 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct > > imx_i2c_struct *i2c_imx) } > > > > /* Functions for DMA support */ > > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > - dma_addr_t phy_addr) > > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > +dma_addr_t phy_addr) > > { > > struct imx_i2c_dma *dma; > > struct dma_slave_config dma_sconfig; > > - struct device *dev = &i2c_imx->adapter.dev; > > + struct device *dev = i2c_imx->adapter.dev.parent; > > Why is this change needed? What's the purpose? > Because i2c_imx_dma_request has been moved to before i2c_add_numbered_adapter(&i2c_imx->adapter); If we use &i2c_imx->adapter.dev, we will meet NULL pointer at here (arrow points to) /* Functions for DMA support */ static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr) { struct imx_i2c_dma *dma; struct dma_slave_config dma_sconfig; struct device *dev = i2c_imx->adapter.dev.parent; int ret; ----> dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); if (!dma) return -ENOMEM; log is that: [ 1.159261] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 [ 1.168089] Mem abort info: [ 1.170889] ESR = 0x0000000096000044 [ 1.174649] EC = 0x25: DABT (current EL), IL = 32 bits [ 1.179983] SET = 0, FnV = 0 [ 1.183044] EA = 0, S1PTW = 0 [ 1.186193] FSC = 0x04: level 0 translation fault [ 1.191089] Data abort info: [ 1.193975] ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000 [ 1.199483] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 [ 1.204554] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 1.209888] [0000000000000000] user address but active_mm is swapper [ 1.216270] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP [ 1.222566] Modules linked in: [ 1.225634] CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-06331-g5ddb25243333-dirty #1 [ 1.234464] Hardware name: LS1043A RDB Board (DT) [ 1.239186] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 1.246182] pc : devres_add+0x54/0x7c [ 1.249864] lr : devres_add+0x28/0x7c [ 1.253541] sp : ffff80008003ba90 [ 1.256864] x29: ffff80008003ba90 x28: 0000000000000000 x27: 0000000000000013 [ 1.264039] x26: ffff1638043d1080 x25: ffff1638043d14b8 x24: ffff1638043d10f0 [ 1.271214] x23: 0000000000000000 x22: ffff163804267180 x21: ffff1638043d139c [ 1.278388] x20: ffff163804267180 x19: ffff1638043d13a0 x18: 0000000000000006 [ 1.285563] x17: 0000000000000001 x16: 0000000000000000 x15: ffff80008003b6df [ 1.292737] x14: ffffa378172b5f20 x13: 747365757165725f x12: 616d645f786d695f [ 1.299912] x11: ffffa378172b5f20 x10: 000000000000009d x9 : 0000000000000000 [ 1.307087] x8 : ffff163804267200 x7 : 0000000000000000 x6 : 000000000000003f [ 1.314261] x5 : 0000000000000040 x4 : ffff1638043d10f0 x3 : 0000000000000000 [ 1.321435] x2 : ffff163804267100 x1 : 0000000000000000 x0 : ffff1638043d139c [ 1.328609] Call trace: [ 1.331059] devres_add+0x54/0x7c [ 1.334387] devm_kmalloc+0x90/0x100 [ 1.337977] i2c_imx_probe+0x48c/0x714 [ 1.341743] platform_probe+0x68/0xdc [ 1.345419] really_probe+0xbc/0x2bc [ 1.349007] __driver_probe_device+0x78/0x120 [ 1.353381] driver_probe_device+0x3c/0x154 [ 1.357581] __driver_attach+0x90/0x1a0 [ 1.361431] bus_for_each_dev+0x7c/0xe0 [ 1.365284] driver_attach+0x24/0x30 [ 1.368875] bus_add_driver+0xe4/0x208 [ 1.372640] driver_register+0x68/0x124 [ 1.376490] __platform_driver_register+0x24/0x30 [ 1.381214] i2c_adap_imx_init+0x1c/0x28 [ 1.385155] do_one_initcall+0x60/0x1d4 [ 1.389007] kernel_init_freeable+0x214/0x278 [ 1.393384] kernel_init+0x20/0x140 [ 1.396887] ret_from_fork+0x10/0x20 [ 1.400479] Code: aa1503e0 f9415c83 f9015c82 a9380e93 (f9000062) [ 1.406600] ---[ end trace 0000000000000000 ]--- [ 1.411235] note: swapper/0[1] exited with irqs disabled [ 1.416582] note: swapper/0[1] exited with preempt_count 1 [ 1.422102] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b [ 1.429795] SMP: stopping secondary CPUs [1] https://lore.kernel.org/all/20241127-attentive-orthodox-tuna-8f8af0-mkl@pengutronix.de/ > > int ret; > > > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); > > This means you bind the allocated memory to the parent device, not the i2c > device anymore. > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
> -----Original Message----- > From: Carlos Song > Sent: Wednesday, November 27, 2024 6:43 PM > To: Ahmad Fatoum <a.fatoum@pengutronix.de>; mkl@pengutronix.de; Frank Li > <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; > andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com > Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v4] i2c: imx: support DMA defer probing > > > > > -----Original Message----- > > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > > Sent: Wednesday, November 27, 2024 4:38 PM > > To: Carlos Song <carlos.song@nxp.com>; mkl@pengutronix.de; Frank Li > > <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; > > andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > > festevam@gmail.com > > Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; > > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing > > > > Caution: This is an external email. Please take care when clicking > > links or opening attachments. When in doubt, report the message using > > the 'Report this email' button > > > > > > Hello Carlos, > > > > On 27.11.24 09:38, carlos.song@nxp.com wrote: > > > From: Carlos Song <carlos.song@nxp.com> > > > > > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA > > > driver have not ready yet. > > > > > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > > > infinite loop of .probe() calls to the same driver, see > > > "e8c220fac415 Revert "i2c: imx: improve the error handling in > i2c_imx_dma_request()"" > > > and "Documentation/driver-api/driver-model/driver.rst". > > > > > > Use CPU mode to avoid stuck registering i2c adapter when DMA > > > resources are unavailable. > > > > Please try to address open questions before sending new versions of > > the patch set. Otherwise, it's difficult to follow the conversation. > > > > Did you see my question[1] on your v2: > > > > Hi, thank you so much! So sorry about it... I missed it yesterday. I will answer > your question[1] in this mail. > > > > | Wouldn't this break probe for all i2c-imx users who have > > | CONFIG_IMX_SDMA disabled? > > | > > I have tested i2c probe at IMX and LS platform when DMA disabled, it won't > break i2c-imx probe. > When require DMA channel in i2c_imx_dma_request, find no devices and return > -ENODEV, as you see at V4 patch, it will continue to probe and work in PIO > mode. > I2C adapter should keep available whatever DMA mode is or isn't enabled. > > > | Also I am wondering on what kernel version and what configuration > > | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run > > | into this situation. > > | > > I want to correct something, these code about DMA in i2c-imx.c is for eDMA not > for SDMA. > For eDMA mode, I have tested this patch at layerscape-1043 platform. My patch > is based on > cfba9f07a1d6 (tag: next-20241122, origin/master, origin/HEAD). > > Test log is : > No apply this patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=y > root@ls1043ardb:~# dmesg | grep i2c > [ 1.162053] i2c i2c-0: IMX I2C adapter registered > [ 1.166826] i2c i2c-0: using dma0chan16 (tx) and dma0chan17 (rx) for DMA > transfers > [ 4.722057] i2c_dev: i2c /dev entries driver > > Not apply the patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=m > root@ls1043ardb:~# dmesg | grep i2c > [ 1.166381] i2c i2c-0: IMX I2C adapter registered > [ 4.719226] i2c_dev: i2c /dev entries driver > (result shows i2c not enabled the eDMA mode) root@ls1043ardb:~# i2cdetect > -y -l > i2c-0 i2c 2180000.i2c I2C > adapter > root@ls1043ardb:~# i2cdetect -y 0 > 0 1 2 3 4 5 6 7 8 9 a b c d e f > 00: 08 -- -- -- -- -- -- -- > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > 70: -- -- -- -- -- -- -- -- > > After apply the patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=m > root@ls1043ardb:~# > root@ls1043ardb:~# dmesg | grep i2c > [ 4.697046] i2c_dev: i2c /dev entries driver > [ 7.304142] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 > (rx) for DMA transfers > [ 7.313532] i2c i2c-0: IMX I2C adapter registered > (result shows i2c probed after eDMA module installed) root@ls1043ardb:~# > root@ls1043ardb:~# i2cdetect -y -l > i2c-0 i2c 2180000.i2c I2C > adapter > root@ls1043ardb:~# i2cdetect -y 0 > 0 1 2 3 4 5 6 7 8 9 a b c d e f > 00: 08 -- -- -- -- -- -- -- > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > 70: -- -- -- -- -- -- -- -- > > > > | I'd have expected that with fw_devlink enabled, the I2C controller > > | wouldn't be probed before the DMA provider is available. > > > > This is a legacy patch, it has been in our local tree for a long time. The related > history is relatively vague. > I reproduced the problem and found this patch is effective, so I referred the > community patch and legacy patch to rewrite the commit log(I am not sure if > this would happened in some cases so I kept this information). > Now it seems that these descriptions are redundant. I should completely > removed this in the commit log: > Move i2c_imx_dma_request() before registering I2C adapter to avoid > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > and "Documentation/driver-api/driver-model/driver.rst". > > [1]: > https://lore.kernel.org/all/19a43db4-db5c-4638-9778-d94fb571a206@pengutr > onix.de/ > [2]:https://lore.kernel.org/all/153e8e36-7b0e-4379-9cc3-6dacb5d705be@peng > utronix.de/ > Hi, Ahmad I have done more test about following comment in [1]: ---------------- > | I'd have expected that with fw_devlink enabled, the I2C controller > | wouldn't be probed before the DMA provider is available. > This is a legacy patch, it has been in our local tree for a long time. The related history is relatively vague. I reproduced the problem and found this patch is effective, so I referred the community patch and legacy patch to rewrite the commit log(I am not sure if this would happened in some cases so I kept this information). Now it seems that these descriptions are redundant. I should completely removed this in the commit log: Move i2c_imx_dma_request() before registering I2C adapter to avoid infinite loop of .probe() calls to the same driver, see "e8c220fac415 Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" and "Documentation/driver-api/driver-model/driver.rst". ---------------- You can refer the link about the history: [1]: https://lore.kernel.org/all/AM0PR0402MB39370E69BC4B71C761EE8377E8282@AM0PR0402MB3937.eurprd04.prod.outlook.com/ I don't know if I have understanded you rightly. But I am trying your expect. I create a new case, only add "return defer probe and error in i2c_imx_dma_request()" and still keep i2c_imx_dma_request() after the adapter register. The code step like: ret = i2c_add_numbered_adapter() ... ret = i2c_imx_dma_request(i2c_imx, phy_addr); if (ret == -EPROBE_DEFER) goto delete_adapter; ... return 0; delete_adapter: /* remove adapter */ dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); i2c_del_adapter(&i2c_imx->adapter); clk_notifier_unregister: ... Now, CONFIG_I2C_IMX=y CONFIG_FSL_EDMA=m As you expected, if I understand rightly, I think I shouldn't see any i2c probe log until DMA is available. When I make all i2c devices module installed, i2c still defer probe if DMA is not available. root@ls1043ardb:~# dmesg | grep i2c [ 1.153535] i2c i2c-0: IMX I2C adapter registered [ 4.705068] i2c_dev: i2c /dev entries driver [ 4.995118] i2c i2c-0: IMX I2C adapter registered [ 5.003324] i2c i2c-0: IMX I2C adapter registered [ 8.079458] i2c i2c-0: IMX I2C adapter registered [ 8.213536] i2c i2c-0: IMX I2C adapter registered [ 8.278704] i2c i2c-0: IMX I2C adapter registered [ 8.283669] i2c i2c-0: using dma1chan16 (tx) and dma1chan17 (rx) for DMA transfers However when I make some i2c devices build-in, the infinite loop of .probe() calls will happened: [ 5.004716] ina2xx 0-0040: supply vs not found, using dummy regulator [ 5.012526] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 5.021217] pcf85363 0-0051: registered as rtc0 [ 5.027042] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934) [ 5.035644] i2c i2c-0: IMX I2C adapter registered [ 5.045357] ina2xx 0-0040: supply vs not found, using dummy regulator [ 5.053111] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 5.061748] pcf85363 0-0051: registered as rtc0 [ 5.067569] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934) [ 5.076148] i2c i2c-0: IMX I2C adapter registered [ 5.097323] ina2xx 0-0040: supply vs not found, using dummy regulator [ 5.105081] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 5.113585] pcf85363 0-0051: registered as rtc0 [ 5.119402] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934) [ 5.127958] i2c i2c-0: IMX I2C adapter registered [ 5.139289] ina2xx 0-0040: supply vs not found, using dummy regulator [ 5.147014] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 5.155584] pcf85363 0-0051: registered as rtc0 [ 5.161427] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934) [ 5.169968] i2c i2c-0: IMX I2C adapter registered [ 5.181278] ina2xx 0-0040: supply vs not found, using dummy regulator [ 5.189015] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 5.197515] pcf85363 0-0051: registered as rtc0 [ 5.203394] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934) [ 5.211930] i2c i2c-0: IMX I2C adapter registered ... [ 26.405830] i2c i2c-0: IMX I2C adapter registered [ 26.413765] ina2xx 0-0040: supply vs not found, using dummy regulator [ 26.421485] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 26.429980] pcf85363 0-0051: registered as rtc0 [ 26.435735] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955) [ 26.444324] i2c i2c-0: IMX I2C adapter registered [ 26.452250] ina2xx 0-0040: supply vs not found, using dummy regulator [ 26.459968] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 26.468395] pcf85363 0-0051: registered as rtc0 [ 26.474233] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955) [ 26.482774] i2c i2c-0: IMX I2C adapter registered [ 26.490694] ina2xx 0-0040: supply vs not found, using dummy regulator [ 26.498405] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 26.506897] pcf85363 0-0051: registered as rtc0 [ 26.512678] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955) Is above test not in line with your expectations?(with fw_devlink enabled, I2C controller wouldn't be probed before the DMA provider is available.) When I apply the V4 patch. I2C adapter can defer probe and whatever i2c devices module installed/build-in, no issue happened. The build-in device driver also start probing after adapter really registered: (-------------i2c_imx_dma_request--------------) [ 7.437482] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers (----------i2c_add_numbered_adapter-------------) [ 7.446581] ina2xx 0-0040: supply vs not found, using dummy regulator [ 7.455945] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm) [ 7.465146] pcf85363 0-0051: registered as rtc0 [ 7.471052] pcf85363 0-0051: setting system clock to 2020-12-05T17:54:39 UTC (1607190879) [ 7.479902] i2c i2c-0: IMX I2C adapter registered Inevitably, on some boards some i2c device drivers are build-in. The V4 patch will avoid some issue and is safer for i2c-imx user at different platform. So I think it is necessary to move i2c_imx_dma_request() before registering I2C adapter and I should also keep the log part. > > > > > > Signed-off-by: Carlos Song <carlos.song@nxp.com> > > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > > > --- > > > Change for V4: > > > - Output "Only use PIO mode" log in debug level when no DMA configure. > > > Change for V3: > > > - According to Marc's comment, remove error print when defer probe. > > > Add info log when DMA has not been enabled and add error log when > > > DMA error, both won't stuck the i2c adapter register, just for reminding, > > > i2c adapter is working only in PIO mode. > > > Change for V2: > > > - According to Frank's comments, wrap at 75 char and Simplify fix code > > > at i2c_imx_dma_request(). > > > - Use strict patch check, fix alignment warning at > > > i2c_imx_dma_request() > > > --- > > > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > > > 1 file changed, 23 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > > b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 > > > 100644 > > > --- a/drivers/i2c/busses/i2c-imx.c > > > +++ b/drivers/i2c/busses/i2c-imx.c > > > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct > > > imx_i2c_struct *i2c_imx) } > > > > > > /* Functions for DMA support */ > > > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > > - dma_addr_t > > phy_addr) > > > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > > > +dma_addr_t phy_addr) > > > { > > > struct imx_i2c_dma *dma; > > > struct dma_slave_config dma_sconfig; > > > - struct device *dev = &i2c_imx->adapter.dev; > > > + struct device *dev = i2c_imx->adapter.dev.parent; > > > int ret; > > > > > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); > > > if (!dma) > > > - return; > > > + return -ENOMEM; > > > > > > dma->chan_tx = dma_request_chan(dev, "tx"); > > > if (IS_ERR(dma->chan_tx)) { > > > @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct > > imx_i2c_struct *i2c_imx, > > > dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", > > > dma_chan_name(dma->chan_tx), > > > dma_chan_name(dma->chan_rx)); > > > > > > - return; > > > + return 0; > > > > > > fail_rx: > > > dma_release_channel(dma->chan_rx); > > > @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct > > imx_i2c_struct *i2c_imx, > > > dma_release_channel(dma->chan_tx); > > > fail_al: > > > devm_kfree(dev, dma); > > > + > > > + return ret; > > > } > > > > > > static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@ > > > static int i2c_imx_probe(struct platform_device *pdev) > > > if (ret == -EPROBE_DEFER) > > > goto clk_notifier_unregister; > > > > > > + /* > > > + * Init DMA config if supported, -ENODEV means DMA not enabled > at > > > + * this platform, that is not a real error, so just remind "only > > > + * PIO mode is used". If DMA is enabled, but meet error when > request > > > + * DMA channel, error should be showed in probe error log. PIO > mode > > > + * should be available regardless of DMA. > > > + */ > > > + ret = i2c_imx_dma_request(i2c_imx, phy_addr); > > > + if (ret) { > > > + if (ret == -EPROBE_DEFER) > > > + goto clk_notifier_unregister; > > > + else if (ret == -ENODEV) > > > + dev_dbg(&pdev->dev, "Only use PIO mode\n"); > > > + else > > > + dev_err_probe(&pdev->dev, ret, "Failed to > > > + setup > > DMA, only use PIO mode\n"); > > > + } > > > + > > > /* Add I2C adapter */ > > > ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > > > if (ret < 0) > > > @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct > > > platform_device > > *pdev) > > > i2c_imx->adapter.name); > > > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter > > > registered\n"); > > > > > > - /* Init DMA config if supported */ > > > - i2c_imx_dma_request(i2c_imx, phy_addr); > > > - > > > return 0; /* Return OK */ > > > > > > clk_notifier_unregister: > > > > > > -- > > Pengutronix e.K. | > > | > > Steuerwalder Str. 21 | > > http://www.p/ > > en > gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C1acf840d499f > > > 49a7872408dd0ebedc39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > > %7C638682935131084746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > w > > > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&s > > > data=Y9Qn9XEk15yu4CespwsNu6hl3%2FqfNTvEeOn4ZvnGxbo%3D&reserved=0 > > | > > 31137 Hildesheim, Germany | Phone: > +49-5121-206917-0 > > | > > Amtsgericht Hildesheim, HRA 2686 | Fax: > > +49-5121-206917-5555 |
Hello Carlos, On 27.11.24 11:43, Carlos Song wrote: >> -----Original Message----- >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> >> Sent: Wednesday, November 27, 2024 4:38 PM >> To: Carlos Song <carlos.song@nxp.com>; mkl@pengutronix.de; Frank Li >> <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; >> andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; >> festevam@gmail.com >> Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >> Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing >> Please try to address open questions before sending new versions of the patch >> set. Otherwise, it's difficult to follow the conversation. >> >> Did you see my question[1] on your v2: >> > > Hi, thank you so much! So sorry about it... I missed it yesterday. I will answer your question[1] in this mail. > > >> | Wouldn't this break probe for all i2c-imx users who have >> | CONFIG_IMX_SDMA disabled? >> | > > I have tested i2c probe at IMX and LS platform when DMA disabled, What does DMA disabled mean? Did you leave the dmas property in the device tree unchanged, but just disabled the config option? > it won't break i2c-imx probe. > When require DMA channel in i2c_imx_dma_request, find no devices and return > -ENODEV, as you see at V4 patch, it will continue to probe and work in PIO mode. > I2C adapter should keep available whatever DMA mode is or isn't enabled. If that's the case, then all is good. I thought the situation described above would return -EPROBE_DEFER instead. I haven't dug into the code to understand what the difference between when dma_request_chan(). >> | Also I am wondering on what kernel version and what configuration >> | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run into >> | this situation. >> | > > I want to correct something, these code about DMA in i2c-imx.c is for eDMA not for SDMA. > For eDMA mode, I have tested this patch at layerscape-1043 platform. My patch is based on > cfba9f07a1d6 (tag: next-20241122, origin/master, origin/HEAD). The driver also handles i.MX variants like i.MX6, i.MX8 and so on, which have SDMA. So eDMA is not the only DMA driver it is used with. > > Test log is : > No apply this patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=y > root@ls1043ardb:~# dmesg | grep i2c > [ 1.162053] i2c i2c-0: IMX I2C adapter registered > [ 1.166826] i2c i2c-0: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers > [ 4.722057] i2c_dev: i2c /dev entries driver > > Not apply the patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=m > root@ls1043ardb:~# dmesg | grep i2c > [ 1.166381] i2c i2c-0: IMX I2C adapter registered > [ 4.719226] i2c_dev: i2c /dev entries driver > (result shows i2c not enabled the eDMA mode) > root@ls1043ardb:~# i2cdetect -y -l > i2c-0 i2c 2180000.i2c I2C adapter > root@ls1043ardb:~# i2cdetect -y 0 > 0 1 2 3 4 5 6 7 8 9 a b c d e f > 00: 08 -- -- -- -- -- -- -- > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > 70: -- -- -- -- -- -- -- -- > > After apply the patch: > CONFIG_I2C_IMX=y > CONFIG_FSL_EDMA=m > root@ls1043ardb:~# > root@ls1043ardb:~# dmesg | grep i2c > [ 4.697046] i2c_dev: i2c /dev entries driver > [ 7.304142] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers > [ 7.313532] i2c i2c-0: IMX I2C adapter registered > (result shows i2c probed after eDMA module installed) My concern is this configuration: - A user has eDMA/SDMA module or disabled, but enabled in DT - The I2C has a PMIC, which is needed for eMMC VCC - System startup is stuck or considerably delayed > root@ls1043ardb:~# > root@ls1043ardb:~# i2cdetect -y -l > i2c-0 i2c 2180000.i2c I2C adapter > root@ls1043ardb:~# i2cdetect -y 0 > 0 1 2 3 4 5 6 7 8 9 a b c d e f > 00: 08 -- -- -- -- -- -- -- > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > 70: -- -- -- -- -- -- -- -- > > >> | I'd have expected that with fw_devlink enabled, the I2C controller >> | wouldn't be probed before the DMA provider is available. >> > > This is a legacy patch, it has been in our local tree for a long time. The related history is relatively vague. > I reproduced the problem and found this patch is effective, so I referred the community patch and > legacy patch to rewrite the commit log(I am not sure if this would happened in some cases so I kept this information). > Now it seems that these descriptions are redundant. I should completely removed this in the commit log: > Move i2c_imx_dma_request() before registering I2C adapter to avoid > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()"" > and "Documentation/driver-api/driver-model/driver.rst". Cheers, Ahmad > > [1]: https://lore.kernel.org/all/19a43db4-db5c-4638-9778-d94fb571a206@pengutronix.de/ > [2]:https://lore.kernel.org/all/153e8e36-7b0e-4379-9cc3-6dacb5d705be@pengutronix.de/ > >>> >>> Signed-off-by: Carlos Song <carlos.song@nxp.com> >>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> >>> --- >>> Change for V4: >>> - Output "Only use PIO mode" log in debug level when no DMA configure. >>> Change for V3: >>> - According to Marc's comment, remove error print when defer probe. >>> Add info log when DMA has not been enabled and add error log when >>> DMA error, both won't stuck the i2c adapter register, just for reminding, >>> i2c adapter is working only in PIO mode. >>> Change for V2: >>> - According to Frank's comments, wrap at 75 char and Simplify fix code >>> at i2c_imx_dma_request(). >>> - Use strict patch check, fix alignment warning at >>> i2c_imx_dma_request() >>> --- >>> drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- >>> 1 file changed, 23 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-imx.c >>> b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 100644 >>> --- a/drivers/i2c/busses/i2c-imx.c >>> +++ b/drivers/i2c/busses/i2c-imx.c >>> @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct >>> imx_i2c_struct *i2c_imx) } >>> >>> /* Functions for DMA support */ >>> -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, >>> - dma_addr_t >> phy_addr) >>> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, >>> +dma_addr_t phy_addr) >>> { >>> struct imx_i2c_dma *dma; >>> struct dma_slave_config dma_sconfig; >>> - struct device *dev = &i2c_imx->adapter.dev; >>> + struct device *dev = i2c_imx->adapter.dev.parent; >>> int ret; >>> >>> dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); >>> if (!dma) >>> - return; >>> + return -ENOMEM; >>> >>> dma->chan_tx = dma_request_chan(dev, "tx"); >>> if (IS_ERR(dma->chan_tx)) { >>> @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct >> imx_i2c_struct *i2c_imx, >>> dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", >>> dma_chan_name(dma->chan_tx), >>> dma_chan_name(dma->chan_rx)); >>> >>> - return; >>> + return 0; >>> >>> fail_rx: >>> dma_release_channel(dma->chan_rx); >>> @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct >> imx_i2c_struct *i2c_imx, >>> dma_release_channel(dma->chan_tx); >>> fail_al: >>> devm_kfree(dev, dma); >>> + >>> + return ret; >>> } >>> >>> static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@ >>> static int i2c_imx_probe(struct platform_device *pdev) >>> if (ret == -EPROBE_DEFER) >>> goto clk_notifier_unregister; >>> >>> + /* >>> + * Init DMA config if supported, -ENODEV means DMA not enabled at >>> + * this platform, that is not a real error, so just remind "only >>> + * PIO mode is used". If DMA is enabled, but meet error when request >>> + * DMA channel, error should be showed in probe error log. PIO mode >>> + * should be available regardless of DMA. >>> + */ >>> + ret = i2c_imx_dma_request(i2c_imx, phy_addr); >>> + if (ret) { >>> + if (ret == -EPROBE_DEFER) >>> + goto clk_notifier_unregister; >>> + else if (ret == -ENODEV) >>> + dev_dbg(&pdev->dev, "Only use PIO mode\n"); >>> + else >>> + dev_err_probe(&pdev->dev, ret, "Failed to setup >> DMA, only use PIO mode\n"); >>> + } >>> + >>> /* Add I2C adapter */ >>> ret = i2c_add_numbered_adapter(&i2c_imx->adapter); >>> if (ret < 0) >>> @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device >> *pdev) >>> i2c_imx->adapter.name); >>> dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); >>> >>> - /* Init DMA config if supported */ >>> - i2c_imx_dma_request(i2c_imx, phy_addr); >>> - >>> return 0; /* Return OK */ >>> >>> clk_notifier_unregister: >> >> >> -- >> Pengutronix e.K. | >> | >> Steuerwalder Str. 21 | >> http://www.pen/ >> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C1acf840d499f >> 49a7872408dd0ebedc39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 >> %7C638682935131084746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw >> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&s >> data=Y9Qn9XEk15yu4CespwsNu6hl3%2FqfNTvEeOn4ZvnGxbo%3D&reserved=0 >> | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 >> | >> Amtsgericht Hildesheim, HRA 2686 | Fax: >> +49-5121-206917-5555 | >
> -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: Friday, December 6, 2024 2:50 AM > To: Carlos Song <carlos.song@nxp.com>; mkl@pengutronix.de; Frank Li > <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; > andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > festevam@gmail.com > Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > Hello Carlos, > > On 27.11.24 11:43, Carlos Song wrote: > >> -----Original Message----- > >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> > >> Sent: Wednesday, November 27, 2024 4:38 PM > >> To: Carlos Song <carlos.song@nxp.com>; mkl@pengutronix.de; Frank Li > >> <frank.li@nxp.com>; o.rempel@pengutronix.de; kernel@pengutronix.de; > >> andi.shyti@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > >> festevam@gmail.com > >> Cc: imx@lists.linux.dev; linux-i2c@vger.kernel.org; > >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > >> Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing > > >> Please try to address open questions before sending new versions of > >> the patch set. Otherwise, it's difficult to follow the conversation. > >> > >> Did you see my question[1] on your v2: > >> > > > > Hi, thank you so much! So sorry about it... I missed it yesterday. I will answer > your question[1] in this mail. > > > > > >> | Wouldn't this break probe for all i2c-imx users who have > >> | CONFIG_IMX_SDMA disabled? > >> | > > > > I have tested i2c probe at IMX and LS platform when DMA disabled, > > What does DMA disabled mean? Did you leave the dmas property in the device > tree unchanged, but just disabled the config option? > > > it won't break i2c-imx probe. > > When require DMA channel in i2c_imx_dma_request, find no devices and > > return -ENODEV, as you see at V4 patch, it will continue to probe and work in > PIO mode. > > I2C adapter should keep available whatever DMA mode is or isn't enabled. > > If that's the case, then all is good. I thought the situation described above would > return -EPROBE_DEFER instead. I haven't dug into the code to understand what > the difference between when dma_request_chan(). > > >> | Also I am wondering on what kernel version and what configuration > >> | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run > >> | into this situation. > >> | > > > > I want to correct something, these code about DMA in i2c-imx.c is for eDMA > not for SDMA. > > For eDMA mode, I have tested this patch at layerscape-1043 platform. > > My patch is based on > > cfba9f07a1d6 (tag: next-20241122, origin/master, origin/HEAD). > > The driver also handles i.MX variants like i.MX6, i.MX8 and so on, which have > SDMA. So eDMA is not the only DMA driver it is used with. > > > > > Test log is : > > No apply this patch: > > CONFIG_I2C_IMX=y > > CONFIG_FSL_EDMA=y > > root@ls1043ardb:~# dmesg | grep i2c > > [ 1.162053] i2c i2c-0: IMX I2C adapter registered > > [ 1.166826] i2c i2c-0: using dma0chan16 (tx) and dma0chan17 (rx) for > DMA transfers > > [ 4.722057] i2c_dev: i2c /dev entries driver > > > > Not apply the patch: > > CONFIG_I2C_IMX=y > > CONFIG_FSL_EDMA=m > > root@ls1043ardb:~# dmesg | grep i2c > > [ 1.166381] i2c i2c-0: IMX I2C adapter registered > > [ 4.719226] i2c_dev: i2c /dev entries driver > > (result shows i2c not enabled the eDMA mode) root@ls1043ardb:~# > > i2cdetect -y -l > > i2c-0 i2c 2180000.i2c > I2C adapter > > root@ls1043ardb:~# i2cdetect -y 0 > > 0 1 2 3 4 5 6 7 8 9 a b c d e f > > 00: 08 -- -- -- -- -- -- -- > > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > > 70: -- -- -- -- -- -- -- -- > > > > After apply the patch: > > CONFIG_I2C_IMX=y > > CONFIG_FSL_EDMA=m > > root@ls1043ardb:~# > > root@ls1043ardb:~# dmesg | grep i2c > > [ 4.697046] i2c_dev: i2c /dev entries driver > > [ 7.304142] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 > (rx) for DMA transfers > > [ 7.313532] i2c i2c-0: IMX I2C adapter registered > > (result shows i2c probed after eDMA module installed) > Hi, Ahmad Refer previous history: https://lore.kernel.org/linux-arm-kernel/6b39268b-7487-427d-aff5-f3ca3b2afd42@pengutronix.de/ Your suggestions are very insightful. I'm very grateful! Before replying to your question, I think we should synchronize SDMA and eDMA firstly. sDMA and EDMA for i2c is different. Only 1 SDMA channel for i2c SDMA TX/RX event. But 2 eDMA channels for i2c eDMA TX request and RX request. Now in i2c-imx, DMA code all are for eDMA, they have nothing to do with SDMA initialization. It is very different initialization of SDMA and eDMA. SDMA at dts: @@ -1046,6 +1046,8 @@ i2c1: i2c@30a20000 { ... + dmas = <&sdma1 18 27 0>; + dma-names = "rx-tx"; status = "disabled"; }; eDMA at dts: i2c0: i2c@2180000 { .... + dmas = <&edma0 1 38>, + <&edma0 1 39>; + dma-names = "rx", "tx"; status = "disabled"; }; SDMA initialization in i2c-imx driver: + dma->chan_tx = dma_request_chan(dev, "rx-tx"); + if (IS_ERR(dma->chan_tx)) { + ret = PTR_ERR(dma->chan_tx); + goto fail_dma; + } + + dma->chan_rx = dma->chan_tx; + i2c_imx->dma = dma; eDMA initialization in i2c-imx driver: ... + dma->chan_tx = dma_request_chan(dev, "tx"); .... + dma->chan_rx = dma_request_chan(dev, "rx"); .... i2c_imx->dma = dma; So if need to enable SDMA, should define a separate dma_request function and should not reuse the current i2c_imx_dma_request function. Also according to different platforms i2c-imx driver need to choose to use eDMA or SDMA. So now we start to discuss about your confusion: > My concern is this configuration: > > - A user has eDMA/SDMA module or disabled, but enabled in DT [Carlos]: I delete edma channel at dts to disable eDMA before. It works in PIO mode. I also test your case : when enable dma channel in DT but disable eDMA module. It will meet error: +++ b/arch/arm64/configs/defconfig .... -CONFIG_FSL_EDMA=y .... root@ls1043ardb:~# dmesg | grep i2c [ 4.697392] i2c_dev: i2c /dev entries driver [ 18.357685] platform 2180000.i2c: deferred probe pending: (reason unknown) root@ls1043ardb:~# i2cdetect -y -l The case you mentioned is completely inconsistent with the actual usage. Since you have defined the dma channel in dts, it means that you need to use DMA mode in i2c, but you disabled the eDMA module when building the Image. This makes no sense at all. I think this is a usage error. And the deferred probe pending error is predictable. Since there is no DMA driver loaded, I2C will keep defer probe and be hanged. > - The I2C has a PMIC, which is needed for eMMC VCC [Carlos]: PMIC is critical for the whole board, so PMIC will finish all critical system power-on configuration(include eMMC VCC) in the uboot not at kernel. So pmic driver probe in kernel won't and shouldn't effect system boot. > - System startup is stuck or considerably delayed > The purpose of defer probe is to solve the problem of interdependence of module loading, and to try to load again after a while is to ensure that the required functions will not be unavailable because the resources are not ready. This delay is unavoidable. If a defer probe occurs, the first consideration should be to reasonably adjust the loading order of each module, rather than directly giving up the functions that should be enabled. > > root@ls1043ardb:~# > > root@ls1043ardb:~# i2cdetect -y -l > > i2c-0 i2c 2180000.i2c > I2C adapter > > root@ls1043ardb:~# i2cdetect -y 0 > > 0 1 2 3 4 5 6 7 8 9 a b c d e f > > 00: 08 -- -- -- -- -- -- -- > > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- > > 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- -- > > 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- -- > > 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- -- > > 70: -- -- -- -- -- -- -- -- > > > > > >> | I'd have expected that with fw_devlink enabled, the I2C controller > >> | wouldn't be probed before the DMA provider is available. > >> > > > > This is a legacy patch, it has been in our local tree for a long time. The related > history is relatively vague. > > I reproduced the problem and found this patch is effective, so I > > referred the community patch and legacy patch to rewrite the commit log(I > am not sure if this would happened in some cases so I kept this information). > > Now it seems that these descriptions are redundant. I should completely > removed this in the commit log: > > Move i2c_imx_dma_request() before registering I2C adapter to avoid > > infinite loop of .probe() calls to the same driver, see "e8c220fac415 > > Revert "i2c: imx: improve the error handling in > i2c_imx_dma_request()"" > > and "Documentation/driver-api/driver-model/driver.rst". > > Cheers, > Ahmad > > > > > [1]: > > https://lore/ > > .kernel.org%2Fall%2F19a43db4-db5c-4638-9778-d94fb571a206%40pengutro > nix > > .de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C3e9bcb1cc97b43a69 > ca608dd > > > 155d9977%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638690213 > 9450713 > > > 71%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAu > MDAwMCI > > > sIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdat > a=T > > z0JcOxf9rrSOl2FoCHNbQz4rtKY5V0eS1ZMV%2BXED4I%3D&reserved=0 > > [2]:https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > lore.kernel.org%2Fall%2F153e8e36-7b0e-4379-9cc3-6dacb5d705be%40pengutr > > > onix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C3e9bcb1cc97b43a > 69ca6 > > > 08dd155d9977%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63869 > 0213945 > > > 090490%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwL > jAuMDA > > > wMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C > &sda > > ta=pAT68urcH7CTFknvK1xM3lfuYjZQO0I16B%2FTTsCJw6Q%3D&reserved=0 > > > >>> > >>> Signed-off-by: Carlos Song <carlos.song@nxp.com> > >>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com> > >>> --- > >>> Change for V4: > >>> - Output "Only use PIO mode" log in debug level when no DMA configure. > >>> Change for V3: > >>> - According to Marc's comment, remove error print when defer probe. > >>> Add info log when DMA has not been enabled and add error log when > >>> DMA error, both won't stuck the i2c adapter register, just for reminding, > >>> i2c adapter is working only in PIO mode. > >>> Change for V2: > >>> - According to Frank's comments, wrap at 75 char and Simplify fix code > >>> at i2c_imx_dma_request(). > >>> - Use strict patch check, fix alignment warning at > >>> i2c_imx_dma_request() > >>> --- > >>> drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++-------- > >>> 1 file changed, 23 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/i2c/busses/i2c-imx.c > >>> b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 > >>> 100644 > >>> --- a/drivers/i2c/busses/i2c-imx.c > >>> +++ b/drivers/i2c/busses/i2c-imx.c > >>> @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct > >>> imx_i2c_struct *i2c_imx) } > >>> > >>> /* Functions for DMA support */ > >>> -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > >>> - dma_addr_t > >> phy_addr) > >>> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, > >>> +dma_addr_t phy_addr) > >>> { > >>> struct imx_i2c_dma *dma; > >>> struct dma_slave_config dma_sconfig; > >>> - struct device *dev = &i2c_imx->adapter.dev; > >>> + struct device *dev = i2c_imx->adapter.dev.parent; > >>> int ret; > >>> > >>> dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); > >>> if (!dma) > >>> - return; > >>> + return -ENOMEM; > >>> > >>> dma->chan_tx = dma_request_chan(dev, "tx"); > >>> if (IS_ERR(dma->chan_tx)) { > >>> @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct > >> imx_i2c_struct *i2c_imx, > >>> dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", > >>> dma_chan_name(dma->chan_tx), > >>> dma_chan_name(dma->chan_rx)); > >>> > >>> - return; > >>> + return 0; > >>> > >>> fail_rx: > >>> dma_release_channel(dma->chan_rx); > >>> @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct > >> imx_i2c_struct *i2c_imx, > >>> dma_release_channel(dma->chan_tx); > >>> fail_al: > >>> devm_kfree(dev, dma); > >>> + > >>> + return ret; > >>> } > >>> > >>> static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@ > >>> static int i2c_imx_probe(struct platform_device *pdev) > >>> if (ret == -EPROBE_DEFER) > >>> goto clk_notifier_unregister; > >>> > >>> + /* > >>> + * Init DMA config if supported, -ENODEV means DMA not enabled > at > >>> + * this platform, that is not a real error, so just remind "only > >>> + * PIO mode is used". If DMA is enabled, but meet error when > request > >>> + * DMA channel, error should be showed in probe error log. PIO > mode > >>> + * should be available regardless of DMA. > >>> + */ > >>> + ret = i2c_imx_dma_request(i2c_imx, phy_addr); > >>> + if (ret) { > >>> + if (ret == -EPROBE_DEFER) > >>> + goto clk_notifier_unregister; > >>> + else if (ret == -ENODEV) > >>> + dev_dbg(&pdev->dev, "Only use PIO mode\n"); > >>> + else > >>> + dev_err_probe(&pdev->dev, ret, "Failed to > >>> + setup > >> DMA, only use PIO mode\n"); > >>> + } > >>> + > >>> /* Add I2C adapter */ > >>> ret = i2c_add_numbered_adapter(&i2c_imx->adapter); > >>> if (ret < 0) > >>> @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct > >>> platform_device > >> *pdev) > >>> i2c_imx->adapter.name); > >>> dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter > >>> registered\n"); > >>> > >>> - /* Init DMA config if supported */ > >>> - i2c_imx_dma_request(i2c_imx, phy_addr); > >>> - > >>> return 0; /* Return OK */ > >>> > >>> clk_notifier_unregister: > >> > >> > >> -- > >> Pengutronix e.K. | > >> | > >> Steuerwalder Str. 21 | > >> http://www/. > >> > pen%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C3e9bcb1cc97b43a69c > a608d > >> > d155d9977%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63869021 > 394510 > >> > 5284%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjA > uMDAw > >> > MCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&s > da > >> ta=qE5UgdZSuGjzXtDFcY1BVbjNgb6sPPrpykvr3gpHsLg%3D&reserved=0 > >> > gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C1acf840d499f > >> > 49a7872408dd0ebedc39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > >> %7C638682935131084746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > Aw > >> > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&s > >> > data=Y9Qn9XEk15yu4CespwsNu6hl3%2FqfNTvEeOn4ZvnGxbo%3D&reserved=0 > >> | > >> 31137 Hildesheim, Germany | Phone: > +49-5121-206917-0 > >> | > >> Amtsgericht Hildesheim, HRA 2686 | Fax: > >> +49-5121-206917-5555 | > > > > > -- > Pengutronix e.K. | > | > Steuerwalder Str. 21 | > http://www.pen/ > gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C3e9bcb1cc97b > 43a69ca608dd155d9977%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > %7C638690213945119868%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki > OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ > %3D%3D%7C0%7C%7C%7C&sdata=SU8Ak78%2BZCcOo4G%2F9Kq1E3IZNAgg5E > 0m1CC1qr4ANYk%3D&reserved=0 | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
Hi, On 06.12.24 10:07, Carlos Song wrote: >> -----Original Message----- >> From: Ahmad Fatoum <a.fatoum@pengutronix.de> >> Sent: Friday, December 6, 2024 2:50 AM > Refer previous history: > https://lore.kernel.org/linux-arm-kernel/6b39268b-7487-427d-aff5-f3ca3b2afd42@pengutronix.de/ > Before replying to your question, I think we should synchronize SDMA and eDMA firstly. [snip] > SDMA initialization in i2c-imx driver: > > + dma->chan_tx = dma_request_chan(dev, "rx-tx"); > + if (IS_ERR(dma->chan_tx)) { > + ret = PTR_ERR(dma->chan_tx); > + goto fail_dma; > + } > + > + dma->chan_rx = dma->chan_tx; > + i2c_imx->dma = dma; > > eDMA initialization in i2c-imx driver: > > ... > + dma->chan_tx = dma_request_chan(dev, "tx"); > .... > + dma->chan_rx = dma_request_chan(dev, "rx"); > .... > i2c_imx->dma = dma; Thanks for the clarification, I missed that. > So if need to enable SDMA, should define a separate dma_request function and should not reuse the current i2c_imx_dma_request function. > Also according to different platforms i2c-imx driver need to choose to use eDMA or SDMA. Understood. > So now we start to discuss about your confusion: > >> My concern is this configuration: >> >> - A user has eDMA/SDMA module or disabled, but enabled in DT > [Carlos]: > I delete edma channel at dts to disable eDMA before. It works in PIO mode. > I also test your case : when enable dma channel in DT but disable eDMA module. It will meet error: > > +++ b/arch/arm64/configs/defconfig > .... > -CONFIG_FSL_EDMA=y > .... > > root@ls1043ardb:~# dmesg | grep i2c > [ 4.697392] i2c_dev: i2c /dev entries driver > [ 18.357685] platform 2180000.i2c: deferred probe pending: (reason unknown) > root@ls1043ardb:~# i2cdetect -y -l > > The case you mentioned is completely inconsistent with the actual usage. Since you have defined the dma channel in dts, it means that you need to > use DMA mode in i2c, but you disabled the eDMA module when building the Image. This makes no sense at all. I think this is a usage error. > And the deferred probe pending error is predictable. Since there is no DMA driver loaded, I2C will keep defer probe and be hanged. I (or rather the user) didn't knowingly define anything though. They just used the upstream DT, which always enables the DMA nodes and it worked fine without DMA support enabled in the config for years. Now they update the kernel after your patches are merged and then the I2C driver is no longer being probed. > >> - The I2C has a PMIC, which is needed for eMMC VCC > [Carlos]: > PMIC is critical for the whole board, so PMIC will finish all critical system power-on configuration(include eMMC VCC) in the uboot not at kernel. > So pmic driver probe in kernel won't and shouldn't effect system boot. > >> - System startup is stuck or considerably delayed >> > The purpose of defer probe is to solve the problem of interdependence of module loading, and to try to load again after a while is to ensure that the required functions > will not be unavailable because the resources are not ready. This delay is unavoidable. If a defer probe occurs, the first consideration should be to reasonably adjust the > loading order of each module, rather than directly giving up the functions that should be enabled. If I2C is a critical dependency, the system becomes unbootable instead of the user just having to wait a little longer. To me that is a regression. You clarified though that this is only for Layerscape, so the breakage is more limited, because e.g. on LS1046A-RDB only EEPROM/sensors/RTC won't be available and system should still be able to boot. So while I am not yet convinced an equivalent of this change to be a good idea for i.MX, which tends to have critical peripherals like PMIC on the I2C, I don't object to it being done for Layerscape. You may add my: Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Cheers, Ahmad
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx) } /* Functions for DMA support */ -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, - dma_addr_t phy_addr) +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr) { struct imx_i2c_dma *dma; struct dma_slave_config dma_sconfig; - struct device *dev = &i2c_imx->adapter.dev; + struct device *dev = i2c_imx->adapter.dev.parent; int ret; dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL); if (!dma) - return; + return -ENOMEM; dma->chan_tx = dma_request_chan(dev, "tx"); if (IS_ERR(dma->chan_tx)) { @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n", dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx)); - return; + return 0; fail_rx: dma_release_channel(dma->chan_rx); @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_release_channel(dma->chan_tx); fail_al: devm_kfree(dev, dma); + + return ret; } static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@ static int i2c_imx_probe(struct platform_device *pdev) if (ret == -EPROBE_DEFER) goto clk_notifier_unregister; + /* + * Init DMA config if supported, -ENODEV means DMA not enabled at + * this platform, that is not a real error, so just remind "only + * PIO mode is used". If DMA is enabled, but meet error when request + * DMA channel, error should be showed in probe error log. PIO mode + * should be available regardless of DMA. + */ + ret = i2c_imx_dma_request(i2c_imx, phy_addr); + if (ret) { + if (ret == -EPROBE_DEFER) + goto clk_notifier_unregister; + else if (ret == -ENODEV) + dev_dbg(&pdev->dev, "Only use PIO mode\n"); + else + dev_err_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n"); + } + /* Add I2C adapter */ ret = i2c_add_numbered_adapter(&i2c_imx->adapter); if (ret < 0) @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct platform_device *pdev) i2c_imx->adapter.name); dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); - /* Init DMA config if supported */ - i2c_imx_dma_request(i2c_imx, phy_addr); - return 0; /* Return OK */ clk_notifier_unregister: