Re: [U-Boot] [PATCH v2] spi: kirkwood: avoid divide by zero crash
On Mon, Nov 13, 2017 at 06:26 AM, Baruch Siach wrote: >On Sun, Nov 12, 2017 at 08:42:28PM +0100, Heinrich Schuchardt wrote: >> On 11/12/2017 04:30 PM, Baruch Siach wrote: >> > Calling .set_speed with zero speed is definitely a bug. Instead of >> > crashing, set hz to 1 so that we get the lowest possible frequency rate. >> >> Did this actually occur? Why? > > Yes. CONFIG_ARCH_MVEBU selects CONFIG_DM_SPI_FLASH. In > cmd/sf.c:do_spi_flash_probe(), > speed is set to 0 when CONFIG_DM_SPI_FLASH, passed to > spi_flash_probe_bus_cs(), > and from there to spi_get_bus_and_cs(). > Unfortunately, since the ClearFog SPI flash node has no "spi-flash" compatible > string, the spi_flash_std driver is not bound, so spi_child_post_bind() > returns > early without calling spi_slave_ofdata_to_platdata(). The > dm_spi_slave_platdata > speed field is left uninitialized, and we end up with > speed=0 when calling spi_set_speed_mode(). > > This should be fixed with http://patchwork.ozlabs.org/patch/837360/ for > ClearFog, > by adding the "spi-flash" compatible. But "spi-flash" is non standard. Most > kernel > DT files use "jedec,spi-nor" instead. So you can expect this issue to show up > again in the future. This is exactly what I just got with the cadence-spi driver when starting my mach-socfpga board with the device tree working for linux. I also 'fixed' it by changing the dts, but I'd rather use the same dts for linux and U-Boot. Should this be fixed in the core code or is it a bug of each driver not detecting the '0'? > A workaround is to provide speed argument in the 'sf probe' command line. This is not a valid workaround for me: the spi-nor is my boot device and I got this error even in SPL when loading U-Boot. Not using 'sf probe'. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: kirkwood: avoid divide by zero crash
Hi Heinrich,
On Sun, Nov 12, 2017 at 08:42:28PM +0100, Heinrich Schuchardt wrote:
> On 11/12/2017 04:30 PM, Baruch Siach wrote:
> > Calling .set_speed with zero speed is definitely a bug. Instead of crashing,
> > set hz to 1 so that we get the lowest possible frequency rate.
>
> Did this actually occur? Why?
Yes. CONFIG_ARCH_MVEBU selects CONFIG_DM_SPI_FLASH. In
cmd/sf.c:do_spi_flash_probe(), speed is set to 0 when CONFIG_DM_SPI_FLASH,
passed to spi_flash_probe_bus_cs(), and from there to spi_get_bus_and_cs().
Unfortunately, since the ClearFog SPI flash node has no "spi-flash" compatible
string, the spi_flash_std driver is not bound, so spi_child_post_bind()
returns early without calling spi_slave_ofdata_to_platdata(). The
dm_spi_slave_platdata speed field is left uninitialized, and we end up with
speed=0 when calling spi_set_speed_mode().
This should be fixed with http://patchwork.ozlabs.org/patch/837360/ for
ClearFog, by adding the "spi-flash" compatible. But "spi-flash" is non
standard. Most kernel DT files use "jedec,spi-nor" instead. So you can expect
this issue to show up again in the future.
A workaround is to provide speed argument in the 'sf probe' command line.
> > Signed-off-by: Baruch Siach
> > ---
> > v2: Don't fail; set lowest rate (Jagan)
> > ---
> > drivers/spi/kirkwood_spi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
> > index 0c6bd295cde9..5744849a5ad9 100644
> > --- a/drivers/spi/kirkwood_spi.c
> > +++ b/drivers/spi/kirkwood_spi.c
> > @@ -257,6 +257,9 @@ static int mvebu_spi_set_speed(struct udevice *bus,
> > uint hz)
> > struct kwspi_registers *reg = plat->spireg;
> > u32 data;
> > + /* Avoid divide by zero; instead, set the lowest possible speed */
> > + if (hz == 0)
> > + hz = 1;
>
> Shouldn't we write a debug message here like in other drivers?
>
> debug("%s: can't set 0 Hz (actual will be 1 Hz)\n", __func__);
I'm fine with that. The actual speed is platform specific, though. See below.
> Does any bus member support 1 Hz? Wouldn't 125 kHz be a more reasonable
> value?
The code below would write the highest possible prescaler value,
KWSPI_CLKPRESCL_MASK, thus setting the lowest possible per-platform frequency
value.
> Could a division by zero valued max_hz occur in spi_setup_slave too?
I don't know. The prescaler calculation code is duplicated over there, so it
might be a good idea to refactor.
> > /* calculate spi clock prescaller using max_hz */
> > data = ((CONFIG_SYS_TCLK / 2) / hz) + 0x10;
> > data = data < KWSPI_CLKPRESCL_MIN ? KWSPI_CLKPRESCL_MIN : data;
Thanks for reviewing,
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}ooO--U--Ooo{=
- bar...@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: kirkwood: avoid divide by zero crash
On 11/12/2017 04:30 PM, Baruch Siach wrote:
Calling .set_speed with zero speed is definitely a bug. Instead of crashing,
set hz to 1 so that we get the lowest possible frequency rate.
Did this actually occur? Why?
Signed-off-by: Baruch Siach
---
v2: Don't fail; set lowest rate (Jagan)
---
drivers/spi/kirkwood_spi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
index 0c6bd295cde9..5744849a5ad9 100644
--- a/drivers/spi/kirkwood_spi.c
+++ b/drivers/spi/kirkwood_spi.c
@@ -257,6 +257,9 @@ static int mvebu_spi_set_speed(struct udevice *bus, uint hz)
struct kwspi_registers *reg = plat->spireg;
u32 data;
+ /* Avoid divide by zero; instead, set the lowest possible speed */
+ if (hz == 0)
+ hz = 1;
Shouldn't we write a debug message here like in other drivers?
debug("%s: can't set 0 Hz (actual will be 1 Hz)\n", __func__);
Does any bus member support 1 Hz? Wouldn't 125 kHz be a more reasonable
value?
Could a division by zero valued max_hz occur in spi_setup_slave too?
Best regards
Heinrich
/* calculate spi clock prescaller using max_hz */
data = ((CONFIG_SYS_TCLK / 2) / hz) + 0x10;
data = data < KWSPI_CLKPRESCL_MIN ? KWSPI_CLKPRESCL_MIN : data;
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] spi: kirkwood: avoid divide by zero crash
Calling .set_speed with zero speed is definitely a bug. Instead of crashing, set hz to 1 so that we get the lowest possible frequency rate. Signed-off-by: Baruch Siach --- v2: Don't fail; set lowest rate (Jagan) --- drivers/spi/kirkwood_spi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c index 0c6bd295cde9..5744849a5ad9 100644 --- a/drivers/spi/kirkwood_spi.c +++ b/drivers/spi/kirkwood_spi.c @@ -257,6 +257,9 @@ static int mvebu_spi_set_speed(struct udevice *bus, uint hz) struct kwspi_registers *reg = plat->spireg; u32 data; + /* Avoid divide by zero; instead, set the lowest possible speed */ + if (hz == 0) + hz = 1; /* calculate spi clock prescaller using max_hz */ data = ((CONFIG_SYS_TCLK / 2) / hz) + 0x10; data = data < KWSPI_CLKPRESCL_MIN ? KWSPI_CLKPRESCL_MIN : data; -- 2.15.0 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
