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

[Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>]

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