| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8c697031-9978-40cd-a1e0-f40552db9107@kwiboo.se>
Date: Sun, 16 Mar 2025 22:52:07 +0100
From: Jonas Karlman <jonas@...boo.se>
To: Kever Yang <kever.yang@...k-chips.com>
Cc: heiko@...ech.de, linux-rockchip@...ts.infradead.org,
Finley Xiao <finley.xiao@...k-chips.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Subject: Re: [PATCH v2 2/3] nvmem: rockchip-otp: Add support for rk3568-otp
Hi Kever,
On 2025-02-27 12:08, Kever Yang wrote:
> From: Finley Xiao <finley.xiao@...k-chips.com>
>
> This adds the necessary data for handling efuse on the rk3568.
>
> Signed-off-by: Finley Xiao <finley.xiao@...k-chips.com>
> Signed-off-by: Kever Yang <kever.yang@...k-chips.com>
> ---
>
> 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@kwiboo.se
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.
> +};
> +
> +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,
Powered by blists - more mailing lists