Re: [PATCH v3 5/9] dmaengine: stm32-dma3: Use bus width capability helpers

[Amelie Delaunay]

Hi,

Thanks for the updates!

On 8/31/26 13:46, Nuno Sá wrote:
> Advertise the controller-wide bus width capabilities through the new
> dma_bus_width_set_many() helper and clear the per-channel unsupported
> widths through dma_bus_width_clear().
>
> While at it, validate the requested transfer bus widths against the new
> capability mask instead of the legacy u32 one. Besides not depending on
> the legacy field anymore, this also stops BIT() from being fed a bus
> width coming from the device tree without any bound check.
>
> Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
>   drivers/dma/stm32/stm32-dma3.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/dma/stm32/stm32-dma3.c b/drivers/dma/stm32/stm32-dma3.c
> index 4724e7fa0008..0689aeb7cff5 100644
> --- a/drivers/dma/stm32/stm32-dma3.c
> +++ b/drivers/dma/stm32/stm32-dma3.c
> @@ -9,6 +9,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/clk.h>
>   #include <linux/dma-mapping.h>
> +#include <linux/dma/engine/widthmask.h>
>   #include <linux/dmaengine.h>
>   #include <linux/dmapool.h>
>   #include <linux/init.h>
> @@ -588,7 +589,8 @@ static int stm32_dma3_chan_prep_hw(struct stm32_dma3_chan *chan, enum dma_transf
>   	dbl_max = chan->dma_config.dst_maxburst ? : 1;
>   
>   	/* Following conditions would raise User Setting Error interrupt */
> -	if (!(dma_device.src_addr_widths & BIT(sdw)) || !(dma_device.dst_addr_widths & BIT(ddw))) {
> +	if (!dma_bus_width_test(dma_device.src_bus_widths, sdw) ||
> +	    !dma_bus_width_test(dma_device.dst_bus_widths, ddw)) {

Just to come back to one point you mentioned in your cover letter, "this 
stops BIT() from being fed an unvalidated device tree bus width". Here, 
sdw and ddw are set either by dma_config.[src|dst]_addr_width, or forced 
to DMA_SLAVE_BUSWIDTH_4_BYTES or DMA_SLAVE_BUSWIDTH_8_BYTES depending on 
the channel capabilities. So, unless I'm missing something, they do not 
come from an "unvalidated *device tree* bus width".

Anyway, it's great to see another use of the new API.


>   		dev_err(chan2dev(chan), "Bus width (src=%u, dst=%u) not supported\n", sdw, ddw);
>   		return -EINVAL;
>   	}
> @@ -1469,14 +1471,14 @@ static void stm32_dma3_caps(struct dma_chan *c, struct dma_slave_caps *caps)
>   
>   	if (!chan->fifo_size) {
>   		caps->max_burst = 0;
> -		caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -		caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +		dma_bus_width_clear(caps->src_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> +		dma_bus_width_clear(caps->dst_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
>   	} else {
>   		/* Burst transfer should not exceed half of the fifo size */
>   		caps->max_burst = chan->max_burst;
>   		if (caps->max_burst < DMA_SLAVE_BUSWIDTH_8_BYTES) {
> -			caps->src_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -			caps->dst_addr_widths &= ~BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +			dma_bus_width_clear(caps->src_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
> +			dma_bus_width_clear(caps->dst_bus_widths, DMA_SLAVE_BUSWIDTH_8_BYTES);
>   		}
>   	}
>   }
> @@ -1734,6 +1736,12 @@ static int stm32_dma3_probe(struct platform_device *pdev)
>   	struct reset_control *reset;
>   	struct stm32_dma3_chan *chan;
>   	struct dma_device *dma_dev;
> +	enum dma_slave_buswidth buswidths[] = {
> +		DMA_SLAVE_BUSWIDTH_1_BYTE,
> +		DMA_SLAVE_BUSWIDTH_2_BYTES,
> +		DMA_SLAVE_BUSWIDTH_4_BYTES,
> +		DMA_SLAVE_BUSWIDTH_8_BYTES,
> +	};
>   	u32 master_ports, chan_reserved, i, verr;
>   	u64 hwcfgr;
>   	int ret;
> @@ -1775,14 +1783,14 @@ static int stm32_dma3_probe(struct platform_device *pdev)
>   	 * channel, and can only access address at even boundaries, multiple of the buswidth.
>   	 */
>   	dma_dev->copy_align = DMAENGINE_ALIGN_8_BYTES;
> -	dma_dev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> -	dma_dev->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> -				   BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
> +	ret = dma_bus_width_set_many(dma_dev->src_bus_widths, buswidths, ARRAY_SIZE(buswidths));
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	ret = dma_bus_width_set_many(dma_dev->dst_bus_widths, buswidths, ARRAY_SIZE(buswidths));
> +	if (ret)
> +		goto err_clk_disable;
> +
>   	dma_dev->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) | BIT(DMA_MEM_TO_MEM);
>   
>   	dma_dev->descriptor_reuse = true;

It looks nicer this way. Thanks!

Reviewed-by: Amelie Delaunay <amelie.delaunay@xxxxxxxxxxx>

Regards,
Amelie