Message ID | f89702e6f2b012551921ce959de7a918bc1c3978.1510500657.git.baruch@tkos.co.il |
---|---|
State | Rejected |
Delegated to: | Jagannadha Sutradharudu Teki |
Headers | show |
Series | [U-Boot,v2] spi: kirkwood: avoid divide by zero crash | expand |
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 <baruch@tkos.co.il> > --- > 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; >
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 <baruch@tkos.co.il> > > --- > > 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
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;
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 <baruch@tkos.co.il> --- v2: Don't fail; set lowest rate (Jagan) --- drivers/spi/kirkwood_spi.c | 3 +++ 1 file changed, 3 insertions(+)