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