Re: [PATCH v5 1/2] spi: dw: Add 32 bpw support to DW DMA Controller

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 06:09:16PM +0300, Serge Semin wrote:
> On Tue, Apr 11, 2023 at 05:46:34PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 11, 2023 at 05:11:15PM +0300, Serge Semin wrote:
> > > On Tue, Apr 11, 2023 at 03:13:49PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Mar 30, 2023 at 06:34:49AM +0000, Joy Chakraborty wrote:

...

> > > > > -	if (n_bytes == 1)
> > > > > +	switch (n_bytes) {
> > > > > +	case 1:
> > > > >  		return DMA_SLAVE_BUSWIDTH_1_BYTE;
> > > > > -	else if (n_bytes == 2)
> > > > > +	case 2:
> > > > >  		return DMA_SLAVE_BUSWIDTH_2_BYTES;
> > > > > -
> > > > > -	return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > 
> > > > > +	case 3:
> > > > 
> > > > I'm not sure about this.
> > > 
> > > This actually makes sense seeing the function argument can have values
> > > 1, 2, _3_ and 4:
> > > dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
> > > transfer->bits_per_word = __F__(master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32));
> > > ...
> > > dw_spi_dma_convert_width(dws->n_bytes)
> > > 
> > > The spi_transfer.bits_per_word field value depends on the
> > > SPI peripheral device communication protocol requirements which may
> > > imply the 3-bytes word xfers (even though it's indeed unluckily).
> > > 
> > > This semantic will also match to what we currently have in the
> > > IRQ-based SPI-transfer implementation (see dw_writer() and
> > > dw_reader()).
> 
> > Nice, but we have DMA_SLAVE_BUSWIDTH_3_BYTES definition for that. Why we don't
> > use it?
> 
> We could but there are two more-or-less firm reasons not to do
> that:
> 1. There aren't that much DMA-engines with the
> DMA_SLAVE_BUSWIDTH_3_BYTES capability meanwhile the DW APB SSI just
> ignores the upper bits if CTRLR0.DFS is less than the value actual
> written to the DR registers. Note DW DMAC engine isn't one of such
> controllers. So if we get to meet a peripheral SPI-device with 3-bytes
> word protocol transfers and the DMA-engine doesn't support it the
> DMA-based transfers may fail (depending on the DMA-engine driver
> implementation).
> 2. The DW APB SSIs (3.x and 4.x) can be synthesized with the APB Data
> Bus Width of 8, 16 and 32. So no matter whether DMA-engine supports
> the 3-bytes bus width the system bus most likely will either convert
> the transfers to the proper sized bus-transactions or fail.
> 
> So taking all of the above into account not using the
> DMA_SLAVE_BUSWIDTH_3_BYTES macro here seems better than using it with
> a risk to fail some of the platform setups especially seeing the DW
> APB SSI ignores the upper bits anyway.

But this is not about SPI host hardware, it's about the consumers.
They should know about supported sizes. Either we add the corresponding support
to the driver or remove 3 case as I suggested. I don't think it's correct to
use 3 as 4.

> > > > > +	case 4:
> > > > > +		return DMA_SLAVE_BUSWIDTH_4_BYTES;
> > > > > +	default:
> > > > > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > > > > +	}

-- 
With Best Regards,
Andy Shevchenko