Re: [PATCH] dfu: sf: rely on DT for spi speed and mode

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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>