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

[Simon Glass]

Hi Patrice,

On Tue, 1 Mar 2022 at 03:44, Patrice CHOTARD
<patrice.chotard at foss.st.com> wrote:
>
> 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 ?

Yes I think that makes sense, because we want the non-dt path to be
the exception.

Regards,
Simon