* [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs
@ 2021-01-20 20:28 Brandon Maier
2021-02-12 14:44 ` Michal Simek
2021-02-23 13:56 ` Michal Simek
0 siblings, 2 replies; 3+ messages in thread
From: Brandon Maier @ 2021-01-20 20:28 UTC (permalink / raw)
To: u-boot
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 at amarulasolutions.com
CC: michal.simek at 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(®s->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, ®s->confr);
- zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
- debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
+ confr = readl(®s->confr);
+ confr &= ~GQSPI_BAUD_DIV_MASK;
+ confr |= (baud_rate_val << 3);
+ writel(confr, ®s->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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs
2021-01-20 20:28 [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs Brandon Maier
@ 2021-02-12 14:44 ` Michal Simek
2021-02-23 13:56 ` Michal Simek
1 sibling, 0 replies; 3+ messages in thread
From: Michal Simek @ 2021-02-12 14:44 UTC (permalink / raw)
To: u-boot
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 at amarulasolutions.com
> CC: michal.simek at 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(®s->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, ®s->confr);
>
> - zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> - debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> + confr = readl(®s->confr);
> + confr &= ~GQSPI_BAUD_DIV_MASK;
> + confr |= (baud_rate_val << 3);
> + writel(confr, ®s->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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs
2021-01-20 20:28 [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs Brandon Maier
2021-02-12 14:44 ` Michal Simek
@ 2021-02-23 13:56 ` Michal Simek
1 sibling, 0 replies; 3+ messages in thread
From: Michal Simek @ 2021-02-23 13:56 UTC (permalink / raw)
To: u-boot
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 at amarulasolutions.com
> CC: michal.simek at 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(®s->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, ®s->confr);
>
> - zynqmp_qspi_set_tapdelay(bus, baud_rate_val);
> - debug("regs=%p, speed=%d\n", priv->regs, plat->speed_hz);
> + confr = readl(®s->confr);
> + confr &= ~GQSPI_BAUD_DIV_MASK;
> + confr |= (baud_rate_val << 3);
> + writel(confr, ®s->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
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-23 13:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 20:28 [PATCH] spi: zynqmp_gqspi: fix set_speed bug on multiple runs Brandon Maier
2021-02-12 14:44 ` Michal Simek
2021-02-23 13:56 ` Michal Simek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.