[PATCH v2 0/2] nvmem: rockchip-efuse: add write support with VQPS gating

[PATCH v2 0/2] nvmem: rockchip-efuse: add write support with VQPS gating

[Hrushiraj Gandhi]

Add RK3399 eFuse write (OTP programming) support, gated on the presence
of an optional VQPS programming supply in the device tree.

Changes in v2:
  - Replaced the rockchip,efuse-write-enable boolean property (v1) with
    a proper optional vqps-supply regulator, as suggested by Heiko
    Stuebner. The supply presence acts as the hardware-level gate for
    write support; the driver owns regulator sequencing entirely inside
    the write callback (enable before A_PGM strobe loop, disable after).
  - Removed stale debug dev_info() left from development.
  - Fixed checkpatch: removed blank line after while open-brace in
    rockchip_rk3399_efuse_read().

Link: https://lore.kernel.org/all/20260715110107.409204-1-hrushirajg23@gmail.com/

Hrushiraj Gandhi (2):
  dt-bindings: nvmem: rockchip-efuse: add optional vqps supply
  nvmem: rockchip-efuse: add write support with optional VQPS supply
    gating

 .../bindings/nvmem/rockchip-efuse.yaml        |   5 +
 drivers/nvmem/rockchip-efuse.c                | 178 +++++++++++++++---
 2 files changed, 158 insertions(+), 25 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] dt-bindings: nvmem: rockchip-efuse: add optional vqps supply

[Hrushiraj Gandhi]

Add an optional vqps-supply property for the eFuse programming supply
voltage (VQPS).

Per RK3399 TRM section 21.6.1, table 21-3, VQPS must be 0V during reads
and 1.8V to 1.98V during A_PGM mode writes. The presence of this supply
declares that the board hardware supports software-initiated eFuse
programming and provides the requisite programming supply.

Signed-off-by: Hrushiraj Gandhi <hrushirajg23@gmail.com>
---
 Documentation/devicetree/bindings/nvmem/rockchip-efuse.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/rockchip-efuse.yaml b/Documentation/devicetree/bindings/nvmem/rockchip-efuse.yaml
index b80fd8d1ae5b..25f209fd881e 100644
--- a/Documentation/devicetree/bindings/nvmem/rockchip-efuse.yaml
+++ b/Documentation/devicetree/bindings/nvmem/rockchip-efuse.yaml
@@ -27,6 +27,11 @@ properties:
       # Deprecated: old compatible value for rk3066a, rk3188 and rk3288
       - rockchip,rockchip-efuse
 
+  vqps-supply:
+    description:
+      Supply for the eFuse programming voltage (VQPS), required only
+      on boards where software-initiated OTP programming is intended.
+
   reg:
     description:
       Registers location and eFuse size.
-- 
2.47.3

[PATCH v2 2/2] nvmem: rockchip-efuse: add write support with optional VQPS supply gating

[Hrushiraj Gandhi]

Implement write support for RK3399 eFuse using A_PGM programming mode,
and gate write access on the presence of an optional "vqps" regulator.

eFuse programming requires the VQPS supply (1.8V~1.98V per RK3399 TRM)
to be active only during write operations and kept at 0V during reads.
To support this:
  - Probe uses devm_regulator_get_optional() for the "vqps" supply.
  - If "vqps" is present, the driver registers write support. If absent,
    the driver leaves the device read-only to prevent unsafe write.
  - In rockchip_rk3399_efuse_write(), VQPS is enabled prior to the A_PGM
    strobe programming loop and disabled immediately after.

Signed-off-by: Hrushiraj Gandhi <hrushirajg23@gmail.com>
---
 drivers/nvmem/rockchip-efuse.c | 178 ++++++++++++++++++++++++++++-----
 1 file changed, 153 insertions(+), 25 deletions(-)

diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
index 013e67136f3b..3bde09236c74 100644
--- a/drivers/nvmem/rockchip-efuse.c
+++ b/drivers/nvmem/rockchip-efuse.c
@@ -12,6 +12,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/nvmem-provider.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -37,12 +38,16 @@
 #define RK3399_NBYTES		4
 #define RK3399_STROBSFTSEL	BIT(9)
 #define RK3399_RSB		BIT(7)
