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
signature.asc
Description: PGP signature
