[PATCH 0/2] rockchip: efuse: add driver to support rk3288 efuse

[PATCH 0/2] rockchip: efuse: add driver to support rk3288 efuse

[Jianqun Xu]

In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time
programmable electrical fuses with random access interface, and the other
is organized as 32bits by 32 one-time programmable electrical fuses.

Jianqun Xu (2):
  rockchip: efuse: add documentation for rk3288 efuse driver
  rockchip: efuse: add efuse driver for rk3288 efuse

 .../bindings/fuse/rockchip,rk3288-efuse.txt        |  14 ++
 arch/arm/mach-rockchip/efuse.c                     | 165 +++++++++++++++++++++
 arch/arm/mach-rockchip/efuse.h                     |  15 ++
 3 files changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
 create mode 100644 arch/arm/mach-rockchip/efuse.c
 create mode 100644 arch/arm/mach-rockchip/efuse.h

-- 
1.9.1

[PATCH 0/2] rockchip: efuse: add driver to support rk3288 efuse

[Jianqun Xu]

In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time
programmable electrical fuses with random access interface, and the other
is organized as 32bits by 32 one-time programmable electrical fuses.

Jianqun Xu (2):
  rockchip: efuse: add documentation for rk3288 efuse driver
  rockchip: efuse: add efuse driver for rk3288 efuse

 .../bindings/fuse/rockchip,rk3288-efuse.txt        |  14 ++
 arch/arm/mach-rockchip/efuse.c                     | 165 +++++++++++++++++++++
 arch/arm/mach-rockchip/efuse.h                     |  15 ++
 3 files changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
 create mode 100644 arch/arm/mach-rockchip/efuse.c
 create mode 100644 arch/arm/mach-rockchip/efuse.h

-- 
1.9.1

[PATCH 1/2] rockchip: efuse: add documentation for rk3288 efuse driver

[Jianqun Xu]

In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time programmable
electrical fuses with random access interface, and the other is organized as 32bits by 32
one-time programmable electrical fuses.
The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 .../devicetree/bindings/fuse/rockchip,rk3288-efuse.txt     | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt

diff --git a/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt b/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
new file mode 100644
index 0000000..82af730
--- /dev/null
+++ b/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
@@ -0,0 +1,14 @@
+ROCKCHIP RK3288 efuse block.
+
+Required properties:
+- compatible : should be:
+	"rockchip,rk3288-efuse"
+- reg: Should contain 1 entry: the entry gives the physical address and length
+       of the fuse registers.
+
+Example:
+
+	efuse: efuse@ffb40000 {
+		compatible = "rockchip,rk3288-efuse";
+		reg = <0xffb40000 0x10000>;
+	};
-- 
1.9.1

[PATCH 1/2] rockchip: efuse: add documentation for rk3288 efuse driver

[Jianqun Xu]

In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time programmable
electrical fuses with random access interface, and the other is organized as 32bits by 32
one-time programmable electrical fuses.
The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 .../devicetree/bindings/fuse/rockchip,rk3288-efuse.txt     | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt

diff --git a/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt b/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
new file mode 100644
index 0000000..82af730
--- /dev/null
+++ b/Documentation/devicetree/bindings/fuse/rockchip,rk3288-efuse.txt
@@ -0,0 +1,14 @@
+ROCKCHIP RK3288 efuse block.
+
+Required properties:
+- compatible : should be:
+	"rockchip,rk3288-efuse"
+- reg: Should contain 1 entry: the entry gives the physical address and length
+       of the fuse registers.
+
+Example:
+
+	efuse: efuse at ffb40000 {
+		compatible = "rockchip,rk3288-efuse";
+		reg = <0xffb40000 0x10000>;
+	};
-- 
1.9.1

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Jianqun Xu]

Add driver for efuse found on rk3288 board based on rk3288 SoC.
Driver will read fuse information of chip at the boot stage of
kernel, this information new is for further usage.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rockchip/efuse.h |  15 ++++
 2 files changed, 180 insertions(+)
 create mode 100644 arch/arm/mach-rockchip/efuse.c
 create mode 100644 arch/arm/mach-rockchip/efuse.h

diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
new file mode 100644
index 0000000..326d81e
--- /dev/null
+++ b/arch/arm/mach-rockchip/efuse.c
@@ -0,0 +1,165 @@
+/* mach-rockchip/efuse.c
+ *
+ * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
+ * Author: Jianqun Xu <jay.xu@rock-chips.com>
+ *
+ * Tmis program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * Tmis program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * tmis program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
+ *
+ * Tme full GNU General Public License is included in this distribution in the
+ * file called LICENSE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/io.h>
+
+#include "efuse.h"
+
+#define EFUSE_BUF_SIZE	(32)
+#define EFUSE_BUF_LKG_CPU	(23)
+#define EFUSE_BUF_LKG_GPU	(24)
+#define EFUSE_BUF_LKG_LOG	(25)
+
+struct rk_efuse_info {
+	/* Platform device */
+	struct device *dev;
+
+	/* Hardware resources */
+	void __iomem *regs;
+
+	/* buffer to store registers' values */
+	unsigned int buf[EFUSE_BUF_SIZE];
+};
+
+static void efuse_writel(struct rk_efuse_info *efuse,
+			 unsigned int value,
+			 unsigned int offset)
+{
+	writel_relaxed(value, efuse->regs + offset);
+}
+
+static unsigned int efuse_readl(struct rk_efuse_info *efuse,
+				unsigned int offset)
+{
+	return readl_relaxed(efuse->regs + offset);
+}
+
+static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
+					 int channel)
+{
+	switch (channel) {
+	case EFUSE_BUF_LKG_CPU:
+	case EFUSE_BUF_LKG_GPU:
+	case EFUSE_BUF_LKG_LOG:
+		return efuse->buf[channel];
+	default:
+		dev_err(efuse->dev, "unknown channel\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rockchip_efuse_info(struct rk_efuse_info *efuse)
+{
+	dev_info(efuse->dev, "leakage (%d %d %d)\n",
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
+}
+
+static int rockchip_efuse_init(struct rk_efuse_info *efuse)
+{
+	int start = 0;
+	int ret = 0;
+
+	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
+	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
+	udelay(2);
+
+	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
+			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
+			     REG_EFUSE_CTRL);
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
+			     REG_EFUSE_CTRL);
+		udelay(2);
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+			     EFUSE_STROBE, REG_EFUSE_CTRL);
+		udelay(2);
+
+		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
+
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
+			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
+		udelay(2);
+	}
+
+	udelay(2);
+	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+		     EFUSE_CSB, REG_EFUSE_CTRL);
+	udelay(2);
+
+	return ret;
+}
+
+static int rockchip_efuse_probe(struct platform_device *pdev)
+{
+	struct rk_efuse_info *efuse;
+	struct resource *mem;
+	int ret = 0;
+
+	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(efuse->regs))
+		return PTR_ERR(efuse->regs);
+
+	platform_set_drvdata(pdev, efuse);
+	efuse->dev = &pdev->dev;
+
+	ret = rockchip_efuse_init(efuse);
+	if (!ret)
+		rockchip_efuse_info(efuse);
+
+	return ret;
+}
+
+static const struct of_device_id rockchip_efuse_match[] = {
+	{ .compatible = "rockchip,rk3288-efuse", },
+	{},
+};
+
+static struct platform_driver rockchip_efuse_driver = {
+	.probe = rockchip_efuse_probe,
+	.driver = {
+		.name = "rk3288-efuse",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(rockchip_efuse_match),
+	},
+};
+
+module_platform_driver(rockchip_efuse_driver);
+
+MODULE_DESCRIPTION("Rockchip eFuse Driver");
+MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
new file mode 100644
index 0000000..3fdcf6d
--- /dev/null
+++ b/arch/arm/mach-rockchip/efuse.h
@@ -0,0 +1,15 @@
+#ifndef _ARCH_ROCKCHIP_EFUSE_H_
+#define _ARCH_ROCKCHIP_EFUSE_H_
+
+/* Rockchip eFuse controller register */
+#define EFUSE_A_SHIFT		(6)
+#define EFUSE_A_MASK		(0x3FF)
+#define EFUSE_PGENB		(1 << 3)
+#define EFUSE_LOAD		(1 << 2)
+#define EFUSE_STROBE		(1 << 1)
+#define EFUSE_CSB		(1 << 0)
+
+#define REG_EFUSE_CTRL		(0x0000)
+#define REG_EFUSE_DOUT		(0x0004)
+
+#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */
-- 
1.9.1

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Jianqun Xu]

Add driver for efuse found on rk3288 board based on rk3288 SoC.
Driver will read fuse information of chip at the boot stage of
kernel, this information new is for further usage.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-rockchip/efuse.h |  15 ++++
 2 files changed, 180 insertions(+)
 create mode 100644 arch/arm/mach-rockchip/efuse.c
 create mode 100644 arch/arm/mach-rockchip/efuse.h

diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
new file mode 100644
index 0000000..326d81e
--- /dev/null
+++ b/arch/arm/mach-rockchip/efuse.c
@@ -0,0 +1,165 @@
+/* mach-rockchip/efuse.c
+ *
+ * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
+ * Author: Jianqun Xu <jay.xu@rock-chips.com>
+ *
+ * Tmis program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * Tmis program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * tmis program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
+ *
+ * Tme full GNU General Public License is included in this distribution in the
+ * file called LICENSE.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/io.h>
+
+#include "efuse.h"
+
+#define EFUSE_BUF_SIZE	(32)
+#define EFUSE_BUF_LKG_CPU	(23)
+#define EFUSE_BUF_LKG_GPU	(24)
+#define EFUSE_BUF_LKG_LOG	(25)
+
+struct rk_efuse_info {
+	/* Platform device */
+	struct device *dev;
+
+	/* Hardware resources */
+	void __iomem *regs;
+
+	/* buffer to store registers' values */
+	unsigned int buf[EFUSE_BUF_SIZE];
+};
+
+static void efuse_writel(struct rk_efuse_info *efuse,
+			 unsigned int value,
+			 unsigned int offset)
+{
+	writel_relaxed(value, efuse->regs + offset);
+}
+
+static unsigned int efuse_readl(struct rk_efuse_info *efuse,
+				unsigned int offset)
+{
+	return readl_relaxed(efuse->regs + offset);
+}
+
+static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
+					 int channel)
+{
+	switch (channel) {
+	case EFUSE_BUF_LKG_CPU:
+	case EFUSE_BUF_LKG_GPU:
+	case EFUSE_BUF_LKG_LOG:
+		return efuse->buf[channel];
+	default:
+		dev_err(efuse->dev, "unknown channel\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rockchip_efuse_info(struct rk_efuse_info *efuse)
+{
+	dev_info(efuse->dev, "leakage (%d %d %d)\n",
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
+		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
+}
+
+static int rockchip_efuse_init(struct rk_efuse_info *efuse)
+{
+	int start = 0;
+	int ret = 0;
+
+	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
+	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
+	udelay(2);
+
+	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
+			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
+			     REG_EFUSE_CTRL);
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
+			     REG_EFUSE_CTRL);
+		udelay(2);
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+			     EFUSE_STROBE, REG_EFUSE_CTRL);
+		udelay(2);
+
+		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
+
+		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
+			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
+		udelay(2);
+	}
+
+	udelay(2);
+	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
+		     EFUSE_CSB, REG_EFUSE_CTRL);
+	udelay(2);
+
+	return ret;
+}
+
+static int rockchip_efuse_probe(struct platform_device *pdev)
+{
+	struct rk_efuse_info *efuse;
+	struct resource *mem;
+	int ret = 0;
+
+	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(efuse->regs))
+		return PTR_ERR(efuse->regs);
+
+	platform_set_drvdata(pdev, efuse);
+	efuse->dev = &pdev->dev;
+
+	ret = rockchip_efuse_init(efuse);
+	if (!ret)
+		rockchip_efuse_info(efuse);
+
+	return ret;
+}
+
+static const struct of_device_id rockchip_efuse_match[] = {
+	{ .compatible = "rockchip,rk3288-efuse", },
+	{},
+};
+
+static struct platform_driver rockchip_efuse_driver = {
+	.probe = rockchip_efuse_probe,
+	.driver = {
+		.name = "rk3288-efuse",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(rockchip_efuse_match),
+	},
+};
+
+module_platform_driver(rockchip_efuse_driver);
+
+MODULE_DESCRIPTION("Rockchip eFuse Driver");
+MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
new file mode 100644
index 0000000..3fdcf6d
--- /dev/null
+++ b/arch/arm/mach-rockchip/efuse.h
@@ -0,0 +1,15 @@
+#ifndef _ARCH_ROCKCHIP_EFUSE_H_
+#define _ARCH_ROCKCHIP_EFUSE_H_
+
+/* Rockchip eFuse controller register */
+#define EFUSE_A_SHIFT		(6)
+#define EFUSE_A_MASK		(0x3FF)
+#define EFUSE_PGENB		(1 << 3)
+#define EFUSE_LOAD		(1 << 2)
+#define EFUSE_STROBE		(1 << 1)
+#define EFUSE_CSB		(1 << 0)
+
+#define REG_EFUSE_CTRL		(0x0000)
+#define REG_EFUSE_DOUT		(0x0004)
+
+#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */
-- 
1.9.1

Re: [PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Heiko Stübner]

Hi Jianqun,

Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> Driver will read fuse information of chip at the boot stage of
> kernel, this information new is for further usage.
> 
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>

General question would be, what is the purpose of this driver?
I don't see any publically usable functions and the only thing happening is 
the
	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
output that emits some information from the efuse to the kernel log?


In the dt-binding doc you write:
> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.

While the TRM also says this, IO_SECURITY is not mentioned anywhere else in 
the document. Is this a pin or a bit somewhere in the GRF - i.e. something 
whichs state is readable?


Some more comments inline.

> ---
>  arch/arm/mach-rockchip/efuse.c | 165
> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h | 
> 15 ++++
>  2 files changed, 180 insertions(+)
>  create mode 100644 arch/arm/mach-rockchip/efuse.c
>  create mode 100644 arch/arm/mach-rockchip/efuse.h
> 
> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
> new file mode 100644
> index 0000000..326d81e
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.c

a driver like this should probably live in something like 
drivers/soc/rockchip.


> @@ -0,0 +1,165 @@
> +/* mach-rockchip/efuse.c
> + *
> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> + *
> + * Tmis program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * Tmis program is distributed in the hope that it will be useful, but

type Tmis -> This


> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> General Public License for + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with + * tmis program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
> + *
> + * Tme full GNU General Public License is included in this distribution in
> the + * file called LICENSE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +
> +#include "efuse.h"
> +
> +#define EFUSE_BUF_SIZE	(32)
> +#define EFUSE_BUF_LKG_CPU	(23)
> +#define EFUSE_BUF_LKG_GPU	(24)
> +#define EFUSE_BUF_LKG_LOG	(25)

no braces needed for those numbers


> +
> +struct rk_efuse_info {
> +	/* Platform device */
> +	struct device *dev;
> +
> +	/* Hardware resources */
> +	void __iomem *regs;
> +
> +	/* buffer to store registers' values */
> +	unsigned int buf[EFUSE_BUF_SIZE];
> +};
> +
> +static void efuse_writel(struct rk_efuse_info *efuse,
> +			 unsigned int value,
> +			 unsigned int offset)
> +{
> +	writel_relaxed(value, efuse->regs + offset);
> +}
> +
> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> +				unsigned int offset)
> +{
> +	return readl_relaxed(efuse->regs + offset);
> +}

why these indirections for readl and writel? They don't seem to provide any 
additional benefit over calling writel_relaxed/readl_relaxed directly below.


> +
> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> +					 int channel)
> +{
> +	switch (channel) {
> +	case EFUSE_BUF_LKG_CPU:
> +	case EFUSE_BUF_LKG_GPU:
> +	case EFUSE_BUF_LKG_LOG:
> +		return efuse->buf[channel];
> +	default:
> +		dev_err(efuse->dev, "unknown channel\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
> +{
> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
> +}
> +
> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
> +{
> +	int start = 0;
> +	int ret = 0;
> +
> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
> +	udelay(2);
> +
> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
> +			     REG_EFUSE_CTRL);
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
> +			     REG_EFUSE_CTRL);
> +		udelay(2);
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
> +		udelay(2);
> +
> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
> +
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
> +		udelay(2);
> +	}
> +
> +	udelay(2);
> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +		     EFUSE_CSB, REG_EFUSE_CTRL);
> +	udelay(2);
> +
> +	return ret;
> +}
> +
> +static int rockchip_efuse_probe(struct platform_device *pdev)
> +{
> +	struct rk_efuse_info *efuse;
> +	struct resource *mem;
> +	int ret = 0;
> +
> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> +	if (!efuse)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(efuse->regs))
> +		return PTR_ERR(efuse->regs);
> +
> +	platform_set_drvdata(pdev, efuse);
> +	efuse->dev = &pdev->dev;
> +
> +	ret = rockchip_efuse_init(efuse);
> +	if (!ret)
> +		rockchip_efuse_info(efuse);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id rockchip_efuse_match[] = {
> +	{ .compatible = "rockchip,rk3288-efuse", },

what about other Rockchip SoCs? At least the rk3188 seems to contain an efuse 
[though the TRM I have only directs to a RK3188 eFuse.pdf without describing 
the component. So I don't know if it's the same type.


> +	{},
> +};
> +
> +static struct platform_driver rockchip_efuse_driver = {
> +	.probe = rockchip_efuse_probe,
> +	.driver = {
> +		.name = "rk3288-efuse",
> +		.owner = THIS_MODULE,

.owner gets already set through module_platform_driver


> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
> +	},
> +};
> +
> +module_platform_driver(rockchip_efuse_driver);
> +
> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
> new file mode 100644
> index 0000000..3fdcf6d
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.h

why does this need to be a separate header? The stuff below could very well 
simply live inside the fuse.c


> @@ -0,0 +1,15 @@
> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
> +#define _ARCH_ROCKCHIP_EFUSE_H_
> +
> +/* Rockchip eFuse controller register */
> +#define EFUSE_A_SHIFT		(6)
> +#define EFUSE_A_MASK		(0x3FF)
> +#define EFUSE_PGENB		(1 << 3)

please use BIT(3) instead of (1 << 3)
Same for the bits below.


> +#define EFUSE_LOAD		(1 << 2)
> +#define EFUSE_STROBE		(1 << 1)
> +#define EFUSE_CSB		(1 << 0)
> +
> +#define REG_EFUSE_CTRL		(0x0000)
> +#define REG_EFUSE_DOUT		(0x0004)

no braces necessary for basic numerals


> +
> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Heiko Stübner]

