spi: zynqmp_gqspi: fix set_speed bug on multiple runs

If zynqmp_qspi_set_speed() is called multiple times with the same speed,
then on the second call it will skip recalculating the baud_rate_val as
it assumes the speed is already configured correctly. But it will still
write the baud_rate_val to the configuration register and call
zynqmp_gqspi_set_tapdelay(). Because it skipped recalculating the
baud_rate_val, it will use the initial value of 0 . This causes the
driver to run at maximum speed which for many spi flashes is too fast and
causes data corruption.

Instead only write out a new baud_rate_val if we have calculated the
correct baud_rate_val.

This opens up another issue with the "if (speed == 0)", we don't save
off the new plat->speed_hz value when setting the baud rate on the
speed=0 path. Instead mimic what the Linux zynqmp gqspi driver does, and
have speed==0 just use the same calculation as a normal speed. That will
cause the baud_rate_val to use the slowest speed possible, which is the
safest option.

Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
CC: jagan@amarulasolutions.com
CC: michal.simek@xilinx.com
CC: Ashok Reddy Soma <ashokred@xilinx.com>
---
 drivers/spi/zynqmp_gqspi.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Hi Ashok

On 1/20/21 9:28 PM, Brandon Maier wrote:
> If zynqmp_qspi_set_speed() is called multiple times with the same speed,
> then on the second call it will skip recalculating the baud_rate_val as
> it assumes the speed is already configured correctly. But it will still
> write the baud_rate_val to the configuration register and call
> zynqmp_gqspi_set_tapdelay(). Because it skipped recalculating the
> baud_rate_val, it will use the initial value of 0 . This causes the
> driver to run at maximum speed which for many spi flashes is too fast and
> causes data corruption.
> 
> Instead only write out a new baud_rate_val if we have calculated the
> correct baud_rate_val.
> 
> This opens up another issue with the "if (speed == 0)", we don't save
> off the new plat->speed_hz value when setting the baud rate on the
> speed=0 path. Instead mimic what the Linux zynqmp gqspi driver does, and
> have speed==0 just use the same calculation as a normal speed. That will
> cause the baud_rate_val to use the slowest speed possible, which is the
> safest option.
> 
> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
> CC: jagan@amarulasolutions.com
> CC: michal.simek@xilinx.com
> CC: Ashok Reddy Soma <ashokred@xilinx.com>
> ---
>  drivers/spi/zynqmp_gqspi.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/zynqmp_gqspi.c b/drivers/spi/zynqmp_gqspi.c
> index c7db43a09a..6641c2e9d5 100644
> --- a/drivers/spi/zynqmp_gqspi.c
> +++ b/drivers/spi/zynqmp_gqspi.c
> @@ -320,12 +320,9 @@ static int zynqmp_qspi_set_speed(struct udevice *bus, uint speed)
>  	if (speed > plat->frequency)
>  		speed = plat->frequency;
>  
> -	/* Set the clock frequency */
> -	confr = readl(&regs->confr);
> -	if (speed == 0) {
> -		/* Set baudrate x8, if the freq is 0 */
> -		baud_rate_val = GQSPI_DFLT_BAUD_RATE_VAL;
> -	} else if (plat->speed_hz != speed) {
> +	if (plat->speed_hz != speed) {
> +		/* Set the clock frequency */
> +		/* If speed == 0, default to lowest speed */
>  		while ((baud_rate_val < 8) &&
>  		       ((plat->frequency /
>  		       (2 << baud_rate_val)) > speed))
> @@ -335,13 +332,15 @@ static int zynqmp_qspi_set_speed(struct udevice *bus, uint speed)
>  			baud_rate_val = GQSPI_DFLT_BAUD_RATE_VAL;
>  
>  		plat->speed_hz = plat->frequency / (2 << baud_rate_val);
> -	}
> -	confr &= ~GQSPI_BAUD_DIV_MASK;
> -	confr |= (baud_rate_val << 3);
> -	writel(confr, &regs->confr);
>  
> -	zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> -	debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> +		confr = readl(&regs->confr);
> +		confr &= ~GQSPI_BAUD_DIV_MASK;
> +		confr |= (baud_rate_val << 3);
> +		writel(confr, &regs->confr);
> +		zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> +
> +		debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> +	}
>  
>  	return 0;
>  }
> 

