[PATCH v2 0/2] Add driver for OCOTP in Sunplus SP7021

[PATCH v2 0/2] Add driver for OCOTP in Sunplus SP7021

[Vincent Shih]

This is a patch series for OCOTP driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Cortex A7 (4 cores) based SoC. It integrates
many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD Card and
etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Vincent Shih (2):
  nvmem: Add driver for OCOTP in Sunplus SP7021
  dt-bindings: nvmem: Add bindings doc for Sunplus OCOTP driver

 .../bindings/nvmem/sunplus,sp7021-ocotp.yaml       |  86 +++++++
 MAINTAINERS                                        |   6 +
 drivers/nvmem/Kconfig                              |  12 +
 drivers/nvmem/Makefile                             |   2 +
 drivers/nvmem/sunplus-ocotp.c                      | 268 +++++++++++++++++++++
 5 files changed, 374 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
 create mode 100644 drivers/nvmem/sunplus-ocotp.c

-- 
2.7.4

[PATCH v2 1/2] nvmem: Add driver for OCOTP in Sunplus SP7021

[Vincent Shih]

Add driver for OCOTP in Sunplus SP7021

Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
---
Changes in v2:
 - Merge sunplus-ocotp.h and sunplus-ocotp0.c to sunplus-ocotp.c
 - Clean up codes.

 MAINTAINERS                   |   5 +
 drivers/nvmem/Kconfig         |  12 ++
 drivers/nvmem/Makefile        |   2 +
 drivers/nvmem/sunplus-ocotp.c | 268 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 drivers/nvmem/sunplus-ocotp.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 80eebc1..0e6593a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17947,6 +17947,11 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS OCOTP DRIVER
+M:	Vincent Shih <vincent.sunplus@gmail.com>
+S:	Maintained
+F:	drivers/nvmem/sunplus-ocotp.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index da41461..fb053d6 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -300,4 +300,16 @@ config NVMEM_BRCM_NVRAM
 	  This driver provides support for Broadcom's NVRAM that can be accessed
 	  using I/O mapping.
 
+config NVMEM_SUNPLUS_OCOTP
+	tristate "Sunplus SoC OTP support"
+	depends on SOC_SP7021
+	depends on HAS_IOMEM
+	help
+	  This is a driver for the On-chip OTP controller (OCOTP) available
+	  on Sunplus SoCs. It provids access to 128 bytes of one-time
+	  programmable eFuse.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-sunplus-ocotp.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index dcbbde3..0f14cd9 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -61,3 +61,5 @@ obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
 nvmem-rmem-y			:= rmem.o
 obj-$(CONFIG_NVMEM_BRCM_NVRAM)	+= nvmem_brcm_nvram.o
 nvmem_brcm_nvram-y		:= brcm_nvram.o
