Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

Hi,

On Mon, Oct 19, 2020 at 04:17:18PM +0300, Alexander Kochetkov wrote:
> >> +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
> >> +				 struct spi_transfer *tfr)
> >> +{
> >> +	struct dma_async_tx_descriptor *rxdesc, *txdesc;
> >> +	struct spi_master *master = sspi->master;
> >> +
> >> +	rxdesc = NULL;
> >> +	if (tfr->rx_buf) {
> >> +		struct dma_slave_config rxconf = {
> >> +			.direction = DMA_DEV_TO_MEM,
> >> +			.src_addr = sspi->dma_addr_rx,
> >> +			.src_addr_width = 1,
> >> +			.src_maxburst = 1,
> >> +		};
> > 
> > That doesn't really look optimal, the controller seems to be able to
> > read / write 32 bits at a time from its FIFO and we probably can
> > increase the burst length too?
> 
> 
> I had doubts if it would work. I didn’t know will DMA work for
> transfers with lengths not aligned to 32 bits. For example, if we init
> DMA with src_addr_width = 1 and .src_maxburst = 8 will DMA work for
> transfer with length 11?

Bursts are usually defined by how many transfers the controller is
allowed to do at once, so it shouldn't cause any harm if the length
isn't aligned, it will just do less than the maximum number allowed.

Whether or not the hardware agrees to that definition is something else
though, but from experience it should

> I expect that DMA fill FIFO with 16 bytes and
> SPI transfer only 11 bytes and 5 bytes will leave in TX fifo. I did
> the test and there is no additional data left in the fifo buffer. Also
> at reception the is no memory overwrites.
> 
> I made test with src_addr_width = 4, src_maxburst = 1 and transfer
> length 3. Looks like in that case DMA doesn’t issue 4 bytes transfer.
> 
> For test with src_addr_width = 4, src_maxburst = 8 I had to adjust
> RF_RDY_TRIG_LEVEL_BITS and TF_ERQ_TRIG_LEVEL_BITS of FIFO_CTL_REG to
> half of FIFO (32 bytes). With the config DMA will transfer burst of
> half of FIFO length during transfer and remaining bytes at the end of
> transfer.

Yeah, that might need some tuning. With the width, I guess we should pay
attention to the order the bytes are sent in, but it can be done later.

> >> 
> >> @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
> >> 	/* Transfer complete */
> >> 	if (status & SUN6I_INT_CTL_TC) {
> >> 		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
> >> -		sun6i_spi_drain_fifo(sspi);
> >> +		if (!sspi->use_dma)
> >> +			sun6i_spi_drain_fifo(sspi);
> > 
> > Is it causing any issue? The FIFO will be drained only if there's
> > something remaining in the FIFO, which shouldn't happen with DMA?
> > 
> 
> No. It’s for make code patch explicit.
> Remove the change?

Yes, that also simplifies the driver since we don't have to rely on the
boolean in the main structure anymore

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]