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.

To fix this issue, the legacy spi_get_bus_and_cs() is renamed to _spi_get_bus_and_cs() and keep the same behaviour. Add a new spi_get_bus_and_cs() which rely on DT for spi speed and mode values.

Now, spi_flash_probe_bus_cs() relies on DT for spi speed and mode and logically calls spi_get_bus_and_cs(). In case spi mode and speed are not read from DT, make usage of spi_flash_probe(). This patch impacts the following : - cmd/sf.c: if spi mode and/or speed is passed in argument, call spi_flash_probe() otherwise call spi_flash_probe_bus_cs().

_ cmd/spi.c: keep legacy, replace spi_get_bus_and_cs() by _spi_get_bus_and_cs()

_ drivers/net/fm/fm.c: no impact as by default spi speed and mode was set to 0 and a comment indicates that speed and mode are read from DT.

_ drivers/net/pfe_eth/pfe_firmware.c: spi speed and mode are not read from DT by all platforms using this driver, so keep legacy and replace spi_flash_probe_bus_cs() by spi_flash_probe();

_ drivers/usb/gadget/max3420_udc.c: spi speed and mode are not read from DT, keep legacy, replace spi_get_bus_and_cs() by _spi_get_bus_and_cs()

_ drivers/net/sni_netsec.c : spi speed and mode are not read from DT, so replace spi_flash_probe_bus_cs() by spi_flash_probe().

_ drivers/usb/gadget/max3420_udc.c: Can't find any platform which make usage of this driver, nevertheless, keep legacy and replace spi_get_bus_and_cs() by _spi_get_bus_and_cs().

_ env/sf.c: no impact, as by default spi speed and mode was set to 0 and a comment indicates that speed and mode are read from DT.

- update test/dm/spi.c: when spi_get_bus_and_cs() was used wihtout driver and device name, no change. When spi_get_bus_and_cs() was used with driver and device name, use the legacy by calling _spi_get_bus_and_cs().

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

Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com ---

Changes in v3: - Update commit header to reflect what the patch really do. - Rename legacy spi_get_bus_and_cs() to _spi_get_bus_and_cs(). - New spi_get_bus_and_cs() rely on DT for spi speed and mode values. - spi_flash_probe_bus_cs() rely also on DT for spi and mode values.

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 | 5 +-- cmd/sf.c | 15 +++++-- cmd/spi.c | 4 +- drivers/mtd/spi/sf-uclass.c | 34 ++++++++------- drivers/net/fm/fm.c | 4 +- drivers/net/pfe_eth/pfe_firmware.c | 15 +------ drivers/net/sni_netsec.c | 6 +-- drivers/spi/spi-uclass.c | 65 +++++++++++++++++++++++++--- drivers/usb/gadget/max3420_udc.c | 4 +- env/sf.c | 1 - include/spi.h | 19 +++++++- include/spi_flash.h | 1 - test/dm/spi.c | 32 +++++++------- 13 files changed, 134 insertions(+), 71 deletions(-)

diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c index 8888a2dcab..8fec1622cd 100644 --- a/board/CZ.NIC/turris_mox/turris_mox.c +++ b/board/CZ.NIC/turris_mox/turris_mox.c @@ -148,9 +148,8 @@ 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, - "spi_generic_drv", "moxtet@1", &dev, - &slave); + ret = _spi_get_bus_and_cs(0, 1, 1000000, SPI_CPHA | SPI_CPOL, + "spi_generic_drv", "moxtet@1", &dev, &slave); if (ret) goto fail;

diff --git a/cmd/sf.c b/cmd/sf.c index 8bdebd9fd8..8713736b2a 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,14 +134,18 @@ 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 (ret) { + if (use_dt) { + spi_flash_probe_bus_cs(bus, cs, &new); + flash = dev_get_uclass_priv(new); + } else { + flash = spi_flash_probe(bus, cs, speed, mode); + } + + if (!flash) { printf("Failed to initialize SPI flash at %u:%u (error %d)\n", bus, cs, ret); return 1; } - - flash = dev_get_uclass_priv(new); #else if (flash) spi_flash_free(flash); diff --git a/cmd/spi.c b/cmd/spi.c index 6dc32678da..454ebe37d7 100644 --- a/cmd/spi.c +++ b/cmd/spi.c @@ -46,8 +46,8 @@ 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", - str, &dev, &slave); + ret = _spi_get_bus_and_cs(bus, cs, freq, mode, "spi_generic_drv", + str, &dev, &slave); if (ret) return ret; #else diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c index 63d16291ff..4f186011e0 100644 --- a/drivers/mtd/spi/sf-uclass.c +++ b/drivers/mtd/spi/sf-uclass.c @@ -46,25 +46,12 @@ int spl_flash_get_sw_write_prot(struct udevice *dev) * TODO(sjg@chromium.org): This is an old-style function. We should remove * it when all SPI flash drivers use dm */ -struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs, +struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs, unsigned int max_hz, unsigned int spi_mode) -{ - struct udevice *dev; - - if (spi_flash_probe_bus_cs(bus, cs, max_hz, spi_mode, &dev)) - return NULL; - - return dev_get_uclass_priv(dev); -} - -int spi_flash_probe_bus_cs(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"; @@ -74,12 +61,27 @@ 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); + + if (_spi_get_bus_and_cs(busnum, cs, max_hz, spi_mode, + "jedec_spi_nor", str, &bus, &slave)) + return NULL; + + return dev_get_uclass_priv(slave->dev); +} + +int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs, + struct udevice **devp) +{ + struct spi_slave *slave; + struct udevice *bus; + int ret; + + ret = spi_get_bus_and_cs(busnum, cs, &bus, &slave); if (ret) return ret;

*devp = slave->dev; + return 0; }

diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index f825612640..d0b492b5a1 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -388,7 +388,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_SF_DEFAULT_BUS, - CONFIG_SF_DEFAULT_CS, 0, 0, &new); + CONFIG_SF_DEFAULT_CS, &new);

ucode_flash = dev_get_uclass_priv(new); #else @@ -475,7 +475,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_SF_DEFAULT_BUS, CONFIG_SF_DEFAULT_CS, - 0, 0, &new); + &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 6669048181..52142e1976 100644 --- a/drivers/net/pfe_eth/pfe_firmware.c +++ b/drivers/net/pfe_eth/pfe_firmware.c @@ -172,31 +172,20 @@ static int pfe_fit_check(void) int pfe_spi_flash_init(void) { struct spi_flash *pfe_flash; - struct udevice *new; int ret = 0; void *addr = malloc(CONFIG_SYS_LS_PFE_FW_LENGTH);

if (!addr) return -ENOMEM;

- ret = spi_flash_probe_bus_cs(CONFIG_SYS_FSL_PFE_SPI_BUS, + pfe_flash = spi_flash_probe(CONFIG_SYS_FSL_PFE_SPI_BUS, CONFIG_SYS_FSL_PFE_SPI_CS, CONFIG_SYS_FSL_PFE_SPI_MAX_HZ, - CONFIG_SYS_FSL_PFE_SPI_MODE, - &new); - if (ret) { - printf("SF: failed to probe spi\n"); - free(addr); - device_remove(new, DM_REMOVE_NORMAL); - return ret; - } - + CONFIG_SYS_FSL_PFE_SPI_MODE);

- pfe_flash = dev_get_uclass_priv(new); if (!pfe_flash) { printf("SF: probe for pfe failed\n"); free(addr); - device_remove(new, DM_REMOVE_NORMAL); return -ENODEV; }

diff --git a/drivers/net/sni_netsec.c b/drivers/net/sni_netsec.c index 4901321d0c..c132e9bb32 100644 --- a/drivers/net/sni_netsec.c +++ b/drivers/net/sni_netsec.c @@ -621,12 +621,10 @@ static int netsec_stop_gmac(struct netsec_priv *priv)

static void netsec_spi_read(char *buf, loff_t len, loff_t offset) { - struct udevice *new; 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); - flash = dev_get_uclass_priv(new); + flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS, CONFIG_SF_DEFAULT_CS, + CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE);

spi_flash_read(flash, offset, len, buf); } diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index f8ec312d71..b6cda6ef99 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -340,9 +340,65 @@ 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, - 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, struct udevice **busp, + struct spi_slave **devp) +{ + struct udevice *bus, *dev; + struct dm_spi_bus *bus_data; + struct spi_slave *slave; + int ret; + +#if CONFIG_IS_ENABLED(OF_PLATDATA) + ret = uclass_first_device_err(UCLASS_SPI, &bus); +#else + ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus); +#endif + if (ret) { + log_err("Invalid bus %d (err=%d)\n", busnum, ret); + return ret; + } + ret = spi_find_chip_select(bus, cs, &dev); + if (ret) { + dev_err(bus, "Invalid chip select %d:%d (err=%d)\n", busnum, cs, ret); + return ret; + } + + if (!device_active(dev)) { + struct spi_slave *slave; + + ret = device_probe(dev); + if (ret) + goto err; + slave = dev_get_parent_priv(dev); + slave->dev = dev; + } + + slave = dev_get_parent_priv(dev); + bus_data = dev_get_uclass_priv(bus); + + /* + * In case the operation speed is not yet established by + * dm_spi_claim_bus() ensure the bus is configured properly. + */ + if (!bus_data->speed) { + ret = spi_claim_bus(slave); + if (ret) + goto err; + } + *busp = bus; + *devp = slave; + + return 0; + +err: + log_debug("%s: Error path, device '%s'\n", __func__, dev->name); + + return ret; +} + +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) { struct udevice *bus, *dev; struct dm_spi_slave_plat *plat; @@ -453,8 +509,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, - &slave); + ret = spi_get_bus_and_cs(busnum, cs, &dev, &slave); if (ret) return NULL;

diff --git a/drivers/usb/gadget/max3420_udc.c b/drivers/usb/gadget/max3420_udc.c index a16095f892..fa655c98dc 100644 --- a/drivers/usb/gadget/max3420_udc.c +++ b/drivers/usb/gadget/max3420_udc.c @@ -830,8 +830,8 @@ 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", - NULL, &spid, &udc->slave); + _spi_get_bus_and_cs(busnum, cs, speed, mode, false, "spi_generic_drv", + NULL, &spid, &udc->slave);

udc->dev = dev; udc->gadget.ep0 = &udc->ep[0].ep_usb; diff --git a/env/sf.c b/env/sf.c index 6a4bb756f0..3467097345 100644 --- a/env/sf.c +++ b/env/sf.c @@ -52,7 +52,6 @@ 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); if (ret) { env_set_default("spi_flash_probe_bus_cs() failed", 0); diff --git a/include/spi.h b/include/spi.h index fa9ab12dbe..9a8c1fb260 100644 --- a/include/spi.h +++ b/include/spi.h @@ -572,6 +572,23 @@ int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, * Given a bus number and chip select, this finds the corresponding bus * device and slave device. * + * @busnum: SPI bus number + * @cs: Chip select to look for + * @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, + struct udevice **busp, struct spi_slave **devp); + +/** + * _spi_get_bus_and_cs() - Find and activate bus and slave devices by number + * As spi_flash_probe(), This is an old-style function. We should remove + * it when all SPI flash drivers use dm + * + * Given a bus number and chip select, this finds the corresponding bus + * device and slave device. + * * If no such slave exists, and drv_name is not NULL, then a new slave device * is automatically bound on this chip select with requested speed and mode. * @@ -588,7 +605,7 @@ int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp, * @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, +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);

diff --git a/include/spi_flash.h b/include/spi_flash.h index d33d0dd46a..10d19fd4b1 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -102,7 +102,6 @@ int spl_flash_get_sw_write_prot(struct udevice *dev); 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);

/* Compatibility function - this is the old U-Boot API */ diff --git a/test/dm/spi.c b/test/dm/spi.c index ee4ad3abaa..cf14b6c900 100644 --- a/test/dm/spi.c +++ b/test/dm/spi.c @@ -46,8 +46,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)); + ut_asserteq(-ENODEV, spi_get_bus_and_cs(busnum, cs, &bus, &slave));