+obj-$(CONFIG_NVMEM_SUNPLUS_OCOTP)	+= nvmem_sunplus_ocotp.o
+nvmem_sunplus_ocotp-y		:= sunplus-ocotp.o
diff --git a/drivers/nvmem/sunplus-ocotp.c b/drivers/nvmem/sunplus-ocotp.c
new file mode 100644
index 0000000..2997b29
--- /dev/null
+++ b/drivers/nvmem/sunplus-ocotp.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The OCOTP driver for Sunplus	SP7021
+ *
+ * Copyright (C) 2019 Sunplus Technology Inc., All rights reseerved.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+/*
+ * OTP memory
+ * Each bank contains 4 words (32 bits).
+ * Bank 0 starts at offset 0 from the base.
+ */
+
+#define OTP_WORDS_PER_BANK		4
+#define OTP_WORD_SIZE			sizeof(u32)
+#define OTP_BIT_ADDR_OF_BANK		(8 * OTP_WORD_SIZE * OTP_WORDS_PER_BANK)
+#define QAC628_OTP_NUM_BANKS            8
+#define QAC628_OTP_SIZE                 (QAC628_OTP_NUM_BANKS * OTP_WORDS_PER_BANK * OTP_WORD_SIZE)
+#define OTP_READ_TIMEOUT                20000
+
+/* HB_GPIO */
+#define ADDRESS_8_DATA			0x20
+
+/* OTP_RX */
+#define OTP_CONTROL_2			0x48
+#define OTP_RD_PERIOD			GENMASK(15, 8)
+#define OTP_RD_PERIOD_MASK		~GENMASK(15, 8)
+#define ONE_CPU_CLOCK			0x1
+#define SEL_BAK_KEY2			BIT(5)
+#define SEL_BAK_KEY2_MASK		~BIT(5)
+#define SW_TRIM_EN			BIT(4)
+#define SW_TRIM_EN_MASK			~BIT(4)
+#define SEL_BAK_KEY			BIT(3)
+#define SEL_BAK_KEY_MASK		~BIT(3)
+#define OTP_READ			BIT(2)
+#define OTP_LOAD_SECURE_DATA		BIT(1)
+#define OTP_LOAD_SECURE_DATA_MASK	~BIT(1)
+#define OTP_DO_CRC			BIT(0)
+#define OTP_DO_CRC_MASK			~BIT(0)
+#define OTP_STATUS			0x4c
+#define OTP_READ_DONE			BIT(4)
+#define OTP_READ_DONE_MASK		~BIT(4)
+#define OTP_LOAD_SECURE_DONE_MASK	~BIT(2)
+#define OTP_READ_ADDRESS		0x50
+
+enum base_type {
+	HB_GPIO,
+	OTPRX,
+	BASEMAX,
+};
+
+struct sp_otp_data_t {
+	struct device *dev;
+	void __iomem *base[BASEMAX];
+	struct clk *clk;
+	struct nvmem_config *config;
+};
+
+struct sp_otp_vX_t {
+	int size;
+};
+
+const struct sp_otp_vX_t  sp_otp_v0 = {
+	.size = QAC628_OTP_SIZE,
+};
+
+static int sp_otp_wait(void __iomem *reg_base)
+{
+	int timeout = OTP_READ_TIMEOUT;
+	unsigned int status;
+
+	do {
+		usleep_range(10);
+		if (timeout-- == 0)
+			return -ETIMEDOUT;
+
+		status = readl(reg_base + OTP_STATUS);
+	} while ((status & OTP_READ_DONE) != OTP_READ_DONE);
+
+	return 0;
+}
+
+static int sp_otp_read_real(struct sp_otp_data_t *otp, int addr, char *value)
+{
+	unsigned int addr_data;
+	unsigned int byte_shift;
+	int ret = 0;
+
+	addr_data = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
+	addr_data = addr_data / OTP_WORD_SIZE;
+
+	byte_shift = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
+	byte_shift = byte_shift % OTP_WORD_SIZE;
+
+	addr = addr / (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
+	addr = addr * OTP_BIT_ADDR_OF_BANK;
+
+	writel(readl(otp->base[OTPRX] + OTP_STATUS) & OTP_READ_DONE_MASK &
+		     OTP_LOAD_SECURE_DONE_MASK, otp->base[OTPRX] + OTP_STATUS);
+	writel(addr, otp->base[OTPRX] + OTP_READ_ADDRESS);
+	writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) | OTP_READ,
+	       otp->base[OTPRX] + OTP_CONTROL_2);
+	writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) & SEL_BAK_KEY2_MASK & SW_TRIM_EN_MASK
+	       & SEL_BAK_KEY_MASK & OTP_LOAD_SECURE_DATA_MASK & OTP_DO_CRC_MASK,
+	       otp->base[OTPRX] + OTP_CONTROL_2);
+	writel((readl(otp->base[OTPRX] + OTP_CONTROL_2) & OTP_RD_PERIOD_MASK) |
+	       ((ONE_CPU_CLOCK * 0x1e) << 8), otp->base[OTPRX] + OTP_CONTROL_2);
+
+	ret = sp_otp_wait(otp->base[OTPRX]);
+	if (ret < 0)
+		return ret;
+
+	*value = (readl(otp->base[HB_GPIO] + ADDRESS_8_DATA + addr_data * OTP_WORD_SIZE)
+			>> (8 * byte_shift)) & 0xFF;
+
+	return ret;
+}
+
+static int sp_ocotp_read(void *priv, unsigned int offset, void *value, size_t bytes)
+{
+	struct sp_otp_data_t *otp = priv;
+	unsigned int addr;
+	char *buf = value;
+	char val[4];
+	int ret;
+
+	dev_dbg(otp->dev, "OTP read %u bytes at %u", bytes, offset);
+
+	if (offset >= QAC628_OTP_SIZE || bytes == 0 || ((offset + bytes) > QAC628_OTP_SIZE))
+		return -EINVAL;
+
+	ret = clk_enable(otp->clk);
+	if (ret)
+		return ret;
+
+	*buf = 0;
+	for (addr = offset; addr < (offset + bytes); addr++) {
+		ret = sp_otp_read_real(otp, addr, val);
+		if (ret < 0) {
+			dev_err(otp->dev, "OTP read fail:%d at %d", ret, addr);
+			goto disable_clk;
+		}
+
+		*buf++ = *val;
+	}
+
+disable_clk:
+	clk_disable(otp->clk);
+	dev_dbg(otp->dev, "OTP read complete");
+
+	return ret;
+}
+
+static struct nvmem_config sp_ocotp_nvmem_config = {
+	.name = "sp-ocotp",
+	.read_only = true,
+	.word_size = 1,
+	.size = QAC628_OTP_SIZE,
+	.stride = 1,
+	.reg_read = sp_ocotp_read,
+	.owner = THIS_MODULE,
+};
+
+static int sp_ocotp_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct sp_otp_vX_t *sp_otp_vX;
+	struct device *dev = &pdev->dev;
+	struct nvmem_device *nvmem;
+	struct sp_otp_data_t *otp;
+	struct resource *res;
+	int ret;
+
+	match = of_match_device(dev->driver->of_match_table, dev);
+	if (match && match->data)
+		sp_otp_vX = match->data;
+	else
+		dev_err(dev, "OTP vX does not match");
+
+	otp = devm_kzalloc(dev, sizeof(*otp), GFP_KERNEL);
+	if (!otp)
+		return -ENOMEM;
+
+	otp->dev = dev;
+	otp->config = &sp_ocotp_nvmem_config;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hb_gpio");
+	otp->base[HB_GPIO] = devm_ioremap_resource(dev, res);
+	if (IS_ERR(otp->base[HB_GPIO]))
+		return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[HB_GPIO]),
+						"hb_gpio devm_ioremap_resource fail\n");
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otprx");
+	otp->base[OTPRX] = devm_ioremap_resource(dev, res);
+	if (IS_ERR(otp->base[OTPRX]))
+		return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[OTPRX]),
+						"otprx devm_ioremap_resource fail\n");
+
+	otp->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(otp->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(otp->clk),
+						"devm_clk_get fail\n");
+
+	ret = clk_prepare(otp->clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to prepare clk: %d\n", ret);
+		return ret;
+	}
+
+	sp_ocotp_nvmem_config.priv = otp;
+	sp_ocotp_nvmem_config.dev = dev;
+
+	nvmem = devm_nvmem_register(dev, &sp_ocotp_nvmem_config);
+	if (IS_ERR(nvmem))
+		return dev_err_probe(&pdev->dev, PTR_ERR(nvmem),
+						"register nvmem device fail\n");
+
+	platform_set_drvdata(pdev, nvmem);
+
+	dev_dbg(dev, "banks:%d x wpb:%d x wsize:%d = %d",
+		QAC628_OTP_NUM_BANKS, OTP_WORDS_PER_BANK,
+		OTP_WORD_SIZE, QAC628_OTP_SIZE);
+
+	dev_info(dev, "by Sunplus (C) 2020");
+
+	return 0;
+}
+
+static const struct of_device_id sp_ocotp_dt_ids[] = {
+	{ .compatible = "sunplus,sp7021-ocotp", .data = &sp_otp_v0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sp_ocotp_dt_ids);
+
+static struct platform_driver sp_otp_driver = {
+	.probe     = sp_ocotp_probe,
+	.driver    = {
+		.name           = "sunplus,sp7021-ocotp",
+		.of_match_table = sp_ocotp_dt_ids,
+	}
+};
+
+static int __init sp_otp0_drv_new(void)
+{
+	return platform_driver_register(&sp_otp_driver);
+}
+subsys_initcall(sp_otp0_drv_new);
+
+static void __exit sp_otp0_drv_del(void)
+{
+	platform_driver_unregister(&sp_otp_driver);
+}
+module_exit(sp_otp0_drv_del);
+
+MODULE_AUTHOR("Vincent Shih <vincent.sunplus@gmail.com>");
+MODULE_DESCRIPTION("Sunplus On-Chip OTP driver");
+MODULE_LICENSE("GPL v2");
+
-- 
2.7.4

Re: [PATCH v2 1/2] nvmem: Add driver for OCOTP in Sunplus SP7021

[Srinivas Kandagatla]

On 07/12/2021 08:53, Vincent Shih wrote:
> Add driver for OCOTP in Sunplus SP7021
> 
> Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> ---
> Changes in v2:
>   - Merge sunplus-ocotp.h and sunplus-ocotp0.c to sunplus-ocotp.c
>   - Clean up codes.
> 
>   MAINTAINERS                   |   5 +
>   drivers/nvmem/Kconfig         |  12 ++
>   drivers/nvmem/Makefile        |   2 +
>   drivers/nvmem/sunplus-ocotp.c | 268 ++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 287 insertions(+)
>   create mode 100644 drivers/nvmem/sunplus-ocotp.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80eebc1..0e6593a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17947,6 +17947,11 @@ L:	netdev@vger.kernel.org
>   S:	Maintained
>   F:	drivers/net/ethernet/dlink/sundance.c
>   
> +SUNPLUS OCOTP DRIVER
> +M:	Vincent Shih <vincent.sunplus@gmail.com>
> +S:	Maintained
> +F:	drivers/nvmem/sunplus-ocotp.c
> +
>   SUPERH
>   M:	Yoshinori Sato <ysato@users.sourceforge.jp>
>   M:	Rich Felker <dalias@libc.org>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index da41461..fb053d6 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -300,4 +300,16 @@ config NVMEM_BRCM_NVRAM
>   	  This driver provides support for Broadcom's NVRAM that can be accessed
>   	  using I/O mapping.
>   
> +config NVMEM_SUNPLUS_OCOTP
> +	tristate "Sunplus SoC OTP support"
> +	depends on SOC_SP7021

COMPILE_TEST ?

> +	depends on HAS_IOMEM
> +	help
> +	  This is a driver for the On-chip OTP controller (OCOTP) available
> +	  on Sunplus SoCs. It provids access to 128 bytes of one-time

s/provids/provides

> +	  programmable eFuse.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nvmem-sunplus-ocotp.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index dcbbde3..0f14cd9 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -61,3 +61,5 @@ obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
>   nvmem-rmem-y			:= rmem.o
>   obj-$(CONFIG_NVMEM_BRCM_NVRAM)	+= nvmem_brcm_nvram.o
>   nvmem_brcm_nvram-y		:= brcm_nvram.o
> +obj-$(CONFIG_NVMEM_SUNPLUS_OCOTP)	+= nvmem_sunplus_ocotp.o
> +nvmem_sunplus_ocotp-y		:= sunplus-ocotp.o
> diff --git a/drivers/nvmem/sunplus-ocotp.c b/drivers/nvmem/sunplus-ocotp.c
> new file mode 100644
> index 0000000..2997b29
> --- /dev/null
> +++ b/drivers/nvmem/sunplus-ocotp.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * The OCOTP driver for Sunplus	SP7021
> + *
> + * Copyright (C) 2019 Sunplus Technology Inc., All rights reseerved.

reserved ?

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * OTP memory
> + * Each bank contains 4 words (32 bits).
> + * Bank 0 starts at offset 0 from the base.
> + */
> +
> +#define OTP_WORDS_PER_BANK		4
> +#define OTP_WORD_SIZE			sizeof(u32)
> +#define OTP_BIT_ADDR_OF_BANK		(8 * OTP_WORD_SIZE * OTP_WORDS_PER_BANK)
> +#define QAC628_OTP_NUM_BANKS            8
> +#define QAC628_OTP_SIZE                 (QAC628_OTP_NUM_BANKS * OTP_WORDS_PER_BANK * OTP_WORD_SIZE)
> +#define OTP_READ_TIMEOUT                20000
> +
> +/* HB_GPIO */
> +#define ADDRESS_8_DATA			0x20
> +
> +/* OTP_RX */
> +#define OTP_CONTROL_2			0x48
> +#define OTP_RD_PERIOD			GENMASK(15, 8)
> +#define OTP_RD_PERIOD_MASK		~GENMASK(15, 8)
> +#define ONE_CPU_CLOCK			0x1
> +#define SEL_BAK_KEY2			BIT(5)
> +#define SEL_BAK_KEY2_MASK		~BIT(5)
> +#define SW_TRIM_EN			BIT(4)
> +#define SW_TRIM_EN_MASK			~BIT(4)
> +#define SEL_BAK_KEY			BIT(3)
> +#define SEL_BAK_KEY_MASK		~BIT(3)
> +#define OTP_READ			BIT(2)
> +#define OTP_LOAD_SECURE_DATA		BIT(1)
> +#define OTP_LOAD_SECURE_DATA_MASK	~BIT(1)
> +#define OTP_DO_CRC			BIT(0)
> +#define OTP_DO_CRC_MASK			~BIT(0)
> +#define OTP_STATUS			0x4c
> +#define OTP_READ_DONE			BIT(4)
> +#define OTP_READ_DONE_MASK		~BIT(4)
> +#define OTP_LOAD_SECURE_DONE_MASK	~BIT(2)
> +#define OTP_READ_ADDRESS		0x50
> +
> +enum base_type {
> +	HB_GPIO,
> +	OTPRX,
> +	BASEMAX,
> +};
> +
> +struct sp_otp_data_t {
> +	struct device *dev;
> +	void __iomem *base[BASEMAX];
> +	struct clk *clk;
> +	struct nvmem_config *config;

totally redundant to store config in this stucture as you will never use 
this after probe.

> +};
> +
> +struct sp_otp_vX_t {

does X needs to be caps?

> +	int size;
> +};
> +
> +const struct sp_otp_vX_t  sp_otp_v0 = {
> +	.size = QAC628_OTP_SIZE,
> +};
> +
> +static int sp_otp_wait(void __iomem *reg_base)
> +{
> +	int timeout = OTP_READ_TIMEOUT;
> +	unsigned int status;
> +
> +	do {
> +		usleep_range(10);

Doesn't this take two arguments?

> +		if (timeout-- == 0)
> +			return -ETIMEDOUT;
> +
> +		status = readl(reg_base + OTP_STATUS);
> +	} while ((status & OTP_READ_DONE) != OTP_READ_DONE);
> +
> +	return 0;
> +}
> +
> +static int sp_otp_read_real(struct sp_otp_data_t *otp, int addr, char *value)
> +{
> +	unsigned int addr_data;
> +	unsigned int byte_shift;
> +	int ret = 0;

unnecessary initializatoin here.

> +
> +	addr_data = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> +	addr_data = addr_data / OTP_WORD_SIZE;
> +
> +	byte_shift = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> +	byte_shift = byte_shift % OTP_WORD_SIZE;
> +
> +	addr = addr / (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> +	addr = addr * OTP_BIT_ADDR_OF_BANK;
> +
> +	writel(readl(otp->base[OTPRX] + OTP_STATUS) & OTP_READ_DONE_MASK &
> +		     OTP_LOAD_SECURE_DONE_MASK, otp->base[OTPRX] + OTP_STATUS);
> +	writel(addr, otp->base[OTPRX] + OTP_READ_ADDRESS);
> +	writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) | OTP_READ,
> +	       otp->base[OTPRX] + OTP_CONTROL_2);
> +	writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) & SEL_BAK_KEY2_MASK & SW_TRIM_EN_MASK
> +	       & SEL_BAK_KEY_MASK & OTP_LOAD_SECURE_DATA_MASK & OTP_DO_CRC_MASK,
> +	       otp->base[OTPRX] + OTP_CONTROL_2);
> +	writel((readl(otp->base[OTPRX] + OTP_CONTROL_2) & OTP_RD_PERIOD_MASK) |
> +	       ((ONE_CPU_CLOCK * 0x1e) << 8), otp->base[OTPRX] + OTP_CONTROL_2);
> +
> +	ret = sp_otp_wait(otp->base[OTPRX]);
> +	if (ret < 0)
> +		return ret;
> +
> +	*value = (readl(otp->base[HB_GPIO] + ADDRESS_8_DATA + addr_data * OTP_WORD_SIZE)
> +			>> (8 * byte_shift)) & 0xFF;
> +
> +	return ret;
> +}
> +
> +static int sp_ocotp_read(void *priv, unsigned int offset, void *value, size_t bytes)
> +{
> +	struct sp_otp_data_t *otp = priv;
> +	unsigned int addr;
> +	char *buf = value;
> +	char val[4];
> +	int ret;
> +
> +	dev_dbg(otp->dev, "OTP read %u bytes at %u", bytes, offset);
> +
why do we need all these debug ?

> +	if (offset >= QAC628_OTP_SIZE || bytes == 0 || ((offset + bytes) > QAC628_OTP_SIZE))
> +		return -EINVAL;

Core should already do this sanity test, if not these checks belong to 
nvmem core.

> +
> +	ret = clk_enable(otp->clk);
> +	if (ret)
> +		return ret;
> +
> +	*buf = 0;
> +	for (addr = offset; addr < (offset + bytes); addr++) {
> +		ret = sp_otp_read_real(otp, addr, val);
> +		if (ret < 0) {
> +			dev_err(otp->dev, "OTP read fail:%d at %d", ret, addr);
> +			goto disable_clk;
> +		}
> +
> +		*buf++ = *val;
> +	}
> +
> +disable_clk:
> +	clk_disable(otp->clk);
> +	dev_dbg(otp->dev, "OTP read complete");
> +
> +	return ret;
> +}
> +
> +static struct nvmem_config sp_ocotp_nvmem_config = {
> +	.name = "sp-ocotp",
> +	.read_only = true,
> +	.word_size = 1,
> +	.size = QAC628_OTP_SIZE,
> +	.stride = 1,
> +	.reg_read = sp_ocotp_read,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sp_ocotp_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct sp_otp_vX_t *sp_otp_vX;
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct sp_otp_data_t *otp;
> +	struct resource *res;
> +	int ret;
> +

<---
> +	match = of_match_device(dev->driver->of_match_table, dev);
> +	if (match && match->data)
> +		sp_otp_vX = match->data;
> +	else
> +		dev_err(dev, "OTP vX does not match");
--->

This looks like dead code, variable is set but not used anywhere.
Error case is ignored.


> +
> +	otp = devm_kzalloc(dev, sizeof(*otp), GFP_KERNEL);
> +	if (!otp)
> +		return -ENOMEM;
> +
> +	otp->dev = dev;
> +	otp->config = &sp_ocotp_nvmem_config;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hb_gpio");
> +	otp->base[HB_GPIO] = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(otp->base[HB_GPIO]))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[HB_GPIO]),
> +						"hb_gpio devm_ioremap_resource fail\n");

