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