[PATCH 00/11] i.MX SDMA cleanups and fixes

[PATCH 00/11] i.MX SDMA cleanups and fixes

[Marco Felsch]

Hi,

the current i.MX SDMA driver doesn't honor current active DMA users once
the i.MX SDMA driver is getting removed. Which can lead into very
situations e.g. hang the whole system.

This is fixed by cleaning up the driver and adding devlink support to
the SDMA driver.

This series also fixes the i.MX SDMA handling on i.MX8M* devices, which
do have multiple SPBA buses.

Regards,
  Marco

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Marco Felsch (11):
      dmaengine: imx-sdma: drop legacy device_node np check
      dmaengine: imx-sdma: sdma_remove minor cleanups
      dmaengine: imx-sdma: cosmetic cleanup
      dmaengine: imx-sdma: make use of devm_kzalloc for script_addrs
      dmaengine: imx-sdma: make use of devm_clk_get_prepared()
      dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device
      dmaengine: imx-sdma: make use of dev_err_probe()
      dmaengine: imx-sdma: fix missing of_dma_controller_free()
      dmaengine: add support for device_link
      dmaengine: imx-sdma: drop remove callback
      dmaengine: imx-sdma: fix spba-bus handling for i.MX8M

 drivers/dma/dmaengine.c |   8 +++
 drivers/dma/imx-sdma.c  | 188 +++++++++++++++++++++++-------------------------
 2 files changed, 97 insertions(+), 99 deletions(-)
---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250903-v6-16-topic-sdma-4c8fd3bb0738

Best regards,
-- 
Marco Felsch <m.felsch@pengutronix.de>

[PATCH 01/11] dmaengine: imx-sdma: drop legacy device_node np check

[Marco Felsch]

The legacy 'if (np)' was required in past where we had pdata and dt.
Nowadays the driver binds only to dt platforms. So using a new kernel
but still use pdata is not possible, therefore we can drop the legacy
'if' code path.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 02a85d6f1bea2df7d355858094c0c0b0bd07148e..89b4b1266726a9c8a552dc48670825ae00717e1c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2325,11 +2325,9 @@ static int sdma_probe(struct platform_device *pdev)
 			vchan_init(&sdmac->vc, &sdma->dma_device);
 	}
 
-	if (np) {
-		sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
-		if (sdma->iram_pool)
-			dev_info(&pdev->dev, "alloc bd from iram.\n");
-	}
+	sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
+	if (sdma->iram_pool)
+		dev_info(&pdev->dev, "alloc bd from iram.\n");
 
 	ret = sdma_init(sdma);
 	if (ret)
@@ -2369,21 +2367,19 @@ static int sdma_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	if (np) {
-		ret = of_dma_controller_register(np, sdma_xlate, sdma);
-		if (ret) {
-			dev_err(&pdev->dev, "failed to register controller\n");
-			goto err_register;
-		}
+	ret = of_dma_controller_register(np, sdma_xlate, sdma);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register controller\n");
+		goto err_register;
+	}
 
-		spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
-		ret = of_address_to_resource(spba_bus, 0, &spba_res);
-		if (!ret) {
-			sdma->spba_start_addr = spba_res.start;
-			sdma->spba_end_addr = spba_res.end;
-		}
-		of_node_put(spba_bus);
+	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
+	ret = of_address_to_resource(spba_bus, 0, &spba_res);
+	if (!ret) {
+		sdma->spba_start_addr = spba_res.start;
+		sdma->spba_end_addr = spba_res.end;
 	}
+	of_node_put(spba_bus);
 
 	/*
 	 * Because that device tree does not encode ROM script address,

-- 
2.47.2

[PATCH 02/11] dmaengine: imx-sdma: sdma_remove minor cleanups

[Marco Felsch]

We don't need to set the pdev driver data to NULL since the device will
be freed anyways.

Also drop the tasklet_kill() since this is done by the virt-dma driver
during the vchan_synchronize().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 89b4b1266726a9c8a552dc48670825ae00717e1c..422086632d3445b2ce3f2e5df9b2130174a311e8 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2423,11 +2423,8 @@ static void sdma_remove(struct platform_device *pdev)
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];
 
-		tasklet_kill(&sdmac->vc.task);
 		sdma_free_chan_resources(&sdmac->vc.chan);
 	}
-
-	platform_set_drvdata(pdev, NULL);
 }
 
 static struct platform_driver sdma_driver = {

-- 
2.47.2

[PATCH 03/11] dmaengine: imx-sdma: cosmetic cleanup

[Marco Felsch]

Make use of local struct device pointer to not dereference the
platform_device pointer everytime.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 422086632d3445b2ce3f2e5df9b2130174a311e8..a85739d279f51fdb517fce90b3dc67673cf2b56c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2234,7 +2234,8 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
 
 static int sdma_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct device_node *spba_bus;
 	const char *fw_name;
 	int ret;
@@ -2244,18 +2245,18 @@ static int sdma_probe(struct platform_device *pdev)
 	struct sdma_engine *sdma;
 	s32 *saddr_arr;
 
-	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
 
-	sdma = devm_kzalloc(&pdev->dev, sizeof(*sdma), GFP_KERNEL);
+	sdma = devm_kzalloc(dev, sizeof(*sdma), GFP_KERNEL);
 	if (!sdma)
 		return -ENOMEM;
 
 	spin_lock_init(&sdma->channel_0_lock);
 
-	sdma->dev = &pdev->dev;
-	sdma->drvdata = of_device_get_match_data(sdma->dev);
+	sdma->dev = dev;
+	sdma->drvdata = of_device_get_match_data(dev);
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -2265,11 +2266,11 @@ static int sdma_probe(struct platform_device *pdev)
 	if (IS_ERR(sdma->regs))
 		return PTR_ERR(sdma->regs);
 
-	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	sdma->clk_ipg = devm_clk_get(dev, "ipg");
 	if (IS_ERR(sdma->clk_ipg))
 		return PTR_ERR(sdma->clk_ipg);
 
-	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
+	sdma->clk_ahb = devm_clk_get(dev, "ahb");
 	if (IS_ERR(sdma->clk_ahb))
 		return PTR_ERR(sdma->clk_ahb);
 
@@ -2281,8 +2282,8 @@ static int sdma_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_clk;
 
-	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
-				dev_name(&pdev->dev), sdma);
+	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
+			       dev_name(dev), sdma);
 	if (ret)
 		goto err_irq;
 
@@ -2327,7 +2328,7 @@ static int sdma_probe(struct platform_device *pdev)
 
 	sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
 	if (sdma->iram_pool)
-		dev_info(&pdev->dev, "alloc bd from iram.\n");
+		dev_info(dev, "alloc bd from iram.\n");
 
 	ret = sdma_init(sdma);
 	if (ret)
@@ -2340,7 +2341,7 @@ static int sdma_probe(struct platform_device *pdev)
 	if (sdma->drvdata->script_addrs)
 		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
 
-	sdma->dma_device.dev = &pdev->dev;
+	sdma->dma_device.dev = dev;
 
 	sdma->dma_device.device_alloc_chan_resources = sdma_alloc_chan_resources;
 	sdma->dma_device.device_free_chan_resources = sdma_free_chan_resources;
@@ -2363,13 +2364,13 @@ static int sdma_probe(struct platform_device *pdev)
 
 	ret = dma_async_device_register(&sdma->dma_device);
 	if (ret) {
-		dev_err(&pdev->dev, "unable to register\n");
+		dev_err(dev, "unable to register\n");
 		goto err_init;
 	}
 
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
 	if (ret) {
-		dev_err(&pdev->dev, "failed to register controller\n");
+		dev_err(dev, "failed to register controller\n");
 		goto err_register;
 	}
 
@@ -2389,11 +2390,11 @@ static int sdma_probe(struct platform_device *pdev)
 	ret = of_property_read_string(np, "fsl,sdma-ram-script-name",
 				      &fw_name);
 	if (ret) {
-		dev_warn(&pdev->dev, "failed to get firmware name\n");
+		dev_warn(dev, "failed to get firmware name\n");
 	} else {
 		ret = sdma_get_firmware(sdma, fw_name);
 		if (ret)
-			dev_warn(&pdev->dev, "failed to get firmware from device tree\n");
+			dev_warn(dev, "failed to get firmware from device tree\n");
 	}
 
 	return 0;

-- 
2.47.2

[PATCH 04/11] dmaengine: imx-sdma: make use of devm_kzalloc for script_addrs

[Marco Felsch]

Shuffle the allocation of script_addrs and make use of devm_kzalloc() to
drop the local error handling as well as the kfree() during the remove.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a85739d279f51fdb517fce90b3dc67673cf2b56c..b6e649fda71dbce12a2106c94887f90d0aaaf600 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2253,6 +2253,10 @@ static int sdma_probe(struct platform_device *pdev)
 	if (!sdma)
 		return -ENOMEM;
 
+	sdma->script_addrs = devm_kzalloc(dev, sizeof(*sdma->script_addrs), GFP_KERNEL);
+	if (!sdma->script_addrs)
+		return -ENOMEM;
+
 	spin_lock_init(&sdma->channel_0_lock);
 
 	sdma->dev = dev;
@@ -2289,12 +2293,6 @@ static int sdma_probe(struct platform_device *pdev)
 
 	sdma->irq = irq;
 
-	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
-	if (!sdma->script_addrs) {
-		ret = -ENOMEM;
-		goto err_irq;
-	}
-
 	/* initially no scripts available */
 	saddr_arr = (s32 *)sdma->script_addrs;
 	for (i = 0; i < sizeof(*sdma->script_addrs) / sizeof(s32); i++)
@@ -2332,11 +2330,11 @@ static int sdma_probe(struct platform_device *pdev)
 
 	ret = sdma_init(sdma);
 	if (ret)
-		goto err_init;
+		goto err_irq;
 
 	ret = sdma_event_remap(sdma);
 	if (ret)
-		goto err_init;
+		goto err_irq;
 
 	if (sdma->drvdata->script_addrs)
 		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
@@ -2365,7 +2363,7 @@ static int sdma_probe(struct platform_device *pdev)
 	ret = dma_async_device_register(&sdma->dma_device);
 	if (ret) {
 		dev_err(dev, "unable to register\n");
-		goto err_init;
+		goto err_irq;
 	}
 
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
@@ -2401,8 +2399,6 @@ static int sdma_probe(struct platform_device *pdev)
 
 err_register:
 	dma_async_device_unregister(&sdma->dma_device);
-err_init:
-	kfree(sdma->script_addrs);
 err_irq:
 	clk_unprepare(sdma->clk_ahb);
 err_clk:
@@ -2417,7 +2413,6 @@ static void sdma_remove(struct platform_device *pdev)
 
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
 	dma_async_device_unregister(&sdma->dma_device);
-	kfree(sdma->script_addrs);
 	clk_unprepare(sdma->clk_ahb);
 	clk_unprepare(sdma->clk_ipg);
 	/* Kill the tasklet */

-- 
2.47.2

[PATCH 05/11] dmaengine: imx-sdma: make use of devm_clk_get_prepared()

[Marco Felsch]

Make use of the devm_clk_get_prepared() to cleanup the error handling
during probe() and to automatically unprepare the clock during remove.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b6e649fda71dbce12a2106c94887f90d0aaaf600..5a571d3f33158813e0c56484600a49b19a6a72e2 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2270,26 +2270,18 @@ static int sdma_probe(struct platform_device *pdev)
 	if (IS_ERR(sdma->regs))
 		return PTR_ERR(sdma->regs);
 
-	sdma->clk_ipg = devm_clk_get(dev, "ipg");
+	sdma->clk_ipg = devm_clk_get_prepared(dev, "ipg");
 	if (IS_ERR(sdma->clk_ipg))
 		return PTR_ERR(sdma->clk_ipg);
 
-	sdma->clk_ahb = devm_clk_get(dev, "ahb");
+	sdma->clk_ahb = devm_clk_get_prepared(dev, "ahb");
 	if (IS_ERR(sdma->clk_ahb))
 		return PTR_ERR(sdma->clk_ahb);
 
-	ret = clk_prepare(sdma->clk_ipg);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare(sdma->clk_ahb);
-	if (ret)
-		goto err_clk;
-
 	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
 			       dev_name(dev), sdma);
 	if (ret)
-		goto err_irq;
+		return ret;
 
 	sdma->irq = irq;
 
@@ -2330,11 +2322,11 @@ static int sdma_probe(struct platform_device *pdev)
 
 	ret = sdma_init(sdma);
 	if (ret)
-		goto err_irq;
+		return ret;
 
 	ret = sdma_event_remap(sdma);
 	if (ret)
-		goto err_irq;
+		return ret;
 
 	if (sdma->drvdata->script_addrs)
 		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
@@ -2363,7 +2355,7 @@ static int sdma_probe(struct platform_device *pdev)
 	ret = dma_async_device_register(&sdma->dma_device);
 	if (ret) {
 		dev_err(dev, "unable to register\n");
-		goto err_irq;
+		return ret;
 	}
 
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
@@ -2399,10 +2391,7 @@ static int sdma_probe(struct platform_device *pdev)
 
 err_register:
 	dma_async_device_unregister(&sdma->dma_device);
-err_irq:
-	clk_unprepare(sdma->clk_ahb);
-err_clk:
-	clk_unprepare(sdma->clk_ipg);
+
 	return ret;
 }
 
