Hi Vignesh, > -----Original Message----- > From: Vignesh R [mailto:vigne...@ti.com] > Sent: Wednesday, April 20, 2016 6:47 PM > To: Qianyu Gong <qianyu.g...@nxp.com>; jt...@openedev.com; > tr...@konsulko.com > Cc: u-boot@lists.denx.de; s...@denx.de; Mingkai Hu <mingkai...@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 <vigne...@ti.com <mailto:vigne...@ti.com>> > > > >> To: Jagan Teki <jt...@openedev.com <mailto:jt...@openedev.com>>, Tom > > Rini <tr...@konsulko.com <mailto:tr...@konsulko.com>> > > > >> Cc: u-boot@lists.denx.de <mailto:u-boot@lists.denx.de>, Stefan Roese > > <s...@denx.de <mailto:s...@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-vigne...@ti.com > > <mailto:1460542253-10580-1-git-send-email-vigne...@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 <vigne...@ti.com <mailto:vigne...@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. > > > > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
