dm: spi: Read default speed and mode values from DT

Date: Wed, 13 Apr 2016 15:40:53 +0530

From: Vignesh R <vigneshr at ti.com<mailto:[email protected]>>

To: Jagan Teki <jteki at openedev.com<mailto:[email protected]>>, Tom Rini <trini at konsulko.com<mailto:[email protected]>>

Cc u-boot at lists.denx.de<mailto:[email protected]>, Stefan Roese <sr at denx.de<mailto:[email protected]>>

Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode

values from DT

Message-ID: <1460542253-10580-1-git-send-email-vigneshr at ti.com<mailto:[email protected]>>

Content-Type: text/plain

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.

Signed-off-by Vignesh R <vigneshr at ti.com<mailto:[email protected]>>

---

v3: Update commit message to mention SPI mode changes

v2: Initialize speed, mode to 0 instead of -1

 cmd/sf.c                 | 2 ++

 drivers/spi/spi-uclass.c | 8 ++++++--

 2 files changed, 8 insertions(+), 2 deletions(-)

cmd/sf.c +0 -0

index 42862d9d921a..286906c3a151 100644

--- a/cmd/sf.c

+++ b/cmd/sf.c

cmd/sf.c +2 -0

@@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[])
 
 #ifdef CONFIG_DM_SPI_FLASH
 
            struct udevice *new, *bus_dev;
 
            int ret;
 
+          /* In DM mode defaults will be taken from DT */
 
+          speed = 0, mode = 0;
 
 #else
 
            struct spi_flash *new;
 
 #endif

drivers/spi/spi-uclass.c +0 -0

index 5561f36762f9..5fb5630e2981 100644

--- a/drivers/spi/spi-uclass.c

+++ b/drivers/spi/spi-uclass.c

drivers/spi/spi-uclass.c +1 -2