Hi Jianqun,

Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> Driver will read fuse information of chip at the boot stage of
> kernel, this information new is for further usage.
> 
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>

General question would be, what is the purpose of this driver?
I don't see any publically usable functions and the only thing happening is 
the
	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
output that emits some information from the efuse to the kernel log?


In the dt-binding doc you write:
> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.

While the TRM also says this, IO_SECURITY is not mentioned anywhere else in 
the document. Is this a pin or a bit somewhere in the GRF - i.e. something 
whichs state is readable?


Some more comments inline.

> ---
>  arch/arm/mach-rockchip/efuse.c | 165
> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h | 
> 15 ++++
>  2 files changed, 180 insertions(+)
>  create mode 100644 arch/arm/mach-rockchip/efuse.c
>  create mode 100644 arch/arm/mach-rockchip/efuse.h
> 
> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
> new file mode 100644
> index 0000000..326d81e
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.c

a driver like this should probably live in something like 
drivers/soc/rockchip.


> @@ -0,0 +1,165 @@
> +/* mach-rockchip/efuse.c
> + *
> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> + *
> + * Tmis program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * Tmis program is distributed in the hope that it will be useful, but

type Tmis -> This


> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> General Public License for + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with + * tmis program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
> + *
> + * Tme full GNU General Public License is included in this distribution in
> the + * file called LICENSE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +
> +#include "efuse.h"
> +
> +#define EFUSE_BUF_SIZE	(32)
> +#define EFUSE_BUF_LKG_CPU	(23)
> +#define EFUSE_BUF_LKG_GPU	(24)
> +#define EFUSE_BUF_LKG_LOG	(25)

no braces needed for those numbers


> +
> +struct rk_efuse_info {
> +	/* Platform device */
> +	struct device *dev;
> +
> +	/* Hardware resources */
> +	void __iomem *regs;
> +
> +	/* buffer to store registers' values */
> +	unsigned int buf[EFUSE_BUF_SIZE];
> +};
> +
> +static void efuse_writel(struct rk_efuse_info *efuse,
> +			 unsigned int value,
> +			 unsigned int offset)
> +{
> +	writel_relaxed(value, efuse->regs + offset);
> +}
> +
> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> +				unsigned int offset)
> +{
> +	return readl_relaxed(efuse->regs + offset);
> +}

why these indirections for readl and writel? They don't seem to provide any 
additional benefit over calling writel_relaxed/readl_relaxed directly below.


> +
> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> +					 int channel)
> +{
> +	switch (channel) {
> +	case EFUSE_BUF_LKG_CPU:
> +	case EFUSE_BUF_LKG_GPU:
> +	case EFUSE_BUF_LKG_LOG:
> +		return efuse->buf[channel];
> +	default:
> +		dev_err(efuse->dev, "unknown channel\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
> +{
> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
> +}
> +
> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
> +{
> +	int start = 0;
> +	int ret = 0;
> +
> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
> +	udelay(2);
> +
> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
> +			     REG_EFUSE_CTRL);
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
> +			     REG_EFUSE_CTRL);
> +		udelay(2);
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
> +		udelay(2);
> +
> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
> +
> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
> +		udelay(2);
> +	}
> +
> +	udelay(2);
> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> +		     EFUSE_CSB, REG_EFUSE_CTRL);
> +	udelay(2);
> +
> +	return ret;
> +}
> +
> +static int rockchip_efuse_probe(struct platform_device *pdev)
> +{
> +	struct rk_efuse_info *efuse;
> +	struct resource *mem;
> +	int ret = 0;
> +
> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> +	if (!efuse)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(efuse->regs))
> +		return PTR_ERR(efuse->regs);
> +
> +	platform_set_drvdata(pdev, efuse);
> +	efuse->dev = &pdev->dev;
> +
> +	ret = rockchip_efuse_init(efuse);
> +	if (!ret)
> +		rockchip_efuse_info(efuse);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id rockchip_efuse_match[] = {
> +	{ .compatible = "rockchip,rk3288-efuse", },

what about other Rockchip SoCs? At least the rk3188 seems to contain an efuse 
[though the TRM I have only directs to a RK3188 eFuse.pdf without describing 
the component. So I don't know if it's the same type.


> +	{},
> +};
> +
> +static struct platform_driver rockchip_efuse_driver = {
> +	.probe = rockchip_efuse_probe,
> +	.driver = {
> +		.name = "rk3288-efuse",
> +		.owner = THIS_MODULE,

.owner gets already set through module_platform_driver


> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
> +	},
> +};
> +
> +module_platform_driver(rockchip_efuse_driver);
> +
> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
> new file mode 100644
> index 0000000..3fdcf6d
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.h

why does this need to be a separate header? The stuff below could very well 
simply live inside the fuse.c


> @@ -0,0 +1,15 @@
> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
> +#define _ARCH_ROCKCHIP_EFUSE_H_
> +
> +/* Rockchip eFuse controller register */
> +#define EFUSE_A_SHIFT		(6)
> +#define EFUSE_A_MASK		(0x3FF)
> +#define EFUSE_PGENB		(1 << 3)

please use BIT(3) instead of (1 << 3)
Same for the bits below.


> +#define EFUSE_LOAD		(1 << 2)
> +#define EFUSE_STROBE		(1 << 1)
> +#define EFUSE_CSB		(1 << 0)
> +
> +#define REG_EFUSE_CTRL		(0x0000)
> +#define REG_EFUSE_DOUT		(0x0004)

no braces necessary for basic numerals


> +
> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */

Re: [PATCH 0/2] rockchip: efuse: add driver to support rk3288 efuse

[Stefan Wahren]

Hi Jianqun,

> Jianqun Xu <jay.xu@rock-chips.com> hat am 1. Dezember 2014 um 08:34
> geschrieben:
>
>
> In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time
> programmable electrical fuses with random access interface, and the other
> is organized as 32bits by 32 one-time programmable electrical fuses.
>
> Jianqun Xu (2):
> rockchip: efuse: add documentation for rk3288 efuse driver
> rockchip: efuse: add efuse driver for rk3288 efuse
>
> .../bindings/fuse/rockchip,rk3288-efuse.txt | 14 ++
> arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++
> arch/arm/mach-rockchip/efuse.h | 15 ++

is it possible that you forgot Kconfig and Makefile?

Stefan

[PATCH 0/2] rockchip: efuse: add driver to support rk3288 efuse

[Stefan Wahren]

Hi Jianqun,

> Jianqun Xu <jay.xu@rock-chips.com> hat am 1. Dezember 2014 um 08:34
> geschrieben:
>
>
> In RK3288, there are two eFuse. One is organized as 32bits by 8 one-time
> programmable electrical fuses with random access interface, and the other
> is organized as 32bits by 32 one-time programmable electrical fuses.
>
> Jianqun Xu (2):
> rockchip: efuse: add documentation for rk3288 efuse driver
> rockchip: efuse: add efuse driver for rk3288 efuse
>
> .../bindings/fuse/rockchip,rk3288-efuse.txt | 14 ++
> arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++
> arch/arm/mach-rockchip/efuse.h | 15 ++

is it possible that you forgot Kconfig and Makefile?

Stefan

Re: [PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Stefan Wahren]

Hi Jianqun,

> Jianqun Xu <jay.xu@rock-chips.com> hat am 1. Dezember 2014 um 08:34
> geschrieben:
>
>
> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> Driver will read fuse information of chip at the boot stage of
> kernel, this information new is for further usage.
>
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> ---
> arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-rockchip/efuse.h | 15 ++++
> 2 files changed, 180 insertions(+)
> create mode 100644 arch/arm/mach-rockchip/efuse.c
> create mode 100644 arch/arm/mach-rockchip/efuse.h
>
> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
> new file mode 100644
> index 0000000..326d81e
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.c
> @@ -0,0 +1,165 @@
> +/* mach-rockchip/efuse.c
> + *
> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> + *
> + * Tmis program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * Tmis program is distributed in the hope that it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> + * tmis program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA

as far as i know this address is outdated and should be removed.

> + *
> + * Tme full GNU General Public License is included in this distribution in
> the
> + * file called LICENSE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>

Please order the includes alphabetical.

> +
> +#include "efuse.h"
> +
> +#define EFUSE_BUF_SIZE (32)
> +#define EFUSE_BUF_LKG_CPU (23)
> +#define EFUSE_BUF_LKG_GPU (24)
> +#define EFUSE_BUF_LKG_LOG (25)
> +
> +struct rk_efuse_info {
> + /* Platform device */
> + struct device *dev;

I think it's not really necessary to store this in the driver data. Better call
the relevant functions with struct platform_device as parameter and get your
driver data from their.

> +
> + /* Hardware resources */
> + void __iomem *regs;
> +
> + /* buffer to store registers' values */
> + unsigned int buf[EFUSE_BUF_SIZE];

The variable name buf isn't very helpful.

> +};
> +
> +static void efuse_writel(struct rk_efuse_info *efuse,
> + unsigned int value,
> + unsigned int offset)
> +{
> + writel_relaxed(value, efuse->regs + offset);
> +}
> +
> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> + unsigned int offset)
> +{
> + return readl_relaxed(efuse->regs + offset);
> +}
> +
> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> + int channel)
> +{
> + switch (channel) {
> + case EFUSE_BUF_LKG_CPU:
> + case EFUSE_BUF_LKG_GPU:
> + case EFUSE_BUF_LKG_LOG:
> + return efuse->buf[channel];
> + default:
> + dev_err(efuse->dev, "unknown channel\n");
> + return -EINVAL;

Returning a negative value in a function with unsigned return type isn't good.

Printing only "unknown channel" isn't optimal, it would be more helpful to print
also the invalid value.

> + }
> +
> + return 0;

Looks like unreachable code, maybe you could move the default case above down.

Thanks

Stefan

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Stefan Wahren]

