Re: [PATCH v4] i2c: imx: support DMA defer probing

From: Carlos Song
Date: Fri Dec 06 2024 - 04:09:05 EST

Next message: Srikar Dronamraju: "Re: [PATCH] sched/fair: Fix CPU bandwidth limit bypass during CPU hotplug"
Previous message: Vincent Mailhol: "Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()"
In reply to: Ahmad Fatoum: "Re: [PATCH v4] i2c: imx: support DMA defer probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> Sent: Friday, December 6, 2024 2:50 AM
> 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 11:43, Carlos Song wrote:
> >> -----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
>
> >> 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@xxxxxxxxxxxxxx/

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@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/.
> >>
> 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 |

Next message: Srikar Dronamraju: "Re: [PATCH] sched/fair: Fix CPU bandwidth limit bypass during CPU hotplug"
Previous message: Vincent Mailhol: "Re: [PATCH 02/10] compiler.h: add is_const() as a replacement of __is_constexpr()"
In reply to: Ahmad Fatoum: "Re: [PATCH v4] i2c: imx: support DMA defer probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]