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

[Patrice CHOTARD]

Hi Simon

On 2/26/22 19:36, Simon Glass wrote:
> Hi Patrice,
> 
> On Mon, 31 Jan 2022 at 09:14, Patrice CHOTARD
> <patrice.chotard at foss.st.com> wrote:
>>
>> 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, ...)
> 
> 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