printing error here is totally redundant.

you could just do

if (IS_ERR(otp->base[HB_GPIO]))
	return PTR_ERR(otp->base[HB_GPIO]);


> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otprx");
> +	otp->base[OTPRX] = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(otp->base[OTPRX]))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[OTPRX]),
> +						"otprx devm_ioremap_resource fail\n");
> +
same here,

> +	otp->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(otp->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(otp->clk),
> +						"devm_clk_get fail\n");
> +
> +	ret = clk_prepare(otp->clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to prepare clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	sp_ocotp_nvmem_config.priv = otp;
> +	sp_ocotp_nvmem_config.dev = dev;
> +
> +	nvmem = devm_nvmem_register(dev, &sp_ocotp_nvmem_config);
> +	if (IS_ERR(nvmem))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(nvmem),
> +						"register nvmem device fail\n");
> +
> +	platform_set_drvdata(pdev, nvmem);
> +
> +	dev_dbg(dev, "banks:%d x wpb:%d x wsize:%d = %d",
> +		QAC628_OTP_NUM_BANKS, OTP_WORDS_PER_BANK,
> +		OTP_WORD_SIZE, QAC628_OTP_SIZE);
> +
> +	dev_info(dev, "by Sunplus (C) 2020");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sp_ocotp_dt_ids[] = {
> +	{ .compatible = "sunplus,sp7021-ocotp", .data = &sp_otp_v0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sp_ocotp_dt_ids);
> +
> +static struct platform_driver sp_otp_driver = {
> +	.probe     = sp_ocotp_probe,
> +	.driver    = {
> +		.name           = "sunplus,sp7021-ocotp",
> +		.of_match_table = sp_ocotp_dt_ids,
> +	}
> +};
> +
> +static int __init sp_otp0_drv_new(void)
> +{
> +	return platform_driver_register(&sp_otp_driver);
> +}
> +subsys_initcall(sp_otp0_drv_new);

Why this needs to be subsys_initcall() why can't it be module_init?


> +
> +static void __exit sp_otp0_drv_del(void)
> +{
> +	platform_driver_unregister(&sp_otp_driver);
> +}
> +module_exit(sp_otp0_drv_del);
> +
> +MODULE_AUTHOR("Vincent Shih <vincent.sunplus@gmail.com>");
> +MODULE_DESCRIPTION("Sunplus On-Chip OTP driver");
> +MODULE_LICENSE("GPL v2");

Just GPL should be good here.

--srini
> +
> 

Re: [PATCH v2 1/2] nvmem: Add driver for OCOTP in Sunplus SP7021

[施錕鴻]

hello

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 於 2021年12月9日 週四 下午11:50寫道：
>
>
>
> On 07/12/2021 08:53, Vincent Shih wrote:
> > Add driver for OCOTP in Sunplus SP7021
> >
> > Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> > ---
> > Changes in v2:
> >   - Merge sunplus-ocotp.h and sunplus-ocotp0.c to sunplus-ocotp.c
> >   - Clean up codes.
> >
> >   MAINTAINERS                   |   5 +
> >   drivers/nvmem/Kconfig         |  12 ++
> >   drivers/nvmem/Makefile        |   2 +
> >   drivers/nvmem/sunplus-ocotp.c | 268 ++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 287 insertions(+)
> >   create mode 100644 drivers/nvmem/sunplus-ocotp.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 80eebc1..0e6593a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17947,6 +17947,11 @@ L:   netdev@vger.kernel.org
> >   S:  Maintained
> >   F:  drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS OCOTP DRIVER
> > +M:   Vincent Shih <vincent.sunplus@gmail.com>
> > +S:   Maintained
> > +F:   drivers/nvmem/sunplus-ocotp.c
> > +
> >   SUPERH
> >   M:  Yoshinori Sato <ysato@users.sourceforge.jp>
> >   M:  Rich Felker <dalias@libc.org>
> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > index da41461..fb053d6 100644
> > --- a/drivers/nvmem/Kconfig
> > +++ b/drivers/nvmem/Kconfig
> > @@ -300,4 +300,16 @@ config NVMEM_BRCM_NVRAM
> >         This driver provides support for Broadcom's NVRAM that can be accessed
> >         using I/O mapping.
> >
> > +config NVMEM_SUNPLUS_OCOTP
> > +     tristate "Sunplus SoC OTP support"
> > +     depends on SOC_SP7021
>
> COMPILE_TEST ?
>

Yes, I will add it.

> > +     depends on HAS_IOMEM
> > +     help
> > +       This is a driver for the On-chip OTP controller (OCOTP) available
> > +       on Sunplus SoCs. It provids access to 128 bytes of one-time
>
> s/provids/provides
>

Yes, I will modify it.


> > +       programmable eFuse.
> > +
> > +       This driver can also be built as a module. If so, the module
> > +       will be called nvmem-sunplus-ocotp.
> > +
> >   endif
> > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> > index dcbbde3..0f14cd9 100644
> > --- a/drivers/nvmem/Makefile
> > +++ b/drivers/nvmem/Makefile
> > @@ -61,3 +61,5 @@ obj-$(CONFIG_NVMEM_RMEM)    += nvmem-rmem.o
> >   nvmem-rmem-y                        := rmem.o
> >   obj-$(CONFIG_NVMEM_BRCM_NVRAM)      += nvmem_brcm_nvram.o
> >   nvmem_brcm_nvram-y          := brcm_nvram.o
> > +obj-$(CONFIG_NVMEM_SUNPLUS_OCOTP)    += nvmem_sunplus_ocotp.o
> > +nvmem_sunplus_ocotp-y                := sunplus-ocotp.o
> > diff --git a/drivers/nvmem/sunplus-ocotp.c b/drivers/nvmem/sunplus-ocotp.c
> > new file mode 100644
> > index 0000000..2997b29
> > --- /dev/null
> > +++ b/drivers/nvmem/sunplus-ocotp.c
> > @@ -0,0 +1,268 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * The OCOTP driver for Sunplus      SP7021
> > + *
> > + * Copyright (C) 2019 Sunplus Technology Inc., All rights reseerved.
>
> reserved ?
>

Yes, I will modify it.

> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +/*
> > + * OTP memory
> > + * Each bank contains 4 words (32 bits).
> > + * Bank 0 starts at offset 0 from the base.
> > + */
> > +
> > +#define OTP_WORDS_PER_BANK           4
> > +#define OTP_WORD_SIZE                        sizeof(u32)
> > +#define OTP_BIT_ADDR_OF_BANK         (8 * OTP_WORD_SIZE * OTP_WORDS_PER_BANK)
> > +#define QAC628_OTP_NUM_BANKS            8
> > +#define QAC628_OTP_SIZE                 (QAC628_OTP_NUM_BANKS * OTP_WORDS_PER_BANK * OTP_WORD_SIZE)
> > +#define OTP_READ_TIMEOUT                20000
> > +
> > +/* HB_GPIO */
> > +#define ADDRESS_8_DATA                       0x20
> > +
> > +/* OTP_RX */
> > +#define OTP_CONTROL_2                        0x48
> > +#define OTP_RD_PERIOD                        GENMASK(15, 8)
> > +#define OTP_RD_PERIOD_MASK           ~GENMASK(15, 8)
> > +#define ONE_CPU_CLOCK                        0x1
> > +#define SEL_BAK_KEY2                 BIT(5)
> > +#define SEL_BAK_KEY2_MASK            ~BIT(5)
> > +#define SW_TRIM_EN                   BIT(4)
> > +#define SW_TRIM_EN_MASK                      ~BIT(4)
> > +#define SEL_BAK_KEY                  BIT(3)
> > +#define SEL_BAK_KEY_MASK             ~BIT(3)
> > +#define OTP_READ                     BIT(2)
> > +#define OTP_LOAD_SECURE_DATA         BIT(1)
> > +#define OTP_LOAD_SECURE_DATA_MASK    ~BIT(1)
> > +#define OTP_DO_CRC                   BIT(0)
> > +#define OTP_DO_CRC_MASK                      ~BIT(0)
> > +#define OTP_STATUS                   0x4c
> > +#define OTP_READ_DONE                        BIT(4)
> > +#define OTP_READ_DONE_MASK           ~BIT(4)
> > +#define OTP_LOAD_SECURE_DONE_MASK    ~BIT(2)
> > +#define OTP_READ_ADDRESS             0x50
> > +
> > +enum base_type {
> > +     HB_GPIO,
> > +     OTPRX,
> > +     BASEMAX,
> > +};
> > +
> > +struct sp_otp_data_t {
> > +     struct device *dev;
> > +     void __iomem *base[BASEMAX];
> > +     struct clk *clk;
> > +     struct nvmem_config *config;
>
> totally redundant to store config in this stucture as you will never use
> this after probe.
>

Yes, I will remove it.

> > +};
> > +
> > +struct sp_otp_vX_t {
>
> does X needs to be caps?
>

Yes, I will modify it.

> > +     int size;
> > +};
> > +
> > +const struct sp_otp_vX_t  sp_otp_v0 = {
> > +     .size = QAC628_OTP_SIZE,
> > +};
> > +
> > +static int sp_otp_wait(void __iomem *reg_base)
> > +{
> > +     int timeout = OTP_READ_TIMEOUT;
> > +     unsigned int status;
> > +
> > +     do {
> > +             usleep_range(10);
>
> Doesn't this take two arguments?
>

I will replace sp_otp_wait() with readl_poll_timeout().

> > +             if (timeout-- == 0)
> > +                     return -ETIMEDOUT;
> > +
> > +             status = readl(reg_base + OTP_STATUS);
> > +     } while ((status & OTP_READ_DONE) != OTP_READ_DONE);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sp_otp_read_real(struct sp_otp_data_t *otp, int addr, char *value)
> > +{
> > +     unsigned int addr_data;
> > +     unsigned int byte_shift;
> > +     int ret = 0;
>
> unnecessary initializatoin here.
>

Yes, I will modify it.

> > +
> > +     addr_data = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> > +     addr_data = addr_data / OTP_WORD_SIZE;
> > +
> > +     byte_shift = addr % (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> > +     byte_shift = byte_shift % OTP_WORD_SIZE;
> > +
> > +     addr = addr / (OTP_WORD_SIZE * OTP_WORDS_PER_BANK);
> > +     addr = addr * OTP_BIT_ADDR_OF_BANK;
> > +
> > +     writel(readl(otp->base[OTPRX] + OTP_STATUS) & OTP_READ_DONE_MASK &
> > +                  OTP_LOAD_SECURE_DONE_MASK, otp->base[OTPRX] + OTP_STATUS);
> > +     writel(addr, otp->base[OTPRX] + OTP_READ_ADDRESS);
> > +     writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) | OTP_READ,
> > +            otp->base[OTPRX] + OTP_CONTROL_2);
> > +     writel(readl(otp->base[OTPRX] + OTP_CONTROL_2) & SEL_BAK_KEY2_MASK & SW_TRIM_EN_MASK
> > +            & SEL_BAK_KEY_MASK & OTP_LOAD_SECURE_DATA_MASK & OTP_DO_CRC_MASK,
> > +            otp->base[OTPRX] + OTP_CONTROL_2);
> > +     writel((readl(otp->base[OTPRX] + OTP_CONTROL_2) & OTP_RD_PERIOD_MASK) |
> > +            ((ONE_CPU_CLOCK * 0x1e) << 8), otp->base[OTPRX] + OTP_CONTROL_2);
> > +
> > +     ret = sp_otp_wait(otp->base[OTPRX]);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     *value = (readl(otp->base[HB_GPIO] + ADDRESS_8_DATA + addr_data * OTP_WORD_SIZE)
> > +                     >> (8 * byte_shift)) & 0xFF;
> > +
> > +     return ret;
> > +}
> > +
> > +static int sp_ocotp_read(void *priv, unsigned int offset, void *value, size_t bytes)
> > +{
> > +     struct sp_otp_data_t *otp = priv;
> > +     unsigned int addr;
> > +     char *buf = value;
> > +     char val[4];
> > +     int ret;
> > +
> > +     dev_dbg(otp->dev, "OTP read %u bytes at %u", bytes, offset);
> > +
> why do we need all these debug ?
>

Yes, I will remove it.

> > +     if (offset >= QAC628_OTP_SIZE || bytes == 0 || ((offset + bytes) > QAC628_OTP_SIZE))
> > +             return -EINVAL;
>
> Core should already do this sanity test, if not these checks belong to
> nvmem core.
>

Yes, I will remove it.

> > +
> > +     ret = clk_enable(otp->clk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *buf = 0;
> > +     for (addr = offset; addr < (offset + bytes); addr++) {
> > +             ret = sp_otp_read_real(otp, addr, val);
> > +             if (ret < 0) {
> > +                     dev_err(otp->dev, "OTP read fail:%d at %d", ret, addr);
> > +                     goto disable_clk;
> > +             }
> > +
> > +             *buf++ = *val;
> > +     }
> > +
> > +disable_clk:
> > +     clk_disable(otp->clk);
> > +     dev_dbg(otp->dev, "OTP read complete");
> > +
> > +     return ret;
> > +}
> > +
> > +static struct nvmem_config sp_ocotp_nvmem_config = {
> > +     .name = "sp-ocotp",
> > +     .read_only = true,
> > +     .word_size = 1,
> > +     .size = QAC628_OTP_SIZE,
> > +     .stride = 1,
> > +     .reg_read = sp_ocotp_read,
> > +     .owner = THIS_MODULE,
> > +};
> > +
> > +static int sp_ocotp_probe(struct platform_device *pdev)
> > +{
> > +     const struct of_device_id *match;
> > +     const struct sp_otp_vX_t *sp_otp_vX;
> > +     struct device *dev = &pdev->dev;
> > +     struct nvmem_device *nvmem;
> > +     struct sp_otp_data_t *otp;
> > +     struct resource *res;
> > +     int ret;
> > +
>
> <---
> > +     match = of_match_device(dev->driver->of_match_table, dev);
> > +     if (match && match->data)
> > +             sp_otp_vX = match->data;
> > +     else
> > +             dev_err(dev, "OTP vX does not match");
> --->
>
> This looks like dead code, variable is set but not used anywhere.
> Error case is ignored.
>

Yes, I will remove it.

>
> > +
> > +     otp = devm_kzalloc(dev, sizeof(*otp), GFP_KERNEL);
> > +     if (!otp)
> > +             return -ENOMEM;
> > +
> > +     otp->dev = dev;
> > +     otp->config = &sp_ocotp_nvmem_config;
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hb_gpio");
> > +     otp->base[HB_GPIO] = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(otp->base[HB_GPIO]))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[HB_GPIO]),
> > +                                             "hb_gpio devm_ioremap_resource fail\n");
>
> printing error here is totally redundant.
>
> you could just do
>
> if (IS_ERR(otp->base[HB_GPIO]))
>         return PTR_ERR(otp->base[HB_GPIO]);
>