@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int
 
 mode,
 
                                  struct udevice **busp, struct spi_slave **devp)
 
 {
 
            struct udevice *bus, *dev;
 
+          struct dm_spi_slave_platdata *plat;
 
            bool created = false;
 
            int ret;

@@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int
 
 mode,
 
             * SPI flash chip - we will bind to the correct driver.
 
             */
 
            if (ret == -ENODEV && drv_name) {
 
-                         struct dm_spi_slave_platdata *plat;
 
-
 
                           debug("%s: Binding new device '%s', busnum=%d, cs=%d,

driver=%s\n",

                                 __func__, dev_name, busnum, cs, drv_name);

                           ret = device_bind_driver(bus, drv_name, dev_name, &dev);

drivers/spi/spi-uclass.c +4 -0

@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int
 
 mode,
 
                           slave->dev = dev;
 
            }
 
 
 
+          plat = dev_get_parent_platdata(dev);
 
+          if (!speed) {
 
+                         speed = plat->max_hz;
 
+                         mode = plat->mode;

Hi Vignesh,

Quoted 5 lines

Date: Wed, 13 Apr 2016 15:40:53 +0530

From: Vignesh R <vigneshr at ti.com <mailto:[email protected]>>

To: Jagan Teki <jteki at openedev.com <mailto:[email protected]>>, Tom

Rini <trini at konsulko.com <mailto:[email protected]>>

Quoted 1 line

Cc u-boot at lists.denx.de <mailto:[email protected]>, Stefan Roese

<sr at denx.de <mailto:[email protected]>>

Quoted 5 lines

Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode

values from DT

Message-ID: <[email protected]

<mailto:[email protected]>>

cmd/sf.c static int do_spi_flash_probe(int argc, char * const

Quoted 45 lines

Content-Type: text/plain

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.

Signed-off-by Vignesh R <vigneshr at ti.com <mailto:[email protected]>>

---

v3: Update commit message to mention SPI mode changes

v2: Initialize speed, mode to 0 instead of -1

 cmd/sf.c                 | 2 ++

 drivers/spi/spi-uclass.c | 8 ++++++--

 2 files changed, 8 insertions(+), 2 deletions(-)

cmd/sf.c +0 -0

index 42862d9d921a..286906c3a151 100644

--- a/cmd/sf.c

+++ b/cmd/sf.c

cmd/sf.c +0 -0

@@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const

argv[])

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 25 lines

cmd/sf.c +2 -0

 #ifdef CONFIG_DM_SPI_FLASH
 
            struct udevice *new, *bus_dev;
 
            int ret;
 
+          /* In DM mode defaults will be taken from DT */
 
+          speed = 0, mode = 0;
 
 #else
 
            struct spi_flash *new;
 
 #endif

drivers/spi/spi-uclass.c +0 -0

index 5561f36762f9..5fb5630e2981 100644

--- a/drivers/spi/spi-uclass.c

+++ b/drivers/spi/spi-uclass.c

drivers/spi/spi-uclass.c +0 -0

@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int · line 266

Quoted 3 lines

drivers/spi/spi-uclass.c +0 -0

 mode,
 
                                  struct udevice **busp, struct

spi_slave **devp)

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 13 lines

drivers/spi/spi-uclass.c +1 -0

 {
 
            struct udevice *bus, *dev;
 
+          struct dm_spi_slave_platdata *plat;
 
            bool created = false;
 
            int ret;

@@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 13 lines

drivers/spi/spi-uclass.c +0 -2

 mode,
 
             * SPI flash chip - we will bind to the correct driver.
 
             */
 
            if (ret == -ENODEV && drv_name) {
 
-                         struct dm_spi_slave_platdata *plat;
 
-
 
                           debug("%s: Binding new device '%s',

busnum=%d, cs=%d,

Quoted 3 lines

driver=%s\n",

                                 __func__, dev_name, busnum, cs,

drv_name);

Quoted 1 line

ret = device_bind_driver(bus, drv_name,

dev_name, &dev);

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 1 line

drivers/spi/spi-uclass.c +0 -0

@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

Quoted 23 lines

drivers/spi/spi-uclass.c +5 -0

 mode,
 
                           slave->dev = dev;
 
            }
 
 
 
+          plat = dev_get_parent_platdata(dev);
 
+          if (!speed) {
 
+                         speed = plat->max_hz;
 
+                         mode = plat->mode;
 
+          }
 
            ret = spi_set_speed_mode(bus, speed, mode);
 
            if (ret)
 
                           goto err;

Quoted 1 line

--

I just doubt if spi_set_speed_mode() has really made a difference to

the actual transfer.

Seems that if the device is inactive, calling device_probe() would also call

spi_set_speed_mode() and do the data transfer. Even if it?s active, setting

speed and mode for it again would not be necessary.

-----Original Message-----
From: Vignesh R [mailto:vigneshr at ti.com]
Sent: Wednesday, April 20, 2016 6:47 PM
To: Qianyu Gong <[email protected]>; jteki at openedev.com; trini at konsulko.com
Cc: u-boot at lists.denx.de; sr at denx.de; Mingkai Hu <[email protected]>
Subject: Re: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT

Quoted 164 lines · Qianyu Gong

Hi Vignesh,

Quoted 5 lines

Date: Wed, 13 Apr 2016 15:40:53 +0530

From: Vignesh R <vigneshr at ti.com <mailto:[email protected]>>

To: Jagan Teki <jteki at openedev.com <mailto:[email protected]>>, Tom

Rini <trini at konsulko.com <mailto:[email protected]>>

Quoted 1 line

Cc u-boot at lists.denx.de <mailto:[email protected]>, Stefan Roese

<sr at denx.de <mailto:[email protected]>>

Quoted 5 lines

Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode

values from DT

Message-ID: <[email protected]

<mailto:[email protected]>>

Quoted 45 lines

Content-Type: text/plain

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.

Signed-off-by Vignesh R <vigneshr at ti.com <mailto:[email protected]>>

---

v3: Update commit message to mention SPI mode changes

v2: Initialize speed, mode to 0 instead of -1

 cmd/sf.c                 | 2 ++

 drivers/spi/spi-uclass.c | 8 ++++++--

 2 files changed, 8 insertions(+), 2 deletions(-)

cmd/sf.c +0 -0

index 42862d9d921a..286906c3a151 100644

--- a/cmd/sf.c

+++ b/cmd/sf.c

cmd/sf.c +0 -0

@@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char *

Quoted 1 line

const

argv[])

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 25 lines

#ifdef CONFIG_DM_SPI_FLASH

            struct udevice *new, *bus_dev;

            int ret;

cmd/sf.c +2 -0

+          /* In DM mode defaults will be taken from DT */
 
+          speed = 0, mode = 0;
 
 #else
 
            struct spi_flash *new;
 
 #endif

drivers/spi/spi-uclass.c +0 -0

index 5561f36762f9..5fb5630e2981 100644

--- a/drivers/spi/spi-uclass.c

+++ b/drivers/spi/spi-uclass.c

drivers/spi/spi-uclass.c +0 -0

@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int · line 266

Quoted 3 lines

drivers/spi/spi-uclass.c +0 -0

 mode,
 
                                  struct udevice **busp, struct

spi_slave **devp)

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 13 lines

