[linux-3.16 PATCH 0/3] Add efuse driver for jz4780

[Zubair Lutfullah Kakakhel]

The driver has been forward ported from 3.0.8 after significant cleanup.

Read works.
CI20 is not populated with components to check writes.

The JZ4780 has an 8K one time programmable efuse in it.

Read/Writes are indirect using address,read/write strobes etc.

Simple device driver. Put in drivers/misc/efuse because there is
no formal location for efuse yet.

Upstream had drivers/misc/efuse for some Tegra efuse patches. But
they didn't make it.

Zubair Lutfullah Kakakhel (3):
dt: misc: Add binding documentation for Ingenic JZ4780 Efuse driver
misc: efuse: Add jz4780 efuse driver
mips: jz4780: Add DT node for efuse driver

.../bindings/misc/ingenic,jz4780-efuse.txt | 20 +
arch/mips/jz4780/dts/jz4780.dtsi | 10 +
drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/efuse/Kconfig | 17 +
drivers/misc/efuse/Makefile | 1 +
drivers/misc/efuse/jz4780_efuse.c | 685 +++++++++++++++++++++
drivers/misc/efuse/jz4780_efuse.h | 81 +++
8 files changed, 816 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/ingenic,jz4780-efuse.txt
create mode 100644 drivers/misc/efuse/Kconfig
create mode 100644 drivers/misc/efuse/Makefile
create mode 100644 drivers/misc/efuse/jz4780_efuse.c
create mode 100644 drivers/misc/efuse/jz4780_efuse.h

--
1.9.1

[Zubair Lutfullah Kakakhel]

The JZ4780 has an 8K one time programmable efuse.

Simple self-explanatory bindings.

vddq-gpio is needed. It is a pin on the SoC. Must be held high if
programming bits in the efuse. A gpio from the SoC is looped
and connected to this pin for programming.

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair....@imgtec.com>
---
.../bindings/misc/ingenic,jz4780-efuse.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/ingenic,jz4780-efuse.txt

diff --git a/Documentation/devicetree/bindings/misc/ingenic,jz4780-efuse.txt b/Documentation/devicetree/bindings/misc/ingenic,jz4780-efuse.txt
new file mode 100644
index 0000000..ee69286
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/ingenic,jz4780-efuse.txt
@@ -0,0 +1,20 @@
+Ingenic JZ4780 EFUSE driver bindings
+
+Required properties:
+- "compatible" Must be set to "ingenic,jz4780-efuse"
+- "reg" Register location and length
+- "vddq-gpio" gpio handle for VDDQ gpio. Used for programming the efuse
+- "clocks" Handle for the ahb clock for the efuse.
+- "clock-names" Must be "ahb2"
+
+Example:
+
+efuse: efuse@134100D0 {
+ compatible = "ingenic,jz4780-efuse";
+ reg = <0x134100D0 0xFF>;
+
+ vddq-gpio = <&gpe 4 GPIO_ACTIVE_LOW>;
+
+ clocks = <&cgu JZ4780_CLK_AHB2>;
+ clock-names = "ahb2";
+};
--
1.9.1

[Zubair Lutfullah Kakakhel]

The jz4780 has an 8K efuse one time programmable rom.

The driver supports read/write. Reading the chip and user id
is possible via sysfs.

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair....@imgtec.com>
---

arch/mips/jz4780/dts/jz4780.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/arch/mips/jz4780/dts/jz4780.dtsi b/arch/mips/jz4780/dts/jz4780.dtsi
index 44b219d..e635275 100644
--- a/arch/mips/jz4780/dts/jz4780.dtsi
+++ b/arch/mips/jz4780/dts/jz4780.dtsi
@@ -517,6 +517,16 @@
clocks = <&cgu JZ4780_CLK_NEMC>;
};

+ efuse: efuse@134100D0 {

+ compatible = "ingenic,jz4780-efuse";
+ reg = <0x134100D0 0xFF>;
+
+ vddq-gpio = <&gpe 4 GPIO_ACTIVE_LOW>;
+
+ clocks = <&cgu JZ4780_CLK_AHB2>;
+ clock-names = "ahb2";
+ };

+
dma: dma@13420000 {
compatible = "ingenic,jz4780-dma";
reg = <0x13420000 0x10000>;
--
1.9.1

[Zubair Lutfullah Kakakhel]

This series adds an efuse driver to read/write from the JZ4780 EFUSE.

The JZ4780 EFUSE is a one time programmable efuse inside the SoC.

It is used to store the customer id, serial number, mac address,
hdcp keys, etc.

This driver allows to read/write to the efuse in the JZ4780 SoC.

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair....@imgtec.com>
---

drivers/misc/Kconfig | 1 +
drivers/misc/Makefile | 1 +
drivers/misc/efuse/Kconfig | 17 +
drivers/misc/efuse/Makefile | 1 +

drivers/misc/efuse/jz4780_efuse.c | 685 ++++++++++++++++++++++++++++++++++++++
drivers/misc/efuse/jz4780_efuse.h | 81 +++++
6 files changed, 786 insertions(+)

create mode 100644 drivers/misc/efuse/Kconfig
create mode 100644 drivers/misc/efuse/Makefile
create mode 100644 drivers/misc/efuse/jz4780_efuse.c
create mode 100644 drivers/misc/efuse/jz4780_efuse.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index ee94023..b3e1fb7 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -526,6 +526,7 @@ config VEXPRESS_SYSCFG
of generating transactions on this bus.

source "drivers/misc/c2port/Kconfig"
+source "drivers/misc/efuse/Kconfig"
source "drivers/misc/eeprom/Kconfig"
source "drivers/misc/cb710/Kconfig"
source "drivers/misc/ti-st/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d59ce12..a142b2a 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_DS1682) += ds1682.o
obj-$(CONFIG_TI_DAC7512) += ti_dac7512.o
obj-$(CONFIG_C2PORT) += c2port/
obj-$(CONFIG_HMC6352) += hmc6352.o
+obj-y += efuse/
obj-y += eeprom/
obj-y += cb710/
obj-$(CONFIG_SPEAR13XX_PCIE_GADGET) += spear13xx_pcie_gadget.o
diff --git a/drivers/misc/efuse/Kconfig b/drivers/misc/efuse/Kconfig
new file mode 100644
index 0000000..3700682
--- /dev/null
+++ b/drivers/misc/efuse/Kconfig
@@ -0,0 +1,17 @@
+#
+# Misc efuse devices. generally found in SoCs. Used to store mac addresses,
+# encryption keys, etc.
+#
+
+menu "Efuse devices"
+
+config JZ4780_EFUSE
+ bool "Ingenic JZ4780 EFUSE driver"
+ depends on MACH_JZ4780
+ default n
+ help
+ Say Y here if you want the efuse driver for the Ingenic JZ4780 SoC.
+
+
+endmenu
+
diff --git a/drivers/misc/efuse/Makefile b/drivers/misc/efuse/Makefile
new file mode 100644
index 0000000..fcc3c75
--- /dev/null
+++ b/drivers/misc/efuse/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_JZ4780_EFUSE) += jz4780_efuse.o
diff --git a/drivers/misc/efuse/jz4780_efuse.c b/drivers/misc/efuse/jz4780_efuse.c
new file mode 100644
index 0000000..cf4fef8
--- /dev/null
+++ b/drivers/misc/efuse/jz4780_efuse.c
@@ -0,0 +1,685 @@
+/*
+ * jz4780_efuse.c - Driver to read/write from the JZ4780 one time programmable
+ * efuse 8K memory
+ *
+ * The rom itself is accessed using a 9 bit address line and an 8 word wide bus
+ * which reads/writes based on strobes. The strobe is configured in the config
+ * register and is based on number of cycles of the AHB2 clock.
+ *
+ * Copyright (C) 2014 Imagination Technologies
+ *
+ * Based on work by Ingenic Semiconductor.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/kernel.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/miscdevice.h>
+#include <linux/sched.h>
+#include <linux/ctype.h>
+#include <linux/fs.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/spinlock.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/delay.h>
+
+#include "jz4780_efuse.h"
+
+void dump_jz_efuse(struct jz_efuse *efuse)
+{
+ dev_info(efuse->dev, "max_program_length = %x\n",
+ efuse->max_program_length);
+ dev_info(efuse->dev, "use_count = %x\n", efuse->use_count);
+ dev_info(efuse->dev, "is_timer_on = %x\n", efuse->is_timer_on);
+ dev_info(efuse->dev, "gpio_vddq_en_n = %x\n", efuse->gpio_vddq_en_n);
+
+ dev_info(efuse->dev, "rd_adj = %x\n", efuse->efucfg_info.rd_adj);
+ dev_info(efuse->dev, "rd_strobe = %x\n", efuse->efucfg_info.rd_strobe);
+ dev_info(efuse->dev, "wr_adj = %x\n", efuse->efucfg_info.wr_adj);
+ dev_info(efuse->dev, "wr_strobe = %x\n", efuse->efucfg_info.wr_strobe);
+
+ dev_info(efuse->dev, "min_rd_adj = %x\n",
+ efuse->efucfg_info.strict.min_rd_adj);
+ dev_info(efuse->dev, "min_rd_adj_strobe = %x\n",
+ efuse->efucfg_info.strict.min_rd_adj_strobe);
+ dev_info(efuse->dev, "min_wr_adj = %x\n",
+ efuse->efucfg_info.strict.min_wr_adj);
+ dev_info(efuse->dev, "min_wr_adj_strobe = %x\n",
+ efuse->efucfg_info.strict.min_wr_adj_strobe);
+ dev_info(efuse->dev, "max_wr_adj_strobe = %x\n",
+ efuse->efucfg_info.strict.max_wr_adj_strobe);
+}
+
+static void jz_efuse_vddq_set(unsigned long efuse_ptr)
+{
+ struct jz_efuse *efuse = (struct jz_efuse *)efuse_ptr;
+
+ dev_info(efuse->dev, "JZ4780-EFUSE: vddq_set %d\n",
+ (int)efuse->is_timer_on);
+
+ if (efuse->is_timer_on)
+ mod_timer(&efuse->vddq_protect_timer, jiffies + HZ);
+
+ gpio_set_value(efuse->gpio_vddq_en_n, !efuse->is_timer_on);
+}
+
+static inline int jz_efuse_get_skip(size_t size)
+{
+ if (size >= 32)
+ return 32;
+ else if ((size / 4) > 0)
+ return (size / 4) * 4;
+ else
+ return size % 4;
+}
+
+static int calculate_efuse_strict(struct jz_efuse *efuse,
+ struct jz_efuse_strict *t, unsigned long rate)
+{
+ unsigned int tmp;
+
+ /* The EFUSE read/write strobes need to be within these values for it
+ * to function properly. Based on programmers manual.
+ */
+
+ tmp = (((6500 * (rate / 1000000)) / 1000000) + 1) - 1;
+ if (tmp > 0xf) {
+ dev_err(efuse->dev, "Cannot calculate min RD_ADJ\n");
+ return -EINVAL;
+ } else {
+ t->min_rd_adj = tmp;
+ }
+
+ tmp = ((((35000 * (rate / 1000000)) / 1000000) + 1) - 5);
+ if (tmp > (0xf + 0xf)) {
+ dev_err(efuse->dev, "Cannot calculate min RD_STROBE\n");
+ return -EINVAL;
+ } else {
+ t->min_rd_adj_strobe = tmp;
+ }
+
+ tmp = (((6500 * (rate / 1000000)) / 1000000) + 1) - 1;
+ if (tmp > 0xf) {
+ dev_err(efuse->dev, "Cannot calculate min WR_ADJ\n");
+ return -EINVAL;
+ } else {
+ t->min_wr_adj = tmp;
+ }
+
+ tmp = ((((9000000 / 1000000) * (rate / 1000000))) + 1) - 1666;
+ if (tmp > (0xfff + 0xf)) {
+ dev_err(efuse->dev, "Cannot calculate min WR_STROBE\n");
+ return -EINVAL;
+ } else {
+ t->min_wr_adj_strobe = tmp;
+ }
+
+ tmp = ((((11000000 / 1000000) * (rate / 1000000))) + 1) - 1666;
+ if (tmp > (0xfff + 0xf)) {
+ dev_err(efuse->dev, "Cannot calculate max WR_STROBE\n");
+ return -EINVAL;
+ } else {
+ t->max_wr_adj_strobe = tmp;
+ }
+
+ return 0;
+}
+
+static int jz_init_efuse_cfginfo(struct jz_efuse *efuse, unsigned long clk_rate)
+{
+ struct jz_efucfg_info *info = &efuse->efucfg_info;
+ struct jz_efuse_strict *s = &info->strict;
+ unsigned int tmp = 0;
+
+ if (calculate_efuse_strict(efuse, s, clk_rate) < 0) {
+ dev_err(efuse->dev, "can't calculate strobe parameters\n");
+ return -EINVAL;
+ }
+
+ info->rd_adj = (s->min_rd_adj + 0xf) / 2;
+ tmp = s->min_rd_adj_strobe - info->rd_adj;
+ info->rd_strobe = ((tmp + 0xf) / 2 < 7) ? 7 : (tmp + 0xf) / 2;
+ if (info->rd_strobe > 0xf) {
+ dev_err(efuse->dev, "can't calculate read strobe\n");
+ return -EINVAL;
+ }
+
+ tmp = (s->min_wr_adj_strobe + s->max_wr_adj_strobe) / 2;
+ info->wr_adj = tmp < 0xf ? tmp : 0xf;
+ info->wr_strobe = tmp - info->wr_adj;
+ if (info->wr_strobe > 0xfff) {
+ dev_err(efuse->dev, "can't calculate write strobe\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int jz_efuse_open(struct inode *inode, struct file *filp)
+{
+ struct miscdevice *dev = filp->private_data;
+ struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
+
+ spin_lock(&efuse->lock);
+ efuse->use_count++;
+ spin_unlock(&efuse->lock);
+
+ return 0;
+}
+
+static int jz_efuse_release(struct inode *inode, struct file *filp)
+{
+ struct miscdevice *dev = filp->private_data;
+ struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
+
+ spin_lock(&efuse->lock);
+ efuse->use_count--;
+ spin_unlock(&efuse->lock);
+
+ return 0;
+}
+
+static ssize_t _jz_efuse_read_bytes(struct jz_efuse *efuse, char *buf,
+ unsigned int addr, int skip)
+{
+ unsigned int tmp = 0;
+ int i = 0;
+ unsigned long flag;
+ int timeout = 1000;
+
+ /* 1. Set config register */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCFG);
+ tmp &= ~((JZ_EFUSE_EFUCFG_RD_ADJ_MASK << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
+ | (JZ_EFUSE_EFUCFG_RD_STR_MASK << JZ_EFUSE_EFUCFG_RD_STR_SHIFT));
+ tmp |= (efuse->efucfg_info.rd_adj << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
+ | (efuse->efucfg_info.rd_strobe << JZ_EFUSE_EFUCFG_RD_STR_SHIFT);
+ writel(tmp, efuse->iomem + JZ_EFUCFG);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /*
+ * 2. Set control register to indicate what to read data address,
+ * read data numbers and read enable.
+ */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCTRL);
+ tmp &= ~(JZ_EFUSE_EFUCFG_RD_STR_SHIFT
+ | (JZ_EFUSE_EFUCTRL_ADDR_MASK << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
+ | JZ_EFUSE_EFUCTRL_PG_EN | JZ_EFUSE_EFUCTRL_WR_EN
+ | JZ_EFUSE_EFUCTRL_WR_EN);
+
+ if (addr >= (JZ_EFUSE_START_ADDR + 0x200))
+ tmp |= JZ_EFUSE_EFUCTRL_CS;
+
+ tmp |= (addr << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
+ | ((skip - 1) << JZ_EFUSE_EFUCTRL_LEN_SHIFT)
+ | JZ_EFUSE_EFUCTRL_RD_EN;
+ writel(tmp, efuse->iomem + JZ_EFUCTRL);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /*
+ * 3. Wait status register RD_DONE set to 1 or EFUSE interrupted,
+ * software can read EFUSE data buffer 0 – 8 registers.
+ */
+ do {
+ tmp = readl(efuse->iomem + JZ_EFUSTATE);
+ usleep_range(100, 200);
+ if (timeout--)
+ break;
+ } while (!(tmp & JZ_EFUSE_EFUSTATE_RD_DONE));
+
+ if (timeout == 0) {
+ dev_err(efuse->dev, "Timed out while reading\n");
+ return -EAGAIN;
+ }
+
+ if ((skip % 4) == 0) {
+ for (i = 0; i < (skip / 4); i++) {
+ *((unsigned int *)(buf + i * 4))
+ = readl(efuse->iomem + JZ_EFUDATA(i));
+ }
+ } else {
+ *((unsigned int *)buf)
+ = readl(efuse->iomem + JZ_EFUDATA(0)) & BYTEMASK(skip);
+ }
+
+ return 0;
+}
+
+static ssize_t _jz_efuse_read(struct jz_efuse *efuse, char *buf,
+ size_t size, loff_t *l)
+{
+ int bytes = 0;
+ unsigned int addr = 0, start = *l + JZ_EFUSE_START_ADDR;
+ int skip = jz_efuse_get_skip(size - bytes);
+ int org_skip = 0;
+
+ for (addr = start; addr < start + size; addr += org_skip) {
+ if (_jz_efuse_read_bytes(efuse, buf + bytes, addr,
+ skip) < 0) {
+ dev_err(efuse->dev, "Can't read addr=%x\n", addr);
+ return -1;
+ }
+
+ *l += skip;
+ bytes += skip;
+
+ org_skip = skip;
+ skip = jz_efuse_get_skip(size - bytes);
+ }
+
+ return bytes;
+}
+
+
+static ssize_t jz_efuse_read(struct file *filp, char *buf, size_t size,
+ loff_t *lpos)
+{
+ struct miscdevice *dev = filp->private_data;
+ struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
+ int ret = -1;
+ char *tmp_buf = NULL;
+
+ if (size > (JZ_EFUSE_END_ADDR - (JZ_EFUSE_START_ADDR + *lpos) + 1)) {
+ dev_err(efuse->dev, "Trying to read beyond efuse\n");
+ return -EINVAL;
+ }
+
+ tmp_buf = devm_kzalloc(efuse->dev, size, GFP_KERNEL);
+ if (!tmp_buf)
+ ret = -ENOMEM;
+
+ ret = _jz_efuse_read(efuse, tmp_buf, size, lpos);
+ if (ret < 0) {
+ dev_err(efuse->dev, "Could not read efuse\n");
+ return -1;
+ }
+
+ copy_to_user(buf, tmp_buf, ret);
+
+ return ret;
+}
+
+static int is_space_written(const char *tmp, const char *buf, int skip)
+{
+ int i = 0;
+
+ for (i = 0; i < skip; i++)
+ if ((tmp[i] & buf[i]) > 0)
+ return 1;
+
+ return 0;
+}
+
+static ssize_t _jz_efuse_write_bytes(struct jz_efuse *efuse,
+ const char *buf, unsigned int addr, int skip)
+{
+ unsigned int tmp_buf[8];
+ unsigned int tmp = 0;
+ unsigned long flag = 0;
+ int i = 0;
+ int timeout = 1000;
+
+ if (_jz_efuse_read_bytes(efuse, (char *)&tmp_buf, addr, skip) < 0) {
+ dev_err(efuse->dev, "read efuse at addr = %x failed\n", addr);
+ return -EINVAL;
+ }
+
+ if (is_space_written((char *)tmp_buf, (char *)buf, skip)) {
+ dev_err(efuse->dev,
+ "ERROR: the write spaced has already been written\n");
+ return -EINVAL;
+ }
+
+ /* 1. Set config register */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCFG);
+ tmp &= ~((JZ_EFUSE_EFUCFG_WR_ADJ_MASK << JZ_EFUSE_EFUCFG_WR_ADJ_SHIFT)
+ | (JZ_EFUSE_EFUCFG_WR_STR_MASK << JZ_EFUSE_EFUCFG_WR_STR_SHIFT));
+ tmp |= (efuse->efucfg_info.wr_adj << JZ_EFUSE_EFUCFG_WR_ADJ_SHIFT)
+ | (efuse->efucfg_info.wr_strobe << JZ_EFUSE_EFUCFG_WR_STR_SHIFT);
+ writel(tmp, efuse->iomem + JZ_EFUCFG);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 2. Write want program data to EFUSE data buffer 0-7 registers */
+ if (skip % 4 == 0) {
+ for (i = 0; i < skip / 4; i++) {
+ writel(*((unsigned int *)(buf + i * 4)),
+ efuse->iomem + JZ_EFUDATA(i));
+ }
+ } else {
+ writel(*((unsigned int *)buf) & BYTEMASK(skip),
+ efuse->iomem + JZ_EFUDATA(0));
+ }
+
+ /*
+ * 3. Set control register, indicate want to program address,
+ * data length.
+ */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCTRL);
+ tmp &= ~((JZ_EFUSE_EFUCTRL_CS)
+ | (JZ_EFUSE_EFUCTRL_ADDR_MASK << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
+ | (JZ_EFUSE_EFUCTRL_PG_EN)
+ | JZ_EFUSE_EFUCTRL_WR_EN | JZ_EFUSE_EFUCTRL_RD_EN);
+
+ if (addr >= (JZ_EFUSE_START_ADDR + 0x200))
+ tmp |= JZ_EFUSE_EFUCTRL_CS;
+
+ tmp |= (addr << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
+ | ((skip - 1) << JZ_EFUSE_EFUCTRL_LEN_SHIFT);
+ writel(tmp, efuse->iomem + JZ_EFUCTRL);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 4. Write control register PG_EN bit to 1 */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCTRL);
+ tmp |= JZ_EFUSE_EFUCTRL_PG_EN;
+ writel(tmp, efuse->iomem + JZ_EFUCTRL);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 5. Connect VDDQ pin to 2.5V */
+ spin_lock_irqsave(&efuse->lock, flag);
+ efuse->is_timer_on = 1;
+ jz_efuse_vddq_set((unsigned long)efuse);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 6. Write control register WR_EN bit */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCTRL);
+ tmp |= JZ_EFUSE_EFUCTRL_WR_EN;
+ writel(tmp, efuse->iomem + JZ_EFUCTRL);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 7. Wait status register WR_DONE set to 1. */
+ do {
+ tmp = readl(efuse->iomem + JZ_EFUSTATE);
+ usleep_range(100, 200);
+ if (timeout--)
+ break;
+ } while (!(tmp & JZ_EFUSE_EFUSTATE_WR_DONE));
+
+ /* 8. Disconnect VDDQ pin from 2.5V. */
+ spin_lock_irqsave(&efuse->lock, flag);
+ efuse->is_timer_on = 0;
+ jz_efuse_vddq_set((unsigned long)efuse);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ /* 9. Write control register PG_EN bit to 0. */
+ spin_lock_irqsave(&efuse->lock, flag);
+ tmp = readl(efuse->iomem + JZ_EFUCTRL);
+ tmp &= ~JZ_EFUSE_EFUCTRL_PG_EN;
+ writel(tmp, efuse->iomem + JZ_EFUCTRL);
+ spin_unlock_irqrestore(&efuse->lock, flag);
+
+ if (timeout == 0) {
+ dev_err(efuse->dev, "Timed out while reading\n");
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
+static ssize_t _jz_efuse_write(struct jz_efuse *efuse, const char *buf,
+ size_t size, loff_t *l)
+{
+ int bytes = 0;
+ unsigned int addr = 0, start = *l + JZ_EFUSE_START_ADDR;
+ int skip = jz_efuse_get_skip(size - bytes);
+ int org_skip = 0;
+
+ for (addr = start; addr < (start + size); addr += org_skip) {
+ if (_jz_efuse_write_bytes(efuse, buf + bytes, addr,
+ skip) < 0) {
+ dev_err(efuse->dev,
+ "error to write efuse byte at addr=%x\n", addr);
+ return -1;
+ }
+
+ *l += skip;
+ bytes += skip;
+
+ org_skip = skip;
+ skip = jz_efuse_get_skip(size - bytes);
+ }
+
+ return bytes;
+}
+
+static ssize_t jz_efuse_write(struct file *filp, const char *buf, size_t size,
+ loff_t *lpos)
+{
+ int ret = -1;
+ char *tmp_buf = NULL;
+ struct miscdevice *dev = filp->private_data;
+ struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
+
+ if (size > (JZ_EFUSE_END_ADDR - (JZ_EFUSE_START_ADDR + *lpos) + 1)) {
+ dev_err(efuse->dev, "Trying to write beyond efuse\n");
+ return -EINVAL;
+ }
+
+ tmp_buf = devm_kzalloc(efuse->dev, size, GFP_KERNEL);
+ if (!tmp_buf)
+ return -ENOMEM;
+
+ copy_from_user(tmp_buf, buf, size);
+
+ ret = _jz_efuse_write(efuse, tmp_buf, size, lpos);
+ if (ret < 0) {
+ dev_err(efuse->dev, "Could not write efuse\n");
+ return -1;
+ }
+
+ return ret;
+}
+
+static const struct file_operations efuse_misc_fops = {
+ .open = jz_efuse_open,
+ .release = jz_efuse_release,
+ .llseek = default_llseek,
+ .read = jz_efuse_read,
+ .write = jz_efuse_write,
+};
+
+static ssize_t jz_efuse_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf, loff_t lpos)
+{
+ struct jz_efuse *efuse = dev_get_drvdata(dev);
+ unsigned int data[16];
+ char *tmp_buf = (char *) &data[0];
+ int ret = -1;
+
+ ret = _jz_efuse_read(efuse, tmp_buf, 16, &lpos);
+ if (ret < 0) {
+ dev_err(dev, "Cannot read efuse\n");
+ return -EINVAL;
+ }
+
+ return snprintf(buf, PAGE_SIZE, "%08x %08x %08x %08x\n",
+ data[0], data[1], data[2], data[3]);
+}
+
+static ssize_t jz_efuse_id_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count,
+ loff_t lpos)
+{
+ struct jz_efuse *efuse = dev_get_drvdata(dev);
+ unsigned int data[16];
+ char *tmp_buf = (char *) &data[0];
+ int ret = -1;
+
+ ret = sscanf(buf, "%08x %08x %08x %08x",
+ &data[0], &data[1], &data[2], &data[3]);
+
+ ret = _jz_efuse_write(efuse, tmp_buf, 16, &lpos);
+ if (ret < 0) {
+ dev_err(dev, "Could not write to efuse\n");
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static ssize_t jz_efuse_chip_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return jz_efuse_id_show(dev, attr, buf, JZ_EFUSE_SEG2_OFF);
+}
+
+static ssize_t jz_efuse_chip_id_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ return jz_efuse_id_store(dev, attr, buf, count, JZ_EFUSE_SEG2_OFF);
+}
+
+static ssize_t jz_efuse_user_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return jz_efuse_id_show(dev, attr, buf, JZ_EFUSE_SEG3_OFF);
+}
+
+static ssize_t jz_efuse_user_id_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ return jz_efuse_id_store(dev, attr, buf, count, JZ_EFUSE_SEG3_OFF);
+}
+
+static struct device_attribute jz_efuse_sysfs_attrs[] = {
+ __ATTR(chip_id, S_IRUGO | S_IWUSR, jz_efuse_chip_id_show,
+ jz_efuse_chip_id_store),
+ __ATTR(user_id, S_IRUGO | S_IWUSR, jz_efuse_user_id_show,
+ jz_efuse_user_id_store),
+};
+
+static struct of_device_id jz_efuse_of_match[] = {
+{ .compatible = "ingenic,jz4780-efuse", },
+ { },
+};
+
+static int jz_efuse_probe(struct platform_device *pdev)
+{
+ int ret, i;
+ struct resource *regs;
+ struct jz_efuse *efuse = NULL;
+ unsigned long clk_rate;
+ struct device *dev = &pdev->dev;
+ enum of_gpio_flags flags;
+
+ efuse = devm_kzalloc(dev, sizeof(struct jz_efuse), GFP_KERNEL);
+ if (!efuse)
+ return -ENOMEM;
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ efuse->iomem = devm_ioremap(dev, regs->start, resource_size(regs));
+ if (IS_ERR(efuse->iomem))
+ return PTR_ERR(efuse->iomem);
+
+ efuse->clk = devm_clk_get(dev, "ahb2");
+ if (IS_ERR(efuse->clk))
+ return PTR_ERR(efuse->clk);
+
+ clk_rate = clk_get_rate(efuse->clk);
+ /* Based on maximum time for read/write strobe of 11us */
+ if ((clk_rate < (185 * 1000000LL)) || (clk_rate > (512 * 1000000LL))) {
+ dev_err(dev, "clock rate not between 185M-512M\n");
+ return -1;
+ }
+
+ if (jz_init_efuse_cfginfo(efuse, clk_rate) < 0) {
+ dev_err(dev, "Cannot set clock configuration\n");
+ return -1;
+ }
+
+ efuse->dev = dev;
+ efuse->mdev.minor = MISC_DYNAMIC_MINOR;
+ efuse->mdev.name = "jz-efuse";
+ efuse->mdev.fops = &efuse_misc_fops;
+
+ efuse->gpio_vddq_en_n = of_get_named_gpio_flags(dev->of_node,
+ "vddq-gpio", 0, &flags);
+ if (gpio_is_valid(efuse->gpio_vddq_en_n)) {
+ ret = devm_gpio_request_one(dev, efuse->gpio_vddq_en_n, flags,
+ dev_name(&pdev->dev));
+ if (ret) {
+ dev_err(dev, "Failed to request vddq gpio pin: %d\n",
+ ret);
+ return -ret;
+ }
+
+ /* power off by default */
+ ret = gpio_direction_output(efuse->gpio_vddq_en_n, 1);
+ if (ret) {
+ dev_err(dev, "Failed to set gpio as output: %d\n", ret);
+ return -ret;
+ }
+
+ efuse->is_timer_on = 0;
+ setup_timer(&efuse->vddq_protect_timer, jz_efuse_vddq_set,
+ (unsigned long)efuse);
+ } else {
+ dev_err(dev, "can't find gpio vddq.\n");
+ return -1;
+ }
+
+ spin_lock_init(&efuse->lock);
+
+ for (i = 0; i < ARRAY_SIZE(jz_efuse_sysfs_attrs); i++) {
+ ret = device_create_file(&pdev->dev, &jz_efuse_sysfs_attrs[i]);
+ if (ret) {
+ dev_err(dev, "Cannot make sysfs device files\n");
+ return -ret;
+ }
+ }
+
+ ret = misc_register(&efuse->mdev);
+ if (ret < 0) {
+ dev_err(dev, "misc_register failed\n");
+ return ret;
+ } else
+ dev_info(dev, "misc_register done!\n");
+
+ platform_set_drvdata(pdev, efuse);
+
+ dump_jz_efuse(efuse);
+
+ return 0;
+}
+
+static int jz_efuse_remove(struct platform_device *pdev)
+{
+ struct jz_efuse *efuse = platform_get_drvdata(pdev);
+
+ misc_deregister(&efuse->mdev);
+ del_timer(&efuse->vddq_protect_timer);
+
+ return 0;
+}
+
+static struct platform_driver jz_efuse_driver = {
+ .probe = jz_efuse_probe,
+ .remove = jz_efuse_remove,
+ .driver = {
+ .name = "jz-efuse",
+ .of_match_table = jz_efuse_of_match,
+ .owner = THIS_MODULE,
+ }
+};
+
+module_platform_driver(jz_efuse_driver);
+
+MODULE_AUTHOR("Zubair Lutfullah Kakakhel <Zubair....@imgtec.com>");
+MODULE_LICENSE("GPL");
+
diff --git a/drivers/misc/efuse/jz4780_efuse.h b/drivers/misc/efuse/jz4780_efuse.h
new file mode 100644
index 0000000..f01ff8b
--- /dev/null
+++ b/drivers/misc/efuse/jz4780_efuse.h
@@ -0,0 +1,81 @@
+#ifndef __JZ4780_EFUSE_H__
+#define __JZ4780_EFUSE_H__
+
+#include <linux/device.h>
+#include <linux/miscdevice.h>
+#include <linux/clk.h>
+#include <linux/timer.h>
+#include <linux/bitops.h>
+
+#define BYTEMASK(n) (((unsigned int)0xffffffff) >> (32 - n * 8))
+
+#define JZ_EFUCTRL (0x0) /* Control Register */
+#define JZ_EFUCFG (0x4) /* Configure Register*/
+#define JZ_EFUSTATE (0x8) /* Status Register */
+#define JZ_EFUDATA(n) (0xC + (n)*4)
+
+#define JZ_EFUSE_START_ADDR 0x200
+#define JZ_EFUSE_SEG1_OFF 0x00 /* 64 bit Random Number */
+#define JZ_EFUSE_SEG2_OFF 0x08 /* 128 bit Ingenic Chip ID */
+#define JZ_EFUSE_SEG3_OFF 0x18 /* 128 bit Customer ID */
+#define JZ_EFUSE_SEG4_OFF 0x28 /* 3520 bit Reserved */
+#define JZ_EFUSE_SEG5_OFF 0x1E0 /* 8 bit Protect Segment */
+#define JZ_EFUSE_SEG6_OFF 0x1E1 /* 2296 bit HDMI Key */
+#define JZ_EFUSE_SEG7_OFF 0x300 /* 2048 bit Security boot key */
+#define JZ_EFUSE_END_ADDR 0x5FF
+
+#define JZ_EFUSE_EFUCTRL_CS BIT(30)
+#define JZ_EFUSE_EFUCTRL_ADDR_MASK 0x1FF
+#define JZ_EFUSE_EFUCTRL_ADDR_SHIFT 21
+#define JZ_EFUSE_EFUCTRL_LEN_MASK 0x1F
+#define JZ_EFUSE_EFUCTRL_LEN_SHIFT 16
+#define JZ_EFUSE_EFUCTRL_PG_EN BIT(15)
+#define JZ_EFUSE_EFUCTRL_WR_EN BIT(1)
+#define JZ_EFUSE_EFUCTRL_RD_EN BIT(0)
+
+#define JZ_EFUSE_EFUCFG_INT_EN BIT(31)
+#define JZ_EFUSE_EFUCFG_RD_ADJ_MASK 0xF
+#define JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT 20
+#define JZ_EFUSE_EFUCFG_RD_STR_MASK 0xF
+#define JZ_EFUSE_EFUCFG_RD_STR_SHIFT 16
+#define JZ_EFUSE_EFUCFG_WR_ADJ_MASK 0xF
+#define JZ_EFUSE_EFUCFG_WR_ADJ_SHIFT 12
+#define JZ_EFUSE_EFUCFG_WR_STR_MASK 0xFFF
+#define JZ_EFUSE_EFUCFG_WR_STR_SHIFT 0
+
+#define JZ_EFUSE_EFUSTATE_WR_DONE BIT(1)
+#define JZ_EFUSE_EFUSTATE_RD_DONE BIT(0)
+
+struct jz_efuse_strict {
+ unsigned int min_rd_adj;
+ unsigned int min_rd_adj_strobe;
+ unsigned int min_wr_adj;
+ unsigned int min_wr_adj_strobe;
+ unsigned int max_wr_adj_strobe;
+};
+
+struct jz_efucfg_info {
+ unsigned int rd_adj;
+ unsigned int rd_strobe;
+ unsigned int wr_adj;
+ unsigned int wr_strobe;
+ struct jz_efuse_strict strict;
+};
+
+struct jz_efuse {
+ struct device *dev;
+ spinlock_t lock;
+ void __iomem *iomem;
+ struct miscdevice mdev;
+ struct clk *clk;
+ int max_program_length;
+ int use_count;
+ struct timer_list vddq_protect_timer;
+ unsigned int is_timer_on;
+ unsigned int gpio_vddq_en_n;
+ struct jz_efucfg_info efucfg_info;
+};
+
+void jz_efuse_id_read(int is_chip_id, uint32_t *buf);
+
+#endif
--
1.9.1

[Paul Burton]

I'll look through the logic later, but have some comments below from a
first pass.

Thanks,
Paul

On Mon, Nov 10, 2014 at 01:14:16PM +0000, Zubair Lutfullah Kakakhel wrote:
> This series adds an efuse driver to read/write from the JZ4780 EFUSE.
>
> The JZ4780 EFUSE is a one time programmable efuse inside the SoC.
>
> It is used to store the customer id, serial number, mac address,
> hdcp keys, etc.

Probably worth clarifying that some of that is specific to the CI20.

>
> This driver allows to read/write to the efuse in the JZ4780 SoC.

As you say in the cover letter, writing has not been tested. Perhaps it
would make sense to drop that code & add it later if & when it can be
tested? I haven't bothered looking through the write code, but some or
all of my comments from the read path apply there too.

Should be static. Inconsistent with the jz_ prefix used for other
functions.

> +{
> + dev_info(efuse->dev, "max_program_length = %x\n",
> + efuse->max_program_length);
> + dev_info(efuse->dev, "use_count = %x\n", efuse->use_count);
> + dev_info(efuse->dev, "is_timer_on = %x\n", efuse->is_timer_on);
> + dev_info(efuse->dev, "gpio_vddq_en_n = %x\n", efuse->gpio_vddq_en_n);
> +
> + dev_info(efuse->dev, "rd_adj = %x\n", efuse->efucfg_info.rd_adj);
> + dev_info(efuse->dev, "rd_strobe = %x\n", efuse->efucfg_info.rd_strobe);
> + dev_info(efuse->dev, "wr_adj = %x\n", efuse->efucfg_info.wr_adj);
> + dev_info(efuse->dev, "wr_strobe = %x\n", efuse->efucfg_info.wr_strobe);
> +
> + dev_info(efuse->dev, "min_rd_adj = %x\n",
> + efuse->efucfg_info.strict.min_rd_adj);
> + dev_info(efuse->dev, "min_rd_adj_strobe = %x\n",
> + efuse->efucfg_info.strict.min_rd_adj_strobe);
> + dev_info(efuse->dev, "min_wr_adj = %x\n",
> + efuse->efucfg_info.strict.min_wr_adj);
> + dev_info(efuse->dev, "min_wr_adj_strobe = %x\n",
> + efuse->efucfg_info.strict.min_wr_adj_strobe);
> + dev_info(efuse->dev, "max_wr_adj_strobe = %x\n",
> + efuse->efucfg_info.strict.max_wr_adj_strobe);

Do you really want to print all this normally? dev_dbg perhaps?

> +}
> +
> +static void jz_efuse_vddq_set(unsigned long efuse_ptr)
> +{
> + struct jz_efuse *efuse = (struct jz_efuse *)efuse_ptr;
> +
> + dev_info(efuse->dev, "JZ4780-EFUSE: vddq_set %d\n",
> + (int)efuse->is_timer_on);

The "JZ4780-EFUSE: " prefix is probably overkill with dev_info.

> +
> + if (efuse->is_timer_on)
> + mod_timer(&efuse->vddq_protect_timer, jiffies + HZ);
> +
> + gpio_set_value(efuse->gpio_vddq_en_n, !efuse->is_timer_on);
> +}
> +
> +static inline int jz_efuse_get_skip(size_t size)
> +{
> + if (size >= 32)
> + return 32;
> + else if ((size / 4) > 0)
> + return (size / 4) * 4;
> + else
> + return size % 4;
> +}
> +
> +static int calculate_efuse_strict(struct jz_efuse *efuse,
> + struct jz_efuse_strict *t, unsigned long rate)

Inconsistent with the jz_ prefix used for other function names.

> +{
> + unsigned int tmp;
> +
> + /* The EFUSE read/write strobes need to be within these values for it
> + * to function properly. Based on programmers manual.
> + */

Not quite valid comment style - start the text a line down.

I don't like all these magic numbers - could the values be given
meaningful names please?

> +
> + return 0;
> +}
> +
> +static int jz_init_efuse_cfginfo(struct jz_efuse *efuse, unsigned long clk_rate)
> +{
> + struct jz_efucfg_info *info = &efuse->efucfg_info;
> + struct jz_efuse_strict *s = &info->strict;
> + unsigned int tmp = 0;
> +
> + if (calculate_efuse_strict(efuse, s, clk_rate) < 0) {
> + dev_err(efuse->dev, "can't calculate strobe parameters\n");
> + return -EINVAL;
> + }
> +
> + info->rd_adj = (s->min_rd_adj + 0xf) / 2;
> + tmp = s->min_rd_adj_strobe - info->rd_adj;
> + info->rd_strobe = ((tmp + 0xf) / 2 < 7) ? 7 : (tmp + 0xf) / 2;

max((tmp + 0xf) / 2, 7)

I believe this but with meaningful names for the constants would be
better than open-coding the max macro.

> + if (info->rd_strobe > 0xf) {
> + dev_err(efuse->dev, "can't calculate read strobe\n");
> + return -EINVAL;
> + }
> +
> + tmp = (s->min_wr_adj_strobe + s->max_wr_adj_strobe) / 2;
> + info->wr_adj = tmp < 0xf ? tmp : 0xf;

min(tmp, 0xf)

Again, with meaningful names for constants.

> + info->wr_strobe = tmp - info->wr_adj;
> + if (info->wr_strobe > 0xfff) {
> + dev_err(efuse->dev, "can't calculate write strobe\n");
> + return -EINVAL;
> + }

More magic numbers throughout this function.

> +
> + return 0;
> +}
> +
> +static int jz_efuse_open(struct inode *inode, struct file *filp)
> +{
> + struct miscdevice *dev = filp->private_data;
> + struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
> +
> + spin_lock(&efuse->lock);
> + efuse->use_count++;
> + spin_unlock(&efuse->lock);

I'd suggest making use_count an atomic_t or using a kref, but you don't
seem to actually make use of the value of use_count beyond printing it
in dump_jz_efuse. Drop it?

> +
> + return 0;
> +}
> +
> +static int jz_efuse_release(struct inode *inode, struct file *filp)
> +{
> + struct miscdevice *dev = filp->private_data;
> + struct jz_efuse *efuse = container_of(dev, struct jz_efuse, mdev);
> +
> + spin_lock(&efuse->lock);
> + efuse->use_count--;
> + spin_unlock(&efuse->lock);

Likewise, drop use_count.

> +
> + return 0;
> +}
> +
> +static ssize_t _jz_efuse_read_bytes(struct jz_efuse *efuse, char *buf,
> + unsigned int addr, int skip)

jz_ or _jz_? Pick one :)

> +{
> + unsigned int tmp = 0;
> + int i = 0;
> + unsigned long flag;
> + int timeout = 1000;
> +
> + /* 1. Set config register */
> + spin_lock_irqsave(&efuse->lock, flag);
> + tmp = readl(efuse->iomem + JZ_EFUCFG);
> + tmp &= ~((JZ_EFUSE_EFUCFG_RD_ADJ_MASK << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
> + | (JZ_EFUSE_EFUCFG_RD_STR_MASK << JZ_EFUSE_EFUCFG_RD_STR_SHIFT));
> + tmp |= (efuse->efucfg_info.rd_adj << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
> + | (efuse->efucfg_info.rd_strobe << JZ_EFUSE_EFUCFG_RD_STR_SHIFT);
> + writel(tmp, efuse->iomem + JZ_EFUCFG);
> + spin_unlock_irqrestore(&efuse->lock, flag);
> +

Is there a reason to drop & reacquire the lock here? Especially since
it's as heavyweight as disabling IRQs. Also, why is it disabling IRQs?

> + /*
> + * 2. Set control register to indicate what to read data address,
> + * read data numbers and read enable.
> + */
> + spin_lock_irqsave(&efuse->lock, flag);
> + tmp = readl(efuse->iomem + JZ_EFUCTRL);
> + tmp &= ~(JZ_EFUSE_EFUCFG_RD_STR_SHIFT
> + | (JZ_EFUSE_EFUCTRL_ADDR_MASK << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
> + | JZ_EFUSE_EFUCTRL_PG_EN | JZ_EFUSE_EFUCTRL_WR_EN
> + | JZ_EFUSE_EFUCTRL_WR_EN);
> +
> + if (addr >= (JZ_EFUSE_START_ADDR + 0x200))

Magic number.

I'm not 100% clear yet on how the call to _jz_efuse_read_bytes from the
write path is meant to be useful, but particularly if the write code is
dropped why not just read directly into the user memory with put_user
rather than allocating a buffer & copying it? In any case, the memory
you allocate with devm_kzalloc will only be freed if the device is
removed which wouldn't usually be the case. So you'll leak like a sieve
here.

> --
> You received this message because you are subscribed to the Google Groups "MIPS Creator CI20 Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to mips-creator-ci2...@googlegroups.com.
> To post to this group, send an email to mips-creat...@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/mips-creator-ci20-dev/1415625257-49882-3-git-send-email-Zubair.Kakakhel%40imgtec.com.
> For more options, visit https://groups.google.com/d/optout.

[Zubair Lutfullah Kakakhel]

Thanks for the review.

I'll address them all in the next series.

ZubairLK

[James Hogan]

Hi Zubair,

I've only glanced through it tbh...

On 10/11/14 13:14, Zubair Lutfullah Kakakhel wrote:
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index ee94023..b3e1fb7 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -526,6 +526,7 @@ config VEXPRESS_SYSCFG
> of generating transactions on this bus.
>
> source "drivers/misc/c2port/Kconfig"
> +source "drivers/misc/efuse/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> source "drivers/misc/ti-st/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index d59ce12..a142b2a 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_DS1682) += ds1682.o
> obj-$(CONFIG_TI_DAC7512) += ti_dac7512.o
> obj-$(CONFIG_C2PORT) += c2port/
> obj-$(CONFIG_HMC6352) += hmc6352.o
> +obj-y += efuse/

Whole new subsystem? Does it fit into anything that already exists?

E.g. there is a drivers/misc/fuse/ with the tegra driver, for which the
patchset also mentioned that the interface is modelled after sunxi_sid
driver.

Out of interest, is this the sort of device that could theoretically
have a standardised user API and driver interface?

> diff --git a/drivers/misc/efuse/Kconfig b/drivers/misc/efuse/Kconfig
> new file mode 100644
> index 0000000..3700682
> --- /dev/null
> +++ b/drivers/misc/efuse/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# Misc efuse devices. generally found in SoCs. Used to store mac addresses,
> +# encryption keys, etc.
> +#
> +
> +menu "Efuse devices"
> +
> +config JZ4780_EFUSE
> + bool "Ingenic JZ4780 EFUSE driver"
> + depends on MACH_JZ4780
> + default n

n is default, so you never need to put "default n".

> + help
> + Say Y here if you want the efuse driver for the Ingenic JZ4780 SoC.

This should be indented by an extra 2 spaces.

> +
> +
> +endmenu
> +

> diff --git a/drivers/misc/efuse/jz4780_efuse.c b/drivers/misc/efuse/jz4780_efuse.c
> new file mode 100644
> index 0000000..cf4fef8
> --- /dev/null
> +++ b/drivers/misc/efuse/jz4780_efuse.c
> @@ -0,0 +1,685 @@
> +/*
> + * jz4780_efuse.c - Driver to read/write from the JZ4780 one time programmable
> + * efuse 8K memory
> + *
> + * The rom itself is accessed using a 9 bit address line and an 8 word wide bus
> + * which reads/writes based on strobes. The strobe is configured in the config
> + * register and is based on number of cycles of the AHB2 clock.
> + *
> + * Copyright (C) 2014 Imagination Technologies
> + *
> + * Based on work by Ingenic Semiconductor.

Did they claim any copyright on their original work?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

> +static ssize_t _jz_efuse_read_bytes(struct jz_efuse *efuse, char *buf,
> + unsigned int addr, int skip)
> +{
> + unsigned int tmp = 0;
> + int i = 0;
> + unsigned long flag;
> + int timeout = 1000;
> +
> + /* 1. Set config register */
> + spin_lock_irqsave(&efuse->lock, flag);

Is this actually locking against an IRQ or just other callback functions
in normal user context. I've not seen anything glancing through that
necessitates the irqsave/irqrestore.

> + tmp = readl(efuse->iomem + JZ_EFUCFG);
> + tmp &= ~((JZ_EFUSE_EFUCFG_RD_ADJ_MASK << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
> + | (JZ_EFUSE_EFUCFG_RD_STR_MASK << JZ_EFUSE_EFUCFG_RD_STR_SHIFT));
> + tmp |= (efuse->efucfg_info.rd_adj << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
> + | (efuse->efucfg_info.rd_strobe << JZ_EFUSE_EFUCFG_RD_STR_SHIFT);
> + writel(tmp, efuse->iomem + JZ_EFUCFG);
> + spin_unlock_irqrestore(&efuse->lock, flag);
> +
> + /*
> + * 2. Set control register to indicate what to read data address,
> + * read data numbers and read enable.
> + */
> + spin_lock_irqsave(&efuse->lock, flag);

necessary to keep grabbing and releasing the spinlock?

-1 isn't really a standard error code.

A bit like GPIOF_OUT_INIT_HIGH flag that can be passed to
devm_gpio_request_one?

> + if (ret) {
> + dev_err(dev, "Failed to set gpio as output: %d\n", ret);
> + return -ret;

why are you negating ret throughout this function?

braces needed on all arms of if/else if/else.

Actually, this dev_info is a bit pointless, looks more like debug.

> +
> + platform_set_drvdata(pdev, efuse);

I suspect this should be before misc_register, so userland cannot screw
it up from a uevent notification before probe is complete.

> +
> + dump_jz_efuse(efuse);
> +
> + return 0;
> +}
> +
> +static int jz_efuse_remove(struct platform_device *pdev)
> +{
> + struct jz_efuse *efuse = platform_get_drvdata(pdev);
> +
> + misc_deregister(&efuse->mdev);
> + del_timer(&efuse->vddq_protect_timer);

i suspect you'll want del_timer_sync, since you don't the handler to be
running this point on an SMP system, potentially accessing deallocated
memory or whatever.

> +
> + return 0;
> +}
> +
> +static struct platform_driver jz_efuse_driver = {
> + .probe = jz_efuse_probe,
> + .remove = jz_efuse_remove,
> + .driver = {
> + .name = "jz-efuse",
> + .of_match_table = jz_efuse_of_match,
> + .owner = THIS_MODULE,

No need to set owner, already done by
platform_driver_register/__platform_driver_register.

> + }
> +};

Cheers
James

[Zubair Lutfullah Kakakhel]

Hi,

I wouldnt call it a subsystem yet..

https://lkml.org/lkml/2014/5/28/287

Some SoC specific drivers and some common code.

>
> E.g. there is a drivers/misc/fuse/ with the tegra driver, for which the
> patchset also mentioned that the interface is modelled after sunxi_sid
> driver.

Now that I've seen the sunxi_sid code, I think this could sit under
drivers/misc/eeprom.

And those drivers are also just to read eeprom only. Reason stated that the writes
are done in factory and user is only playing with the reads.

>
> Out of interest, is this the sort of device that could theoretically
> have a standardised user API and driver interface?

Possibly. I think a lot of SoCs would have a rom for serial numbers etc.

But the subsystem will have to be quite dynamic. Because SoCs place
different relevant bits of various sizes.

Ingenic has a 128 bit customer id.
I checked sunxi, they have a 16 bit customer id.

And locations vary as well.

Having a subsystem for this would mean that the core would be quite flexible
and dt bindings would have lots of optional properties.

e.g.
usual
compatible
reg
clocks

segment1 = <Start_Address Bits>
segment1 = <name>

segment2 =...

similar. or an array perhaps.

e.g. for sunxi

0x000 128 bit root-key (sun[457]i)
0x010 128 bit boot-key (sun7i)
0x020 64 bit security-jtag-key (sun7i)
0x028 16 bit key configuration (sun7i)
0x02b 16 bit custom-vendor-key (sun7i)
0x02c 320 bit low general key (sun7i)
0x040 32 bit read-control access (sun7i)
0x064 224 bit low general key (sun7i)
0x080 2304 bit HDCP-key (sun7i)
0x1a0 768 bit high general key (sun7i)

For jz3780

0x200 64 bit random
0x208 128 bit ingenic chip
0x218 128 bit customer id
0x228 3520 bit reserved
0x3e0 8 bit protect
0x3e1 2296 bit hdcp
0x500 2048 bit security boot key.

The most common ones would be hdcp, 2048 bit security key, chip id, serial number.
Those could have dedicated bindings so other subsystems could use them. mac address perhaps.
But some generic ones will be needed. segment1, segment2 etc.

Apart from variable naming, length, locations.

From the subsystems core perspective, I think the driver side wouldn't be that complicated.
I imagine most efuse/eeproms just provide a register and a read interface primarily.
And an optional write interface as well.

The benefit of a subsystem would be slightly standardized access to customer/chip id,
mac address, hdcp etc.

I doubt it would reduce the driver complexity by much.

because instead of misc_register, it'll become efuse_register.

And each efuse would have its own read routine.
sunxi and jz4780 don't have common interfaces for reading..

perhaps the drivers which have only simple reads i.e. read from a memory location
could have a generic-efuse.c once dt defines locations.

jz4780 has a rather intricate address,length,rd/wr strobe etc.
Direct memory read didn't work..

Although, I just remebered a possible reason regarding why a direct read didn't work..
I'll check again..

No. The code had no header/footer regarding author or whatever.
Thankfully, there was a MODULE_LICENSE("GPL");
:)

>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>
>
>> +static ssize_t _jz_efuse_read_bytes(struct jz_efuse *efuse, char *buf,
>> + unsigned int addr, int skip)
>> +{
>> + unsigned int tmp = 0;
>> + int i = 0;
>> + unsigned long flag;
>> + int timeout = 1000;
>> +
>> + /* 1. Set config register */
>> + spin_lock_irqsave(&efuse->lock, flag);
>
> Is this actually locking against an IRQ or just other callback functions
> in normal user context. I've not seen anything glancing through that
> necessitates the irqsave/irqrestore.
>

I was cleaning up 3.0.8. Most of the complexity regarding locks is for writes.
Because vddq cannot be held high longer than 1 second.
Similar complexity creeps into read sequences.

I think removing write as Paul suggested would be wise. It serves no purpose
anyways as its one-time programmable. And the factory does this..

>> + tmp = readl(efuse->iomem + JZ_EFUCFG);

Thanks for the review.

ZubairLK