Yes, I will modify it.

>
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otprx");
> > +     otp->base[OTPRX] = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(otp->base[OTPRX]))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(otp->base[OTPRX]),
> > +                                             "otprx devm_ioremap_resource fail\n");
> > +
> same here,
>

Yes, I will modify it.

> > +     otp->clk = devm_clk_get(&pdev->dev, NULL);
> > +     if (IS_ERR(otp->clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(otp->clk),
> > +                                             "devm_clk_get fail\n");
> > +
> > +     ret = clk_prepare(otp->clk);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to prepare clk: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     sp_ocotp_nvmem_config.priv = otp;
> > +     sp_ocotp_nvmem_config.dev = dev;
> > +
> > +     nvmem = devm_nvmem_register(dev, &sp_ocotp_nvmem_config);
> > +     if (IS_ERR(nvmem))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(nvmem),
> > +                                             "register nvmem device fail\n");
> > +
> > +     platform_set_drvdata(pdev, nvmem);
> > +
> > +     dev_dbg(dev, "banks:%d x wpb:%d x wsize:%d = %d",
> > +             QAC628_OTP_NUM_BANKS, OTP_WORDS_PER_BANK,
> > +             OTP_WORD_SIZE, QAC628_OTP_SIZE);
> > +
> > +     dev_info(dev, "by Sunplus (C) 2020");
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id sp_ocotp_dt_ids[] = {
> > +     { .compatible = "sunplus,sp7021-ocotp", .data = &sp_otp_v0 },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sp_ocotp_dt_ids);
> > +
> > +static struct platform_driver sp_otp_driver = {
> > +     .probe     = sp_ocotp_probe,
> > +     .driver    = {
> > +             .name           = "sunplus,sp7021-ocotp",
> > +             .of_match_table = sp_ocotp_dt_ids,
> > +     }
> > +};
> > +
> > +static int __init sp_otp0_drv_new(void)
> > +{
> > +     return platform_driver_register(&sp_otp_driver);
> > +}
> > +subsys_initcall(sp_otp0_drv_new);
>
> Why this needs to be subsys_initcall() why can't it be module_init?
>

