[v3,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.

This patch also fixes build issue reported by kernel test robot <lkp@intel.com>
on v1 version of patch.
Following are some of the build issues reported which are fixed in this patch:
    >> ld.lld: error: undefined symbol: blk_mq_alloc_tag_set
    >> ld.lld: error: undefined symbol: blk_mq_init_queue
    >> ld.lld: error: undefined symbol: blk_mq_free_tag_set
    >> ld.lld: error: undefined symbol: blk_mq_start_request
    >> ld.lld: error: undefined symbol: blk_mq_end_request
    >> ld.lld: error: undefined symbol: unregister_blkdev
    >> ld.lld: error: undefined symbol: __register_blkdev
    >> ld.lld: error: undefined symbol: blk_queue_logical_block_size
    >> ld.lld: error: undefined symbol: blk_queue_physical_block_size
    >> ld.lld: error: undefined symbol: __blk_mq_alloc_disk
    >> ld.lld: error: undefined symbol: set_capacity

Reported-by: kernel test robot <lkp@intel.com>
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>
---
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 | 693 ++++++++++++++++++
 4 files changed, 696 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c

On Tue, Jan 17, 2023 at 12:59:07AM +0530, Kumaravel Thiagarajan wrote:
> 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.
> 
> This patch also fixes build issue reported by kernel test robot <lkp@intel.com>
> on v1 version of patch.
> Following are some of the build issues reported which are fixed in this patch:
>     >> ld.lld: error: undefined symbol: blk_mq_alloc_tag_set
>     >> ld.lld: error: undefined symbol: blk_mq_init_queue
>     >> ld.lld: error: undefined symbol: blk_mq_free_tag_set
>     >> ld.lld: error: undefined symbol: blk_mq_start_request
>     >> ld.lld: error: undefined symbol: blk_mq_end_request
>     >> ld.lld: error: undefined symbol: unregister_blkdev
>     >> ld.lld: error: undefined symbol: __register_blkdev
>     >> ld.lld: error: undefined symbol: blk_queue_logical_block_size
>     >> ld.lld: error: undefined symbol: blk_queue_physical_block_size
>     >> ld.lld: error: undefined symbol: __blk_mq_alloc_disk
>     >> ld.lld: error: undefined symbol: set_capacity
> 
> Reported-by: kernel test robot <lkp@intel.com>

Note, the kernel test robot did not report the need for a new driver
here, so it's not really relevant for this changelog (nor is the error
fixes as obviously you would only submit a working driver...)

So when you submit the next version, please remove.

Review comments below:

> 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>
> ---
> 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 | 693 ++++++++++++++++++
>  4 files changed, 696 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9886aa1a4403..195af3ac451d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13614,6 +13614,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..03a537d45873
> --- /dev/null
> +++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_otpe2p.c
> @@ -0,0 +1,693 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 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/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)

Extra tab used here after "define", please don't.

> +#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 MMAP_EEPROM_OFFSET(x)		(EEPROM_REG_ADDR_BASE + (x))
> +
> +#define E2P_CMD_REG			(0x00)
> +#define E2P_DATA_REG			(0x04)
> +#define E2P_CFG_REG			(0x08)
> +#define E2P_PAD_CTL_REG			(0x0C)
> +
> +#define E2P_CMD_EPC_BUSY_BIT		BIT(31)
> +#define E2P_CMD_EPC_TIMEOUT_BIT		BIT(17)
> +#define E2P_CMD_EPC_WRITE		(BIT(29) | BIT(28))
> +
> +#define E2P_CFG_BAUD_RATE_100KHZ	BIT(9)
> +#define E2P_CFG_SIZE_SEL		BIT(12)
> +#define E2P_CFG_PULSE_WIDTH_100KHZ	(BIT(17) | BIT(16))
> +#define OTP_E2P_SECTOR_SIZE		(512)
> +#define OTP_SIZE_IN_BYTES		(8 * 1024)
> +#define E2P_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;

Odd extra space?

> +	struct gendisk *gd;
> +	bool E2P;

What is "E2P"?  Spell it out as a lower case variable.

> +	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_E2P_SECTOR_SIZE;
> +static int E2P_sector_count = E2P_SIZE_IN_BYTES / OTP_E2P_SECTOR_SIZE;

