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