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 ? Patrice > > Regards, > Simon
