| Message ID | 20240917-uboot-topic-dfu-sf-dt-v1-1-8cf38451eea4@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dfu: sf: rely on DT for spi speed and mode | expand |
Hi Neil, Thank you for the patch and sorry the review delay. On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: > Align with cmd_sf, and try to rely on DT for spi speed and mode, > and still fallback on spi_flash_probe() if it fails. > > With the current scheme, spi_flash_probe() will be called > with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE > with are set to 0 by default on DT platforms using DM_SPI_FLASH. > > Like cmd_sf, keep the option to specify the speed and mode > from the dfu_alt_mode string, but rely on DT properties > if not specified. > > Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE > makes the SPIFC controller on Amlogic Meson G12B & SM1 > hardware fail and is unable to recover until a system reboot. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c > index 7c1c0f9e2dc..b5d875be5ea 100644 > --- a/drivers/dfu/dfu_sf.c > +++ b/drivers/dfu/dfu_sf.c > @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) > unsigned int mode = CONFIG_SF_DEFAULT_MODE; > char *s, *endp; > struct spi_flash *dev; > +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) > + bool use_dt = true; > +#endif Can we use if (IS_ENABLED(CONFIG...)) for this logic instead of #if ? checkpatch.pl seems to warn about this: $ ./scripts/checkpatch.pl --u-boot --git HEAD^..HEAD WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible #36: FILE: drivers/dfu/dfu_sf.c:126: +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) I know we will have an unused local variable (use_dt) when DM_SPI_FLASH=n but to me that is fine. We already have unused local variables based on KConfig symbols in dfu_flush_medium_sf() Thanks, Mattijs > > s = strsep(&devstr, ":"); > if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { > @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) > printf("Invalid SPI speed %s\n", s); > return NULL; > } > +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) > + use_dt = false; > +#endif > } > > s = strsep(&devstr, ":"); > @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) > printf("Invalid SPI mode %s\n", s); > return NULL; > } > +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) > + use_dt = false; > +#endif > } > > +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) > + if (use_dt) { > + struct udevice *new; > + > + if (!spi_flash_probe_bus_cs(bus, cs, &new)) > + dev = dev_get_uclass_priv(new); > + else > + dev = NULL; > + } else { > + dev = spi_flash_probe(bus, cs, speed, mode); > + } > +#else > dev = spi_flash_probe(bus, cs, speed, mode); > +#endif > if (!dev) { > printf("Failed to create SPI flash at %u:%u:%u:%u\n", > bus, cs, speed, mode); > > --- > base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d > change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 > > Best regards, > -- > Neil Armstrong <neil.armstrong@linaro.org>
On 01/10/2024 10:52, Mattijs Korpershoek wrote: > Hi Neil, > > Thank you for the patch and sorry the review delay. > > On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> Align with cmd_sf, and try to rely on DT for spi speed and mode, >> and still fallback on spi_flash_probe() if it fails. >> >> With the current scheme, spi_flash_probe() will be called >> with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >> with are set to 0 by default on DT platforms using DM_SPI_FLASH. >> >> Like cmd_sf, keep the option to specify the speed and mode >> from the dfu_alt_mode string, but rely on DT properties >> if not specified. >> >> Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >> makes the SPIFC controller on Amlogic Meson G12B & SM1 >> hardware fail and is unable to recover until a system reboot. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c >> index 7c1c0f9e2dc..b5d875be5ea 100644 >> --- a/drivers/dfu/dfu_sf.c >> +++ b/drivers/dfu/dfu_sf.c >> @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) >> unsigned int mode = CONFIG_SF_DEFAULT_MODE; >> char *s, *endp; >> struct spi_flash *dev; >> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >> + bool use_dt = true; >> +#endif > > Can we use if (IS_ENABLED(CONFIG...)) for this logic instead of #if ? > > checkpatch.pl seems to warn about this: > > $ ./scripts/checkpatch.pl --u-boot --git HEAD^..HEAD > WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible > #36: FILE: drivers/dfu/dfu_sf.c:126: > +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) > > I know we will have an unused local variable (use_dt) when > DM_SPI_FLASH=n but to me that is fine. > We already have unused local variables based on KConfig symbols in > dfu_flush_medium_sf() Sure, let me check ! Neil > > Thanks, > Mattijs > >> >> s = strsep(&devstr, ":"); >> if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { >> @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) >> printf("Invalid SPI speed %s\n", s); >> return NULL; >> } >> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >> + use_dt = false; >> +#endif >> } >> >> s = strsep(&devstr, ":"); >> @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) >> printf("Invalid SPI mode %s\n", s); >> return NULL; >> } >> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >> + use_dt = false; >> +#endif >> } >> >> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >> + if (use_dt) { >> + struct udevice *new; >> + >> + if (!spi_flash_probe_bus_cs(bus, cs, &new)) >> + dev = dev_get_uclass_priv(new); >> + else >> + dev = NULL; >> + } else { >> + dev = spi_flash_probe(bus, cs, speed, mode); >> + } >> +#else >> dev = spi_flash_probe(bus, cs, speed, mode); >> +#endif >> if (!dev) { >> printf("Failed to create SPI flash at %u:%u:%u:%u\n", >> bus, cs, speed, mode); >> >> --- >> base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d >> change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 >> >> Best regards, >> -- >> Neil Armstrong <neil.armstrong@linaro.org>
On 01/10/2024 12:01, Neil Armstrong wrote: > On 01/10/2024 10:52, Mattijs Korpershoek wrote: >> Hi Neil, >> >> Thank you for the patch and sorry the review delay. >> >> On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >>> Align with cmd_sf, and try to rely on DT for spi speed and mode, >>> and still fallback on spi_flash_probe() if it fails. >>> >>> With the current scheme, spi_flash_probe() will be called >>> with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>> with are set to 0 by default on DT platforms using DM_SPI_FLASH. >>> >>> Like cmd_sf, keep the option to specify the speed and mode >>> from the dfu_alt_mode string, but rely on DT properties >>> if not specified. >>> >>> Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>> makes the SPIFC controller on Amlogic Meson G12B & SM1 >>> hardware fail and is unable to recover until a system reboot. >>> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c >>> index 7c1c0f9e2dc..b5d875be5ea 100644 >>> --- a/drivers/dfu/dfu_sf.c >>> +++ b/drivers/dfu/dfu_sf.c >>> @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) >>> unsigned int mode = CONFIG_SF_DEFAULT_MODE; >>> char *s, *endp; >>> struct spi_flash *dev; >>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>> + bool use_dt = true; >>> +#endif >> >> Can we use if (IS_ENABLED(CONFIG...)) for this logic instead of #if ? >> >> checkpatch.pl seems to warn about this: >> >> $ ./scripts/checkpatch.pl --u-boot --git HEAD^..HEAD >> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >> #36: FILE: drivers/dfu/dfu_sf.c:126: >> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >> >> I know we will have an unused local variable (use_dt) when >> DM_SPI_FLASH=n but to me that is fine. >> We already have unused local variables based on KConfig symbols in >> dfu_flush_medium_sf() > > Sure, let me check ! OK so there's: u-boot/drivers/dfu/dfu_sf.c: In function ‘parse_dev’: u-boot-upstream/u-boot/drivers/dfu/dfu_sf.c:163:8: warning: implicit declaration of function ‘spi_flash_probe_bus_cs’; did you mean ‘spi_flash_protect’? [-Wimplicit-function-declaration] 163 | if (!spi_flash_probe_bus_cs(bus, cs, &new)) | ^~~~~~~~~~~~~~~~~~~~~~ | spi_flash_protect because there's no dummy spi_flash_probe_bus_cs fallback when !DM_SPI_FLASH Neil > > Neil > >> >> Thanks, >> Mattijs >> >>> s = strsep(&devstr, ":"); >>> if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { >>> @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) >>> printf("Invalid SPI speed %s\n", s); >>> return NULL; >>> } >>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>> + use_dt = false; >>> +#endif >>> } >>> s = strsep(&devstr, ":"); >>> @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) >>> printf("Invalid SPI mode %s\n", s); >>> return NULL; >>> } >>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>> + use_dt = false; >>> +#endif >>> } >>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>> + if (use_dt) { >>> + struct udevice *new; >>> + >>> + if (!spi_flash_probe_bus_cs(bus, cs, &new)) >>> + dev = dev_get_uclass_priv(new); >>> + else >>> + dev = NULL; >>> + } else { >>> + dev = spi_flash_probe(bus, cs, speed, mode); >>> + } >>> +#else >>> dev = spi_flash_probe(bus, cs, speed, mode); >>> +#endif >>> if (!dev) { >>> printf("Failed to create SPI flash at %u:%u:%u:%u\n", >>> bus, cs, speed, mode); >>> >>> --- >>> base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d >>> change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 >>> >>> Best regards, >>> -- >>> Neil Armstrong <neil.armstrong@linaro.org> >
On mar., oct. 01, 2024 at 12:13, Neil Armstrong <neil.armstrong@linaro.org> wrote: > On 01/10/2024 12:01, Neil Armstrong wrote: >> On 01/10/2024 10:52, Mattijs Korpershoek wrote: >>> Hi Neil, >>> >>> Thank you for the patch and sorry the review delay. >>> >>> On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>> >>>> Align with cmd_sf, and try to rely on DT for spi speed and mode, >>>> and still fallback on spi_flash_probe() if it fails. >>>> >>>> With the current scheme, spi_flash_probe() will be called >>>> with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>>> with are set to 0 by default on DT platforms using DM_SPI_FLASH. >>>> >>>> Like cmd_sf, keep the option to specify the speed and mode >>>> from the dfu_alt_mode string, but rely on DT properties >>>> if not specified. >>>> >>>> Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>>> makes the SPIFC controller on Amlogic Meson G12B & SM1 >>>> hardware fail and is unable to recover until a system reboot. >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c >>>> index 7c1c0f9e2dc..b5d875be5ea 100644 >>>> --- a/drivers/dfu/dfu_sf.c >>>> +++ b/drivers/dfu/dfu_sf.c >>>> @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) >>>> unsigned int mode = CONFIG_SF_DEFAULT_MODE; >>>> char *s, *endp; >>>> struct spi_flash *dev; >>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>> + bool use_dt = true; >>>> +#endif >>> >>> Can we use if (IS_ENABLED(CONFIG...)) for this logic instead of #if ? >>> >>> checkpatch.pl seems to warn about this: >>> >>> $ ./scripts/checkpatch.pl --u-boot --git HEAD^..HEAD >>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >>> #36: FILE: drivers/dfu/dfu_sf.c:126: >>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>> >>> I know we will have an unused local variable (use_dt) when >>> DM_SPI_FLASH=n but to me that is fine. >>> We already have unused local variables based on KConfig symbols in >>> dfu_flush_medium_sf() >> >> Sure, let me check ! > > OK so there's: > u-boot/drivers/dfu/dfu_sf.c: In function ‘parse_dev’: > u-boot-upstream/u-boot/drivers/dfu/dfu_sf.c:163:8: warning: implicit declaration of function ‘spi_flash_probe_bus_cs’; did you mean ‘spi_flash_protect’? [-Wimplicit-function-declaration] > 163 | if (!spi_flash_probe_bus_cs(bus, cs, &new)) > | ^~~~~~~~~~~~~~~~~~~~~~ > | spi_flash_protect > > because there's no dummy spi_flash_probe_bus_cs fallback when !DM_SPI_FLASH Ok, Can we add a dummy function? If you'd prefer not to, we can keep the #if block for that part and use if (IS_ENABLED) on the other 3 occurences. > > Neil >> >> Neil >> >>> >>> Thanks, >>> Mattijs >>> >>>> s = strsep(&devstr, ":"); >>>> if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { >>>> @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) >>>> printf("Invalid SPI speed %s\n", s); >>>> return NULL; >>>> } >>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>> + use_dt = false; >>>> +#endif >>>> } >>>> s = strsep(&devstr, ":"); >>>> @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) >>>> printf("Invalid SPI mode %s\n", s); >>>> return NULL; >>>> } >>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>> + use_dt = false; >>>> +#endif >>>> } >>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>> + if (use_dt) { >>>> + struct udevice *new; >>>> + >>>> + if (!spi_flash_probe_bus_cs(bus, cs, &new)) >>>> + dev = dev_get_uclass_priv(new); >>>> + else >>>> + dev = NULL; >>>> + } else { >>>> + dev = spi_flash_probe(bus, cs, speed, mode); >>>> + } >>>> +#else >>>> dev = spi_flash_probe(bus, cs, speed, mode); >>>> +#endif >>>> if (!dev) { >>>> printf("Failed to create SPI flash at %u:%u:%u:%u\n", >>>> bus, cs, speed, mode); >>>> >>>> --- >>>> base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d >>>> change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 >>>> >>>> Best regards, >>>> -- >>>> Neil Armstrong <neil.armstrong@linaro.org> >>
On 01/10/2024 14:02, Mattijs Korpershoek wrote: > On mar., oct. 01, 2024 at 12:13, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> On 01/10/2024 12:01, Neil Armstrong wrote: >>> On 01/10/2024 10:52, Mattijs Korpershoek wrote: >>>> Hi Neil, >>>> >>>> Thank you for the patch and sorry the review delay. >>>> >>>> On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>>> >>>>> Align with cmd_sf, and try to rely on DT for spi speed and mode, >>>>> and still fallback on spi_flash_probe() if it fails. >>>>> >>>>> With the current scheme, spi_flash_probe() will be called >>>>> with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>>>> with are set to 0 by default on DT platforms using DM_SPI_FLASH. >>>>> >>>>> Like cmd_sf, keep the option to specify the speed and mode >>>>> from the dfu_alt_mode string, but rely on DT properties >>>>> if not specified. >>>>> >>>>> Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE >>>>> makes the SPIFC controller on Amlogic Meson G12B & SM1 >>>>> hardware fail and is unable to recover until a system reboot. >>>>> >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c >>>>> index 7c1c0f9e2dc..b5d875be5ea 100644 >>>>> --- a/drivers/dfu/dfu_sf.c >>>>> +++ b/drivers/dfu/dfu_sf.c >>>>> @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) >>>>> unsigned int mode = CONFIG_SF_DEFAULT_MODE; >>>>> char *s, *endp; >>>>> struct spi_flash *dev; >>>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>>> + bool use_dt = true; >>>>> +#endif >>>> >>>> Can we use if (IS_ENABLED(CONFIG...)) for this logic instead of #if ? >>>> >>>> checkpatch.pl seems to warn about this: >>>> >>>> $ ./scripts/checkpatch.pl --u-boot --git HEAD^..HEAD >>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible >>>> #36: FILE: drivers/dfu/dfu_sf.c:126: >>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>> >>>> I know we will have an unused local variable (use_dt) when >>>> DM_SPI_FLASH=n but to me that is fine. >>>> We already have unused local variables based on KConfig symbols in >>>> dfu_flush_medium_sf() >>> >>> Sure, let me check ! >> >> OK so there's: >> u-boot/drivers/dfu/dfu_sf.c: In function ‘parse_dev’: >> u-boot-upstream/u-boot/drivers/dfu/dfu_sf.c:163:8: warning: implicit declaration of function ‘spi_flash_probe_bus_cs’; did you mean ‘spi_flash_protect’? [-Wimplicit-function-declaration] >> 163 | if (!spi_flash_probe_bus_cs(bus, cs, &new)) >> | ^~~~~~~~~~~~~~~~~~~~~~ >> | spi_flash_protect >> >> because there's no dummy spi_flash_probe_bus_cs fallback when !DM_SPI_FLASH > > Ok, > Can we add a dummy function? We can, not sure about the strategy of the SPI FLASH Driver Model, but it should be the default at some point, so let me add it. Neil > > If you'd prefer not to, we can keep the #if block for that part and > use if (IS_ENABLED) on the other 3 occurences. > >> >> Neil >>> >>> Neil >>> >>>> >>>> Thanks, >>>> Mattijs >>>> >>>>> s = strsep(&devstr, ":"); >>>>> if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { >>>>> @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) >>>>> printf("Invalid SPI speed %s\n", s); >>>>> return NULL; >>>>> } >>>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>>> + use_dt = false; >>>>> +#endif >>>>> } >>>>> s = strsep(&devstr, ":"); >>>>> @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) >>>>> printf("Invalid SPI mode %s\n", s); >>>>> return NULL; >>>>> } >>>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>>> + use_dt = false; >>>>> +#endif >>>>> } >>>>> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) >>>>> + if (use_dt) { >>>>> + struct udevice *new; >>>>> + >>>>> + if (!spi_flash_probe_bus_cs(bus, cs, &new)) >>>>> + dev = dev_get_uclass_priv(new); >>>>> + else >>>>> + dev = NULL; >>>>> + } else { >>>>> + dev = spi_flash_probe(bus, cs, speed, mode); >>>>> + } >>>>> +#else >>>>> dev = spi_flash_probe(bus, cs, speed, mode); >>>>> +#endif >>>>> if (!dev) { >>>>> printf("Failed to create SPI flash at %u:%u:%u:%u\n", >>>>> bus, cs, speed, mode); >>>>> >>>>> --- >>>>> base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d >>>>> change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 >>>>> >>>>> Best regards, >>>>> -- >>>>> Neil Armstrong <neil.armstrong@linaro.org> >>>
diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c index 7c1c0f9e2dc..b5d875be5ea 100644 --- a/drivers/dfu/dfu_sf.c +++ b/drivers/dfu/dfu_sf.c @@ -123,6 +123,9 @@ static struct spi_flash *parse_dev(char *devstr) unsigned int mode = CONFIG_SF_DEFAULT_MODE; char *s, *endp; struct spi_flash *dev; +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) + bool use_dt = true; +#endif s = strsep(&devstr, ":"); if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) { @@ -143,6 +146,9 @@ static struct spi_flash *parse_dev(char *devstr) printf("Invalid SPI speed %s\n", s); return NULL; } +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) + use_dt = false; +#endif } s = strsep(&devstr, ":"); @@ -152,9 +158,25 @@ static struct spi_flash *parse_dev(char *devstr) printf("Invalid SPI mode %s\n", s); return NULL; } +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) + use_dt = false; +#endif } +#if CONFIG_IS_ENABLED(DM_SPI_FLASH) + if (use_dt) { + struct udevice *new; + + if (!spi_flash_probe_bus_cs(bus, cs, &new)) + dev = dev_get_uclass_priv(new); + else + dev = NULL; + } else { + dev = spi_flash_probe(bus, cs, speed, mode); + } +#else dev = spi_flash_probe(bus, cs, speed, mode); +#endif if (!dev) { printf("Failed to create SPI flash at %u:%u:%u:%u\n", bus, cs, speed, mode);
Align with cmd_sf, and try to rely on DT for spi speed and mode, and still fallback on spi_flash_probe() if it fails. With the current scheme, spi_flash_probe() will be called with CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE with are set to 0 by default on DT platforms using DM_SPI_FLASH. Like cmd_sf, keep the option to specify the speed and mode from the dfu_alt_mode string, but rely on DT properties if not specified. Using CONFIG_SF_DEFAULT_SPEED and CONFIG_SF_DEFAULT_MODE makes the SPIFC controller on Amlogic Meson G12B & SM1 hardware fail and is unable to recover until a system reboot. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/dfu/dfu_sf.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) --- base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79 Best regards,