[U-Boot] [PATCH v3] dm: spi: Read default speed and mode values from DT

[Qianyu Gong]

Hi Vignesh,

> -----Original Message-----
> From: Vignesh R [mailto:vigneshr at ti.com]
> Sent: Wednesday, April 20, 2016 6:47 PM
> To: Qianyu Gong <qianyu.gong at nxp.com>; jteki at openedev.com;
> trini at konsulko.com
> Cc: u-boot at lists.denx.de; sr at denx.de; Mingkai Hu <mingkai.hu at 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 at ti.com <mailto:vigneshr at ti.com>>
> >
> >> To: Jagan Teki <jteki at openedev.com <mailto:jteki at openedev.com>>, Tom
> > Rini <trini at konsulko.com <mailto:trini at konsulko.com>>
> >
> >> Cc: u-boot at lists.denx.de <mailto:u-boot at lists.denx.de>, Stefan Roese
> > <sr at denx.de <mailto:sr at 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 at ti.com
> > <mailto:1460542253-10580-1-git-send-email-vigneshr at 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 at ti.com <mailto:vigneshr at 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