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

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

[Marco Felsch]

Hi,

by this series the i.MX SDMA handling for i.MX8M devices is fixed. This
is required because these SoCs do have multiple SPBA busses.

Furthermore this series does some cleanups to prepare the driver for the
upcoming DMA devlink support. The DMA devlink support is required to fix
the consumer <-> provider issue because 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.

Regards,
  Marco

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changes in v2:
- Link to v1: https://lore.kernel.org/r/20250903-v6-16-topic-sdma-v1-0-ac7bab629e8b@pengutronix.de
- Split DMA devlink support and SDMA driver fixes&cleanups into two series
- Make of_dma_controller_free() fix backportable
- Update struct sdma_channel documentation
- Shuffle patches to have fixes patches at the very start of the series
- Fix commit message wording
- Check return value of devm_add_action_or_reset()

---
Marco Felsch (10):
      dmaengine: imx-sdma: fix missing of_dma_controller_free()
      dmaengine: imx-sdma: fix spba-bus handling for i.MX8M
      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 devm_add_action_or_reset to unregiser the dma-controller
      dmaengine: imx-sdma: make use of dev_err_probe()

 drivers/dma/imx-sdma.c | 181 ++++++++++++++++++++++++++-----------------------
 1 file changed, 96 insertions(+), 85 deletions(-)
---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250903-v6-16-topic-sdma-4c8fd3bb0738

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

[PATCH v2 01/10] 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() during probe(). 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.

Use the driver remove() callback to make it possible to backport this
commit.

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 02a85d6f1bea2df7d355858094c0c0b0bd07148e..3ecb917214b1268b148a29df697b780bc462afa4 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2418,6 +2418,7 @@ static void sdma_remove(struct platform_device *pdev)
 	struct sdma_engine *sdma = platform_get_drvdata(pdev);
 	int i;
 
+	of_dma_controller_free(sdma->dev->of_node);
 	devm_free_irq(&pdev->dev, sdma->irq, sdma);
 	dma_async_device_unregister(&sdma->dma_device);
 	kfree(sdma->script_addrs);

-- 
2.47.3

Re: [PATCH v2 01/10] dmaengine: imx-sdma: fix missing of_dma_controller_free()

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:42PM +0200, Marco Felsch wrote:
>Add the missing of_dma_controller_free() to free the resources allocated
>via of_dma_controller_register() during probe(). 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.
>
>Use the driver remove() callback to make it possible to backport this
>commit.
>
>Fixes: 23e118113782 ("dma: imx-sdma: use module_platform_driver for SDMA driver")
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 01/10] dmaengine: imx-sdma: fix missing of_dma_controller_free()

[Frank Li]

On Thu, Sep 11, 2025 at 11:56:42PM +0200, Marco Felsch wrote:
> Add the missing of_dma_controller_free() to free the resources allocated
> via of_dma_controller_register() during probe(). 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.
>
> Use the driver remove() callback to make it possible to backport this
> commit.
>
> Fixes: 23e118113782 ("dma: imx-sdma: use module_platform_driver for SDMA driver")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---

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

>  drivers/dma/imx-sdma.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 02a85d6f1bea2df7d355858094c0c0b0bd07148e..3ecb917214b1268b148a29df697b780bc462afa4 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2418,6 +2418,7 @@ static void sdma_remove(struct platform_device *pdev)
>  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
>  	int i;
>
> +	of_dma_controller_free(sdma->dev->of_node);
>  	devm_free_irq(&pdev->dev, sdma->irq, sdma);
>  	dma_async_device_unregister(&sdma->dma_device);
>  	kfree(sdma->script_addrs);
>
> --
> 2.47.3
>

[PATCH v2 02/10] 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 | 58 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -429,6 +429,8 @@ struct sdma_desc {
  * @event_mask:		event mask used in p_2_p script
  * @watermark_level:	value for gReg[7], some script will extend it from
  *			basic watermark such as p_2_p
+ * @spba_start_addr:	SDMA controller SPBA bus start address
+ * @spba_end_addr:	SDMA controller SPBA bus end address
  * @shp_addr:		value for gReg[6]
  * @per_addr:		value for gReg[2]
  * @status:		status of dma channel
@@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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 +1552,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 +1864,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 +1895,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;
 }
 
@@ -2235,11 +2267,9 @@ 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_node *spba_bus;
 	const char *fw_name;
 	int ret;
 	int irq;
-	struct resource spba_res;
 	int i;
 	struct sdma_engine *sdma;
 	s32 *saddr_arr;
@@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
 			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);
 	}
 
 	/*

-- 
2.47.3

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

[Peng Fan]

Hi Marco,

On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
>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.

Could you please explain a bit why it is per sdma_channel, not per sdma_engine?

As I understand, all the channels belong to a sdma engine should use same
spba_{start,end}_addr.

Thanks,
Peng

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

[Marco Felsch]

Hi Peng,

On 25-09-12, Peng Fan wrote:
> Hi Marco,
> 
> On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
> >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.
> 
> Could you please explain a bit why it is per sdma_channel, not per sdma_engine?

Well first, the sdma-slave/user defines which SPBA bus is used.
Furthermore not all users have to be part of the same SPBA bus or be
part of a SPBA bus at all. E.g. the i.mx8mm.dtsi sdma1 engine is used
by uart4 (not part of a SPBA bus) and uart1/2/3 (part of the
spba-bus@30800000).

I know that the use-case: "The SDMA engine can serve for multiple users
which are not part of the same SPBA bus" is not yet used but having the
code in place makes the driver more future proof.

Therefore I think having the SPBA addresses per user is correct.

Regards,
  Marco

> As I understand, all the channels belong to a sdma engine should use same
> spba_{start,end}_addr.
> 
> Thanks,
> Peng
> 

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

[Frank Li]

On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
> 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>

I think the below method should be better.

of_translate_address(per_address) == OF_BAD_ADDR to check if belong spba-bus

aips3: bus@30800000 {
	...
        ranges = <0x30800000 0x30800000 0x400000>,
                 <0x8000000 0x8000000 0x10000000>;

                        spba1: spba-bus@30800000 {
                                compatible = "fsl,spba-bus", "simple-bus";
                                #address-cells = <1>;
                                #size-cells = <1>;
                                reg = <0x30800000 0x100000>;
                                ranges;

				...
				sdma1:

};

of_translate_address() will 1:1 map at spba-bus@30800000 spba1.
then
reach ranges = <0x30800000 0x30800000 0x400000> of aips3

if per_address is not in this range, it should return OF_BAD_ADDR. So
needn't parse reg of bus@30800000 at all.

Frank

> ---
>  drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -429,6 +429,8 @@ struct sdma_desc {
>   * @event_mask:		event mask used in p_2_p script
>   * @watermark_level:	value for gReg[7], some script will extend it from
>   *			basic watermark such as p_2_p
> + * @spba_start_addr:	SDMA controller SPBA bus start address
> + * @spba_end_addr:	SDMA controller SPBA bus end address
>   * @shp_addr:		value for gReg[6]
>   * @per_addr:		value for gReg[2]
>   * @status:		status of dma channel
> @@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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 +1552,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 +1864,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 +1895,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;
>  }
>
> @@ -2235,11 +2267,9 @@ 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_node *spba_bus;
>  	const char *fw_name;
>  	int ret;
>  	int irq;
> -	struct resource spba_res;
>  	int i;
>  	struct sdma_engine *sdma;
>  	s32 *saddr_arr;
> @@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
>  			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);
>  	}
>
>  	/*
>
> --
> 2.47.3
>

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

[Marco Felsch]

On 25-09-12, Frank Li wrote:
> On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
> > 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>
> 
> I think the below method should be better.
> 
> of_translate_address(per_address) == OF_BAD_ADDR to check if belong spba-bus

The SDMA engine doesn't have to be part of the SPBA bus, please see the
i.MX8MM for example.

Regards,
  Marco

> aips3: bus@30800000 {
> 	...
>         ranges = <0x30800000 0x30800000 0x400000>,
>                  <0x8000000 0x8000000 0x10000000>;
> 
>                         spba1: spba-bus@30800000 {
>                                 compatible = "fsl,spba-bus", "simple-bus";
>                                 #address-cells = <1>;
>                                 #size-cells = <1>;
>                                 reg = <0x30800000 0x100000>;
>                                 ranges;
> 
> 				...
> 				sdma1:
> 
> };
> 
> of_translate_address() will 1:1 map at spba-bus@30800000 spba1.
> then
> reach ranges = <0x30800000 0x30800000 0x400000> of aips3
> 
> if per_address is not in this range, it should return OF_BAD_ADDR. So
> needn't parse reg of bus@30800000 at all.
> 
> Frank
> 
> > ---
> >  drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 40 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -429,6 +429,8 @@ struct sdma_desc {
> >   * @event_mask:		event mask used in p_2_p script
> >   * @watermark_level:	value for gReg[7], some script will extend it from
> >   *			basic watermark such as p_2_p
> > + * @spba_start_addr:	SDMA controller SPBA bus start address
> > + * @spba_end_addr:	SDMA controller SPBA bus end address
> >   * @shp_addr:		value for gReg[6]
> >   * @per_addr:		value for gReg[2]
> >   * @status:		status of dma channel
> > @@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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 +1552,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 +1864,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 +1895,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;
> >  }
> >
> > @@ -2235,11 +2267,9 @@ 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_node *spba_bus;
> >  	const char *fw_name;
> >  	int ret;
> >  	int irq;
> > -	struct resource spba_res;
> >  	int i;
> >  	struct sdma_engine *sdma;
> >  	s32 *saddr_arr;
> > @@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
> >  			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);
> >  	}
> >
> >  	/*
> >
> > --
> > 2.47.3
> >
> 

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

[Frank Li]

On Fri, Sep 12, 2025 at 05:27:48PM +0200, Marco Felsch wrote:
> On 25-09-12, Frank Li wrote:
> > On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
> > > 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>
> >
> > I think the below method should be better.
> >
> > of_translate_address(per_address) == OF_BAD_ADDR to check if belong spba-bus
>
> The SDMA engine doesn't have to be part of the SPBA bus, please see the
> i.MX8MM for example.

which one, can you point me?

spba_bus = of_get_parent(chan->slave->of_node);

Actaully you get dma consumers' spba-bus, not dmaengine one.

And I also confused

 +	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;

what's purpsoe of this code, check if dma target address in spba_bus ?

And only here use spba_start_addr!

Frank

>
> Regards,
>   Marco
>
> > aips3: bus@30800000 {
> > 	...
> >         ranges = <0x30800000 0x30800000 0x400000>,
> >                  <0x8000000 0x8000000 0x10000000>;
> >
> >                         spba1: spba-bus@30800000 {
> >                                 compatible = "fsl,spba-bus", "simple-bus";
> >                                 #address-cells = <1>;
> >                                 #size-cells = <1>;
> >                                 reg = <0x30800000 0x100000>;
> >                                 ranges;
> >
> > 				...
> > 				sdma1:
> >
> > };
> >
> > of_translate_address() will 1:1 map at spba-bus@30800000 spba1.
> > then
> > reach ranges = <0x30800000 0x30800000 0x400000> of aips3
> >
> > if per_address is not in this range, it should return OF_BAD_ADDR. So
> > needn't parse reg of bus@30800000 at all.
> >
> > Frank
> >
> > > ---
> > >  drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++++++++++++++----------------
> > >  1 file changed, 40 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -429,6 +429,8 @@ struct sdma_desc {
> > >   * @event_mask:		event mask used in p_2_p script
> > >   * @watermark_level:	value for gReg[7], some script will extend it from
> > >   *			basic watermark such as p_2_p
> > > + * @spba_start_addr:	SDMA controller SPBA bus start address
> > > + * @spba_end_addr:	SDMA controller SPBA bus end address
> > >   * @shp_addr:		value for gReg[6]
> > >   * @per_addr:		value for gReg[2]
> > >   * @status:		status of dma channel
> > > @@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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 +1552,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 +1864,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 +1895,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;
> > >  }
> > >
> > > @@ -2235,11 +2267,9 @@ 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_node *spba_bus;
> > >  	const char *fw_name;
> > >  	int ret;
> > >  	int irq;
> > > -	struct resource spba_res;
> > >  	int i;
> > >  	struct sdma_engine *sdma;
> > >  	s32 *saddr_arr;
> > > @@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
> > >  			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);
> > >  	}
> > >
> > >  	/*
> > >
> > > --
> > > 2.47.3
> > >
> >

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

[Marco Felsch]

On 25-09-12, Frank Li wrote:
> On Fri, Sep 12, 2025 at 05:27:48PM +0200, Marco Felsch wrote:
> > On 25-09-12, Frank Li wrote:
> > > On Thu, Sep 11, 2025 at 11:56:43PM +0200, Marco Felsch wrote:
> > > > 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>
> > >
> > > I think the below method should be better.
> > >
> > > of_translate_address(per_address) == OF_BAD_ADDR to check if belong spba-bus
> >
> > The SDMA engine doesn't have to be part of the SPBA bus, please see the
> > i.MX8MM for example.
> 
> which one, can you point me?

The imx8mm.dtsi, e.g. the SDMA2 [1] is not part of the SPBA bus but
serves as DMA for devices within the SPBA bus [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm.dtsi?h=v6.17#n525
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm.dtsi?h=v6.17#n305

> spba_bus = of_get_parent(chan->slave->of_node);
> 
> Actaully you get dma consumers' spba-bus, not dmaengine one.

I know that and this is intended because the dmaengine is not part of
the SPBA bus. There was only one SPBA bus but this changed with the
i.MX8M* SoCs and the driver is not aware of this (as explained within
the commit message).

That beeing said, the dmaegine was never part of the SPBA bus, e.g. see
the imx6qdl.dtsi. There only one SDMA engine exists which is not part of
the SPBA bus but it serves as DMA engine for devices within the SPBA
bus:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl.dtsi?h=v6.17#n932
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl.dtsi?h=v6.17#n299

The driver logic only worked because there was only one SPBA bus.

> And I also confused
> 
>  +	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;
> 
> what's purpsoe of this code, check if dma target address in spba_bus ?

I didn't add this code, just adapted it. But yes somehow the address is
checked.

> And only here use spba_start_addr!

Again I didn't added the code. I just fixed it for multi SPBA bus SoCs.

Regards,
  Marco


> 
> Frank
> 
> >
> > Regards,
> >   Marco
> >
> > > aips3: bus@30800000 {
> > > 	...
> > >         ranges = <0x30800000 0x30800000 0x400000>,
> > >                  <0x8000000 0x8000000 0x10000000>;
> > >
> > >                         spba1: spba-bus@30800000 {
> > >                                 compatible = "fsl,spba-bus", "simple-bus";
> > >                                 #address-cells = <1>;
> > >                                 #size-cells = <1>;
> > >                                 reg = <0x30800000 0x100000>;
> > >                                 ranges;
> > >
> > > 				...
> > > 				sdma1:
> > >
> > > };
> > >
> > > of_translate_address() will 1:1 map at spba-bus@30800000 spba1.
> > > then
> > > reach ranges = <0x30800000 0x30800000 0x400000> of aips3
> > >
> > > if per_address is not in this range, it should return OF_BAD_ADDR. So
> > > needn't parse reg of bus@30800000 at all.
> > >
> > > Frank
> > >
> > > > ---
> > > >  drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++++++++++++++----------------
> > > >  1 file changed, 40 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > > index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -429,6 +429,8 @@ struct sdma_desc {
> > > >   * @event_mask:		event mask used in p_2_p script
> > > >   * @watermark_level:	value for gReg[7], some script will extend it from
> > > >   *			basic watermark such as p_2_p
> > > > + * @spba_start_addr:	SDMA controller SPBA bus start address
> > > > + * @spba_end_addr:	SDMA controller SPBA bus end address
> > > >   * @shp_addr:		value for gReg[6]
> > > >   * @per_addr:		value for gReg[2]
> > > >   * @status:		status of dma channel
> > > > @@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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 +1552,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 +1864,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 +1895,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;
> > > >  }
> > > >
> > > > @@ -2235,11 +2267,9 @@ 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_node *spba_bus;
> > > >  	const char *fw_name;
> > > >  	int ret;
> > > >  	int irq;
> > > > -	struct resource spba_res;
> > > >  	int i;
> > > >  	struct sdma_engine *sdma;
> > > >  	s32 *saddr_arr;
> > > > @@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
> > > >  			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);
> > > >  	}
> > > >
> > > >  	/*
> > > >
> > > > --
> > > > 2.47.3
> > > >
> > >
> 

RE: [PATCH v2 02/10] dmaengine: imx-sdma: fix spba-bus handling for i.MX8M

[Peng Fan]

> Subject: [PATCH v2 02/10] dmaengine: imx-sdma: fix spba-bus
> handling for i.MX8M
> 
> 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>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

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

[Shengjiu Wang]

On Fri, Sep 12, 2025 at 6:05 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> 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 | 58 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 3ecb917214b1268b148a29df697b780bc462afa4..56daaeb7df03986850c9c74273d0816700581dc0 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -429,6 +429,8 @@ struct sdma_desc {
>   * @event_mask:                event mask used in p_2_p script
>   * @watermark_level:   value for gReg[7], some script will extend it from
>   *                     basic watermark such as p_2_p
> + * @spba_start_addr:   SDMA controller SPBA bus start address
> + * @spba_end_addr:     SDMA controller SPBA bus end address
>   * @shp_addr:          value for gReg[6]
>   * @per_addr:          value for gReg[2]
>   * @status:            status of dma channel
> @@ -461,6 +463,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 +538,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 +1238,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 +1263,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 +1447,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);

if the chan is requested by __dma_request_channel(),  the chan->slave = NULL
Then there will be an issue here.

Best regards
Shengjiu Wang

> +       /* 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 +1552,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 +1864,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 +1895,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;
>  }
>
> @@ -2235,11 +2267,9 @@ 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_node *spba_bus;
>         const char *fw_name;
>         int ret;
>         int irq;
> -       struct resource spba_res;
>         int i;
>         struct sdma_engine *sdma;
>         s32 *saddr_arr;
> @@ -2375,14 +2405,6 @@ static int sdma_probe(struct platform_device *pdev)
>                         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);
>         }
>
>         /*
>
> --
> 2.47.3
>
>

[PATCH v2 03/10] 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.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 56daaeb7df03986850c9c74273d0816700581dc0..dab3589e1c8d9efe06e16925c53554f8e22ce679 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2355,11 +2355,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)
@@ -2399,12 +2397,10 @@ 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;
 	}
 
 	/*

-- 
2.47.3

Re: [PATCH v2 03/10] dmaengine: imx-sdma: drop legacy device_node np check

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:44PM +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.
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

[PATCH v2 04/10] 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().

Reviewed-by: Frank Li <Frank.Li@nxp.com>
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 dab3589e1c8d9efe06e16925c53554f8e22ce679..44411c15029c9b09f307c4b9e3f7b1ab77fa5093 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2446,11 +2446,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.3

Re: [PATCH v2 04/10] dmaengine: imx-sdma: sdma_remove minor cleanups

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:45PM +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().
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

[PATCH v2 05/10] dmaengine: imx-sdma: cosmetic cleanup

[Marco Felsch]

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

Reviewed-by: Frank Li <Frank.Li@nxp.com>
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 44411c15029c9b09f307c4b9e3f7b1ab77fa5093..b0def2bde77fe53b0805bddc0c5d6116c9cefcbe 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2266,7 +2266,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;
 	const char *fw_name;
 	int ret;
 	int irq;
@@ -2274,18 +2275,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)
@@ -2295,11 +2296,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);
 
@@ -2311,8 +2312,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;
 
@@ -2357,7 +2358,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)
@@ -2370,7 +2371,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;
@@ -2393,13 +2394,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;
 	}
 
@@ -2411,11 +2412,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.3

Re: [PATCH v2 05/10] dmaengine: imx-sdma: cosmetic cleanup

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:46PM +0200, Marco Felsch wrote:
>Make use of local struct device pointer to not dereference the
>platform_device pointer everytime.
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

[PATCH v2 06/10] 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.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
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 b0def2bde77fe53b0805bddc0c5d6116c9cefcbe..d6239900ba12063bdb7d807db1bdbdc2b446a94c 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2283,6 +2283,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;
@@ -2319,12 +2323,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++)
@@ -2362,11 +2360,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);
@@ -2395,7 +2393,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);
@@ -2423,8 +2421,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:
@@ -2440,7 +2436,6 @@ static void sdma_remove(struct platform_device *pdev)
 	of_dma_controller_free(sdma->dev->of_node);
 	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.3

Re: [PATCH v2 06/10] dmaengine: imx-sdma: make use of devm_kzalloc for script_addrs

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:47PM +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.
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

[PATCH v2 07/10] 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.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
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 d6239900ba12063bdb7d807db1bdbdc2b446a94c..d39589c20c4b2a26d0239feb86cce8d5a0f5acdd 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2300,26 +2300,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;
 
@@ -2360,11 +2352,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);
@@ -2393,7 +2385,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);
@@ -2421,10 +2413,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;
 }
 
@@ -2436,8 +2425,6 @@ static void sdma_remove(struct platform_device *pdev)
 	of_dma_controller_free(sdma->dev->of_node);
 	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.3

Re: [PATCH v2 07/10] dmaengine: imx-sdma: make use of devm_clk_get_prepared()

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:48PM +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.
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

[PATCH v2 08/10] 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 to turn off the IRQs before calling
dma_async_device_unregister().

Furthermore it removes the last goto error handling within probe() and
trims the remove().

Make use of disable_irq() and let the devm-irq do the job to free the
IRQ, because the only purpose of using devm_free_irq() was to disable
the IRQ before calling dma_async_device_unregister().

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

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index d39589c20c4b2a26d0239feb86cce8d5a0f5acdd..d6d0d4300f540268a3ab4a6b14af685f7b93275a 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2264,6 +2264,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;
@@ -2388,10 +2396,16 @@ static int sdma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
+	if (ret) {
+		dev_err(dev, "Unable to register release hook\n");
+		return ret;
+	}
+
 	ret = of_dma_controller_register(np, sdma_xlate, sdma);
 	if (ret) {
 		dev_err(dev, "failed to register controller\n");
-		goto err_register;
+		return ret;
 	}
 
 	/*
@@ -2410,11 +2424,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)
@@ -2423,8 +2432,6 @@ static void sdma_remove(struct platform_device *pdev)
 	int i;
 
 	of_dma_controller_free(sdma->dev->of_node);
-	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.3

Re: [PATCH v2 08/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:49PM +0200, Marco Felsch wrote:
>Make use of the devm_add_action_or_reset() to register a custom devm_
>release hook. This is required to turn off the IRQs before calling
>dma_async_device_unregister().
>
>Furthermore it removes the last goto error handling within probe() and
>trims the remove().
>
>Make use of disable_irq() and let the devm-irq do the job to free the
>IRQ, because the only purpose of using devm_free_irq() was to disable
>the IRQ before calling dma_async_device_unregister().
>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 08/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Frank Li]

On Thu, Sep 11, 2025 at 11:56:49PM +0200, Marco Felsch wrote:
> Make use of the devm_add_action_or_reset() to register a custom devm_
> release hook. This is required to turn off the IRQs before calling
> dma_async_device_unregister().
>
> Furthermore it removes the last goto error handling within probe() and
> trims the remove().
>
> Make use of disable_irq() and let the devm-irq do the job to free the
> IRQ, because the only purpose of using devm_free_irq() was to disable
> the IRQ before calling dma_async_device_unregister().
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index d39589c20c4b2a26d0239feb86cce8d5a0f5acdd..d6d0d4300f540268a3ab4a6b14af685f7b93275a 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2264,6 +2264,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);

May not related this cleanup patch, I am just curious why not mask sdma irq
request.

> +	dma_async_device_unregister(&sdma->dma_device);
> +}
> +
>  static int sdma_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -2388,10 +2396,16 @@ static int sdma_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> +	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> +	if (ret) {
> +		dev_err(dev, "Unable to register release hook\n");
> +		return ret;
> +	}

why not use dev_err_probe() her?

> +
>  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
>  	if (ret) {
>  		dev_err(dev, "failed to register controller\n");
> -		goto err_register;
> +		return ret;

the same here.

Frank
>  	}
>
>  	/*
> @@ -2410,11 +2424,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)
> @@ -2423,8 +2432,6 @@ static void sdma_remove(struct platform_device *pdev)
>  	int i;
>
>  	of_dma_controller_free(sdma->dev->of_node);
> -	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.3
>

Re: [PATCH v2 08/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Marco Felsch]

On 25-09-12, Frank Li wrote:
> On Thu, Sep 11, 2025 at 11:56:49PM +0200, Marco Felsch wrote:
> > Make use of the devm_add_action_or_reset() to register a custom devm_
> > release hook. This is required to turn off the IRQs before calling
> > dma_async_device_unregister().
> >
> > Furthermore it removes the last goto error handling within probe() and
> > trims the remove().
> >
> > Make use of disable_irq() and let the devm-irq do the job to free the
> > IRQ, because the only purpose of using devm_free_irq() was to disable
> > the IRQ before calling dma_async_device_unregister().
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index d39589c20c4b2a26d0239feb86cce8d5a0f5acdd..d6d0d4300f540268a3ab4a6b14af685f7b93275a 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -2264,6 +2264,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);
> 
> May not related this cleanup patch, I am just curious why not mask sdma irq
> request.

You mean by setting irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)
infront? Not sure if this is required since this is just the cleanup
path.

> > +	dma_async_device_unregister(&sdma->dma_device);
> > +}
> > +
> >  static int sdma_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -2388,10 +2396,16 @@ static int sdma_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >
> > +	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> > +	if (ret) {
> > +		dev_err(dev, "Unable to register release hook\n");
> > +		return ret;
> > +	}
> 
> why not use dev_err_probe() her?

Please see my last patch.

Regards,
  Marco

> 
> > +
> >  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> >  	if (ret) {
> >  		dev_err(dev, "failed to register controller\n");
> > -		goto err_register;
> > +		return ret;
> 
> the same here.
> 
> Frank
> >  	}
> >
> >  	/*
> > @@ -2410,11 +2424,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)
> > @@ -2423,8 +2432,6 @@ static void sdma_remove(struct platform_device *pdev)
> >  	int i;
> >
> >  	of_dma_controller_free(sdma->dev->of_node);
> > -	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.3
> >
> 

Re: [PATCH v2 08/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Frank Li]

On Fri, Sep 12, 2025 at 05:25:08PM +0200, Marco Felsch wrote:
> On 25-09-12, Frank Li wrote:
> > On Thu, Sep 11, 2025 at 11:56:49PM +0200, Marco Felsch wrote:
> > > Make use of the devm_add_action_or_reset() to register a custom devm_
> > > release hook. This is required to turn off the IRQs before calling
> > > dma_async_device_unregister().
> > >
> > > Furthermore it removes the last goto error handling within probe() and
> > > trims the remove().
> > >
> > > Make use of disable_irq() and let the devm-irq do the job to free the
> > > IRQ, because the only purpose of using devm_free_irq() was to disable
> > > the IRQ before calling dma_async_device_unregister().
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/dma/imx-sdma.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > index d39589c20c4b2a26d0239feb86cce8d5a0f5acdd..d6d0d4300f540268a3ab4a6b14af685f7b93275a 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -2264,6 +2264,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);
> >
> > May not related this cleanup patch, I am just curious why not mask sdma irq
> > request.
>
> You mean by setting irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)
> infront? Not sure if this is required since this is just the cleanup
> path.

It is not important for this patch.

>
> > > +	dma_async_device_unregister(&sdma->dma_device);
> > > +}
> > > +
> > >  static int sdma_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > > @@ -2388,10 +2396,16 @@ static int sdma_probe(struct platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >
> > > +	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> > > +	if (ret) {
> > > +		dev_err(dev, "Unable to register release hook\n");
> > > +		return ret;
> > > +	}
> >
> > why not use dev_err_probe() her?
>
> Please see my last patch.

You can direct use dev_err_probe() here, instead replace all in last one.

Frank

>
> Regards,
>   Marco
>
> >
> > > +
> > >  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> > >  	if (ret) {
> > >  		dev_err(dev, "failed to register controller\n");
> > > -		goto err_register;
> > > +		return ret;
> >
> > the same here.
> >
> > Frank
> > >  	}
> > >
> > >  	/*
> > > @@ -2410,11 +2424,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)
> > > @@ -2423,8 +2432,6 @@ static void sdma_remove(struct platform_device *pdev)
> > >  	int i;
> > >
> > >  	of_dma_controller_free(sdma->dev->of_node);
> > > -	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.3
> > >
> >

Re: [PATCH v2 08/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma_device

[Marco Felsch]

On 25-09-12, Frank Li wrote:
> On Fri, Sep 12, 2025 at 05:25:08PM +0200, Marco Felsch wrote:
> > On 25-09-12, Frank Li wrote:
> > > On Thu, Sep 11, 2025 at 11:56:49PM +0200, Marco Felsch wrote:
> > > > Make use of the devm_add_action_or_reset() to register a custom devm_
> > > > release hook. This is required to turn off the IRQs before calling
> > > > dma_async_device_unregister().
> > > >
> > > > Furthermore it removes the last goto error handling within probe() and
> > > > trims the remove().
> > > >
> > > > Make use of disable_irq() and let the devm-irq do the job to free the
> > > > IRQ, because the only purpose of using devm_free_irq() was to disable
> > > > the IRQ before calling dma_async_device_unregister().
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  drivers/dma/imx-sdma.c | 23 +++++++++++++++--------
> > > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > > index d39589c20c4b2a26d0239feb86cce8d5a0f5acdd..d6d0d4300f540268a3ab4a6b14af685f7b93275a 100644
> > > > --- a/drivers/dma/imx-sdma.c
> > > > +++ b/drivers/dma/imx-sdma.c
> > > > @@ -2264,6 +2264,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);
> > >
> > > May not related this cleanup patch, I am just curious why not mask sdma irq
> > > request.
> >
> > You mean by setting irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)
> > infront? Not sure if this is required since this is just the cleanup
> > path.
> 
> It is not important for this patch.
> 
> >
> > > > +	dma_async_device_unregister(&sdma->dma_device);
> > > > +}
> > > > +
> > > >  static int sdma_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct device *dev = &pdev->dev;
> > > > @@ -2388,10 +2396,16 @@ static int sdma_probe(struct platform_device *pdev)
> > > >  		return ret;
> > > >  	}
> > > >
> > > > +	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Unable to register release hook\n");
> > > > +		return ret;
> > > > +	}
> > >
> > > why not use dev_err_probe() her?
> >
> > Please see my last patch.
> 
> You can direct use dev_err_probe() here, instead replace all in last one.

I get your point and yes I could do this, but I have to do the last
patch anyway. Therefore I'm not sure what difference this makes for this
series.

> 
> Frank
> 
> >
> > Regards,
> >   Marco
> >
> > >
> > > > +
> > > >  	ret = of_dma_controller_register(np, sdma_xlate, sdma);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "failed to register controller\n");
> > > > -		goto err_register;
> > > > +		return ret;
> > >
> > > the same here.
> > >
> > > Frank
> > > >  	}
> > > >
> > > >  	/*
> > > > @@ -2410,11 +2424,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)
> > > > @@ -2423,8 +2432,6 @@ static void sdma_remove(struct platform_device *pdev)
> > > >  	int i;
> > > >
> > > >  	of_dma_controller_free(sdma->dev->of_node);
> > > > -	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.3
> > > >
> > >
> 

[PATCH v2 09/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma-controller

[Marco Felsch]

Use the devres capabilities to cleanup the driver remove() callback.

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

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index d6d0d4300f540268a3ab4a6b14af685f7b93275a..a7e6554ca223e2e980caf2e2dea832db9ad60ed6 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2264,6 +2264,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;
@@ -2408,6 +2415,12 @@ static int sdma_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
+	if (ret) {
+		dev_err(dev, "failed to register of-dma-controller unregister hook\n");
+		return ret;
+	}
+
 	/*
 	 * Because that device tree does not encode ROM script address,
 	 * the RAM script in firmware is mandatory for device tree
@@ -2431,7 +2444,6 @@ static void sdma_remove(struct platform_device *pdev)
 	struct sdma_engine *sdma = platform_get_drvdata(pdev);
 	int i;
 
-	of_dma_controller_free(sdma->dev->of_node);
 	/* Kill the tasklet */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
 		struct sdma_channel *sdmac = &sdma->channel[i];

