drivers: nrf_mram: verify writes by detecting Bus faults

Fix the case where the MRAM writes get corrupted in some conditions.
After each "MRAM_WORD_SIZE write", a read of the written/erased MRAM word
is performed while masking the bus faults to avoid halting the system.
If an error is detected in the BFSR status bits, retry the write for a
maximum of CONFIG_NRF_MRAM_MAX_RETRIES (default 20) times.

Signed-off-by: Riadh Ghaddab <riadh.ghaddab@nordicsemi.no>

Author: rghaddab

drivers/flash/Kconfig.nrf_mram

@@ -8,7 +8,7 @@ config SOC_FLASH_NRF_MRAM
 	bool "Nordic Semiconductor flash driver for MRAM"
 	default y
 	depends on DT_HAS_NORDIC_MRAM_ENABLED
-	select NRFX_MRAMC
+	depends on HAS_NRFX
 	select FLASH_HAS_DRIVER_ENABLED
 	select FLASH_HAS_PAGE_LAYOUT
 	select FLASH_HAS_NO_EXPLICIT_ERASE
@@ -21,3 +21,15 @@ config SOC_FLASH_NRF_MRAM
 	  Note that MRAM words are auto-erased when written to, but writing to a
 	  pre-erased area is faster. Hence, the erase API is not required, but
 	  it can be used to amortize write performance for some use cases.
+
+config NRF_MRAM_MAX_RETRIES
+	int "Maximum number of retries for MRAM write/erase operations"
+	default 20
+	range 0 255
+	depends on SOC_FLASH_NRF_MRAM
+	help
+	  This option sets the maximum number of retry attempts for MRAM
+	  write and erase operations when verification fails. Each retry
+	  attempts to rewrite or re-erase the data to ensure reliability.
+	  Higher values increase reliability but may impact performance
+	  in case of persistent failures.

drivers/flash/soc_flash_nrf_mram.c

@@ -10,7 +10,6 @@
 #include <zephyr/logging/log.h>
 #include <zephyr/sys/barrier.h>
 #include <zephyr/cache.h>
-#include <nrfx_mramc.h>
 #include <ironside/se/versions.h>
 #if defined(CONFIG_MRAM_LATENCY)
 #include "../soc/nordic/common/mram_latency.h"
@@ -32,6 +31,13 @@ LOG_MODULE_REGISTER(flash_nrf_mram, CONFIG_FLASH_LOG_LEVEL);
 #define WRITE_BLOCK_SIZE DT_INST_PROP_OR(0, write_block_size, MRAM_WORD_SIZE)
 #define ERASE_BLOCK_SIZE DT_INST_PROP_OR(0, erase_block_size, WRITE_BLOCK_SIZE)
 
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
+#warning "write-block-size is not a multiple of 16 bytes. \
+This may lead to data corruption in corner cases where partial writes to MRAM words are needed. \
+If a partial write fails and a read-modify-write is needed for recovery, the read may return \
+corrupted data due to the previous corrupted write, which can cause the recovery to fail as well."
+#endif
+
 #define ERASE_VALUE 0xff
 
 #define SOC_NRF_MRAM_BANK_11_OFFSET  (MRAM_SIZE / 2)
@@ -45,8 +51,123 @@ BUILD_ASSERT((ERASE_BLOCK_SIZE % WRITE_BLOCK_SIZE) == 0,
 
 struct nrf_mram_data_t {
 	uint8_t ironside_se_ver;
+	struct k_mutex nrf_mram_mutex;
 };
 
+/**
+ * Safely probes one MRAM word to detect a BusFault.
+ *
+ * The function temporarily masks fault handling and ignores BusFault escalation
+ * while reading one aligned MRAM word. It then checks and clears BFSR status
+ * bits to determine whether the read faulted.
+ *
+ * @param addr Address to probe. The read is performed on the 16-byte-aligned
+ *             base address that contains this location.
+ *
+ * @retval 0 Read completed without BusFault.
+ * @retval non-zero BusFault was observed during the read.
+ */
+uint32_t nrf_mram_safe_read_word(uint32_t addr)
+{
+	uint8_t rbuf[MRAM_WORD_SIZE];
+	uint32_t faulted;
+	unsigned int irq_lock_key = irq_lock();
+
+	/* Clear any pre-existing BusFault status bits (write-1-to-clear) */
+	SCB->CFSR = SCB_CFSR_BUSFAULTSR_Msk;   /* 0x0000FF00 — entire BFSR byte */
+
+	__set_FAULTMASK(1);
+	SCB->CCR |= SCB_CCR_BFHFNMIGN_Msk;
+
+	/* Ensure that all operations are in effect before doing the risky read */
+	barrier_dsync_fence_full();
+	barrier_isync_fence_full();
+
+	/* Read operation that may fault */
+	memcpy(rbuf, (void *)(addr & ~MRAM_WORD_MASK), MRAM_WORD_SIZE);
+
+	/* DSB ensures the load and any resulting fault status write complete */
+	barrier_dsync_fence_full();
+
+	/* Read fault status BEFORE re-enabling faults */
+	faulted = (SCB->CFSR & SCB_CFSR_BUSFAULTSR_Msk) != 0;
+
+	/* Clear whatever was recorded so we don't leave stale status */
+	SCB->CFSR = SCB_CFSR_BUSFAULTSR_Msk;
+
+	__set_FAULTMASK(0);
+	SCB->CCR &= ~SCB_CCR_BFHFNMIGN_Msk;
+
+	barrier_dsync_fence_full();
+	barrier_isync_fence_full();
+
+	irq_unlock(irq_lock_key);
+
+	return faulted;    /* 0 = read succeeded, non-zero = bus fault occurred */
+}
+
+static int nrf_mram_write_and_verify(uint32_t addr, const void *data, size_t len)
+{
+	uint8_t retries = CONFIG_NRF_MRAM_MAX_RETRIES;
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
+	uint8_t rbuf[MRAM_WORD_SIZE];
+
+	if (len % MRAM_WORD_SIZE) {
+		/* For partial word writes, we need to read the existing data first to avoid
+		 * overwriting the unwritten bytes in the same word.
+		 */
+		memcpy(rbuf + len, (void *)(addr + len), MRAM_WORD_SIZE - len);
+		memcpy(rbuf, data, len);
+		data = rbuf;
+		len = MRAM_WORD_SIZE;
+	}
+#endif
+
+	while (retries--) {
+		memcpy((void *)addr, data, len);
+		if (!nrf_mram_safe_read_word(addr)) {
+			return 0;
+		}
+		LOG_ERR("MRAM write verification failed at address 0x%x, retrying... (%u retries "
+			"left)",
+			addr, retries);
+	}
+
+	return -EIO;
+}
+
+static int nrf_mram_erase_and_verify(uint32_t addr, size_t len)
+{
+	uint8_t retries = CONFIG_NRF_MRAM_MAX_RETRIES;
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
+	uint8_t rbuf[MRAM_WORD_SIZE];
+
+	if (len % MRAM_WORD_SIZE) {
+		/* For partial word writes, we need to read the existing data first to avoid
+		 * overwriting the unwritten bytes in the same word.
+		 */
+		memcpy(rbuf, (void *)(addr + len), MRAM_WORD_SIZE - len);
+	}
+#endif
+
+	while (retries--) {
+		memset((void *)addr, ERASE_VALUE, len);
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
+		if (len % MRAM_WORD_SIZE) {
+			memcpy((void *)addr, rbuf, MRAM_WORD_SIZE - len);
+		}
+#endif
+		if (!nrf_mram_safe_read_word(addr)) {
+			return 0;
+		}
+		LOG_ERR("MRAM erase verification failed at address 0x%x, retrying... (%u retries "
+			"left)",
+			addr, retries);
+	}
+
+	return -EIO;
+}
+
 #ifdef CONFIG_MRAM_LATENCY
 static inline bool nrf_mram_ready(uint32_t addr, uint8_t ironside_se_ver)
 {
@@ -55,9 +176,9 @@ static inline bool nrf_mram_ready(uint32_t addr, uint8_t ironside_se_ver)
 	}
 
 	if (addr < SOC_NRF_MRAM_BANK_11_ADDRESS) {
-		return nrf_mramc_ready_get(NRF_MRAMC110);
+		return (bool)NRF_MRAMC110->READY;
 	} else {
-		return nrf_mramc_ready_get(NRF_MRAMC111);
+		return (bool)NRF_MRAMC111->READY;
 	}
 }
 #else