The otp driver stores the mac address for the ethernet driver and
disconnect voltage for USB2 one.
Therefore, it must take priority over the other ones for the probe.

>
> > +
> > +static void __exit sp_otp0_drv_del(void)
> > +{
> > +     platform_driver_unregister(&sp_otp_driver);
> > +}
> > +module_exit(sp_otp0_drv_del);
> > +
> > +MODULE_AUTHOR("Vincent Shih <vincent.sunplus@gmail.com>");
> > +MODULE_DESCRIPTION("Sunplus On-Chip OTP driver");
> > +MODULE_LICENSE("GPL v2");
>
> Just GPL should be good here.
>

Yes, I will modify it.

> --srini
> > +
> >

Thanks for your review.

Re: [PATCH v2 1/2] nvmem: Add driver for OCOTP in Sunplus SP7021

[Srinivas Kandagatla]

On 10/12/2021 08:50, 施錕鴻 wrote:
> 
>      > +
>      > +static const struct of_device_id sp_ocotp_dt_ids[] = {
>      > +     { .compatible = "sunplus,sp7021-ocotp", .data = &sp_otp_v0 },
>      > +     { }
>      > +};
>      > +MODULE_DEVICE_TABLE(of, sp_ocotp_dt_ids);
>      > +
>      > +static struct platform_driver sp_otp_driver = {
>      > +     .probe     = sp_ocotp_probe,
>      > +     .driver    = {
>      > +             .name           = "sunplus,sp7021-ocotp",
>      > +             .of_match_table = sp_ocotp_dt_ids,
>      > +     }
>      > +};
>      > +
>      > +static int __init sp_otp0_drv_new(void)
>      > +{
>      > +     return platform_driver_register(&sp_otp_driver);
>      > +}
>      > +subsys_initcall(sp_otp0_drv_new);
> 
>     Why this needs to be subsys_initcall() why can't it be module_init?
> 
>   The OTP driver will store the mac address for ethernet driver and usb2 
> disconnect voltage
>   for usb2 one. Therefore the order of the OTP driver should take higher 
> priority over the
> other ones.

