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

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

[Patrice Chotard]

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(-)

diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
index 2202eb8cfb..363edc115f 100644
--- a/board/CZ.NIC/turris_mox/turris_mox.c
+++ b/board/CZ.NIC/turris_mox/turris_mox.c
@@ -135,7 +135,7 @@ static int mox_do_spi(u8 *in, u8 *out, size_t size)
 	struct udevice *dev;
 	int ret;
 
-	ret = spi_get_bus_and_cs(0, 1, 1000000, SPI_CPHA | SPI_CPOL,
+	ret = spi_get_bus_and_cs(0, 1, 1000000, SPI_CPHA | SPI_CPOL, false,
 				 "spi_generic_drv", "moxtet@1", &dev,
 				 &slave);
 	if (ret)
diff --git a/cmd/sf.c b/cmd/sf.c
index eac27ed2d7..0c1ddbbab7 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -91,6 +91,7 @@ static int do_spi_flash_probe(int argc, char *const argv[])
 	unsigned int speed = CONFIG_SF_DEFAULT_SPEED;
 	unsigned int mode = CONFIG_SF_DEFAULT_MODE;
 	char *endp;
+	bool use_dt = true;
 #if CONFIG_IS_ENABLED(DM_SPI_FLASH)
 	struct udevice *new, *bus_dev;
 	int ret;
@@ -117,11 +118,13 @@ static int do_spi_flash_probe(int argc, char *const argv[])
 		speed = simple_strtoul(argv[2], &endp, 0);
 		if (*argv[2] == 0 || *endp != 0)
 			return -1;
+		use_dt = false;
 	}
 	if (argc >= 4) {
 		mode = hextoul(argv[3], &endp);
 		if (*argv[3] == 0 || *endp != 0)
 			return -1;
+		use_dt = false;
 	}
 
 #if CONFIG_IS_ENABLED(DM_SPI_FLASH)
@@ -131,7 +134,7 @@ static int do_spi_flash_probe(int argc, char *const argv[])
 		device_remove(new, DM_REMOVE_NORMAL);
 	}
 	flash = NULL;
-	ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new);
+	ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, use_dt, &new);
 	if (ret) {
 		printf("Failed to initialize SPI flash at %u:%u (error %d)\n",
 		       bus, cs, ret);
diff --git a/cmd/spi.c b/cmd/spi.c
index 6dc32678da..46bd817e60 100644
--- a/cmd/spi.c
+++ b/cmd/spi.c
@@ -46,7 +46,7 @@ static int do_spi_xfer(int bus, int cs)
 	str = strdup(name);
 	if (!str)
 		return -ENOMEM;
-	ret = spi_get_bus_and_cs(bus, cs, freq, mode, "spi_generic_drv",
+	ret = spi_get_bus_and_cs(bus, cs, freq, mode, false, "spi_generic_drv",
 				 str, &dev, &slave);
 	if (ret)
 		return ret;
diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c
index 63d16291ff..ef3e3bb9de 100644
--- a/drivers/mtd/spi/sf-uclass.c
+++ b/drivers/mtd/spi/sf-uclass.c
@@ -51,7 +51,7 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 {
 	struct udevice *dev;
 
-	if (spi_flash_probe_bus_cs(bus, cs, max_hz, spi_mode, &dev))
+	if (spi_flash_probe_bus_cs(bus, cs, max_hz, spi_mode, false, &dev))
 		return NULL;
 
 	return dev_get_uclass_priv(dev);
@@ -59,7 +59,7 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 
 int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
 			   unsigned int max_hz, unsigned int spi_mode,
-			   struct udevice **devp)
+			   bool use_dt, struct udevice **devp)
 {
 	struct spi_slave *slave;
 	struct udevice *bus;
@@ -74,8 +74,8 @@ int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
 	snprintf(name, sizeof(name), "spi_flash@%d:%d", busnum, cs);
 	str = strdup(name);
 #endif
-	ret = spi_get_bus_and_cs(busnum, cs, max_hz, spi_mode,
-				  "jedec_spi_nor", str, &bus, &slave);
+	ret = spi_get_bus_and_cs(busnum, cs, max_hz, spi_mode, use_dt,
+				 "jedec_spi_nor", str, &bus, &slave);
 	if (ret)
 		return ret;
 
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c
index 7d51be1f72..c56afc6a47 100644
--- a/drivers/net/fm/fm.c
+++ b/drivers/net/fm/fm.c
@@ -388,7 +388,8 @@ int fm_init_common(int index, struct ccsr_fman *reg)
 
 		/* speed and mode will be read from DT */
 		ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS,
-					     CONFIG_ENV_SPI_CS, 0, 0, &new);
+					     CONFIG_ENV_SPI_CS, 0, 0, true,
+					     &new);
 
 		ucode_flash = dev_get_uclass_priv(new);
 #else