@@ -86,7 +207,7 @@ static uintptr_t validate_and_map_addr(off_t offset, size_t len, bool must_align
 
 	const uintptr_t addr = MRAM_START + offset;
 
-	if (WRITE_BLOCK_SIZE > 1 && must_align &&
+	if ((WRITE_BLOCK_SIZE > 1) && must_align &&
 	    unlikely((addr % WRITE_BLOCK_SIZE) != 0 || (len % WRITE_BLOCK_SIZE) != 0)) {
 		LOG_ERR("invalid alignment: %p:%zu", (void *)addr, len);
 		return 0;
@@ -95,33 +216,6 @@ static uintptr_t validate_and_map_addr(off_t offset, size_t len, bool must_align
 	return addr;
 }
 
-/**
- * @param[in] addr_end  Last modified MRAM address (not inclusive).
- */
-static void commit_changes(uintptr_t addr_end)
-{
-	/* Barrier following our last write. */
-	barrier_dmem_fence_full();
-
-	if ((WRITE_BLOCK_SIZE & MRAM_WORD_MASK) == 0 || (addr_end & MRAM_WORD_MASK) == 0) {
-		/* Our last operation was MRAM word-aligned, so we're done.
-		 * Note: if WRITE_BLOCK_SIZE is a multiple of MRAM_WORD_SIZE,
-		 * then this was already checked in validate_and_map_addr().
-		 */
-		return;
-	}
-
-	/* Get the most significant byte (MSB) of the last MRAM word we were modifying.
-	 * Writing to this byte makes the MRAM controller commit other pending writes to that word.
-	 */
-	addr_end |= MRAM_WORD_MASK;
-
-	/* Issue a dummy write, since we didn't have anything to write here.
-	 * Doing this lets us finalize our changes before we exit the driver API.
-	 */
-	sys_write8(sys_read8(addr_end), addr_end);
-}
-
 static int nrf_mram_read(const struct device *dev, off_t offset, void *data, size_t len)
 {
 	ARG_UNUSED(dev);
@@ -143,6 +237,7 @@ static int nrf_mram_write(const struct device *dev, off_t offset, const void *da
 {
 	struct nrf_mram_data_t *nrf_mram_data = dev->data;
 	uint8_t ironside_se_ver = nrf_mram_data->ironside_se_ver;
+	int ret = 0;
 
 	const uintptr_t addr = validate_and_map_addr(offset, len, true);
 
@@ -152,40 +247,54 @@ static int nrf_mram_write(const struct device *dev, off_t offset, const void *da
 
 	LOG_DBG("write: %p:%zu", (void *)addr, len);
 
+	k_mutex_lock(&nrf_mram_data->nrf_mram_mutex, K_FOREVER);
+
 	if (ironside_se_ver >= IRONSIDE_SE_SUPPORT_READY_VER) {
 #if defined(CONFIG_MRAM_LATENCY)
 		mram_no_latency_sync_request();
 #endif
 	}
+
 	for (uint32_t i = 0; i < (len / MRAM_WORD_SIZE); i++) {
 		while (!nrf_mram_ready(addr + (i * MRAM_WORD_SIZE), ironside_se_ver)) {
 			/* Wait until MRAM controller is ready */
 		}
-		memcpy((void *)(addr + (i * MRAM_WORD_SIZE)),
-		       (void *)((uintptr_t)data + (i * MRAM_WORD_SIZE)), MRAM_WORD_SIZE);
+		ret = nrf_mram_write_and_verify(addr + (i * MRAM_WORD_SIZE),
+						(void *)((uintptr_t)data + (i * MRAM_WORD_SIZE)),
+						MRAM_WORD_SIZE);
+		if (ret) {
+			goto unlock;
+		}
 	}
-
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
 	if (len % MRAM_WORD_SIZE) {
 		while (!nrf_mram_ready(addr + (len & ~MRAM_WORD_MASK), ironside_se_ver)) {
 			/* Wait until MRAM controller is ready */
 		}
-		memcpy((void *)(addr + (len & ~MRAM_WORD_MASK)),
-		       (void *)((uintptr_t)data + (len & ~MRAM_WORD_MASK)), len & MRAM_WORD_MASK);
+		ret = nrf_mram_write_and_verify(addr + (len & ~MRAM_WORD_MASK),
+						(void *)((uintptr_t)data + (len & ~MRAM_WORD_MASK)),
+						len & MRAM_WORD_MASK);
+		if (ret) {
+			goto unlock;
+		}
 	}
-	commit_changes(addr + len);
+#endif
 	if (ironside_se_ver >= IRONSIDE_SE_SUPPORT_READY_VER) {
 #if defined(CONFIG_MRAM_LATENCY)
 		mram_no_latency_sync_release();
 #endif
 	}
 
-	return 0;
+unlock:
+	k_mutex_unlock(&nrf_mram_data->nrf_mram_mutex);
+	return ret;
 }
 
 static int nrf_mram_erase(const struct device *dev, off_t offset, size_t size)
 {
 	struct nrf_mram_data_t *nrf_mram_data = dev->data;
 	uint8_t ironside_se_ver = nrf_mram_data->ironside_se_ver;
+	int ret = 0;
 
 	const uintptr_t addr = validate_and_map_addr(offset, size, true);
 
@@ -195,6 +304,8 @@ static int nrf_mram_erase(const struct device *dev, off_t offset, size_t size)
 
 	LOG_DBG("erase: %p:%zu", (void *)addr, size);
 
+	k_mutex_lock(&nrf_mram_data->nrf_mram_mutex, K_FOREVER);
+
 	/* Ensure that the mramc banks are powered on */
 	if (ironside_se_ver >= IRONSIDE_SE_SUPPORT_READY_VER) {
 #if defined(CONFIG_MRAM_LATENCY)
@@ -205,23 +316,34 @@ static int nrf_mram_erase(const struct device *dev, off_t offset, size_t size)
 		while (!nrf_mram_ready(addr + (i * MRAM_WORD_SIZE), ironside_se_ver)) {
 			/* Wait until MRAM controller is ready */
 		}
-		memset((void *)(addr + (i * MRAM_WORD_SIZE)), ERASE_VALUE, MRAM_WORD_SIZE);
+		ret = nrf_mram_erase_and_verify(addr + (i * MRAM_WORD_SIZE), MRAM_WORD_SIZE);
+		if (ret) {
+			goto unlock;
+		}
 	}
+
+#if (WRITE_BLOCK_SIZE & MRAM_WORD_MASK)
 	if (size % MRAM_WORD_SIZE) {
 		while (!nrf_mram_ready(addr + (size & ~MRAM_WORD_MASK), ironside_se_ver)) {
 			/* Wait until MRAM controller is ready */
 		}
-		memset((void *)(addr + (size & ~MRAM_WORD_MASK)), ERASE_VALUE,
-		       size & MRAM_WORD_MASK);
+		ret = nrf_mram_erase_and_verify(addr + (size & ~MRAM_WORD_MASK),
+						size & MRAM_WORD_MASK);
+		if (ret) {
+			goto unlock;
+		}
 	}
-	commit_changes(addr + size);
+#endif
+
 	if (ironside_se_ver >= IRONSIDE_SE_SUPPORT_READY_VER) {
 #if defined(CONFIG_MRAM_LATENCY)
 		mram_no_latency_sync_release();
 #endif
 	}
 
-	return 0;
+unlock:
+	k_mutex_unlock(&nrf_mram_data->nrf_mram_mutex);
+	return ret;
 }
 
 static int nrf_mram_get_size(const struct device *dev, uint64_t *size)
@@ -285,6 +407,8 @@ static int nrf_mram_init(const struct device *dev)
 #endif
 	LOG_DBG("Ironside SE version: %u", nrf_mram_data->ironside_se_ver);
 
+	k_mutex_init(&nrf_mram_data->nrf_mram_mutex);
+
 	return 0;
 }