+#define RK3399_PS		BIT(4)
 #define RK3399_PD		BIT(5)
 #define RK3399_PGENB		BIT(3)
 #define RK3399_LOAD		BIT(2)
 #define RK3399_STROBE		BIT(1)
 #define RK3399_CSB		BIT(0)
 
+/* Program time per TRM Section 21.1: 12us +/- 1us; use 15us for margin */
+#define RK3399_PGM_TIME_US	15
+
 #define REG_EFUSE_CTRL		0x0000
 #define REG_EFUSE_DOUT		0x0004
 
@@ -50,6 +55,17 @@ struct rockchip_efuse_chip {
 	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
+	struct regulator *vqps;
+};
+
+/**
+ * struct rockchip_efuse_soc_data - per-SoC efuse callbacks
+ * @reg_read:  mandatory read callback
+ * @reg_write: optional write callback; NULL means read-only
+ */
+struct rockchip_efuse_soc_data {
+	int (*reg_read)(void *ctx, unsigned int offset, void *val, size_t bytes);
+	int (*reg_write)(void *ctx, unsigned int offset, void *val, size_t bytes);
 };
 
 static int rockchip_rk3288_efuse_read(void *context, unsigned int offset,
@@ -177,6 +193,7 @@ static int rockchip_rk3399_efuse_read(void *context, unsigned int offset,
 	writel(RK3399_LOAD | RK3399_PGENB | RK3399_STROBSFTSEL | RK3399_RSB,
 	       efuse->base + REG_EFUSE_CTRL);
 	udelay(1);
+
 	while (addr_len--) {
 		writel(readl(efuse->base + REG_EFUSE_CTRL) | RK3399_STROBE |
 		       ((addr_start++ & RK3399_A_MASK) << RK3399_A_SHIFT),
@@ -203,48 +220,128 @@ static int rockchip_rk3399_efuse_read(void *context, unsigned int offset,
 	return 0;
 }
 
-static struct nvmem_config econfig = {
-	.name = "rockchip-efuse",
-	.add_legacy_fixed_of_cells = true,
-	.type = NVMEM_TYPE_OTP,
-	.stride = 1,
-	.word_size = 1,
-	.read_only = true,
+/**
+ * rockchip_rk3399_efuse_write - program fuse bits on RK3399 (A_PGM mode)
+ * @context: pointer to rockchip_efuse_chip
+ * @offset:  byte offset in the efuse space
+ * @val:     data to write (only set bits are programmed; OTP cannot clear)
+ * @bytes:   number of bytes
+ *
+ * Each bit is programmed individually using a hardware-timed STROBE pulse.
+ * The caller must ensure VQPS (1.8V~1.98V) is present during the write.
+ * Per TRM Section 21.6, only one bit is programmed per STROBE cycle.
+ */
+static int rockchip_rk3399_efuse_write(void *context, unsigned int offset,
+				       void *val, size_t bytes)
+{
+	struct rockchip_efuse_chip *efuse = context;
+	u8 *buf = val;
+	int ret;
+
+	ret = clk_prepare_enable(efuse->clk);
+	if (ret < 0) {
+		dev_err(efuse->dev, "failed to prepare/enable efuse clk\n");
+		return ret;
+	}
+
+	ret = regulator_enable(efuse->vqps);
+	if (ret < 0) {
+		dev_err(efuse->dev, "failed to enable vqps regulator\n");
+		clk_disable_unprepare(efuse->clk);
+		return ret;
+	}
+
+	while (bytes--) {
+		u8 byte = *buf++;
+		int bit;
+
+		for (bit = 0; bit < 8; bit++) {
+			u32 addr, ctrl;
+
+			if (!(byte & BIT(bit)))
+				continue;
+
+			addr = offset * 8 + bit;
+
+			/*
+			 * A_PGM mode (TRM table 23-3):
+			 * CSB=L, PGENB=L, PS=H, PD=L, LOAD=L, RSB=L,
+			 * STROBSFTSEL=H (software controls STROBE)
+			 * STROBE is asserted separately below.
+			 */
+			ctrl = RK3399_STROBSFTSEL | RK3399_PS |
+			       ((addr & RK3399_A_MASK) << RK3399_A_SHIFT);
+
+			writel(ctrl, efuse->base + REG_EFUSE_CTRL);
+			udelay(1);
+
+			/* Assert STROBE to program the selected bit */
+			writel(ctrl | RK3399_STROBE, efuse->base + REG_EFUSE_CTRL);
+			udelay(RK3399_PGM_TIME_US);
+
+			/* Deassert STROBE */
+			writel(ctrl, efuse->base + REG_EFUSE_CTRL);
+			udelay(1);
+		}
+
+		offset++;
+	}
+
+	/* Return to standby mode: PD=H, CSB=H */
+	writel(RK3399_PD | RK3399_CSB, efuse->base + REG_EFUSE_CTRL);
+
+	regulator_disable(efuse->vqps);
+	clk_disable_unprepare(efuse->clk);
+
+	return 0;
+}
+
+static const struct rockchip_efuse_soc_data rk3288_efuse_data = {
+	.reg_read = rockchip_rk3288_efuse_read,
+};
+
+static const struct rockchip_efuse_soc_data rk3328_efuse_data = {
+	.reg_read = rockchip_rk3328_efuse_read,
+};
+
+static const struct rockchip_efuse_soc_data rk3399_efuse_data = {
+	.reg_read  = rockchip_rk3399_efuse_read,
+	.reg_write = rockchip_rk3399_efuse_write,
 };
 
 static const struct of_device_id rockchip_efuse_match[] = {
 	/* deprecated but kept around for dts binding compatibility */
 	{
 		.compatible = "rockchip,rockchip-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3066a-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3188-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3228-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3288-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3368-efuse",
-		.data = (void *)&rockchip_rk3288_efuse_read,
+		.data = &rk3288_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3328-efuse",
-		.data = (void *)&rockchip_rk3328_efuse_read,
+		.data = &rk3328_efuse_data,
 	},
 	{
 		.compatible = "rockchip,rk3399-efuse",
-		.data = (void *)&rockchip_rk3399_efuse_read,
+		.data = &rk3399_efuse_data,
 	},
 	{ /* sentinel */},
 };
@@ -252,20 +349,26 @@ MODULE_DEVICE_TABLE(of, rockchip_efuse_match);
 
 static int rockchip_efuse_probe(struct platform_device *pdev)
 {
-	struct resource *res;
-	struct nvmem_device *nvmem;
+	const struct rockchip_efuse_soc_data *soc_data;
 	struct rockchip_efuse_chip *efuse;
-	const void *data;
+	struct nvmem_device *nvmem;
+	struct nvmem_config econfig = {
+		.name              = "rockchip-efuse",
+		.add_legacy_fixed_of_cells = true,
+		.type              = NVMEM_TYPE_OTP,
+		.stride            = 1,
+		.word_size         = 1,
+	};
+	struct resource *res;
 	struct device *dev = &pdev->dev;
 
-	data = of_device_get_match_data(dev);
-	if (!data) {
+	soc_data = of_device_get_match_data(dev);
+	if (!soc_data) {
 		dev_err(dev, "failed to get match data\n");
 		return -EINVAL;
 	}
 
-	efuse = devm_kzalloc(dev, sizeof(struct rockchip_efuse_chip),
-			     GFP_KERNEL);
+	efuse = devm_kzalloc(dev, sizeof(*efuse), GFP_KERNEL);
 	if (!efuse)
 		return -ENOMEM;
 
@@ -278,12 +381,36 @@ static int rockchip_efuse_probe(struct platform_device *pdev)
 		return PTR_ERR(efuse->clk);
 
 	efuse->dev = dev;
+
 	if (of_property_read_u32(dev->of_node, "rockchip,efuse-size",
 				 &econfig.size))
 		econfig.size = resource_size(res);
-	econfig.reg_read = data;
-	econfig.priv = efuse;
-	econfig.dev = efuse->dev;
+
+	econfig.reg_read = soc_data->reg_read;
+	econfig.priv     = efuse;
+	econfig.dev      = dev;
+
+	/*
+	 * Enable write support only when a VQPS programming supply is
+	 * described in the device tree. Its presence is the hardware
+	 * declaration that irreversible OTP programming is intended on
+	 * this board. VQPS must be 0V during reads (the regulator is
+	 * only enabled inside the write callback).
+	 */
+	if (soc_data->reg_write) {
+		efuse->vqps = devm_regulator_get_optional(dev, "vqps");
+		if (!IS_ERR(efuse->vqps)) {
+			econfig.reg_write = soc_data->reg_write;
+		} else if (PTR_ERR(efuse->vqps) == -ENODEV) {
+			efuse->vqps = NULL;
+			dev_dbg(dev, "vqps supply absent, write support disabled\n");
+		} else {
+			return PTR_ERR(efuse->vqps);
+		}
+	}
+
+	econfig.read_only = !econfig.reg_write;
+
 	nvmem = devm_nvmem_register(dev, &econfig);
 
 	return PTR_ERR_OR_ZERO(nvmem);
@@ -300,3 +427,4 @@ static struct platform_driver rockchip_efuse_driver = {
 module_platform_driver(rockchip_efuse_driver);
 MODULE_DESCRIPTION("rockchip_efuse driver");
 MODULE_LICENSE("GPL v2");
+
-- 
2.47.3

Re: [PATCH v2 2/2] nvmem: rockchip-efuse: add write support with optional VQPS supply gating

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Lack of preemption and IRQ protection around precisely timed STROBE pulse during eFuse programming.
- [Critical] Missing driver-level locking allows concurrent reads and writes to corrupt global controller registers and destroy eFuses.
- [Medium] VQPS regulator is enabled without explicitly configuring its strictly required programming voltage.

Pre-existing issues:
- [High] Read address bits are not cleared before ORing the next address, permanently corrupting multi-word read addresses.
- [High] CPU-endian register reads are copied via memcpy, causing byte-order reversal on Big Endian systems.
--

--- Patch [2]: [PATCH v2 2/2] nvmem: rockchip-efuse: add write support with optional VQPS supply gating ---
commit 9787a852435c97fd8c38650a5baf58286ca592d1
Author: Hrushiraj Gandhi <hrushirajg23@gmail.com>

nvmem: rockchip-efuse: add write support with optional VQPS supply gating

This commit adds write support for the RK3399 eFuse using the A_PGM
programming mode. It retrieves an optional VQPS regulator that is enabled
only during write operations to fulfill hardware requirements. If the
regulator is absent, the device is left read-only to prevent unsafe writes.

> diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
> index 013e67136f3b9..3bde09236c74c 100644
> --- a/drivers/nvmem/rockchip-efuse.c
> +++ b/drivers/nvmem/rockchip-efuse.c
[ ... ]
> @@ -177,6 +193,7 @@ static int rockchip_rk3399_efuse_read(void *context, unsigned int offset,
>  	writel(RK3399_LOAD | RK3399_PGENB | RK3399_STROBSFTSEL | RK3399_RSB,
>  	       efuse->base + REG_EFUSE_CTRL);
>  	udelay(1);
> +
>  	while (addr_len--) {
>  		writel(readl(efuse->base + REG_EFUSE_CTRL) | RK3399_STROBE |
>  		       ((addr_start++ & RK3399_A_MASK) << RK3399_A_SHIFT),
>  		       efuse->base + REG_EFUSE_CTRL);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for multi-word
reads to use corrupted addresses here? 

The loop bitwise-ORs the new read address into REG_EFUSE_CTRL but never
clears the previous address bits. On subsequent iterations, the old address
bits remain set, which causes the new read address to be a bitwise OR of all
prior addresses in the sequence.

>  		udelay(1);
>  		out_value = readl(efuse->base + REG_EFUSE_DOUT);
>  		writel(readl(efuse->base + REG_EFUSE_CTRL) & (~RK3399_STROBE),
>  		       efuse->base + REG_EFUSE_CTRL);
>  		udelay(1);
>  
>  		memcpy(&buf[i], &out_value, RK3399_NBYTES);

[Severity: High]
This is a pre-existing issue, but does copying the register value this way
cause byte-order reversal on big-endian systems? 

readl() returns the 32-bit value in the CPU's native endianness, and using
memcpy() to serialize it will store the most significant byte at the lowest
address on big-endian systems. Should this use put_unaligned_le32() or
similar to guarantee consistent byte layout across architectures?

[ ... ]
> +static int rockchip_rk3399_efuse_write(void *context, unsigned int offset,
> +				       void *val, size_t bytes)
> +{
> +	struct rockchip_efuse_chip *efuse = context;
> +	u8 *buf = val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(efuse->clk);
> +	if (ret < 0) {
> +		dev_err(efuse->dev, "failed to prepare/enable efuse clk\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(efuse->vqps);

[Severity: Medium]
Could enabling the VQPS regulator without enforcing a specific voltage cause
hardware issues? 

The commit message mentions a 1.8V to 1.98V requirement, but
regulator_enable() does not enforce this. If the device tree defines this as
a shared or variable regulator currently set to a different voltage, could it
fail to program the OTP or cause overvoltage damage? Should this use
regulator_set_voltage() to guarantee the required programming voltage?

> +	if (ret < 0) {
> +		dev_err(efuse->dev, "failed to enable vqps regulator\n");
> +		clk_disable_unprepare(efuse->clk);
> +		return ret;
> +	}
> +
> +	while (bytes--) {
> +		u8 byte = *buf++;
> +		int bit;
> +
> +		for (bit = 0; bit < 8; bit++) {
> +			u32 addr, ctrl;
> +
> +			if (!(byte & BIT(bit)))
> +				continue;
> +
> +			addr = offset * 8 + bit;
> +
> +			/*
> +			 * A_PGM mode (TRM table 23-3):
> +			 * CSB=L, PGENB=L, PS=H, PD=L, LOAD=L, RSB=L,
> +			 * STROBSFTSEL=H (software controls STROBE)
> +			 * STROBE is asserted separately below.
> +			 */
> +			ctrl = RK3399_STROBSFTSEL | RK3399_PS |
> +			       ((addr & RK3399_A_MASK) << RK3399_A_SHIFT);
> +
> +			writel(ctrl, efuse->base + REG_EFUSE_CTRL);

[Severity: Critical]
Can concurrent reads and writes corrupt the global controller registers? 

The NVMEM core does not serialize device-level reads and writes. Both
rockchip_rk3399_efuse_write() and rockchip_rk3399_efuse_read() modify the
global REG_EFUSE_CTRL register.

If a concurrent read occurs from sysfs during a write operation, couldn't it
abruptly overwrite the A_PGM state, clear RK3399_PS, or assert standby mode
while the programming STROBE is active? Furthermore, reading while the vqps
regulator is enabled violates the hardware requirement that VQPS must be 0V
during reads. Should a driver-level mutex be used to protect the register
state and regulator transitions?

> +			udelay(1);
> +
> +			/* Assert STROBE to program the selected bit */
> +			writel(ctrl | RK3399_STROBE, efuse->base + REG_EFUSE_CTRL);
> +			udelay(RK3399_PGM_TIME_US);

[Severity: Critical]
Can this tight programming loop cause issues if it gets preempted? 

The RK3399 TRM specifies a precise programming STROBE pulse. This code uses
a udelay() loop that spins in process context without disabling interrupts or
preemption.

If a timer interrupt fires or the thread is preempted during this udelay(),
would the STROBE line remain asserted for the duration of the interruption,
potentially permanently degrading the OTP memory cells? Should
local_irq_save() and local_irq_restore() or similar atomic context guarantees
be used around the strobe?

> +
> +			/* Deassert STROBE */
> +			writel(ctrl, efuse->base + REG_EFUSE_CTRL);
> +			udelay(1);
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729085856.226001-1-hrushirajg23@gmail.com?part=2