Re: [PATCH] spi: dw: Fix dynamic speed change

From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
To: Andy Shevchenko
	<andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	<linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	<feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	<setka-3PjVBYxTQDg@public.gmane.org>,
	<tthayer-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH] spi: dw: Fix dynamic speed change
Date: Wed, 5 Nov 2014 14:25:03 -0600	[thread overview]
Message-ID: <545A879F.7020600@opensource.altera.com> (raw)
In-Reply-To: <1415181423.472.15.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>


On 11/05/2014 03:57 AM, Andy Shevchenko wrote:
> On Tue, 2014-11-04 at 16:36 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> Changing SPI transfer speed using a utility such as spi_config with
>> spidev updates chip->speed_hz which is compared to the next transfer
>> speed.
>>
>> When speed_hz is not declared in a SPI transfer, the transfer speed is
>> not updated for the next read/write on /dev/spidevX.Y. The element
>> spi_transfer->speed_hz is filled with spi->max_speed_hz. The test of
>> if (transfer->speed_hz != speed) doesn't work because the chip->speed_hz
>> matches transfer->speed_hz and the clock divider is not updated.
>>
>> This fix: On each transfer update the clock divider, compare to the
>> previous clock divider and update if necessary. This fixes another
>> bug where the clock divider calculation at the top of the
>> pump_transfers() function could be an odd-number.
>>
> My intention is to use SPI core API as much as possible. Thus,
> pump_transfers() I think should be gone in future. Instead of doing an
> additional work can you provide a helper function to set speed_hz and
> call it from pump_transfers()?
>
Hi. I see your point but this patch may change significantly as a result 
of discussion. If some form of clock divider changes are implemented, 
I'm happy to make a helper function.
Thanks for reviewing!
>
>> Reported-by: Vlastimil Setka <setka-3PjVBYxTQDg@public.gmane.org>
>> Signed-off-by: Vlastimil Setka <setka-3PjVBYxTQDg@public.gmane.org>
>> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> ---
>>   drivers/spi/spi-dw.c |   33 ++++++++++++++++++---------------
>>   1 file changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
>> index 72e12ba..3456b34 100644
>> --- a/drivers/spi/spi-dw.c
>> +++ b/drivers/spi/spi-dw.c
>> @@ -376,9 +376,6 @@ static void pump_transfers(unsigned long data)
>>   	chip = dws->cur_chip;
>>   	spi = message->spi;
>>   
>> -	if (unlikely(!chip->clk_div))
>> -		chip->clk_div = dws->max_freq / chip->speed_hz;
>> -
>>   	if (message->state == ERROR_STATE) {
>>   		message->status = -EIO;
>>   		goto early_exit;
>> @@ -415,21 +412,27 @@ static void pump_transfers(unsigned long data)
>>   
>>   	cr0 = chip->cr0;
>>   
>> -	/* Handle per transfer options for bpw and speed */
>> -	if (transfer->speed_hz) {
>> -		speed = chip->speed_hz;
>> +	/* Always calculate the desired clock divider */
>> +	speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz;
>> +
>> +	if (speed > dws->max_freq) {
>> +		dev_err(&spi->dev, "Unsupported SPI freq: %d Hz\n", speed);
>> +		message->status = -EIO;
>> +		goto early_exit;
>> +	}
>> +
>> +	/* clk_div doesn't support odd number */
>> +	clk_div = dws->max_freq / speed;
>> +	clk_div = (clk_div + 1) & 0xfffe;
>>   
>> -		if (transfer->speed_hz != speed) {
>> -			speed = transfer->speed_hz;
>> +	/* Determine if the clock divider changed, if so update chip struct */
> Maybe "…update speed_hz" ?
>
>> +	if (clk_div != chip->clk_div)
>> +		chip->clk_div = clk_div;
>> +	else
>> +		clk_div = 0; /* Prevent register programming below */
>>   
>> -			/* clk_div doesn't support odd number */
>> -			clk_div = dws->max_freq / speed;
>> -			clk_div = (clk_div + 1) & 0xfffe;
>> +	chip->speed_hz = speed;
>>   
>> -			chip->speed_hz = speed;
>> -			chip->clk_div = clk_div;
>> -		}
>> -	}
>>   	if (transfer->bits_per_word) {
>>   		bits = transfer->bits_per_word;
>>   		dws->n_bytes = dws->dma_width = bits >> 3;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html