External flash Write Protect and Hold pins

[J.P. Hutchins]

Hello!  The only Zephyr flash driver I see that uses the wp_gpios pin is spi_flash_at45.c.  It seems that none of them use hold_gpios.

Presently, for jedec,spi-nor compatible, we are using spi_nor.c.

It seems that in the simple case, spi_nor.c should be patched to set both pins as GPIO_OUTPUT_INACTIVE (typically high for these wp and hold pins).

Next, it's reasonable to initialize WP active and only de-assert during flash writes.  Could still be done with spi_nor.c simply following the model from spi_flash_at45.c.

For now, I'd like to initialize hold as inactive and not use it within the driver.

Finally, if the external flash is on a power domain, both of these pins should be set to DISCONNECTED when the power domain goes OFF.

Does this sound like a reasonable patch to spi_nor.c?

Cheers,
J.P.

[J.P. Hutchins]

diff --git a/drivers/flash/spi_nor.c b/drivers/flash/spi_nor.c
index fd2701d23a..4266992999 100644
--- a/drivers/flash/spi_nor.c
+++ b/drivers/flash/spi_nor.c
@@ -12,6 +12,8 @@
 #include <errno.h>
 #include <zephyr/drivers/flash.h>
 #include <zephyr/drivers/spi.h>
+#include <zephyr/pm/device.h>
+#include <zephyr/pm/device_runtime.h>
 #include <zephyr/init.h>
 #include <string.h>
 #include <zephyr/logging/log.h>
@@ -61,6 +63,12 @@ LOG_MODULE_REGISTER(spi_nor, CONFIG_FLASH_LOG_LEVEL);
 #define T_DPDD_MS 0
 #endif /* DPD_WAKEUP_SEQUENCE */
 
