Speed/mode setting via "sf probe" command

[Tom Rini]

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;

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201103/3f41377c/attachment.sig>