Hi Patrice, On Tue, 1 Mar 2022 at 03:44, Patrice CHOTARD <[email protected]> wrote: > > Hi Simon > > On 2/26/22 19:36, Simon Glass wrote: > > Hi Patrice, > > > > On Mon, 31 Jan 2022 at 09:14, Patrice CHOTARD > > <[email protected]> wrote: > >> > >> Hi Simon > >> > >> On 1/21/22 16:20, Simon Glass wrote: > >>> Hi Patrice, > >>> > >>> On Wed, 12 Jan 2022 at 03:59, Patrice Chotard > >>> <[email protected]> wrote: > >>>> > >>>> Add spi_flash_probe_bus_cs() and spi_get_bus_and_cs() new "use_dt" > >>>> param which allows to select SPI speed and mode from DT or from > >>>> default value passed in parameters. > >>>> > >>>> Since commit e2e95e5e2542 ("spi: Update speed/mode on change") > >>>> when calling "sf probe" or "env save" on SPI flash, > >>>> spi_set_speed_mode() is called twice. > >>>> > >>>> spi_get_bus_and_cs() > >>>> |--> spi_claim_bus() > >>>> | |--> spi_set_speed_mode(speed and mode from DT) > >>>> ... > >>>> |--> spi_set_speed_mode(default speed and mode value) > >>>> > >>>> The first spi_set_speed_mode() call is done with speed and mode > >>>> values from DT, whereas the second call is done with speed > >>>> and mode set to default value (speed is set to CONFIG_SF_DEFAULT_SPEED) > >>>> > >>>> This is an issue because SPI flash performance are impacted by > >>>> using default speed which can be lower than the one defined in DT. > >>>> > >>>> One solution is to set CONFIG_SF_DEFAULT_SPEED to the speed defined > >>>> in DT, but we loose flexibility offered by DT. > >>>> > >>>> Another issue can be encountered with 2 SPI flashes using 2 different > >>>> speeds. In this specific case usage of CONFIG_SF_DEFAULT_SPEED is not > >>>> flexible compared to get the 2 different speeds from DT. > >>>> > >>>> By adding new parameter "use_dt" to spi_flash_probe_bus_cs() and to > >>>> spi_get_bus_and_cs(), this allows to force usage of either speed and > >>>> mode from DT (use_dt = true) or to use speed and mode parameter value. > >>>> > >>>> Signed-off-by: Patrice Chotard <[email protected]> > >>>> Cc: Marek Behun <[email protected]> > >>>> Cc: Jagan Teki <[email protected]> > >>>> Cc: Vignesh R <[email protected]> > >>>> Cc: Joe Hershberger <[email protected]> > >>>> Cc: Ramon Fried <[email protected]> > >>>> Cc: Lukasz Majewski <[email protected]> > >>>> Cc: Marek Vasut <[email protected]> > >>>> Cc: Wolfgang Denk <[email protected]> > >>>> Cc: Simon Glass <[email protected]> > >>>> Cc: Stefan Roese <[email protected]> > >>>> Cc: "Pali Rohár" <[email protected]> > >>>> Cc: Konstantin Porotchkin <[email protected]> > >>>> Cc: Igal Liberman <[email protected]> > >>>> Cc: Bin Meng <[email protected]> > >>>> Cc: Pratyush Yadav <[email protected]> > >>>> Cc: Sean Anderson <[email protected]> > >>>> Cc: Anji J <[email protected]> > >>>> Cc: Biwen Li <[email protected]> > >>>> Cc: Priyanka Jain <[email protected]> > >>>> Cc: Chaitanya Sakinam <[email protected]> > >>>> --- > >>>> > >>>> board/CZ.NIC/turris_mox/turris_mox.c | 2 +- > >>>> cmd/sf.c | 5 ++++- > >>>> cmd/spi.c | 2 +- > >>>> drivers/mtd/spi/sf-uclass.c | 8 ++++---- > >>>> drivers/net/fm/fm.c | 5 +++-- > >>>> drivers/net/pfe_eth/pfe_firmware.c | 2 +- > >>>> drivers/net/sni_netsec.c | 3 ++- > >>>> drivers/spi/spi-uclass.c | 8 ++++---- > >>>> drivers/usb/gadget/max3420_udc.c | 2 +- > >>>> env/sf.c | 2 +- > >>>> include/spi.h | 9 +++++---- > >>>> include/spi_flash.h | 2 +- > >>>> test/dm/spi.c | 15 ++++++++------- > >>>> 13 files changed, 36 insertions(+), 29 deletions(-) > >>> > >>> I think this is a good idea, but should use a separate function name > >>> instead of adding an argument, since it doesn't make sense to pass > >>> parameters that are ignored. > >> > >> I am confused, do you mean duplicate spi_flash_probe_bus_cs() in another > >> function spi_flash_probe_bus_cs_default() > >> for example ? > >> In the former spi_get_bus_and_cs() will be called with "use_dt" param set > >> to true and in the latter > >> "use_dt" param will be set to false ? > >> > >> spi_flash_probe_bus_cs() => spi_get_bus_and_cs(.., true , ...) > >> spi_flash_probe_bus_cs_default() => spi_get_bus_and_cs(.., false, ...) > > > > Maybe rename spi_get_bus_and_cs() to _spi_get_bus_and_cs() ? > > > > It seems to me that if use_dt is provided, we should actually be using > > DT and not calling this function at all. We should just be able to > > probe the device and the right things should happen. > > > > If we must use the bus and cs numbers, and use_dt is true, then we > > should not need to specify the mode, speed, etc. So the args to that > > function should be different. > > > > So I believe the two functions should have quite different args. > > Perhaps the core part of spi_get_bus_and_cs() could be split out? I > > just feel there are way too many arguments and adding an argument that > > cancels out some of the others just makes a confusing mess. > > Thanks for clarification, i understand now what you expect. > > > > >> > >> Thanks > >> Patrice > >>> > >>> Also we should probably have devicetree as the default (the base function > >>> name). > >>> > > > > See that also ^ > > You mean that spi_get_bus_and_cs() will imply using device tree by default ?
Yes I think that makes sense, because we want the non-dt path to be the exception. Regards, Simon