@@ -2413,8 +2402,6 @@ static void sdma_remove(struct platform_device *pdev)
 
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
 	dma_async_device_unregister(&sdma->dma_device);
-	clk_unprepare(sdma->clk_ahb);
-	clk_unprepare(sdma->clk_ipg);
 	/* Kill the tasklet */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];

-- 
2.47.2

[PATCH 06/11] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Marco Felsch]

Make use of the devm_add_action_or_reset() to register a custom devm_
release hook. This is required since we want to turn of the IRQs before
doing the dma_async_device_unregister().

This removes the last goto error handling within the probe function and
further trims the remove() function. Instead of freeing the irq, we can
disable it and let the devm-irq do the job to free the irq, since the
only purpose was to have the irqs disabled before calling
dma_async_device_unregister().

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 5a571d3f33158813e0c56484600a49b19a6a72e2..f6bb2f88a62781c0431336c365fa30c46f1401ad 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2232,6 +2232,14 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
 				     ofdma->of_node);
 }
 
+static void sdma_dma_device_unregister_action(void *data)
+{
+	struct sdma_engine *sdma = data;
+
+	disable_irq(sdma->irq);
+	dma_async_device_unregister(&sdma->dma_device);
+}
+
 static int sdma_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2358,10 +2366,12 @@ static int sdma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
+
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
 	if (ret) {
 		dev_err(dev, "failed to register controller\n");
-		goto err_register;
+		return ret;
 	}
 
 	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
@@ -2388,11 +2398,6 @@ static int sdma_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_register:
-	dma_async_device_unregister(&sdma->dma_device);
-
-	return ret;
 }
 
 static void sdma_remove(struct platform_device *pdev)
@@ -2400,8 +2405,6 @@ static void sdma_remove(struct platform_device *pdev)
 	struct sdma_engine *sdma = platform_get_drvdata(pdev);
 	int i;
 
-	devm_free_irq(&pdev->dev, sdma->irq, sdma);
-	dma_async_device_unregister(&sdma->dma_device);
 	/* Kill the tasklet */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];

-- 
2.47.2

[PATCH 07/11] dmaengine: imx-sdma: make use of dev_err_probe()

[Marco Felsch]

Convert the probe function to dev_err_probe() which helps users to
identify issues better.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index f6bb2f88a62781c0431336c365fa30c46f1401ad..e30dd46cf6522ee2aa4d3aca9868a01afbd29615 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2255,7 +2255,7 @@ static int sdma_probe(struct platform_device *pdev)
 
 	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to set DMA mask\n");
 
 	sdma = devm_kzalloc(dev, sizeof(*sdma), GFP_KERNEL);
 	if (!sdma)
@@ -2272,24 +2272,24 @@ static int sdma_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
-		return irq;
+		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
 
 	sdma->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(sdma->regs))
-		return PTR_ERR(sdma->regs);
+		return dev_err_probe(dev, PTR_ERR(sdma->regs), "ioremap failed\n");
 
 	sdma->clk_ipg = devm_clk_get_prepared(dev, "ipg");
 	if (IS_ERR(sdma->clk_ipg))
-		return PTR_ERR(sdma->clk_ipg);
+		return dev_err_probe(dev, PTR_ERR(sdma->clk_ipg), "IPG clk_get_prepared failed\n");
 
 	sdma->clk_ahb = devm_clk_get_prepared(dev, "ahb");
 	if (IS_ERR(sdma->clk_ahb))
-		return PTR_ERR(sdma->clk_ahb);
+		return dev_err_probe(dev, PTR_ERR(sdma->clk_ahb), "AHB clk_get_prepared failed\n");
 
 	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
 			       dev_name(dev), sdma);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
 
 	sdma->irq = irq;
 
@@ -2330,11 +2330,11 @@ static int sdma_probe(struct platform_device *pdev)
 
 	ret = sdma_init(sdma);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "sdma_init failed\n");
 
 	ret = sdma_event_remap(sdma);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "sdma_event_remap failed\n");
 
 	if (sdma->drvdata->script_addrs)
 		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
@@ -2361,18 +2361,14 @@ static int sdma_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sdma);
 
 	ret = dma_async_device_register(&sdma->dma_device);