-- 
2.47.3

Re: [PATCH v2 09/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma-controller

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:50PM +0200, Marco Felsch wrote:
>Use the devres capabilities to cleanup the driver remove() callback.
>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 09/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma-controller

[Frank Li]

On Thu, Sep 11, 2025 at 11:56:50PM +0200, Marco Felsch wrote:
> Use the devres capabilities to cleanup the driver remove() callback.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index d6d0d4300f540268a3ab4a6b14af685f7b93275a..a7e6554ca223e2e980caf2e2dea832db9ad60ed6 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2264,6 +2264,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;
> @@ -2408,6 +2415,12 @@ static int sdma_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> +	ret = devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
> +	if (ret) {
> +		dev_err(dev, "failed to register of-dma-controller unregister hook\n");
> +		return ret;
> +	}
> +

return dev_err_probe()

Frank
>  	/*
>  	 * Because that device tree does not encode ROM script address,
>  	 * the RAM script in firmware is mandatory for device tree
> @@ -2431,7 +2444,6 @@ static void sdma_remove(struct platform_device *pdev)
>  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
>  	int i;
>
> -	of_dma_controller_free(sdma->dev->of_node);
>  	/* Kill the tasklet */
>  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
>  		struct sdma_channel *sdmac = &sdma->channel[i];
>
> --
> 2.47.3
>

Re: [PATCH v2 09/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma-controller

[Marco Felsch]

On 25-09-12, Frank Li wrote:
> On Thu, Sep 11, 2025 at 11:56:50PM +0200, Marco Felsch wrote:
> > Use the devres capabilities to cleanup the driver remove() callback.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index d6d0d4300f540268a3ab4a6b14af685f7b93275a..a7e6554ca223e2e980caf2e2dea832db9ad60ed6 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -2264,6 +2264,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;
> > @@ -2408,6 +2415,12 @@ static int sdma_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >
> > +	ret = devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register of-dma-controller unregister hook\n");
> > +		return ret;
> > +	}
> > +
> 
> return dev_err_probe()

Please check my last patch.

Regards,
  Marco

> 
> Frank
> >  	/*
> >  	 * Because that device tree does not encode ROM script address,
> >  	 * the RAM script in firmware is mandatory for device tree
> > @@ -2431,7 +2444,6 @@ static void sdma_remove(struct platform_device *pdev)
> >  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
> >  	int i;
> >
> > -	of_dma_controller_free(sdma->dev->of_node);
> >  	/* Kill the tasklet */
> >  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> >  		struct sdma_channel *sdmac = &sdma->channel[i];
> >
> > --
> > 2.47.3
> >
> 

Re: [PATCH v2 09/10] dmaengine: imx-sdma: make use of devm_add_action_or_reset to unregiser the dma-controller

[Frank Li]

On Fri, Sep 12, 2025 at 05:28:09PM +0200, Marco Felsch wrote:
> On 25-09-12, Frank Li wrote:
> > On Thu, Sep 11, 2025 at 11:56:50PM +0200, Marco Felsch wrote:
> > > Use the devres capabilities to cleanup the driver remove() callback.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/dma/imx-sdma.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > index d6d0d4300f540268a3ab4a6b14af685f7b93275a..a7e6554ca223e2e980caf2e2dea832db9ad60ed6 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -2264,6 +2264,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;
> > > @@ -2408,6 +2415,12 @@ static int sdma_probe(struct platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >
> > > +	ret = devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to register of-dma-controller unregister hook\n");
> > > +		return ret;
> > > +	}
> > > +
> >
> > return dev_err_probe()
>
> Please check my last patch.

You can use dev_err_probe() here, last patch only convert exist one.
Avoid add A, then remove A later.

Frank

>
> Regards,
>   Marco
>
> >
> > Frank
> > >  	/*
> > >  	 * Because that device tree does not encode ROM script address,
> > >  	 * the RAM script in firmware is mandatory for device tree
> > > @@ -2431,7 +2444,6 @@ static void sdma_remove(struct platform_device *pdev)
> > >  	struct sdma_engine *sdma = platform_get_drvdata(pdev);
> > >  	int i;
> > >
> > > -	of_dma_controller_free(sdma->dev->of_node);
> > >  	/* Kill the tasklet */
> > >  	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> > >  		struct sdma_channel *sdmac = &sdma->channel[i];
> > >
> > > --
> > > 2.47.3
> > >
> >

[PATCH v2 10/10] dmaengine: imx-sdma: make use of dev_err_probe()

[Marco Felsch]

Add dev_err_probe() at return path of probe() to support users to
identify issues easier.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a7e6554ca223e2e980caf2e2dea832db9ad60ed6..d4430e6e56deda7de3538e42af7987a456957b43 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -2292,7 +2292,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)
@@ -2309,24 +2309,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;
 
