[PATCH] spi: Update speed/mode on change

[Patrick DELAUNAY]

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 ?

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


>   	*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().

>   err:
>   	log_debug("%s: Error path, created=%d, device '%s'\n", __func__,
>   		  created, dev->name);

Regards

Patrick