-	if (ret) {
-		dev_err(dev, "unable to register\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "unable to register\n");
 
 	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
 
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
-	if (ret) {
-		dev_err(dev, "failed to register controller\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register controller\n");
 
 	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
 	ret = of_address_to_resource(spba_bus, 0, &spba_res);

-- 
2.47.2

[PATCH 08/11] dmaengine: imx-sdma: fix missing of_dma_controller_free()

[Marco Felsch]

Add the missing of_dma_controller_free() to free the resources allocated
via of_dma_controller_register(). The missing free was introduced long
time ago  by commit 23e118113782 ("dma: imx-sdma: use
module_platform_driver for SDMA driver") while adding a proper .remove()
implementation.

Fixes: 23e118113782 ("dma: imx-sdma: use module_platform_driver for SDMA driver")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index e30dd46cf6522ee2aa4d3aca9868a01afbd29615..6c6d38b202dd2deffc36b1bd27bc7c60de3d7403 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2232,6 +2232,13 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
 				     ofdma->of_node);
 }
 
+static void sdma_dma_of_dma_controller_unregister_action(void *data)
+{
+	struct sdma_engine *sdma = data;
+
+	of_dma_controller_free(sdma->dev->of_node);
+}
+
 static void sdma_dma_device_unregister_action(void *data)
 {
 	struct sdma_engine *sdma = data;
@@ -2370,6 +2377,8 @@ static int sdma_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to register controller\n");
 
+	devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
+
 	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
 	ret = of_address_to_resource(spba_bus, 0, &spba_res);
 	if (!ret) {

-- 
2.47.2

[PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

Add support to create device_links between dmaengine suppliers and the
dma consumers. This shifts the device dep-chain teardown/bringup logic
to the driver core.

Moving this to the core allows the dmaengine drivers to simplify the
.remove() hooks and also to ensure that no dmaengine driver is ever
removed before the consumer is removed.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/dmaengine.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct dma_device *d, *_d;
 	struct dma_chan *chan = NULL;
+	struct device_link *dl;
 
 	if (is_of_node(fwnode))
 		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
@@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
 	/* No functional issue if it fails, users are supposed to test before use */
 #endif
 
+	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+	if (!dl) {
+		dev_err(dev, "failed to create device link to %s\n",
+			dev_name(chan->device->dev));
+		return ERR_PTR(-EINVAL);
+	}
+
 	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
 	if (!chan->name)
 		return chan;

-- 
2.47.2

[PATCH 10/11] dmaengine: imx-sdma: drop remove callback

[Marco Felsch]

The whole driver was converted to the devm APIs except for this last
for-loop. This loop is buggy due to three reasons:
 1) It removes the channels without removing the users first. This can
    lead to very bad situations.
 2) The loop starts at 0 and which is channel0 which is a special
    control channel not registered via vchan_init(). Therefore the
    remove() always Oops because of NULL pointer exception.
 3) sdma_free_chan_resources() disable the clks unconditional without
    checking if the clks are enabled. This is done for all
    MAX_DMA_CHANNELS which hang the system if there is at least one unused
    channel.

Since the dmaengine core supports devlinks we already addressed the
first issue.

The devlink support also addresses the third issue because during the
consumer teardown phase each requested channel is dropped accordingly so
the dmaengine driver doesn't need to this.

The second issue is fixed by not doing anything on channel0.

To sum-up, all issues are fixed by dropping the .remove() callback and
let the frameworks do their job.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 6c6d38b202dd2deffc36b1bd27bc7c60de3d7403..c31785977351163d6fddf4d8b2f90dfebb508400 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2405,25 +2405,11 @@ static int sdma_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void sdma_remove(struct platform_device *pdev)
-{
-	struct sdma_engine *sdma = platform_get_drvdata(pdev);
-	int i;
-
-	/* Kill the tasklet */
-	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
-		struct sdma_channel *sdmac = &sdma->channel[i];
-
-		sdma_free_chan_resources(&sdmac->vc.chan);
-	}
-}
-
 static struct platform_driver sdma_driver = {
 	.driver		= {
 		.name	= "imx-sdma",
 		.of_match_table = sdma_dt_ids,
 	},
-	.remove		= sdma_remove,
 	.probe		= sdma_probe,
 };
 

-- 
2.47.2

[PATCH 11/11] dmaengine: imx-sdma: fix spba-bus handling for i.MX8M

[Marco Felsch]

Starting with i.MX8M* devices there are multiple spba-busses so we can't
just search the whole DT for the first spba-bus match and take it.
Instead we need to check for each device to which bus it belongs and
setup the spba_{start,end}_addr accordingly per sdma_channel.

While on it, don't ignore errors from of_address_to_resource() if they
are valid.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 56 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index c31785977351163d6fddf4d8b2f90dfebb508400..3ef415aa578a96e35a969ac2488d08bcab9fadc3 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -461,6 +461,8 @@ struct sdma_channel {
 	dma_addr_t			per_address, per_address2;
 	unsigned long			event_mask[2];
 	unsigned long			watermark_level;
+	u32				spba_start_addr;
+	u32				spba_end_addr;
 	u32				shp_addr, per_addr;
 	enum dma_status			status;
 	struct imx_dma_data		data;
@@ -534,8 +536,6 @@ struct sdma_engine {
 	u32				script_number;
 	struct sdma_script_start_addrs	*script_addrs;
 	const struct sdma_driver_data	*drvdata;
-	u32				spba_start_addr;
-	u32				spba_end_addr;
 	unsigned int			irq;
 	dma_addr_t			bd0_phys;
 	struct sdma_buffer_descriptor	*bd0;
@@ -1236,8 +1236,6 @@ static void sdma_channel_synchronize(struct dma_chan *chan)
 
 static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
 {
-	struct sdma_engine *sdma = sdmac->sdma;
-
 	int lwml = sdmac->watermark_level & SDMA_WATERMARK_LEVEL_LWML;
 	int hwml = (sdmac->watermark_level & SDMA_WATERMARK_LEVEL_HWML) >> 16;
 
@@ -1263,12 +1261,12 @@ static void sdma_set_watermarklevel_for_p2p(struct sdma_channel *sdmac)
 		swap(sdmac->event_mask[0], sdmac->event_mask[1]);
 	}
 
-	if (sdmac->per_address2 >= sdma->spba_start_addr &&
-			sdmac->per_address2 <= sdma->spba_end_addr)
+	if (sdmac->per_address2 >= sdmac->spba_start_addr &&
+			sdmac->per_address2 <= sdmac->spba_end_addr)
 		sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_SP;
 
-	if (sdmac->per_address >= sdma->spba_start_addr &&
-			sdmac->per_address <= sdma->spba_end_addr)
+	if (sdmac->per_address >= sdmac->spba_start_addr &&
+			sdmac->per_address <= sdmac->spba_end_addr)
 		sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_DP;
 
 	sdmac->watermark_level |= SDMA_WATERMARK_LEVEL_CONT;
@@ -1447,6 +1445,31 @@ static void sdma_desc_free(struct virt_dma_desc *vd)
 	kfree(desc);
 }
 
+static int sdma_config_spba_slave(struct dma_chan *chan)
+{
+	struct sdma_channel *sdmac = to_sdma_chan(chan);
+	struct device_node *spba_bus;
+	struct resource spba_res;
+	int ret;
+
+	spba_bus = of_get_parent(chan->slave->of_node);
+	/* Device doesn't belong to the spba-bus */
+	if (!of_device_is_compatible(spba_bus, "fsl,spba-bus"))
+		return 0;
+
+	ret = of_address_to_resource(spba_bus, 0, &spba_res);
+	of_node_put(spba_bus);
+	if (ret) {
+		dev_err(sdmac->sdma->dev, "Failed to get spba-bus resources\n");
+		return -EINVAL;
+	}
+
+	sdmac->spba_start_addr = spba_res.start;
+	sdmac->spba_end_addr = spba_res.end;
+
+	return 0;
+}
+
 static int sdma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
@@ -1527,6 +1550,8 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 
 	sdmac->event_id0 = 0;
 	sdmac->event_id1 = 0;
+	sdmac->spba_start_addr = 0;
+	sdmac->spba_end_addr = 0;
 
 	sdma_set_channel_priority(sdmac, 0);
 
@@ -1837,6 +1862,7 @@ static int sdma_config(struct dma_chan *chan,
 {
 	struct sdma_channel *sdmac = to_sdma_chan(chan);
 	struct sdma_engine *sdma = sdmac->sdma;
+	int ret;
 
 	memcpy(&sdmac->slave_config, dmaengine_cfg, sizeof(*dmaengine_cfg));
 
@@ -1867,6 +1893,10 @@ static int sdma_config(struct dma_chan *chan,
 		sdma_event_enable(sdmac, sdmac->event_id1);
 	}
 
+	ret = sdma_config_spba_slave(chan);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -2251,11 +2281,9 @@ static int sdma_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *spba_bus;
 	const char *fw_name;
 	int ret;
 	int irq;
-	struct resource spba_res;
 	int i;
 	struct sdma_engine *sdma;
 	s32 *saddr_arr;
@@ -2379,14 +2407,6 @@ static int sdma_probe(struct platform_device *pdev)
 
 	devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
 
-	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
-	ret = of_address_to_resource(spba_bus, 0, &spba_res);
-	if (!ret) {
-		sdma->spba_start_addr = spba_res.start;
-		sdma->spba_end_addr = spba_res.end;
-	}
-	of_node_put(spba_bus);
-
 	/*
 	 * Because that device tree does not encode ROM script address,
 	 * the RAM script in firmware is mandatory for device tree

-- 
2.47.2

Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> Add support to create device_links between dmaengine suppliers and the
> dma consumers. This shifts the device dep-chain teardown/bringup logic
> to the driver core.
>
> Moving this to the core allows the dmaengine drivers to simplify the
> .remove() hooks and also to ensure that no dmaengine driver is ever
> removed before the consumer is removed.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---

Thank you work for devlink between dmaengine and devices. I have similar
idea.

This patch should be first patch.

The below what planned commit message in my local tree.

Implementing runtime PM for DMA channels is challenging. If a channel
resumes at allocation and suspends at free, the DMA engine often remains on
because most drivers request a channel at probe.

Tracking the number of pending DMA descriptors is also problematic, as some
consumers append new descriptors in atomic contexts, such as IRQ handlers,
where runtime resume cannot be called.

Using a device link simplifies this issue. If a consumer requires data
transfer, it must be in a runtime-resumed state, ensuring that the DMA
channel is also active by device link. This allows safe operations, like
appending new descriptors. Conversely, when the consumer no longer requires
data transfer, both it and the supplier (DMA channel) can enter a suspended
state if no other consumer is using it.

Introduce the `create_link` flag to enable this feature.

also suggest add create_link flag to enable this feature in case some
side impact to other dma-engine. After some time test, we can enable it
default.

>  drivers/dma/dmaengine.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
>  	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct dma_device *d, *_d;
>  	struct dma_chan *chan = NULL;
> +	struct device_link *dl;
>
>  	if (is_of_node(fwnode))
>  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
>  	/* No functional issue if it fails, users are supposed to test before use */
>  #endif
>
> +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

chan->device->dev is dmaengine devices. But some dmaengine's each channel
have device, consumer should link to chan's device, not dmaengine device
because some dmaengine support per channel clock\power management.

chan's device's parent devices is dmaengine devices. it should also work
for sdma case


        if (chan->device->create_devlink) {
                u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;

                if (pm_runtime_active(dev))
                        flags |= DL_FLAG_RPM_ACTIVE;

When create device link (apply channel), consume may active.


                dl = device_link_add(chan->slave, &chan->dev->device, flags);
        }

Need update kernel doc

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index bb146c5ac3e4c..ffb3a8f0070ba 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -323,7 +323,8 @@ struct dma_router {
  * @cookie: last cookie value returned to client
  * @completed_cookie: last completed cookie for this channel
  * @chan_id: channel ID for sysfs
- * @dev: class device for sysfs
+ * @dev: class device for sysfs, also use for pre channel runtime pm and
+ *       use custom/different dma-mapping

Frank


> +	if (!dl) {
> +		dev_err(dev, "failed to create device link to %s\n",
> +			dev_name(chan->device->dev));
> +		return ERR_PTR(-EINVAL);
> +	}
>  	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
>  	if (!chan->name)
>  		return chan;
>
> --
> 2.47.2
>

Re: [PATCH 01/11] dmaengine: imx-sdma: drop legacy device_node np check

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:09PM +0200, Marco Felsch wrote:
> The legacy 'if (np)' was required in past where we had pdata and dt.
> Nowadays the driver binds only to dt platforms. So using a new kernel
> but still use pdata is not possible, therefore we can drop the legacy
> 'if' code path.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/dma/imx-sdma.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 02a85d6f1bea2df7d355858094c0c0b0bd07148e..89b4b1266726a9c8a552dc48670825ae00717e1c 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2325,11 +2325,9 @@ static int sdma_probe(struct platform_device *pdev)
>  			vchan_init(&sdmac->vc, &sdma->dma_device);
>  	}
>
> -	if (np) {
> -		sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
> -		if (sdma->iram_pool)
> -			dev_info(&pdev->dev, "alloc bd from iram.\n");
> -	}
> +	sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
> +	if (sdma->iram_pool)
> +		dev_info(&pdev->dev, "alloc bd from iram.\n");
>
>  	ret = sdma_init(sdma);
>  	if (ret)
> @@ -2369,21 +2367,19 @@ static int sdma_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>
> -	if (np) {
> -		ret = of_dma_controller_register(np, sdma_xlate, sdma);
> -		if (ret) {
> -			dev_err(&pdev->dev, "failed to register controller\n");
> -			goto err_register;
> -		}
> +	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register controller\n");
> +		goto err_register;
> +	}
>
> -		spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> -		ret = of_address_to_resource(spba_bus, 0, &spba_res);
> -		if (!ret) {
> -			sdma->spba_start_addr = spba_res.start;
> -			sdma->spba_end_addr = spba_res.end;
> -		}
> -		of_node_put(spba_bus);
> +	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> +	ret = of_address_to_resource(spba_bus, 0, &spba_res);
> +	if (!ret) {
> +		sdma->spba_start_addr = spba_res.start;
> +		sdma->spba_end_addr = spba_res.end;
>  	}
> +	of_node_put(spba_bus);
>
>  	/*
>  	 * Because that device tree does not encode ROM script address,
>
> --
> 2.47.2
>

Re: [PATCH 02/11] dmaengine: imx-sdma: sdma_remove minor cleanups

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:10PM +0200, Marco Felsch wrote:
> We don't need to set the pdev driver data to NULL since the device will
> be freed anyways.
>
> Also drop the tasklet_kill() since this is done by the virt-dma driver
> during the vchan_synchronize().
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/dma/imx-sdma.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 89b4b1266726a9c8a552dc48670825ae00717e1c..422086632d3445b2ce3f2e5df9b2130174a311e8 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2423,11 +2423,8 @@ static void sdma_remove(struct platform_device *pdev)
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>
> -		tasklet_kill(&sdmac->vc.task);
>  		sdma_free_chan_resources(&sdmac->vc.chan);
>  	}
> -
> -	platform_set_drvdata(pdev, NULL);
>  }
>
>  static struct platform_driver sdma_driver = {
>
> --
> 2.47.2
>

Re: [PATCH 06/11] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:14PM +0200, Marco Felsch wrote:
> Make use of the devm_add_action_or_reset() to register a custom devm_
> release hook. This is required since we want to turn of the IRQs before

turn off?

> doing the dma_async_device_unregister().
>
> This removes the last goto error handling within the probe function and

Remove the last ..

> further trims the remove() function. Instead of freeing the irq, we can
> disable it and let the devm-irq do the job to free the irq, since the
> only purpose was to have the irqs disabled before calling
> dma_async_device_unregister().
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 5a571d3f33158813e0c56484600a49b19a6a72e2..f6bb2f88a62781c0431336c365fa30c46f1401ad 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2232,6 +2232,14 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
>  				     ofdma->of_node);
>  }
>
> +static void sdma_dma_device_unregister_action(void *data)
> +{
> +	struct sdma_engine *sdma = data;
> +
> +	disable_irq(sdma->irq);
> +	dma_async_device_unregister(&sdma->dma_device);
> +}
> +
>  static int sdma_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -2358,10 +2366,12 @@ static int sdma_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> +	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> +

need check return value.

Frank

>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
>  	if (ret) {
>  		dev_err(dev, "failed to register controller\n");
> -		goto err_register;
> +		return ret;
>  	}
>
>  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> @@ -2388,11 +2398,6 @@ static int sdma_probe(struct platform_device *pdev)
>  	}
>
>  	return 0;
> -
> -err_register:
> -	dma_async_device_unregister(&sdma->dma_device);
> -
> -	return ret;
>  }
>
>  static void sdma_remove(struct platform_device *pdev)
> @@ -2400,8 +2405,6 @@ static void sdma_remove(struct platform_device *pdev)
>  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
>  	int i;
>
> -	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> -	dma_async_device_unregister(&sdma->dma_device);
>  	/* Kill the tasklet */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>
> --
> 2.47.2
>

Re: [PATCH 03/11] dmaengine: imx-sdma: cosmetic cleanup

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:11PM +0200, Marco Felsch wrote:
> Make use of local struct device pointer to not dereference the
> platform_device pointer everytime.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 422086632d3445b2ce3f2e5df9b2130174a311e8..a85739d279f51fdb517fce90b3dc67673cf2b56c 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2234,7 +2234,8 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
>
>  static int sdma_probe(struct platform_device *pdev)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
>  	struct device_node *spba_bus;
>  	const char *fw_name;
>  	int ret;
> @@ -2244,18 +2245,18 @@ static int sdma_probe(struct platform_device *pdev)
>  	struct sdma_engine *sdma;
>  	s32 *saddr_arr;
>
> -	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret)
>  		return ret;

Not related this patch, you can remove this check later because
dma_coerce_mask_and_coherent() never return failure when >= 32bit.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> -	sdma = devm_kzalloc(&pdev->dev, sizeof(*sdma), GFP_KERNEL);
> +	sdma = devm_kzalloc(dev, sizeof(*sdma), GFP_KERNEL);
>  	if (!sdma)
>  		return -ENOMEM;
>
>  	spin_lock_init(&sdma->channel_0_lock);
>
> -	sdma->dev = &pdev->dev;
> -	sdma->drvdata = of_device_get_match_data(sdma->dev);
> +	sdma->dev = dev;
> +	sdma->drvdata = of_device_get_match_data(dev);
>
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> @@ -2265,11 +2266,11 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (IS_ERR(sdma->regs))
>  		return PTR_ERR(sdma->regs);
>
> -	sdma->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	sdma->clk_ipg = devm_clk_get(dev, "ipg");
>  	if (IS_ERR(sdma->clk_ipg))
>  		return PTR_ERR(sdma->clk_ipg);
>
> -	sdma->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	sdma->clk_ahb = devm_clk_get(dev, "ahb");
>  	if (IS_ERR(sdma->clk_ahb))
>  		return PTR_ERR(sdma->clk_ahb);
>
> @@ -2281,8 +2282,8 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_clk;
>
> -	ret = devm_request_irq(&pdev->dev, irq, sdma_int_handler, 0,
> -				dev_name(&pdev->dev), sdma);
> +	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
> +			       dev_name(dev), sdma);
>  	if (ret)
>  		goto err_irq;
>
> @@ -2327,7 +2328,7 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	sdma->iram_pool = of_gen_pool_get(np, "iram", 0);
>  	if (sdma->iram_pool)
> -		dev_info(&pdev->dev, "alloc bd from iram.\n");
> +		dev_info(dev, "alloc bd from iram.\n");
>
>  	ret = sdma_init(sdma);
>  	if (ret)
> @@ -2340,7 +2341,7 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (sdma->drvdata->script_addrs)
>  		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
>
> -	sdma->dma_device.dev = &pdev->dev;
> +	sdma->dma_device.dev = dev;
>
>  	sdma->dma_device.device_alloc_chan_resources = sdma_alloc_chan_resources;
>  	sdma->dma_device.device_free_chan_resources = sdma_free_chan_resources;
> @@ -2363,13 +2364,13 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	ret = dma_async_device_register(&sdma->dma_device);
>  	if (ret) {
> -		dev_err(&pdev->dev, "unable to register\n");
> +		dev_err(dev, "unable to register\n");
>  		goto err_init;
>  	}
>
>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to register controller\n");
> +		dev_err(dev, "failed to register controller\n");
>  		goto err_register;
>  	}
>
> @@ -2389,11 +2390,11 @@ static int sdma_probe(struct platform_device *pdev)
>  	ret = of_property_read_string(np, "fsl,sdma-ram-script-name",
>  				      &fw_name);
>  	if (ret) {
> -		dev_warn(&pdev->dev, "failed to get firmware name\n");
> +		dev_warn(dev, "failed to get firmware name\n");
>  	} else {
>  		ret = sdma_get_firmware(sdma, fw_name);
>  		if (ret)
> -			dev_warn(&pdev->dev, "failed to get firmware from device tree\n");
> +			dev_warn(dev, "failed to get firmware from device tree\n");
>  	}
>
>  	return 0;
>
> --
> 2.47.2
>

Re: [PATCH 04/11] dmaengine: imx-sdma: make use of devm_kzalloc for script_addrs

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:12PM +0200, Marco Felsch wrote:
> Shuffle the allocation of script_addrs and make use of devm_kzalloc() to
> drop the local error handling as well as the kfree() during the remove.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/imx-sdma.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index a85739d279f51fdb517fce90b3dc67673cf2b56c..b6e649fda71dbce12a2106c94887f90d0aaaf600 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2253,6 +2253,10 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (!sdma)
>  		return -ENOMEM;
>
> +	sdma->script_addrs = devm_kzalloc(dev, sizeof(*sdma->script_addrs), GFP_KERNEL);
> +	if (!sdma->script_addrs)
> +		return -ENOMEM;
> +
>  	spin_lock_init(&sdma->channel_0_lock);
>
>  	sdma->dev = dev;
> @@ -2289,12 +2293,6 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	sdma->irq = irq;
>
> -	sdma->script_addrs = kzalloc(sizeof(*sdma->script_addrs), GFP_KERNEL);
> -	if (!sdma->script_addrs) {
> -		ret = -ENOMEM;
> -		goto err_irq;
> -	}
> -
>  	/* initially no scripts available */
>  	saddr_arr = (s32 *)sdma->script_addrs;
>  	for (i = 0; i < sizeof(*sdma->script_addrs) / sizeof(s32); i++)
> @@ -2332,11 +2330,11 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	ret = sdma_init(sdma);
>  	if (ret)
> -		goto err_init;
> +		goto err_irq;
>
>  	ret = sdma_event_remap(sdma);
>  	if (ret)
> -		goto err_init;
> +		goto err_irq;
>
>  	if (sdma->drvdata->script_addrs)
>  		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
> @@ -2365,7 +2363,7 @@ static int sdma_probe(struct platform_device *pdev)
>  	ret = dma_async_device_register(&sdma->dma_device);
>  	if (ret) {
>  		dev_err(dev, "unable to register\n");
> -		goto err_init;
> +		goto err_irq;
>  	}
>
>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> @@ -2401,8 +2399,6 @@ static int sdma_probe(struct platform_device *pdev)
>
>  err_register:
>  	dma_async_device_unregister(&sdma->dma_device);
> -err_init:
> -	kfree(sdma->script_addrs);
>  err_irq:
>  	clk_unprepare(sdma->clk_ahb);
>  err_clk:
> @@ -2417,7 +2413,6 @@ static void sdma_remove(struct platform_device *pdev)
>
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
>  	dma_async_device_unregister(&sdma->dma_device);
> -	kfree(sdma->script_addrs);
>  	clk_unprepare(sdma->clk_ahb);
>  	clk_unprepare(sdma->clk_ipg);
>  	/* Kill the tasklet */
>
> --
> 2.47.2
>

Re: [PATCH 05/11] dmaengine: imx-sdma: make use of devm_clk_get_prepared()

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:13PM +0200, Marco Felsch wrote:
> Make use of the devm_clk_get_prepared() to cleanup the error handling
> during probe() and to automatically unprepare the clock during remove.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/dma/imx-sdma.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index b6e649fda71dbce12a2106c94887f90d0aaaf600..5a571d3f33158813e0c56484600a49b19a6a72e2 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2270,26 +2270,18 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (IS_ERR(sdma->regs))
>  		return PTR_ERR(sdma->regs);
>
> -	sdma->clk_ipg = devm_clk_get(dev, "ipg");
> +	sdma->clk_ipg = devm_clk_get_prepared(dev, "ipg");
>  	if (IS_ERR(sdma->clk_ipg))
>  		return PTR_ERR(sdma->clk_ipg);
>
> -	sdma->clk_ahb = devm_clk_get(dev, "ahb");
> +	sdma->clk_ahb = devm_clk_get_prepared(dev, "ahb");
>  	if (IS_ERR(sdma->clk_ahb))
>  		return PTR_ERR(sdma->clk_ahb);
>
> -	ret = clk_prepare(sdma->clk_ipg);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare(sdma->clk_ahb);
> -	if (ret)
> -		goto err_clk;
> -
>  	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
>  			       dev_name(dev), sdma);
>  	if (ret)
> -		goto err_irq;
> +		return ret;
>
>  	sdma->irq = irq;
>
> @@ -2330,11 +2322,11 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	ret = sdma_init(sdma);
>  	if (ret)
> -		goto err_irq;
> +		return ret;
>
>  	ret = sdma_event_remap(sdma);
>  	if (ret)
> -		goto err_irq;
> +		return ret;
>
>  	if (sdma->drvdata->script_addrs)
>  		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
> @@ -2363,7 +2355,7 @@ static int sdma_probe(struct platform_device *pdev)
>  	ret = dma_async_device_register(&sdma->dma_device);
>  	if (ret) {
>  		dev_err(dev, "unable to register\n");
> -		goto err_irq;
> +		return ret;
>  	}
>
>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> @@ -2399,10 +2391,7 @@ static int sdma_probe(struct platform_device *pdev)
>
>  err_register:
>  	dma_async_device_unregister(&sdma->dma_device);
> -err_irq:
> -	clk_unprepare(sdma->clk_ahb);
> -err_clk:
> -	clk_unprepare(sdma->clk_ipg);
> +
>  	return ret;
>  }
>
> @@ -2413,8 +2402,6 @@ static void sdma_remove(struct platform_device *pdev)
>
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
>  	dma_async_device_unregister(&sdma->dma_device);
> -	clk_unprepare(sdma->clk_ahb);
> -	clk_unprepare(sdma->clk_ipg);
>  	/* Kill the tasklet */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>
> --
> 2.47.2
>