@@ -2367,11 +2367,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);
@@ -2398,28 +2398,21 @@ 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");
 
 	ret = devm_add_action_or_reset(dev, sdma_dma_device_unregister_action, sdma);
-	if (ret) {
-		dev_err(dev, "Unable to register release hook\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to register release hook\n");
 
 	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");
 
 	ret = devm_add_action_or_reset(dev, sdma_dma_of_dma_controller_unregister_action, sdma);
-	if (ret) {
-		dev_err(dev, "failed to register of-dma-controller unregister hook\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register of-dma-controller unregister hook\n");
 
 	/*
 	 * Because that device tree does not encode ROM script address,

-- 
2.47.3

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

[Peng Fan]

On Thu, Sep 11, 2025 at 11:56:51PM +0200, Marco Felsch wrote:
>Add dev_err_probe() at return path of probe() to support users to
>identify issues easier.
>
>Reviewed-by: Frank Li <Frank.Li@nxp.com>
>Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 00/10] i.MX SDMA cleanups and fixes

[Frank Li]

On Thu, Sep 11, 2025 at 11:56:41PM +0200, Marco Felsch wrote:
> Hi,
>
> by this series the i.MX SDMA handling for i.MX8M devices is fixed. This
> is required because these SoCs do have multiple SPBA busses.
>
> Furthermore this series does some cleanups to prepare the driver for the
> upcoming DMA devlink support. The DMA devlink support is required to fix
> the consumer <-> provider issue because 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.

Marco Felsch:

	Can you help rebase these patches?

Frank

>
> Regards,
>   Marco
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changes in v2:
> - Link to v1: https://lore.kernel.org/r/20250903-v6-16-topic-sdma-v1-0-ac7bab629e8b@pengutronix.de
> - Split DMA devlink support and SDMA driver fixes&cleanups into two series
> - Make of_dma_controller_free() fix backportable
> - Update struct sdma_channel documentation
> - Shuffle patches to have fixes patches at the very start of the series
> - Fix commit message wording
> - Check return value of devm_add_action_or_reset()
>
> ---
> Marco Felsch (10):
>       dmaengine: imx-sdma: fix missing of_dma_controller_free()
>       dmaengine: imx-sdma: fix spba-bus handling for i.MX8M
>       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 devm_add_action_or_reset to unregiser the dma-controller
>       dmaengine: imx-sdma: make use of dev_err_probe()
>
>  drivers/dma/imx-sdma.c | 181 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 96 insertions(+), 85 deletions(-)
> ---
> base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
> change-id: 20250903-v6-16-topic-sdma-4c8fd3bb0738
>
> Best regards,
> --
> Marco Felsch <m.felsch@pengutronix.de>
>

Re: [PATCH v2 00/10] i.MX SDMA cleanups and fixes

[Marco Felsch]

On 26-02-25, Frank Li wrote:
> On Thu, Sep 11, 2025 at 11:56:41PM +0200, Marco Felsch wrote:
> > Hi,
> >
> > by this series the i.MX SDMA handling for i.MX8M devices is fixed. This
> > is required because these SoCs do have multiple SPBA busses.
> >
> > Furthermore this series does some cleanups to prepare the driver for the
> > upcoming DMA devlink support. The DMA devlink support is required to fix
> > the consumer <-> provider issue because 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.
> 
> Marco Felsch:
> 
> 	Can you help rebase these patches?

Sure, will do.

Regards,
  Marco


> 
> Frank
> 
> >
> > Regards,
> >   Marco
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changes in v2:
> > - Link to v1: https://lore.kernel.org/r/20250903-v6-16-topic-sdma-v1-0-ac7bab629e8b@pengutronix.de
> > - Split DMA devlink support and SDMA driver fixes&cleanups into two series
> > - Make of_dma_controller_free() fix backportable
> > - Update struct sdma_channel documentation
> > - Shuffle patches to have fixes patches at the very start of the series
> > - Fix commit message wording
> > - Check return value of devm_add_action_or_reset()
> >
> > ---
> > Marco Felsch (10):
> >       dmaengine: imx-sdma: fix missing of_dma_controller_free()
> >       dmaengine: imx-sdma: fix spba-bus handling for i.MX8M
> >       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 devm_add_action_or_reset to unregiser the dma-controller
> >       dmaengine: imx-sdma: make use of dev_err_probe()
> >
> >  drivers/dma/imx-sdma.c | 181 ++++++++++++++++++++++++++-----------------------
> >  1 file changed, 96 insertions(+), 85 deletions(-)
> > ---
> > base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
> > change-id: 20250903-v6-16-topic-sdma-4c8fd3bb0738
> >
> > Best regards,
> > --
> > Marco Felsch <m.felsch@pengutronix.de>
> >
> 

-- 
#gernperDu 
#CallMeByMyFirstName

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |