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

[Alexander Kochetkov <al.kochet@xxxxxxxxx>]

Hi, Maxime! Thanks for reviewing patches!


>> 
>> +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? 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.


>> 
>> @@ -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?

Alexander.