Speed/mode setting via "sf probe" command

[Ralph Siemsen]

Hi,

The "sf probe" command is documented to take optional speed/mode args:

sf probe [[bus:]cs] [hz] [mode] - init flash device on given SPI bus
                                  and chip select

This worked correctly in older u-boot versions, but as of 2019.07 the 
speed/mode arguments appear to be effectively ignored. As this feature 
is quite useful for testing purposes, I would like to find a way to 
support it again. But the logic is surprisingly complicated, so I would 
like to get some advice/ideas.

The user-specified speed/mode are parsed in do_spi_flash_probe() and 
eventually are passed to spi_get_bus_and_cs(). In 2019.04 and earlier, 
the logic here was pretty straightforward:

        plat = dev_get_parent_platdata(dev);
        if (!speed) {
                speed = plat->max_hz;
                mode = plat->mode;
        }
        ret = spi_set_speed_mode(bus, speed, mode);
        if (ret)
                goto err;

So this calls spi_set_speed_mode() with the user-specified speed, or a 
default value.

As of commit 0cc1b846fcb310c0ac2f8cbeb4ed5947dc52912 ("dm: spi: Read default speed and mode values from DT")
the logic has changed. The user-specified speed value is now stored into 
plat->max_hz, but *only* when no chip select has been configured. In 
practice this means only the first call to spi_get_bus_and_cs() uses the 
speed parameter. Thereafter the speed will not change.

Related commit f7dd5370986087af9b9cfa601f34b344ec910b87 ("spi: prevent overriding established bus settings")
removes the call to spi_set_speed_mode() entirely. So the speed is now 
set by dm_spi_claim_bus() which uses slave->max_hz, which in turn is set 
from plat->max_hz in the spi_child_pre_probe() function.

The first call to spi_get_bus_and_cs() typically occurs when searching 
for u-boot environment, and uses the speed specified in DT. This becomes 
the only speed, as subsequent "sf probe" do not update plat->max_hz.

One potential work-around would be to clear the chip select prior to 
re-binding the device. This allows plat->max_hz to be updated, andalso 
means that device_bind_driver() will be repeateed. However I am not 
entirely certain if this is correct approach. In particular, is using -1 
for the cs appropriate in all cases? It seems it can come from DT.

diff --git a/cmd/sf.c b/cmd/sf.c
index d18f6a888c..8cb70f6487 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -128,6 +128,9 @@ static int do_spi_flash_probe(int argc, char *const argv[])
 	/* Remove the old device, otherwise probe will just be a nop */
 	ret = spi_find_bus_and_cs(bus, cs, &bus_dev, &new);
 	if (!ret) {
+		struct dm_spi_slave_platdata *plat;
+		plat = dev_get_parent_platdata(new);
+		plat->cs = -1;
 		device_remove(new, DM_REMOVE_NORMAL);
 	}
 	flash = NULL;