[PATCH] dfu: sf: rely on DT for spi speed and 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(+)
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);
--- base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79
Best regards,
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);elsedev = 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);elsedev = 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
participants (3)
-
Mattijs Korpershoek -
Neil Armstrong -
neil.armstrong＠linaro.org