@@ -475,7 +476,7 @@ int fm_init_common(int index, struct ccsr_fman *reg)
 
 	/* speed and mode will be read from DT */
 	ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
-				     0, 0, &new);
+				     0, 0, true, &new);
 
 	ucode_flash = dev_get_uclass_priv(new);
 #else
diff --git a/drivers/net/pfe_eth/pfe_firmware.c b/drivers/net/pfe_eth/pfe_firmware.c
index ad5bc3c862..d30991571c 100644
--- a/drivers/net/pfe_eth/pfe_firmware.c
+++ b/drivers/net/pfe_eth/pfe_firmware.c
@@ -183,7 +183,7 @@ int pfe_spi_flash_init(void)
 				     CONFIG_ENV_SPI_CS,
 				     CONFIG_ENV_SPI_MAX_HZ,
 				     CONFIG_ENV_SPI_MODE,
-				     &new);
+				     true, &new);
 	if (ret) {
 		printf("SF: failed to probe spi\n");
 		free(addr);
diff --git a/drivers/net/sni_netsec.c b/drivers/net/sni_netsec.c
index 4901321d0c..6d00a3fd3a 100644
--- a/drivers/net/sni_netsec.c
+++ b/drivers/net/sni_netsec.c
@@ -625,7 +625,8 @@ static void netsec_spi_read(char *buf, loff_t len, loff_t offset)
 	struct spi_flash *flash;
 
 	spi_flash_probe_bus_cs(CONFIG_SF_DEFAULT_BUS, CONFIG_SF_DEFAULT_CS,
-			       CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE, &new);
+			       CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE,
+			       true, &new);
 	flash = dev_get_uclass_priv(new);
 
 	spi_flash_read(flash, offset, len, buf);
diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index f8ec312d71..4366124e3c 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -340,7 +340,7 @@ int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
 	return ret;
 }
 
-int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, bool use_dt,
 		       const char *drv_name, const char *dev_name,
 		       struct udevice **busp, struct spi_slave **devp)
 {
@@ -419,8 +419,8 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
 	}
 
 	/* In case bus frequency or mode changed, update it. */
-	if ((speed && bus_data->speed && bus_data->speed != speed) ||
-	    (plat && plat->mode != mode)) {
+	if (((speed && bus_data->speed && bus_data->speed != speed) ||
+	     (plat && plat->mode != mode)) && !use_dt) {
 		ret = spi_set_speed_mode(bus, speed, mode);
 		if (ret)
 			goto err_speed_mode;
@@ -453,7 +453,7 @@ struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
 	struct udevice *dev;
 	int ret;
 
-	ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev,
+	ret = spi_get_bus_and_cs(busnum, cs, speed, mode, false, NULL, 0, &dev,
 				 &slave);
 	if (ret)
 		return NULL;
diff --git a/drivers/usb/gadget/max3420_udc.c b/drivers/usb/gadget/max3420_udc.c
index a16095f892..ccec337f99 100644
--- a/drivers/usb/gadget/max3420_udc.c
+++ b/drivers/usb/gadget/max3420_udc.c
@@ -830,7 +830,7 @@ static int max3420_udc_probe(struct udevice *dev)
 	cs = slave_pdata->cs;
 	speed = slave_pdata->max_hz;
 	mode = slave_pdata->mode;
-	spi_get_bus_and_cs(busnum, cs, speed, mode, "spi_generic_drv",
+	spi_get_bus_and_cs(busnum, cs, speed, mode, false, "spi_generic_drv",
 			   NULL, &spid, &udc->slave);
 
 	udc->dev = dev;
diff --git a/env/sf.c b/env/sf.c
index 6a4bb756f0..ed6bfb2115 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -53,7 +53,7 @@ static int setup_flash_device(struct spi_flash **env_flash)
 	/* speed and mode will be read from DT */
 	ret = spi_flash_probe_bus_cs(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
 				     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE,
-				     &new);
+				     true, &new);
 	if (ret) {
 		env_set_default("spi_flash_probe_bus_cs() failed", 0);
 		return ret;
diff --git a/include/spi.h b/include/spi.h
index dc3b21132a..5d12f27b19 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -582,15 +582,16 @@ int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
  * @cs:		Chip select to look for
  * @speed:	SPI speed to use for this slave when not available in plat
  * @mode:	SPI mode to use for this slave when not available in plat
+ * @use_dt:     Force usage of SPI speed and SPI mode from DT
  * @drv_name:	Name of driver to attach to this chip select
  * @dev_name:	Name of the new device thus created
  * @busp:	Returns bus device
  * @devp:	Return slave device
- * @return 0 if found, -ve on error
+  * @return 0 if found, -ve on error
  */
-int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
-			const char *drv_name, const char *dev_name,
-			struct udevice **busp, struct spi_slave **devp);
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, bool use_dt,
+		       const char *drv_name, const char *dev_name,
+		       struct udevice **busp, struct spi_slave **devp);
 
 /**
  * spi_chip_select() - Get the chip select for a slave
diff --git a/include/spi_flash.h b/include/spi_flash.h
index 4d4ae89c19..07bca1ee3b 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -117,7 +117,7 @@ int spi_flash_std_probe(struct udevice *dev);
 
 int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
 			   unsigned int max_hz, unsigned int spi_mode,
-			   struct udevice **devp);
+			   bool use_dt, struct udevice **devp);
 
 /* Compatibility function - this is the old U-Boot API */
 struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
diff --git a/test/dm/spi.c b/test/dm/spi.c
index ee4ad3abaa..2b1f5535c6 100644
--- a/test/dm/spi.c
+++ b/test/dm/spi.c
@@ -47,7 +47,7 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	/* This finds nothing because we removed the device */
 	ut_asserteq(-ENODEV, spi_find_bus_and_cs(busnum, cs, &bus, &dev));
 	ut_asserteq(-ENODEV, spi_get_bus_and_cs(busnum, cs, speed, mode,
-						NULL, 0, &bus, &slave));
+						false, NULL, 0, &bus, &slave));
 
 	/*
 	 * This forces the device to be re-added, but there is no emulation
@@ -56,7 +56,7 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	 * a 'partially-inited' device.
 	 */
 	ut_asserteq(-ENODEV, spi_find_bus_and_cs(busnum, cs, &bus, &dev));
