[PATCH v2] spi: Add spi_get_bus_and_cs() new use_dt param
Patrice CHOTARD
patrice.chotard at foss.st.com
Mon Feb 28 08:45:41 CET 2022
Hi
Don't take care of this patch, a v3 will be send
Patrice
On 2/21/22 08:33, Patrice Chotard wrote:
> Add 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.
>
> Introduce spi_flash_probe_bus_cs_default() which is identical
> to spi_flash_probe_bus_cs() except it calls spi_get_bus_and_cs()
> with use_dt param set to false.
>
> 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_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>
>
> ---
>
> Changes in v2:
> - add spi_flash_probe_bus_cs_default() which calls spi_get_bus_and_cs()
> with "use_dt" param set to true, whereas spi_flash_probe_bus_cs() calls
> spi_get_bus_and_cs() with "use_dt" param set to true.
>
> board/CZ.NIC/turris_mox/turris_mox.c | 2 +-
> cmd/sf.c | 9 ++++++++-
> cmd/spi.c | 2 +-
> drivers/mtd/spi/sf-uclass.c | 30 ++++++++++++++++++++++++++--
> drivers/spi/spi-uclass.c | 8 ++++----
> drivers/usb/gadget/max3420_udc.c | 2 +-
> include/spi.h | 7 ++++---
> include/spi_flash.h | 4 ++++
> test/dm/spi.c | 15 +++++++-------
> 9 files changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c
> index f0c5aa6a52..4b755e1420 100644
> --- a/board/CZ.NIC/turris_mox/turris_mox.c
> +++ b/board/CZ.NIC/turris_mox/turris_mox.c
> @@ -148,7 +148,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 at 1", &dev,
> &slave);
> if (ret)
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 8bdebd9fd8..40b2cc3297 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,11 @@ 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);
> + if (use_dt)
> + ret = spi_flash_probe_bus_cs(bus, cs, speed, mode, &new);
> + else
> + ret = spi_flash_probe_bus_cs_default(bus, cs, speed, mode, &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..f38518b7fd 100644
> --- a/drivers/mtd/spi/sf-uclass.c
> +++ b/drivers/mtd/spi/sf-uclass.c
> @@ -74,8 +74,34 @@ 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, true,
> + "jedec_spi_nor", str, &bus, &slave);
> + if (ret)
> + return ret;
> +
> + *devp = slave->dev;
> + return 0;
> +}
> +
> +int spi_flash_probe_bus_cs_default(unsigned int busnum, unsigned int cs,
> + unsigned int max_hz, unsigned int spi_mode,
> + struct udevice **devp)
> +{
> + struct spi_slave *slave;
> + struct udevice *bus;
> + char *str;
> + int ret;
> +
> +#if defined(CONFIG_SPL_BUILD) && CONFIG_IS_ENABLED(USE_TINY_PRINTF)
> + str = "spi_flash";
> +#else
> + char name[30];
> +
> + 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, false,
> + "jedec_spi_nor", str, &bus, &slave);
> if (ret)
> return ret;
>
> 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/include/spi.h b/include/spi.h
> index fa9ab12dbe..0aa5fe033c 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
> */
> -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 d33d0dd46a..256878df6a 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -105,6 +105,10 @@ int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
> unsigned int max_hz, unsigned int spi_mode,
> struct udevice **devp);
>
> +int spi_flash_probe_bus_cs_default(unsigned int busnum, unsigned int cs,
> + unsigned int max_hz, unsigned int spi_mode,
> + struct udevice **devp);
> +
> /* Compatibility function - this is the old U-Boot API */
> struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
> unsigned int max_hz, unsigned int spi_mode);
> 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,
More information about the U-Boot
mailing list