drivers that are looking for mac address or other values from nvmem 
should get an EPROBE_DEFER if nvmem provider is not ready.

does that not work for you?


--srini

Re: [PATCH v2 1/2] nvmem: Add driver for OCOTP in Sunplus SP7021

[施錕鴻]

hello

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 於 2021年12月14日 週二 下午5:08寫道：
>
>
>
> On 10/12/2021 08:50, 施錕鴻 wrote:
> >
> >      > +
> >      > +static const struct of_device_id sp_ocotp_dt_ids[] = {
> >      > +     { .compatible = "sunplus,sp7021-ocotp", .data = &sp_otp_v0 },
> >      > +     { }
> >      > +};
> >      > +MODULE_DEVICE_TABLE(of, sp_ocotp_dt_ids);
> >      > +
> >      > +static struct platform_driver sp_otp_driver = {
> >      > +     .probe     = sp_ocotp_probe,
> >      > +     .driver    = {
> >      > +             .name           = "sunplus,sp7021-ocotp",
> >      > +             .of_match_table = sp_ocotp_dt_ids,
> >      > +     }
> >      > +};
> >      > +
> >      > +static int __init sp_otp0_drv_new(void)
> >      > +{
> >      > +     return platform_driver_register(&sp_otp_driver);
> >      > +}
> >      > +subsys_initcall(sp_otp0_drv_new);
> >
> >     Why this needs to be subsys_initcall() why can't it be module_init?
> >
> >   The OTP driver will store the mac address for ethernet driver and usb2
> > disconnect voltage
> >   for usb2 one. Therefore the order of the OTP driver should take higher
> > priority over the
> > other ones.
>
> drivers that are looking for mac address or other values from nvmem
> should get an EPROBE_DEFER if nvmem provider is not ready.
>
> does that not work for you?
>
>
> --srini

