Hi Neil,
Thank you for the patch and sorry the review delay.
On mar., sept. 17, 2024 at 14:24, Neil Armstrong <neil.armstr...@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.armstr...@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.armstr...@linaro.org>
