Re: [PATCH 01/11] rockchip: otp: Refactor to use driver data and ops

[Kever Yang]

Hi Jonas,

    Thanks for your patches.

On 2023/2/16 07:48, Jonas Karlman wrote:

Refactor the driver to use driver data and ops to simplify handling
of SoCs that require a unique read op.

Use readl_poll_sleep_timeout instead of a custom poll loop, and add
validation of input parameter to main read op.

Signed-off-by: Jonas Karlman <jo...@kwiboo.se>

Reviewed-by: Kever Yang <kever.y...@rock-chips.com>

Thanks,
- Kever

---
  drivers/misc/rockchip-otp.c | 83 +++++++++++++++++++------------------
  1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/drivers/misc/rockchip-otp.c b/drivers/misc/rockchip-otp.c
index cc9a5450e0ce..a6df0834ebbc 100644
--- a/drivers/misc/rockchip-otp.c
+++ b/drivers/misc/rockchip-otp.c
@@ -5,10 +5,10 @@

#include <common.h>

  #include <asm/io.h>
-#include <command.h>
  #include <dm.h>
  #include <linux/bitops.h>
  #include <linux/delay.h>
+#include <linux/iopoll.h>
  #include <misc.h>

/* OTP Register Offsets */

@@ -49,35 +49,31 @@

struct rockchip_otp_plat {

        void __iomem *base;
-       unsigned long secure_conf_base;
-       unsigned long otp_mask_base;
  };

-static int rockchip_otp_wait_status(struct rockchip_otp_plat *otp,

-                                   u32 flag)
+struct rockchip_otp_data {
+       int (*read)(struct udevice *dev, int offset, void *buf, int size);
+       int size;
+};
+
+static int rockchip_otp_poll_timeout(struct rockchip_otp_plat *otp, u32 flag)
  {
-       int delay = OTPC_TIMEOUT;
-
-       while (!(readl(otp->base + OTPC_INT_STATUS) & flag)) {
-               udelay(1);
-               delay--;
-               if (delay <= 0) {
-                       printf("%s: wait init status timeout\n", __func__);
-                       return -ETIMEDOUT;
-               }
-       }
+       u32 status;
+       int ret;
+
+       ret = readl_poll_sleep_timeout(otp->base + OTPC_INT_STATUS, status,
+                                      (status & flag), 1, OTPC_TIMEOUT);
+       if (ret)
+               return ret;

- /* clean int status */

+       /* Clear int flag */
        writel(flag, otp->base + OTPC_INT_STATUS);

return 0;

  }

-static int rockchip_otp_ecc_enable(struct rockchip_otp_plat *otp,

-                                  bool enable)
+static int rockchip_otp_ecc_enable(struct rockchip_otp_plat *otp, bool enable)
  {
-       int ret = 0;
-
        writel(SBPI_DAP_ADDR_MASK | (SBPI_DAP_ADDR << SBPI_DAP_ADDR_SHIFT),
               otp->base + OTPC_SBPI_CTRL);

@@ -92,11 +88,7 @@ static int rockchip_otp_ecc_enable(struct rockchip_otp_plat *otp, writel(SBPI_ENABLE_MASK | SBPI_ENABLE, otp->base + OTPC_SBPI_CTRL); - ret = rockchip_otp_wait_status(otp, OTPC_SBPI_DONE);

-       if (ret < 0)
-               printf("%s timeout during ecc_enable\n", __func__);
-
-       return ret;
+       return rockchip_otp_poll_timeout(otp, OTPC_SBPI_DONE);
  }

static int rockchip_px30_otp_read(struct udevice *dev, int offset,

@@ -104,27 +96,24 @@ static int rockchip_px30_otp_read(struct udevice *dev, int 
offset,
  {
        struct rockchip_otp_plat *otp = dev_get_plat(dev);
        u8 *buffer = buf;
-       int ret = 0;
+       int ret;

ret = rockchip_otp_ecc_enable(otp, false);

-       if (ret < 0) {
-               printf("%s rockchip_otp_ecc_enable err\n", __func__);
+       if (ret)
                return ret;
-       }

writel(OTPC_USE_USER | OTPC_USE_USER_MASK, otp->base + OTPC_USER_CTRL);

        udelay(5);
+
        while (size--) {
                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_USER_DONE);

-               if (ret < 0) {
-                       printf("%s timeout during read setup\n", __func__);
+               ret = rockchip_otp_poll_timeout(otp, OTPC_USER_DONE);
+               if (ret)
                        goto read_end;
-               }

*buffer++ = readb(otp->base + OTPC_USER_Q);

        }
@@ -138,7 +127,16 @@ read_end:
  static int rockchip_otp_read(struct udevice *dev, int offset,
                             void *buf, int size)
  {
-       return rockchip_px30_otp_read(dev, offset, buf, size);
+       const struct rockchip_otp_data *data =
+               (void *)dev_get_driver_data(dev);
+
+       if (offset < 0 || !buf || size <= 0 || offset + size > data->size)
+               return -EINVAL;
+
+       if (!data->read)
+               return -ENOSYS;
+
+       return data->read(dev, offset, buf, size);
  }

static const struct misc_ops rockchip_otp_ops = {

@@ -147,21 +145,26 @@ static const struct misc_ops rockchip_otp_ops = {

static int rockchip_otp_of_to_plat(struct udevice *dev)

  {
-       struct rockchip_otp_plat *otp = dev_get_plat(dev);
+       struct rockchip_otp_plat *plat = dev_get_plat(dev);

- otp->base = dev_read_addr_ptr(dev);

+       plat->base = dev_read_addr_ptr(dev);

return 0;

  }

+static const struct rockchip_otp_data px30_data = {

+       .read = rockchip_px30_otp_read,
+       .size = 0x40,
+};
+
  static const struct udevice_id rockchip_otp_ids[] = {
        {
                .compatible = "rockchip,px30-otp",
-               .data = (ulong)&rockchip_px30_otp_read,
+               .data = (ulong)&px30_data,
        },
        {
                .compatible = "rockchip,rk3308-otp",
-               .data = (ulong)&rockchip_px30_otp_read,
+               .data = (ulong)&px30_data,
        },
        {}
  };
@@ -170,7 +173,7 @@ U_BOOT_DRIVER(rockchip_otp) = {
        .name = "rockchip_otp",
        .id = UCLASS_MISC,
        .of_match = rockchip_otp_ids,
-       .ops = &rockchip_otp_ops,
        .of_to_plat = rockchip_otp_of_to_plat,
-       .plat_auto      = sizeof(struct rockchip_otp_plat),
+       .plat_auto = sizeof(struct rockchip_otp_plat),
+       .ops = &rockchip_otp_ops,
  };