| 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: <9e2b5033-b09b-4264-a9d6-f5354292be64@rock-chips.com>
Date: Tue, 15 Apr 2025 17:21:08 +0800
From: Kever Yang <kever.yang@...k-chips.com>
To: Jonas Karlman <jonas@...boo.se>
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 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@...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
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,
>
Powered by blists - more mailing lists