| Message ID | 20201015162541.GA24746@maple.netwinder.org |
|---|---|
| State | Rejected |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | Speed/mode setting via "sf probe" command | expand |
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;
On Wed, Oct 28, 2020 at 11:46:07AM -0400, Ralph Siemsen wrote: > 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. This does sound like something that should be fixed, yes. > > 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;
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;
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.