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, ...) Thanks Patrice > > Also we should probably have devicetree as the default (the base function > name). > > Regards, > Simon
