Re: [PATCH] spi: fsl_spi: Don't change speed while chipselect is active

[Herve Codina]

Hi Christophe,

On Wed, 14 Dec 2022 14:41:33 +0100
Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote:

> Commit c9bfcb315104 ("spi_mpc83xx: much improved driver") made
> modifications to the driver to not perform speed changes while
> chipselect is active. But those changes where lost with the
> convertion to tranfer_one.
> 
> Previous implementation was allowing speed changes during
> message transfer when cs_change flag was set.
> At the time being, core SPI does not provide any feature to change
> speed while chipselect is off, so do not allow any speed change during
> message transfer, and perform the transfer setup in prepare_message
> in order to set correct speed while chipselect is still off.
> 
> Reported-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> Fixes: 64ca1a034f00 ("spi: fsl_spi: Convert to transfer_one")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> Tested-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> ---
>  drivers/spi/spi-fsl-spi.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
> index 731624f157fc..93152144fd2e 100644
> --- a/drivers/spi/spi-fsl-spi.c
> +++ b/drivers/spi/spi-fsl-spi.c
> @@ -333,13 +333,26 @@ static int fsl_spi_prepare_message(struct spi_controller *ctlr,
>  {
>  	struct mpc8xxx_spi *mpc8xxx_spi = spi_controller_get_devdata(ctlr);
>  	struct spi_transfer *t;
> +	struct spi_transfer *first;
> +
> +	first = list_first_entry(&m->transfers, struct spi_transfer,
> +				 transfer_list);
>  
>  	/*
>  	 * In CPU mode, optimize large byte transfers to use larger
>  	 * bits_per_word values to reduce number of interrupts taken.
> +	 *
> +	 * Some glitches can appear on the SPI clock when the mode changes.
> +	 * Check that there is no speed change during the transfer and set it up
> +	 * now to change the mode without having a chip-select asserted.
>  	 */
> -	if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
> -		list_for_each_entry(t, &m->transfers, transfer_list) {
> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		if (t->speed_hz != first->speed_hz) {
> +			dev_err(&m->spi->dev,
> +				"speed_hz cannot change during message.\n");
> +			return -EINVAL;
> +		}
> +		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
>  			if (t->len < 256 || t->bits_per_word != 8)
>  				continue;
>  			if ((t->len & 3) == 0)
> @@ -348,7 +361,7 @@ static int fsl_spi_prepare_message(struct spi_controller *ctlr,
>  				t->bits_per_word = 16;
>  		}
>  	}
> -	return 0;
> +	return fsl_spi_setup_transfer(m->spi, first);
>  }
>  
>  static int fsl_spi_transfer_one(struct spi_controller *controller,

Looks good to me.

Reviewed-by: Herve Codina <herve.codina@xxxxxxxxxxx>

Regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com