-	ut_asserteq(-ENOENT, spi_get_bus_and_cs(busnum, cs, speed, mode,
+	ut_asserteq(-ENOENT, spi_get_bus_and_cs(busnum, cs, speed, mode, false,
 						"jedec_spi_nor", "name", &bus,
 						&slave));
 	sandbox_sf_unbind_emul(state_get_current(), busnum, cs);
@@ -67,7 +67,7 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	ut_assertok(sandbox_sf_bind_emul(state, busnum, cs, bus, node,
 					 "name"));
 	ut_assertok(spi_find_bus_and_cs(busnum, cs, &bus, &dev));
-	ut_assertok(spi_get_bus_and_cs(busnum, cs, speed, mode,
+	ut_assertok(spi_get_bus_and_cs(busnum, cs, speed, mode, false,
 				       "jedec_spi_nor", "name", &bus, &slave));
 
 	ut_assertok(spi_cs_info(bus, cs, &info));
@@ -77,7 +77,8 @@ static int dm_test_spi_find(struct unit_test_state *uts)
 	ut_assertok(sandbox_sf_bind_emul(state, busnum, cs_b, bus, node,
 					 "name"));
 	ut_asserteq(-EINVAL, spi_get_bus_and_cs(busnum, cs_b, speed, mode,
-				       "jedec_spi_nor", "name", &bus, &slave));
+						false, "jedec_spi_nor", "name",
+						&bus, &slave));
 	ut_asserteq(-EINVAL, spi_cs_info(bus, cs_b, &info));
 	ut_asserteq_ptr(NULL, info.dev);
 
@@ -145,10 +146,10 @@ static int dm_test_spi_claim_bus(struct unit_test_state *uts)
 	const int busnum = 0, cs_a = 0, cs_b = 1, mode = 0;
 
 	/* Get spi slave on CS0 */
-	ut_assertok(spi_get_bus_and_cs(busnum, cs_a, 1000000, mode, NULL, 0,
+	ut_assertok(spi_get_bus_and_cs(busnum, cs_a, 1000000, mode, false, NULL, 0,
 				       &bus, &slave_a));
 	/* Get spi slave on CS1 */
-	ut_assertok(spi_get_bus_and_cs(busnum, cs_b, 1000000, mode, NULL, 0,
+	ut_assertok(spi_get_bus_and_cs(busnum, cs_b, 1000000, mode, false, NULL, 0,
 				       &bus, &slave_b));
 
 	/* Different max_hz, different mode. */
@@ -182,7 +183,7 @@ static int dm_test_spi_xfer(struct unit_test_state *uts)
 	const char dout[5] = {0x9f};
 	unsigned char din[5];
 
-	ut_assertok(spi_get_bus_and_cs(busnum, cs, 1000000, mode, NULL, 0,
+	ut_assertok(spi_get_bus_and_cs(busnum, cs, 1000000, mode, false, NULL, 0,
 				       &bus, &slave));
 	ut_assertok(spi_claim_bus(slave));
 	ut_assertok(spi_xfer(slave, 40, dout, din,
-- 
2.17.1

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

[Simon Glass]

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.

Also we should probably have devicetree as the default (the base function name).

Regards,
Simon

Re: [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@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

Re: [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@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@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, ...)

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

Re: [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@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@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, ...)
> 
> 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

Re: [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@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@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@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, ...)
> >
> > 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