Ashok: Can you please review this patch?

Thanks,
Michal

st 20. 1. 2021 v 21:29 odesílatel Brandon Maier
<brandon.maier@rockwellcollins.com> napsal:
>
> If zynqmp_qspi_set_speed() is called multiple times with the same speed,
> then on the second call it will skip recalculating the baud_rate_val as
> it assumes the speed is already configured correctly. But it will still
> write the baud_rate_val to the configuration register and call
> zynqmp_gqspi_set_tapdelay(). Because it skipped recalculating the
> baud_rate_val, it will use the initial value of 0 . This causes the
> driver to run at maximum speed which for many spi flashes is too fast and
> causes data corruption.
>
> Instead only write out a new baud_rate_val if we have calculated the
> correct baud_rate_val.
>
> This opens up another issue with the "if (speed == 0)", we don't save
> off the new plat->speed_hz value when setting the baud rate on the
> speed=0 path. Instead mimic what the Linux zynqmp gqspi driver does, and
> have speed==0 just use the same calculation as a normal speed. That will
> cause the baud_rate_val to use the slowest speed possible, which is the
> safest option.
>
> Signed-off-by: Brandon Maier <brandon.maier@rockwellcollins.com>
> CC: jagan@amarulasolutions.com
> CC: michal.simek@xilinx.com
> CC: Ashok Reddy Soma <ashokred@xilinx.com>
> ---
>  drivers/spi/zynqmp_gqspi.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/spi/zynqmp_gqspi.c b/drivers/spi/zynqmp_gqspi.c
> index c7db43a09a..6641c2e9d5 100644
> --- a/drivers/spi/zynqmp_gqspi.c
> +++ b/drivers/spi/zynqmp_gqspi.c
> @@ -320,12 +320,9 @@ static int zynqmp_qspi_set_speed(struct udevice *bus, uint speed)
>         if (speed > plat->frequency)
>                 speed = plat->frequency;
>
> -       /* Set the clock frequency */
> -       confr = readl(&regs->confr);
> -       if (speed == 0) {
> -               /* Set baudrate x8, if the freq is 0 */
> -               baud_rate_val = GQSPI_DFLT_BAUD_RATE_VAL;
> -       } else if (plat->speed_hz != speed) {
> +       if (plat->speed_hz != speed) {
> +               /* Set the clock frequency */
> +               /* If speed == 0, default to lowest speed */
>                 while ((baud_rate_val < 8) &&
>                        ((plat->frequency /
>                        (2 << baud_rate_val)) > speed))
> @@ -335,13 +332,15 @@ static int zynqmp_qspi_set_speed(struct udevice *bus, uint speed)
>                         baud_rate_val = GQSPI_DFLT_BAUD_RATE_VAL;
>
>                 plat->speed_hz = plat->frequency / (2 << baud_rate_val);
> -       }
> -       confr &= ~GQSPI_BAUD_DIV_MASK;
> -       confr |= (baud_rate_val << 3);
> -       writel(confr, &regs->confr);
>
> -       zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> -       debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> +               confr = readl(&regs->confr);
> +               confr &= ~GQSPI_BAUD_DIV_MASK;
> +               confr |= (baud_rate_val << 3);
> +               writel(confr, &regs->confr);
> +               zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> +
> +               debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> +       }
>
>         return 0;
>  }
> --
> 2.29.1
>

Applied.
Ashok: Please fix it on the top of this one if there is something wrong.

Thanks,
Michal