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

Discussion:

Patrick Delaunay

2018-11-19 17:33:09 UTC

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 <***@st.com>
Signed-off-by: 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

Patrick Delaunay

2018-11-19 17:33:10 UTC

Permalink

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

Simon Goldschmidt

2018-11-19 19:08:40 UTC

Permalink

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

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

Reviewed-by: Simon 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

Post by Patrick Delaunay
---
- 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;

Patrick DELAUNAY

2018-11-27 08:06:36 UTC

Permalink

Hi Simon,

-----Original Message-----
Sent: lundi 19 novembre 2018 20:09

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

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

Simon Goldschmidt

2018-11-27 08:11:22 UTC

Permalink

On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY

Post by Patrick DELAUNAY
Hi Simon,

-----Original Message-----
Sent: lundi 19 novembre 2018 20:09

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

This looks correct to me.

Post by Patrick DELAUNAY
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

Patrick DELAUNAY

2018-11-27 12:33:06 UTC

Permalink

Hi Simon,

Sent: mardi 27 novembre 2018 09:11
On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY

Post by Patrick DELAUNAY
Hi Simon,

-----Original Message-----
Sent: lundi 19 novembre 2018 20:09

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

for that?).

Post by Patrick DELAUNAY

But there are other places that still do it like you do here, so
it's probably better to change them all at once...

This looks correct to me.

Post by Patrick DELAUNAY
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

Simon Goldschmidt

2018-11-27 12:54:55 UTC

Permalink

On Tue, Nov 27, 2018 at 1:33 PM Patrick DELAUNAY

Post by Patrick DELAUNAY
Hi Simon,

Sent: mardi 27 novembre 2018 09:11
On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY

Post by Patrick DELAUNAY
Hi Simon,

-----Original Message-----
Sent: lundi 19 novembre 2018 20:09

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

for that?).

Post by Patrick DELAUNAY

But there are other places that still do it like you do here, so
it's probably better to change them all at once...

This looks correct to me.

Post by Patrick DELAUNAY
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

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

Regards,
Simon

Reagrds
Patrick

Patrick DELAUNAY

2018-12-10 11:01:59 UTC

Permalink

Hi Simon,

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

Post by Patrick DELAUNAY
Hi Simon,

Sent: mardi 27 novembre 2018 09:11
On Tue, Nov 27, 2018 at 9:06 AM Patrick DELAUNAY

Post by Patrick DELAUNAY
Hi Simon,

-----Original Message-----
Sent: lundi 19 novembre 2018 20:09
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?).

Post by Patrick DELAUNAY

But there are other places that still do it like you do here, so
it's probably better to change them all at once...

This looks correct to me.

Post by Patrick DELAUNAY
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.

Adam Ford

2018-12-10 15:50:08 UTC

Permalink

On Mon, Nov 19, 2018 at 12:01 PM 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.
- use variables to avoid duplicated code
- use variables to avoid duplicated code
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
https://lists.denx.de/listinfo/u-boot

Adam Ford

2018-12-10 22:48:02 UTC

Permalink

On Mon, Nov 19, 2018 at 11:55 AM Patrick Delaunay

Post by Patrick Delaunay
In case of DT boot, don't read default speed and mode for SPI from
CONFIG_*, instead read from DT node.
---
- 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

Post by Patrick Delaunay
+#ifdef CONFIG_DM_SPI_FLASH

What about using:

#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)

Post by Patrick Delaunay
+ /* 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

Post by Patrick Delaunay
/*
* 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
https://lists.denx.de/listinfo/u-boot