[U-Boot] [PATCH v2 1/2] spl_spi: Read default speed and mode values from DT

[Patrick DELAUNAY]

Hi Adam,

Thanks for the tests, 
And sorry for the regression on your board.

> From: Adam Ford <aford173 at gmail.com>
> Sent: lundi 10 décembre 2018 23:48
> 
> On Mon, Nov 19, 2018 at 11:55 AM Patrick Delaunay
> <patrick.delaunay at st.com> wrote:
> >
> > In case of DT boot, don't read default speed and mode for SPI from
> > CONFIG_*, instead read from DT node.
> >
> > Signed-off-by: Christophe Kerello <christophe.kerello at st.com>
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > Changes in v2:
> > - use variables to avoid duplicated code
> >
> >  common/spl/spl_spi.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c index
> > 8cd4830..b348b45 100644
> > --- a/common/spl/spl_spi.c
> > +++ b/common/spl/spl_spi.c
> > @@ -74,15 +74,21 @@ static int spl_spi_load_image(struct spl_image_info
> *spl_image,
> >         unsigned payload_offs = CONFIG_SYS_SPI_U_BOOT_OFFS;
> >         struct spi_flash *flash;
> >         struct image_header *header;
> > +       unsigned int max_hz = CONFIG_SF_DEFAULT_SPEED;
> > +       unsigned int spi_mode = CONFIG_SF_DEFAULT_MODE;
> >
> 
> Instead of
> > +#ifdef CONFIG_DM_SPI_FLASH
> 
> What about using:
> 
> #if CONFIG_IS_ENABLED(OF_CONTROL) &&
> !CONFIG_IS_ENABLED(OF_PLATDATA)
> 
> > +       /* In DM mode defaults will be taken from DT */
> > +       max_hz = 0, spi_mode = 0;
> > +#endif
> 
> This one-line change 'should' give you what you want, the settings of
> 0 for boards using the device tree.  If board that uses OF_PLATDATA cannot load
> the device tree, it'll default back to the configs set above.  You could probably
> combine all if statements into one, and I'd be fine with that too.
> 
> This one-line change made my board boot with this series.  I haven't applied the
> follow-up series yet but if a v3 was posted with this change, I'd mark it as
> 'tested-by.
> 
> adam

I am not familiar with platdata (my platform use only device tree).
So I take some time to check and answer.

I clearly no anticipate the issue for OF_PLATDATA, mainly caused by code in drivers/spi/spi-uclass.c

In function spi_get_bus_and_cs
The parameter speed expect 0 to load parameter from device tree / with parent platdata

	if (!speed) {
		speed = plat->max_hz;
		mode = plat->mode;
	}
=> it should the default behavior when parent is correctly initialized for ALL the user (SPL/ENV/Commands).

But for some case, when driver is automatically created (no device tree node or platform data)

Speed is ALSO used as max_hz entry for platdata.
It is a bad idea when speed = 0 

Simon already make a patch to avoid the issue

=> "dm: spi: prevent setting a speed of 0 Hz"
SHA1 = 12bfb2e05fc29bfbec7eb76ea8cc02e130268801

But in your case , it is not enough .....
I don't known why,

I try to understood how the platdata for parent (SPI_UCLASS) can be configurated...
I need to go deeper.

A solution can to change the code introduce by Simon :

+		if (speed) {
+			plat->max_hz = speed;
+		} else {
+			printf("Warning: SPI speed fallback to %u kHz\n",
+			       SPI_DEFAULT_SPEED_HZ / 1000);
+			plat->max_hz = SPI_DEFAULT_SPEED_HZ;
+		}

some update can work in your case

	if (speed) {
		plat->max_hz = speed; 
	} else {
#ifdef CONFIG_SF_DEFAULT_SPEED
		plat->max_hz = CONFIG_SF_DEFAULT_SPEED; 
#else
		printf("Warning: SPI speed fallback to %u kHz\n",
			       SPI_DEFAULT_SPEED_HZ / 1000);
			plat->max_hz = SPI_DEFAULT_SPEED_HZ;
#endif
	}
	If (mode)
		plat->mode = mode;
	else
#ifdef CONFIG_SF_DEFAULT_MODE
		plat->mode = CONFIG_SF_DEFAULT_MODE
#else
		plat->mode = SPI_MODE_3
#endif

In thise case the CONFIG_DEFAULT value are defined, they are used....

So the 2 series are not enough mature today, we need some work.
But I start my holiday today, I will come back only in 3 weeks....

I will check this point when I will come back.

NB: for short term solution, for plaform with DT only flash descrition (as my platform), we can use

CONFIG_SF_DEFAULT_SPEED=0
CONFIG_SF_DEFAULT_MODE=0

That should be solved the current issue in my board without need to merge the 2 series (not yet tested).


> >         /*
> >          * Load U-Boot image from SPI flash into RAM
> >          */
> >
> >         flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS,
> >                                 CONFIG_SF_DEFAULT_CS,
> > -                               CONFIG_SF_DEFAULT_SPEED,
> > -                               CONFIG_SF_DEFAULT_MODE);
> > +                               max_hz,
> > +                               spi_mode);
> >         if (!flash) {
> >                 puts("SPI probe failed.\n");
> >                 return -ENODEV;
> > --
> > 2.7.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot

Regards 

Patrick