Re: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT
Hi Vignesh,
Date: Wed, 13 Apr 2016 15:40:53 +0530
From: Vignesh R <vigneshr@ti.commailto:vigneshr@ti.com>
To: Jagan Teki <jteki@openedev.commailto:jteki@openedev.com>, Tom Rini <trini@konsulko.commailto:trini@konsulko.com>
Cc: u-boot@lists.denx.demailto:u-boot@lists.denx.de, Stefan Roese <sr@denx.demailto:sr@denx.de>
Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode
values from DT
Message-ID: <1460542253-10580-1-git-send-email-vigneshr@ti.commailto:1460542253-10580-1-git-send-email-vigneshr@ti.com>
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@ti.commailto:vigneshr@ti.com>
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(-)
diff --git a/cmd/sf.c b/cmd/sf.c
index 42862d9d921a..286906c3a151 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -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
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 5561f36762f9..5fb5630e2981 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -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);
@@ -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;
}
ret = spi_set_speed_mode(bus, speed, mode);
if (ret)
goto err;
--
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.
Regards, Qianyu
On 04/20/2016 03:26 PM, Qianyu Gong wrote:
Hi Vignesh,
Date: Wed, 13 Apr 2016 15:40:53 +0530
From: Vignesh R <vigneshr@ti.com mailto:vigneshr@ti.com>
To: Jagan Teki <jteki@openedev.com mailto:jteki@openedev.com>, Tom
Rini <trini@konsulko.com mailto:trini@konsulko.com>
Cc: u-boot@lists.denx.de mailto:u-boot@lists.denx.de, Stefan Roese
<sr@denx.de mailto:sr@denx.de>
Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode
values from DTMessage-ID: <1460542253-10580-1-git-send-email-vigneshr@ti.com
mailto:1460542253-10580-1-git-send-email-vigneshr@ti.com>
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@ti.com mailto:vigneshr@ti.com>
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(-)
diff --git a/cmd/sf.c b/cmd/sf.c
index 42862d9d921a..286906c3a151 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -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
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 5561f36762f9..5fb5630e2981 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int
speed, int
mode,
struct udevice **busp, structspi_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);
@@ -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;
}ret = spi_set_speed_mode(bus, speed, mode);if (ret)goto err;--
I just doubt if spi_set_speed_mode() has really made a difference to
the actual transfer.
It does (see below)...
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.
Hi Vignesh,
-----Original Message----- From: Vignesh R [mailto:vigneshr@ti.com] Sent: Wednesday, April 20, 2016 6:47 PM To: Qianyu Gong qianyu.gong@nxp.com; jteki@openedev.com; trini@konsulko.com Cc: u-boot@lists.denx.de; sr@denx.de; Mingkai Hu mingkai.hu@nxp.com Subject: Re: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT
On 04/20/2016 03:26 PM, Qianyu Gong wrote:
Hi Vignesh,
Date: Wed, 13 Apr 2016 15:40:53 +0530
From: Vignesh R <vigneshr@ti.com mailto:vigneshr@ti.com>
To: Jagan Teki <jteki@openedev.com mailto:jteki@openedev.com>, Tom
Rini <trini@konsulko.com mailto:trini@konsulko.com>
Cc: u-boot@lists.denx.de mailto:u-boot@lists.denx.de, Stefan Roese
<sr@denx.de mailto:sr@denx.de>
Subject: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode
values from DTMessage-ID: <1460542253-10580-1-git-send-email-vigneshr@ti.com
mailto:1460542253-10580-1-git-send-email-vigneshr@ti.com>
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@ti.com mailto:vigneshr@ti.com>
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(-)
diff --git a/cmd/sf.c b/cmd/sf.c
index 42862d9d921a..286906c3a151 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -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
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index 5561f36762f9..5fb5630e2981 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int
speed, int
mode,
struct udevice **busp, structspi_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);
@@ -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;
}ret = spi_set_speed_mode(bus, speed, mode);if (ret)goto err;--
I just doubt if spi_set_speed_mode() has really made a difference to
the actual transfer.
It does (see below)...
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
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.
_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.
Regards, Qianyu
Hi Qianyu,
[...]
@@ -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;
}ret = spi_set_speed_mode(bus, speed, mode);if (ret)goto err;--
I just doubt if spi_set_speed_mode() has really made a difference to
the actual transfer.
It does (see below)...
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
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)
_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.
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.
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.
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.
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.
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.
Hi Vignesh,
-----Original Message----- From: Vignesh R [mailto:vigneshr@ti.com] Sent: Thursday, April 21, 2016 12:30 PM To: Qianyu Gong qianyu.gong@nxp.com; jteki@openedev.com; trini@konsulko.com Cc: u-boot@lists.denx.de; sr@denx.de; Mingkai Hu mingkai.hu@nxp.com Subject: Re: [U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT
Hi Qianyu,
[...]
@@ -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;
}ret = spi_set_speed_mode(bus, speed, mode);if (ret)goto err;--
I just doubt if spi_set_speed_mode() has really made a difference to
the actual transfer.
It does (see below)...
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
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)
OK, I see. Thanks.
_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.
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.
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.
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.
Yes, I should say reading jedec id from flash.
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 */
Here reading jedec id is finished before calling the second spi_set_speed_mode().
---> 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.
OK. So my conclusion is that the second spi_set_speed_mode() does affect 'sf read/write/..' (overridden values) but not affect 'sf probe'(always setting DT values). I think that's kind of weird..
Regards, Qianyu
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.
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
participants (2)
-
Qianyu Gong -
Vignesh R