Hi Jianqun,

> Jianqun Xu <jay.xu@rock-chips.com> hat am 1. Dezember 2014 um 08:34
> geschrieben:
>
>
> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> Driver will read fuse information of chip at the boot stage of
> kernel, this information new is for further usage.
>
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> ---
> arch/arm/mach-rockchip/efuse.c | 165 +++++++++++++++++++++++++++++++++++++++++
> arch/arm/mach-rockchip/efuse.h | 15 ++++
> 2 files changed, 180 insertions(+)
> create mode 100644 arch/arm/mach-rockchip/efuse.c
> create mode 100644 arch/arm/mach-rockchip/efuse.h
>
> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
> new file mode 100644
> index 0000000..326d81e
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/efuse.c
> @@ -0,0 +1,165 @@
> +/* mach-rockchip/efuse.c
> + *
> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> + *
> + * Tmis program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * Tmis program is distributed in the hope that it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> + * tmis program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA

as far as i know this address is outdated and should be removed.

> + *
> + * Tme full GNU General Public License is included in this distribution in
> the
> + * file called LICENSE.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>

Please order the includes alphabetical.

> +
> +#include "efuse.h"
> +
> +#define EFUSE_BUF_SIZE (32)
> +#define EFUSE_BUF_LKG_CPU (23)
> +#define EFUSE_BUF_LKG_GPU (24)
> +#define EFUSE_BUF_LKG_LOG (25)
> +
> +struct rk_efuse_info {
> + /* Platform device */
> + struct device *dev;

I think it's not really necessary to store this in the driver data. Better call
the relevant functions with struct platform_device as parameter and get your
driver data from their.

> +
> + /* Hardware resources */
> + void __iomem *regs;
> +
> + /* buffer to store registers' values */
> + unsigned int buf[EFUSE_BUF_SIZE];

The variable name buf isn't very helpful.

