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

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

[Patrick Delaunay]

This serie generalize the commit 96907c0fe50a
("dm: spi: Read default speed and mode values from DT")

In case of DT boot, don't read default speed and mode for SPI from
CONFIG_*, instead read from DT node. This will make sure that boards
with multiple SPI/QSPI controllers can be probed at different
bus frequencies and SPI modes.

Today it is only done in the command sf; this patch do the same
for the other user of the spi_flash_probe(): spl and splash
to avoid probe issue.


Changes in v2:
- use variables to avoid duplicated code
- use variables to avoid duplicated code

Patrick Delaunay (2):
  spl_spi: Read default speed and mode values from DT
  splash: sf: Read default speed and mode values from DT

 common/spl/spl_spi.c   | 10 ++++++++--
 common/splash_source.c | 12 ++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.7.4

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

[Patrick Delaunay]

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@st.com>
Signed-off-by: Patrick Delaunay <patrick.delaunay@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;
 
+#ifdef CONFIG_DM_SPI_FLASH
+	/* In DM mode defaults will be taken from DT */
+	max_hz = 0, spi_mode = 0;
+#endif
 	/*
 	 * 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] [PATCH v2 2/2] splash: sf: Read default speed and mode values from DT

[Patrick Delaunay]

In case of DT boot, don't read default speed and mode for SPI from
CONFIG_*, instead read from DT node.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

Changes in v2:
- use variables to avoid duplicated code

 common/splash_source.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/common/splash_source.c b/common/splash_source.c
index 62763b9..427196c 100644
--- a/common/splash_source.c
+++ b/common/splash_source.c
@@ -24,11 +24,19 @@ DECLARE_GLOBAL_DATA_PTR;
 static struct spi_flash *sf;
 static int splash_sf_read_raw(u32 bmp_load_addr, int offset, size_t read_size)
 {
+	unsigned int max_hz = CONFIG_SF_DEFAULT_SPEED;
+	unsigned int spi_mode = CONFIG_SF_DEFAULT_MODE;
+
+#ifdef CONFIG_DM_SPI_FLASH
+	/* In DM mode defaults will be taken from DT */
+	max_hz = 0, spi_mode = 0;
+#endif
+
 	if (!sf) {
 		sf = spi_flash_probe(CONFIG_SF_DEFAULT_BUS,
 				     CONFIG_SF_DEFAULT_CS,
-				     CONFIG_SF_DEFAULT_SPEED,
-				     CONFIG_SF_DEFAULT_MODE);
+				     max_hz,
+				     spi_mode);
 		if (!sf)
 			return -ENODEV;
 	}
-- 
2.7.4

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

[Simon Goldschmidt]

On 19.11.2018 18:33, Patrick Delaunay 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@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>

I was commenting about code duplication, which is better now, so:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

