mbox series

[v2,0/2] nvmem: lan9662-otpc: add support

Message ID 20220825204041.1485731-1-horatiu.vultur@microchip.com
Headers show
Series nvmem: lan9662-otpc: add support | expand

Message

Horatiu Vultur Aug. 25, 2022, 8:40 p.m. UTC 
Add support for lan9662 OTP controller that is available on lan9662.
The driver gives access to non-volatile memory. It allows both write
and read access to it.

v1->V2:
- remove unneed quotes for $ref: nvmem.yaml#
- rename lan966x to lan9662 as not to have any wildcards
- remove compatible string syscon

Horatiu Vultur (2):
  dt-bindings: lan9662-otpc: document Lan9662 OTPC
  nvmem: lan9662-otp: add support.

 .../nvmem/microchip,lan9662-otpc.yaml         |  42 +++
 drivers/nvmem/Kconfig                         |   8 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/lan9662-otpc.c                  | 249 ++++++++++++++++++
 4 files changed, 301 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
 create mode 100644 drivers/nvmem/lan9662-otpc.c

Comments

Krzysztof Kozlowski Aug. 26, 2022, 6:43 a.m. UTC | #1 
On 25/08/2022 23:40, Horatiu Vultur wrote:
> +static int lan9662_otp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct lan9662_otp *otp;
> +
> +	otp = devm_kzalloc(&pdev->dev, sizeof(*otp), GFP_KERNEL);
> +	if (!otp)
> +		return -ENOMEM;
> +
> +	otp->dev = dev;
> +	otp->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(otp->base))
> +		return PTR_ERR(otp->base);
> +
> +	otp_config.priv = otp;
> +	otp_config.dev = dev;
> +
> +	nvmem = devm_nvmem_register(dev, &otp_config);
> +
> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id lan9662_otp_match[] = {
> +	{ .compatible = "microchip,lan9662-otp", },
> +	{ .compatible = "microchip,lan9668-otp", },

Why do you need two compatibles here? Your bindings are saying only one
is needed...

Best regards,
Krzysztof
Srinivas Kandagatla Aug. 30, 2022, 12:08 p.m. UTC | #2 
On 25/08/2022 21:40, Horatiu Vultur wrote:
> Add support for OTP controller available on LAN9662. The OTPC controls
> the access to a non-volatile memory. The size of the memory is 8KB.
> The OTPC can access the memory based on an offset.
> Implement both the read and the write functionality.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>   drivers/nvmem/Kconfig        |   8 ++
>   drivers/nvmem/Makefile       |   2 +
>   drivers/nvmem/lan9662-otpc.c | 249 +++++++++++++++++++++++++++++++++++
>   3 files changed, 259 insertions(+)
>   create mode 100644 drivers/nvmem/lan9662-otpc.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 967d0084800e..c9929ec35a39 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -84,6 +84,14 @@ config NVMEM_LPC18XX_OTP
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called nvmem_lpc18xx_otp.
>   
> +config NVMEM_LAN9662_OTPC
> +	tristate "Microchip LAN9662 OTP controller support"
> +	depends on SOC_LAN966 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  This driver enables the OTP controller available on Microchip LAN9662
> +	  SoCs. It controlls the access to the OTP memory connected to it.
> +

s/controlls/controls/


>   config NVMEM_MXS_OCOTP
>   	tristate "Freescale MXS On-Chip OTP Memory Support"
>   	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 00e136a0a123..e1baface2c53 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -21,6 +21,8 @@ obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
>   nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
>   obj-$(CONFIG_NVMEM_LPC18XX_OTP)	+= nvmem_lpc18xx_otp.o
>   nvmem_lpc18xx_otp-y		:= lpc18xx_otp.o
> +obj-$(CONFIG_NVMEM_LAN9662_OTPC)	+= nvmem-lan9662-otpc.o
> +nvmem-lan9662-otpc-y		:= lan9662-otpc.o
>   obj-$(CONFIG_NVMEM_MXS_OCOTP)	+= nvmem-mxs-ocotp.o
>   nvmem-mxs-ocotp-y		:= mxs-ocotp.o
>   obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
> diff --git a/drivers/nvmem/lan9662-otpc.c b/drivers/nvmem/lan9662-otpc.c
> new file mode 100644
> index 000000000000..302a5bae04dc
> --- /dev/null
> +++ b/drivers/nvmem/lan9662-otpc.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define OTP_OTP_PWR_DN(t)			(t + 0x00)
> +#define OTP_OTP_PWR_DN_OTP_PWRDN_N		BIT(0)
> +#define OTP_OTP_ADDR_HI(t)			(t + 0x04)
> +#define OTP_OTP_ADDR_LO(t)			(t + 0x08)
> +#define OTP_OTP_PRGM_DATA(t)			(t + 0x10)
> +#define OTP_OTP_PRGM_MODE(t)			(t + 0x14)
> +#define OTP_OTP_PRGM_MODE_OTP_PGM_MODE_BYTE	BIT(0)
> +#define OTP_OTP_RD_DATA(t)			(t + 0x18)
> +#define OTP_OTP_FUNC_CMD(t)			(t + 0x20)
> +#define OTP_OTP_FUNC_CMD_OTP_PROGRAM		BIT(1)
> +#define OTP_OTP_FUNC_CMD_OTP_READ		BIT(0)
> +#define OTP_OTP_CMD_GO(t)			(t + 0x28)
> +#define OTP_OTP_CMD_GO_OTP_GO			BIT(0)
> +#define OTP_OTP_PASS_FAIL(t)			(t + 0x2c)
> +#define OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED	BIT(3)
> +#define OTP_OTP_PASS_FAIL_OTP_WRITE_PROHIBITED	BIT(2)
> +#define OTP_OTP_PASS_FAIL_OTP_FAIL		BIT(0)
> +#define OTP_OTP_STATUS(t)			(t + 0x30)
> +#define OTP_OTP_STATUS_OTP_CPUMPEN		BIT(1)
> +#define OTP_OTP_STATUS_OTP_BUSY			BIT(0)
> +
> +#define OTP_MEM_SIZE 8192
> +#define OTP_SLEEP_US 10
> +#define OTP_TIMEOUT_US 500000
> +
> +struct lan9662_otp {
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +static inline void lan9662_writel(void __iomem *addr, u32 val)
> +{
> +	writel(val, addr);
> +}
> +
> +static inline u32 lan9662_readl(void __iomem *addr)
> +{
> +	return readl(addr);
> +}
> +

Why these boiler plate functions?

> +static inline void lan9662_clrbits(void __iomem *addr, u32 clear)
> +{
> +	writel(readl(addr) & ~clear, addr);
> +}
> +
> +static inline void lan9662_setbits(void __iomem *addr, u32 set)
> +{
> +	writel(readl(addr) | set, addr);
> +}

These two functions are called just once and I see no point in having a 
wrapper function for this, instead you could use them directly or use 
./include/linux/bitfield.h helper macros.

> +
> +static bool lan9662_otp_wait_flag_clear(void __iomem *reg, u32 flag)
> +{
> +	u32 val;
> +
> +	return readl_poll_timeout(reg, val, !(val & flag),
> +				  OTP_SLEEP_US, OTP_TIMEOUT_US);
> +}
> +
> +static int lan9662_otp_power(struct lan9662_otp *otp, bool up)
> +{
> +	if (up) {
> +		lan9662_clrbits(OTP_OTP_PWR_DN(otp->base),
> +				OTP_OTP_PWR_DN_OTP_PWRDN_N);
> +		if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> +						OTP_OTP_STATUS_OTP_CPUMPEN))
> +			return -ETIMEDOUT;
> +	} else {
> +		lan9662_setbits(OTP_OTP_PWR_DN(otp->base),
> +				OTP_OTP_PWR_DN_OTP_PWRDN_N);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan9662_otp_execute(struct lan9662_otp *otp)
> +{
> +	if (lan9662_otp_wait_flag_clear(OTP_OTP_CMD_GO(otp->base),
> +					OTP_OTP_CMD_GO_OTP_GO))
> +		return -ETIMEDOUT;
> +
> +	if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> +					OTP_OTP_STATUS_OTP_BUSY))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static void lan9662_otp_set_address(struct lan9662_otp *otp, u32 offset)
> +{
> +	WARN_ON(offset >= OTP_MEM_SIZE);
> +
would we ever hit this condition? looks like unecessary check.



> +	lan9662_writel(OTP_OTP_ADDR_HI(otp->base), 0xff & (offset >> 8));
> +	lan9662_writel(OTP_OTP_ADDR_LO(otp->base), 0xff & offset);
> +}
> +
> +static int lan9662_otp_read_byte(struct lan9662_otp *otp, u32 offset, u8 *dst)
> +{
> +	u32 pass;
> +	int rc;
> +
> +	lan9662_otp_set_address(otp, offset);
> +	lan9662_writel(OTP_OTP_FUNC_CMD(otp->base),
> +		       OTP_OTP_FUNC_CMD_OTP_READ);
> +	lan9662_writel(OTP_OTP_CMD_GO(otp->base),
> +		       OTP_OTP_CMD_GO_OTP_GO);
Can be wrapped into single line.

> +	rc = lan9662_otp_execute(otp);
> +	if (!rc) {
> +		pass = lan9662_readl(OTP_OTP_PASS_FAIL(otp->base));
> +		if (pass & OTP_OTP_PASS_FAIL_OTP_READ_PROHIBITED)
> +			return -EACCES;
> +		*dst = (u8) lan9662_readl(OTP_OTP_RD_DATA(otp->base));
> +	}
> +	return rc;
> +}
> +

...

thanks,
srini
> +
> +static const struct of_device_id lan9662_otp_match[] = {
> +	{ .compatible = "microchip,lan9662-otp", },
> +	{ .compatible = "microchip,lan9668-otp", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lan9662_otp_match);
> +
> +static struct platform_driver lan9662_otp_driver = {
> +	.probe = lan9662_otp_probe,
> +	.driver = {
> +		.name = "lan9662-otp",
> +		.of_match_table = lan9662_otp_match,
> +	},
> +};
> +module_platform_driver(lan9662_otp_driver);
> +
> +MODULE_AUTHOR("Horatiu Vultur <horatiu.vultur@microchip.com>");
> +MODULE_DESCRIPTION("lan9662 OTP driver");
> +MODULE_LICENSE("GPL");
Horatiu Vultur Aug. 30, 2022, 8:07 p.m. UTC | #3 
The 08/30/2022 13:08, Srinivas Kandagatla wrote:
> 
> > +static inline void lan9662_writel(void __iomem *addr, u32 val)
> > +{
> > +     writel(val, addr);
> > +}
> > +
> > +static inline u32 lan9662_readl(void __iomem *addr)
> > +{
> > +     return readl(addr);
> > +}
> > +
> 
> Why these boiler plate functions?

It was more for the style purpose. I will remove these ones.

> 
> > +static inline void lan9662_clrbits(void __iomem *addr, u32 clear)
> > +{
> > +     writel(readl(addr) & ~clear, addr);
> > +}
> > +
> > +static inline void lan9662_setbits(void __iomem *addr, u32 set)
> > +{
> > +     writel(readl(addr) | set, addr);
> > +}
> 
> These two functions are called just once and I see no point in having a
> wrapper function for this, instead you could use them directly or use
> ./include/linux/bitfield.h helper macros.

I will remove also these ones and use them directly.

> 
> > +
> > +static bool lan9662_otp_wait_flag_clear(void __iomem *reg, u32 flag)
> > +{
> > +     u32 val;
> > +
> > +     return readl_poll_timeout(reg, val, !(val & flag),
> > +                               OTP_SLEEP_US, OTP_TIMEOUT_US);
> > +}
> > +
> > +static int lan9662_otp_power(struct lan9662_otp *otp, bool up)
> > +{
> > +     if (up) {
> > +             lan9662_clrbits(OTP_OTP_PWR_DN(otp->base),
> > +                             OTP_OTP_PWR_DN_OTP_PWRDN_N);
> > +             if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> > +                                             OTP_OTP_STATUS_OTP_CPUMPEN))
> > +                     return -ETIMEDOUT;
> > +     } else {
> > +             lan9662_setbits(OTP_OTP_PWR_DN(otp->base),
> > +                             OTP_OTP_PWR_DN_OTP_PWRDN_N);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int lan9662_otp_execute(struct lan9662_otp *otp)
> > +{
> > +     if (lan9662_otp_wait_flag_clear(OTP_OTP_CMD_GO(otp->base),
> > +                                     OTP_OTP_CMD_GO_OTP_GO))
> > +             return -ETIMEDOUT;
> > +
> > +     if (lan9662_otp_wait_flag_clear(OTP_OTP_STATUS(otp->base),
> > +                                     OTP_OTP_STATUS_OTP_BUSY))
> > +             return -ETIMEDOUT;
> > +
> > +     return 0;
> > +}
> > +
> > +static void lan9662_otp_set_address(struct lan9662_otp *otp, u32 offset)
> > +{
> > +     WARN_ON(offset >= OTP_MEM_SIZE);
> > +
> would we ever hit this condition? looks like unecessary check.

That is not the case. I will remove it.

> 
> 
>