After modification and verification, I found the algorithm of
EPROBE_DEFER worked.
I will replace subsys_initcall() with module_init() for the driver.
Thanks for your review.

[PATCH v2 2/2] dt-bindings: nvmem: Add bindings doc for Sunplus OCOTP driver

[Vincent Shih]

Add bindings doc for Sunplus OCOTP driver

Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
---
Changes in v2:
 - Address the comments from Mr. Rob Herring

 .../bindings/nvmem/sunplus,sp7021-ocotp.yaml       | 86 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml b/Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
new file mode 100644
index 0000000..4b28f37
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/sunplus,sp7021-ocotp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: On-Chip OTP Memory for Sunplus SP7021
+
+maintainers:
+  - Vincent Shih <vincent.sunplus@gmail.com>
+
+allOf:
+  - $ref: "nvmem.yaml#"
+
+properties:
+  compatible:
+    const: sunplus,sp7021-ocotp
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: hb_gpio
+      - const: otprx
+
+  clocks:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  thermal-calibration:
+    type: object
+    description: thermal calibration values
+
+  disconnect-voltage:
+    type: object
+    description: disconnect voltages of usb2 port 0 and port 1
+
+  mac-address0:
+    type: object
+    description: MAC address of ethernet port 0
+
+  mac-address1:
+    type: object
+    description: MAC address of ethernet port 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sp-sp7021.h>
+
+    otp: otp@9c00af00 {
+        compatible = "sunplus,sp7021-ocotp";
+        reg = <0x9c00af00 0x34>, <0x9c00af80 0x58>;
+        reg-names = "hb_gpio", "otprx";
+        clocks = <&clks OTPRX>;
+
+        #address-cells = <1>;
+        #size-cells = <1>;
+        therm_calib: thermal-calibration@14 {
+          reg = <0x14 0x3>;
+        };
+        disc_vol: disconnect-voltage@18 {
+          reg = <0x18 0x2>;
+        };
+        mac_addr0: mac-address0@34 {
+          reg = <0x34 0x6>;
+        };
+        mac_addr1: mac-address1@3a {
+          reg = <0x3a 0x6>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e6593a..2afd1ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17950,6 +17950,7 @@ F:	drivers/net/ethernet/dlink/sundance.c
 SUNPLUS OCOTP DRIVER
 M:	Vincent Shih <vincent.sunplus@gmail.com>
 S:	Maintained
+F:	Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
 F:	drivers/nvmem/sunplus-ocotp.c
 
 SUPERH
-- 
2.7.4

Re: [PATCH v2 2/2] dt-bindings: nvmem: Add bindings doc for Sunplus OCOTP driver

[Rob Herring]

On Tue, 07 Dec 2021 16:53:53 +0800, Vincent Shih wrote:
> Add bindings doc for Sunplus OCOTP driver
> 
> Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> ---
> Changes in v2:
>  - Address the comments from Mr. Rob Herring
> 
>  .../bindings/nvmem/sunplus,sp7021-ocotp.yaml       | 86 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>