Hi Simon
On 1/21/22 16:20, Simon Glass wrote:
Hi Patrice,
On Wed, 12 Jan 2022 at 03:59, Patrice Chotard patrice.chotard@foss.st.com 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 patrice.chotard@foss.st.com Cc: Marek Behun marek.behun@nic.cz Cc: Jagan Teki jagan@amarulasolutions.com Cc: Vignesh R vigneshr@ti.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com Cc: Lukasz Majewski lukma@denx.de Cc: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Cc: "Pali Rohár" pali@kernel.org Cc: Konstantin Porotchkin kostap@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Pratyush Yadav p.yadav@ti.com Cc: Sean Anderson seanga2@gmail.com Cc: Anji J anji.jagarlmudi@nxp.com Cc: Biwen Li biwen.li@nxp.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Chaitanya Sakinam chaitanya.sakinam@nxp.com
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