Hi Vignesh, > -----Original Message----- > From: Vignesh R [mailto:vigne...@ti.com] > Sent: Thursday, April 21, 2016 12:30 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 > > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
