[PATCH v2 1/2] drm/tinydrm: Remove chunk splitting in tinydrm_spi_transfer

[Noralf Trønnes]

Den 25.02.2018 18.03, skrev Lukas Wunner:
> On Sat, Feb 24, 2018 at 06:16:46PM +0000, Meghana Madhyastha wrote:
>> -Remove chunk splitting in tinydrm_spi_transfer in tinydrm-helpers as The spi core will split a buffer into max_dma_len chunks for the
>>   spi controller driver to handle.
>> -Remove automatic byte swapping in tinydrm_spi_transfer as it doesn't have users.
>> -Remove the upper bound check on dma transfer length in bcm2835_spi_can_dma().
> AFAICS you need to reverse the order of the two patches, so that the
> splitting is added to spi-bcm2835 before you remove it from the DRM
> drivers.  Otherwise there's no splitting at all in-between the patches,
> which may cause issues for someone hitting this patch when bisecting.
>
> It would be good if you could explain the rationale of the patch
> (the "why") in more detail.  You've provided the rationale in the
> cover letter but the cover letter isn't recorded in the git history.
> Right now the commit message only summarizes the "what".

Yeah, the why is important.

Currently clients using spi-bcm2835 need to know about the max_dma_len
limit to ensure DMA transfers. Trying to transfer larger buffers will
result in a warning and a slow PIO transfer.
tinydrm works around this by splitting the framebuffers before transfer
(a legacy from fbtft which tinydrm grew out of).

So the change is about removing the upper bound on DMA SPI transfers for
this particular spi controller driver.

(When I checked this a year ago, there was one more controller driver with
an upper bound on DMA transfers, but I don't remember which)

Noralf.


> Also, please wrap the commit message at 72 chars.
>
>
>> --- a/drivers/spi/spi-bcm2835.c
>> +++ b/drivers/spi/spi-bcm2835.c
>> @@ -365,19 +365,6 @@ static bool bcm2835_spi_can_dma(struct spi_master *master,
>>   	if (tfr->len < BCM2835_SPI_DMA_MIN_LENGTH)
>>   		return false;
>>   
>> -	/* BCM2835_SPI_DLEN has defined a max transfer size as
>> -	 * 16 bit, so max is 65535
>> -	 * we can revisit this by using an alternative transfer
>> -	 * method - ideally this would get done without any more
>> -	 * interaction...
>> -	 */
>> -	if (tfr->len > 65535) {
>> -		dev_warn_once(&spi->dev,
>> -			      "transfer size of %d too big for dma-transfer\n",
>> -			      tfr->len);
>> -		return false;
>> -	}
>> -
>>   	/* if we run rx/tx_buf with word aligned addresses then we are OK */
>>   	if ((((size_t)tfr->rx_buf & 3) == 0) &&
>>   	    (((size_t)tfr->tx_buf & 3) == 0))
> Move this hunk to the other patch so that it's together with the
> changes that remove the 65535 limitation.  It would probably good
> to retain a portion of the comment to explain why the splitting is
> necessary.
>
>
>> @@ -461,7 +448,7 @@ static void bcm2835_dma_init(struct spi_master *master, struct device *dev)
>>   
>>   	/* all went well, so set can_dma */
>>   	master->can_dma = bcm2835_spi_can_dma;
>> -	master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
>> +	master->max_dma_len = 65528; /* limitation by BCM2835_SPI_DLEN */
> Spurious unexplained change.
>
> Thanks,
>
> Lukas