Re: [PATCH 07/11] dmaengine: imx-sdma: make use of dev_err_probe()

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:15PM +0200, Marco Felsch wrote:
> Convert the probe function to dev_err_probe() which helps users to
> identify issues better.

I think it is not "convert"

Add dev_err_probe() at return path of probe to help user to ...

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index f6bb2f88a62781c0431336c365fa30c46f1401ad..e30dd46cf6522ee2aa4d3aca9868a01afbd29615 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2255,7 +2255,7 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to set DMA mask\n");
>
>  	sdma = devm_kzalloc(dev, sizeof(*sdma), GFP_KERNEL);
>  	if (!sdma)
> @@ -2272,24 +2272,24 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> -		return irq;
> +		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
>
>  	sdma->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(sdma->regs))
> -		return PTR_ERR(sdma->regs);
> +		return dev_err_probe(dev, PTR_ERR(sdma->regs), "ioremap failed\n");
>
>  	sdma->clk_ipg = devm_clk_get_prepared(dev, "ipg");
>  	if (IS_ERR(sdma->clk_ipg))
> -		return PTR_ERR(sdma->clk_ipg);
> +		return dev_err_probe(dev, PTR_ERR(sdma->clk_ipg), "IPG clk_get_prepared failed\n");
>
>  	sdma->clk_ahb = devm_clk_get_prepared(dev, "ahb");
>  	if (IS_ERR(sdma->clk_ahb))
> -		return PTR_ERR(sdma->clk_ahb);
> +		return dev_err_probe(dev, PTR_ERR(sdma->clk_ahb), "AHB clk_get_prepared failed\n");
>
>  	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
>  			       dev_name(dev), sdma);
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
>
>  	sdma->irq = irq;
>
> @@ -2330,11 +2330,11 @@ static int sdma_probe(struct platform_device *pdev)
>
>  	ret = sdma_init(sdma);
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(dev, ret, "sdma_init failed\n");
>
>  	ret = sdma_event_remap(sdma);
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(dev, ret, "sdma_event_remap failed\n");
>
>  	if (sdma->drvdata->script_addrs)
>  		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
> @@ -2361,18 +2361,14 @@ static int sdma_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, sdma);
>
>  	ret = dma_async_device_register(&sdma->dma_device);
> -	if (ret) {
> -		dev_err(dev, "unable to register\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to register\n");
>
>  	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
>
>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> -	if (ret) {
> -		dev_err(dev, "failed to register controller\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register controller\n");
>
>  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
>  	ret = of_address_to_resource(spba_bus, 0, &spba_res);
>
> --
> 2.47.2
>

Re: [PATCH 08/11] dmaengine: imx-sdma: fix missing of_dma_controller_free()

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:16PM +0200, Marco Felsch wrote:
> Add the missing of_dma_controller_free() to free the resources allocated
> via of_dma_controller_register(). The missing free was introduced long
> time ago  by commit 23e118113782 ("dma: imx-sdma: use
> module_platform_driver for SDMA driver") while adding a proper .remove()
> implementation.
>
> Fixes: 23e118113782 ("dma: imx-sdma: use module_platform_driver for SDMA driver")

Look it is hard to back port to old kernel.  Can move it to before cleanup?

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index e30dd46cf6522ee2aa4d3aca9868a01afbd29615..6c6d38b202dd2deffc36b1bd27bc7c60de3d7403 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2232,6 +2232,13 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
>  				     ofdma->of_node);
>  }
>
> +static void sdma_dma_of_dma_controller_unregister_action(void *data)
> +{
> +	struct sdma_engine *sdma = data;
> +
> +	of_dma_controller_free(sdma->dev->of_node);
> +}
> +
>  static void sdma_dma_device_unregister_action(void *data)
>  {
>  	struct sdma_engine *sdma = data;
> @@ -2370,6 +2377,8 @@ static int sdma_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to register controller\n");
>
> +	devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
> +
>  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
>  	ret = of_address_to_resource(spba_bus, 0, &spba_res);
>  	if (!ret) {
>
> --
> 2.47.2
>

Re: [PATCH 10/11] dmaengine: imx-sdma: drop remove callback

[Frank Li]

On Wed, Sep 03, 2025 at 03:06:18PM +0200, Marco Felsch wrote:
> The whole driver was converted to the devm APIs except for this last
> for-loop. This loop is buggy due to three reasons:
>  1) It removes the channels without removing the users first. This can
>     lead to very bad situations.
>  2) The loop starts at 0 and which is channel0 which is a special
>     control channel not registered via vchan_init(). Therefore the
>     remove() always Oops because of NULL pointer exception.
>  3) sdma_free_chan_resources() disable the clks unconditional without
>     checking if the clks are enabled. This is done for all
>     MAX_DMA_CHANNELS which hang the system if there is at least one unused
>     channel.
>
> Since the dmaengine core supports devlinks we already addressed the
> first issue.
>
> The devlink support also addresses the third issue because during the
> consumer teardown phase each requested channel is dropped accordingly so
> the dmaengine driver doesn't need to this.
>
> The second issue is fixed by not doing anything on channel0.
>
> To sum-up, all issues are fixed by dropping the .remove() callback and
> let the frameworks do their job.

I not realize devlink have this beanfit also. devlink may need more review,
which change some default behavior. I suggest put dmaengin dmalink at this
patch to new serial.

It is easily omited by other dmaengine owner if mixed it to cleanup serial.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 6c6d38b202dd2deffc36b1bd27bc7c60de3d7403..c31785977351163d6fddf4d8b2f90dfebb508400 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2405,25 +2405,11 @@ static int sdma_probe(struct platform_device *pdev)
>  	return 0;
>  }
>
> -static void sdma_remove(struct platform_device *pdev)
> -{
> -	struct sdma_engine *sdma = platform_get_drvdata(pdev);
> -	int i;
> -
> -	/* Kill the tasklet */
> -	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> -		struct sdma_channel *sdmac = &sdma->channel[i];
> -
> -		sdma_free_chan_resources(&sdmac->vc.chan);
> -	}
> -}
> -
>  static struct platform_driver sdma_driver = {
>  	.driver		= {
>  		.name	= "imx-sdma",
>  		.of_match_table = sdma_dt_ids,
>  	},
> -	.remove		= sdma_remove,
>  	.probe		= sdma_probe,
>  };
>
>
> --
> 2.47.2
>

Re: [PATCH 11/11] dmaengine: imx-sdma: fix spba-bus handling for i.MX8M

[kernel test robot]

Hi Marco,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 038d61fd642278bab63ee8ef722c50d10ab01e8f]

