[PATCH] spi: dw: Fix dynamic speed change

[PATCH] spi: dw: Fix dynamic speed change

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

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.

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 */
+	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;
-- 
1.7.9.5

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

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

[Andy Shevchenko]

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()?



> 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;


-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

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

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

[Mark Brown]

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

On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:

> 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

Why is the behaviour of spidev relevant here, if there is a problem with
spidev why is it being addressed in a specific driver?

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

In what way does the test "not work"?  What should the test be doing and
what is it actually doing?

> +	/* Always calculate the desired clock divider */
> +	speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz;

Please avoid using the ternery operator standalone, write a normal if to
help people read the code.  Though in this case the core should always
ensure that there is a speed set on each transfer so I'm not clear when
this test would ever use anything other than transfer->speed_hz anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Vlastimil Šetka]

5.11.2014 11:54, Mark Brown:
> On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>
>> 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
> Why is the behaviour of spidev relevant here, if there is a problem with
> spidev why is it being addressed in a specific driver?
You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case 
you can declare speed_hz per transfer, and everything is OK in spi-dw.

If you do not declare speed_hz (default is 0) when using 
SPI_IOC_MESSAGE, or when you do just read/write on /dev/spidevX.Y, the 
spi_transfer->speed_hz is filled by spi->max_speed_hz (which is the 
value previously set by SPI_IOC_WR_MAX_SPEED_HZ). This triggers a 
problem in spi-dw.

>> 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.
> In what way does the test "not work"?  What should the test be doing and
> what is it actually doing?

Test:
- set SPI clock to 100 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl
- write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero 
speed_hz)
- set SPI clock to 200 000 Hz by SPI_IOC_WR_MAX_SPEED_HZ ioctl
- write something to /dev/spidevX.Y (or use SPI_IOC_MESSAGE with zero 
speed_hz)

In last step, the clock should be 200 000 Hz, but actually it's 100 000 Hz.

