[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: Thursday, April 21, 2016 12:30 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
> 
> 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