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

[Simon Glass]

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

See that also ^

Regards,
Simon