[U-Boot,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 <baruch@tkos.co.il>
---
v2: Don't fail; set lowest rate (Jagan)
---
 drivers/spi/kirkwood_spi.c | 3 +++
 1 file changed, 3 insertions(+)

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