> +};
> +
> +static void efuse_writel(struct rk_efuse_info *efuse,
> + unsigned int value,
> + unsigned int offset)
> +{
> + writel_relaxed(value, efuse->regs + offset);
> +}
> +
> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> + unsigned int offset)
> +{
> + return readl_relaxed(efuse->regs + offset);
> +}
> +
> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> + int channel)
> +{
> + switch (channel) {
> + case EFUSE_BUF_LKG_CPU:
> + case EFUSE_BUF_LKG_GPU:
> + case EFUSE_BUF_LKG_LOG:
> + return efuse->buf[channel];
> + default:
> + dev_err(efuse->dev, "unknown channel\n");
> + return -EINVAL;

Returning a negative value in a function with unsigned return type isn't good.

Printing only "unknown channel" isn't optimal, it would be more helpful to print
also the invalid value.

> + }
> +
> + return 0;

Looks like unreachable code, maybe you could move the default case above down.

Thanks

Stefan

Re: [PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Jianqun]

Hi Heiko

在 12/01/2014 10:10 PM, Heiko Stübner 写道:
> Hi Jianqun,
> 
> Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
>> Add driver for efuse found on rk3288 board based on rk3288 SoC.
>> Driver will read fuse information of chip at the boot stage of
>> kernel, this information new is for further usage.
>>
>> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> 
> General question would be, what is the purpose of this driver?
This driver will get efuse information, and other module will use it for some useage,
such as dvfs will ajust OPP according to the differences between chips, that can make
chips run on a powersave status

Also can get the chip version... but this patch only show a part feathur

> I don't see any publically usable functions and the only thing happening is 
> the
> 	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
> output that emits some information from the efuse to the kernel log?
> 
can I make it a node under some debug directory ? For now only show it in boot message.
> 
> In the dt-binding doc you write:
>> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.
> 
> While the TRM also says this, IO_SECURITY is not mentioned anywhere else in 
> the document. Is this a pin or a bit somewhere in the GRF - i.e. something 
> whichs state is readable?
> 
> 
> Some more comments inline.
> 
>> ---
>>  arch/arm/mach-rockchip/efuse.c | 165
>> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h | 
>> 15 ++++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 arch/arm/mach-rockchip/efuse.c
>>  create mode 100644 arch/arm/mach-rockchip/efuse.h
>>
>> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
>> new file mode 100644
>> index 0000000..326d81e
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/efuse.c
> 
> a driver like this should probably live in something like 
> drivers/soc/rockchip.
> 
> 
>> @@ -0,0 +1,165 @@
>> +/* mach-rockchip/efuse.c
>> + *
>> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
>> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
>> + *
>> + * Tmis program is free software; you can redistribute it and/or modify it
>> + * under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * Tmis program is distributed in the hope that it will be useful, but
> 
> type Tmis -> This
> 
> 
>> WITHOUT + * ANY WARRANTY; without even the implied warranty of
>> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> General Public License for + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> with + * tmis program; if not, write to the Free Software Foundation, Inc.,
>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
>> + *
>> + * Tme full GNU General Public License is included in this distribution in
>> the + * file called LICENSE.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/io.h>
>> +
>> +#include "efuse.h"
>> +
>> +#define EFUSE_BUF_SIZE	(32)
>> +#define EFUSE_BUF_LKG_CPU	(23)
>> +#define EFUSE_BUF_LKG_GPU	(24)
>> +#define EFUSE_BUF_LKG_LOG	(25)
> 
> no braces needed for those numbers
> 
> 
>> +
>> +struct rk_efuse_info {
>> +	/* Platform device */
>> +	struct device *dev;
>> +
>> +	/* Hardware resources */
>> +	void __iomem *regs;
>> +
>> +	/* buffer to store registers' values */
>> +	unsigned int buf[EFUSE_BUF_SIZE];
>> +};
>> +
>> +static void efuse_writel(struct rk_efuse_info *efuse,
>> +			 unsigned int value,
>> +			 unsigned int offset)
>> +{
>> +	writel_relaxed(value, efuse->regs + offset);
>> +}
>> +
>> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
>> +				unsigned int offset)
>> +{
>> +	return readl_relaxed(efuse->regs + offset);
>> +}
> 
> why these indirections for readl and writel? They don't seem to provide any 
> additional benefit over calling writel_relaxed/readl_relaxed directly below.
> 
> 
>> +
>> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
>> +					 int channel)
>> +{
>> +	switch (channel) {
>> +	case EFUSE_BUF_LKG_CPU:
>> +	case EFUSE_BUF_LKG_GPU:
>> +	case EFUSE_BUF_LKG_LOG:
>> +		return efuse->buf[channel];
>> +	default:
>> +		dev_err(efuse->dev, "unknown channel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
>> +{
>> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
>> +}
>> +
>> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
>> +{
>> +	int start = 0;
>> +	int ret = 0;
>> +
>> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
>> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
>> +	udelay(2);
>> +
>> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
>> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
>> +			     REG_EFUSE_CTRL);
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
>> +			     REG_EFUSE_CTRL);
>> +		udelay(2);
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
>> +		udelay(2);
>> +
>> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
>> +
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
>> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
>> +		udelay(2);
>> +	}
>> +
>> +	udelay(2);
>> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +		     EFUSE_CSB, REG_EFUSE_CTRL);
>> +	udelay(2);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rockchip_efuse_probe(struct platform_device *pdev)
>> +{
>> +	struct rk_efuse_info *efuse;
>> +	struct resource *mem;
>> +	int ret = 0;
>> +
>> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
>> +	if (!efuse)
>> +		return -ENOMEM;
>> +
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
>> +	if (IS_ERR(efuse->regs))
>> +		return PTR_ERR(efuse->regs);
>> +
>> +	platform_set_drvdata(pdev, efuse);
>> +	efuse->dev = &pdev->dev;
>> +
>> +	ret = rockchip_efuse_init(efuse);
>> +	if (!ret)
>> +		rockchip_efuse_info(efuse);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id rockchip_efuse_match[] = {
>> +	{ .compatible = "rockchip,rk3288-efuse", },
> 
> what about other Rockchip SoCs? At least the rk3188 seems to contain an efuse 
> [though the TRM I have only directs to a RK3188 eFuse.pdf without describing 
> the component. So I don't know if it's the same type.
> 
> 
>> +	{},
>> +};
>> +
>> +static struct platform_driver rockchip_efuse_driver = {
>> +	.probe = rockchip_efuse_probe,
>> +	.driver = {
>> +		.name = "rk3288-efuse",
>> +		.owner = THIS_MODULE,
> 
> .owner gets already set through module_platform_driver
> 
> 
>> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
>> +	},
>> +};
>> +
>> +module_platform_driver(rockchip_efuse_driver);
>> +
>> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
>> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
>> new file mode 100644
>> index 0000000..3fdcf6d
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/efuse.h
> 
> why does this need to be a separate header? The stuff below could very well 
> simply live inside the fuse.c
> 
> 
>> @@ -0,0 +1,15 @@
>> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
>> +#define _ARCH_ROCKCHIP_EFUSE_H_
>> +
>> +/* Rockchip eFuse controller register */
>> +#define EFUSE_A_SHIFT		(6)
>> +#define EFUSE_A_MASK		(0x3FF)
>> +#define EFUSE_PGENB		(1 << 3)
> 
> please use BIT(3) instead of (1 << 3)
> Same for the bits below.
> 
> 
>> +#define EFUSE_LOAD		(1 << 2)
>> +#define EFUSE_STROBE		(1 << 1)
>> +#define EFUSE_CSB		(1 << 0)
>> +
>> +#define REG_EFUSE_CTRL		(0x0000)
>> +#define REG_EFUSE_DOUT		(0x0004)
> 
> no braces necessary for basic numerals
> 
> 
>> +
>> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */
> 
> 
> 
> 

-- 
Jianqun Xu

****************************************************************************
*IMPORTANT NOTICE:*This email is from Fuzhou Rockchip Electronics Co.,
Ltd .The contents of this email and any attachments may contain
information that is privileged, confidential and/or exempt from
disclosure under applicable law and relevant NDA. If you are not the
intended recipient, you are hereby notified that any disclosure,
copying, distribution, or use of the information is STRICTLY PROHIBITED.
Please immediately contact the sender as soon as possible and destroy
the material in its entirety in any format. Thank you.
****************************************************************************

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Jianqun]

Hi Heiko

? 12/01/2014 10:10 PM, Heiko St?bner ??:
> Hi Jianqun,
> 
> Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
>> Add driver for efuse found on rk3288 board based on rk3288 SoC.
>> Driver will read fuse information of chip at the boot stage of
>> kernel, this information new is for further usage.
>>
>> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> 
> General question would be, what is the purpose of this driver?
This driver will get efuse information, and other module will use it for some useage,
such as dvfs will ajust OPP according to the differences between chips, that can make
chips run on a powersave status

Also can get the chip version... but this patch only show a part feathur

> I don't see any publically usable functions and the only thing happening is 
> the
> 	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
> output that emits some information from the efuse to the kernel log?
> 
can I make it a node under some debug directory ? For now only show it in boot message.
> 
> In the dt-binding doc you write:
>> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is high.
> 
> While the TRM also says this, IO_SECURITY is not mentioned anywhere else in 
> the document. Is this a pin or a bit somewhere in the GRF - i.e. something 
> whichs state is readable?
> 
> 
> Some more comments inline.
> 
>> ---
>>  arch/arm/mach-rockchip/efuse.c | 165
>> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h | 
>> 15 ++++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 arch/arm/mach-rockchip/efuse.c
>>  create mode 100644 arch/arm/mach-rockchip/efuse.h
>>
>> diff --git a/arch/arm/mach-rockchip/efuse.c b/arch/arm/mach-rockchip/efuse.c
>> new file mode 100644
>> index 0000000..326d81e
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/efuse.c
> 
> a driver like this should probably live in something like 
> drivers/soc/rockchip.
> 
> 
>> @@ -0,0 +1,165 @@
>> +/* mach-rockchip/efuse.c
>> + *
>> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
>> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
>> + *
>> + * Tmis program is free software; you can redistribute it and/or modify it
>> + * under the terms of version 2 of the GNU General Public License as
>> + * published by the Free Software Foundation.
>> + *
>> + * Tmis program is distributed in the hope that it will be useful, but
> 
> type Tmis -> This
> 
> 
>> WITHOUT + * ANY WARRANTY; without even the implied warranty of
>> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> General Public License for + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> with + * tmis program; if not, write to the Free Software Foundation, Inc.,
>> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
>> + *
>> + * Tme full GNU General Public License is included in this distribution in
>> the + * file called LICENSE.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/io.h>
>> +
>> +#include "efuse.h"
>> +
>> +#define EFUSE_BUF_SIZE	(32)
>> +#define EFUSE_BUF_LKG_CPU	(23)
>> +#define EFUSE_BUF_LKG_GPU	(24)
>> +#define EFUSE_BUF_LKG_LOG	(25)
> 
> no braces needed for those numbers
> 
> 
>> +
>> +struct rk_efuse_info {
>> +	/* Platform device */
>> +	struct device *dev;
>> +
>> +	/* Hardware resources */
>> +	void __iomem *regs;
>> +
>> +	/* buffer to store registers' values */
>> +	unsigned int buf[EFUSE_BUF_SIZE];
>> +};
>> +
>> +static void efuse_writel(struct rk_efuse_info *efuse,
>> +			 unsigned int value,
>> +			 unsigned int offset)
>> +{
>> +	writel_relaxed(value, efuse->regs + offset);
>> +}
>> +
>> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
>> +				unsigned int offset)
>> +{
>> +	return readl_relaxed(efuse->regs + offset);
>> +}
> 
> why these indirections for readl and writel? They don't seem to provide any 
> additional benefit over calling writel_relaxed/readl_relaxed directly below.
> 
> 
>> +
>> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
>> +					 int channel)
>> +{
>> +	switch (channel) {
>> +	case EFUSE_BUF_LKG_CPU:
>> +	case EFUSE_BUF_LKG_GPU:
>> +	case EFUSE_BUF_LKG_LOG:
>> +		return efuse->buf[channel];
>> +	default:
>> +		dev_err(efuse->dev, "unknown channel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
>> +{
>> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
>> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
>> +}
>> +
>> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
>> +{
>> +	int start = 0;
>> +	int ret = 0;
>> +
>> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
>> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
>> +	udelay(2);
>> +
>> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
>> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
>> +			     REG_EFUSE_CTRL);
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
>> +			     REG_EFUSE_CTRL);
>> +		udelay(2);
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
>> +		udelay(2);
>> +
>> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
>> +
>> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
>> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
>> +		udelay(2);
>> +	}
>> +
>> +	udelay(2);
>> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
>> +		     EFUSE_CSB, REG_EFUSE_CTRL);
>> +	udelay(2);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rockchip_efuse_probe(struct platform_device *pdev)
>> +{
>> +	struct rk_efuse_info *efuse;
>> +	struct resource *mem;
>> +	int ret = 0;
>> +
>> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
>> +	if (!efuse)
>> +		return -ENOMEM;
>> +
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
>> +	if (IS_ERR(efuse->regs))
>> +		return PTR_ERR(efuse->regs);
>> +
>> +	platform_set_drvdata(pdev, efuse);
>> +	efuse->dev = &pdev->dev;
>> +
>> +	ret = rockchip_efuse_init(efuse);
>> +	if (!ret)
>> +		rockchip_efuse_info(efuse);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id rockchip_efuse_match[] = {
>> +	{ .compatible = "rockchip,rk3288-efuse", },
> 
> what about other Rockchip SoCs? At least the rk3188 seems to contain an efuse 
> [though the TRM I have only directs to a RK3188 eFuse.pdf without describing 
> the component. So I don't know if it's the same type.
> 
> 
>> +	{},
>> +};
>> +
>> +static struct platform_driver rockchip_efuse_driver = {
>> +	.probe = rockchip_efuse_probe,
>> +	.driver = {
>> +		.name = "rk3288-efuse",
>> +		.owner = THIS_MODULE,
> 
> .owner gets already set through module_platform_driver
> 
> 
>> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
>> +	},
>> +};
>> +
>> +module_platform_driver(rockchip_efuse_driver);
>> +
>> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
>> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/arch/arm/mach-rockchip/efuse.h b/arch/arm/mach-rockchip/efuse.h
>> new file mode 100644
>> index 0000000..3fdcf6d
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/efuse.h
> 
> why does this need to be a separate header? The stuff below could very well 
> simply live inside the fuse.c
> 
> 
>> @@ -0,0 +1,15 @@
>> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
>> +#define _ARCH_ROCKCHIP_EFUSE_H_
>> +
>> +/* Rockchip eFuse controller register */
>> +#define EFUSE_A_SHIFT		(6)
>> +#define EFUSE_A_MASK		(0x3FF)
>> +#define EFUSE_PGENB		(1 << 3)
> 
> please use BIT(3) instead of (1 << 3)
> Same for the bits below.
> 
> 
>> +#define EFUSE_LOAD		(1 << 2)
>> +#define EFUSE_STROBE		(1 << 1)
>> +#define EFUSE_CSB		(1 << 0)
>> +
>> +#define REG_EFUSE_CTRL		(0x0000)
>> +#define REG_EFUSE_DOUT		(0x0004)
> 
> no braces necessary for basic numerals
> 
> 
>> +
>> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */
> 
> 
> 
> 

-- 
Jianqun Xu

****************************************************************************
*IMPORTANT NOTICE:*This email is from Fuzhou Rockchip Electronics Co.,
Ltd .The contents of this email and any attachments may contain
information that is privileged, confidential and/or exempt from
disclosure under applicable law and relevant NDA. If you are not the
intended recipient, you are hereby notified that any disclosure,
copying, distribution, or use of the information is STRICTLY PROHIBITED.
Please immediately contact the sender as soon as possible and destroy
the material in its entirety in any format. Thank you.
****************************************************************************

Re: [PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Heiko Stübner]

Hi Jianqun,

Am Dienstag, 2. Dezember 2014, 23:04:57 schrieb Jianqun:
> 在 12/01/2014 10:10 PM, Heiko Stübner 写道:
> > Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
> >> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> >> Driver will read fuse information of chip at the boot stage of
> >> kernel, this information new is for further usage.
> >> 
> >> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> > 
> > General question would be, what is the purpose of this driver?
> 
> This driver will get efuse information, and other module will use it for
> some useage, such as dvfs will ajust OPP according to the differences
> between chips, that can make chips run on a powersave status
> 
> Also can get the chip version... but this patch only show a part feathur

just because I noticed this today, there seems to be an eeprom [0] and efuse 
[1] framework in the works. And as the mail from Maxime [2] suggests, both 
should probably merge.

So the rockchip efuse driver should probably the framework that will become of 
those two.


Heiko


[0] https://lkml.org/lkml/2015/2/19/307
[1] https://lkml.org/lkml/2015/2/25/173
[2] https://lkml.org/lkml/2015/2/25/191


> > I don't see any publically usable functions and the only thing happening
> > is
> > the
> > 
> > 	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
> > 
> > output that emits some information from the efuse to the kernel log?
> 
> can I make it a node under some debug directory ? For now only show it in
> boot message.
> > In the dt-binding doc you write:
> >> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is
> >> high.> 
> > While the TRM also says this, IO_SECURITY is not mentioned anywhere else
> > in
> > the document. Is this a pin or a bit somewhere in the GRF - i.e. something
> > whichs state is readable?
> > 
> > 
> > Some more comments inline.
> > 
> >> ---
> >> 
> >>  arch/arm/mach-rockchip/efuse.c | 165
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h
> >> |
> >> 15 ++++
> >> 
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.c
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.h
> >> 
> >> diff --git a/arch/arm/mach-rockchip/efuse.c
> >> b/arch/arm/mach-rockchip/efuse.c new file mode 100644
> >> index 0000000..326d81e
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.c
> > 
> > a driver like this should probably live in something like
> > drivers/soc/rockchip.
> > 
> >> @@ -0,0 +1,165 @@
> >> +/* mach-rockchip/efuse.c
> >> + *
> >> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> >> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> >> + *
> >> + * Tmis program is free software; you can redistribute it and/or modify
> >> it
> >> + * under the terms of version 2 of the GNU General Public License as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Tmis program is distributed in the hope that it will be useful, but
> > 
> > type Tmis -> This
> > 
> >> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> >> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> General Public License for + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> along
> >> with + * tmis program; if not, write to the Free Software Foundation,
> >> Inc.,
> >> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
> >> + *
> >> + * Tme full GNU General Public License is included in this distribution
> >> in
> >> the + * file called LICENSE.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/device.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/io.h>
> >> +
> >> +#include "efuse.h"
> >> +
> >> +#define EFUSE_BUF_SIZE	(32)
> >> +#define EFUSE_BUF_LKG_CPU	(23)
> >> +#define EFUSE_BUF_LKG_GPU	(24)
> >> +#define EFUSE_BUF_LKG_LOG	(25)
> > 
> > no braces needed for those numbers
> > 
> >> +
> >> +struct rk_efuse_info {
> >> +	/* Platform device */
> >> +	struct device *dev;
> >> +
> >> +	/* Hardware resources */
> >> +	void __iomem *regs;
> >> +
> >> +	/* buffer to store registers' values */
> >> +	unsigned int buf[EFUSE_BUF_SIZE];
> >> +};
> >> +
> >> +static void efuse_writel(struct rk_efuse_info *efuse,
> >> +			 unsigned int value,
> >> +			 unsigned int offset)
> >> +{
> >> +	writel_relaxed(value, efuse->regs + offset);
> >> +}
> >> +
> >> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> >> +				unsigned int offset)
> >> +{
> >> +	return readl_relaxed(efuse->regs + offset);
> >> +}
> > 
> > why these indirections for readl and writel? They don't seem to provide
> > any
> > additional benefit over calling writel_relaxed/readl_relaxed directly
> > below.> 
> >> +
> >> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> >> +					 int channel)
> >> +{
> >> +	switch (channel) {
> >> +	case EFUSE_BUF_LKG_CPU:
> >> +	case EFUSE_BUF_LKG_GPU:
> >> +	case EFUSE_BUF_LKG_LOG:
> >> +		return efuse->buf[channel];
> >> +	default:
> >> +		dev_err(efuse->dev, "unknown channel\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
> >> +{
> >> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
> >> +}
> >> +
> >> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
> >> +{
> >> +	int start = 0;
> >> +	int ret = 0;
> >> +
> >> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
> >> +			     REG_EFUSE_CTRL);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
> >> +			     REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +
> >> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
> >> +
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +	}
> >> +
> >> +	udelay(2);
> >> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +		     EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rockchip_efuse_probe(struct platform_device *pdev)
> >> +{
> >> +	struct rk_efuse_info *efuse;
> >> +	struct resource *mem;
> >> +	int ret = 0;
> >> +
> >> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> >> +	if (!efuse)
> >> +		return -ENOMEM;
> >> +
> >> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
> >> +	if (IS_ERR(efuse->regs))
> >> +		return PTR_ERR(efuse->regs);
> >> +
> >> +	platform_set_drvdata(pdev, efuse);
> >> +	efuse->dev = &pdev->dev;
> >> +
> >> +	ret = rockchip_efuse_init(efuse);
> >> +	if (!ret)
> >> +		rockchip_efuse_info(efuse);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static const struct of_device_id rockchip_efuse_match[] = {
> >> +	{ .compatible = "rockchip,rk3288-efuse", },
> > 
> > what about other Rockchip SoCs? At least the rk3188 seems to contain an
> > efuse [though the TRM I have only directs to a RK3188 eFuse.pdf without
> > describing the component. So I don't know if it's the same type.
> > 
> >> +	{},
> >> +};
> >> +
> >> +static struct platform_driver rockchip_efuse_driver = {
> >> +	.probe = rockchip_efuse_probe,
> >> +	.driver = {
> >> +		.name = "rk3288-efuse",
> >> +		.owner = THIS_MODULE,
> > 
> > .owner gets already set through module_platform_driver
> > 
> >> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
> >> +	},
> >> +};
> >> +
> >> +module_platform_driver(rockchip_efuse_driver);
> >> +
> >> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
> >> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/arch/arm/mach-rockchip/efuse.h
> >> b/arch/arm/mach-rockchip/efuse.h new file mode 100644
> >> index 0000000..3fdcf6d
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.h
> > 
> > why does this need to be a separate header? The stuff below could very
> > well
> > simply live inside the fuse.c
> > 
> >> @@ -0,0 +1,15 @@
> >> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
> >> +#define _ARCH_ROCKCHIP_EFUSE_H_
> >> +
> >> +/* Rockchip eFuse controller register */
> >> +#define EFUSE_A_SHIFT		(6)
> >> +#define EFUSE_A_MASK		(0x3FF)
> >> +#define EFUSE_PGENB		(1 << 3)
> > 
> > please use BIT(3) instead of (1 << 3)
> > Same for the bits below.
> > 
> >> +#define EFUSE_LOAD		(1 << 2)
> >> +#define EFUSE_STROBE		(1 << 1)
> >> +#define EFUSE_CSB		(1 << 0)
> >> +
> >> +#define REG_EFUSE_CTRL		(0x0000)
> >> +#define REG_EFUSE_DOUT		(0x0004)
> > 
> > no braces necessary for basic numerals
> > 
> >> +
> >> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */

Re: [PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Heiko Stübner]

Hi Jianqun,

Am Dienstag, 2. Dezember 2014, 23:04:57 schrieb Jianqun:
> 在 12/01/2014 10:10 PM, Heiko Stübner 写道:
> > Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
> >> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> >> Driver will read fuse information of chip at the boot stage of
> >> kernel, this information new is for further usage.
> >> 
> >> Signed-off-by: Jianqun Xu <jay.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > 
> > General question would be, what is the purpose of this driver?
> 
> This driver will get efuse information, and other module will use it for
> some useage, such as dvfs will ajust OPP according to the differences
> between chips, that can make chips run on a powersave status
> 
> Also can get the chip version... but this patch only show a part feathur

just because I noticed this today, there seems to be an eeprom [0] and efuse 
[1] framework in the works. And as the mail from Maxime [2] suggests, both 
should probably merge.

So the rockchip efuse driver should probably the framework that will become of 
those two.


Heiko


[0] https://lkml.org/lkml/2015/2/19/307
[1] https://lkml.org/lkml/2015/2/25/173
[2] https://lkml.org/lkml/2015/2/25/191


> > I don't see any publically usable functions and the only thing happening
> > is
> > the
> > 
> > 	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
> > 
> > output that emits some information from the efuse to the kernel log?
> 
> can I make it a node under some debug directory ? For now only show it in
> boot message.
> > In the dt-binding doc you write:
> >> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is
> >> high.> 
> > While the TRM also says this, IO_SECURITY is not mentioned anywhere else
> > in
> > the document. Is this a pin or a bit somewhere in the GRF - i.e. something
> > whichs state is readable?
> > 
> > 
> > Some more comments inline.
> > 
> >> ---
> >> 
> >>  arch/arm/mach-rockchip/efuse.c | 165
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h
> >> |
> >> 15 ++++
> >> 
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.c
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.h
> >> 
> >> diff --git a/arch/arm/mach-rockchip/efuse.c
> >> b/arch/arm/mach-rockchip/efuse.c new file mode 100644
> >> index 0000000..326d81e
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.c
> > 
> > a driver like this should probably live in something like
> > drivers/soc/rockchip.
> > 
> >> @@ -0,0 +1,165 @@
> >> +/* mach-rockchip/efuse.c
> >> + *
> >> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> >> + * Author: Jianqun Xu <jay.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >> + *
> >> + * Tmis program is free software; you can redistribute it and/or modify
> >> it
> >> + * under the terms of version 2 of the GNU General Public License as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Tmis program is distributed in the hope that it will be useful, but
> > 
> > type Tmis -> This
> > 
> >> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> >> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> General Public License for + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> along
> >> with + * tmis program; if not, write to the Free Software Foundation,
> >> Inc.,
> >> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
> >> + *
> >> + * Tme full GNU General Public License is included in this distribution
> >> in
> >> the + * file called LICENSE.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/device.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/io.h>
> >> +
> >> +#include "efuse.h"
> >> +
> >> +#define EFUSE_BUF_SIZE	(32)
> >> +#define EFUSE_BUF_LKG_CPU	(23)
> >> +#define EFUSE_BUF_LKG_GPU	(24)
> >> +#define EFUSE_BUF_LKG_LOG	(25)
> > 
> > no braces needed for those numbers
> > 
> >> +
> >> +struct rk_efuse_info {
> >> +	/* Platform device */
> >> +	struct device *dev;
> >> +
> >> +	/* Hardware resources */
> >> +	void __iomem *regs;
> >> +
> >> +	/* buffer to store registers' values */
> >> +	unsigned int buf[EFUSE_BUF_SIZE];
> >> +};
> >> +
> >> +static void efuse_writel(struct rk_efuse_info *efuse,
> >> +			 unsigned int value,
> >> +			 unsigned int offset)
> >> +{
> >> +	writel_relaxed(value, efuse->regs + offset);
> >> +}
> >> +
> >> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> >> +				unsigned int offset)
> >> +{
> >> +	return readl_relaxed(efuse->regs + offset);
> >> +}
> > 
> > why these indirections for readl and writel? They don't seem to provide
> > any
> > additional benefit over calling writel_relaxed/readl_relaxed directly
> > below.> 
> >> +
> >> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> >> +					 int channel)
> >> +{
> >> +	switch (channel) {
> >> +	case EFUSE_BUF_LKG_CPU:
> >> +	case EFUSE_BUF_LKG_GPU:
> >> +	case EFUSE_BUF_LKG_LOG:
> >> +		return efuse->buf[channel];
> >> +	default:
> >> +		dev_err(efuse->dev, "unknown channel\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
> >> +{
> >> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
> >> +}
> >> +
> >> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
> >> +{
> >> +	int start = 0;
> >> +	int ret = 0;
> >> +
> >> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
> >> +			     REG_EFUSE_CTRL);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
> >> +			     REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +
> >> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
> >> +
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +	}
> >> +
> >> +	udelay(2);
> >> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +		     EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rockchip_efuse_probe(struct platform_device *pdev)
> >> +{
> >> +	struct rk_efuse_info *efuse;
> >> +	struct resource *mem;
> >> +	int ret = 0;
> >> +
> >> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> >> +	if (!efuse)
> >> +		return -ENOMEM;
> >> +
> >> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
> >> +	if (IS_ERR(efuse->regs))
> >> +		return PTR_ERR(efuse->regs);
> >> +
> >> +	platform_set_drvdata(pdev, efuse);
> >> +	efuse->dev = &pdev->dev;
> >> +
> >> +	ret = rockchip_efuse_init(efuse);
> >> +	if (!ret)
> >> +		rockchip_efuse_info(efuse);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static const struct of_device_id rockchip_efuse_match[] = {
> >> +	{ .compatible = "rockchip,rk3288-efuse", },
> > 
> > what about other Rockchip SoCs? At least the rk3188 seems to contain an
> > efuse [though the TRM I have only directs to a RK3188 eFuse.pdf without
> > describing the component. So I don't know if it's the same type.
> > 
> >> +	{},
> >> +};
> >> +
> >> +static struct platform_driver rockchip_efuse_driver = {
> >> +	.probe = rockchip_efuse_probe,
> >> +	.driver = {
> >> +		.name = "rk3288-efuse",
> >> +		.owner = THIS_MODULE,
> > 
> > .owner gets already set through module_platform_driver
> > 
> >> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
> >> +	},
> >> +};
> >> +
> >> +module_platform_driver(rockchip_efuse_driver);
> >> +
> >> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
> >> +MODULE_AUTHOR("Jianqun Xu <jay.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/arch/arm/mach-rockchip/efuse.h
> >> b/arch/arm/mach-rockchip/efuse.h new file mode 100644
> >> index 0000000..3fdcf6d
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.h
> > 
> > why does this need to be a separate header? The stuff below could very
> > well
> > simply live inside the fuse.c
> > 
> >> @@ -0,0 +1,15 @@
> >> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
> >> +#define _ARCH_ROCKCHIP_EFUSE_H_
> >> +
> >> +/* Rockchip eFuse controller register */
> >> +#define EFUSE_A_SHIFT		(6)
> >> +#define EFUSE_A_MASK		(0x3FF)
> >> +#define EFUSE_PGENB		(1 << 3)
> > 
> > please use BIT(3) instead of (1 << 3)
> > Same for the bits below.
> > 
> >> +#define EFUSE_LOAD		(1 << 2)
> >> +#define EFUSE_STROBE		(1 << 1)
> >> +#define EFUSE_CSB		(1 << 0)
> >> +
> >> +#define REG_EFUSE_CTRL		(0x0000)
> >> +#define REG_EFUSE_DOUT		(0x0004)
> > 
> > no braces necessary for basic numerals
> > 
> >> +
> >> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] rockchip: efuse: add efuse driver for rk3288 efuse

[Heiko Stübner]

Hi Jianqun,

Am Dienstag, 2. Dezember 2014, 23:04:57 schrieb Jianqun:
> ? 12/01/2014 10:10 PM, Heiko St?bner ??:
> > Am Montag, 1. Dezember 2014, 15:34:41 schrieb Jianqun Xu:
> >> Add driver for efuse found on rk3288 board based on rk3288 SoC.
> >> Driver will read fuse information of chip at the boot stage of
> >> kernel, this information new is for further usage.
> >> 
> >> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> > 
> > General question would be, what is the purpose of this driver?
> 
> This driver will get efuse information, and other module will use it for
> some useage, such as dvfs will ajust OPP according to the differences
> between chips, that can make chips run on a powersave status
> 
> Also can get the chip version... but this patch only show a part feathur

just because I noticed this today, there seems to be an eeprom [0] and efuse 
[1] framework in the works. And as the mail from Maxime [2] suggests, both 
should probably merge.

So the rockchip efuse driver should probably the framework that will become of 
those two.


Heiko


[0] https://lkml.org/lkml/2015/2/19/307
[1] https://lkml.org/lkml/2015/2/25/173
[2] https://lkml.org/lkml/2015/2/25/191


> > I don't see any publically usable functions and the only thing happening
> > is
> > the
> > 
> > 	dev_info(efuse->dev, "leakage (%d %d %d)\n",...
> > 
> > output that emits some information from the efuse to the kernel log?
> 
> can I make it a node under some debug directory ? For now only show it in
> boot message.
> > In the dt-binding doc you write:
> >> The 32x32 eFuse can only be accessed by APB bus when IO_SECURITYsel is
> >> high.> 
> > While the TRM also says this, IO_SECURITY is not mentioned anywhere else
> > in
> > the document. Is this a pin or a bit somewhere in the GRF - i.e. something
> > whichs state is readable?
> > 
> > 
> > Some more comments inline.
> > 
> >> ---
> >> 
> >>  arch/arm/mach-rockchip/efuse.c | 165
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++ arch/arm/mach-rockchip/efuse.h
> >> |
> >> 15 ++++
> >> 
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.c
> >>  create mode 100644 arch/arm/mach-rockchip/efuse.h
> >> 
> >> diff --git a/arch/arm/mach-rockchip/efuse.c
> >> b/arch/arm/mach-rockchip/efuse.c new file mode 100644
> >> index 0000000..326d81e
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.c
> > 
> > a driver like this should probably live in something like
> > drivers/soc/rockchip.
> > 
> >> @@ -0,0 +1,165 @@
> >> +/* mach-rockchip/efuse.c
> >> + *
> >> + * Copyright (c) 2014 Rockchip Electronics Co. Ltd.
> >> + * Author: Jianqun Xu <jay.xu@rock-chips.com>
> >> + *
> >> + * Tmis program is free software; you can redistribute it and/or modify
> >> it
> >> + * under the terms of version 2 of the GNU General Public License as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Tmis program is distributed in the hope that it will be useful, but
> > 
> > type Tmis -> This
> > 
> >> WITHOUT + * ANY WARRANTY; without even the implied warranty of
> >> MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> General Public License for + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> along
> >> with + * tmis program; if not, write to the Free Software Foundation,
> >> Inc.,
> >> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110, USA
> >> + *
> >> + * Tme full GNU General Public License is included in this distribution
> >> in
> >> the + * file called LICENSE.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/device.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/io.h>
> >> +
> >> +#include "efuse.h"
> >> +
> >> +#define EFUSE_BUF_SIZE	(32)
> >> +#define EFUSE_BUF_LKG_CPU	(23)
> >> +#define EFUSE_BUF_LKG_GPU	(24)
> >> +#define EFUSE_BUF_LKG_LOG	(25)
> > 
> > no braces needed for those numbers
> > 
> >> +
> >> +struct rk_efuse_info {
> >> +	/* Platform device */
> >> +	struct device *dev;
> >> +
> >> +	/* Hardware resources */
> >> +	void __iomem *regs;
> >> +
> >> +	/* buffer to store registers' values */
> >> +	unsigned int buf[EFUSE_BUF_SIZE];
> >> +};
> >> +
> >> +static void efuse_writel(struct rk_efuse_info *efuse,
> >> +			 unsigned int value,
> >> +			 unsigned int offset)
> >> +{
> >> +	writel_relaxed(value, efuse->regs + offset);
> >> +}
> >> +
> >> +static unsigned int efuse_readl(struct rk_efuse_info *efuse,
> >> +				unsigned int offset)
> >> +{
> >> +	return readl_relaxed(efuse->regs + offset);
> >> +}
> > 
> > why these indirections for readl and writel? They don't seem to provide
> > any
> > additional benefit over calling writel_relaxed/readl_relaxed directly
> > below.> 
> >> +
> >> +static unsigned int rockchip_efuse_leakage(struct rk_efuse_info *efuse,
> >> +					 int channel)
> >> +{
> >> +	switch (channel) {
> >> +	case EFUSE_BUF_LKG_CPU:
> >> +	case EFUSE_BUF_LKG_GPU:
> >> +	case EFUSE_BUF_LKG_LOG:
> >> +		return efuse->buf[channel];
> >> +	default:
> >> +		dev_err(efuse->dev, "unknown channel\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void rockchip_efuse_info(struct rk_efuse_info *efuse)
> >> +{
> >> +	dev_info(efuse->dev, "leakage (%d %d %d)\n",
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_CPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_GPU),
> >> +		 rockchip_efuse_leakage(efuse, EFUSE_BUF_LKG_LOG));
> >> +}
> >> +
> >> +static int rockchip_efuse_init(struct rk_efuse_info *efuse)
> >> +{
> >> +	int start = 0;
> >> +	int ret = 0;
> >> +
> >> +	efuse_writel(efuse, EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	efuse_writel(efuse, EFUSE_LOAD | EFUSE_PGENB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	for (start = 0; start <= EFUSE_BUF_SIZE; start++) {
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~(EFUSE_A_MASK << EFUSE_A_SHIFT)),
> >> +			     REG_EFUSE_CTRL);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     ((start & EFUSE_A_MASK) << EFUSE_A_SHIFT),
> >> +			     REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +			     EFUSE_STROBE, REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +
> >> +		efuse->buf[start] = efuse_readl(efuse, REG_EFUSE_DOUT);
> >> +
> >> +		efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) &
> >> +			     (~EFUSE_STROBE), REG_EFUSE_CTRL);
> >> +		udelay(2);
> >> +	}
> >> +
> >> +	udelay(2);
> >> +	efuse_writel(efuse, efuse_readl(efuse, REG_EFUSE_CTRL) |
> >> +		     EFUSE_CSB, REG_EFUSE_CTRL);
> >> +	udelay(2);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rockchip_efuse_probe(struct platform_device *pdev)
> >> +{
> >> +	struct rk_efuse_info *efuse;
> >> +	struct resource *mem;
> >> +	int ret = 0;
> >> +
> >> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> >> +	if (!efuse)
> >> +		return -ENOMEM;
> >> +
> >> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	efuse->regs = devm_ioremap_resource(&pdev->dev, mem);
> >> +	if (IS_ERR(efuse->regs))
> >> +		return PTR_ERR(efuse->regs);
> >> +
> >> +	platform_set_drvdata(pdev, efuse);
> >> +	efuse->dev = &pdev->dev;
> >> +
> >> +	ret = rockchip_efuse_init(efuse);
> >> +	if (!ret)
> >> +		rockchip_efuse_info(efuse);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static const struct of_device_id rockchip_efuse_match[] = {
> >> +	{ .compatible = "rockchip,rk3288-efuse", },
> > 
> > what about other Rockchip SoCs? At least the rk3188 seems to contain an
> > efuse [though the TRM I have only directs to a RK3188 eFuse.pdf without
> > describing the component. So I don't know if it's the same type.
> > 
> >> +	{},
> >> +};
> >> +
> >> +static struct platform_driver rockchip_efuse_driver = {
> >> +	.probe = rockchip_efuse_probe,
> >> +	.driver = {
> >> +		.name = "rk3288-efuse",
> >> +		.owner = THIS_MODULE,
> > 
> > .owner gets already set through module_platform_driver
> > 
> >> +		.of_match_table = of_match_ptr(rockchip_efuse_match),
> >> +	},
> >> +};
> >> +
> >> +module_platform_driver(rockchip_efuse_driver);
> >> +
> >> +MODULE_DESCRIPTION("Rockchip eFuse Driver");
> >> +MODULE_AUTHOR("Jianqun Xu <jay.xu@rock-chips.com>");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/arch/arm/mach-rockchip/efuse.h
> >> b/arch/arm/mach-rockchip/efuse.h new file mode 100644
> >> index 0000000..3fdcf6d
> >> --- /dev/null
> >> +++ b/arch/arm/mach-rockchip/efuse.h
> > 
> > why does this need to be a separate header? The stuff below could very
> > well
> > simply live inside the fuse.c
> > 
> >> @@ -0,0 +1,15 @@
> >> +#ifndef _ARCH_ROCKCHIP_EFUSE_H_
> >> +#define _ARCH_ROCKCHIP_EFUSE_H_
> >> +
> >> +/* Rockchip eFuse controller register */
> >> +#define EFUSE_A_SHIFT		(6)
> >> +#define EFUSE_A_MASK		(0x3FF)
> >> +#define EFUSE_PGENB		(1 << 3)
> > 
> > please use BIT(3) instead of (1 << 3)
> > Same for the bits below.
> > 
> >> +#define EFUSE_LOAD		(1 << 2)
> >> +#define EFUSE_STROBE		(1 << 1)
> >> +#define EFUSE_CSB		(1 << 0)
> >> +
> >> +#define REG_EFUSE_CTRL		(0x0000)
> >> +#define REG_EFUSE_DOUT		(0x0004)
> > 
> > no braces necessary for basic numerals
> > 
> >> +
> >> +#endif /* _ARCH_ROCKCHIP_EFUSE_H_ */