Re: [PATCH v2 2/3] nvmem: rockchip-otp: Add support for rk3568-otp

From: Kever Yang
Date: Tue Apr 15 2025 - 05:21:29 EST

Next message: Alice Ryhl: "Re: [PATCH] workqueue: rust: add delayed work items"
Previous message: Tiezhu Yang: "[PATCH v2] LoongArch: Handle fp, lsx, lasx and lbt assembly symbols"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jonas,

On 2025/3/17 05:52, Jonas Karlman wrote:

Hi Kever,

On 2025-02-27 12:08, Kever Yang wrote:

From: Finley Xiao <finley.xiao@xxxxxxxxxxxxxx>

This adds the necessary data for handling efuse on the rk3568.

Signed-off-by: Finley Xiao <finley.xiao@xxxxxxxxxxxxxx>
Signed-off-by: Kever Yang <kever.yang@xxxxxxxxxxxxxx>
---

Changes in v2: None

drivers/nvmem/rockchip-otp.c | 82 ++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)

diff --git a/drivers/nvmem/rockchip-otp.c b/drivers/nvmem/rockchip-otp.c
index ebc3f0b24166..a04bce89ecc8 100644
--- a/drivers/nvmem/rockchip-otp.c
+++ b/drivers/nvmem/rockchip-otp.c
@@ -27,6 +27,7 @@
#define OTPC_USER_CTRL 0x0100
#define OTPC_USER_ADDR 0x0104
#define OTPC_USER_ENABLE 0x0108
+#define OTPC_USER_QP 0x0120
#define OTPC_USER_Q 0x0124
#define OTPC_INT_STATUS 0x0304
#define OTPC_SBPI_CMD0_OFFSET 0x1000
@@ -53,6 +54,8 @@
#define SBPI_ENABLE_MASK GENMASK(16, 16)
#define OTPC_TIMEOUT 10000
+#define OTPC_TIMEOUT_PROG 100000

This is not used anywhere in this patch, please drop it.

