Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs

Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs

[Michael Walle]

Am 2023-04-13 12:53, schrieb Vaibhaav Ram T.L:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> 
> Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for consumer,
> industrial, and automotive applications. This switch integrates OTP
> and EEPROM to enable customization of the part in the field.
> This patch adds support to read and write into PCI1XXXX OTP
> via NVMEM sysfs

Thanks for working on that. Now this driver is less of an oddball
one.

> Signed-off-by: Kumaravel Thiagarajan 
> <kumaravel.thiagarajan@microchip.com>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Co-developed-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
> ---
>  MAINTAINERS                                   |   2 +
>  drivers/misc/mchp_pci1xxxx/Kconfig            |   1 +
>  drivers/misc/mchp_pci1xxxx/Makefile           |   2 +-
>  .../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 301 ++++++++++++++++++
>  4 files changed, 305 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a33261b4592f..58e4b9d6ed61 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13721,12 +13721,14 @@
> F:	Documentation/devicetree/bindings/mtd/atmel-nand.txt
>  F:	drivers/mtd/nand/raw/atmel/*
> 
>  MICROCHIP PCI1XXXX GP DRIVER
> +M:	Vaibhaav Ram T.L <vaibhaavram.tl@microchip.com>
>  M:	Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
>  L:	linux-gpio@vger.kernel.org
>  S:	Supported
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
>  F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gpio.c
> +F:	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
>  MICROCHIP OTPC DRIVER
>  M:	Claudiu Beznea <claudiu.beznea@microchip.com>
> diff --git a/drivers/misc/mchp_pci1xxxx/Kconfig
> b/drivers/misc/mchp_pci1xxxx/Kconfig
> index 4abb47de7219..64e457581fb4 100644
> --- a/drivers/misc/mchp_pci1xxxx/Kconfig
> +++ b/drivers/misc/mchp_pci1xxxx/Kconfig
> @@ -2,6 +2,7 @@ config GP_PCI1XXXX
>         tristate "Microchip PCI1XXXX PCIe to GPIO Expander + OTP/EEPROM 
> manager"
>         depends on PCI
>         depends on GPIOLIB
> +       depends on NVMEM_SYSFS
>         select GPIOLIB_IRQCHIP
>         select AUXILIARY_BUS
>         help
> diff --git a/drivers/misc/mchp_pci1xxxx/Makefile
> b/drivers/misc/mchp_pci1xxxx/Makefile
> index fc4615cfe28b..ae31251dab37 100644
> --- a/drivers/misc/mchp_pci1xxxx/Makefile
> +++ b/drivers/misc/mchp_pci1xxxx/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o
> +obj-$(CONFIG_GP_PCI1XXXX) := mchp_pci1xxxx_gp.o mchp_pci1xxxx_gpio.o
> mchp_pci1xxxx_otpe2p.o
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> new file mode 100644
> index 000000000000..23f44ad6773e
> --- /dev/null
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022-2023 Microchip Technology Inc.
> +// PCI1xxxx OTP/EEPROM driver
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/bio.h>
> +#include <linux/device.h>
> +#include <linux/iopoll.h>
> +#include <linux/kthread.h>

Is this needed? I don't see any threads. Also bio.h. Please double check
your includes.

> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +
> +#include "mchp_pci1xxxx_gp.h"
> +
> +#define AUX_DRIVER_NAME			"PCI1xxxxOTPE2P"
> +#define OTP_NAME			"pci1xxxx_otp"
> +
> +#define PERI_PF3_SYSTEM_REG_ADDR_BASE	0x2000
> +#define PERI_PF3_SYSTEM_REG_LENGTH	0x4000
> +
> +#define OTP_SIZE_BYTES			8192
> +
> +#define CONFIG_REG_ADDR_BASE		0
> +#define OTP_REG_ADDR_BASE		0x1000
> +
> +#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + x)
> +#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + x)
> +
> +#define STATUS_READ_DELAY_US		1
> +#define STATUS_READ_TIMEOUT_US		20000
> +
> +#define OTP_ADDR_HIGH_OFFSET		0x04
> +#define OTP_ADDR_LOW_OFFSET		0x08
> +#define OTP_PRGM_DATA_OFFSET		0x10
> +#define OTP_PRGM_MODE_OFFSET		0x14
> +#define OTP_RD_DATA_OFFSET		0x18
> +#define OTP_FUNC_CMD_OFFSET		0x20
> +#define OTP_CMD_GO_OFFSET		0x28
> +#define OTP_PASS_FAIL_OFFSET		0x2C
> +#define OTP_STATUS_OFFSET		0x30
> +
> +#define OTP_FUNC_RD_BIT			BIT(0)
> +#define OTP_FUNC_PGM_BIT		BIT(1)
> +#define OTP_CMD_GO_BIT			BIT(0)
> +#define OTP_STATUS_BUSY_BIT		BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT		BIT(0)
> +#define OTP_FAIL_BIT			BIT(0)
> +
> +#define OTP_PWR_DN_BIT			BIT(0)
> +#define OTP_PWR_DN_OFFSET		0x00
> +
> +#define CFG_SYS_LOCK_OFFSET		0xA0
> +#define CFG_SYS_LOCK_PF3		BIT(5)
> +
> +#define BYTE_LOW			(GENMASK(7, 0))
> +#define BYTE_HIGH			(GENMASK(12, 8))
> +
> +struct pci1xxxx_otp_eeprom_device {
> +	struct auxiliary_device *pdev;
> +	void __iomem *reg_base;
> +	struct nvmem_config nvmem_config_otp;
> +	struct nvmem_device *nvmem_otp;
> +};
> +
> +static int set_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	u8 data;
> +
> +	writel(CFG_SYS_LOCK_PF3, sys_lock);
> +	data = readl(sys_lock);
> +	if (data != CFG_SYS_LOCK_PF3)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static void release_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> +	void __iomem *sys_lock = priv->reg_base +
> +				 MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	writel(0, sys_lock);
> +}
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_eeprom_device 
> *priv,
> +				   u16 address)
> +{
> +	u16 lo, hi;
> +
> +	lo = address & BYTE_LOW;
> +	hi = (address & BYTE_HIGH) >> 8;
> +	writew(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> +	writew(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static int pci1xxxx_otp_read(void *priv_t, unsigned int off,
> +			     void *buf_t, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
> +	char *buf = buf_t;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;

Unneeded check, priv cannot be NULL, right?

> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_RD_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;

No error handling?

> +
> +		buf[byte] = readl(rb + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static int pci1xxxx_otp_write(void *priv_t, unsigned int off,
> +			      void *value_t, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv = priv_t;
> +	char *value = value_t;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;

Same as above.

> +
> +	ret = set_sys_lock(priv);
> +	if (ret)
> +		return ret;
> +
> +	for (byte = 0; byte < count; byte++) {
> +		otp_device_set_address(priv, (u16)(off + byte));
> +
> +		/*
> +		 * Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register
> +		 * to enable Byte programming
> +		 */
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(data | OTP_PGM_MODE_BYTE_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +		writel(*(value + byte), rb + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		writel(data | OTP_FUNC_PGM_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +		writel(data | OTP_CMD_GO_BIT,
> +		       rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +		ret = read_poll_timeout(readl, regval,
> +					!(regval & OTP_STATUS_BUSY_BIT),
> +					STATUS_READ_DELAY_US,
> +					STATUS_READ_TIMEOUT_US, true,
> +					rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +
> +		data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +		if (ret < 0 || data & OTP_FAIL_BIT)
> +			break;

also here.

> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static int pci1xxxx_otp_eeprom_probe(struct auxiliary_device *aux_dev,
> +				     const struct auxiliary_device_id *id)
> +{
> +	struct auxiliary_device_wrapper *aux_dev_wrapper;
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct gp_aux_data_type *pdata;
> +	int ret;
> +	u8 data;
> +
> +	aux_dev_wrapper = container_of(aux_dev, struct 
> auxiliary_device_wrapper,
> +				       aux_dev);
> +	pdata = &aux_dev_wrapper->gp_aux_data;
> +	if (!pdata)
> +		return dev_err_probe(&aux_dev->dev, -EINVAL,
> +				     "Invalid data in aux_dev_wrapper->gp_aux_data\n");
> +
> +	priv = devm_kzalloc(&aux_dev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return dev_err_probe(&aux_dev->dev, -ENOMEM,
> +				     "Memory Allocation Failed\n");

Just return -ENOMEM. No need for that message.

-michael

Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs

[VaibhaavRam.TL]

> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/bio.h>
> > +#include <linux/device.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kthread.h>
> 
> Is this needed? I don't see any threads. Also bio.h. Please double
> check
> your includes.
Ok, Will remove in next version of patch
> > +     if (priv != NULL)
> > +             rb = priv->reg_base;
> > +     else
> > +             return -ENODEV;
> 
> Unneeded check, priv cannot be NULL, right?
Ok, I think this can be removed
> > +
> > +             data = readl(rb +
> > MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> > +             if (ret < 0 || data & OTP_FAIL_BIT)
> > +                     break;
> 
> No error handling?
We have implemented short read which returns count of successful bytes
read and therefore userspace will recognise the situation when the
requested count is not obtained.
> > +     if (!priv)
> > +             return dev_err_probe(&aux_dev->dev, -ENOMEM,
> > +                                  "Memory Allocation Failed\n");
> 
> Just return -ENOMEM. No need for that message.
Ok. Will remove

Thanks
Vaibhaav Ram

Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs

[Michael Walle]

Am 2023-04-17 14:47, schrieb VaibhaavRam.TL@microchip.com:
>> > +#include <linux/auxiliary_bus.h>
>> > +#include <linux/bio.h>
>> > +#include <linux/device.h>
>> > +#include <linux/iopoll.h>
>> > +#include <linux/kthread.h>
>> 
>> Is this needed? I don't see any threads. Also bio.h. Please double
>> check
>> your includes.
> Ok, Will remove in next version of patch
>> > +     if (priv != NULL)
>> > +             rb = priv->reg_base;
>> > +     else
>> > +             return -ENODEV;
>> 
>> Unneeded check, priv cannot be NULL, right?
> Ok, I think this can be removed
>> > +
>> > +             data = readl(rb +
>> > MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
>> > +             if (ret < 0 || data & OTP_FAIL_BIT)
>> > +                     break;
>> 
>> No error handling?
> We have implemented short read which returns count of successful bytes
> read and therefore userspace will recognise the situation when the
> requested count is not obtained.

I'll leave that up to Greg, but I'd prefer a proper error to userspace.
I'm not sure if you'd need to differentiate between a short read/write
and an error.

Maybe a short read or write is a valid situation where an access which
leads to the OTP_FAIL_BIT is for sure an error situation.

-michael

Re: [PATCH v9 char-misc-next 1/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX OTP via NVMEM sysfs

[Greg KH]

On Tue, Apr 18, 2023 at 11:00:18AM +0200, Michael Walle wrote:
> Am 2023-04-17 14:47, schrieb VaibhaavRam.TL@microchip.com:
> > > > +#include <linux/auxiliary_bus.h>
> > > > +#include <linux/bio.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/iopoll.h>
> > > > +#include <linux/kthread.h>
> > > 
> > > Is this needed? I don't see any threads. Also bio.h. Please double
> > > check
> > > your includes.
> > Ok, Will remove in next version of patch
> > > > +     if (priv != NULL)
> > > > +             rb = priv->reg_base;
> > > > +     else
> > > > +             return -ENODEV;
> > > 
> > > Unneeded check, priv cannot be NULL, right?
> > Ok, I think this can be removed
> > > > +
> > > > +             data = readl(rb +
> > > > MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> > > > +             if (ret < 0 || data & OTP_FAIL_BIT)
> > > > +                     break;
> > > 
> > > No error handling?
> > We have implemented short read which returns count of successful bytes
> > read and therefore userspace will recognise the situation when the
> > requested count is not obtained.
> 
> I'll leave that up to Greg, but I'd prefer a proper error to userspace.
> I'm not sure if you'd need to differentiate between a short read/write
> and an error.

A short read isn't an error, it's just returning the amount of data
present which might be less than the buffer passed in, which is totally
normal.  If there is an error, return an error.

thanks,

greg k-h