url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/dmaengine-imx-sdma-drop-legacy-device_node-np-check/20250903-212133
base:   038d61fd642278bab63ee8ef722c50d10ab01e8f
patch link:    https://lore.kernel.org/r/20250903-v6-16-topic-sdma-v1-11-ac7bab629e8b%40pengutronix.de
patch subject: [PATCH 11/11] dmaengine: imx-sdma: fix spba-bus handling for i.MX8M
config: arm-randconfig-002-20250904 (https://download.01.org/0day-ci/archive/20250904/202509041238.qCKW7MeD-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250904/202509041238.qCKW7MeD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509041238.qCKW7MeD-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/dma/imx-sdma.c:477 struct member 'spba_start_addr' not described in 'sdma_channel'
>> Warning: drivers/dma/imx-sdma.c:477 struct member 'spba_end_addr' not described in 'sdma_channel'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

Hi Frank,

On 25-09-03, Frank Li wrote:
> On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > Add support to create device_links between dmaengine suppliers and the
> > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > to the driver core.
> >
> > Moving this to the core allows the dmaengine drivers to simplify the
> > .remove() hooks and also to ensure that no dmaengine driver is ever
> > removed before the consumer is removed.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> 
> Thank you work for devlink between dmaengine and devices. I have similar
> idea.
> 
> This patch should be first patch.

I can shuffle it of course!

> The below what planned commit message in my local tree.

Okay, so you focused on runtime PM handling. Not quite sure if I can
test this feature with the SDMA engine. I also have limited time for
this feature.

Is it okay for you and the DMA maintainers to add the runtime PM feature
as separate patch (provided by NXP/Frank)?

> Implementing runtime PM for DMA channels is challenging. If a channel
> resumes at allocation and suspends at free, the DMA engine often remains on
> because most drivers request a channel at probe.
> 
> Tracking the number of pending DMA descriptors is also problematic, as some
> consumers append new descriptors in atomic contexts, such as IRQ handlers,
> where runtime resume cannot be called.
> 
> Using a device link simplifies this issue. If a consumer requires data
> transfer, it must be in a runtime-resumed state, ensuring that the DMA
> channel is also active by device link. This allows safe operations, like
> appending new descriptors. Conversely, when the consumer no longer requires
> data transfer, both it and the supplier (DMA channel) can enter a suspended
> state if no other consumer is using it.
> 
> Introduce the `create_link` flag to enable this feature.
>
> also suggest add create_link flag to enable this feature in case some
> side impact to other dma-engine. After some time test, we can enable it
> default.

What regressions do you have in mind? I wouldn't hide the feature behind
a flag because this may slow done the convert process, because no one is
interessted in, or has no time for testing, ...

> >  drivers/dma/dmaengine.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > --- a/drivers/dma/dmaengine.c
> > +++ b/drivers/dma/dmaengine.c
> > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >  	struct dma_device *d, *_d;
> >  	struct dma_chan *chan = NULL;
> > +	struct device_link *dl;
> >
> >  	if (is_of_node(fwnode))
> >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> >  	/* No functional issue if it fails, users are supposed to test before use */
> >  #endif
> >
> > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> 
> chan->device->dev is dmaengine devices. But some dmaengine's each channel
> have device, consumer should link to chan's device, not dmaengine device
> because some dmaengine support per channel clock\power management.

I get your point. Can you give me some pointers please? To me it seems
like the dma_chan_dev is only used for sysfs purpose according the
dmaengine.h.

> chan's device's parent devices is dmaengine devices. it should also work
> for sdma case

I see, this must be tested of course.

>         if (chan->device->create_devlink) {
>                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;

According device_link.rst: using DL_FLAG_STATELESS and
DL_FLAG_AUTOREMOVE_CONSUMER is invalid.

>                 if (pm_runtime_active(dev))
>                         flags |= DL_FLAG_RPM_ACTIVE;

This is of course interessting, thanks for the hint.

> When create device link (apply channel), consume may active.

I have read it as: "resue the supplier and ensure that the supplier
follows the consumer runtime state".

>                 dl = device_link_add(chan->slave, &chan->dev->device, flags);

Huh.. you used the dmaengine device too?

Regards,
  Marco


>         }
> 
> Need update kernel doc
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index bb146c5ac3e4c..ffb3a8f0070ba 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -323,7 +323,8 @@ struct dma_router {
>   * @cookie: last cookie value returned to client
>   * @completed_cookie: last completed cookie for this channel
>   * @chan_id: channel ID for sysfs
> - * @dev: class device for sysfs
> + * @dev: class device for sysfs, also use for pre channel runtime pm and
> + *       use custom/different dma-mapping
> 
> Frank
> 
> 
> > +	if (!dl) {
> > +		dev_err(dev, "failed to create device link to %s\n",
> > +			dev_name(chan->device->dev));
> > +		return ERR_PTR(-EINVAL);
> > +	}
> >  	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
> >  	if (!chan->name)
> >  		return chan;
> >
> > --
> > 2.47.2
> >
> 

Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li]

On Tue, Sep 09, 2025 at 02:03:09PM +0200, Marco Felsch wrote:
> Hi Frank,
>
> On 25-09-03, Frank Li wrote:
> > On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > > Add support to create device_links between dmaengine suppliers and the
> > > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > > to the driver core.
> > >
> > > Moving this to the core allows the dmaengine drivers to simplify the
> > > .remove() hooks and also to ensure that no dmaengine driver is ever
> > > removed before the consumer is removed.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> >
> > Thank you work for devlink between dmaengine and devices. I have similar
> > idea.
> >
> > This patch should be first patch.
>
> I can shuffle it of course!
>
> > The below what planned commit message in my local tree.
>
> Okay, so you focused on runtime PM handling. Not quite sure if I can
> test this feature with the SDMA engine. I also have limited time for
> this feature.
>
> Is it okay for you and the DMA maintainers to add the runtime PM feature
> as separate patch (provided by NXP/Frank)?

we can support runtime pm later.

>
> > Implementing runtime PM for DMA channels is challenging. If a channel
> > resumes at allocation and suspends at free, the DMA engine often remains on
> > because most drivers request a channel at probe.
> >
> > Tracking the number of pending DMA descriptors is also problematic, as some
> > consumers append new descriptors in atomic contexts, such as IRQ handlers,
> > where runtime resume cannot be called.
> >
> > Using a device link simplifies this issue. If a consumer requires data
> > transfer, it must be in a runtime-resumed state, ensuring that the DMA
> > channel is also active by device link. This allows safe operations, like
> > appending new descriptors. Conversely, when the consumer no longer requires
> > data transfer, both it and the supplier (DMA channel) can enter a suspended
> > state if no other consumer is using it.
> >
> > Introduce the `create_link` flag to enable this feature.
> >
> > also suggest add create_link flag to enable this feature in case some
> > side impact to other dma-engine. After some time test, we can enable it
> > default.
>
> What regressions do you have in mind? I wouldn't hide the feature behind
> a flag because this may slow done the convert process, because no one is
> interessted in, or has no time for testing, ...

Unlike other devices, like phys, regulator, mailbox..., which auto create
devlink at probe. I am not clear why dma skip this one. So I think there
should be some reason behind. Maybe other people, rob or Vinod Koul know
the reason.

static const struct supplier_bindings of_supplier_bindings[] = {
        ...
	{ .parse_prop = parse_dmas, .optional = true, },

If remove "optional = true", devlink will auto create. I am not sure why
set true here.

>
> > >  drivers/dma/dmaengine.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > --- a/drivers/dma/dmaengine.c
> > > +++ b/drivers/dma/dmaengine.c
> > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >  	struct dma_device *d, *_d;
> > >  	struct dma_chan *chan = NULL;
> > > +	struct device_link *dl;
> > >
> > >  	if (is_of_node(fwnode))
> > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > >  	/* No functional issue if it fails, users are supposed to test before use */
> > >  #endif
> > >
> > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> >
> > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > have device, consumer should link to chan's device, not dmaengine device
> > because some dmaengine support per channel clock\power management.
>
> I get your point. Can you give me some pointers please? To me it seems
> like the dma_chan_dev is only used for sysfs purpose according the
> dmaengine.h.

Not really, there are other dma engineer already reuse it for other purpose.
So It needs update kernel doc for dma_chan_dev.

>
> > chan's device's parent devices is dmaengine devices. it should also work
> > for sdma case
>
> I see, this must be tested of course.
> > >         if (chan->device->create_devlink) {
> >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
>
> According device_link.rst: using DL_FLAG_STATELESS and
> DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
>
> >                 if (pm_runtime_active(dev))
> >                         flags |= DL_FLAG_RPM_ACTIVE;
>
> This is of course interessting, thanks for the hint.
>
> > When create device link (apply channel), consume may active.
>
> I have read it as: "resue the supplier and ensure that the supplier
> follows the consumer runtime state".
>
> >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
>
> Huh.. you used the dmaengine device too?

/**
 * struct dma_chan_dev - relate sysfs device node to backing channel device
 * @chan: driver channel device
 * @device: sysfs device
 * @dev_id: parent dma_device dev_id
 * @chan_dma_dev: The channel is using custom/different dma-mapping
 * compared to the parent dma_device
 */
struct dma_chan_dev {
	struct dma_chan *chan;
	struct device device;
	int dev_id;
	bool chan_dma_dev;
};

struct dma_chan {
	struct dma_device *device; /// this one should be dmaengine
	struct dma_chan_dev *dev; /// this one is pre-chan device.
}

Frank
>
> Regards,
>   Marco
>
>
> >         }
> >
> > Need update kernel doc
> >
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index bb146c5ac3e4c..ffb3a8f0070ba 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -323,7 +323,8 @@ struct dma_router {
> >   * @cookie: last cookie value returned to client
> >   * @completed_cookie: last completed cookie for this channel
> >   * @chan_id: channel ID for sysfs
> > - * @dev: class device for sysfs
> > + * @dev: class device for sysfs, also use for pre channel runtime pm and
> > + *       use custom/different dma-mapping
> >
> > Frank
> >
> >
> > > +	if (!dl) {
> > > +		dev_err(dev, "failed to create device link to %s\n",
> > > +			dev_name(chan->device->dev));
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > >  	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
> > >  	if (!chan->name)
> > >  		return chan;
> > >
> > > --
> > > 2.47.2
> > >
> >

Re: [PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

On 25-09-09, Frank Li wrote:
> On Tue, Sep 09, 2025 at 02:03:09PM +0200, Marco Felsch wrote:
> > Hi Frank,
> >
> > On 25-09-03, Frank Li wrote:
> > > On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > > > Add support to create device_links between dmaengine suppliers and the
> > > > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > > > to the driver core.
> > > >
> > > > Moving this to the core allows the dmaengine drivers to simplify the
> > > > .remove() hooks and also to ensure that no dmaengine driver is ever
> > > > removed before the consumer is removed.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > >
> > > Thank you work for devlink between dmaengine and devices. I have similar
> > > idea.
> > >
> > > This patch should be first patch.
> >
> > I can shuffle it of course!
> >
> > > The below what planned commit message in my local tree.
> >
> > Okay, so you focused on runtime PM handling. Not quite sure if I can
> > test this feature with the SDMA engine. I also have limited time for
> > this feature.
> >
> > Is it okay for you and the DMA maintainers to add the runtime PM feature
> > as separate patch (provided by NXP/Frank)?
> 
> we can support runtime pm later.
> 
> >
> > > Implementing runtime PM for DMA channels is challenging. If a channel
> > > resumes at allocation and suspends at free, the DMA engine often remains on
> > > because most drivers request a channel at probe.
> > >
> > > Tracking the number of pending DMA descriptors is also problematic, as some
> > > consumers append new descriptors in atomic contexts, such as IRQ handlers,
> > > where runtime resume cannot be called.
> > >
> > > Using a device link simplifies this issue. If a consumer requires data
> > > transfer, it must be in a runtime-resumed state, ensuring that the DMA
> > > channel is also active by device link. This allows safe operations, like
> > > appending new descriptors. Conversely, when the consumer no longer requires
> > > data transfer, both it and the supplier (DMA channel) can enter a suspended
> > > state if no other consumer is using it.
> > >
> > > Introduce the `create_link` flag to enable this feature.
> > >
> > > also suggest add create_link flag to enable this feature in case some
> > > side impact to other dma-engine. After some time test, we can enable it
> > > default.
> >
> > What regressions do you have in mind? I wouldn't hide the feature behind
> > a flag because this may slow done the convert process, because no one is
> > interessted in, or has no time for testing, ...
> 
> Unlike other devices, like phys, regulator, mailbox..., which auto create
> devlink at probe. I am not clear why dma skip this one. So I think there
> should be some reason behind. Maybe other people, rob or Vinod Koul know
> the reason.
> 
> static const struct supplier_bindings of_supplier_bindings[] = {
>         ...
> 	{ .parse_prop = parse_dmas, .optional = true, },
> 
> If remove "optional = true", devlink will auto create. I am not sure why
> set true here.

I've seen this too. Could be because DMA controllers + users aren't OF
related and therefore should be handled within the framework itself.

> > > >  drivers/dma/dmaengine.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > --- a/drivers/dma/dmaengine.c
> > > > +++ b/drivers/dma/dmaengine.c
> > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >  	struct dma_device *d, *_d;
> > > >  	struct dma_chan *chan = NULL;
> > > > +	struct device_link *dl;
> > > >
> > > >  	if (is_of_node(fwnode))
> > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > >  #endif
> > > >
> > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > >
> > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > have device, consumer should link to chan's device, not dmaengine device
> > > because some dmaengine support per channel clock\power management.
> >
> > I get your point. Can you give me some pointers please? To me it seems
> > like the dma_chan_dev is only used for sysfs purpose according the
> > dmaengine.h.
> 
> Not really, there are other dma engineer already reuse it for other purpose.
> So It needs update kernel doc for dma_chan_dev.

Okay.

> > > chan's device's parent devices is dmaengine devices. it should also work
> > > for sdma case
> >
> > I see, this must be tested of course.
> > > >         if (chan->device->create_devlink) {
> > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> >
> > According device_link.rst: using DL_FLAG_STATELESS and
> > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> >
> > >                 if (pm_runtime_active(dev))
> > >                         flags |= DL_FLAG_RPM_ACTIVE;
> >
> > This is of course interessting, thanks for the hint.
> >
> > > When create device link (apply channel), consume may active.
> >
> > I have read it as: "resue the supplier and ensure that the supplier
> > follows the consumer runtime state".
> >
> > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> >
> > Huh.. you used the dmaengine device too?
> 
> /**
>  * struct dma_chan_dev - relate sysfs device node to backing channel device
>  * @chan: driver channel device
>  * @device: sysfs device
>  * @dev_id: parent dma_device dev_id
>  * @chan_dma_dev: The channel is using custom/different dma-mapping
>  * compared to the parent dma_device
>  */
> struct dma_chan_dev {
> 	struct dma_chan *chan;
> 	struct device device;
> 	int dev_id;
> 	bool chan_dma_dev;
> };
> 
> struct dma_chan {
> 	struct dma_device *device; /// this one should be dmaengine
> 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> }

Argh.. mixed it within my head while writing the mail :/

Regards,
  Marco

Re: [PATCH 08/11] dmaengine: imx-sdma: fix missing of_dma_controller_free()

[Marco Felsch]

On 25-09-03, Frank Li wrote:
> On Wed, Sep 03, 2025 at 03:06:16PM +0200, Marco Felsch wrote:
> > Add the missing of_dma_controller_free() to free the resources allocated
> > via of_dma_controller_register(). The missing free was introduced long
> > time ago  by commit 23e118113782 ("dma: imx-sdma: use
> > module_platform_driver for SDMA driver") while adding a proper .remove()
> > implementation.
> >
> > Fixes: 23e118113782 ("dma: imx-sdma: use module_platform_driver for SDMA driver")
> 
> Look it is hard to back port to old kernel.  Can move it to before cleanup?

I know that fixing commits should come first but this commit dates back
to v3.18-rc1, therefore I thought that backporting this commit would
cause more troubles than it's worth it.

Anyway, after checking the current LTS and stable kernels I think that
the commit could be backported without troubles because the APIs used
do exist on all these kernels.

Regards,
  Marco

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index e30dd46cf6522ee2aa4d3aca9868a01afbd29615..6c6d38b202dd2deffc36b1bd27bc7c60de3d7403 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -2232,6 +2232,13 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
> >  				     ofdma->of_node);
> >  }
> >
> > +static void sdma_dma_of_dma_controller_unregister_action(void *data)
> > +{
> > +	struct sdma_engine *sdma = data;
> > +
> > +	of_dma_controller_free(sdma->dev->of_node);
> > +}
> > +
> >  static void sdma_dma_device_unregister_action(void *data)
> >  {
> >  	struct sdma_engine *sdma = data;
> > @@ -2370,6 +2377,8 @@ static int sdma_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "failed to register controller\n");
> >
> > +	devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
> > +
> >  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> >  	ret = of_address_to_resource(spba_bus, 0, &spba_res);
> >  	if (!ret) {
> >
> > --
> > 2.47.2
> >
> 

Re: [PATCH 06/11] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Marco Felsch]

On 25-09-03, Frank Li wrote:
> On Wed, Sep 03, 2025 at 03:06:14PM +0200, Marco Felsch wrote:
> > Make use of the devm_add_action_or_reset() to register a custom devm_
> > release hook. This is required since we want to turn of the IRQs before
> 
> turn off?
> 
> > doing the dma_async_device_unregister().
> >
> > This removes the last goto error handling within the probe function and
> 
> Remove the last ..

I rephrased the commit message.

> > further trims the remove() function. Instead of freeing the irq, we can
> > disable it and let the devm-irq do the job to free the irq, since the
> > only purpose was to have the irqs disabled before calling
> > dma_async_device_unregister().
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 5a571d3f33158813e0c56484600a49b19a6a72e2..f6bb2f88a62781c0431336c365fa30c46f1401ad 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -2232,6 +2232,14 @@ static struct dma_chan *sdma_xlate(struct of_phandle_args *dma_spec,
> >  				     ofdma->of_node);
> >  }
> >
> > +static void sdma_dma_device_unregister_action(void *data)
> > +{
> > +	struct sdma_engine *sdma = data;
> > +
> > +	disable_irq(sdma->irq);
> > +	dma_async_device_unregister(&sdma->dma_device);
> > +}
> > +
> >  static int sdma_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -2358,10 +2366,12 @@ static int sdma_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >
> > +	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> > +
> 
> need check return value.

Sure, thanks.

Regards,
  Marco

> 
> Frank
> 
> >  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> >  	if (ret) {
> >  		dev_err(dev, "failed to register controller\n");
> > -		goto err_register;
> > +		return ret;
> >  	}
> >
> >  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> > @@ -2388,11 +2398,6 @@ static int sdma_probe(struct platform_device *pdev)
> >  	}
> >
> >  	return 0;
> > -
> > -err_register:
> > -	dma_async_device_unregister(&sdma->dma_device);
> > -
> > -	return ret;
> >  }
> >
> >  static void sdma_remove(struct platform_device *pdev)
> > @@ -2400,8 +2405,6 @@ static void sdma_remove(struct platform_device *pdev)
> >  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
> >  	int i;
> >
> > -	devm_free_irq(&pdev->dev, sdma->irq, sdma);
> > -	dma_async_device_unregister(&sdma->dma_device);
> >  	/* Kill the tasklet */
> >  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> >  		struct sdma_channel *sdmac = &sdma->channel[i];
> >
> > --
> > 2.47.2
> >
> 

Re: [PATCH 07/11] dmaengine: imx-sdma: make use of dev_err_probe()

[Marco Felsch]

On 25-09-03, Frank Li wrote:
> On Wed, Sep 03, 2025 at 03:06:15PM +0200, Marco Felsch wrote:
> > Convert the probe function to dev_err_probe() which helps users to
> > identify issues better.
> 
> I think it is not "convert"
> 
> Add dev_err_probe() at return path of probe to help user to ...

Done, thanks.


> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 28 ++++++++++++----------------
> >  1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index f6bb2f88a62781c0431336c365fa30c46f1401ad..e30dd46cf6522ee2aa4d3aca9868a01afbd29615 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -2255,7 +2255,7 @@ static int sdma_probe(struct platform_device *pdev)
> >
> >  	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> >  	if (ret)
> > -		return ret;
> > +		return dev_err_probe(dev, ret, "Failed to set DMA mask\n");
> >
> >  	sdma = devm_kzalloc(dev, sizeof(*sdma), GFP_KERNEL);
> >  	if (!sdma)
> > @@ -2272,24 +2272,24 @@ static int sdma_probe(struct platform_device *pdev)
> >
> >  	irq = platform_get_irq(pdev, 0);
> >  	if (irq < 0)
> > -		return irq;
> > +		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> >
> >  	sdma->regs = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(sdma->regs))
> > -		return PTR_ERR(sdma->regs);
> > +		return dev_err_probe(dev, PTR_ERR(sdma->regs), "ioremap failed\n");
> >
> >  	sdma->clk_ipg = devm_clk_get_prepared(dev, "ipg");
> >  	if (IS_ERR(sdma->clk_ipg))
> > -		return PTR_ERR(sdma->clk_ipg);
> > +		return dev_err_probe(dev, PTR_ERR(sdma->clk_ipg), "IPG clk_get_prepared failed\n");
> >
> >  	sdma->clk_ahb = devm_clk_get_prepared(dev, "ahb");
> >  	if (IS_ERR(sdma->clk_ahb))
> > -		return PTR_ERR(sdma->clk_ahb);
> > +		return dev_err_probe(dev, PTR_ERR(sdma->clk_ahb), "AHB clk_get_prepared failed\n");
> >
> >  	ret = devm_request_irq(dev, irq, sdma_int_handler, 0,
> >  			       dev_name(dev), sdma);
> >  	if (ret)
> > -		return ret;
> > +		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
> >
> >  	sdma->irq = irq;
> >
> > @@ -2330,11 +2330,11 @@ static int sdma_probe(struct platform_device *pdev)
> >
> >  	ret = sdma_init(sdma);
> >  	if (ret)
> > -		return ret;
> > +		return dev_err_probe(dev, ret, "sdma_init failed\n");
> >
> >  	ret = sdma_event_remap(sdma);
> >  	if (ret)
> > -		return ret;
> > +		return dev_err_probe(dev, ret, "sdma_event_remap failed\n");
> >
> >  	if (sdma->drvdata->script_addrs)
> >  		sdma_add_scripts(sdma, sdma->drvdata->script_addrs);
> > @@ -2361,18 +2361,14 @@ static int sdma_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, sdma);
> >
> >  	ret = dma_async_device_register(&sdma->dma_device);
> > -	if (ret) {
> > -		dev_err(dev, "unable to register\n");
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "unable to register\n");
> >
> >  	devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> >
> >  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> > -	if (ret) {
> > -		dev_err(dev, "failed to register controller\n");
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register controller\n");
> >
> >  	spba_bus = of_find_compatible_node(NULL, NULL, "fsl,spba-bus");
> >  	ret = of_address_to_resource(spba_bus, 0, &spba_res);
> >
> > --
> > 2.47.2
> >
> 

Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li]

On Wed, Sep 10, 2025 at 11:34:28AM +0200, Marco Felsch wrote:
> On 25-09-09, Frank Li wrote:
> > On Tue, Sep 09, 2025 at 02:03:09PM +0200, Marco Felsch wrote:
> > > Hi Frank,
> > >
> > > On 25-09-03, Frank Li wrote:
> > > > On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > > > > Add support to create device_links between dmaengine suppliers and the
> > > > > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > > > > to the driver core.
> > > > >
> > > > > Moving this to the core allows the dmaengine drivers to simplify the
> > > > > .remove() hooks and also to ensure that no dmaengine driver is ever
> > > > > removed before the consumer is removed.
> > > > >
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > ---
> > > >
> > > > Thank you work for devlink between dmaengine and devices. I have similar
> > > > idea.
> > > >
> > > > This patch should be first patch.
> > >
> > > I can shuffle it of course!
> > >
> > > > The below what planned commit message in my local tree.
> > >
> > > Okay, so you focused on runtime PM handling. Not quite sure if I can
> > > test this feature with the SDMA engine. I also have limited time for
> > > this feature.
> > >
> > > Is it okay for you and the DMA maintainers to add the runtime PM feature
> > > as separate patch (provided by NXP/Frank)?
> >
> > we can support runtime pm later.
> >
> > >
> > > > Implementing runtime PM for DMA channels is challenging. If a channel
> > > > resumes at allocation and suspends at free, the DMA engine often remains on
> > > > because most drivers request a channel at probe.
> > > >
> > > > Tracking the number of pending DMA descriptors is also problematic, as some
> > > > consumers append new descriptors in atomic contexts, such as IRQ handlers,
> > > > where runtime resume cannot be called.
> > > >
> > > > Using a device link simplifies this issue. If a consumer requires data
> > > > transfer, it must be in a runtime-resumed state, ensuring that the DMA
> > > > channel is also active by device link. This allows safe operations, like
> > > > appending new descriptors. Conversely, when the consumer no longer requires
> > > > data transfer, both it and the supplier (DMA channel) can enter a suspended
> > > > state if no other consumer is using it.
> > > >
> > > > Introduce the `create_link` flag to enable this feature.
> > > >
> > > > also suggest add create_link flag to enable this feature in case some
> > > > side impact to other dma-engine. After some time test, we can enable it
> > > > default.
> > >
> > > What regressions do you have in mind? I wouldn't hide the feature behind
> > > a flag because this may slow done the convert process, because no one is
> > > interessted in, or has no time for testing, ...
> >
> > Unlike other devices, like phys, regulator, mailbox..., which auto create
> > devlink at probe. I am not clear why dma skip this one. So I think there
> > should be some reason behind. Maybe other people, rob or Vinod Koul know
> > the reason.
> >
> > static const struct supplier_bindings of_supplier_bindings[] = {
> >         ...
> > 	{ .parse_prop = parse_dmas, .optional = true, },
> >
> > If remove "optional = true", devlink will auto create. I am not sure why
> > set true here.
>
> I've seen this too. Could be because DMA controllers + users aren't OF
> related and therefore should be handled within the framework itself.

Not sure, may be some historical reason. I have not enough confidence just
because I don't know why optional = true here.

So I think you can post this patch seperated with good cover letter, may
someone jump into discussion.

Frank
>
> > > > >  drivers/dma/dmaengine.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > > --- a/drivers/dma/dmaengine.c
> > > > > +++ b/drivers/dma/dmaengine.c
> > > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > >  	struct dma_device *d, *_d;
> > > > >  	struct dma_chan *chan = NULL;
> > > > > +	struct device_link *dl;
> > > > >
> > > > >  	if (is_of_node(fwnode))
> > > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > > >  #endif
> > > > >
> > > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > > >
> > > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > > have device, consumer should link to chan's device, not dmaengine device
> > > > because some dmaengine support per channel clock\power management.
> > >
> > > I get your point. Can you give me some pointers please? To me it seems
> > > like the dma_chan_dev is only used for sysfs purpose according the
> > > dmaengine.h.
> >
> > Not really, there are other dma engineer already reuse it for other purpose.
> > So It needs update kernel doc for dma_chan_dev.
>
> Okay.
>
> > > > chan's device's parent devices is dmaengine devices. it should also work
> > > > for sdma case
> > >
> > > I see, this must be tested of course.
> > > > >         if (chan->device->create_devlink) {
> > > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> > >
> > > According device_link.rst: using DL_FLAG_STATELESS and
> > > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> > >
> > > >                 if (pm_runtime_active(dev))
> > > >                         flags |= DL_FLAG_RPM_ACTIVE;
> > >
> > > This is of course interessting, thanks for the hint.
> > >
> > > > When create device link (apply channel), consume may active.
> > >
> > > I have read it as: "resue the supplier and ensure that the supplier
> > > follows the consumer runtime state".
> > >
> > > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> > >
> > > Huh.. you used the dmaengine device too?
> >
> > /**
> >  * struct dma_chan_dev - relate sysfs device node to backing channel device
> >  * @chan: driver channel device
> >  * @device: sysfs device
> >  * @dev_id: parent dma_device dev_id
> >  * @chan_dma_dev: The channel is using custom/different dma-mapping
> >  * compared to the parent dma_device
> >  */
> > struct dma_chan_dev {
> > 	struct dma_chan *chan;
> > 	struct device device;
> > 	int dev_id;
> > 	bool chan_dma_dev;
> > };
> >
> > struct dma_chan {
> > 	struct dma_device *device; /// this one should be dmaengine
> > 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> > }
>
> Argh.. mixed it within my head while writing the mail :/
>
> Regards,
>   Marco

Re: [PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

On 25-09-09, Frank Li wrote:

...

> > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > --- a/drivers/dma/dmaengine.c
> > > > +++ b/drivers/dma/dmaengine.c
> > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > >  	struct dma_device *d, *_d;
> > > >  	struct dma_chan *chan = NULL;
> > > > +	struct device_link *dl;
> > > >
> > > >  	if (is_of_node(fwnode))
> > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > >  #endif
> > > >
> > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > >
> > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > have device, consumer should link to chan's device, not dmaengine device
> > > because some dmaengine support per channel clock\power management.
> >
> > I get your point. Can you give me some pointers please? To me it seems
> > like the dma_chan_dev is only used for sysfs purpose according the
> > dmaengine.h.
> 
> Not really, there are other dma engineer already reuse it for other purpose.
> So It needs update kernel doc for dma_chan_dev.

Can you please provide me some pointers? I checked the kernel code base
for the struct::dma_chan_dev. I didn't found any references within the
dmaengine drivers. The only usage I found was for the sysfs purpose.

> > > chan's device's parent devices is dmaengine devices. it should also work
> > > for sdma case
> >
> > I see, this must be tested of course.
> > > >         if (chan->device->create_devlink) {
> > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> >
> > According device_link.rst: using DL_FLAG_STATELESS and
> > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> >
> > >                 if (pm_runtime_active(dev))
> > >                         flags |= DL_FLAG_RPM_ACTIVE;
> >
> > This is of course interessting, thanks for the hint.
> >
> > > When create device link (apply channel), consume may active.
> >
> > I have read it as: "resue the supplier and ensure that the supplier
> > follows the consumer runtime state".
> >
> > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> >
> > Huh.. you used the dmaengine device too?
> 
> /**
>  * struct dma_chan_dev - relate sysfs device node to backing channel device
>  * @chan: driver channel device
>  * @device: sysfs device
>  * @dev_id: parent dma_device dev_id
>  * @chan_dma_dev: The channel is using custom/different dma-mapping
>  * compared to the parent dma_device
>  */
> struct dma_chan_dev {
> 	struct dma_chan *chan;
> 	struct device device;
> 	int dev_id;
> 	bool chan_dma_dev;
> };
> 
> struct dma_chan {
> 	struct dma_device *device; /// this one should be dmaengine
> 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> }

I've tested your approach but it turns out that teh dma_chan_dev has no
driver. Of course we could use the DL_FLAG_STATELESS flag but this is
described as:

| When driver presence on the supplier is irrelevant and only correct
| suspend/resume and shutdown ordering is needed, the device link may
| simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
| enforcing driver presence on the supplier is optional.

I want to enforce the driver presence, therefore I used the manged flags
which excludes the DL_FLAG_STATELESS, if I get it right.

Please see the below the debug output:

** use the dmaengine device as supplier **

device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1


** use the dma channel device as supplier **

device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
------------[ cut here ]------------
WARNING: CPU: 0 PID: 51 at /drivers/base/core.c:1387 device_links_driver_bound+0x170/0x3a0
...
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

As said, I get your point regarding the usage of the dma-channel device
but I didn't found any reference to a driver which used the dma-channel
device. Also since I want to have the supply driver to enforced by the
devlink I don't want to use the DL_FLAG_STATELESS flag.

Regarding your point, that some DMA controllers may have seperate clocks
for each channel: I think this can be handled by the dmaengine driver,
e.g. via the device_alloc_chan_resources() hook.

@all
I'm pleased about any input :)

Regards,
  Marco

Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li]

On Wed, Sep 10, 2025 at 09:35:45PM +0200, Marco Felsch wrote:
> On 25-09-09, Frank Li wrote:
>
> ...
>
> > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > > --- a/drivers/dma/dmaengine.c
> > > > > +++ b/drivers/dma/dmaengine.c
> > > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > >  	struct dma_device *d, *_d;
> > > > >  	struct dma_chan *chan = NULL;
> > > > > +	struct device_link *dl;
> > > > >
> > > > >  	if (is_of_node(fwnode))
> > > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > > >  #endif
> > > > >
> > > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > > >
> > > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > > have device, consumer should link to chan's device, not dmaengine device
> > > > because some dmaengine support per channel clock\power management.
> > >
> > > I get your point. Can you give me some pointers please? To me it seems
> > > like the dma_chan_dev is only used for sysfs purpose according the
> > > dmaengine.h.
> >
> > Not really, there are other dma engineer already reuse it for other purpose.
> > So It needs update kernel doc for dma_chan_dev.
>
> Can you please provide me some pointers? I checked the kernel code base
> for the struct::dma_chan_dev. I didn't found any references within the
> dmaengine drivers. The only usage I found was for the sysfs purpose.

static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
{
	struct device *chan_dev = &chan->dev->device;
	...
}

>
> > > > chan's device's parent devices is dmaengine devices. it should also work
> > > > for sdma case
> > >
> > > I see, this must be tested of course.
> > > > >         if (chan->device->create_devlink) {
> > > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> > >
> > > According device_link.rst: using DL_FLAG_STATELESS and
> > > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> > >
> > > >                 if (pm_runtime_active(dev))
> > > >                         flags |= DL_FLAG_RPM_ACTIVE;
> > >
> > > This is of course interessting, thanks for the hint.
> > >
> > > > When create device link (apply channel), consume may active.
> > >
> > > I have read it as: "resue the supplier and ensure that the supplier
> > > follows the consumer runtime state".
> > >
> > > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> > >
> > > Huh.. you used the dmaengine device too?
> >
> > /**
> >  * struct dma_chan_dev - relate sysfs device node to backing channel device
> >  * @chan: driver channel device
> >  * @device: sysfs device
> >  * @dev_id: parent dma_device dev_id
> >  * @chan_dma_dev: The channel is using custom/different dma-mapping
> >  * compared to the parent dma_device
> >  */
> > struct dma_chan_dev {
> > 	struct dma_chan *chan;
> > 	struct device device;
> > 	int dev_id;
> > 	bool chan_dma_dev;
> > };
> >
> > struct dma_chan {
> > 	struct dma_device *device; /// this one should be dmaengine
> > 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> > }
>
> I've tested your approach but it turns out that teh dma_chan_dev has no
> driver. Of course we could use the DL_FLAG_STATELESS flag but this is
> described as:
>
> | When driver presence on the supplier is irrelevant and only correct
> | suspend/resume and shutdown ordering is needed, the device link may
> | simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> | enforcing driver presence on the supplier is optional.
>
> I want to enforce the driver presence, therefore I used the manged flags
> which excludes the DL_FLAG_STATELESS, if I get it right.
>
> Please see the below the debug output:
>
> ** use the dmaengine device as supplier **
>
> device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1
>
>
> ** use the dma channel device as supplier **
>
> device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1

It should be similar with phy drivers, which phy_create() create individual
phy devices (like dma channel devices).

> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 51 at /drivers/base/core.c:1387 device_links_driver_bound+0x170/0x3a0
> ...
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
>
> As said, I get your point regarding the usage of the dma-channel device
> but I didn't found any reference to a driver which used the dma-channel
> device. Also since I want to have the supply driver to enforced by the
> devlink I don't want to use the DL_FLAG_STATELESS flag.

Maybe add DL_FLAG, link to parent's device driver. Need some time to
investigate more. PHY driver should good example to refer to.

>
> Regarding your point, that some DMA controllers may have seperate clocks
> for each channel: I think this can be handled by the dmaengine driver,
> e.g. via the device_alloc_chan_resources() hook.

device_alloc_chan_resources() is not efficient enough, most driver allocate
channel at probe, so clk of this channel will be always on. ideally, only
when consumer devices is runtime resume state,  turn on dma channel clock.

Frank
>
> @all
> I'm pleased about any input :)
>
> Regards,
>   Marco

Re: [PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

On 25-09-10, Frank Li wrote:
> On Wed, Sep 10, 2025 at 09:35:45PM +0200, Marco Felsch wrote:
> > On 25-09-09, Frank Li wrote:
> >
> > ...
> >
> > > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > > > --- a/drivers/dma/dmaengine.c
> > > > > > +++ b/drivers/dma/dmaengine.c
> > > > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > > >  	struct dma_device *d, *_d;
> > > > > >  	struct dma_chan *chan = NULL;
> > > > > > +	struct device_link *dl;
> > > > > >
> > > > > >  	if (is_of_node(fwnode))
> > > > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > > > >  #endif
> > > > > >
> > > > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > > > >
> > > > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > > > have device, consumer should link to chan's device, not dmaengine device
> > > > > because some dmaengine support per channel clock\power management.
> > > >
> > > > I get your point. Can you give me some pointers please? To me it seems
> > > > like the dma_chan_dev is only used for sysfs purpose according the
> > > > dmaengine.h.
> > >
> > > Not really, there are other dma engineer already reuse it for other purpose.
> > > So It needs update kernel doc for dma_chan_dev.
> >
> > Can you please provide me some pointers? I checked the kernel code base
> > for the struct::dma_chan_dev. I didn't found any references within the
> > dmaengine drivers. The only usage I found was for the sysfs purpose.
> 
> static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
> {
> 	struct device *chan_dev = &chan->dev->device;
> 	...
> }
> 
> >
> > > > > chan's device's parent devices is dmaengine devices. it should also work
> > > > > for sdma case
> > > >
> > > > I see, this must be tested of course.
> > > > > >         if (chan->device->create_devlink) {
> > > > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> > > >
> > > > According device_link.rst: using DL_FLAG_STATELESS and
> > > > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> > > >
> > > > >                 if (pm_runtime_active(dev))
> > > > >                         flags |= DL_FLAG_RPM_ACTIVE;
> > > >
> > > > This is of course interessting, thanks for the hint.
> > > >
> > > > > When create device link (apply channel), consume may active.
> > > >
> > > > I have read it as: "resue the supplier and ensure that the supplier
> > > > follows the consumer runtime state".
> > > >
> > > > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> > > >
> > > > Huh.. you used the dmaengine device too?
> > >
> > > /**
> > >  * struct dma_chan_dev - relate sysfs device node to backing channel device
> > >  * @chan: driver channel device
> > >  * @device: sysfs device
> > >  * @dev_id: parent dma_device dev_id
> > >  * @chan_dma_dev: The channel is using custom/different dma-mapping
> > >  * compared to the parent dma_device
> > >  */
> > > struct dma_chan_dev {
> > > 	struct dma_chan *chan;
> > > 	struct device device;
> > > 	int dev_id;
> > > 	bool chan_dma_dev;
> > > };
> > >
> > > struct dma_chan {
> > > 	struct dma_device *device; /// this one should be dmaengine
> > > 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> > > }
> >
> > I've tested your approach but it turns out that teh dma_chan_dev has no
> > driver. Of course we could use the DL_FLAG_STATELESS flag but this is
> > described as:
> >
> > | When driver presence on the supplier is irrelevant and only correct
> > | suspend/resume and shutdown ordering is needed, the device link may
> > | simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> > | enforcing driver presence on the supplier is optional.
> >
> > I want to enforce the driver presence, therefore I used the manged flags
> > which excludes the DL_FLAG_STATELESS, if I get it right.
> >
> > Please see the below the debug output:
> >
> > ** use the dmaengine device as supplier **
> >
> > device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1
> >
> >
> > ** use the dma channel device as supplier **
> >
> > device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> 
> It should be similar with phy drivers, which phy_create() create individual
> phy devices (like dma channel devices).

Unfortunately phy drivers do use the DL_FLAG_STATELESS mechanism. My
main goal was to have managed links to overcome the current situation:
dmaengine drivers can be removed without removing the consumer drivers
first.

You have a valid point by making use dma-channel devices ( dma<X>cha<Y>)
to manage suspend/resume, as well as runtime-PM for each channel.

But I see this rather as an addition to my solution because these links
must be stateless and stateless/unmanaged links don't guarantee the
correct remove order (my main goal).

That beeing said, I'm not sure how you want to handle the clock/power
enablement per channel-device. This would require additional work on the
dma_devclass to add a proper .pm hook else the PM and runtime-PM calls
are only forwarded to the parent dmaengine driver. On this level the
dmaengine driver has no knowledge which channel is going to be
enabled/disabled.

In conclusion, I see my approach as valid to ensure the correct remove
order. Your suggestion is valid and can be added later on too since this
needs more work to have a proper per-channel runtime-PM.

Regards,
  Marco

> 
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 51 at /drivers/base/core.c:1387 device_links_driver_bound+0x170/0x3a0
> > ...
> > ---[ end trace 0000000000000000 ]---
> > ------------[ cut here ]------------
> >
> > As said, I get your point regarding the usage of the dma-channel device
> > but I didn't found any reference to a driver which used the dma-channel
> > device. Also since I want to have the supply driver to enforced by the
> > devlink I don't want to use the DL_FLAG_STATELESS flag.
> 
> Maybe add DL_FLAG, link to parent's device driver. Need some time to
> investigate more. PHY driver should good example to refer to.
> 
> >
> > Regarding your point, that some DMA controllers may have seperate clocks
> > for each channel: I think this can be handled by the dmaengine driver,
> > e.g. via the device_alloc_chan_resources() hook.
> 
> device_alloc_chan_resources() is not efficient enough, most driver allocate
> channel at probe, so clk of this channel will be always on. ideally, only
> when consumer devices is runtime resume state,  turn on dma channel clock.
> 
> Frank
> >
> > @all
> > I'm pleased about any input :)
> >
> > Regards,
> >   Marco
> 

Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li]

On Thu, Sep 11, 2025 at 01:50:56PM +0200, Marco Felsch wrote:
> On 25-09-10, Frank Li wrote:
> > On Wed, Sep 10, 2025 at 09:35:45PM +0200, Marco Felsch wrote:
> > > On 25-09-09, Frank Li wrote:
> > >
> > > ...
> > >
> > > > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > > > > --- a/drivers/dma/dmaengine.c
> > > > > > > +++ b/drivers/dma/dmaengine.c
> > > > > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > > > >  	struct dma_device *d, *_d;
> > > > > > >  	struct dma_chan *chan = NULL;
> > > > > > > +	struct device_link *dl;
> > > > > > >
> > > > > > >  	if (is_of_node(fwnode))
> > > > > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > > > > >  #endif
> > > > > > >
> > > > > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > > > > >
> > > > > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > > > > have device, consumer should link to chan's device, not dmaengine device
> > > > > > because some dmaengine support per channel clock\power management.
> > > > >
> > > > > I get your point. Can you give me some pointers please? To me it seems
> > > > > like the dma_chan_dev is only used for sysfs purpose according the
> > > > > dmaengine.h.
> > > >
> > > > Not really, there are other dma engineer already reuse it for other purpose.
> > > > So It needs update kernel doc for dma_chan_dev.
> > >
> > > Can you please provide me some pointers? I checked the kernel code base
> > > for the struct::dma_chan_dev. I didn't found any references within the
> > > dmaengine drivers. The only usage I found was for the sysfs purpose.
> >
> > static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
> > {
> > 	struct device *chan_dev = &chan->dev->device;
> > 	...
> > }
> >
> > >
> > > > > > chan's device's parent devices is dmaengine devices. it should also work
> > > > > > for sdma case
> > > > >
> > > > > I see, this must be tested of course.
> > > > > > >         if (chan->device->create_devlink) {
> > > > > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> > > > >
> > > > > According device_link.rst: using DL_FLAG_STATELESS and
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> > > > >
> > > > > >                 if (pm_runtime_active(dev))
> > > > > >                         flags |= DL_FLAG_RPM_ACTIVE;
> > > > >
> > > > > This is of course interessting, thanks for the hint.
> > > > >
> > > > > > When create device link (apply channel), consume may active.
> > > > >
> > > > > I have read it as: "resue the supplier and ensure that the supplier
> > > > > follows the consumer runtime state".
> > > > >
> > > > > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> > > > >
> > > > > Huh.. you used the dmaengine device too?
> > > >
> > > > /**
> > > >  * struct dma_chan_dev - relate sysfs device node to backing channel device
> > > >  * @chan: driver channel device
> > > >  * @device: sysfs device
> > > >  * @dev_id: parent dma_device dev_id
> > > >  * @chan_dma_dev: The channel is using custom/different dma-mapping
> > > >  * compared to the parent dma_device
> > > >  */
> > > > struct dma_chan_dev {
> > > > 	struct dma_chan *chan;
> > > > 	struct device device;
> > > > 	int dev_id;
> > > > 	bool chan_dma_dev;
> > > > };
> > > >
> > > > struct dma_chan {
> > > > 	struct dma_device *device; /// this one should be dmaengine
> > > > 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> > > > }
> > >
> > > I've tested your approach but it turns out that teh dma_chan_dev has no
> > > driver. Of course we could use the DL_FLAG_STATELESS flag but this is
> > > described as:
> > >
> > > | When driver presence on the supplier is irrelevant and only correct
> > > | suspend/resume and shutdown ordering is needed, the device link may
> > > | simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> > > | enforcing driver presence on the supplier is optional.
> > >
> > > I want to enforce the driver presence, therefore I used the manged flags
> > > which excludes the DL_FLAG_STATELESS, if I get it right.
> > >
> > > Please see the below the debug output:
> > >
> > > ** use the dmaengine device as supplier **
> > >
> > > device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1
> > >
> > >
> > > ** use the dma channel device as supplier **
> > >
> > > device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> >
> > It should be similar with phy drivers, which phy_create() create individual
> > phy devices (like dma channel devices).
>
> Unfortunately phy drivers do use the DL_FLAG_STATELESS mechanism. My
> main goal was to have managed links to overcome the current situation:
> dmaengine drivers can be removed without removing the consumer drivers
> first.
>
> You have a valid point by making use dma-channel devices ( dma<X>cha<Y>)
> to manage suspend/resume, as well as runtime-PM for each channel.
>
> But I see this rather as an addition to my solution because these links
> must be stateless and stateless/unmanaged links don't guarantee the
> correct remove order (my main goal).

If that, phy should have simiar problems. It should be resolved at higher
layer to avoid fix this kinds of problem one by one.

>
> That beeing said, I'm not sure how you want to handle the clock/power
> enablement per channel-device. This would require additional work on the
> dma_devclass to add a proper .pm hook else the PM and runtime-PM calls
> are only forwarded to the parent dmaengine driver. On this level the
> dmaengine driver has no knowledge which channel is going to be
> enabled/disabled.

I have draft runtime pm patch for eDMA.

>
> In conclusion, I see my approach as valid to ensure the correct remove
> order. Your suggestion is valid and can be added later on too since this
> needs more work to have a proper per-channel runtime-PM.

We need pave a good road. This part is common dma-engine, which is worth to
do good solution.

Frank

>
> Regards,
>   Marco
>
> >
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 51 at /drivers/base/core.c:1387 device_links_driver_bound+0x170/0x3a0
> > > ...
> > > ---[ end trace 0000000000000000 ]---
> > > ------------[ cut here ]------------
> > >
> > > As said, I get your point regarding the usage of the dma-channel device
> > > but I didn't found any reference to a driver which used the dma-channel
> > > device. Also since I want to have the supply driver to enforced by the
> > > devlink I don't want to use the DL_FLAG_STATELESS flag.
> >
> > Maybe add DL_FLAG, link to parent's device driver. Need some time to
> > investigate more. PHY driver should good example to refer to.
> >
> > >
> > > Regarding your point, that some DMA controllers may have seperate clocks
> > > for each channel: I think this can be handled by the dmaengine driver,
> > > e.g. via the device_alloc_chan_resources() hook.
> >
> > device_alloc_chan_resources() is not efficient enough, most driver allocate
> > channel at probe, so clk of this channel will be always on. ideally, only
> > when consumer devices is runtime resume state,  turn on dma channel clock.
> >
> > Frank
> > >
> > > @all
> > > I'm pleased about any input :)
> > >
> > > Regards,
> > >   Marco
> >

Re: [PATCH 09/11] dmaengine: add support for device_link

[Marco Felsch]

On 25-09-11, Frank Li wrote:
> On Thu, Sep 11, 2025 at 01:50:56PM +0200, Marco Felsch wrote:
> > On 25-09-10, Frank Li wrote:
> > > On Wed, Sep 10, 2025 at 09:35:45PM +0200, Marco Felsch wrote:
> > > > On 25-09-09, Frank Li wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > > > > > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > > > > > > --- a/drivers/dma/dmaengine.c
> > > > > > > > +++ b/drivers/dma/dmaengine.c
> > > > > > > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > > > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > > > > > > >  	struct dma_device *d, *_d;
> > > > > > > >  	struct dma_chan *chan = NULL;
> > > > > > > > +	struct device_link *dl;
> > > > > > > >
> > > > > > > >  	if (is_of_node(fwnode))
> > > > > > > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > > > > > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > > > > > > >  	/* No functional issue if it fails, users are supposed to test before use */
> > > > > > > >  #endif
> > > > > > > >
> > > > > > > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> > > > > > >
> > > > > > > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > > > > > > have device, consumer should link to chan's device, not dmaengine device
> > > > > > > because some dmaengine support per channel clock\power management.
> > > > > >
> > > > > > I get your point. Can you give me some pointers please? To me it seems
> > > > > > like the dma_chan_dev is only used for sysfs purpose according the
> > > > > > dmaengine.h.
> > > > >
> > > > > Not really, there are other dma engineer already reuse it for other purpose.
> > > > > So It needs update kernel doc for dma_chan_dev.
> > > >
> > > > Can you please provide me some pointers? I checked the kernel code base
> > > > for the struct::dma_chan_dev. I didn't found any references within the
> > > > dmaengine drivers. The only usage I found was for the sysfs purpose.
> > >
> > > static void k3_configure_chan_coherency(struct dma_chan *chan, u32 asel)
> > > {
> > > 	struct device *chan_dev = &chan->dev->device;
> > > 	...
> > > }
> > >
> > > >
> > > > > > > chan's device's parent devices is dmaengine devices. it should also work
> > > > > > > for sdma case
> > > > > >
> > > > > > I see, this must be tested of course.
> > > > > > > >         if (chan->device->create_devlink) {
> > > > > > >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
> > > > > >
> > > > > > According device_link.rst: using DL_FLAG_STATELESS and
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
> > > > > >
> > > > > > >                 if (pm_runtime_active(dev))
> > > > > > >                         flags |= DL_FLAG_RPM_ACTIVE;
> > > > > >
> > > > > > This is of course interessting, thanks for the hint.
> > > > > >
> > > > > > > When create device link (apply channel), consume may active.
> > > > > >
> > > > > > I have read it as: "resue the supplier and ensure that the supplier
> > > > > > follows the consumer runtime state".
> > > > > >
> > > > > > >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
> > > > > >
> > > > > > Huh.. you used the dmaengine device too?
> > > > >
> > > > > /**
> > > > >  * struct dma_chan_dev - relate sysfs device node to backing channel device
> > > > >  * @chan: driver channel device
> > > > >  * @device: sysfs device
> > > > >  * @dev_id: parent dma_device dev_id
> > > > >  * @chan_dma_dev: The channel is using custom/different dma-mapping
> > > > >  * compared to the parent dma_device
> > > > >  */
> > > > > struct dma_chan_dev {
> > > > > 	struct dma_chan *chan;
> > > > > 	struct device device;
> > > > > 	int dev_id;
> > > > > 	bool chan_dma_dev;
> > > > > };
> > > > >
> > > > > struct dma_chan {
> > > > > 	struct dma_device *device; /// this one should be dmaengine
> > > > > 	struct dma_chan_dev *dev; /// this one is pre-chan device.
> > > > > }
> > > >
> > > > I've tested your approach but it turns out that teh dma_chan_dev has no
> > > > driver. Of course we could use the DL_FLAG_STATELESS flag but this is
> > > > described as:
> > > >
> > > > | When driver presence on the supplier is irrelevant and only correct
> > > > | suspend/resume and shutdown ordering is needed, the device link may
> > > > | simply be set up with the ``DL_FLAG_STATELESS`` flag.  In other words,
> > > > | enforcing driver presence on the supplier is optional.
> > > >
> > > > I want to enforce the driver presence, therefore I used the manged flags
> > > > which excludes the DL_FLAG_STATELESS, if I get it right.
> > > >
> > > > Please see the below the debug output:
> > > >
> > > > ** use the dmaengine device as supplier **
> > > >
> > > > device_link_init_status: supplier.dev:30bd0000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > > device_link_init_status: supplier.dev:30e10000.dma-controller supplier.drv:imx-sdma supplier.status:0x2 consumer:dev:30c20000.sai consumer.drv:fsl-sai consumer.status:0x1
> > > >
> > > >
> > > > ** use the dma channel device as supplier **
> > > >
> > > > device_link_init_status: supplier.dev:dma0chan0 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > > > device_link_init_status: supplier.dev:dma0chan1 supplier.drv:no-driver supplier.status:0x0 consumer:dev:30840000.spi consumer.drv:spi_imx consumer.status:0x1
> > >
> > > It should be similar with phy drivers, which phy_create() create individual
> > > phy devices (like dma channel devices).
> >
> > Unfortunately phy drivers do use the DL_FLAG_STATELESS mechanism. My
> > main goal was to have managed links to overcome the current situation:
> > dmaengine drivers can be removed without removing the consumer drivers
> > first.
> >
> > You have a valid point by making use dma-channel devices ( dma<X>cha<Y>)
> > to manage suspend/resume, as well as runtime-PM for each channel.
> >
> > But I see this rather as an addition to my solution because these links
> > must be stateless and stateless/unmanaged links don't guarantee the
> > correct remove order (my main goal).
> 
> If that, phy should have simiar problems. It should be resolved at higher
> layer to avoid fix this kinds of problem one by one.

I'm not sure because the devlink API is more or less clear about STATELESS
links. So phy should rather consider using managed links if they want to
ensure this.

> > That beeing said, I'm not sure how you want to handle the clock/power
> > enablement per channel-device. This would require additional work on the
> > dma_devclass to add a proper .pm hook else the PM and runtime-PM calls
> > are only forwarded to the parent dmaengine driver. On this level the
> > dmaengine driver has no knowledge which channel is going to be
> > enabled/disabled.
> 
> I have draft runtime pm patch for eDMA.
> 
> >
> > In conclusion, I see my approach as valid to ensure the correct remove
> > order. Your suggestion is valid and can be added later on too since this
> > needs more work to have a proper per-channel runtime-PM.
> 
> We need pave a good road. This part is common dma-engine, which is worth to
> do good solution.

+1

I will send a new v2 which split the devlink patches (common part and
the imx-sdma part) from the rest of this series. This decouples most of
the the imx-sdma cleanup from the devlink discussion and we can continue
there.

Regards,
  Marco