[PATCH] spi: Update speed/mode on change

[Marek Vasut]

On 2/26/21 2:07 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
> On 2/25/21 9:52 PM, Marek Vasut wrote:
>> The spi_get_bus_and_cs() may be called on the same bus and chipselect
>> with different frequency or mode. This is valid usecase, but the code
>> fails to notify the controller of such a configuration change. Call
>> spi_set_speed_mode() in case bus frequency or bus mode changed to let
>> the controller update the configuration.
>>
>> The problem can easily be triggered using the sspi command:
>> => sspi 0:0 at 1000
>> => sspi 0:0 at 2000
>> Without this patch, both transfers happen at 1000 Hz. With this patch,
>> the later transfer happens correctly at 2000 Hz.
>>
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> Cc: Jagan Teki <jagan at amarulasolutions.com>
>> Cc: Patrick Delaunay <patrick.delaunay at st.com>
>> ---
>>   drivers/spi/spi-uclass.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
>> index 7155d4aebd6..96c9a83e761 100644
>> --- a/drivers/spi/spi-uclass.c
>> +++ b/drivers/spi/spi-uclass.c
>> @@ -405,12 +405,22 @@ int spi_get_bus_and_cs(int busnum, int cs, int 
>> speed, int mode,
>>               goto err;
>>       }
>> +    /* In case bus frequency or mode changed, update it. */
>> +    if ((speed && slave->speed && slave->speed != speed) ||
>> +        (plat->mode != mode)) {
>> +        ret = spi_set_speed_mode(bus, speed, mode);
>> +        if (ret)
>> +            goto err_speed_mode;
>> +    }
>> +
> 
> We compare bus (with lastest configured value ) or slave (value 
> requested) here ?

Argh, this should in fact be bus_data->speed , it seems I sent 
un-rebased patches.

> in dm_spi_claim_bus() it is compared with bus :
> 
> spi->speed and spi->mode and spi = dev_get_uclass_priv(bus);
> 
> +    /* In case bus frequency or mode changed, update it. */
> +    if ((speed && bus->speed != speed) || (bus->mode != mode)) {
> +        ret = spi_set_speed_mode(bus, speed, mode);
> +        if (ret)
> +            goto err_speed_mode;
> +        bus->speed = speed;
> +        bus->mode = mode;
> +    }
> 
> NB: bus->speed can't be 0 here after previous spi_claim_bus()
> 
> PS: the update of spi->speed and spi->mode can be done in 
> spi_set_speed_mode() function

I don't think you want to do this every time you call dm_spi_claim_bus()

>>       *busp = bus;
>>       *devp = slave;
>>       log_debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp);
>>       return 0;
>> +err_speed_mode:
>> +    spi_claim_bus(slave);
> 
> 
> here I think it is:
> 
> spi_release_bus(slave);
> 
> called after previous spi_claim_bus().

Right