[PATCH v2 0/2] dfu: sf: fix flash probe when DM_SPI_FLASH is enabled

[PATCH v2 0/2] dfu: sf: fix flash probe when DM_SPI_FLASH is enabled

[Neil Armstrong]

With DM_SPI_FLASH is enabled, the code uses the legacy
SPI FLASH code leading to probable errors since it doesn't
use speed and mode provided by DT.

This adds the DM functions as dummy inline functions
to add both legacy and DM support in DFU sf code avoiding
using #if/#else conditionals.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v2:
- switch to if(IS_ENABLED())
- add SPI FLASH DM functions as dummy inline functions
- Link to v1: https://lore.kernel.org/r/20240917-uboot-topic-dfu-sf-dt-v1-1-8cf38451eea4@linaro.org

---
Neil Armstrong (2):
      spi: add DM_SPI_FLASH compatibility inline functions
      dfu: sf: rely on DT for spi speed and mode

 drivers/dfu/dfu_sf.c | 16 +++++++++++++++-
 include/spi_flash.h  | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
---
base-commit: 19dbc09405d3503ce3efef3c2e4b4f0f1a03372d
change-id: 20240917-uboot-topic-dfu-sf-dt-8ae62e5c7d79

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v2 1/2] spi: add DM_SPI_FLASH compatibility inline functions

[Neil Armstrong]

To smoothly handle the transition from the legacy SPI FLASH
API to the the driver model API, add the DM functions
as dummy inline functions.

Today, client code uses #if/#else conditionals, but it's better
to use if(IS_ENABLED()) to make sure all code builds fine
and avoid configuration hell, leaving the compiler remove
the dead code.

An example is cmd/sf, which could make use of those dummy
functions to drop the conditional compilation.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 include/spi_flash.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/spi_flash.h b/include/spi_flash.h
index 10d19fd4b11..2e703e85250 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -139,6 +139,40 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
 void sandbox_sf_unbind_emul(struct sandbox_state *state, int busnum, int cs);
 
 #else
+/* Compatibility functions for when DM_SPI_FLASH is disabled */
+static inline int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
+					 struct udevice **devp)
+{
+	return -ENODEV;
+}
+
+static inline int spi_flash_read_dm(struct udevice *dev, u32 offset, size_t len,
+				    void *buf)
+{
+	return -ENODEV;
+}
+
+static inline int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,
+				     const void *buf)
+{
+	return -ENODEV;
+}
+
+static inline int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len)
+{
+	return -ENODEV;
+}
+
+static inline int spl_flash_get_sw_write_prot(struct udevice *dev)
+{
+	return -ENODEV;
+}
+
+static inline int spi_flash_std_probe(struct udevice *dev)
+{
+	return -ENODEV;
+}
+
 struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
 		unsigned int max_hz, unsigned int spi_mode);
 

-- 
2.34.1

Re: [PATCH v2 1/2] spi: add DM_SPI_FLASH compatibility inline functions

[Mattijs Korpershoek]

Hi Neil,

Thank you for the patch.

On mar., oct. 01, 2024 at 18:06, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> To smoothly handle the transition from the legacy SPI FLASH
> API to the the driver model API, add the DM functions
> as dummy inline functions.
>
> Today, client code uses #if/#else conditionals, but it's better
> to use if(IS_ENABLED()) to make sure all code builds fine
> and avoid configuration hell, leaving the compiler remove
> the dead code.
>
> An example is cmd/sf, which could make use of those dummy
> functions to drop the conditional compilation.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  include/spi_flash.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/include/spi_flash.h b/include/spi_flash.h
> index 10d19fd4b11..2e703e85250 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -139,6 +139,40 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
>  void sandbox_sf_unbind_emul(struct sandbox_state *state, int busnum, int cs);
>  
>  #else
> +/* Compatibility functions for when DM_SPI_FLASH is disabled */
> +static inline int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
> +					 struct udevice **devp)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_read_dm(struct udevice *dev, u32 offset, size_t len,
> +				    void *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,
> +				     const void *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spl_flash_get_sw_write_prot(struct udevice *dev)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_std_probe(struct udevice *dev)
> +{
> +	return -ENODEV;
> +}
> +
>  struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
>  		unsigned int max_hz, unsigned int spi_mode);
>  
>
> -- 
> 2.34.1

Re: [PATCH v2 1/2] spi: add DM_SPI_FLASH compatibility inline functions

[Mattijs Korpershoek]

Hi Jagan, Vignesh,

On mar., oct. 01, 2024 at 18:06, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> To smoothly handle the transition from the legacy SPI FLASH
> API to the the driver model API, add the DM functions
> as dummy inline functions.
>
> Today, client code uses #if/#else conditionals, but it's better
> to use if(IS_ENABLED()) to make sure all code builds fine
> and avoid configuration hell, leaving the compiler remove
> the dead code.
>
> An example is cmd/sf, which could make use of those dummy
> functions to drop the conditional compilation.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

I'd like to submit this through u-boot-dfu but before that i'd prefer to
have an Acked-by from one of you.

Any opinion on this ?

Thank you for your time!

