Re: [PATCH v8 char-misc-next 2/5] misc: microchip: pci1xxxx: Add OTP Functionality to read and write into OTP bin sysfs

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Tue, Mar 28, 2023 at 08:10:05PM +0530, Vaibhaav Ram T.L wrote:
> From: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
> 
> 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 the OTP functionality to support the same.

Why not just use the in-kernel eeprom api instead of creating your own
custom user/kernel api?  Why is this so special to deserve that?

> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@xxxxxxxxxxxxx>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@xxxxxxxxxxxxx>
> Signed-off-by: Vaibhaav Ram T.L <vaibhaavram.tl@xxxxxxxxxxxxx>
> ---
>  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 198 ++++++++++++++++++
>  drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h |   7 +-
>  2 files changed, 204 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> index 64302fdfbefc..bf175e22090e 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
> @@ -3,13 +3,210 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/iopoll.h>
>  #include <linux/pci.h>
>  #include <linux/idr.h>
>  #include <linux/io.h>
>  #include "mchp_pci1xxxx_gp.h"
>  
> +#define OTP_NAME			"pci1xxxx_otp"
>  #define PCI_DRIVER_NAME			"PCI1xxxxGP"
>  
> +#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + x)
> +#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + x)
> +
> +#define OTP_SIZE_BYTES			8192
> +
> +#define CONFIG_REG_ADDR_BASE		0
> +#define OTP_REG_ADDR_BASE		0x1000
> +
> +#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 STATUS_READ_DELAY_US		1
> +#define STATUS_READ_TIMEOUT_US		20000
> +
> +#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))
> +
> +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 ssize_t pci1xxxx_otp_read(struct file *filp, struct kobject *kobj,
> +				 struct bin_attribute *bin_attr,
> +				 char *buf, loff_t off, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct device *dev;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	dev = container_of(kobj, struct device, kobj);
> +	priv = dev_get_drvdata(dev);
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;
> +
> +	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;
> +
> +		buf[byte] = readl(rb + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static ssize_t pci1xxxx_otp_write(struct file *filp, struct kobject *kobj,
> +				  struct bin_attribute *bin_attr,
> +				  char *value, loff_t off, size_t count)
> +{
> +	struct pci1xxxx_otp_eeprom_device *priv;
> +	struct device *dev;
> +	void __iomem *rb;
> +	u32 regval;
> +	u32 byte;
> +	int ret;
> +	u8 data;
> +
> +	dev = container_of(kobj, struct device, kobj);
> +	priv = dev_get_drvdata(dev);
> +	if (priv != NULL)
> +		rb = priv->reg_base;
> +	else
> +		return -ENODEV;
> +
> +	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;
> +	}
> +	release_sys_lock(priv);
> +
> +	return byte;
> +}
> +
> +static struct bin_attribute pci1xxxx_otp_attr = {
> +	.attr = {
> +		.name = OTP_NAME,
> +		.mode = 0600,
> +	},
> +	.size = OTP_SIZE_BYTES,
> +	.read = pci1xxxx_otp_read,
> +	.write = pci1xxxx_otp_write,
> +};
> +
> +static struct bin_attribute *pci1xxxx_bin_attributes[] = {
> +	&pci1xxxx_otp_attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pci1xxxx_bin_attributes_group = {
> +	.bin_attrs = pci1xxxx_bin_attributes,
> +};
> +
> +static const struct attribute_group *pci1xxxx_bin_attributes_groups[] = {
> +	&pci1xxxx_bin_attributes_group,
> +	NULL
> +};
> +
>  struct aux_bus_device {
>  	struct auxiliary_device_wrapper *aux_device_wrapper[2];
>  };
> @@ -55,6 +252,7 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
>  	otp_eeprom_wrapper->aux_dev.name = aux_dev_otp_e2p_name;
>  	otp_eeprom_wrapper->aux_dev.dev.parent = &pdev->dev;
>  	otp_eeprom_wrapper->aux_dev.dev.release = gp_auxiliary_device_release;
> +	otp_eeprom_wrapper->aux_dev.dev.groups = pci1xxxx_bin_attributes_groups;
>  	otp_eeprom_wrapper->aux_dev.id = retval;
>  
>  	otp_eeprom_wrapper->gp_aux_data.region_start = pci_resource_start(pdev, 0);
> diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> index 37eec73b20d7..c6238a817dc6 100644
> --- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -/* Copyright (C) 2022 Microchip Technology Inc. */
> +/* Copyright (C) 2022-2023 Microchip Technology Inc. */
>  
>  #ifndef _GPIO_PCI1XXXX_H
>  #define _GPIO_PCI1XXXX_H
> @@ -25,4 +25,9 @@ struct auxiliary_device_wrapper {
>  	struct gp_aux_data_type gp_aux_data;
>  };
>  
> +struct pci1xxxx_otp_eeprom_device {
> +	struct auxiliary_device *pdev;
> +	void __iomem *reg_base;
> +	bool is_eeprom_present;

This field is never used, why have it?

> +};

Why does this need to be in the .h file and not in the .c file?

thanks,

greg k-h