[PATCH] spi: Add spi_get_bus_and_cs() new use_dt param

[Patrice CHOTARD]

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