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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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>
>>