drivers/spi/spi-uclass.c +1 -0

 {
 
            struct udevice *bus, *dev;
 
+          struct dm_spi_slave_platdata *plat;
 
            bool created = false;
 
            int ret;

@@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 13 lines

drivers/spi/spi-uclass.c +0 -2

 mode,
 
             * SPI flash chip - we will bind to the correct driver.
 
             */
 
            if (ret == -ENODEV && drv_name) {
 
-                         struct dm_spi_slave_platdata *plat;
 
-
 
                           debug("%s: Binding new device '%s',

busnum=%d, cs=%d,

Quoted 3 lines

driver=%s\n",

                                 __func__, dev_name, busnum, cs,

drv_name);

Quoted 1 line

ret = device_bind_driver(bus, drv_name,

dev_name, &dev);

drivers/spi/spi-uclass.c int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 1 line

drivers/spi/spi-uclass.c +0 -0

@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

Quoted 23 lines

drivers/spi/spi-uclass.c +5 -0

 mode,
 
                           slave->dev = dev;
 
            }
 
 
 
+          plat = dev_get_parent_platdata(dev);
 
+          if (!speed) {
 
+                         speed = plat->max_hz;
 
+                         mode = plat->mode;
 
+          }
 
            ret = spi_set_speed_mode(bus, speed, mode);
 
            if (ret)
 
                           goto err;

Quoted 8 lines

Quoted 1 line

--

I just doubt if spi_set_speed_mode() has really made a difference to

the actual transfer.

It does (see below)...

Quoted 7 lines

Seems that if the device is inactive, calling device_probe() would also call

spi_set_speed_mode() and do the data transfer. Even if it's active, setting

speed and mode for it again would not be necessary.

Yes, spi_set_speed_mode() is called from spi_flash_probe_slave()->spi_claim_bus() as part of device_probe(). spi_claim_bus() in spi-uclass.c speed & mode are appropriately passed based on DT data to spi_set_speed_mode(). But that's not the issue.

But in spi_get_bus_and_cs() (called from sf probe) there is a call to spi_set_speed_mode() after device_probe() for inactive devices. This call is to

_override_ the speed set via DT with those passed as cmd line args of sf probe. But, if no args are passed to sf probe, speed and mode default to CONFIG_SF_DEFAULT_SPEED/MODE (see do_spi_flash_probe() in
cmd/sf.c) instead of using DT inputs.

Quoted 52 lines

Quoted 27 lines

Review int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 1 line

(quoted diff) +0 -0

@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

Quoted 23 lines

(quoted diff) +5 -0

 mode,
 
                           slave->dev = dev;
 
            }
 
 
 
+          plat = dev_get_parent_platdata(dev);
 
+          if (!speed) {
 
+                         speed = plat->max_hz;
 
+                         mode = plat->mode;
 
+          }
 
            ret = spi_set_speed_mode(bus, speed, mode);
 
            if (ret)
 
                           goto err;

Quoted 8 lines

Quoted 1 line

--

I just doubt if spi_set_speed_mode() has really made a difference to

the actual transfer.

It does (see below)...

Quoted 7 lines

Seems that if the device is inactive, calling device_probe() would also call

spi_set_speed_mode() and do the data transfer. Even if it's active, setting

speed and mode for it again would not be necessary.

Yes, spi_set_speed_mode() is called from spi_flash_probe_slave()->spi_claim_bus() as part of device_probe(). spi_claim_bus() in spi-uclass.c speed & mode are appropriately passed based on DT data to spi_set_speed_mode(). But that's not the issue.

Quoted 2 lines

But in spi_get_bus_and_cs() (called from sf probe) there is a call to spi_set_speed_mode() after device_probe() for inactive devices. This call is to

Yes. Actually I'm thinking that the second spi_set_speed_mode()(called from spi_get_bus_and_cs()) could just be removed from the end of the function.

Quoted 5 lines

_override_ the speed set via DT with those passed as cmd line args of sf probe. But, if no args are passed to sf probe, speed and mode default to CONFIG_SF_DEFAULT_SPEED/MODE (see do_spi_flash_probe() in
cmd/sf.c) instead of using DT inputs.

Yes. But notice that the speed and mode will only be replaced by CONFIG_SF_DEFAULT_SPEED/MODE at the end of the calling. Every time 'sf probe' is called, the device will be removed(if active) and thus it'll always call device_probe() to set the speed&mode from DT. Then the driver will use them in the actual transfer.

After the transfer is finished, the speed and mode are replaced by CONFIG_SF_DEFAULT_SPEED/MODE(or anything else) again but they wouldn't be used for the transfer at all.

And there may be another related issue in this case that I have reported to Simon. If you would like to test on your board, please remove the device_unbind() in do_spi_flash_probe(). The current SPI driver model will discard users' spi slave in DT from the second 'sf probe' and create a new one using CONFIG_SF_DEFAULT_SPEED/MODE.

-----Original Message-----
From: Vignesh R [mailto:vigneshr at ti.com]
Sent: Thursday, April 21, 2016 12:30 PM
To: Qianyu Gong <[email protected]>; jteki at openedev.com; trini at konsulko.com
Cc: u-boot at lists.denx.de; sr at denx.de; Mingkai Hu <[email protected]>
Subject: Re: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT

Hi Qianyu,

[...]

Quoted 44 lines

Quoted 44 lines

Quoted 27 lines

Review int spi_get_bus_and_cs(int busnum, int cs, int

Quoted 1 line

(quoted diff) +0 -0

@@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int

speed, int

Quoted 23 lines

(quoted diff) +5 -0

 mode,
 
                           slave->dev = dev;
 
            }
 
 
 
+          plat = dev_get_parent_platdata(dev);
 
+          if (!speed) {
 
+                         speed = plat->max_hz;
 
+                         mode = plat->mode;
 
+          }
 
            ret = spi_set_speed_mode(bus, speed, mode);
 
            if (ret)
 
                           goto err;

Quoted 8 lines

Quoted 1 line

--

I just doubt if spi_set_speed_mode() has really made a difference to

the actual transfer.

It does (see below)...

Quoted 6 lines

Seems that if the device is inactive, calling device_probe() would also call

spi_set_speed_mode() and do the data transfer. Even if it's active, setting

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

speed and mode for it again would not be necessary.

Yes, spi_set_speed_mode() is called from spi_flash_probe_slave()->spi_claim_bus() as part of device_probe(). spi_claim_bus() in spi-uclass.c speed & mode are appropriately passed based on

DT

Quoted 5 lines

Quoted 4 lines

data to spi_set_speed_mode(). But that's not the issue.

But in spi_get_bus_and_cs() (called from sf probe) there is a call to spi_set_speed_mode() after device_probe() for inactive devices. This call is to

Quoted 3 lines

Yes. Actually I'm thinking that the second spi_set_speed_mode()(called from spi_get_bus_and_cs()) could just be removed from the end of the function.

If second call to spi_set_speed_mode() is removed then how would you override default speed/mode specified via DT with that of cmd line args passed to sf probe. (else we need to change the definition of sf probe to not accept speed/mode in case of DT)

Quoted 1 line

Quoted 1 line

_override_ the speed set via DT with those passed as cmd line args of sf probe.

But,

Quoted 7 lines

Quoted 4 lines

if no args are passed to sf probe, speed and mode default to CONFIG_SF_DEFAULT_SPEED/MODE (see do_spi_flash_probe() in cmd/sf.c) instead of using DT inputs.

Yes. But notice that the speed and mode will only be replaced by CONFIG_SF_DEFAULT_SPEED/MODE at the end of the calling. Every time 'sf

probe' is

Quoted 1 line

called, the device will be removed(if active) and thus it'll always call

device_probe()

Quoted 1 line

to set the speed&mode from DT. Then the driver will use them in the actual

transfer.

True, I see the first call and speed/mode is set to DT values accordingly. But, that's not the final spi_set_speed_mode() call to the SPI driver.

Quoted 2 lines

After the transfer is finished, the speed and mode are replaced by CONFIG_SF_DEFAULT_SPEED/MODE(or anything else) again but they wouldn't

be used

Quoted 2 lines

for the transfer at all.

Sorry... I don't understand. What do you mean by transfer? sf probe does not do any data transfer other than reading jedec id.
The values set by the SPI driver during device_probe() will be overwritten by the last spi_set_speed_mode() call in spi_get_bus_and_cs() which happens to pass CONFIG_SF_DEFAULT_SPEED/MODE.

Here is the call sequence:

sf probe()
---> device_probe()
---> spi_set_speed_mode(values from DT)

    ---> driver's pi_set_speed_mode() /* set to DT values */

---> spi_set_speed_mode(overridden values)
---> driver's spi_set_speed_mode() /* set to newer(not DT) values */

Now if sf read is called after sf probe. One would see CONFIG_SF_DEFAULT_SPEED/MODE values in driver settings and data transfers happen at DEFAULT_SPEED.

Quoted 4 lines

And there may be another related issue in this case that I have reported to Simon. If you would like to test on your board, please remove the device_unbind() in do_spi_flash_probe(). The current SPI driver model will discard users' spi slave in

DT from

Quoted 1 line

the second 'sf probe' and create a new one using

CONFIG_SF_DEFAULT_SPEED/MODE.

Quoted 1 line

Yes, I am aware of the problem with second sf probe discarding values from DT. IIRC, Simon not just wanted to remove device_unbind() but also do some more refactoring of messages in uclass drivers. Maybe Simon is working on the fix.

--
Regards
Vignesh