+#define RK3568_NBYTES 2
/* RK3588 Register */
#define RK3588_OTPC_AUTO_CTRL 0x04
@@ -184,6 +187,70 @@ static int px30_otp_read(void *context, unsigned int offset,
return ret;
}
+static int rk3568_otp_read(void *context, unsigned int offset, void *val,
+ size_t bytes)
+{
+ struct rockchip_otp *otp = context;
+ unsigned int addr_start, addr_end, addr_offset, addr_len;
+ unsigned int otp_qp;
+ u32 out_value;
+ u8 *buf;
+ int ret = 0, i = 0;
+
+ addr_start = rounddown(offset, RK3568_NBYTES) / RK3568_NBYTES;
+ addr_end = roundup(offset + bytes, RK3568_NBYTES) / RK3568_NBYTES;
+ addr_offset = offset % RK3568_NBYTES;
+ addr_len = addr_end - addr_start;
+
+ buf = kzalloc(array3_size(addr_len, RK3568_NBYTES, sizeof(*buf)),
+ GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = rockchip_otp_reset(otp);
+ if (ret) {
+ dev_err(otp->dev, "failed to reset otp phy\n");
+ return ret;

This is leaking the kzalloc memory above.

+ }
+
+ ret = rockchip_otp_ecc_enable(otp, true);
+ if (ret < 0) {
+ dev_err(otp->dev, "rockchip_otp_ecc_enable err\n");
+ return ret;

Same here.

+ }
+
+ writel(OTPC_USE_USER | OTPC_USE_USER_MASK, otp->base + OTPC_USER_CTRL);
+ udelay(5);
+ while (addr_len--) {
+ writel(addr_start++ | OTPC_USER_ADDR_MASK,
+ otp->base + OTPC_USER_ADDR);
+ writel(OTPC_USER_FSM_ENABLE | OTPC_USER_FSM_ENABLE_MASK,
+ otp->base + OTPC_USER_ENABLE);
+ ret = rockchip_otp_wait_status(otp, OTPC_INT_STATUS, OTPC_USER_DONE);
+ if (ret < 0) {
+ dev_err(otp->dev, "timeout during read setup\n");
+ goto read_end;
+ }
+ otp_qp = readl(otp->base + OTPC_USER_QP);
+ if (((otp_qp & 0xc0) == 0xc0) || (otp_qp & 0x20)) {
+ ret = -EIO;
+ dev_err(otp->dev, "ecc check error during read setup\n");
+ goto read_end;
+ }
+ out_value = readl(otp->base + OTPC_USER_Q);
+ memcpy(&buf[i], &out_value, RK3568_NBYTES);
+ i += RK3568_NBYTES;
+ }
+
+ memcpy(val, buf + addr_offset, bytes);
+
+read_end:
+ writel(0x0 | OTPC_USE_USER_MASK, otp->base + OTPC_USER_CTRL);
+ kfree(buf);
+
+ return ret;
+}

This can be simplified if this is rebased on top of "nvmem: rockchip-otp:
Handle internal word_size in main reg_read op" [1].

[1] https://lore.kernel.org/r/20250316191900.1858944-1-jonas@xxxxxxxxx

This look good to me, I will do it.

Something like following could be squashed in with this:

diff --git a/drivers/nvmem/rockchip-otp.c b/drivers/nvmem/rockchip-otp.c
index ea48d51bc2ff..0991a4047bec 100644
--- a/drivers/nvmem/rockchip-otp.c
+++ b/drivers/nvmem/rockchip-otp.c
@@ -54,8 +54,6 @@
#define SBPI_ENABLE_MASK GENMASK(16, 16)
#define OTPC_TIMEOUT 10000
-#define OTPC_TIMEOUT_PROG 100000
-#define RK3568_NBYTES 2
/* RK3588 Register */
#define RK3588_OTPC_AUTO_CTRL 0x04
@@ -188,24 +186,12 @@ static int px30_otp_read(void *context, unsigned int offset,
}
static int rk3568_otp_read(void *context, unsigned int offset, void *val,
- size_t bytes)
+ size_t count)
{
struct rockchip_otp *otp = context;
- unsigned int addr_start, addr_end, addr_offset, addr_len;
- unsigned int otp_qp;
- u32 out_value;
- u8 *buf;
- int ret = 0, i = 0;
-
- addr_start = rounddown(offset, RK3568_NBYTES) / RK3568_NBYTES;
- addr_end = roundup(offset + bytes, RK3568_NBYTES) / RK3568_NBYTES;
- addr_offset = offset % RK3568_NBYTES;
- addr_len = addr_end - addr_start;
-
- buf = kzalloc(array3_size(addr_len, RK3568_NBYTES, sizeof(*buf)),
- GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
+ u16 *buf = val;
+ u32 otp_qp;
+ int ret;
ret = rockchip_otp_reset(otp);
if (ret) {
@@ -214,39 +200,39 @@ static int rk3568_otp_read(void *context, unsigned int offset, void *val,
}
ret = rockchip_otp_ecc_enable(otp, true);
- if (ret < 0) {
+ if (ret) {
dev_err(otp->dev, "rockchip_otp_ecc_enable err\n");
return ret;
}
writel(OTPC_USE_USER | OTPC_USE_USER_MASK, otp->base + OTPC_USER_CTRL);
udelay(5);
- while (addr_len--) {
- writel(addr_start++ | OTPC_USER_ADDR_MASK,
+
+ while (count--) {
+ writel(offset++ | OTPC_USER_ADDR_MASK,
otp->base + OTPC_USER_ADDR);
writel(OTPC_USER_FSM_ENABLE | OTPC_USER_FSM_ENABLE_MASK,
otp->base + OTPC_USER_ENABLE);
- ret = rockchip_otp_wait_status(otp, OTPC_INT_STATUS, OTPC_USER_DONE);
- if (ret < 0) {
+
+ ret = rockchip_otp_wait_status(otp, OTPC_INT_STATUS,
+ OTPC_USER_DONE);
+ if (ret) {
dev_err(otp->dev, "timeout during read setup\n");
goto read_end;
}
+
otp_qp = readl(otp->base + OTPC_USER_QP);
if (((otp_qp & 0xc0) == 0xc0) || (otp_qp & 0x20)) {
ret = -EIO;
dev_err(otp->dev, "ecc check error during read setup\n");
goto read_end;
}
- out_value = readl(otp->base + OTPC_USER_Q);
- memcpy(&buf[i], &out_value, RK3568_NBYTES);
- i += RK3568_NBYTES;
- }
- memcpy(val, buf + addr_offset, bytes);
+ *buf++ = readl(otp->base + OTPC_USER_Q);
+ }
read_end:
writel(0x0 | OTPC_USE_USER_MASK, otp->base + OTPC_USER_CTRL);
- kfree(buf);
return ret;
}

+
static int rk3588_otp_read(void *context, unsigned int offset,
void *val, size_t bytes)
{
@@ -274,6 +341,17 @@ static const struct rockchip_data px30_data = {
.reg_read = px30_otp_read,
};
+static const char * const rk3568_otp_clocks[] = {
+ "usr", "sbpi", "apb_pclk", "phy",

Why do we change from using the "otp"-name for the main clock?

I suggest we keep the main clock named "otp" instead of "usr" for
consistency.

The name is from the hardware design, it follows the hardware naming by default.

To be honest the driver does not care the clock/reset naming and the sequence at all,

and the clock/reset name and number are keep changing due to the controller IP

may come from different vendor or process change.

I can rename the "usr" to "otp" to make it looks better, and also reorder it so like it

look like rk3588.

Thanks,
- Kever

+};
+
+static const struct rockchip_data rk3568_data = {
+ .size = 0x80,

If this is rebased on top of [1] you should also add:

.word_size = sizeof(u16),

Above suggested changes can also be found in a FIXUP commit at my
linux-rockchip tree of pending RK3528 patches [2].

[2] https://github.com/Kwiboo/linux-rockchip/commits/next-20250314-rk3528/

Regards,
Jonas

+ .clks = rk3568_otp_clocks,
+ .num_clks = ARRAY_SIZE(rk3568_otp_clocks),
+ .reg_read = rk3568_otp_read,
+};
+
static const char * const rk3588_otp_clocks[] = {
"otp", "apb_pclk", "phy", "arb",
};
@@ -294,6 +372,10 @@ static const struct of_device_id rockchip_otp_match[] = {
.compatible = "rockchip,rk3308-otp",
.data = &px30_data,
},
+ {
+ .compatible = "rockchip,rk3568-otp",
+ .data = &rk3568_data,
+ },
{
.compatible = "rockchip,rk3588-otp",
.data = &rk3588_data,

Next message: Alice Ryhl: "Re: [PATCH] workqueue: rust: add delayed work items"
Previous message: Tiezhu Yang: "[PATCH v2] LoongArch: Handle fp, lsx, lasx and lbt assembly symbols"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]