On 01/10/2024 14:02, Mattijs Korpershoek wrote:
On mar., oct. 01, 2024 at 12:13, Neil Armstrong <neil.armstr...@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.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()
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.armstr...@linaro.org>