+#define _INST_HAS_WP_OR(inst) DT_INST_NODE_HAS_PROP(inst, wp_gpios) ||
+#define ANY_INST_HAS_WP_GPIOS DT_INST_FOREACH_STATUS_OKAY(_INST_HAS_WP_OR) 0
+
+#define _INST_HAS_HOLD_OR(inst) DT_INST_NODE_HAS_PROP(inst, hold_gpios) ||
+#define ANY_INST_HAS_HOLD_GPIOS DT_INST_FOREACH_STATUS_OKAY(_INST_HAS_HOLD_OR) 0
+
 /* Build-time data associated with the device. */
 struct spi_nor_config {
 	/* Devicetree SPI configuration */
@@ -101,6 +109,12 @@ struct spi_nor_config {
 	 * This information cannot be derived from SFDP.
 	 */
 	uint8_t has_lock;
+#if ANY_INST_HAS_WP_GPIOS
+	const struct gpio_dt_spec *wp;
+#endif
+#if ANY_INST_HAS_HOLD_GPIOS
+	const struct gpio_dt_spec *hold;
+#endif
 };
 
 /**
@@ -470,6 +484,11 @@ static void acquire_device(const struct device *dev)
 		k_sem_take(&driver_data->sem, K_FOREVER);
 	}
 
+	if (IS_ENABLED(CONFIG_PM_DEVICE_RUNTIME) && pm_device_runtime_is_enabled(dev)) {
+		int err = pm_device_runtime_get(dev);
+		__ASSERT(err == 0, "PM device error: [%d]", err);
+	}
+
 	if (IS_ENABLED(CONFIG_SPI_NOR_IDLE_IN_DPD)) {
 		exit_dpd(dev);
 	}
@@ -486,6 +505,11 @@ static void release_device(const struct device *dev)
 		enter_dpd(dev);
 	}
 
+	if (IS_ENABLED(CONFIG_PM_DEVICE_RUNTIME) && pm_device_runtime_is_enabled(dev)) {
+		int err = pm_device_runtime_put(dev);
+		__ASSERT(err == 0, "PM device error: [%d]", err);
+	}
+
 	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
 		struct spi_nor_data *const driver_data = dev->data;
 
@@ -821,8 +845,17 @@ static int spi_nor_erase(const struct device *dev, off_t addr, size_t size)
 static int spi_nor_write_protection_set(const struct device *dev,
 					bool write_protect)
 {
+#if ANY_INST_HAS_WP_GPIOS
+	const struct spi_nor_config *cfg = dev->config;
+#endif
 	int ret;
 
+#if ANY_INST_HAS_WP_GPIOS
+	if (cfg->wp) {
+		gpio_pin_set_dt(cfg->wp, write_protect);
+	}
+#endif
+
 	ret = spi_nor_cmd_write(dev, (write_protect) ?
 	      SPI_NOR_CMD_WRDI : SPI_NOR_CMD_WREN);
 
@@ -1238,6 +1271,77 @@ static int spi_nor_configure(const struct device *dev)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_PM_DEVICE)
+static int spi_nor_pm_action(const struct device *dev,
+			     enum pm_device_action action)
+{
+	const struct spi_nor_config *cfg = dev->config;
+
+	switch (action) {
+	case PM_DEVICE_ACTION_RESUME:
+		/* DPD handled by acquire_device() */
+		break;
+
+	case PM_DEVICE_ACTION_SUSPEND:
+		/* DPD handled by acquire_device() */
+		break;
+
+	case PM_DEVICE_ACTION_TURN_ON:
+		/* Initialize write-protect and hold pins, if any */
+#if ANY_INST_HAS_WP_GPIOS
+		if (cfg->wp) {
+			if (!device_is_ready(cfg->wp->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->wp, GPIO_OUTPUT_ACTIVE)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+		if (cfg->hold) {
+			if (!device_is_ready(cfg->hold->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->hold, GPIO_OUTPUT_INACTIVE)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+		break;
+
+	case PM_DEVICE_ACTION_TURN_OFF:
+		/* Disconnect write-protect and hold pins, if any */
+#if ANY_INST_HAS_WP_GPIOS
+		if (cfg->wp) {
+			if (!device_is_ready(cfg->wp->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->wp, GPIO_DISCONNECTED)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+		if (cfg->hold) {
+			if (!device_is_ready(cfg->hold->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->hold, GPIO_DISCONNECTED)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+		break;
+
+	default:
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_PM_DEVICE) */
+
 /**
  * @brief Initialize and configure the flash
  *
@@ -1246,12 +1350,43 @@ static int spi_nor_configure(const struct device *dev)
  */
 static int spi_nor_init(const struct device *dev)
 {
+#if (ANY_INST_HAS_WP_GPIOS || ANY_INST_HAS_HOLD_GPIOS)
+	const struct spi_nor_config *cfg = dev->config;
+#endif
+	int ret = 0;
+
 	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
 		struct spi_nor_data *const driver_data = dev->data;
 
 		k_sem_init(&driver_data->sem, 1, K_SEM_MAX_LIMIT);
 	}
 
+	ret = pm_device_runtime_enable(dev);
+	if ((ret < 0) && (ret != -ENOSYS)) {
+		return ret;
+	}
+
+#if ANY_INST_HAS_WP_GPIOS
+	if (cfg->wp) {
+		if (!device_is_ready(cfg->wp->port)) {
+			return -ENODEV;
+		}
+		if (gpio_pin_configure_dt(cfg->wp, GPIO_OUTPUT_ACTIVE)) {
+			return -ENODEV;
+		}
+	}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+	if (cfg->hold) {
+		if (!device_is_ready(cfg->hold->port)) {
+			return -ENODEV;
+		}
+		if (gpio_pin_configure_dt(cfg->hold, GPIO_OUTPUT_INACTIVE)) {
+			return -ENODEV;
+		}
+	}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+
 	return spi_nor_configure(dev);
 }
 
@@ -1348,6 +1483,25 @@ BUILD_ASSERT(DT_INST_PROP(0, has_lock) == (DT_INST_PROP(0, has_lock) & 0xFF),
 	     "Need support for lock clear beyond SR1");
 #endif
 
+#define INST_HAS_WP_GPIO(idx) \
+	DT_INST_NODE_HAS_PROP(idx, wp_gpios)
+
+#define INST_WP_GPIO_SPEC(idx)						\
+	IF_ENABLED(INST_HAS_WP_GPIO(idx),				\
+		(static const struct gpio_dt_spec wp_##idx =		\
+		GPIO_DT_SPEC_INST_GET(idx, wp_gpios);))
+
+#define INST_HAS_HOLD_GPIO(idx) \
+	DT_INST_NODE_HAS_PROP(idx, hold_gpios)
+
+#define INST_HOLD_GPIO_SPEC(idx)						\
+	IF_ENABLED(INST_HAS_HOLD_GPIO(idx),				\
+		(static const struct gpio_dt_spec hold_##idx =		\
+		GPIO_DT_SPEC_INST_GET(idx, hold_gpios);))
+
+INST_WP_GPIO_SPEC(0)
+INST_HOLD_GPIO_SPEC(0)
+
 static const struct spi_nor_config spi_nor_config_0 = {
 	.spi = SPI_DT_SPEC_INST_GET(0, SPI_WORD_SET(8),
 				    CONFIG_SPI_NOR_CS_WAIT_DELAY),
@@ -1377,11 +1531,21 @@ static const struct spi_nor_config spi_nor_config_0 = {
 #endif /* CONFIG_SPI_NOR_SFDP_DEVICETREE */
 
 #endif /* CONFIG_SPI_NOR_SFDP_RUNTIME */
+
+#if DT_INST_NODE_HAS_PROP(0, wp_gpios)
+	.wp = &wp_0,
+#endif
+
+#if DT_INST_NODE_HAS_PROP(0, hold_gpios)
+	.hold =&hold_0,
+#endif
 };
 
 static struct spi_nor_data spi_nor_data_0;
 
-DEVICE_DT_INST_DEFINE(0, &spi_nor_init, NULL,
+PM_DEVICE_DT_INST_DEFINE(0, spi_nor_pm_action);
+
+DEVICE_DT_INST_DEFINE(0, &spi_nor_init, PM_DEVICE_DT_INST_GET(0),
 		 &spi_nor_data_0, &spi_nor_config_0,
 		 POST_KERNEL, CONFIG_SPI_NOR_INIT_PRIORITY,
 		 &spi_nor_api);

A bit of progress so far...

[Nathan O.]

Hello J.P. Hutchins 

I am encountering the same issue: I need to HOLDn pin to be initialized with the spi-nor driver. Did you contact the Zephyr discord / github to talk about your patch ?

[J.P. Hutchins]

Hi Nathan,

Yes, I do recommend the Discord.  I've been meaning to PR the initialization of these pins, I'll get it in this week and keep you posted.  In the meantime, here's the patch we've been using.  There's a bunch of stuff about power management in there that you don't need and will not be in the PR.

diff --git a/drivers/flash/spi_nor.c b/drivers/flash/spi_nor.c
index fd2701d23a..4266992999 100644
--- a/drivers/flash/spi_nor.c
+++ b/drivers/flash/spi_nor.c
@@ -12,6 +12,8 @@
 #include <errno.h>
 #include <zephyr/drivers/flash.h>
 #include <zephyr/drivers/spi.h>
+#include <zephyr/pm/device.h>
+#include <zephyr/pm/device_runtime.h>
 #include <zephyr/init.h>
 #include <string.h>
 #include <zephyr/logging/log.h>
@@ -61,6 +63,12 @@ LOG_MODULE_REGISTER(spi_nor, CONFIG_FLASH_LOG_LEVEL);
 #define T_DPDD_MS 0
 #endif /* DPD_WAKEUP_SEQUENCE */
 
+#define _INST_HAS_WP_OR(inst) DT_INST_NODE_HAS_PROP(inst, wp_gpios) ||
+#define ANY_INST_HAS_WP_GPIOS DT_INST_FOREACH_STATUS_OKAY(_INST_HAS_WP_OR) 0
+
+#define _INST_HAS_HOLD_OR(inst) DT_INST_NODE_HAS_PROP(inst, hold_gpios) ||
+#define ANY_INST_HAS_HOLD_GPIOS DT_INST_FOREACH_STATUS_OKAY(_INST_HAS_HOLD_OR) 0
+
 /* Build-time data associated with the device. */
 struct spi_nor_config {
 	/* Devicetree SPI configuration */
@@ -101,6 +109,12 @@ struct spi_nor_config {
 	 * This information cannot be derived from SFDP.
 	 */
 	uint8_t has_lock;
+#if ANY_INST_HAS_WP_GPIOS
+	const struct gpio_dt_spec *wp;
+#endif
+#if ANY_INST_HAS_HOLD_GPIOS
+	const struct gpio_dt_spec *hold;
+#endif
 };
 
 /**
@@ -470,6 +484,11 @@ static void acquire_device(const struct device *dev)
 		k_sem_take(&driver_data->sem, K_FOREVER);
 	}
 
+	if (IS_ENABLED(CONFIG_PM_DEVICE_RUNTIME) && pm_device_runtime_is_enabled(dev)) {
+		int err = pm_device_runtime_get(dev);
+		__ASSERT(err == 0, "PM device error: [%d]", err);
+	}
+
 	if (IS_ENABLED(CONFIG_SPI_NOR_IDLE_IN_DPD)) {
 		exit_dpd(dev);
 	}
@@ -486,6 +505,11 @@ static void release_device(const struct device *dev)
 		enter_dpd(dev);
 	}
 
+	if (IS_ENABLED(CONFIG_PM_DEVICE_RUNTIME) && pm_device_runtime_is_enabled(dev)) {
+		int err = pm_device_runtime_put(dev);
+		__ASSERT(err == 0, "PM device error: [%d]", err);
+	}
+
 	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
 		struct spi_nor_data *const driver_data = dev->data;
 
@@ -821,8 +845,17 @@ static int spi_nor_erase(const struct device *dev, off_t addr, size_t size)
 static int spi_nor_write_protection_set(const struct device *dev,
 					bool write_protect)
 {
+#if ANY_INST_HAS_WP_GPIOS
+	const struct spi_nor_config *cfg = dev->config;
+#endif
 	int ret;
 
+#if ANY_INST_HAS_WP_GPIOS
+	if (cfg->wp) {
+		gpio_pin_set_dt(cfg->wp, write_protect);
+	}
+#endif
+
 	ret = spi_nor_cmd_write(dev, (write_protect) ?
 	      SPI_NOR_CMD_WRDI : SPI_NOR_CMD_WREN);
 
@@ -1238,6 +1271,77 @@ static int spi_nor_configure(const struct device *dev)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_PM_DEVICE)
+static int spi_nor_pm_action(const struct device *dev,
+			     enum pm_device_action action)
+{
+	const struct spi_nor_config *cfg = dev->config;
+
+	switch (action) {
+	case PM_DEVICE_ACTION_RESUME:
+		/* DPD handled by acquire_device() */
+		break;
+
+	case PM_DEVICE_ACTION_SUSPEND:
+		/* DPD handled by acquire_device() */
+		break;
+
+	case PM_DEVICE_ACTION_TURN_ON:
+		/* Initialize write-protect and hold pins, if any */
+#if ANY_INST_HAS_WP_GPIOS
+		if (cfg->wp) {
+			if (!device_is_ready(cfg->wp->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->wp, GPIO_OUTPUT_ACTIVE)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+		if (cfg->hold) {
+			if (!device_is_ready(cfg->hold->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->hold, GPIO_OUTPUT_INACTIVE)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+		break;
+
+	case PM_DEVICE_ACTION_TURN_OFF:
+		/* Disconnect write-protect and hold pins, if any */
+#if ANY_INST_HAS_WP_GPIOS
+		if (cfg->wp) {
+			if (!device_is_ready(cfg->wp->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->wp, GPIO_DISCONNECTED)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+		if (cfg->hold) {
+			if (!device_is_ready(cfg->hold->port)) {
+				return -ENODEV;
+			}
+			if (gpio_pin_configure_dt(cfg->hold, GPIO_DISCONNECTED)) {
+				return -ENODEV;
+			}
+		}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+		break;
+
+	default:
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_PM_DEVICE) */
+
 /**
  * @brief Initialize and configure the flash
  *
@@ -1246,12 +1350,43 @@ static int spi_nor_configure(const struct device *dev)
  */
 static int spi_nor_init(const struct device *dev)
 {
+#if (ANY_INST_HAS_WP_GPIOS || ANY_INST_HAS_HOLD_GPIOS)
+	const struct spi_nor_config *cfg = dev->config;
+#endif
+	int ret = 0;
+
 	if (IS_ENABLED(CONFIG_MULTITHREADING)) {
 		struct spi_nor_data *const driver_data = dev->data;
 
 		k_sem_init(&driver_data->sem, 1, K_SEM_MAX_LIMIT);
 	}
 
+	ret = pm_device_runtime_enable(dev);
+	if ((ret < 0) && (ret != -ENOSYS)) {
+		return ret;
+	}
+
+#if ANY_INST_HAS_WP_GPIOS
+	if (cfg->wp) {
+		if (!device_is_ready(cfg->wp->port)) {
+			return -ENODEV;
+		}
+		if (gpio_pin_configure_dt(cfg->wp, GPIO_OUTPUT_ACTIVE)) {
+			return -ENODEV;
+		}
+	}
+#endif /* ANY_INST_HAS_WP_GPIOS */
+#if ANY_INST_HAS_HOLD_GPIOS
+	if (cfg->hold) {
+		if (!device_is_ready(cfg->hold->port)) {
+			return -ENODEV;
+		}
+		if (gpio_pin_configure_dt(cfg->hold, GPIO_OUTPUT_INACTIVE)) {
+			return -ENODEV;
+		}
+	}
+#endif /* ANY_INST_HAS_HOLD_GPIOS */
+
 	return spi_nor_configure(dev);
 }
 
@@ -1348,6 +1483,25 @@ BUILD_ASSERT(DT_INST_PROP(0, has_lock) == (DT_INST_PROP(0, has_lock) & 0xFF),
 	     "Need support for lock clear beyond SR1");
 #endif
 
+#define INST_HAS_WP_GPIO(idx) \
+	DT_INST_NODE_HAS_PROP(idx, wp_gpios)
+
+#define INST_WP_GPIO_SPEC(idx)						\
+	IF_ENABLED(INST_HAS_WP_GPIO(idx),				\
+		(static const struct gpio_dt_spec wp_##idx =		\
+		GPIO_DT_SPEC_INST_GET(idx, wp_gpios);))
+
+#define INST_HAS_HOLD_GPIO(idx) \
+	DT_INST_NODE_HAS_PROP(idx, hold_gpios)
+
+#define INST_HOLD_GPIO_SPEC(idx)						\
+	IF_ENABLED(INST_HAS_HOLD_GPIO(idx),				\
+		(static const struct gpio_dt_spec hold_##idx =		\
+		GPIO_DT_SPEC_INST_GET(idx, hold_gpios);))
+
+INST_WP_GPIO_SPEC(0)
+INST_HOLD_GPIO_SPEC(0)
+
 static const struct spi_nor_config spi_nor_config_0 = {
 	.spi = SPI_DT_SPEC_INST_GET(0, SPI_WORD_SET(8),
 				    CONFIG_SPI_NOR_CS_WAIT_DELAY),
@@ -1377,11 +1531,21 @@ static const struct spi_nor_config spi_nor_config_0 = {
 #endif /* CONFIG_SPI_NOR_SFDP_DEVICETREE */
 
 #endif /* CONFIG_SPI_NOR_SFDP_RUNTIME */
+
+#if DT_INST_NODE_HAS_PROP(0, wp_gpios)
+	.wp = &wp_0,
+#endif
+
+#if DT_INST_NODE_HAS_PROP(0, hold_gpios)
+	.hold =&hold_0,
+#endif
 };
 
 static struct spi_nor_data spi_nor_data_0;
 
-DEVICE_DT_INST_DEFINE(0, &spi_nor_init, NULL,
+PM_DEVICE_DT_INST_DEFINE(0, spi_nor_pm_action);
+
+DEVICE_DT_INST_DEFINE(0, &spi_nor_init, PM_DEVICE_DT_INST_GET(0),
 		 &spi_nor_data_0, &spi_nor_config_0,
 		 POST_KERNEL, CONFIG_SPI_NOR_INIT_PRIORITY,
 		 &spi_nor_api);