* [PATCH] dfu: sf: rely on DT for spi speed and mode
@ 2024-09-17 12:24 Neil Armstrong
2024-10-01 8:52 ` Mattijs Korpershoek
0 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2024-09-17 12:24 UTC (permalink / raw)
To: Lukasz Majewski, Mattijs Korpershoek, Tom Rini; +Cc: u-boot, Neil Armstrong
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,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] dfu: sf: rely on DT for spi speed and mode
2024-09-17 12:24 [PATCH] dfu: sf: rely on DT for spi speed and mode Neil Armstrong
@ 2024-10-01 8:52 ` Mattijs Korpershoek
2024-10-01 10:01 ` Neil Armstrong
0 siblings, 1 reply; 6+ messages in thread
From: Mattijs Korpershoek @ 2024-10-01 8:52 UTC (permalink / raw)
To: Neil Armstrong, Lukasz Majewski, Tom Rini; +Cc: u-boot, Neil Armstrong
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dfu: sf: rely on DT for spi speed and mode
2024-10-01 8:52 ` Mattijs Korpershoek
@ 2024-10-01 10:01 ` Neil Armstrong
2024-10-01 10:13 ` Neil Armstrong
0 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2024-10-01 10:01 UTC (permalink / raw)
To: Mattijs Korpershoek, Lukasz Majewski, Tom Rini; +Cc: u-boot
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);
>> + 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dfu: sf: rely on DT for spi speed and mode
2024-10-01 10:01 ` Neil Armstrong
@ 2024-10-01 10:13 ` Neil Armstrong
2024-10-01 12:02 ` Mattijs Korpershoek
0 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2024-10-01 10:13 UTC (permalink / raw)
To: Mattijs Korpershoek, Lukasz Majewski, Tom Rini; +Cc: u-boot
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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dfu: sf: rely on DT for spi speed and mode
2024-10-01 10:13 ` Neil Armstrong
@ 2024-10-01 12:02 ` Mattijs Korpershoek
2024-10-01 14:09 ` neil.armstrong
0 siblings, 1 reply; 6+ messages in thread
From: Mattijs Korpershoek @ 2024-10-01 12:02 UTC (permalink / raw)
To: neil.armstrong, Lukasz Majewski, Tom Rini; +Cc: u-boot
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>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dfu: sf: rely on DT for spi speed and mode
2024-10-01 12:02 ` Mattijs Korpershoek
@ 2024-10-01 14:09 ` neil.armstrong
0 siblings, 0 replies; 6+ messages in thread
From: neil.armstrong @ 2024-10-01 14:09 UTC (permalink / raw)
To: Mattijs Korpershoek, Lukasz Majewski, Tom Rini; +Cc: u-boot
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>
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-01 14:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-17 12:24 [PATCH] dfu: sf: rely on DT for spi speed and mode Neil Armstrong
2024-10-01 8:52 ` Mattijs Korpershoek
2024-10-01 10:01 ` Neil Armstrong
2024-10-01 10:13 ` Neil Armstrong
2024-10-01 12:02 ` Mattijs Korpershoek
2024-10-01 14:09 ` neil.armstrong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox