Re: [PATCH v4 5/6] spi: spi-fsl-dspi: Increase DMA buffer size

[James Clark]

On 01/07/2025 3:47 pm, Vladimir Oltean wrote:
> On Fri, Jun 27, 2025 at 11:21:41AM +0100, James Clark wrote:
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index e7856f9c9440..46d3cae9efed 100644
> > --- a/drivers/spi/spi-fsl-dspi.c
> > +++ b/drivers/spi/spi-fsl-dspi.c
> > @@ -493,6 +493,39 @@ static u32 dspi_pop_tx_pushr(struct fsl_dspi *dspi)
> >   	return cmd << 16 | data;
> >   }
> >   
> > +static int dspi_dma_bufsize(struct fsl_dspi *dspi)
> > +{
> > +	if (spi_controller_is_target(dspi->ctlr)) {
> > +		/*
> > +		 * In target mode we have to be ready to receive the maximum
> > +		 * that can possibly be transferred at once by EDMA without any
> > +		 * FIFO underflows. This is CITER * SSIZE, where SSIZE is a max
> > +		 * of 4 when transferring to a peripheral.
> > +		 */
> > +		return GENMASK(14, 0) * DMA_SLAVE_BUSWIDTH_4_BYTES;
>
> Is this really a constant that can be hardcoded? Should this be queried
> from the EDMA driver somehow?
>
> I'm not well versed in the dmaengine/dma-mapping API at all, but I see
> fsl_edma_probe() makes a call to dma_set_max_seg_size(), which consumer
> drivers such as DSPI can query using dma_get_max_seg_size(). To the
> untrained eye, and from a great distance, it looks like the value you're
> interested in. Apologies if that isn't the case.

You're probably right, and there's no particular reason to hard code it 
if it can be queried. I'll have a look at this.

> > +	}
> > +
> > +	return PAGE_SIZE;
> > +}
>
> The other question is: what's fundamentally different between the host
> and target operating modes, such that we return different values? Why
> not the same?

This is missing from the commit message, but the reason is because it's 
a large allocation (256K with both tx and rx buffers) that should be 
avoided unless absolutely necessary so we wanted to limit it to only 
target devices.

The other reason to not allocate it dynamically based on the size of the 
message is because we assumed that it was better to do large contiguous 
allocations at boot time. If it's delayed until the device is used then 
the allocation might fail due to memory fragmentation.