Linux kernel style is all lower case please, "otp_sector_count",
"e2p_sector_count".

> +static int otp_device_count, e2p_device_count;

See, you do that here just fine.

> +static int block_driver_registered;

Do you really need this?  How can that happen that this is ever not
true?

> +static int OTP_block_driver_major;

Again, lower case please.


> +
> +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 = (struct pci1xxxx_otp_e2p_disk *)bdev->bd_disk->private_data;
> +	pdev = (struct auxiliary_device *)disk_priv->pdev;
> +	priv = dev_get_drvdata(&pdev->dev);
> +
> +	mutex_lock(&disk_priv->lock);
> +
> +	do {
> +		retval = set_sys_lock(priv);
> +		if (retval)
> +			break;
> +
> +		if (!disk_priv->E2P) {
> +			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));
> +		}
> +	} while (false);

Why do you have these odd do {} while (false); blocks in the driver?
They are not needed at all, just jump to the exit location please.
That's the normal kernel coding style, not this way.

> +
> +	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 = (struct pci1xxxx_otp_e2p_disk *)disk->private_data;

No need for cast.  You do that in a lot of places.

> +	priv = dev_get_drvdata(&disk_priv->pdev->dev);
> +
> +	mutex_lock(&disk_priv->lock);
> +
> +	if (!disk_priv->E2P) {
> +		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 E2P_DATA_REG register */
> +	writel(value, priv->reg_base + MMAP_EEPROM_OFFSET(E2P_DATA_REG));
> +	data = E2P_CMD_EPC_TIMEOUT_BIT | E2P_CMD_EPC_WRITE | byte_offset;
> +
> +	/* Write the data into E2P_CMD_REG register */
> +	writel(data, priv->reg_base + MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of E2P_CMD_REG register */
> +	writel(E2P_CMD_EPC_BUSY_BIT | data, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +
> +	/* Wait for the EPC_BUSY bit to get cleared */
> +	do {
> +		data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +	} while (data & E2P_CMD_EPC_BUSY_BIT);
> +
> +	if (data & E2P_CMD_EPC_TIMEOUT_BIT) {
> +		dev_err(&priv->pdev->dev, "%s timed out\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	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 E2P_CMD_REG
> +	 * register
> +	 */
> +	writel(byte_offset, priv->reg_base + MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of E2P_CMD_REG register */
> +	writel(E2P_CMD_EPC_BUSY_BIT | byte_offset, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +
> +	/* Wait for the EPC_BUSY bit to get cleared */
> +	do {
> +		regval = readl(priv->reg_base + MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +	} while (regval & E2P_CMD_EPC_BUSY_BIT);
> +
> +	if (regval & E2P_CMD_EPC_TIMEOUT_BIT) {
> +		dev_err(&priv->pdev->dev, "%s timed out\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	/* Read the contents from the E2P_DATA_REG */
> +	*data = readl(priv->reg_base + MMAP_EEPROM_OFFSET(E2P_DATA_REG));
> +	return 0;
> +}
> +
> +static bool check_e2p_response(struct pci1xxxx_otp_e2p_device *priv)

So false is "response is good?"  Or bad?  It's not obvious here at all.

> +{
> +	u32 data;
> +
> +	if (set_sys_lock(priv))
> +		return false;
> +
> +	writel((E2P_CFG_PULSE_WIDTH_100KHZ | E2P_CFG_SIZE_SEL |
> +		E2P_CFG_BAUD_RATE_100KHZ), priv->reg_base +
> +		MMAP_EEPROM_OFFSET(E2P_CFG_REG));
> +
> +	/*
> +	 * Write the byte offset into the EPC_ADDRESS field of E2P_CMD_REG
> +	 * register
> +	 */
> +	writel(E2P_CMD_EPC_TIMEOUT_BIT, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(E2P_CMD_REG));
> +
> +	/* Set the EPC_BUSY bit of E2P_CMD_REG register */
> +	writel(E2P_CMD_EPC_BUSY_BIT, priv->reg_base +
> +	       MMAP_EEPROM_OFFSET(E2P_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(E2P_CMD_REG));
> +	} while (data & E2P_CMD_EPC_BUSY_BIT);
> +
> +	/* If EPC_TIMEOUT is set, then the EEPROM is not responsive */
> +	release_sys_lock(priv);
> +
> +	if (data & E2P_CMD_EPC_TIMEOUT_BIT) {
> +		dev_err(&priv->pdev->dev,
> +			"EPC_Timeout, EEPROM is unresponsive: %x\n", data);

Can this spam the kernel logs?  If so, please rate limit it.

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int otp_device_write_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				 unsigned long byte_offset, u8 value)
> +{
> +	unsigned long j0, j1, delay;
> +	u8 data;
> +
> +	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));
> +	delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> +	j0 = jiffies;
> +	j1 = j0 + delay;

Are you sure this math works out?  Please use the jiffies math functions
instead so you can handle wrapping properly.

> +
> +	/* Wait for the OTP_BUSY bit to get cleared in OTP_STATUS register */
> +	do {
> +		data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +	} while ((data & OTP_STATUS_BUSY_BIT) && (time_before(jiffies, j1)));
> +
> +	if (data & OTP_STATUS_BUSY_BIT) {
> +		dev_err(&priv->pdev->dev, "%s timed out\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	/* 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, "%s write read mismatch 0x%x\n", __func__, data);

No need for any __func__ usage in any print messages please.  dev_dbg()
will give you that for free, and any normal message will not need it as
it is obvious what driver/device it came from.

> +	return -EFAULT;
> +}
> +
> +static int otp_device_read_byte(struct pci1xxxx_otp_e2p_device *priv,
> +				unsigned long byte_offset, u8 *data)
> +{
> +	unsigned long j0, j1, delay;
> +
> +	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));
> +	delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> +	j0 = jiffies;
> +	j1 = j0 + delay;
> +
> +	/* Wait for OTP_BUSY bit to get cleared in OTP_STATUS */
> +	do {
> +		*data = readl(priv->reg_base + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> +	} while ((*data & OTP_STATUS_BUSY_BIT) && (time_before(jiffies, j1)));
> +
> +	if (*data & OTP_STATUS_BUSY_BIT) {
> +		dev_err(&priv->pdev->dev, "%s timed out\n", __func__);
> +		return -EFAULT;
> +	}
> +
> +	/* 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 = (struct pci1xxxx_otp_e2p_disk *)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_E2P_SECTOR_SIZE;
> +			for (j = 0; j < OTP_E2P_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_E2P_SECTOR_SIZE;
> +			for (j = 0; j < OTP_E2P_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 = (struct auxiliary_device_wrapper *) container_of(aux_dev,
> +			  struct auxiliary_device_wrapper, aux_dev);

No need to cast your container_of() call.

> +	pdata = &(aux_dev_wrapper->gp_aux_data);

No need for ()

> +	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;
> +	do {
> +		if (check_e2p_response(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) {
> +			retval = -ENOMEM;
> +			break;
> +		}
> +
> +		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");
> +				break;
> +			}
> +
> +			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_E2P_SECTOR_SIZE);
> +			blk_queue_physical_block_size(priv->otp_e2p_device[i].queue,
> +						      OTP_E2P_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].E2P = 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;
> +			}
> +		}
> +
> +	} while (false);

Again, drop the do {} while (false); block logic as it will simplify
this code (you are already doing gotos...)

> +
> +	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);
> +
> +	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++) {
> +

No blank line needed (yeah, I'm getting picky, sorry.)

> +		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)
> +{
> +	int retval;
> +
> +	retval = otp_e2p_device_create_block_device(aux_dev);
> +	if (retval) {
> +		dev_err(&aux_dev->dev,
> +			"otp/eeprom device enumeration failed with errno = %d\n",
> +			retval);

Why doesn't this function already return an error, now you have 2 error
messages, right?



> +		return retval;
> +	}
> +
> +	return 0;

This whole function can now be one line:
	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;
> +
> +	do {
> +		OTP_block_driver_major = register_blkdev(OTP_block_driver_major,
> +			"OTPBlockDevice");
> +		if (OTP_block_driver_major < 0) {
> +			retval = OTP_block_driver_major;
> +			break;
> +		}
> +
> +		block_driver_registered = 1;
> +
> +		retval = auxiliary_driver_register(&pci1xxxx_otp_e2p_driver);
> +		if (retval)
> +			break;

You just left the block device registered :(

> +
> +	} while (false);

Again, drop the do/while.

And again, the block_driver_registered variable is not needed as your
exit function will not be called if this function fails.

> +
> +	return retval;
> +}
> +
> +static void __exit pci1xxxx_otp_e2p_driver_exit(void)
> +{
> +	auxiliary_driver_unregister(&pci1xxxx_otp_e2p_driver);
> +	if (block_driver_registered)
> +		unregister_blkdev(OTP_block_driver_major, "OTPBlockDevice");

You need to unregister your block device _BEFORE_ the aux device goes
away underneath it :(

thanks,

greg k-h

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, January 20, 2023 4:26 PM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>
> > +             dev_err(&priv->pdev->dev,
> > +                     "EPC_Timeout, EEPROM is unresponsive: %x\n",
> > + data);
> 
> Can this spam the kernel logs?  If so, please rate limit it.

This print will only come once at max and will not spam logs.

> > +     delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> > +     j0 = jiffies;
> > +     j1 = j0 + delay;
> 
> Are you sure this math works out?  Please use the jiffies math functions
> instead so you can handle wrapping properly.

You suggest using any existing APIs to add jiffies to handle wrapping? I am not able 
to find any such API. Can you please point out API name. 

Thanks,
Tharun Kumar P

On Mon, Feb 06, 2023 at 09:20:51AM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Friday, January 20, 2023 4:26 PM
> > To: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> > > +             dev_err(&priv->pdev->dev,
> > > +                     "EPC_Timeout, EEPROM is unresponsive: %x\n",
> > > + data);
> > 
> > Can this spam the kernel logs?  If so, please rate limit it.
> 
> This print will only come once at max and will not spam logs.
> 
> > > +     delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> > > +     j0 = jiffies;
> > > +     j1 = j0 + delay;
> > 
> > Are you sure this math works out?  Please use the jiffies math functions
> > instead so you can handle wrapping properly.
> 
> You suggest using any existing APIs to add jiffies to handle wrapping? I am not able 
> to find any such API. Can you please point out API name. 

I have no context here for what you are doing with the math in jiffies,
but that is usually a very odd thing.  If you use the normal timer
functions, you will not have to worry about the wrapping as it is
handled for you, right?

thanks,

greg k-h

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, February 6, 2023 3:12 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> > > > +     delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> > > > +     j0 = jiffies;
> > > > +     j1 = j0 + delay;
> > >
> > > Are you sure this math works out?  Please use the jiffies math
> > > functions instead so you can handle wrapping properly.
> >
> > You suggest using any existing APIs to add jiffies to handle wrapping?
> > I am not able to find any such API. Can you please point out API name.
> 
> I have no context here for what you are doing with the math in jiffies, but
> that is usually a very odd thing.  If you use the normal timer functions, you
> will not have to worry about the wrapping as it is handled for you, right?

Hi Greg,
Our objective with that part of code is to wait for OTP_STATUS_BUSY_BIT 
to be cleared in the hardware, but make sure that we wait for a maximum 
of OTP_RW_TIMEOUT_MILLISECONDS ms in the driver and not beyond that. 
This logic is not possible with normal timer functions. Timer wrapping logic is 
handled by time_before API internally. So, there does not seem to be issue with
existing code. 

Thanks,
Tharun Kumar P

On Mon, Feb 06, 2023 at 05:03:57PM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Monday, February 6, 2023 3:12 PM
> > To: Tharunkumar Pasumarthi - I67821
> > <Tharunkumar.Pasumarthi@microchip.com>
> > > > > +     delay = msecs_to_jiffies(OTP_RW_TIMEOUT_MILLISECONDS);
> > > > > +     j0 = jiffies;
> > > > > +     j1 = j0 + delay;
> > > >
> > > > Are you sure this math works out?  Please use the jiffies math
> > > > functions instead so you can handle wrapping properly.
> > >
> > > You suggest using any existing APIs to add jiffies to handle wrapping?
> > > I am not able to find any such API. Can you please point out API name.
> > 
> > I have no context here for what you are doing with the math in jiffies, but
> > that is usually a very odd thing.  If you use the normal timer functions, you
> > will not have to worry about the wrapping as it is handled for you, right?
> 
> Hi Greg,
> Our objective with that part of code is to wait for OTP_STATUS_BUSY_BIT 
> to be cleared in the hardware, but make sure that we wait for a maximum 
> of OTP_RW_TIMEOUT_MILLISECONDS ms in the driver and not beyond that. 

How exact much do you mean by "not beyond that"?

> This logic is not possible with normal timer functions. Timer wrapping logic is 
> handled by time_before API internally. So, there does not seem to be issue with
> existing code. 

I have no context here as to what the code actually looked like anymore,
so I do not know what is going on anymore, sorry.

greg k-h

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Monday, February 6, 2023 11:19 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@microchip.com>
> Subject: Re: [PATCH v3 char-misc-next] misc: microchip: pci1xxxx: Add
> OTP/EEPROM driver for the pci1xxxx switch
>  
> > This logic is not possible with normal timer functions. Timer wrapping
> > logic is handled by time_before API internally. So, there does not
> > seem to be issue with existing code.
> 
> I have no context here as to what the code actually looked like anymore, so I
> do not know what is going on anymore, sorry.

Greg, I addressed all the comments received for V2 version of the patch (except for the one in discussion). 
Shall I send V3 patch so that you will be able to get back previous context?

Thanks,
Tharun Kumar P

> From: Tharunkumar Pasumarthi - I67821
> Sent: Tuesday, February 7, 2023 3:22 PM
> To: Greg KH <gregkh@linuxfoundation.org>
> Subject: RE: [PATCH v3 char-misc-next] misc: microchip: pci1xxxx: Add
> OTP/EEPROM driver for the pci1xxxx switch
> 
> >
> > I have no context here as to what the code actually looked like
> > anymore, so I do not know what is going on anymore, sorry.
> 
> Greg, I addressed all the comments received for V2 version of the patch
> (except for the one in discussion).
> Shall I send V3 patch so that you will be able to get back previous context?

Sorry, I meant to ask if I could send V4 patch (not V3) addressing V3 comments.

Thanks,
Tharun Kumar P

>> Our objective with that part of code is to wait for OTP_STATUS_BUSY_BIT 
>> to be cleared in the hardware, but make sure that we wait for a maximum 
>> of OTP_RW_TIMEOUT_MILLISECONDS ms in the driver and not beyond that. 
>
> How exact much do you mean by "not beyond that"?

Yeah that sounds strange. What is the reason to not wait "beyond that"?

Reading that paragraph above, it looks like you should have a look at
linux/iopoll.h.

-michael

> From: Michael Walle <michael@walle.cc>
> Sent: Tuesday, February 7, 2023 7:15 PM
> To: gregkh@linuxfoundation.org
> Subject: Re: [PATCH v3 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 
> Yeah that sounds strange. What is the reason to not wait "beyond that"?
> 
> Reading that paragraph above, it looks like you should have a look at
> linux/iopoll.h.

Thanks for your inputs, Michael. I will use 'read_poll_timeout' function in linux/iopoll.h.

Thanks,
Tharun Kumar P

On Tue, Feb 07, 2023 at 10:34:47AM +0000, Tharunkumar.Pasumarthi@microchip.com wrote:
> > From: Tharunkumar Pasumarthi - I67821
> > Sent: Tuesday, February 7, 2023 3:22 PM
> > To: Greg KH <gregkh@linuxfoundation.org>
> > Subject: RE: [PATCH v3 char-misc-next] misc: microchip: pci1xxxx: Add
> > OTP/EEPROM driver for the pci1xxxx switch
> > 
> > >
> > > I have no context here as to what the code actually looked like
> > > anymore, so I do not know what is going on anymore, sorry.
> > 
> > Greg, I addressed all the comments received for V2 version of the patch
> > (except for the one in discussion).
> > Shall I send V3 patch so that you will be able to get back previous context?
> 
> Sorry, I meant to ask if I could send V4 patch (not V3) addressing V3 comments.

You never need to ask if you can send a new patch series.  It is
implied, always do so if changes have been made.

thanks,

greg k-h