I do think it would be nice to invert the logic. That way, we could get 
completely rid of those two CONFIG_SF_DEFAULT settings for DM_SPI_FLASH 
boards (and eventually for all boards - when's the deadline for that?). 
But there are other places that still do it like you do here, so it's 
probably better to change them all at once...

Simon

> ---
>
> 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;
>   
> +#ifdef CONFIG_DM_SPI_FLASH
> +	/* In DM mode defaults will be taken from DT */
> +	max_hz = 0, spi_mode = 0;
> +#endif
>   	/*
>   	 * 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;

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

[Patrick DELAUNAY]

Hi Simon,

> -----Original Message-----
> From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Sent: lundi 19 novembre 2018 20:09
> 
> On 19.11.2018 18:33, Patrick Delaunay 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@st.com>
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> 
> I was commenting about code duplication, which is better now, so:
> 
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> 
> I do think it would be nice to invert the logic. That way, we could get completely
> rid of those two CONFIG_SF_DEFAULT settings for DM_SPI_FLASH boards (and
> eventually for all boards - when's the deadline for that?).
> But there are other places that still do it like you do here, so it's probably better
> to change them all at once...

Agree with you.

In fact I hesitate with directly change the header file 

#ifdef CONFIG_DM_SPI_FLASH
	/* In DM mode defaults will be taken from DT */ 
	#define CONFIG_SF_DEFAULT_SPEED 0
	#define CONFIG_SF_DEFAULT_MODE 0
#endif

But I am afraid of the potential impacts (define is used in many boards),
but I think it sould be more cleaner way to force the expected behavior.

Patrick

> Simon
> 

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

[Simon Goldschmidt]

On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY
<patrick.delaunay@st.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > Sent: lundi 19 novembre 2018 20:09
> >
> > On 19.11.2018 18:33, Patrick Delaunay 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@st.com>
> > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> >
> > I was commenting about code duplication, which is better now, so:
> >
> > Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >
> > I do think it would be nice to invert the logic. That way, we could get completely
> > rid of those two CONFIG_SF_DEFAULT settings for DM_SPI_FLASH boards (and
> > eventually for all boards - when's the deadline for that?).
> > But there are other places that still do it like you do here, so it's probably better
> > to change them all at once...
>
> Agree with you.
>
> In fact I hesitate with directly change the header file
>
> #ifdef CONFIG_DM_SPI_FLASH
>         /* In DM mode defaults will be taken from DT */
>         #define CONFIG_SF_DEFAULT_SPEED 0
>         #define CONFIG_SF_DEFAULT_MODE 0
> #endif

This looks correct to me.

> But I am afraid of the potential impacts (define is used in many boards),
> but I think it sould be more cleaner way to force the expected behavior.

Can't we just push this and fix whatever it breaks?

Regards,
Simon

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

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Sent: mardi 27 novembre 2018 09:11

> 
> On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY
> <patrick.delaunay@st.com> wrote:
> >
> > Hi Simon,
> >
> > > -----Original Message-----
> > > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > Sent: lundi 19 novembre 2018 20:09
> > >
> > > On 19.11.2018 18:33, Patrick Delaunay 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@st.com>
> > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > >
> > > I was commenting about code duplication, which is better now, so:
> > >
> > > Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > >
> > > I do think it would be nice to invert the logic. That way, we could
> > > get completely rid of those two CONFIG_SF_DEFAULT settings for
> > > DM_SPI_FLASH boards (and eventually for all boards - when's the deadline
> for that?).
> > > But there are other places that still do it like you do here, so
> > > it's probably better to change them all at once...
> >
> > Agree with you.
> >
> > In fact I hesitate with directly change the header file
> >
> > #ifdef CONFIG_DM_SPI_FLASH
> >         /* In DM mode defaults will be taken from DT */
> >         #define CONFIG_SF_DEFAULT_SPEED 0
> >         #define CONFIG_SF_DEFAULT_MODE 0 #endif
> 
> This looks correct to me.
> 
> > But I am afraid of the potential impacts (define is used in many
> > boards), but I think it sould be more cleaner way to force the expected
> behavior.
> 
> Can't we just push this and fix whatever it breaks?

Yes I can push it, what is the correct patch strategy.

1/ accept this patch (for short term solution) and propose a update after (I can do it next week).

2/ sent a v3 of this patchset (it can make more time to solve the potential issue)

> 
> Regards,
> Simon

Reagrds
Patrick

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

[Simon Goldschmidt]

On Tue, Nov 27, 2018 at 1:33 PM Patrick DELAUNAY
<patrick.delaunay@st.com> wrote:
>
> Hi Simon,
>
> > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > Sent: mardi 27 novembre 2018 09:11
>
> >
> > On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY
> > <patrick.delaunay@st.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > > -----Original Message-----
> > > > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > > Sent: lundi 19 novembre 2018 20:09
> > > >
> > > > On 19.11.2018 18:33, Patrick Delaunay 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@st.com>
> > > > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > > >
> > > > I was commenting about code duplication, which is better now, so:
> > > >
> > > > Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > >
> > > > I do think it would be nice to invert the logic. That way, we could
> > > > get completely rid of those two CONFIG_SF_DEFAULT settings for
> > > > DM_SPI_FLASH boards (and eventually for all boards - when's the deadline
> > for that?).
> > > > But there are other places that still do it like you do here, so
> > > > it's probably better to change them all at once...
> > >
> > > Agree with you.
> > >
> > > In fact I hesitate with directly change the header file
> > >
> > > #ifdef CONFIG_DM_SPI_FLASH
> > >         /* In DM mode defaults will be taken from DT */
> > >         #define CONFIG_SF_DEFAULT_SPEED 0
> > >         #define CONFIG_SF_DEFAULT_MODE 0 #endif
> >
> > This looks correct to me.
> >
> > > But I am afraid of the potential impacts (define is used in many
> > > boards), but I think it sould be more cleaner way to force the expected
> > behavior.
> >
> > Can't we just push this and fix whatever it breaks?
>
> Yes I can push it, what is the correct patch strategy.
>
> 1/ accept this patch (for short term solution) and propose a update after (I can do it next week).

I'm totally OK with 1, yes. That's why I've already sent my
reviewed-by. But I would really appreciate a later update, this got me
confused already... ;-)

Regards,
Simon

>
> 2/ sent a v3 of this patchset (it can make more time to solve the potential issue)
>
> >
> > Regards,
> > Simon
>
> Reagrds
> Patrick

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

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Sent: mardi 27 novembre 2018 13:55
> Subject: Re: [PATCH v2 1/2] spl_spi: Read default speed and mode values from DT
> 
> On Tue, Nov 27, 2018 at 1:33 PM Patrick DELAUNAY
> <patrick.delaunay@st.com> wrote:
> >
> > Hi Simon,
> >
> > > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > Sent: mardi 27 novembre 2018 09:11
> >
> > >
> > > On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY
> > > <patrick.delaunay@st.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > > -----Original Message-----
> > > > > From: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > > > Sent: lundi 19 novembre 2018 20:09
> > > > >
> > > > > On 19.11.2018 18:33, Patrick Delaunay wrote:
> > > > >
> > > > > I do think it would be nice to invert the logic. That way, we
> > > > > could get completely rid of those two CONFIG_SF_DEFAULT settings
> > > > > for DM_SPI_FLASH boards (and eventually for all boards - when's
> > > > > the deadline
> > > for that?).
> > > > > But there are other places that still do it like you do here, so
> > > > > it's probably better to change them all at once...
> > > >
> > > > Agree with you.
> > > >
> > > > In fact I hesitate with directly change the header file
> > > >
> > > > #ifdef CONFIG_DM_SPI_FLASH
> > > >         /* In DM mode defaults will be taken from DT */
> > > >         #define CONFIG_SF_DEFAULT_SPEED 0
> > > >         #define CONFIG_SF_DEFAULT_MODE 0 #endif
> > >
> > > This looks correct to me.
> > >
> > > > But I am afraid of the potential impacts (define is used in many
> > > > boards), but I think it sould be more cleaner way to force the
> > > > expected
> > > behavior.
> > >
> > > Can't we just push this and fix whatever it breaks?
> >
> > Yes I can push it, what is the correct patch strategy.
> >
> > 1/ accept this patch (for short term solution) and propose a update after (I can
> do it next week).
> 
> I'm totally OK with 1, yes. That's why I've already sent my reviewed-by. But I
> would really appreciate a later update, this got me confused already... ;-)

