diff mbox series

Speed/mode setting via "sf probe" command

Message ID 20201015162541.GA24746@maple.netwinder.org
State New
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series Speed/mode setting via "sf probe" command | expand

Commit Message

Ralph Siemsen Oct. 15, 2020, 4:25 p.m. UTC
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.

Comments

Ralph Siemsen Oct. 28, 2020, 3:46 p.m. UTC | #1
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;
Tom Rini Nov. 3, 2020, 4:19 p.m. UTC | #2
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 mbox series

Patch

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;