[PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Tharun Kumar P]

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 provides the OTP/EEPROM
driver to support the same.

Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
---
v4 -> v5:
- Used proper errno
- Removed un-necessary prints

v3 -> v4:
- Remove extra space, tab, un-necessary casting, paranthesis, do
  while(false) loops
- Used read_poll_timeout for polling BUSY_BIT

v2 -> v3:
- Modified commit description to include build issues reported by Kernel
test robot <lkp@intel.com> which are fixed in this patch

v1 -> v2:
- Resolve build issue reported by kernel test robot <lkp@intel.com>
---
 MAINTAINERS                                   |   1 +
 drivers/misc/mchp_pci1xxxx/Kconfig            |   1 +
 drivers/misc/mchp_pci1xxxx/Makefile           |   2 +-
 .../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 649 ++++++++++++++++++
 4 files changed, 652 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 69d1e8ad52c5..8a57683927fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13803,6 +13803,7 @@ 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..67fa3299cfb6 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 BLOCK
        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..0d097afc84aa
--- /dev/null
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
@@ -0,0 +1,649 @@
+// 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/blkdev.h>
+#include <linux/blk-mq.h>
+#include <linux/delay.h>
+#include <linux/gpio/driver.h>
+#include <linux/iopoll.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
+#include "mchp_pci1xxxx_gp.h"
+
+#define PERI_PF3_SYSTEM_REG_ADDR_BASE	(0x2000)
+#define PERI_PF3_SYSTEM_REG_LENGTH	(0x4000)
+
+#define CONFIG_REG_ADDR_BASE		(0)
+#define EEPROM_REG_ADDR_BASE		(0x0E00)
+#define OTP_REG_ADDR_BASE		(0x1000)
+
+#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + (x))
+
+#define CFG_SYS_LOCK_OFFSET		(0xA0)
+#define CFG_SYS_LOCK_PF3		BIT(5)
+
+#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + (x))
+
+#define OTP_PWR_DN_OFFSET		(0x00)
+#define OTP_ADDR_HIGH_OFFSET		(0x04)
+#define	OTP_ADDR_LOW_OFFSET		(0x08)
+#define	OTP_ADDR_BITS_OFFSET		(0x0C)
+#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_TEST_CMD_OFFSET		(0x24)
+#define OTP_CMD_GO_OFFSET		(0x28)
+#define OTP_PASS_FAIL_OFFSET		(0x2C)
+#define OTP_STATUS_OFFSET		(0x30)
+#define OTP_MAX_PRG_OFFSET		(0x34)
+#define OTP_RSTB_PW_OFFSET		(0x50)
+#define OTP_PGM_PW_OFFSET		(0x60)
+#define OTP_READ_PW_OFFSET		(0x70)
+
+#define OTP_PWR_DN_BIT			BIT(0)
+#define OTP_CMD_GO_BIT			BIT(0)
+#define OTP_PGM_MODE_BYTE_BIT		BIT(0)
+#define OTP_STATUS_BUSY_BIT		BIT(0)
+#define OTP_FUNC_PGM_BIT		BIT(1)
+#define OTP_FUNC_RD_BIT			BIT(0)
+
+#define OTP_RW_TIMEOUT_MILLISECONDS	(5)
+#define OTP_STATUS_READ_DELAY_US	(4000)
+#define OTP_STATUS_READ_TIMEOUT_US	(20000)
+
+#define MMAP_EEPROM_OFFSET(x)		(EEPROM_REG_ADDR_BASE + (x))
+
+#define EEPROM_CMD_REG			(0x00)
+#define EEPROM_DATA_REG			(0x04)
+#define EEPROM_CFG_REG			(0x08)
+
+#define EEPROM_CMD_EPC_BUSY_BIT		BIT(31)
+#define EEPROM_CMD_EPC_TIMEOUT_BIT	BIT(17)
+#define EEPROM_CMD_EPC_WRITE		(BIT(29) | BIT(28))
+
+#define EEPROM_CFG_BAUD_RATE_100KHZ	BIT(9)
+#define EEPROM_CFG_SIZE_SEL		BIT(12)
+#define EEPROM_CFG_PULSE_WIDTH_100KHZ	(BIT(17) | BIT(16))
+#define OTP_EEPROM_SECTOR_SIZE		(512)
+#define OTP_SIZE_IN_BYTES		(8 * 1024)
+#define EEPROM_SIZE_IN_BYTES		(8 * 1024)
+
+struct pci1xxxx_otp_e2p_device {
+	struct pci1xxxx_otp_e2p_disk *otp_e2p_device;
+	struct auxiliary_device *pdev;
+	void __iomem *reg_base;
+	int block_device_count;
+};
+
+struct pci1xxxx_otp_e2p_disk {
+	struct blk_mq_tag_set tag_set;
+	struct auxiliary_device *pdev;
+	struct request_queue *queue;
+	struct mutex lock;
+	struct gendisk *gd;
+	bool has_eeprom;
+	int (*disk_write_byte)(struct pci1xxxx_otp_e2p_device *priv,
+		unsigned long byte_offset, u8 value);
+	int (*disk_read_byte)(struct pci1xxxx_otp_e2p_device *priv,
+		unsigned long byte_offset, u8 *data);
+};
+
+static int otp_sector_count = OTP_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
+static int e2p_sector_count = EEPROM_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
+static int otp_device_count, e2p_device_count;
+static int otp_block_driver_major;
+
+static void otp_device_set_address(struct pci1xxxx_otp_e2p_device *priv, u16 address)
+{
+	u32 lo, hi;
+
+	lo = address & 0xFF;
+	hi = (address & 0x1f00) >> 8;
+	writel(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
+	writel(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
+}
+
+static int set_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
+{
+	void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
+	u8 data;
+
+	writel(CFG_SYS_LOCK_PF3, p);
+	data = readl(p);
+	if (data != CFG_SYS_LOCK_PF3)
+		return -EPERM;
+
+	return 0;
+}
+
+static int release_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
+{
+	void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
+	u8 data;
+
+	data = readl(p);
+	if (data != CFG_SYS_LOCK_PF3)
+		return 0;
+
+	writel(0, p);
+
+	data = readl(p);
+	if (data & CFG_SYS_LOCK_PF3)
+		return -EPERM;
+
+	return 0;
+}
+
+static int otp_e2p_device_open(struct block_device *bdev, fmode_t mode)
+{
+	struct pci1xxxx_otp_e2p_disk *disk_priv;
+	struct pci1xxxx_otp_e2p_device *priv;
+	struct auxiliary_device *pdev;
+	int retval = 0;
+	u8 data;
+
+	disk_priv = bdev->bd_disk->private_data;
+	pdev = (struct auxiliary_device *)disk_priv->pdev;
+	priv = dev_get_drvdata(&pdev->dev);
+
+	mutex_lock(&disk_priv->lock);
+
+	retval = set_sys_lock(priv);
+	if (retval)
+		goto exit;
+
+	if (!disk_priv->has_eeprom) {
+		data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+		writel((data & ~OTP_PWR_DN_BIT), priv->reg_base +
+			MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+	}
+
+exit:
+	mutex_unlock(&disk_priv->lock);
+	return retval;
+}
+
+static void otp_e2p_device_release(struct gendisk *disk, fmode_t mode)
+{
+	struct pci1xxxx_otp_e2p_disk *disk_priv;
+	struct pci1xxxx_otp_e2p_device *priv;
+	u8 data;
+
+	disk_priv = disk->private_data;
+	priv = dev_get_drvdata(&disk_priv->pdev->dev);
+
+	mutex_lock(&disk_priv->lock);
+
+	if (!disk_priv->has_eeprom) {
+		data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+		writel((data | OTP_PWR_DN_BIT), priv->reg_base +
+			MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
+	}
+	release_sys_lock(priv);
+
+	mutex_unlock(&disk_priv->lock);
+}
+
+static int e2p_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
+				 unsigned long byte_offset, u8 value)
+{
+	u32 data;
+
+	/* Write the value into EEPROM_DATA_REG register */
+	writel(value, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
+	data = EEPROM_CMD_EPC_TIMEOUT_BIT | EEPROM_CMD_EPC_WRITE | byte_offset;
+
+	/* Write the data into EEPROM_CMD_REG register */
+	writel(data, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+	writel(EEPROM_CMD_EPC_BUSY_BIT | data, priv->reg_base +
+	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Wait for the EPC_BUSY bit to get cleared */
+	do {
+		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+	} while (data & EEPROM_CMD_EPC_BUSY_BIT);
+
+	if (data & EEPROM_CMD_EPC_TIMEOUT_BIT)
+		return -EBUSY;
+
+	return 0;
+}
+
+static int e2p_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
+				unsigned long byte_offset, u8 *data)
+{
+	u32 regval;
+
+	/*
+	 * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
+	 * register
+	 */
+	writel(byte_offset, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+	writel(EEPROM_CMD_EPC_BUSY_BIT | byte_offset, priv->reg_base +
+	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Wait for the EPC_BUSY bit to get cleared */
+	do {
+		regval = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+	} while (regval & EEPROM_CMD_EPC_BUSY_BIT);
+
+	if (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)
+		return -EBUSY;
+
+	/* Read the contents from the EEPROM_DATA_REG */
+	*data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
+	return 0;
+}
+
+static bool is_e2p_responsive(struct pci1xxxx_otp_e2p_device *priv)
+{
+	u32 data;
+
+	if (set_sys_lock(priv))
+		return false;
+
+	writel((EEPROM_CFG_PULSE_WIDTH_100KHZ | EEPROM_CFG_SIZE_SEL |
+		EEPROM_CFG_BAUD_RATE_100KHZ), priv->reg_base +
+		MMAP_EEPROM_OFFSET(EEPROM_CFG_REG));
+
+	/*
+	 * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
+	 * register
+	 */
+	writel(EEPROM_CMD_EPC_TIMEOUT_BIT, priv->reg_base +
+	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
+	writel(EEPROM_CMD_EPC_BUSY_BIT, priv->reg_base +
+	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+
+	/* Wait for the EPC_BUSY bit to get cleared or timeout bit to get set*/
+	do {
+		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
+	} while (data & EEPROM_CMD_EPC_BUSY_BIT);
+
+	/* If EPC_TIMEOUT is set, then the EEPROM is not responsive */
+	release_sys_lock(priv);
+
+	if (data & EEPROM_CMD_EPC_TIMEOUT_BIT) {
+		dev_err(&priv->pdev->dev,
+			"EPC_Timeout, EEPROM is unresponsive: %x\n", data);
+		return false;
+	}
+
+	return true;
+}
+
+static int otp_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
+				 unsigned long byte_offset, u8 value)
+{
+	u8 data;
+	int ret;
+
+	if (!value)
+		return 0;
+
+	otp_device_set_address(priv, (u16)byte_offset);
+
+	/* Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register */
+	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
+	writel((data | OTP_PGM_MODE_BYTE_BIT), priv->reg_base +
+		MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
+
+	/* Write the value to program into OTP_PRGM_DATA register */
+	writel(value, priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
+
+	/* Set OTP_PROGRAM command bit in OTP_FUNC_CMD register */
+	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+	writel((data | OTP_FUNC_PGM_BIT), priv->reg_base +
+		MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+
+	/* Set OTP_GO command bit in OTP_CMD_GO register */
+	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+	writel((data | OTP_CMD_GO_BIT), priv->reg_base +
+		MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+
+	ret = read_poll_timeout(readl, data, !(data & OTP_STATUS_BUSY_BIT),
+				OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
+				true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
+	if (ret < 0)
+		return -EBUSY;
+
+	/* Read the result from OTP_RD_DATA register */
+	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
+	if (data & 0x02)
+		return 0;
+
+	dev_err(&priv->pdev->dev, "OTP write read mismatch 0x%x\n", data);
+	return -EIO;
+}
+
+static int otp_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
+				unsigned long byte_offset, u8 *data)
+{
+	int ret;
+
+	otp_device_set_address(priv, (u16)byte_offset);
+
+	/* Set OTP_READ command bit in OTP_FUNC_CMD register */
+	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+	writel((*data | OTP_FUNC_RD_BIT), priv->reg_base +
+		MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
+
+	/* Set OTP_GO command bit in OTP_CMD_GO register */
+	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+	writel((*data | OTP_CMD_GO_BIT), priv->reg_base +
+		MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
+
+	ret = read_poll_timeout(readl, *data, !(*data & OTP_STATUS_BUSY_BIT),
+				OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
+				true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
+	if (ret < 0)
+		return -EBUSY;
+
+	/* Read the result from OTP_RD_DATA register */
+	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
+	return 0;
+}
+
+static int otp_e2P_device_transfer(struct request *req)
+{
+	struct pci1xxxx_otp_e2p_disk *disk_priv;
+	struct pci1xxxx_otp_e2p_device *priv;
+	unsigned long sector;
+	unsigned long nsect;
+	long byte_offset;
+	int retval;
+	u8 *buffer;
+	int write;
+	int i, j;
+
+	sector = blk_rq_pos(req);
+	nsect = blk_rq_cur_sectors(req);
+	buffer = bio_data(req->bio);
+	write = rq_data_dir(req);
+	disk_priv = req->q->disk->private_data;
+	priv = dev_get_drvdata(&disk_priv->pdev->dev);
+
+	if (write) {
+		for (i = 0; i < nsect; i++) {
+			byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
+			for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
+				retval = disk_priv->disk_write_byte(priv,
+								    byte_offset + j,
+								    *buffer);
+				if (retval)
+					return retval;
+
+				buffer++;
+			}
+		}
+	} else {
+		for (i = 0; i < nsect; i++) {
+			byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
+			for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
+				retval = disk_priv->disk_read_byte(priv,
+								   byte_offset + j,
+								   buffer);
+				if (retval)
+					return retval;
+
+				buffer++;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static blk_status_t OTPE2P_queue_rq(struct blk_mq_hw_ctx *hctx,
+				    const struct blk_mq_queue_data *bd)
+{
+	struct request *req = bd->rq;
+
+	blk_mq_start_request(req);
+	if (!otp_e2P_device_transfer(req)) {
+		blk_mq_end_request(req, BLK_STS_OK);
+		return BLK_STS_OK;
+	}
+
+	return BLK_STS_IOERR;
+}
+
+static const struct blk_mq_ops OTPE2P_mq_ops = {
+	.queue_rq	= OTPE2P_queue_rq,
+};
+
+static const struct block_device_operations otp_e2p_device_ops = {
+	.owner = THIS_MODULE,
+	.open = otp_e2p_device_open,
+	.release = otp_e2p_device_release,
+};
+
+static int otp_e2p_device_create_block_device(struct auxiliary_device *aux_dev)
+{
+	struct auxiliary_device_wrapper *aux_dev_wrapper;
+	struct pci1xxxx_otp_e2p_device *priv;
+	struct gp_aux_data_type *pdata;
+	int retval = 0, i;
+
+	aux_dev_wrapper = container_of(aux_dev, struct auxiliary_device_wrapper, aux_dev);
+	pdata = &aux_dev_wrapper->gp_aux_data;
+	if (!pdata) {
+		dev_err(&aux_dev->dev, "Invalid data in aux_dev_wrapper->gp_aux_data\n");
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(&aux_dev->dev, sizeof(struct pci1xxxx_otp_e2p_device),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pdev = aux_dev;
+
+	dev_set_drvdata(&aux_dev->dev, priv);
+
+	if (!devm_request_mem_region(&aux_dev->dev, pdata->region_start +
+				     PERI_PF3_SYSTEM_REG_ADDR_BASE,
+		PERI_PF3_SYSTEM_REG_LENGTH, aux_dev->name)) {
+		dev_err(&aux_dev->dev, "can't request otpe2p region\n");
+		return -ENOMEM;
+	}
+
+	priv->reg_base = devm_ioremap(&aux_dev->dev, pdata->region_start +
+				      PERI_PF3_SYSTEM_REG_ADDR_BASE,
+				      PERI_PF3_SYSTEM_REG_LENGTH);
+	if (!priv->reg_base) {
+		dev_err(&aux_dev->dev, "ioremap failed\n");
+		return -ENOMEM;
+	}
+
+	priv->block_device_count = 0;
+	if (is_e2p_responsive(priv))
+		priv->block_device_count = 2;
+	else
+		priv->block_device_count = 1;
+
+	priv->otp_e2p_device = devm_kzalloc(&priv->pdev->dev,
+					    priv->block_device_count *
+					    sizeof(struct pci1xxxx_otp_e2p_disk),
+					    GFP_KERNEL);
+	if (!priv->otp_e2p_device)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->block_device_count; i++) {
+		mutex_init(&priv->otp_e2p_device[i].lock);
+		priv->otp_e2p_device[i].tag_set.ops = &OTPE2P_mq_ops;
+		priv->otp_e2p_device[i].tag_set.nr_hw_queues = 1;
+		priv->otp_e2p_device[i].tag_set.queue_depth = 16;
+		priv->otp_e2p_device[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+
+		retval = blk_mq_alloc_tag_set(&priv->otp_e2p_device[i].tag_set);
+		if (retval) {
+			dev_err(&aux_dev->dev, "blk_mq_alloc_tag_set failed\n");
+			return retval;
+		}
+
+		priv->otp_e2p_device[i].queue =
+			blk_mq_init_queue(&priv->otp_e2p_device[i].tag_set);
+		if (IS_ERR(priv->otp_e2p_device[i].queue)) {
+			retval = PTR_ERR(priv->otp_e2p_device[i].queue);
+			priv->otp_e2p_device[i].queue = NULL;
+			if (i)
+				goto e2p_free_tag_set;
+			else
+				goto otp_free_tag_set;
+		}
+
+		blk_queue_logical_block_size(priv->otp_e2p_device[i].queue,
+				OTP_EEPROM_SECTOR_SIZE);
+		blk_queue_physical_block_size(priv->otp_e2p_device[i].queue,
+				OTP_EEPROM_SECTOR_SIZE);
+		priv->otp_e2p_device[i].queue->queuedata = priv;
+		priv->otp_e2p_device[i].gd =
+			blk_mq_alloc_disk(&priv->otp_e2p_device[i].tag_set,
+					  NULL);
+		if (IS_ERR(priv->otp_e2p_device[i].gd)) {
+			retval = PTR_ERR(priv->otp_e2p_device[i].gd);
+			if (i)
+				goto e2p_destroy_queue;
+			else
+				goto otp_destroy_queue;
+		}
+
+		priv->otp_e2p_device[i].pdev = aux_dev;
+		priv->otp_e2p_device[i].gd->major = otp_block_driver_major;
+		priv->otp_e2p_device[i].gd->minors = 1;
+		priv->otp_e2p_device[i].gd->first_minor =
+					otp_device_count + e2p_device_count;
+		priv->otp_e2p_device[i].gd->fops = &otp_e2p_device_ops;
+		priv->otp_e2p_device[i].gd->private_data =
+			&priv->otp_e2p_device[i];
+
+		if (i == 0) {
+			snprintf(priv->otp_e2p_device[i].gd->disk_name,
+					32, "PCI1xxxxOTP%x", otp_device_count);
+			set_capacity(priv->otp_e2p_device[i].gd, otp_sector_count);
+			priv->otp_e2p_device[i].disk_read_byte = otp_device_read_byte;
+			priv->otp_e2p_device[i].disk_write_byte = otp_device_write_byte;
+			otp_device_count++;
+		} else {
+			snprintf(priv->otp_e2p_device[i].gd->disk_name,
+					32, "PCI1xxxxE2P%x", otp_device_count - 1);
+			set_capacity(priv->otp_e2p_device[i].gd, e2p_sector_count);
+			priv->otp_e2p_device[i].has_eeprom = true;
+			priv->otp_e2p_device[i].disk_read_byte = e2p_device_read_byte;
+			priv->otp_e2p_device[i].disk_write_byte = e2p_device_write_byte;
+			e2p_device_count++;
+		}
+
+		retval = add_disk(priv->otp_e2p_device[i].gd);
+		if (retval) {
+			if (i)
+				goto e2p_free_disk;
+			else
+				goto otp_free_disk;
+		}
+	}
+
+	return retval;
+
+e2p_free_disk:
+	del_gendisk(priv->otp_e2p_device[1].gd);
+	put_disk(priv->otp_e2p_device[1].gd);
+e2p_destroy_queue:
+	blk_mq_destroy_queue(priv->otp_e2p_device[1].queue);
+e2p_free_tag_set:
+	blk_mq_free_tag_set(&priv->otp_e2p_device[1].tag_set);
+otp_free_disk:
+	del_gendisk(priv->otp_e2p_device[0].gd);
+	put_disk(priv->otp_e2p_device[0].gd);
+otp_destroy_queue:
+	blk_mq_destroy_queue(priv->otp_e2p_device[0].queue);
+otp_free_tag_set:
+	blk_mq_free_tag_set(&priv->otp_e2p_device[0].tag_set);
+
+	dev_err(&aux_dev->dev,
+		"otp/eeprom device enumeration failed with errno = %d\n",
+		retval);
+	return retval;
+}
+
+static void pci1xxxx_otp_e2p_remove(struct auxiliary_device *aux_dev)
+{
+	struct pci1xxxx_otp_e2p_device *priv = dev_get_drvdata(&aux_dev->dev);
+	int i;
+
+	for (i = 0; i < priv->block_device_count; i++) {
+		if (priv->otp_e2p_device[i].queue)
+			blk_mq_destroy_queue(priv->otp_e2p_device[i].queue);
+
+		if (priv->otp_e2p_device[i].gd) {
+			del_gendisk(priv->otp_e2p_device[i].gd);
+			put_disk(priv->otp_e2p_device[i].gd);
+			blk_mq_free_tag_set(&priv->otp_e2p_device[i].tag_set);
+		}
+	}
+}
+
+static int pci1xxxx_otp_e2p_probe(struct auxiliary_device *aux_dev,
+				  const struct auxiliary_device_id *id)
+{
+	return otp_e2p_device_create_block_device(aux_dev);
+}
+
+static const struct auxiliary_device_id pci1xxxx_otp_e2p_auxiliary_id_table[] = {
+	{.name = "mchp_pci1xxxx_gp.gp_otp_e2p"},
+	{},
+};
+MODULE_DEVICE_TABLE(auxiliary, pci1xxxx_otp_e2p_auxiliary_id_table);
+
+static struct auxiliary_driver pci1xxxx_otp_e2p_driver = {
+	.driver = {
+		.name = "PCI1xxxxOTPE2P",
+	},
+	.probe = pci1xxxx_otp_e2p_probe,
+	.remove = pci1xxxx_otp_e2p_remove,
+	.id_table = pci1xxxx_otp_e2p_auxiliary_id_table
+};
+
+static int __init pci1xxxx_otp_e2p_driver_init(void)
+{
+	int retval;
+
+	otp_block_driver_major = register_blkdev(otp_block_driver_major,
+						 "OTPBlockDevice");
+	if (otp_block_driver_major < 0)
+		return otp_block_driver_major;
+
+	retval = auxiliary_driver_register(&pci1xxxx_otp_e2p_driver);
+	if (retval)
+		unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
+
+	return retval;
+}
+
+static void __exit pci1xxxx_otp_e2p_driver_exit(void)
+{
+	unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
+	auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);
+}
+
+module_init(pci1xxxx_otp_e2p_driver_init);
+module_exit(pci1xxxx_otp_e2p_driver_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>");
+MODULE_AUTHOR("Tharun Kumar P<tharunkumar.pasumarthi@microchip.com>");
+MODULE_DESCRIPTION("Microchip Technology Inc. PCI1xxxx OTP EEPROM programmer");
-- 
2.25.1

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Sun, Feb 12, 2023 at 09:27:43AM +0530, Tharun Kumar P wrote:
> +	/* Wait for the EPC_BUSY bit to get cleared */
> +	do {
> +		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +	} while (data & EEPROM_CMD_EPC_BUSY_BIT);

Again, you can not sit and spin in a busy-wait like this with no chance
to recover if something goes wrong with the hardware (hint, what if it
got removed?)

please fix.

thanks,

greg k-h

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Tharunkumar.Pasumarthi]

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Sunday, February 12, 2023 12:39 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Again, you can not sit and spin in a busy-wait like this with no chance to
> recover if something goes wrong with the hardware (hint, what if it got
> removed?)
> 
> please fix.

Okay Greg. I will add timeout logic in driver to break out of busy-wait loop to handle cases like these. 

Thanks, 
Tharun Kumar P 

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Christophe JAILLET]

Le 12/02/2023 à 04:57, Tharun Kumar P a écrit :
> 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 provides the OTP/EEPROM
> driver to support the same.
> 
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>

Hi,

a few nits below, should some be useful.

CJ

> ---
> v4 -> v5:
> - Used proper errno
> - Removed un-necessary prints
> 
> v3 -> v4:
> - Remove extra space, tab, un-necessary casting, paranthesis, do
>    while(false) loops
> - Used read_poll_timeout for polling BUSY_BIT
> 
> v2 -> v3:
> - Modified commit description to include build issues reported by Kernel
> test robot <lkp@intel.com> which are fixed in this patch
> 
> v1 -> v2:
> - Resolve build issue reported by kernel test robot <lkp@intel.com>
> ---
>   MAINTAINERS                                   |   1 +
>   drivers/misc/mchp_pci1xxxx/Kconfig            |   1 +
>   drivers/misc/mchp_pci1xxxx/Makefile           |   2 +-
>   .../misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c | 649 ++++++++++++++++++
>   4 files changed, 652 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 69d1e8ad52c5..8a57683927fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13803,6 +13803,7 @@ 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..67fa3299cfb6 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 BLOCK
>          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..0d097afc84aa
> --- /dev/null
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -0,0 +1,649 @@
> +// 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/blkdev.h>
> +#include <linux/blk-mq.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/iopoll.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +
> +#include "mchp_pci1xxxx_gp.h"
> +
> +#define PERI_PF3_SYSTEM_REG_ADDR_BASE	(0x2000)
> +#define PERI_PF3_SYSTEM_REG_LENGTH	(0x4000)
> +
> +#define CONFIG_REG_ADDR_BASE		(0)
> +#define EEPROM_REG_ADDR_BASE		(0x0E00)
> +#define OTP_REG_ADDR_BASE		(0x1000)
> +
> +#define MMAP_CFG_OFFSET(x)		(CONFIG_REG_ADDR_BASE + (x))
> +
> +#define CFG_SYS_LOCK_OFFSET		(0xA0)
> +#define CFG_SYS_LOCK_PF3		BIT(5)
> +
> +#define MMAP_OTP_OFFSET(x)		(OTP_REG_ADDR_BASE + (x))
> +
> +#define OTP_PWR_DN_OFFSET		(0x00)
> +#define OTP_ADDR_HIGH_OFFSET		(0x04)
> +#define	OTP_ADDR_LOW_OFFSET		(0x08)
> +#define	OTP_ADDR_BITS_OFFSET		(0x0C)
> +#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_TEST_CMD_OFFSET		(0x24)
> +#define OTP_CMD_GO_OFFSET		(0x28)
> +#define OTP_PASS_FAIL_OFFSET		(0x2C)
> +#define OTP_STATUS_OFFSET		(0x30)
> +#define OTP_MAX_PRG_OFFSET		(0x34)
> +#define OTP_RSTB_PW_OFFSET		(0x50)
> +#define OTP_PGM_PW_OFFSET		(0x60)
> +#define OTP_READ_PW_OFFSET		(0x70)
> +
> +#define OTP_PWR_DN_BIT			BIT(0)
> +#define OTP_CMD_GO_BIT			BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT		BIT(0)
> +#define OTP_STATUS_BUSY_BIT		BIT(0)
> +#define OTP_FUNC_PGM_BIT		BIT(1)
> +#define OTP_FUNC_RD_BIT			BIT(0)
> +
> +#define OTP_RW_TIMEOUT_MILLISECONDS	(5)
> +#define OTP_STATUS_READ_DELAY_US	(4000)
> +#define OTP_STATUS_READ_TIMEOUT_US	(20000)
> +
> +#define MMAP_EEPROM_OFFSET(x)		(EEPROM_REG_ADDR_BASE + (x))
> +
> +#define EEPROM_CMD_REG			(0x00)
> +#define EEPROM_DATA_REG			(0x04)
> +#define EEPROM_CFG_REG			(0x08)
> +
> +#define EEPROM_CMD_EPC_BUSY_BIT		BIT(31)
> +#define EEPROM_CMD_EPC_TIMEOUT_BIT	BIT(17)
> +#define EEPROM_CMD_EPC_WRITE		(BIT(29) | BIT(28))
> +
> +#define EEPROM_CFG_BAUD_RATE_100KHZ	BIT(9)
> +#define EEPROM_CFG_SIZE_SEL		BIT(12)
> +#define EEPROM_CFG_PULSE_WIDTH_100KHZ	(BIT(17) | BIT(16))
> +#define OTP_EEPROM_SECTOR_SIZE		(512)
> +#define OTP_SIZE_IN_BYTES		(8 * 1024)
> +#define EEPROM_SIZE_IN_BYTES		(8 * 1024)
> +
> +struct pci1xxxx_otp_e2p_device {
> +	struct pci1xxxx_otp_e2p_disk *otp_e2p_device;
> +	struct auxiliary_device *pdev;
> +	void __iomem *reg_base;
> +	int block_device_count;
> +};
> +
> +struct pci1xxxx_otp_e2p_disk {
> +	struct blk_mq_tag_set tag_set;
> +	struct auxiliary_device *pdev;
> +	struct request_queue *queue;
> +	struct mutex lock;
> +	struct gendisk *gd;
> +	bool has_eeprom;
> +	int (*disk_write_byte)(struct pci1xxxx_otp_e2p_device *priv,
> +		unsigned long byte_offset, u8 value);
> +	int (*disk_read_byte)(struct pci1xxxx_otp_e2p_device *priv,
> +		unsigned long byte_offset, u8 *data);
> +};
> +
> +static int otp_sector_count = OTP_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
> +static int e2p_sector_count = EEPROM_SIZE_IN_BYTES / OTP_EEPROM_SECTOR_SIZE;
> +static int otp_device_count, e2p_device_count;
> +static int otp_block_driver_major;
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_e2p_device *priv, u16 address)
> +{
> +	u32 lo, hi;
> +
> +	lo = address & 0xFF;
> +	hi = (address & 0x1f00) >> 8;
> +	writel(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> +	writel(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static int set_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
> +{
> +	void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	u8 data;
> +
> +	writel(CFG_SYS_LOCK_PF3, p);
> +	data = readl(p);
> +	if (data != CFG_SYS_LOCK_PF3)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static int release_sys_lock(struct pci1xxxx_otp_e2p_device *priv)
> +{
> +	void __iomem *p = priv->reg_base + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> +	u8 data;
> +
> +	data = readl(p);
> +	if (data != CFG_SYS_LOCK_PF3)
> +		return 0;
> +
> +	writel(0, p);
> +
> +	data = readl(p);
> +	if (data & CFG_SYS_LOCK_PF3)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static int otp_e2p_device_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct pci1xxxx_otp_e2p_disk *disk_priv;
> +	struct pci1xxxx_otp_e2p_device *priv;
> +	struct auxiliary_device *pdev;
> +	int retval = 0;

No need to initialize.

> +	u8 data;
> +
> +	disk_priv = bdev->bd_disk->private_data;
> +	pdev = (struct auxiliary_device *)disk_priv->pdev;
> +	priv = dev_get_drvdata(&pdev->dev);
> +
> +	mutex_lock(&disk_priv->lock);
> +
> +	retval = set_sys_lock(priv);
> +	if (retval)
> +		goto exit;
> +
> +	if (!disk_priv->has_eeprom) {
> +		data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> +		writel((data & ~OTP_PWR_DN_BIT), priv->reg_base +
> +			MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> +	}
> +
> +exit:
> +	mutex_unlock(&disk_priv->lock);
> +	return retval;
> +}
> +
> +static void otp_e2p_device_release(struct gendisk *disk, fmode_t mode)
> +{
> +	struct pci1xxxx_otp_e2p_disk *disk_priv;
> +	struct pci1xxxx_otp_e2p_device *priv;
> +	u8 data;
> +
> +	disk_priv = disk->private_data;
> +	priv = dev_get_drvdata(&disk_priv->pdev->dev);
> +
> +	mutex_lock(&disk_priv->lock);
> +
> +	if (!disk_priv->has_eeprom) {
> +		data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> +		writel((data | OTP_PWR_DN_BIT), priv->reg_base +
> +			MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> +	}
> +	release_sys_lock(priv);
> +
> +	mutex_unlock(&disk_priv->lock);
> +}
> +
> +static int e2p_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				 unsigned long byte_offset, u8 value)
> +{
> +	u32 data;
> +
> +	/* Write the value into EEPROM_DATA_REG register */
> +	writel(value, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> +	data = EEPROM_CMD_EPC_TIMEOUT_BIT | EEPROM_CMD_EPC_WRITE | byte_offset;
> +
> +	/* Write the data into EEPROM_CMD_REG register */
> +	writel(data, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> +	writel(EEPROM_CMD_EPC_BUSY_BIT | data, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Wait for the EPC_BUSY bit to get cleared */
> +	do {
> +		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +	} while (data & EEPROM_CMD_EPC_BUSY_BIT);

Would it make sense to use read_poll_timeout() as below, instead of a 
busy loop?

> +
> +	if (data & EEPROM_CMD_EPC_TIMEOUT_BIT)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +static int e2p_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				unsigned long byte_offset, u8 *data)
> +{
> +	u32 regval;
> +
> +	/*
> +	 * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
> +	 * register
> +	 */
> +	writel(byte_offset, priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> +	writel(EEPROM_CMD_EPC_BUSY_BIT | byte_offset, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Wait for the EPC_BUSY bit to get cleared */
> +	do {
> +		regval = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +	} while (regval & EEPROM_CMD_EPC_BUSY_BIT);

Would it make sense to use read_poll_timeout() as below, instead of a 
busy loop?


> +
> +	if (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)
> +		return -EBUSY;
> +
> +	/* Read the contents from the EEPROM_DATA_REG */
> +	*data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> +	return 0;
> +}
> +
> +static bool is_e2p_responsive(struct pci1xxxx_otp_e2p_device *priv)
> +{
> +	u32 data;
> +
> +	if (set_sys_lock(priv))
> +		return false;
> +
> +	writel((EEPROM_CFG_PULSE_WIDTH_100KHZ | EEPROM_CFG_SIZE_SEL |
> +		EEPROM_CFG_BAUD_RATE_100KHZ), priv->reg_base +
> +		MMAP_EEPROM_OFFSET(EEPROM_CFG_REG));
> +
> +	/*
> +	 * Write the byte offset into the EPC_ADDRESS field of EEPROM_CMD_REG
> +	 * register
> +	 */
> +	writel(EEPROM_CMD_EPC_TIMEOUT_BIT, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of EEPROM_CMD_REG register */
> +	writel(EEPROM_CMD_EPC_BUSY_BIT, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> +	/* Wait for the EPC_BUSY bit to get cleared or timeout bit to get set*/
> +	do {
> +		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +	} while (data & EEPROM_CMD_EPC_BUSY_BIT);

Would it make sense to use read_poll_timeout() as below, instead of a 
busy loop?

> +
> +	/* If EPC_TIMEOUT is set, then the EEPROM is not responsive */
> +	release_sys_lock(priv);
> +
> +	if (data & EEPROM_CMD_EPC_TIMEOUT_BIT) {
> +		dev_err(&priv->pdev->dev,
> +			"EPC_Timeout, EEPROM is unresponsive: %x\n", data);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int otp_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				 unsigned long byte_offset, u8 value)
> +{
> +	u8 data;
> +	int ret;
> +
> +	if (!value)
> +		return 0;
> +
> +	otp_device_set_address(priv, (u16)byte_offset);
> +
> +	/* Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register */
> +	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +	writel((data | OTP_PGM_MODE_BYTE_BIT), priv->reg_base +
> +		MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> +
> +	/* Write the value to program into OTP_PRGM_DATA register */
> +	writel(value, priv->reg_base + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> +
> +	/* Set OTP_PROGRAM command bit in OTP_FUNC_CMD register */
> +	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +	writel((data | OTP_FUNC_PGM_BIT), priv->reg_base +
> +		MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +
> +	/* Set OTP_GO command bit in OTP_CMD_GO register */
> +	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +	writel((data | OTP_CMD_GO_BIT), priv->reg_base +
> +		MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +	ret = read_poll_timeout(readl, data, !(data & OTP_STATUS_BUSY_BIT),
> +				OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
> +				true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));

(here)

> +	if (ret < 0)
> +		return -EBUSY;
> +
> +	/* Read the result from OTP_RD_DATA register */
> +	data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> +	if (data & 0x02)
> +		return 0;
> +
> +	dev_err(&priv->pdev->dev, "OTP write read mismatch 0x%x\n", data);
> +	return -EIO;
> +}
> +
> +static int otp_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				unsigned long byte_offset, u8 *data)
> +{
> +	int ret;

Some places use ret, some other retval. (I personnaly prefer ret because 
it is shorter)

Does it make sense to be consistent in the naming?

> +
> +	otp_device_set_address(priv, (u16)byte_offset);
> +
> +	/* Set OTP_READ command bit in OTP_FUNC_CMD register */
> +	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +	writel((*data | OTP_FUNC_RD_BIT), priv->reg_base +
> +		MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> +
> +	/* Set OTP_GO command bit in OTP_CMD_GO register */
> +	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +	writel((*data | OTP_CMD_GO_BIT), priv->reg_base +
> +		MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> +	ret = read_poll_timeout(readl, *data, !(*data & OTP_STATUS_BUSY_BIT),
> +				OTP_STATUS_READ_DELAY_US, OTP_STATUS_READ_TIMEOUT_US,
> +				true, priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +	if (ret < 0)
> +		return -EBUSY;
> +
> +	/* Read the result from OTP_RD_DATA register */
> +	*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> +	return 0;
> +}
> +
> +static int otp_e2P_device_transfer(struct request *req)
> +{
> +	struct pci1xxxx_otp_e2p_disk *disk_priv;
> +	struct pci1xxxx_otp_e2p_device *priv;
> +	unsigned long sector;
> +	unsigned long nsect;
> +	long byte_offset;
> +	int retval;
> +	u8 *buffer;
> +	int write;
> +	int i, j;
> +
> +	sector = blk_rq_pos(req);
> +	nsect = blk_rq_cur_sectors(req);
> +	buffer = bio_data(req->bio);
> +	write = rq_data_dir(req);
> +	disk_priv = req->q->disk->private_data;
> +	priv = dev_get_drvdata(&disk_priv->pdev->dev);
> +
> +	if (write) {
> +		for (i = 0; i < nsect; i++) {
> +			byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
> +			for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {

Not sure at all if it is a win, but testing for 'write' here would avoid 
some code duplication.

> +				retval = disk_priv->disk_write_byte(priv,
> +								    byte_offset + j,
> +								    *buffer);
> +				if (retval)
> +					return retval;
> +
> +				buffer++;
> +			}
> +		}
> +	} else {
> +		for (i = 0; i < nsect; i++) {
> +			byte_offset = (sector + i) * OTP_EEPROM_SECTOR_SIZE;
> +			for (j = 0; j < OTP_EEPROM_SECTOR_SIZE; j++) {
> +				retval = disk_priv->disk_read_byte(priv,
> +								   byte_offset + j,
> +								   buffer);
> +				if (retval)
> +					return retval;
> +
> +				buffer++;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static blk_status_t OTPE2P_queue_rq(struct blk_mq_hw_ctx *hctx,
> +				    const struct blk_mq_queue_data *bd)
> +{
> +	struct request *req = bd->rq;
> +
> +	blk_mq_start_request(req);
> +	if (!otp_e2P_device_transfer(req)) {
> +		blk_mq_end_request(req, BLK_STS_OK);
> +		return BLK_STS_OK;
> +	}
> +
> +	return BLK_STS_IOERR;
> +}
> +
> +static const struct blk_mq_ops OTPE2P_mq_ops = {
> +	.queue_rq	= OTPE2P_queue_rq,
> +};
> +
> +static const struct block_device_operations otp_e2p_device_ops = {
> +	.owner = THIS_MODULE,
> +	.open = otp_e2p_device_open,
> +	.release = otp_e2p_device_release,
> +};
> +
> +static int otp_e2p_device_create_block_device(struct auxiliary_device *aux_dev)
> +{
> +	struct auxiliary_device_wrapper *aux_dev_wrapper;
> +	struct pci1xxxx_otp_e2p_device *priv;
> +	struct gp_aux_data_type *pdata;
> +	int retval = 0, i;
> +
> +	aux_dev_wrapper = container_of(aux_dev, struct auxiliary_device_wrapper, aux_dev);
> +	pdata = &aux_dev_wrapper->gp_aux_data;
> +	if (!pdata) {
> +		dev_err(&aux_dev->dev, "Invalid data in aux_dev_wrapper->gp_aux_data\n");
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(&aux_dev->dev, sizeof(struct pci1xxxx_otp_e2p_device),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pdev = aux_dev;
> +
> +	dev_set_drvdata(&aux_dev->dev, priv);
> +
> +	if (!devm_request_mem_region(&aux_dev->dev, pdata->region_start +
> +				     PERI_PF3_SYSTEM_REG_ADDR_BASE,
> +		PERI_PF3_SYSTEM_REG_LENGTH, aux_dev->name)) {
> +		dev_err(&aux_dev->dev, "can't request otpe2p region\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv->reg_base = devm_ioremap(&aux_dev->dev, pdata->region_start +
> +				      PERI_PF3_SYSTEM_REG_ADDR_BASE,
> +				      PERI_PF3_SYSTEM_REG_LENGTH);
> +	if (!priv->reg_base) {
> +		dev_err(&aux_dev->dev, "ioremap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	priv->block_device_count = 0;

Useless. It is zalloc()'ed and overwritten just below anyway.

> +	if (is_e2p_responsive(priv))
> +		priv->block_device_count = 2;
> +	else
> +		priv->block_device_count = 1;
> +
> +	priv->otp_e2p_device = devm_kzalloc(&priv->pdev->dev,
> +					    priv->block_device_count *
> +					    sizeof(struct pci1xxxx_otp_e2p_disk),
> +					    GFP_KERNEL);

devm_kcalloc()?

> +	if (!priv->otp_e2p_device)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->block_device_count; i++) {

having something like:
		struct pci1xxxx_otp_e2p_disk *e2p_dev = &priv->otp_e2p_device[i];

would slighly simplify the code below and would help keeping statement 
on 1 line.

> +		mutex_init(&priv->otp_e2p_device[i].lock);
> +		priv->otp_e2p_device[i].tag_set.ops = &OTPE2P_mq_ops;
> +		priv->otp_e2p_device[i].tag_set.nr_hw_queues = 1;
> +		priv->otp_e2p_device[i].tag_set.queue_depth = 16;
> +		priv->otp_e2p_device[i].tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> +
> +		retval = blk_mq_alloc_tag_set(&priv->otp_e2p_device[i].tag_set);
> +		if (retval) {
> +			dev_err(&aux_dev->dev, "blk_mq_alloc_tag_set failed\n");

Should we have something like:
			if (i)
				goto otp_free_disk;
			else

> +			return retval;
> +		}
> +
> +		priv->otp_e2p_device[i].queue =
> +			blk_mq_init_queue(&priv->otp_e2p_device[i].tag_set);
> +		if (IS_ERR(priv->otp_e2p_device[i].queue)) {
> +			retval = PTR_ERR(priv->otp_e2p_device[i].queue);
> +			priv->otp_e2p_device[i].queue = NULL;
> +			if (i)
> +				goto e2p_free_tag_set;
> +			else
> +				goto otp_free_tag_set;
> +		}
> +
> +		blk_queue_logical_block_size(priv->otp_e2p_device[i].queue,
> +				OTP_EEPROM_SECTOR_SIZE);
> +		blk_queue_physical_block_size(priv->otp_e2p_device[i].queue,
> +				OTP_EEPROM_SECTOR_SIZE);
> +		priv->otp_e2p_device[i].queue->queuedata = priv;
> +		priv->otp_e2p_device[i].gd =
> +			blk_mq_alloc_disk(&priv->otp_e2p_device[i].tag_set,
> +					  NULL);
> +		if (IS_ERR(priv->otp_e2p_device[i].gd)) {
> +			retval = PTR_ERR(priv->otp_e2p_device[i].gd);
> +			if (i)
> +				goto e2p_destroy_queue;
> +			else
> +				goto otp_destroy_queue;
> +		}
> +
> +		priv->otp_e2p_device[i].pdev = aux_dev;
> +		priv->otp_e2p_device[i].gd->major = otp_block_driver_major;
> +		priv->otp_e2p_device[i].gd->minors = 1;
> +		priv->otp_e2p_device[i].gd->first_minor =
> +					otp_device_count + e2p_device_count;
> +		priv->otp_e2p_device[i].gd->fops = &otp_e2p_device_ops;
> +		priv->otp_e2p_device[i].gd->private_data =
> +			&priv->otp_e2p_device[i];
> +
> +		if (i == 0) {
> +			snprintf(priv->otp_e2p_device[i].gd->disk_name,
> +					32, "PCI1xxxxOTP%x", otp_device_count);

s/32/DISK_NAME_LEN/ ?

> +			set_capacity(priv->otp_e2p_device[i].gd, otp_sector_count);
> +			priv->otp_e2p_device[i].disk_read_byte = otp_device_read_byte;
> +			priv->otp_e2p_device[i].disk_write_byte = otp_device_write_byte;
> +			otp_device_count++;
> +		} else {
> +			snprintf(priv->otp_e2p_device[i].gd->disk_name,
> +					32, "PCI1xxxxE2P%x", otp_device_count - 1);

s/32/DISK_NAME_LEN/ ?

> +			set_capacity(priv->otp_e2p_device[i].gd, e2p_sector_count);
> +			priv->otp_e2p_device[i].has_eeprom = true;
> +			priv->otp_e2p_device[i].disk_read_byte = e2p_device_read_byte;
> +			priv->otp_e2p_device[i].disk_write_byte = e2p_device_write_byte;
> +			e2p_device_count++;
> +		}
> +
> +		retval = add_disk(priv->otp_e2p_device[i].gd);
> +		if (retval) {
> +			if (i)
> +				goto e2p_free_disk;
> +			else
> +				goto otp_free_disk;
> +		}
> +	}
> +
> +	return retval;
> +
> +e2p_free_disk:
> +	del_gendisk(priv->otp_e2p_device[1].gd);
> +	put_disk(priv->otp_e2p_device[1].gd);
> +e2p_destroy_queue:
> +	blk_mq_destroy_queue(priv->otp_e2p_device[1].queue);
> +e2p_free_tag_set:
> +	blk_mq_free_tag_set(&priv->otp_e2p_device[1].tag_set);
> +otp_free_disk:
> +	del_gendisk(priv->otp_e2p_device[0].gd);
> +	put_disk(priv->otp_e2p_device[0].gd);
> +otp_destroy_queue:
> +	blk_mq_destroy_queue(priv->otp_e2p_device[0].queue);
> +otp_free_tag_set:
> +	blk_mq_free_tag_set(&priv->otp_e2p_device[0].tag_set);
> +
> +	dev_err(&aux_dev->dev,
> +		"otp/eeprom device enumeration failed with errno = %d\n",
> +		retval);

Maybe dev_err_probe() here?
(and/or even in other places above?)

> +	return retval;
> +}
> +
> +static void pci1xxxx_otp_e2p_remove(struct auxiliary_device *aux_dev)
> +{
> +	struct pci1xxxx_otp_e2p_device *priv = dev_get_drvdata(&aux_dev->dev);
> +	int i;
> +
> +	for (i = 0; i < priv->block_device_count; i++) {
> +		if (priv->otp_e2p_device[i].queue)
> +			blk_mq_destroy_queue(priv->otp_e2p_device[i].queue);
> +
> +		if (priv->otp_e2p_device[i].gd) {
> +			del_gendisk(priv->otp_e2p_device[i].gd);
> +			put_disk(priv->otp_e2p_device[i].gd);
> +			blk_mq_free_tag_set(&priv->otp_e2p_device[i].tag_set);
> +		}
> +	}
> +}
> +
> +static int pci1xxxx_otp_e2p_probe(struct auxiliary_device *aux_dev,
> +				  const struct auxiliary_device_id *id)
> +{
> +	return otp_e2p_device_create_block_device(aux_dev);
> +}
> +
> +static const struct auxiliary_device_id pci1xxxx_otp_e2p_auxiliary_id_table[] = {
> +	{.name = "mchp_pci1xxxx_gp.gp_otp_e2p"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pci1xxxx_otp_e2p_auxiliary_id_table);
> +
> +static struct auxiliary_driver pci1xxxx_otp_e2p_driver = {
> +	.driver = {
> +		.name = "PCI1xxxxOTPE2P",
> +	},
> +	.probe = pci1xxxx_otp_e2p_probe,
> +	.remove = pci1xxxx_otp_e2p_remove,
> +	.id_table = pci1xxxx_otp_e2p_auxiliary_id_table
> +};
> +
> +static int __init pci1xxxx_otp_e2p_driver_init(void)
> +{
> +	int retval;
> +
> +	otp_block_driver_major = register_blkdev(otp_block_driver_major,
> +						 "OTPBlockDevice");

Would it make sense to have a #define for "OTPBlockDevice"?

> +	if (otp_block_driver_major < 0)
> +		return otp_block_driver_major;
> +
> +	retval = auxiliary_driver_register(&pci1xxxx_otp_e2p_driver);
> +	if (retval)
> +		unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
> +
> +	return retval;
> +}
> +
> +static void __exit pci1xxxx_otp_e2p_driver_exit(void)
> +{
> +	unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
> +	auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);

I think it is harmless, but shouldn't it be done in the reverse order to 
match how resources have been allocated?

> +}
> +
> +module_init(pci1xxxx_otp_e2p_driver_init);
> +module_exit(pci1xxxx_otp_e2p_driver_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>");
> +MODULE_AUTHOR("Tharun Kumar P<tharunkumar.pasumarthi@microchip.com>");
> +MODULE_DESCRIPTION("Microchip Technology Inc. PCI1xxxx OTP EEPROM programmer");

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Michael Walle]

>>> +	/* Wait for the EPC_BUSY bit to get cleared */
>>> +	do {
>>> +		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
>>> +	} while (data & EEPROM_CMD_EPC_BUSY_BIT);
>>
>> Again, you can not sit and spin in a busy-wait like this with no chance
>> to recover if something goes wrong with the hardware (hint, what if it
>> got removed?)

Also, it is good practice to CC people who did comments on the
former versions.

-michael

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Tharunkumar.Pasumarthi]

> From: Michael Walle <michael@walle.cc>
> Sent: Monday, February 13, 2023 5:30 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Also, it is good practice to CC people who did comments on the former
> versions.

Okay Michael. Noted.

Thanks,
Tharun Kumar P

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Tharunkumar.Pasumarthi]

> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Sent: Sunday, February 12, 2023 1:33 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>; linux-kernel@vger.kernel.org
> Cc: linux-gpio@vger.kernel.org; gregkh@linuxfoundation.org;
> arnd@arndb.de; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add
> OTP/EEPROM driver for the pci1xxxx switch
> > +     unregister_blkdev(otp_block_driver_major, "OTPBlockDevice");
> > +     auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);
> 
> I think it is harmless, but shouldn't it be done in the reverse order to match
> how resources have been allocated?

Hi Christophe, 
Thanks for your comments.

In the earlier version of patch, auxiliary_driver_unregister was done before unregister_blkdev.
But Greg suggested to change it this way - "You need to unregister your block device _BEFORE_ the aux device goes away underneath it". Hence followed this order.

Thanks,
Tharun Kumar P

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Tharunkumar.Pasumarthi]

> From: Tharunkumar Pasumarthi - I67821
> Sent: Tuesday, February 14, 2023 12:08 PM
> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; linux-
> kernel@vger.kernel.org
> Hi Christophe,
> Thanks for your comments.
> 
> In the earlier version of patch, auxiliary_driver_unregister was done before
> unregister_blkdev.
> But Greg suggested to change it this way - "You need to unregister your block
> device _BEFORE_ the aux device goes away underneath it". Hence followed
> this order.

Since block device is a child of the aux device, it makes sense to remove block device before removing the aux_device.
So that unwinding of the device stack is done in the reverse order of creation.

Thanks,
Tharun Kumar P

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Michael Walle]

> 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 provides the OTP/EEPROM
> driver to support the same.

Why isn't this driver using the nvmem subsystem which is usually used for
OTP and EEPROM?

-michael

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Tuesday, February 14, 2023 1:58 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add
> OTP/EEPROM driver for the pci1xxxx switch
> 
> > 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 provides the OTP/EEPROM driver to support the same.
> 
> Why isn't this driver using the nvmem subsystem which is usually used for
> OTP and EEPROM?
Michael, these OTP and EEPROM memories do not have any fixed location registers which
store values (Eg. mac address, config parameters, etc) at fixed offsets.
It stores a bunch of records, each of which has some data to be written into the device's
hardware registers at different locations. These records are directly consumed by the hardware
and interpreted without the involvement of the software.
Therefore, we don't require any OTP / EEPROM register map to be input to the OS / driver through
device tree or board files.
I only had to enumerate two separate block devices using the driver so that the config binary files can be 
overlayed using the dd command.
Since this is not fitting like a conventional nvme device, I didn't choose the nvme subsystem.
Please let me know your thoughts / comments if any.

Thank You.

Regards,
Kumar

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Michael Walle]

Hi,

>> > 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 provides the OTP/EEPROM driver to support the same.
>> 
>> Why isn't this driver using the nvmem subsystem which is usually used 
>> for
>> OTP and EEPROM?
> Michael, these OTP and EEPROM memories do not have any fixed location
> registers which
> store values (Eg. mac address, config parameters, etc) at fixed 
> offsets.
> It stores a bunch of records, each of which has some data to be
> written into the device's
> hardware registers at different locations. These records are directly
> consumed by the hardware
> and interpreted without the involvement of the software.
> Therefore, we don't require any OTP / EEPROM register map to be input
> to the OS / driver through
> device tree or board files.
> I only had to enumerate two separate block devices using the driver so
> that the config binary files can be
> overlayed using the dd command.
> Since this is not fitting like a conventional nvme device, I didn't
> choose the nvme subsystem.
> Please let me know your thoughts / comments if any.

So this is only for provisioning. i.e. during manufacturing a board
which uses this PCI bridge? There are no kernel users, nor is
there a common interface towards user-space. But just some block
device (why not a char device?) exposed to userspace. I presume
there is a companion userspace application for it? Why do you take
the extra step and have a (random) kernel interface, you could
also just access the PCI device directly from userspace within your
companion application, e.g. through libpci.

Just my two cents. I guess it's up to you and Greg who has to
maintain it.

-michael

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> Hi,
> 
> > > > 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 provides the OTP/EEPROM driver to support the same.
> > > 
> > > Why isn't this driver using the nvmem subsystem which is usually
> > > used for
> > > OTP and EEPROM?
> > Michael, these OTP and EEPROM memories do not have any fixed location
> > registers which
> > store values (Eg. mac address, config parameters, etc) at fixed offsets.
> > It stores a bunch of records, each of which has some data to be
> > written into the device's
> > hardware registers at different locations. These records are directly
> > consumed by the hardware
> > and interpreted without the involvement of the software.
> > Therefore, we don't require any OTP / EEPROM register map to be input
> > to the OS / driver through
> > device tree or board files.
> > I only had to enumerate two separate block devices using the driver so
> > that the config binary files can be
> > overlayed using the dd command.
> > Since this is not fitting like a conventional nvme device, I didn't
> > choose the nvme subsystem.
> > Please let me know your thoughts / comments if any.
> 
> So this is only for provisioning. i.e. during manufacturing a board
> which uses this PCI bridge? There are no kernel users, nor is
> there a common interface towards user-space. But just some block
> device (why not a char device?) exposed to userspace. I presume
> there is a companion userspace application for it? Why do you take
> the extra step and have a (random) kernel interface, you could
> also just access the PCI device directly from userspace within your
> companion application, e.g. through libpci.

Yeah, why not just use userspace, I missed that, thanks!

greg k-h

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Wednesday, February 15, 2023 2:29 PM
> To: Michael Walle <michael@walle.cc>
> Cc: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>; Tharunkumar Pasumarthi -
> I67821 <Tharunkumar.Pasumarthi@microchip.com>; UNGLinuxDriver 
> <UNGLinuxDriver@microchip.com>; arnd@arndb.de; linux- 
> gpio@vger.kernel.org; linux-kernel@vger.kernel.org; 
> srinivas.kandagatla@linaro.org
> Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add 
> OTP/EEPROM driver for the pci1xxxx switch
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> > Hi,
> >
> > > > > 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 provides the OTP/EEPROM driver to 
> > > > > support the
> same.
> > > >
> > > > Why isn't this driver using the nvmem subsystem which is usually 
> > > > used for OTP and EEPROM?
> > > Michael, these OTP and EEPROM memories do not have any fixed 
> > > location registers which store values (Eg. mac address, config 
> > > parameters, etc) at fixed offsets.
> > > It stores a bunch of records, each of which has some data to be 
> > > written into the device's hardware registers at different locations.
> > > These records are directly consumed by the hardware and 
> > > interpreted without the involvement of the software.
> > > Therefore, we don't require any OTP / EEPROM register map to be 
> > > input to the OS / driver through device tree or board files.
> > > I only had to enumerate two separate block devices using the 
> > > driver so that the config binary files can be overlayed using the 
> > > dd command.
> > > Since this is not fitting like a conventional nvme device, I 
> > > didn't choose the nvme subsystem.
> > > Please let me know your thoughts / comments if any.
> >
> > So this is only for provisioning. i.e. during manufacturing a board 
> > which uses this PCI bridge? There are no kernel users, nor is there 
> > a common interface towards user-space. But just some block device 
> > (why not a char device?) exposed to userspace. I presume there is a 
> > companion userspace application for it? Why do you take the extra 
> > step and have a (random) kernel interface, you could also just 
> > access the PCI device directly from userspace within your companion 
> > application, e.g. through libpci.
> 
> Yeah, why not just use userspace, I missed that, thanks!
Greg & Michael, I do not want to expose the entire or even partial set of device
registers to the user space access directly for safety reasons.
I think hardware registers shall be accessible only through safe and robust kernel
mode components and that the user space shall only be able to access the device
through the kernel mode services.
I want the user to use the hardware only in those ways designated by the driver.
We were using the "busybox devmem" to access the hardware registers directly and to program the EEPROM / OTP.
But we understood that it cannot be an end user solution in all cases and based on some of the operating
environments, there can be some restrictions in opening the direct hardware access to the user space.
Please let me know your thoughts / comments if any.

Thank You.

Regards,
Kumar

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

> From: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> Sent: Wednesday, February 15, 2023 3:18 PM
> To: Greg KH <gregkh@linuxfoundation.org>; Michael Walle 
> <michael@walle.cc>
> Cc: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>; UNGLinuxDriver 
> <UNGLinuxDriver@microchip.com>; arnd@arndb.de; linux- 
> gpio@vger.kernel.org; linux-kernel@vger.kernel.org; 
> srinivas.kandagatla@linaro.org
> Subject: RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add 
> OTP/EEPROM driver for the pci1xxxx switch
> 
> >
> > On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> > > Hi,
> > >
> > > > > > 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 provides the OTP/EEPROM 
> > > > > > driver to support the
> > same.
> > > > >
> > > > > Why isn't this driver using the nvmem subsystem which is 
> > > > > usually used for OTP and EEPROM?
> > > > Michael, these OTP and EEPROM memories do not have any fixed 
> > > > location registers which store values (Eg. mac address, config 
> > > > parameters, etc) at fixed offsets.
> > > > It stores a bunch of records, each of which has some data to be 
> > > > written into the device's hardware registers at different locations.
> > > > These records are directly consumed by the hardware and 
> > > > interpreted without the involvement of the software.
> > > > Therefore, we don't require any OTP / EEPROM register map to be 
> > > > input to the OS / driver through device tree or board files.
> > > > I only had to enumerate two separate block devices using the 
> > > > driver so that the config binary files can be overlayed using 
> > > > the dd command.
> > > > Since this is not fitting like a conventional nvme device, I 
> > > > didn't choose the nvme subsystem.
> > > > Please let me know your thoughts / comments if any.
> > >
> > > So this is only for provisioning. i.e. during manufacturing a 
> > > board which uses this PCI bridge? There are no kernel users, nor 
> > > is there a common interface towards user-space. But just some 
> > > block device (why not a char device?) exposed to userspace. I 
> > > presume there is a companion userspace application for it? Why do 
> > > you take the extra step and have a (random) kernel interface, you 
> > > could also just access the PCI device directly from userspace 
> > > within your companion application, e.g. through libpci.
> >
> > Yeah, why not just use userspace, I missed that, thanks!
> Greg & Michael, I do not want to expose the entire or even partial set 
> of device registers to the user space access directly for safety reasons.
> I think hardware registers shall be accessible only through safe and 
> robust kernel mode components and that the user space shall only be 
> able to access the device through the kernel mode services.
> I want the user to use the hardware only in those ways designated by 
> the driver.
> We were using the "busybox devmem" to access the hardware registers 
> directly and to program the EEPROM / OTP.
> But we understood that it cannot be an end user solution in all cases 
> and based on some of the operating environments, there can be some 
> restrictions in opening the direct hardware access to the user space.
> Please let me know your thoughts / comments if any.

I missed one more important point. This driver is targeted not just for the manufacturing environment.
we want to be able to update the OTP / EEPROM when the device is in the field also.

Thank You.

Regards,
Kumar

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Michael Walle]

>> > > > > > 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 provides the OTP/EEPROM
>> > > > > > driver to support the
>> > same.
>> > > > >
>> > > > > Why isn't this driver using the nvmem subsystem which is
>> > > > > usually used for OTP and EEPROM?
>> > > > Michael, these OTP and EEPROM memories do not have any fixed
>> > > > location registers which store values (Eg. mac address, config
>> > > > parameters, etc) at fixed offsets.
>> > > > It stores a bunch of records, each of which has some data to be
>> > > > written into the device's hardware registers at different locations.
>> > > > These records are directly consumed by the hardware and
>> > > > interpreted without the involvement of the software.
>> > > > Therefore, we don't require any OTP / EEPROM register map to be
>> > > > input to the OS / driver through device tree or board files.
>> > > > I only had to enumerate two separate block devices using the
>> > > > driver so that the config binary files can be overlayed using
>> > > > the dd command.
>> > > > Since this is not fitting like a conventional nvme device, I
>> > > > didn't choose the nvme subsystem.
>> > > > Please let me know your thoughts / comments if any.
>> > >
>> > > So this is only for provisioning. i.e. during manufacturing a
>> > > board which uses this PCI bridge? There are no kernel users, nor
>> > > is there a common interface towards user-space. But just some
>> > > block device (why not a char device?) exposed to userspace. I
>> > > presume there is a companion userspace application for it? Why do
>> > > you take the extra step and have a (random) kernel interface, you
>> > > could also just access the PCI device directly from userspace
>> > > within your companion application, e.g. through libpci.
>> >
>> > Yeah, why not just use userspace, I missed that, thanks!
>> Greg & Michael, I do not want to expose the entire or even partial set
>> of device registers to the user space access directly for safety 
>> reasons.

I presume that utility will need root anyway. IOW, it doesn't make
sense to be used as a normal user.

>> I think hardware registers shall be accessible only through safe and
>> robust kernel mode components and that the user space shall only be
>> able to access the device through the kernel mode services.
>> I want the user to use the hardware only in those ways designated by
>> the driver.

I don't get that point. It is not something you are doing regularly
or maybe even in a running system. I guess you'll have to do a reboot
anyway after you modified some registers defaults. Anyway, it's still
only for provisioning.

>> We were using the "busybox devmem" to access the hardware registers
>> directly and to program the EEPROM / OTP.
>> But we understood that it cannot be an end user solution in all cases
>> and based on some of the operating environments, there can be some
>> restrictions in opening the direct hardware access to the user space.

Yes, then just build a tool around libpci as I've mentioned. Who is the
user here? An OEM? An end-user? What would an end-user update within
your PCI bridge?

As a matter of fact, it actually makes it harder for a user because
he will also need this kernel driver (which might be disabled for
whatever reason).

>> Please let me know your thoughts / comments if any.
> 
> I missed one more important point. This driver is targeted not just
> for the manufacturing environment.
> we want to be able to update the OTP / EEPROM when the device is in
> the field also.

What would be an example of that?

-michael

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Wed, Feb 15, 2023 at 09:56:46AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> > From: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> > Sent: Wednesday, February 15, 2023 3:18 PM
> > To: Greg KH <gregkh@linuxfoundation.org>; Michael Walle 
> > <michael@walle.cc>
> > Cc: Tharunkumar Pasumarthi - I67821
> > <Tharunkumar.Pasumarthi@microchip.com>; UNGLinuxDriver 
> > <UNGLinuxDriver@microchip.com>; arnd@arndb.de; linux- 
> > gpio@vger.kernel.org; linux-kernel@vger.kernel.org; 
> > srinivas.kandagatla@linaro.org
> > Subject: RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add 
> > OTP/EEPROM driver for the pci1xxxx switch
> > 
> > >
> > > On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> > > > Hi,
> > > >
> > > > > > > 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 provides the OTP/EEPROM 
> > > > > > > driver to support the
> > > same.
> > > > > >
> > > > > > Why isn't this driver using the nvmem subsystem which is 
> > > > > > usually used for OTP and EEPROM?
> > > > > Michael, these OTP and EEPROM memories do not have any fixed 
> > > > > location registers which store values (Eg. mac address, config 
> > > > > parameters, etc) at fixed offsets.
> > > > > It stores a bunch of records, each of which has some data to be 
> > > > > written into the device's hardware registers at different locations.
> > > > > These records are directly consumed by the hardware and 
> > > > > interpreted without the involvement of the software.
> > > > > Therefore, we don't require any OTP / EEPROM register map to be 
> > > > > input to the OS / driver through device tree or board files.
> > > > > I only had to enumerate two separate block devices using the 
> > > > > driver so that the config binary files can be overlayed using 
> > > > > the dd command.
> > > > > Since this is not fitting like a conventional nvme device, I 
> > > > > didn't choose the nvme subsystem.
> > > > > Please let me know your thoughts / comments if any.
> > > >
> > > > So this is only for provisioning. i.e. during manufacturing a 
> > > > board which uses this PCI bridge? There are no kernel users, nor 
> > > > is there a common interface towards user-space. But just some 
> > > > block device (why not a char device?) exposed to userspace. I 
> > > > presume there is a companion userspace application for it? Why do 
> > > > you take the extra step and have a (random) kernel interface, you 
> > > > could also just access the PCI device directly from userspace 
> > > > within your companion application, e.g. through libpci.
> > >
> > > Yeah, why not just use userspace, I missed that, thanks!
> > Greg & Michael, I do not want to expose the entire or even partial set 
> > of device registers to the user space access directly for safety reasons.

But that's all exposed here through this block device, right?

And this is already exposed to userspace today, no need to add anything
the kernel already provides this.

> > I think hardware registers shall be accessible only through safe and 
> > robust kernel mode components and that the user space shall only be 
> > able to access the device through the kernel mode services.

Again, PCI devices are already exposed to userspace today, what am I
missing?

> > I want the user to use the hardware only in those ways designated by 
> > the driver.
> > We were using the "busybox devmem" to access the hardware registers 
> > directly and to program the EEPROM / OTP.
> > But we understood that it cannot be an end user solution in all cases 
> > and based on some of the operating environments, there can be some 
> > restrictions in opening the direct hardware access to the user space.

What restrictions are you referring to?


> > Please let me know your thoughts / comments if any.
> 
> I missed one more important point. This driver is targeted not just for the manufacturing environment.
> we want to be able to update the OTP / EEPROM when the device is in the field also.

Great, then this really should just be using the firmware download
interface if you wish to write to this hardware.  Don't expose this as a
block device, as that's not what this is.

thanks,

greg k-h

RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Wednesday, February 15, 2023 5:15 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add
> OTP/EEPROM driver for the pci1xxxx switch
> 
> 
> On Wed, Feb 15, 2023 at 09:56:46AM +0000,
> Kumaravel.Thiagarajan@microchip.com wrote:
> > > From: Kumaravel Thiagarajan - I21417
> > > <Kumaravel.Thiagarajan@microchip.com>
> > > Sent: Wednesday, February 15, 2023 3:18 PM
> > > To: Greg KH <gregkh@linuxfoundation.org>; Michael Walle
> > > <michael@walle.cc>
> > > Cc: Tharunkumar Pasumarthi - I67821
> > > <Tharunkumar.Pasumarthi@microchip.com>; UNGLinuxDriver
> > > <UNGLinuxDriver@microchip.com>; arnd@arndb.de; linux-
> > > gpio@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > srinivas.kandagatla@linaro.org
> > > Subject: RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx:
> > > Add OTP/EEPROM driver for the pci1xxxx switch
> > >
> > > >
> > > > On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> > > > > Hi,
> > > > >
> > > > > > > > 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 provides the
> > > > > > > > OTP/EEPROM driver to support the
> > > > same.
> > > > > > >
> > > > > > > Why isn't this driver using the nvmem subsystem which is
> > > > > > > usually used for OTP and EEPROM?
> > > > > > Michael, these OTP and EEPROM memories do not have any fixed
> > > > > > location registers which store values (Eg. mac address, config
> > > > > > parameters, etc) at fixed offsets.
> > > > > > It stores a bunch of records, each of which has some data to
> > > > > > be written into the device's hardware registers at different
> locations.
> > > > > > These records are directly consumed by the hardware and
> > > > > > interpreted without the involvement of the software.
> > > > > > Therefore, we don't require any OTP / EEPROM register map to
> > > > > > be input to the OS / driver through device tree or board files.
> > > > > > I only had to enumerate two separate block devices using the
> > > > > > driver so that the config binary files can be overlayed using
> > > > > > the dd command.
> > > > > > Since this is not fitting like a conventional nvme device, I
> > > > > > didn't choose the nvme subsystem.
> > > > > > Please let me know your thoughts / comments if any.
> > > > >
> > > > > So this is only for provisioning. i.e. during manufacturing a
> > > > > board which uses this PCI bridge? There are no kernel users, nor
> > > > > is there a common interface towards user-space. But just some
> > > > > block device (why not a char device?) exposed to userspace. I
> > > > > presume there is a companion userspace application for it? Why
> > > > > do you take the extra step and have a (random) kernel interface,
> > > > > you could also just access the PCI device directly from
> > > > > userspace within your companion application, e.g. through libpci.
> > > >
> > > > Yeah, why not just use userspace, I missed that, thanks!
> > > Greg & Michael, I do not want to expose the entire or even partial
> > > set of device registers to the user space access directly for safety
> reasons.
> 
> But that's all exposed here through this block device, right?
The block device created by this driver does not expose the device registers to the user space applications.
The device hardware provides separate set of registers to read and write into the OTP memory and EEPROM.
The driver uses these hardware registers and abstracts the programming logic inside and exposes the only the memory as devices to the user space.
I don't have any user program to program the device. I use the Linux dd command only.
If I want to view the contents of the memory, I can use any hex editor tool in Linux this way.

> 
> And this is already exposed to userspace today, no need to add anything the
> kernel already provides this.
Can you explain this? Are you referring to any sysfs directories / files? What is the necessity to do this? I am trying to understand this.
If this is for any debug purpose and whether kernel does this under some conditional compilation or is that the default behavior?
Even if the user is a super user, should he be allowed to access the device hardware registers mandatorily. It should depend on the policy the system owner want to adopt. Right?

> 
> > > I think hardware registers shall be accessible only through safe and
> > > robust kernel mode components and that the user space shall only be
> > > able to access the device through the kernel mode services.
> 
> Again, PCI devices are already exposed to userspace today, what am I
> missing?
Are you referring to any sysfs directories / files? What is the necessity to do this? I am trying to understand this.
If this is for any debug purpose and whether kernel does this under some conditional compilation or is that the default behavior?
Even if the user is a super user, should he be allowed to access the device hardware registers mandatorily. It should depend on the policy the system owner want to adopt. Right?

> 
> > > I want the user to use the hardware only in those ways designated by
> > > the driver.
> > > We were using the "busybox devmem" to access the hardware registers
> > > directly and to program the EEPROM / OTP.
> > > But we understood that it cannot be an end user solution in all
> > > cases and based on some of the operating environments, there can be
> > > some restrictions in opening the direct hardware access to the user
> space.
> 
> What restrictions are you referring to?
I read some articles in internet that exporting the device memory + RAM memory to the user space presents a few security issues that need to be dealt with.
I assumed therefore, exposing the device registers to the user space may not be the default behavior in some operating environments.

> 
> 
> > > Please let me know your thoughts / comments if any.
> >
> > I missed one more important point. This driver is targeted not just for the
> manufacturing environment.
> > we want to be able to update the OTP / EEPROM when the device is in the
> field also.
> 
> Great, then this really should just be using the firmware download interface
> if you wish to write to this hardware.  Don't expose this as a block device, as
> that's not what this is.
As I said earlier, this device is always programmed using the Linux dd command with a 8KB file with "oflag/iflag=direct" option to bypass buffering.
Even though there is no standard file system, I thought enumerating this as a block device would give easy access and quick programming time on ATE.

Greg & Michael,
I am sorry about the delay in response.
This entire week I am into some training sessions.
If required, I would want to discuss more to understand better.

Thank you for your valuable time and information.

Regards,
Kumar

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Thu, Feb 16, 2023 at 11:39:12AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Wednesday, February 15, 2023 5:15 PM
> > To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> > Subject: Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add
> > OTP/EEPROM driver for the pci1xxxx switch

What is this header for?

Please fix your email client to not add this to the body of the text...

> > > > Greg & Michael, I do not want to expose the entire or even partial
> > > > set of device registers to the user space access directly for safety
> > reasons.
> > 
> > But that's all exposed here through this block device, right?
> The block device created by this driver does not expose the device registers to the user space applications.

What is it exposing?

And please use line-wrapping :)

> The device hardware provides separate set of registers to read and write into the OTP memory and EEPROM.
> The driver uses these hardware registers and abstracts the programming logic inside and exposes the only the memory as devices to the user space.

What memory is being exposed?  And how?

> I don't have any user program to program the device. I use the Linux dd command only.
> If I want to view the contents of the memory, I can use any hex editor tool in Linux this way.

Exposing the memory of a device as a block device is not normal, it
should just be mmapped, right?

> > And this is already exposed to userspace today, no need to add anything the
> > kernel already provides this.
> Can you explain this? Are you referring to any sysfs directories / files? What is the necessity to do this? I am trying to understand this.

PCI device accesses can go through userspace directly.  Is this just
memory mapped in your PCI device?

> If this is for any debug purpose and whether kernel does this under some conditional compilation or is that the default behavior?

Is this only for debugging?  If so, please document it as such so that
no one accidentally enables it as a valid build option.

> Even if the user is a super user, should he be allowed to access the device hardware registers mandatorily. It should depend on the policy the system owner want to adopt. Right?

Again, is this PCI memory that can be accessed directly?

And again, a block device is very odd, that is not the normal way to
access a device's memory.

thanks,

greg k-h

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

On Thu, 2023-02-16 at 12:49 +0100, Greg KH wrote:
> > > > > Greg & Michael, I do not want to expose the entire or even
> > > > > partial
> > > > > set of device registers to the user space access directly for
> > > > > safety
> > > reasons.
> > > 
> > > But that's all exposed here through this block device, right?
> > The block device created by this driver does not expose the device
> > registers to the user space applications.
> 
> What is it exposing?
The device's OTP and EEPROM are not directly mapped into the
processor's address space using PCIe's BAR registers.
There is a OTP controller and EEPROM controller in the device and the
registers of these controllers are mapped into the processor's address
space along with other registers using the BAR registers.
OTP/EEPROM driver maps these registers into kernel's virtual space
using devm_ioremap and accomplishes the reads and writes by accessing
these registers. To the user side, the driver shows two separate disks
(one for OTP and one for EEPROM) and both of them could be programmed
using the "linux dd" command with "oflag=direct" option.
The driver handles the IO requests that originate out of the dd command
and this way we would not need a separate user space program also.

> 
> 
> > The device hardware provides separate set of registers to read and
> > write into the OTP memory and EEPROM.
> > The driver uses these hardware registers and abstracts the
> > programming logic inside and exposes the only the memory as devices
> > to the user space.
> 
> What memory is being exposed?  And how?
I think exposing is a wrong word.
OTP and EEPROM memories in the device are enumerated as two separate
block devices.
> 
> > I don't have any user program to program the device. I use the
> > Linux dd command only.
> > If I want to view the contents of the memory, I can use any hex
> > editor tool in Linux this way.
> 
> Exposing the memory of a device as a block device is not normal, it
> should just be mmapped, right?
This PCIe device only maps the OTP controller registers and the EEPROM
controller registers into the address space of the processor and the
actual OTP and EEPROM memories themselves are not mapped. Hence I
enumerated the OTP and EEPROM as block devices and handled the IO for
them in the driver.
> 
> > > And this is already exposed to userspace today, no need to add
> > > anything the
> > > kernel already provides this.
> > Can you explain this? Are you referring to any sysfs directories /
> > files? What is the necessity to do this? I am trying to understand
> > this.
> 
> PCI device accesses can go through userspace directly.  Is this just
> memory mapped in your PCI device?
Our device maps only the OTP controller registers and EEPROM controller
registers into the processor's address space.
> 
> > If this is for any debug purpose and whether kernel does this under
> > some conditional compilation or is that the default behavior?
> 
> Is this only for debugging?  If so, please document it as such so
> that
> no one accidentally enables it as a valid build option.
I thought you are mentioning about /dev/mem and was asking why kernel
maps the memory and device registers into /dev/mem.
> 
> > Even if the user is a super user, should he be allowed to access
> > the device hardware registers mandatorily. It should depend on the
> > policy the system owner want to adopt. Right?
> 
> Again, is this PCI memory that can be accessed directly?
OTP and the EEPROM memories of the device are not directly mapped into
the processor address space and only their controller interface
registers are mapped.
> 
> And again, a block device is very odd, that is not the normal way to
> access a device's memory.
I wanted to use dd command to program and any binary editor to view the
OTP and EEPROM memories in the device and I was able to accomplish that
with this solution.
> 
> Thank You.


Regards,
Kumar

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Fri, Feb 17, 2023 at 08:57:32AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> On Thu, 2023-02-16 at 12:49 +0100, Greg KH wrote:
> > > > > > Greg & Michael, I do not want to expose the entire or even
> > > > > > partial
> > > > > > set of device registers to the user space access directly for
> > > > > > safety
> > > > reasons.
> > > > 
> > > > But that's all exposed here through this block device, right?
> > > The block device created by this driver does not expose the device
> > > registers to the user space applications.
> > 
> > What is it exposing?
> The device's OTP and EEPROM are not directly mapped into the
> processor's address space using PCIe's BAR registers.

Ok, that was not obvious and is a lot of the confusion here.

> There is a OTP controller and EEPROM controller in the device and the
> registers of these controllers are mapped into the processor's address
> space along with other registers using the BAR registers.
> OTP/EEPROM driver maps these registers into kernel's virtual space
> using devm_ioremap and accomplishes the reads and writes by accessing
> these registers. To the user side, the driver shows two separate disks
> (one for OTP and one for EEPROM) and both of them could be programmed
> using the "linux dd" command with "oflag=direct" option.
> The driver handles the IO requests that originate out of the dd command
> and this way we would not need a separate user space program also.

I do not recommend using a block interface for this at all.  Why not the
"normal" EEPROM interface that the kernel has today (i.e. a binary sysfs
file)?  That way you can mmap it and edit locations how ever you want.

thanks,

greg k-h

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Kumaravel.Thiagarajan]

On Fri, 2023-02-17 at 10:22 +0100, Greg KH wrote:
> 
> On Fri, Feb 17, 2023 at 08:57:32AM +0000,
> Kumaravel.Thiagarajan@microchip.com wrote:
> > On Thu, 2023-02-16 at 12:49 +0100, Greg KH wrote:
> > > > > > > Greg & Michael, I do not want to expose the entire or
> > > > > > > even
> > > > > > > partial
> > > > > > > set of device registers to the user space access directly
> > > > > > > for
> > > > > > > safety
> > > > > reasons.
> > > > > 
> > > > > But that's all exposed here through this block device, right?
> > > > The block device created by this driver does not expose the
> > > > device
> > > > registers to the user space applications.
> > > 
> > > What is it exposing?
> > The device's OTP and EEPROM are not directly mapped into the
> > processor's address space using PCIe's BAR registers.
> 
> Ok, that was not obvious and is a lot of the confusion here.
Oh ok, I am sorry if I was not clear.
> 
> > There is a OTP controller and EEPROM controller in the device and
> > the
> > registers of these controllers are mapped into the processor's
> > address
> > space along with other registers using the BAR registers.
> > OTP/EEPROM driver maps these registers into kernel's virtual space
> > using devm_ioremap and accomplishes the reads and writes by
> > accessing
> > these registers. To the user side, the driver shows two separate
> > disks
> > (one for OTP and one for EEPROM) and both of them could be
> > programmed
> > using the "linux dd" command with "oflag=direct" option.
> > The driver handles the IO requests that originate out of the dd
> > command
> > and this way we would not need a separate user space program also.
> 
> I do not recommend using a block interface for this at all.  Why not
> the
> "normal" EEPROM interface that the kernel has today (i.e. a binary
> sysfs
> file)?  That way you can mmap it and edit locations how ever you
> want.
Greg, I have one question about the sysfs interface. If OTP and EEPROM
are enumerated as two sysfs files of 8KB each, will that start
occupying the RAM permamently or is the swapping in and out of the RAM
handled automatically by the kernel based on the user trying to access
it.

Thank You.

Regards,
Kumar

Re: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Mon, Feb 20, 2023 at 09:31:45AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> > I do not recommend using a block interface for this at all.  Why not
> > the
> > "normal" EEPROM interface that the kernel has today (i.e. a binary
> > sysfs
> > file)?  That way you can mmap it and edit locations how ever you
> > want.
> Greg, I have one question about the sysfs interface. If OTP and EEPROM
> are enumerated as two sysfs files of 8KB each, will that start
> occupying the RAM permamently or is the swapping in and out of the RAM
> handled automatically by the kernel based on the user trying to access
> it.

It depends on exactly what you are doing here with the binary sysfs
file.  Try it yourself and see how it works.

thanks,

greg k-h