/* * This forces the device to be re-added, but there is no emulation @@ -56,9 +55,9 @@ 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, - "jedec_spi_nor", "name", &bus, - &slave)); + ut_asserteq(-ENOENT, _spi_get_bus_and_cs(busnum, cs, speed, mode, + "jedec_spi_nor", "name", &bus, + &slave)); sandbox_sf_unbind_emul(state_get_current(), busnum, cs); ut_assertok(spi_cs_info(bus, cs, &info)); ut_asserteq_ptr(NULL, info.dev); @@ -67,8 +66,8 @@ 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, - "jedec_spi_nor", "name", &bus, &slave)); + ut_assertok(_spi_get_bus_and_cs(busnum, cs, speed, mode, + "jedec_spi_nor", "name", &bus, &slave));

ut_assertok(spi_cs_info(bus, cs, &info)); ut_asserteq_ptr(info.dev, slave->dev); @@ -76,8 +75,9 @@ static int dm_test_spi_find(struct unit_test_state *uts) /* We should be able to add something to another chip select */ 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)); + ut_asserteq(-EINVAL, _spi_get_bus_and_cs(busnum, cs_b, speed, mode, + "jedec_spi_nor", "name", + &bus, &slave)); ut_asserteq(-EINVAL, spi_cs_info(bus, cs_b, &info)); ut_asserteq_ptr(NULL, info.dev);

@@ -142,14 +142,13 @@ static int dm_test_spi_claim_bus(struct unit_test_state *uts) struct udevice *bus; struct spi_slave *slave_a, *slave_b; struct dm_spi_slave_plat *slave_plat; - const int busnum = 0, cs_a = 0, cs_b = 1, mode = 0; + const int busnum = 0, cs_a = 0, cs_b = 1;

/* Get spi slave on CS0 */ - ut_assertok(spi_get_bus_and_cs(busnum, cs_a, 1000000, mode, NULL, 0, - &bus, &slave_a)); + ut_assertok(spi_get_bus_and_cs(busnum, cs_a, &bus, &slave_a)); + /* Get spi slave on CS1 */ - ut_assertok(spi_get_bus_and_cs(busnum, cs_b, 1000000, mode, NULL, 0, - &bus, &slave_b)); + ut_assertok(spi_get_bus_and_cs(busnum, cs_b, &bus, &slave_b));

/* Different max_hz, different mode. */ ut_assert(slave_a->max_hz != slave_b->max_hz); @@ -178,12 +177,11 @@ static int dm_test_spi_xfer(struct unit_test_state *uts) { struct spi_slave *slave; struct udevice *bus; - const int busnum = 0, cs = 0, mode = 0; + const int busnum = 0, cs = 0; const char dout[5] = {0x9f}; unsigned char din[5];

- ut_assertok(spi_get_bus_and_cs(busnum, cs, 1000000, mode, NULL, 0, - &bus, &slave)); + ut_assertok(spi_get_bus_and_cs(busnum, cs, &bus, &slave)); ut_assertok(spi_claim_bus(slave)); ut_assertok(spi_xfer(slave, 40, dout, din, SPI_XFER_BEGIN | SPI_XFER_END));