The reason is buggy condition which encloses chip->clk_div recalculation:

      if (transfer->speed_hz) {
          speed = chip->speed_hz;
          if (transfer->speed_hz != speed) {

because transfer->speed_hz is filled by spi->max_speed_hz, which is 
equal to chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in 
dw_spi_setup):

     chip->speed_hz = spi->max_speed_hz = new_spi_speed

>
>> +	/* Always calculate the desired clock divider */
>> +	speed = transfer->speed_hz ? transfer->speed_hz : chip->speed_hz;
> Please avoid using the ternery operator standalone, write a normal if to
> help people read the code.  Though in this case the core should always
> ensure that there is a speed set on each transfer so I'm not clear when
> this test would ever use anything other than transfer->speed_hz anyway.
Yes, as I can catch from code, the transfer->speed_hz should never be 0. 
But in current spi-dw.c version there is a `if (transfer->speed_hz)` 
condition, so I kept it in the patch.

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

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

[Mark Brown]

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

On Wed, Nov 05, 2014 at 01:07:36PM +0100, Vlastimil Šetka wrote:
> 5.11.2014 11:54, Mark Brown:
> >On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:

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

> >Why is the behaviour of spidev relevant here, if there is a problem with
> >spidev why is it being addressed in a specific driver?

> You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case you
> can declare speed_hz per transfer, and everything is OK in spi-dw.

You can declare a speed per transfer for *any* client, this is totally
generic behaviour.  What is the specific relevance of spidev here?

> If you do not declare speed_hz (default is 0) when using SPI_IOC_MESSAGE, or
> when you do just read/write on /dev/spidevX.Y, the spi_transfer->speed_hz is
> filled by spi->max_speed_hz (which is the value previously set by
> SPI_IOC_WR_MAX_SPEED_HZ). This triggers a problem in spi-dw.

Again, this is something that any client could do...

> The reason is buggy condition which encloses chip->clk_div recalculation:

>      if (transfer->speed_hz) {
>          speed = chip->speed_hz;
>          if (transfer->speed_hz != speed) {

> because transfer->speed_hz is filled by spi->max_speed_hz, which is equal to
> chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in dw_spi_setup):

To repeat again: please talk about driver problems in terms of the
driver not in terms of a particular driver.  Glancing at the code here
it looks like spidev is buggy here, it's just blindly allowing userspace
to overwrite the maximum speed configured for the device which seems
like a bad idea, drivers should have no reason to expect that something
called max_speed is actually variable.  It looks like spidev is abusing
this as a default speed.

>     chip->speed_hz = spi->max_speed_hz = new_spi_speed

So the driver is overriding its idea of the current speed without
actually updatng the hardware.  Why is the fix here not to just delete
the assignment here, it seems fairly obviously buggy?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Thor Thayer]

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

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

[Thor Thayer]

On 11/05/2014 07:24 AM, Mark Brown wrote:
> On Wed, Nov 05, 2014 at 01:07:36PM +0100, Vlastimil Šetka wrote:
>> 5.11.2014 11:54, Mark Brown:
>>> On Tue, Nov 04, 2014 at 04:36:56PM -0600, tthayer-yzvPICuk2ABMcg4IHK0kFvd9D2ou9A/h@public.gmane.org.com wrote:
>>>> 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
>>> Why is the behaviour of spidev relevant here, if there is a problem with
>>> spidev why is it being addressed in a specific driver?
>> You can do a transfer at SPI using SPI_IOC_MESSAGE ioctl. In this case you
>> can declare speed_hz per transfer, and everything is OK in spi-dw.
> You can declare a speed per transfer for *any* client, this is totally
> generic behaviour.  What is the specific relevance of spidev here?
spidev was just the vehicle that the issue was observed on and is useful 
for debugging.
>> If you do not declare speed_hz (default is 0) when using SPI_IOC_MESSAGE, or
>> when you do just read/write on /dev/spidevX.Y, the spi_transfer->speed_hz is
>> filled by spi->max_speed_hz (which is the value previously set by
>> SPI_IOC_WR_MAX_SPEED_HZ). This triggers a problem in spi-dw.
> Again, this is something that any client could do...
>
>> The reason is buggy condition which encloses chip->clk_div recalculation:
>>       if (transfer->speed_hz) {
>>           speed = chip->speed_hz;
>>           if (transfer->speed_hz != speed) {
>> because transfer->speed_hz is filled by spi->max_speed_hz, which is equal to
>> chip->speed_hz -- because SPI_IOC_WR_MAX_SPEED_HZ do this (in dw_spi_setup):
> To repeat again: please talk about driver problems in terms of the
> driver not in terms of a particular driver.  Glancing at the code here
> it looks like spidev is buggy here, it's just blindly allowing userspace
> to overwrite the maximum speed configured for the device which seems
> like a bad idea, drivers should have no reason to expect that something
> called max_speed is actually variable.  It looks like spidev is abusing
> this as a default speed.
spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in 
spidev_ioctl() to overwrite the maximum speed. From the code and the 
name, it seems like overwriting the maximum speed was the intended use 
and not a side effect.
>>      chip->speed_hz = spi->max_speed_hz = new_spi_speed
> So the driver is overriding its idea of the current speed without
> actually updatng the hardware.  Why is the fix here not to just delete
> the assignment here, it seems fairly obviously buggy?
Yes, removing line 591 of the spi-dw.c (chip->speed_hz = 
spi->max_speed_hz;) will solve the problem and seems like a clean fix. 
In this case the chip->speed_hz will persist the last transfer speed 
setting.

The chip->speed_hz parameter won't be updated as part of the spi-dw 
driver initialization. This may not matter since it will get updated on 
the first transfer in the (transfer->speed != speed) test shown above.
--
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

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

[Mark Brown]

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

On Wed, Nov 05, 2014 at 02:25:59PM -0600, Thor Thayer wrote:

Please leave blank lines between paragraphs and quoted bits, it makes
things much easier to read than an unbroken wall of text covering many
screenfuls.

> On 11/05/2014 07:24 AM, Mark Brown wrote:

> >To repeat again: please talk about driver problems in terms of the
> >driver not in terms of a particular driver.  Glancing at the code here
> >it looks like spidev is buggy here, it's just blindly allowing userspace
> >to overwrite the maximum speed configured for the device which seems
> >like a bad idea, drivers should have no reason to expect that something
> >called max_speed is actually variable.  It looks like spidev is abusing
> >this as a default speed.

> spidev is calling the case condition SPI_IOC_WR_MAX_SPEED_HZ in
> spidev_ioctl() to overwrite the maximum speed. From the code and the name,
> it seems like overwriting the maximum speed was the intended use and not a
> side effect.

This means it was an API that was implemented thoughtlessly rather than
anything else; it should be fixed to cache and use the current speed
after one has been set.

> >>     chip->speed_hz = spi->max_speed_hz = new_spi_speed
> >So the driver is overriding its idea of the current speed without
> >actually updatng the hardware.  Why is the fix here not to just delete
> >the assignment here, it seems fairly obviously buggy?

> Yes, removing line 591 of the spi-dw.c (chip->speed_hz = spi->max_speed_hz;)
> will solve the problem and seems like a clean fix. In this case the
> chip->speed_hz will persist the last transfer speed setting.

> The chip->speed_hz parameter won't be updated as part of the spi-dw driver
> initialization. This may not matter since it will get updated on the first
> transfer in the (transfer->speed != speed) test shown above.

OK, great - can you submit that version please?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

An IOCTL call that calls spi_setup() and then dw_spi_setup() will
overwrite the persisted last transfer speed. On each transfer, the
SPI speed is compared to the last transfer speed to determine if the
clock divider registers need to be updated (did the speed change?).
This bug was observed with the spidev driver using spi-config to
update the max transfer speed.

This fix: Don't overwrite the persisted last transaction clock speed
when updating the SPI parameters in dw_spi_setup(). On the next
transaction, the new speed won't match the persisted last speed
and the hardware registers will be updated.
On initialization, the persisted last transaction clock
speed will be 0 but will be updated after the first SPI
transaction.

Move zeroed clock divider check into clock change test because
chip->clk_div is zero on startup and would cause a divide-by-zero
error. The calculation was wrong as well (can't support odd #).

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>
---
v2: Simplify fix so that last persistent SPI transfer speed is not
overwritten.
---
 drivers/spi/spi-dw.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 72e12ba..d0d5542 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;
@@ -419,7 +416,7 @@ static void pump_transfers(unsigned long data)
 	if (transfer->speed_hz) {
 		speed = chip->speed_hz;
 
-		if (transfer->speed_hz != speed) {
+		if ((transfer->speed_hz != speed) || (!chip->clk_div)) {
 			speed = transfer->speed_hz;
 
 			/* clk_div doesn't support odd number */
@@ -581,7 +578,6 @@ static int dw_spi_setup(struct spi_device *spi)
 		dev_err(&spi->dev, "No max speed HZ parameter\n");
 		return -EINVAL;
 	}
-	chip->speed_hz = spi->max_speed_hz;
 
 	chip->tmode = 0; /* Tx & Rx */
 	/* Default SPI mode is SCPOL = 0, SCPH = 0 */
-- 
1.7.9.5

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