For information I send the new serie today named  
"[PATCH 0/7] Remove defines for SPI default speed and mode"

http://patchwork.ozlabs.org/project/uboot/list/?series=80769 

Finally I propose to remove the 2 defines when the SPI DM migration will be completed.

> Regards,
> Simon

Regards Patrick.

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

[Adam Ford]

On Mon, Nov 19, 2018 at 12:01 PM Patrick Delaunay
<patrick.delaunay@st.com> wrote:
>
>
> This serie generalize the commit 96907c0fe50a
> ("dm: spi: Read default speed and mode values from DT")
>
> In case of DT boot, don't read default speed and mode for SPI from
> CONFIG_*, instead read from DT node. This will make sure that boards
> with multiple SPI/QSPI controllers can be probed at different
> bus frequencies and SPI modes.
>
> Today it is only done in the command sf; this patch do the same
> for the other user of the spi_flash_probe(): spl and splash
> to avoid probe issue.
>
>
> Changes in v2:
> - use variables to avoid duplicated code
> - use variables to avoid duplicated code
>
> Patrick Delaunay (2):
>   spl_spi: Read default speed and mode values from DT
>   splash: sf: Read default speed and mode values from DT
>
>  common/spl/spl_spi.c   | 10 ++++++++--
>  common/splash_source.c | 12 ++++++++++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
>

I replied to another thread, but to prevent a hasty decision, I
thought I'd mention that this patch series breaks the da850evm.

U-Boot SPL 2019.01-rc1-00747-g1a3453efed (Dec 10 2018 - 09:07:08 -0600)
Trying to boot from SPI
Warning: SPI speed fallback to 100 kHz

(And repeat the above forever)

I know Jagan just re-did some of the da850 SPI driver and I wonder if
the updated driver just needs mode and speed entries added to platdata
then extracted from platdata when the driver is initialized.

adam
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Adam Ford]

On Mon, Nov 19, 2018 at 11:55 AM Patrick Delaunay
<patrick.delaunay@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@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@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

>         /*
>          * 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

[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@gmail.com>
> Sent: lundi 10 décembre 2018 23:48
> 
> On Mon, Nov 19, 2018 at 11:55 AM Patrick Delaunay
> <patrick.delaunay@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@st.com>
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@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

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

[Patrick DELAUNAY]

Hi Adam,

> -----Original Message-----
> From: Adam Ford <aford173@gmail.com>
> Sent: lundi 10 décembre 2018 16:50
> 
> On Mon, Nov 19, 2018 at 12:01 PM Patrick Delaunay
> <patrick.delaunay@st.com> wrote:
> >
> >
> > This serie generalize the commit 96907c0fe50a
> > ("dm: spi: Read default speed and mode values from DT")
> >
> > In case of DT boot, don't read default speed and mode for SPI from
> > CONFIG_*, instead read from DT node. This will make sure that boards
> > with multiple SPI/QSPI controllers can be probed at different bus
> > frequencies and SPI modes.
> >
> > Today it is only done in the command sf; this patch do the same for
> > the other user of the spi_flash_probe(): spl and splash to avoid probe
> > issue.
> >
> >
> > Changes in v2:
> > - use variables to avoid duplicated code
> > - use variables to avoid duplicated code
> >
> > Patrick Delaunay (2):
> >   spl_spi: Read default speed and mode values from DT
> >   splash: sf: Read default speed and mode values from DT
> >
> >  common/spl/spl_spi.c   | 10 ++++++++--
> >  common/splash_source.c | 12 ++++++++++--
> >  2 files changed, 18 insertions(+), 4 deletions(-)
> >
> 
> I replied to another thread, but to prevent a hasty decision, I thought I'd
> mention that this patch series breaks the da850evm.
> 
> U-Boot SPL 2019.01-rc1-00747-g1a3453efed (Dec 10 2018 - 09:07:08 -0600)
> Trying to boot from SPI
> Warning: SPI speed fallback to 100 kHz
> 
> (And repeat the above forever)
> 
> I know Jagan just re-did some of the da850 SPI driver and I wonder if the
> updated driver just needs mode and speed entries added to platdata then
> extracted from platdata when the driver is initialized.
> 
> adam

Thanks for the tests, I agree the serie can't be merged in this state.

I will come back only in 3 weeks....
and I will check this point when I will come back.
That should avoid many issue as already solved for the 2 series 

http://patchwork.ozlabs.org/patch/1010363/
http://patchwork.ozlabs.org/patch/995030/

That should be avoid revert in the next daysas :
http://git.denx.de/?p=u-boot.git;a=commit;h=25a17652c9c2cc4d88098121fc6a2b8f90020531


NB: for short term solution without patch, 
       for platform with DT only flash description (as my platform stm32mp1), 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).

> > --
> > 2.7.4
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot