Re: [PATCH v3 1/9] dmaengine: Support bus widths of 32 bytes and above

["Nuno Sá" <nuno.sa@analog.com>]

On Mon, Aug 31, 2026 at 04:56:18PM +0100, Nuno Sá wrote:
> On Mon, Aug 31, 2026 at 04:45:15PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 31, 2026 at 12:46:38PM +0100, Nuno Sá wrote:
> > > The src_addr_widths and dst_addr_widths capability masks encode each
> > > supported width as a bit whose position equals the corresponding
> > > enum dma_slave_buswidth value (e.g. DMA_SLAVE_BUSWIDTH_4_BYTES sets bit
> > > 4). As these masks are plain u32, widths of 32 bytes and above
> > > (DMA_SLAVE_BUSWIDTH_32/64/128_BYTES map to bits 32, 64 and 128) cannot
> > > be represented at all.
> > > 
> > > Introduce bitmap-based bus width capabilities that span the full enum
> > > range, through a new dma_buswidth_mask_t type modeled after
> > > dma_cap_mask_t. To allow DMA controller drivers to be converted
> > > incrementally, the legacy dma_device u32 fields are kept alongside the
> > > new masks and the core folds a legacy-only driver's u32 into the mask
> > > when the device is registered, so consumers only ever have to look at
> > > the mask.
> > > 
> > > The new interface lives in two new headers under a new
> > > include/linux/dma/engine/ directory instead of growing
> > > linux/dmaengine.h, which is included nearly everywhere:
> > > 
> > >  - dma/engine/types.h holds enum dma_slave_buswidth and the new
> > >    dma_buswidth_mask_t type. Like dma_cap_mask_t, the type only needs
> > >    DECLARE_BITMAP();
> > > 
> > >  - dma/engine/widthmask.h holds the accessors which are based on the new
> > >    dma_buswidth_mask_t type. This gives us freedom to change the core
> > >    without affecting consumers as they only see (and should only use) the
> > >    new type.
> > > 
> > > Note the fold only has to happen in one direction on the producer side:
> > > nothing outside a controller driver reads the legacy dma_device fields,
> > > so a converted driver's mask is not mirrored back into them. The legacy
> > > dma_slave_caps fields are different, as consumers not converted yet
> > > still read them: dma_get_slave_caps() derives them from the mask when a
> > > device_caps() callback adjusted it. Both go away with the legacy fields.
> > 
> > ...
> > 
> > >  F:	drivers/dma/
> > >  F:	include/dt-bindings/dma/
> > >  F:	include/linux/dma/
> > > +F:	include/linux/dma/engine/
> > >  F:	include/linux/dmaengine.h
> > >  F:	include/linux/of_dma.h
> > 
> > Unneeded, previous entry includes recursively.
> > 
> 
> Checkpatch complains so I guess we assume it's a tool issue?
> 
> > ...
> > 
> > > +/*
> > > + * Basic types shared by the DMA engine interfaces.
> > > + */
> > > +#ifndef LINUX_DMA_ENGINE_TYPES_H
> > > +#define LINUX_DMA_ENGINE_TYPES_H
> > > +
> > > +#include <linux/bitops.h>
> > 
> > Not yet? Perhaps next changes will use it, then they can add it.
> 
> Should be dropped!

In fact, I'll keep the above! DECLARE_BITMAP() needs BITS_TO_LONGS().
Yeah, not great...

- Nuno Sá

> 
> > 
> > > +#include <linux/types.h>
> > > +
> > > +/**
> > > + * enum dma_slave_buswidth - defines bus width of the DMA slave
> > > + * device, source or target buses
> > 
> > I would describe the _UNDEFINED case, it might require some clarification on
> > what behaviour is to expect with this one.
> 
> Yeah, here I can of just copy pasted what we had. Also not completely
> sure what's the undefined case is about. I can do some search though.
> 
> > 
> > > + */
> > 
> > > +enum dma_slave_buswidth {
> > > +	DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
> > > +	DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
> > > +	DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
> > > +	DMA_SLAVE_BUSWIDTH_3_BYTES = 3,
> > > +	DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
> > > +	DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
> > > +	DMA_SLAVE_BUSWIDTH_16_BYTES = 16,
> > > +	DMA_SLAVE_BUSWIDTH_32_BYTES = 32,
> > > +	DMA_SLAVE_BUSWIDTH_64_BYTES = 64,
> > > +	DMA_SLAVE_BUSWIDTH_128_BYTES = 128,
> > > +	DMA_SLAVE_BUSWIDTH_MAX
> > > +};
> > 
> > > +/**
> > > + * typedef dma_buswidth_mask_t - bus width capabilities bitmap modeled after
> > > + * dma_cap_mask_t.
> > > + *
> > > + * Each supported bus width is represented by the bit whose position equals the
> > > + * corresponding enum dma_slave_buswidth value, e.g. a device supporting a bus
> > > + * width of 4 bytes has bit 4 set.
> > > + */
> > > +typedef struct {
> > > +	DECLARE_BITMAP(bits, DMA_SLAVE_BUSWIDTH_MAX);
> > > +} dma_buswidth_mask_t;
> > > +
> > > +#endif /* LINUX_DMA_ENGINE_TYPES_H */
> > 
> > ...
> > 
> > > +/*
> > > + * Bus width capabilities of DMA engine devices and channels.
> > > + */
> > > +#ifndef LINUX_DMA_ENGINE_WIDTHMASK_H
> > > +#define LINUX_DMA_ENGINE_WIDTHMASK_H
> > > +
> > > +#include <linux/bitmap.h>
> > > +#include <linux/dma/engine/types.h>
> > 
> > + errno.h
> 
> ack
> 
> > 
> > > +#include <linux/types.h>
> > 
> > I would group subsystem ones.
> > 
> > #include <linux/bitmap.h>
> > #include <linux/errno.h>
> > #include <linux/types.h>
> > 
> > #include <linux/dma/engine/types.h>
> 
> Can do that, yes.
> 
> > 
> > ...
> > 
> > > +static inline enum dma_slave_buswidth
> > > +__dma_bus_width_min(const dma_buswidth_mask_t *mask)
> > > +{
> > > +	enum dma_slave_buswidth width = find_first_bit(mask->bits,
> > > +						       DMA_SLAVE_BUSWIDTH_MAX);
> > > +
> > 
> > For easier maintenance better to split the assignment.
> > 
> > 	enum dma_slave_buswidth width;
> > 
> > 	width = find_first_bit(mask->bits, DMA_SLAVE_BUSWIDTH_MAX);
> > 
> 
> No strong feelings so sure.
> 
> > > +	if (width == DMA_SLAVE_BUSWIDTH_MAX)
> > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > +
> > > +	return width;
> > > +}
> > 
> > ...
> > 
> > >  #ifndef LINUX_DMAENGINE_H
> > >  #define LINUX_DMAENGINE_H
> > >  
> > > +#include <linux/bitops.h>
> > >  #include <linux/device.h>
> > 
> > > +#include <linux/dma/engine/types.h>
> > 
> > Same, group them after generic linux/*.h.
> > 
> > >  #include <linux/err.h>
> > >  #include <linux/uio.h>
> > >  #include <linux/bug.h>
> > 
> > ...
> > 
> > It's possible to split this patch to two:
> > - move the existing type into a new types.h header
> > - add support for the new API
> 
> Yeah I thought about that but this still looked simple enough to go
> together but I can split in the next version.
> 
> - Nuno Sá
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >