* Re: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch
[not found] <20230303170426.353-1-vaibhaavram.tl@microchip.com>
@ 2023-03-09 10:55 ` Greg KH
2023-03-13 16:01 ` VaibhaavRam.TL
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2023-03-09 10:55 UTC (permalink / raw)
To: Vaibhaav Ram T.L
Cc: arnd, kumaravel.thiagarajan, linux-kernel, UNGLinuxDriver,
Tharun Kumar P
On Fri, Mar 03, 2023 at 10:34:26PM +0530, Vaibhaav Ram T.L wrote:
> +#define MMAP_EEPROM_OFFSET(x) (EEPROM_REG_ADDR_BASE + x)
> +#define MMAP_CFG_OFFSET(x) (CONFIG_REG_ADDR_BASE + x)
> +#define MMAP_OTP_OFFSET(x) (OTP_REG_ADDR_BASE + x)
No tabs?
> +#define EEPROM_SIZE_BYTES 8192
> +#define OTP_SIZE_BYTES 8192
> +
> +#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 OTP_PWR_DN_OFFSET 0x00
> +#define OTP_ADDR_HIGH_OFFSET 0x04
> +#define OTP_ADDR_LOW_OFFSET 0x08
> +#define OTP_PRGM_DATA_OFFSET 0x10
> +#define OTP_PRGM_MODE_OFFSET 0x14
> +#define OTP_RD_DATA_OFFSET 0x18
> +#define OTP_FUNC_CMD_OFFSET 0x20
> +#define OTP_CMD_GO_OFFSET 0x28
> +#define OTP_PASS_FAIL_OFFSET 0x2C
> +#define OTP_STATUS_OFFSET 0x30
> +
> +#define OTP_PWR_DN_BIT BIT(0)
> +#define OTP_FUNC_RD_BIT BIT(0)
> +#define OTP_FUNC_PGM_BIT BIT(1)
> +#define OTP_CMD_GO_BIT BIT(0)
> +#define OTP_STATUS_BUSY_BIT BIT(0)
> +#define OTP_PGM_MODE_BYTE_BIT BIT(0)
> +#define OTP_FAIL_BIT BIT(0)
> +
> +#define EEPROM_CMD_REG 0x00
> +#define EEPROM_DATA_REG 0x04
> +#define EEPROM_CFG_REG 0x08
> +
> +#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 EEPROM_CMD_EPC_WRITE (BIT(29) | BIT(28))
> +#define EEPROM_CMD_EPC_BUSY_BIT BIT(31)
> +#define EEPROM_CMD_EPC_TIMEOUT_BIT BIT(17)
> +
> +#define STATUS_READ_DELAY_US 1
> +#define STATUS_READ_TIMEOUT_US 20000
> +
> +#define CFG_SYS_LOCK_OFFSET 0xA0
> +#define CFG_SYS_LOCK_PF3 BIT(5)
> +
> +#define BYTE_LOW 0x00FF
> +#define BYTE_HIGH 0x1F00
> +
> +struct pci1xxxx_otp_eeprom_device {
> + struct auxiliary_device *pdev;
> + void __iomem *reg_base;
> + bool is_eeprom_present;
> +};
> +
> +static int set_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> + void __iomem *sys_lock = priv->reg_base +
> + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> + u8 data;
> +
> + writel(CFG_SYS_LOCK_PF3, sys_lock);
> + data = readl(sys_lock);
> + if (data != CFG_SYS_LOCK_PF3)
> + return -EBUSY;
Why not do the dev_err() call here instead of having to do it everywhere
you call it and check for an error?
Also, why tell userspace about this, what can they do about it?
> +
> + return 0;
> +}
> +
> +static int release_sys_lock(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> + void __iomem *sys_lock = priv->reg_base +
> + MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
> + u8 data;
> +
> + data = readl(sys_lock);
> + if (data != CFG_SYS_LOCK_PF3)
> + return 0;
> +
> + writel(0, sys_lock);
> + data = readl(sys_lock);
> + if (data & CFG_SYS_LOCK_PF3)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static void get_pointers_from_kobj(struct kobject *kobj, struct device **p_dev,
> + struct pci1xxxx_otp_eeprom_device **p_priv,
> + void __iomem **p_rb)
> +{
> + *p_dev = container_of(kobj, struct device, kobj);
> + *p_priv = dev_get_drvdata(*p_dev);
> + *p_rb = (*p_priv)->reg_base;
Ick, no, sorry, just open-code this whenever you need it, as sometimes
you do not need all of these, right?
Also, any need to verify that p_priv is not NULL? Can that happen when
the device is removed and the file is still open?
> +}
> +
> +static ssize_t pci1xxxx_eeprom_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + struct pci1xxxx_otp_eeprom_device *priv;
> + struct device *dev;
> + void __iomem *rb;
> + u32 regval;
> + u32 byte;
> + int ret;
> +
> + get_pointers_from_kobj(kobj, &dev, &priv, &rb);
> +
> + ret = set_sys_lock(priv);
> + if (ret) {
> + dev_err(dev, "SYS_LOCK_NOT_SET\n");
> + return ret;
> + }
> +
> + for (byte = 0; byte < count; byte++) {
> + writel(EEPROM_CMD_EPC_BUSY_BIT | (off + byte), rb +
> + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + ret = read_poll_timeout(readl, regval,
> + !(regval & EEPROM_CMD_EPC_BUSY_BIT),
> + STATUS_READ_DELAY_US,
> + STATUS_READ_TIMEOUT_US, true,
> + rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + if (ret < 0 || (!ret && (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)))
> + return -EBUSY;
Shouldn't you return a short read if you do not read the full amount?
That tells userspace they need to resubmit the rest, otherwise they have
no idea how many bytes were successfully read.
> +
> + buf[byte] = readl(rb + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> + }
> +
> + ret = release_sys_lock(priv);
> + if (ret)
> + dev_err(dev, "SYS_LOCK_NOT_RELEASED\n");
No error return value?
> +
> + return count;
> +}
> +
> +
> +static ssize_t pci1xxxx_eeprom_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *value, loff_t off, size_t count)
> +{
> + struct pci1xxxx_otp_eeprom_device *priv;
> + struct device *dev;
> + void __iomem *rb;
> + u32 regval;
> + u32 byte;
> + int ret;
> +
> + get_pointers_from_kobj(kobj, &dev, &priv, &rb);
> +
> + ret = set_sys_lock(priv);
> + if (ret) {
> + dev_err(dev, "SYS_LOCK_NOT_SET\n");
> + return ret;
> + }
> +
> + for (byte = 0; byte < count; byte++) {
> + writel(*(value + byte), rb + MMAP_EEPROM_OFFSET(EEPROM_DATA_REG));
> + regval = EEPROM_CMD_EPC_TIMEOUT_BIT | EEPROM_CMD_EPC_WRITE |
> + (off + byte);
> + writel(regval, rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + writel(EEPROM_CMD_EPC_BUSY_BIT | regval,
> + rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + ret = read_poll_timeout(readl, regval,
> + !(regval & EEPROM_CMD_EPC_BUSY_BIT),
> + STATUS_READ_DELAY_US,
> + STATUS_READ_TIMEOUT_US, true,
> + rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + if (ret < 0 || (!ret && (regval & EEPROM_CMD_EPC_TIMEOUT_BIT)))
> + return -EBUSY;
Same as above, return a short write, otherwise userspace can never
recover properly.
How was this tested and never noticed?
> + }
> +
> + ret = release_sys_lock(priv);
> + if (ret)
> + dev_err(dev, "SYS_LOCK_NOT_RELEASED\n");
Again, no error?
> +
> + return count;
> +}
> +
> +static void otp_device_set_address(struct pci1xxxx_otp_eeprom_device *priv,
> + u16 address)
> +{
> + u16 lo, hi;
> +
> + lo = address & BYTE_LOW;
> + hi = (address & BYTE_HIGH) >> 8;
> + writew(lo, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_LOW_OFFSET));
> + writew(hi, priv->reg_base + MMAP_OTP_OFFSET(OTP_ADDR_HIGH_OFFSET));
> +}
> +
> +static ssize_t pci1xxxx_otp_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *buf, loff_t off, size_t count)
> +{
> + struct pci1xxxx_otp_eeprom_device *priv;
> + struct device *dev;
> + void __iomem *rb;
> + u32 regval;
> + u32 byte;
> + int ret;
> + u8 data;
> +
> + get_pointers_from_kobj(kobj, &dev, &priv, &rb);
> +
> + ret = set_sys_lock(priv);
> + if (ret) {
> + dev_err(dev, "SYS_LOCK_NOT_SET\n");
> + return ret;
> + }
> +
> + for (byte = 0; byte < count; byte++) {
> + otp_device_set_address(priv, (u16)(off + byte));
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + writel(data | OTP_FUNC_RD_BIT,
> + rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> + writel(data | OTP_CMD_GO_BIT,
> + rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> + ret = read_poll_timeout(readl, regval,
> + !(regval & OTP_STATUS_BUSY_BIT),
> + STATUS_READ_DELAY_US,
> + STATUS_READ_TIMEOUT_US, true,
> + rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> + if (ret < 0)
> + return -EBUSY;
Again, short read please.
> +
> + buf[byte] = readl(rb + MMAP_OTP_OFFSET(OTP_RD_DATA_OFFSET));
> + }
> +
> + ret = release_sys_lock(priv);
> + if (ret)
> + dev_err(dev, "SYS_LOCK_NOT_RELEASED\n");
If this can not cause an error, don't tell userspace that an error
happened.
> +
> + return count;
> +}
> +
> +static ssize_t pci1xxxx_otp_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr,
> + char *value, loff_t off, size_t count)
> +{
> + struct pci1xxxx_otp_eeprom_device *priv;
> + struct device *dev;
> + void __iomem *rb;
> + u32 regval;
> + u32 byte;
> + int ret;
> + u8 data;
> +
> + get_pointers_from_kobj(kobj, &dev, &priv, &rb);
> +
> + ret = set_sys_lock(priv);
> + if (ret) {
> + dev_err(dev, "SYS_LOCK_NOT_SET\n");
> + return ret;
> + }
> +
> + for (byte = 0; byte < count; byte++) {
> + otp_device_set_address(priv, (u16)(off + byte));
> +
> + /*
> + * Set OTP_PGM_MODE_BYTE command bit in OTP_PRGM_MODE register
> + * to enable Byte programming
> + */
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> + writel(data | OTP_PGM_MODE_BYTE_BIT,
> + rb + MMAP_OTP_OFFSET(OTP_PRGM_MODE_OFFSET));
> + writel(*(value + byte), rb + MMAP_OTP_OFFSET(OTP_PRGM_DATA_OFFSET));
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + writel(data | OTP_FUNC_PGM_BIT,
> + rb + MMAP_OTP_OFFSET(OTP_FUNC_CMD_OFFSET));
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> + writel(data | OTP_CMD_GO_BIT,
> + rb + MMAP_OTP_OFFSET(OTP_CMD_GO_OFFSET));
> +
> + ret = read_poll_timeout(readl, regval,
> + !(regval & OTP_STATUS_BUSY_BIT),
> + STATUS_READ_DELAY_US,
> + STATUS_READ_TIMEOUT_US, true,
> + rb + MMAP_OTP_OFFSET(OTP_STATUS_OFFSET));
> + if (ret < 0)
> + return -EBUSY;
> +
> + data = readl(rb + MMAP_OTP_OFFSET(OTP_PASS_FAIL_OFFSET));
> + if (data & OTP_FAIL_BIT)
> + return -EFAULT;
Again, short write please.
> + }
> +
> + ret = release_sys_lock(priv);
> + if (ret)
> + dev_err(dev, "SYS_LOCK_NOT_RELEASED\n");
> +
> + return count;
> +}
> +
> +static const struct bin_attribute pci1xxxx_eeprom_attr = {
> + .attr = {
> + .name = EEPROM_NAME,
> + .mode = 0777,
> + },
> + .size = EEPROM_SIZE_BYTES,
> + .read = pci1xxxx_eeprom_read,
> + .write = pci1xxxx_eeprom_write,
> +};
> +
> +static const struct bin_attribute pci1xxxx_otp_attr = {
> + .attr = {
> + .name = OTP_NAME,
> + .mode = 0777,
> + },
> + .size = OTP_SIZE_BYTES,
> + .read = pci1xxxx_otp_read,
> + .write = pci1xxxx_otp_write,
You have new sysfs binary attributes, where are they documented?
> +};
> +
> +
> +static bool is_eeprom_responsive(struct pci1xxxx_otp_eeprom_device *priv)
> +{
> + void __iomem *rb = priv->reg_base;
> + u32 data;
> + int ret;
> +
> + if (set_sys_lock(priv))
> + return false;
> +
> + writel((EEPROM_CFG_PULSE_WIDTH_100KHZ | EEPROM_CFG_SIZE_SEL |
> + EEPROM_CFG_BAUD_RATE_100KHZ),
> + rb + MMAP_EEPROM_OFFSET(EEPROM_CFG_REG));
> +
> + writel(EEPROM_CMD_EPC_TIMEOUT_BIT, rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> + writel(EEPROM_CMD_EPC_BUSY_BIT, rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + /* Wait for the EPC_BUSY bit to get cleared or timeout bit to get set*/
> + ret = read_poll_timeout(readl, data, !(data & EEPROM_CMD_EPC_BUSY_BIT),
> + STATUS_READ_DELAY_US, STATUS_READ_TIMEOUT_US,
> + true, rb + MMAP_EEPROM_OFFSET(EEPROM_CMD_REG));
> +
> + release_sys_lock(priv);
> + /* Return failure if either of software or hardware timeouts happen */
> + if (ret < 0 || (!ret && (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_eeprom_create_device(struct auxiliary_device *aux_dev)
> +{
> + struct auxiliary_device_wrapper *aux_dev_wrapper;
> + struct pci1xxxx_otp_eeprom_device *priv;
> + struct gp_aux_data_type *pdata;
> + int ret;
> + u8 data;
> +
> + aux_dev_wrapper = container_of(aux_dev, struct auxiliary_device_wrapper,
> + aux_dev);
> + pdata = &aux_dev_wrapper->gp_aux_data;
> + if (!pdata)
> + return dev_err_probe(&aux_dev->dev, -EINVAL,
> + "Invalid data in aux_dev_wrapper->gp_aux_data\n");
> +
> + priv = devm_kzalloc(&aux_dev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return dev_err_probe(&aux_dev->dev, -ENOMEM,
> + "Memory Allocation Failed\n");
> +
> + 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))
> + return dev_err_probe(&aux_dev->dev, -ENOMEM,
> + "can't request otpeeprom region\n");
> +
> + 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)
> + return dev_err_probe(&aux_dev->dev, -ENOMEM, "ioremap failed\n");
> +
> + ret = sysfs_create_bin_file(&aux_dev->dev.kobj, &pci1xxxx_otp_attr);
You just raced with userspace and lost. Please never do that, use a
default group instead.
AND you totally ignored the return value here, that's obviously wrong.
> +
> + /* Set OTP_PWR_DN to 0 to make OTP Operational */
> + 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));
> + if (ret) {
> + writel(OTP_PWR_DN_BIT,
> + priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> + return dev_err_probe(&aux_dev->dev, ret,
> + "sysfs_create_bin_file otp failed\n");
> + }
> +
> + if (is_eeprom_responsive(priv)) {
> + ret = sysfs_create_bin_file(&aux_dev->dev.kobj,
> + &pci1xxxx_eeprom_attr);
Again, default group will handle this automatically, you should never
need to call a sysfs_*() call from a driver. Otherwise something is
usually very wrong.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread