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

[Neil Armstrong]

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 !

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>