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

[Neil Armstrong]

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

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