Dear maintainers,
Any thoughts on this? It seems that "sf probe" behaviour should either
get fixed, or we should remove the "hz" and "mode" arguments entirely,
since they don't work anymore.
Regards,
Ralph
On Thu, Oct 15, 2020 at 12:25:41PM -0400, Ralph Siemsen wrote:
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;
