> -----Original Message----- > From: Carlos Song > Sent: Wednesday, November 27, 2024 6:43 PM > To: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>; mkl@xxxxxxxxxxxxxx; Frank Li > <frank.li@xxxxxxx>; o.rempel@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > andi.shyti@xxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > festevam@xxxxxxxxx > Cc: imx@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v4] i2c: imx: support DMA defer probing > > > > > -----Original Message----- > > From: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> > > Sent: Wednesday, November 27, 2024 4:38 PM > > To: Carlos Song <carlos.song@xxxxxxx>; mkl@xxxxxxxxxxxxxx; Frank Li > > <frank.li@xxxxxxx>; o.rempel@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > > andi.shyti@xxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > festevam@xxxxxxxxx > > Cc: imx@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > > 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@xxxxxxx wrote: > > > From: Carlos Song <carlos.song@xxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ 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@xxxxxxx> > > > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx> > > > --- > > > 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 |
