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

[neil.armstrong at linaro.org]

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