> ---
>  include/spi_flash.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/include/spi_flash.h b/include/spi_flash.h
> index 10d19fd4b11..2e703e85250 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -139,6 +139,40 @@ int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
>  void sandbox_sf_unbind_emul(struct sandbox_state *state, int busnum, int cs);
>  
>  #else
> +/* Compatibility functions for when DM_SPI_FLASH is disabled */
> +static inline int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
> +					 struct udevice **devp)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_read_dm(struct udevice *dev, u32 offset, size_t len,
> +				    void *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,
> +				     const void *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spl_flash_get_sw_write_prot(struct udevice *dev)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline int spi_flash_std_probe(struct udevice *dev)
> +{
> +	return -ENODEV;
> +}
> +
>  struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs,
>  		unsigned int max_hz, unsigned int spi_mode);
>  
>
> -- 
> 2.34.1

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

[Neil Armstrong]

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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
index 7c1c0f9e2dc..8f7296adec6 100644
--- a/drivers/dfu/dfu_sf.c
+++ b/drivers/dfu/dfu_sf.c
@@ -123,6 +123,7 @@ static struct spi_flash *parse_dev(char *devstr)
 	unsigned int mode = CONFIG_SF_DEFAULT_MODE;
 	char *s, *endp;
 	struct spi_flash *dev;
+	bool use_dt = true;
 
 	s = strsep(&devstr, ":");
 	if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) {
@@ -143,6 +144,8 @@ static struct spi_flash *parse_dev(char *devstr)
 			printf("Invalid SPI speed %s\n", s);
 			return NULL;
 		}
+		if (IS_ENABLED(CONFIG_DM_SPI_FLASH))
+			use_dt = false;
 	}
 
 	s = strsep(&devstr, ":");
@@ -152,9 +155,20 @@ static struct spi_flash *parse_dev(char *devstr)
 			printf("Invalid SPI mode %s\n", s);
 			return NULL;
 		}
+		if (IS_ENABLED(CONFIG_DM_SPI_FLASH))
+			use_dt = false;
 	}
 
-	dev = spi_flash_probe(bus, cs, speed, mode);
+	if (IS_ENABLED(CONFIG_DM_SPI_FLASH) && 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);
+	}
 	if (!dev) {
 		printf("Failed to create SPI flash at %u:%u:%u:%u\n",
 		       bus, cs, speed, mode);

-- 
2.34.1

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

[Mattijs Korpershoek]

Hi Neil,

Thank you for the patch.

On mar., oct. 01, 2024 at 18:06, 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>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/dfu/dfu_sf.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dfu/dfu_sf.c b/drivers/dfu/dfu_sf.c
> index 7c1c0f9e2dc..8f7296adec6 100644
> --- a/drivers/dfu/dfu_sf.c
> +++ b/drivers/dfu/dfu_sf.c
> @@ -123,6 +123,7 @@ static struct spi_flash *parse_dev(char *devstr)
>  	unsigned int mode = CONFIG_SF_DEFAULT_MODE;
>  	char *s, *endp;
>  	struct spi_flash *dev;
> +	bool use_dt = true;
>  
>  	s = strsep(&devstr, ":");
>  	if (!s || !*s || (bus = simple_strtoul(s, &endp, 0), *endp)) {
> @@ -143,6 +144,8 @@ static struct spi_flash *parse_dev(char *devstr)
>  			printf("Invalid SPI speed %s\n", s);
>  			return NULL;
>  		}
> +		if (IS_ENABLED(CONFIG_DM_SPI_FLASH))
> +			use_dt = false;
>  	}
>  
>  	s = strsep(&devstr, ":");
> @@ -152,9 +155,20 @@ static struct spi_flash *parse_dev(char *devstr)
>  			printf("Invalid SPI mode %s\n", s);
>  			return NULL;
>  		}
> +		if (IS_ENABLED(CONFIG_DM_SPI_FLASH))
> +			use_dt = false;
>  	}
>  
> -	dev = spi_flash_probe(bus, cs, speed, mode);
> +	if (IS_ENABLED(CONFIG_DM_SPI_FLASH) && 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);
> +	}
>  	if (!dev) {
>  		printf("Failed to create SPI flash at %u:%u:%u:%u\n",
>  		       bus, cs, speed, mode);
>
> -- 
> 2.34.1

Re: [PATCH v2 0/2] dfu: sf: fix flash probe when DM_SPI_FLASH is enabled

[Mattijs Korpershoek]

Hi,

On Tue, 01 Oct 2024 18:06:10 +0200, Neil Armstrong wrote:
> With DM_SPI_FLASH is enabled, the code uses the legacy
> SPI FLASH code leading to probable errors since it doesn't
> use speed and mode provided by DT.
> 
> This adds the DM functions as dummy inline functions
> to add both legacy and DM support in DFU sf code avoiding
> using #if/#else conditionals.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/2] spi: add DM_SPI_FLASH compatibility inline functions
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/0872ac098a400632f02ae442dde65d77cc7eb1cb
[2/2] dfu: sf: rely on DT for spi speed and mode
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/97c29f868dd4e6d8f38d1cfdd964fb6b2b40267c

--
Mattijs