| Message ID | 20241127083818.2108201-1-carlos.song@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] i2c: imx: support DMA defer probing | expand |
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: