[PATCH 0/4] Amlogic Meson6/Meson8/Meson8b efuse support

[PATCH 0/4] Amlogic Meson6/Meson8/Meson8b efuse support

[Martin Blumenstingl]

This series implements a driver for the efuse found on the Amlogic
Meson6/Meson8/Meson8b SoCs.
A new driver is needed because the existing "meson-efuse" driver
communicates through the "secure monitor" firmware with the HW,
while these older SoCs access the hardware registers directly.

An additional patch also explicitly states that the existing
"meson-efuse" driver is only for the Meson GX SoCs by adjusting
the dt-binding docs, Kconfig help texts, etc.

Please note that the dts patch ("ARM: dts: meson: add the efuse node")
is built on top of my other series ("soc: amlogic: Add Meson6/8/8b SoC
Information driver") from: [0]


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-September/004772.html


Martin Blumenstingl (4):
  dt-bindings: nvmem: Describe the Amlogic Meson6/Meson8/Meson8b efuse
  nvmem: meson-efuse: indicate that this driver is only for Meson GX
    SoCs
  nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs
  ARM: dts: meson: add the efuse node

 .../devicetree/bindings/nvmem/amlogic-efuse.txt    |   2 +-
 .../bindings/nvmem/amlogic-meson-mx-efuse.txt      |  22 ++
 arch/arm/boot/dts/meson.dtsi                       |  13 +
 arch/arm/boot/dts/meson6.dtsi                      |   3 +
 arch/arm/boot/dts/meson8.dtsi                      |   6 +
 arch/arm/boot/dts/meson8b.dtsi                     |   7 +
 drivers/nvmem/Kconfig                              |  14 +-
 drivers/nvmem/Makefile                             |   2 +
 drivers/nvmem/meson-efuse.c                        |   4 +-
 drivers/nvmem/meson-mx-efuse.c                     | 272 +++++++++++++++++++++
 10 files changed, 340 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/amlogic-meson-mx-efuse.txt
 create mode 100644 drivers/nvmem/meson-mx-efuse.c

-- 
2.14.2

[PATCH 1/4] dt-bindings: nvmem: Describe the Amlogic Meson6/Meson8/Meson8b efuse

[Martin Blumenstingl]

Amlogic Meson6, Meson8 and Meson8b SoCs have an efuse which contains
calibration data from the factory (for the internal temperature sensor
and some CVBS connector settings). Some manufacturers also store the MAC
address or serial number in the efuse.

This documents the devicetree bindings for the efuse on these SoCs.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/nvmem/amlogic-meson-mx-efuse.txt      | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/amlogic-meson-mx-efuse.txt

diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-meson-mx-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-meson-mx-efuse.txt
new file mode 100644
index 000000000000..a3c63954a1a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/amlogic-meson-mx-efuse.txt
@@ -0,0 +1,22 @@
+Amlogic Meson6/Meson8/Meson8b efuse
+
+Required Properties:
+- compatible: depending on the SoC this should be one of:
+	- "amlogic,meson6-efuse"
+	- "amlogic,meson8-efuse"
+	- "amlogic,meson8b-efuse"
+- reg: base address and size of the efuse registers
+- clocks: a reference to the efuse core gate clock
+- clock-names: must be "core"
+
+All properties and sub-nodes as well as the consumer bindings
+defined in nvmem.txt in this directory are also supported.
+
+
+Example:
+	efuse: nvmem at 0 {
+		compatible = "amlogic,meson8-efuse";
+		reg = <0x0 0x2000>;
+		clocks = <&clkc CLKID_EFUSE>;
+		clock-names = "core";
+	};
-- 
2.14.2

[PATCH 2/4] nvmem: meson-efuse: indicate that this driver is only for Meson GX SoCs

[Martin Blumenstingl]

The current Amlogic Meson eFuse driver only supports the 64-bit SoCs
(GXBB and newer). Older SoCs cannot be supported by the same driver
because they do not use the meson secure monitor firmware to access the
hardware.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt | 2 +-
 drivers/nvmem/Kconfig                                     | 4 ++--
 drivers/nvmem/meson-efuse.c                               | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
index fafd85bd67a6..e3298e18de26 100644
--- a/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
+++ b/Documentation/devicetree/bindings/nvmem/amlogic-efuse.txt
@@ -1,4 +1,4 @@
-= Amlogic eFuse device tree bindings =
+= Amlogic Meson GX eFuse device tree bindings =
 
 Required properties:
 - compatible: should be "amlogic,meson-gxbb-efuse"
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 101ced4c84be..a21a781f587d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -135,11 +135,11 @@ config NVMEM_VF610_OCOTP
 	  be called nvmem-vf610-ocotp.
 
 config MESON_EFUSE
-	tristate "Amlogic eFuse Support"
+	tristate "Amlogic Meson GX eFuse Support"
 	depends on (ARCH_MESON || COMPILE_TEST) && MESON_SM
 	help
 	  This is a driver to retrieve specific values from the eFuse found on
-	  the Amlogic Meson SoCs.
+	  the Amlogic Meson GX SoCs.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem_meson_efuse.
diff --git a/drivers/nvmem/meson-efuse.c b/drivers/nvmem/meson-efuse.c
index 70bfc9839bb2..1ea3cd24a508 100644
--- a/drivers/nvmem/meson-efuse.c
+++ b/drivers/nvmem/meson-efuse.c
@@ -1,5 +1,5 @@
 /*
- * Amlogic eFuse Driver
+ * Amlogic Meson GX eFuse Driver
  *
  * Copyright (c) 2016 Endless Computers, Inc.
  * Author: Carlo Caione <carlo@endlessm.com>
@@ -89,5 +89,5 @@ static struct platform_driver meson_efuse_driver = {
 module_platform_driver(meson_efuse_driver);
 
 MODULE_AUTHOR("Carlo Caione <carlo@endlessm.com>");
-MODULE_DESCRIPTION("Amlogic Meson NVMEM driver");
+MODULE_DESCRIPTION("Amlogic Meson GX NVMEM driver");
 MODULE_LICENSE("GPL v2");
-- 
2.14.2

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Martin Blumenstingl]

This adds a driver to access the efuse on Amlogic Meson6, Meson8 and
Meson8b SoCs.
These SoCs are accessing the efuse IP block directly through the
registers in the "secbus" region. This makes it different from the Meson
GX efuse driver which uses the "secure monitor" firmware to access the
efuse.

The efuse on Meson6 can only read one byte at a time, while the efuse on
Meson8 and Meson8b always reads 4 bytes at a time. The new driver
supports both, but due to lack of hardware Meson6 support was not tested.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/nvmem/Kconfig          |  10 ++
 drivers/nvmem/Makefile         |   2 +
 drivers/nvmem/meson-mx-efuse.c | 272 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 284 insertions(+)
 create mode 100644 drivers/nvmem/meson-mx-efuse.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index a21a781f587d..11f1313a03de 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -144,4 +144,14 @@ config MESON_EFUSE
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem_meson_efuse.
 
+config MESON_MX_EFUSE
+	tristate "Amlogic Meson6/Meson8/Meson8b eFuse Support"
+	depends on ARCH_MESON || COMPILE_TEST
+	help
+	  This is a driver to retrieve specific values from the eFuse found on
+	  the Amlogic Meson6, Meson8 and Meson8b SoCs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem_meson_mx_efuse.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 173140658693..1c2edf869f26 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -30,3 +30,5 @@ obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
 nvmem-vf610-ocotp-y		:= vf610-ocotp.o
 obj-$(CONFIG_MESON_EFUSE)	+= nvmem_meson_efuse.o
 nvmem_meson_efuse-y		:= meson-efuse.o
+obj-$(CONFIG_MESON_MX_EFUSE)	+= nvmem_meson_mx_efuse.o
+nvmem_meson_mx_efuse-y		:= meson-mx-efuse.o
diff --git a/drivers/nvmem/meson-mx-efuse.c b/drivers/nvmem/meson-mx-efuse.c
new file mode 100644
index 000000000000..b7c75f3eadce
--- /dev/null
+++ b/drivers/nvmem/meson-mx-efuse.c
@@ -0,0 +1,272 @@
+/*
+ * Amlogic Meson6, Meson8 and Meson8b eFuse Driver
+ *
+ * Copyright (c) 2017 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ *
+ * This 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.
+ *
+ * This 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.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+
+#define MESON_MX_EFUSE_CNTL1					0x04
+#define MESON_MX_EFUSE_CNTL1_PD_ENABLE				BIT(27)
+#define MESON_MX_EFUSE_CNTL1_AUTO_RD_BUSY			BIT(26)
+#define MESON_MX_EFUSE_CNTL1_AUTO_RD_START			BIT(25)
+#define MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE			BIT(24)
+#define MESON_MX_EFUSE_CNTL1_BYTE_WR_DATA			GENMASK(23, 16)
+#define MESON_MX_EFUSE_CNTL1_AUTO_WR_BUSY			BIT(14)
+#define MESON_MX_EFUSE_CNTL1_AUTO_WR_START			BIT(13)
+#define MESON_MX_EFUSE_CNTL1_AUTO_WR_ENABLE			BIT(12)
+#define MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET			BIT(11)
+#define MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK			GENMASK(10, 0)
+
+#define MESON_MX_EFUSE_CNTL2					0x08
+
+#define MESON_MX_EFUSE_CNTL4					0x10
+#define MESON_MX_EFUSE_CNTL4_ENCRYPT_ENABLE			BIT(10)
+
+struct meson_mx_efuse_platform_data {
+	const char *name;
+	unsigned int word_size;
+};
+
+struct meson_mx_efuse {
+	void __iomem *base;
+	struct clk *core_clk;
+	struct nvmem_device *nvmem;
+	struct nvmem_config config;
+};
+
+static void meson_mx_efuse_mask_bits(struct meson_mx_efuse *efuse, u32 reg,
+				     u32 mask, u32 set)
+{
+	u32 data;
+
+	data = readl(efuse->base + reg);
+	data &= ~mask;
+	data |= (set & mask);
+
+	writel(data, efuse->base + reg);
+}
+
+static int meson_mx_efuse_hw_enable(struct meson_mx_efuse *efuse)
+{
+	int err;
+
+	err = clk_prepare_enable(efuse->core_clk);
+	if (err)
+		return err;
+
+	/* power up the efuse */
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_PD_ENABLE, 0);
+
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL4,
+				 MESON_MX_EFUSE_CNTL4_ENCRYPT_ENABLE, 0);
+
+	return 0;
+}
+
+static void meson_mx_efuse_hw_disable(struct meson_mx_efuse *efuse)
+{
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_PD_ENABLE,
+				 MESON_MX_EFUSE_CNTL1_PD_ENABLE);
+
+	clk_disable_unprepare(efuse->core_clk);
+}
+
+static int meson_mx_efuse_read_addr(struct meson_mx_efuse *efuse,
+				    unsigned int addr, u32 *value)
+{
+	int err;
+	u32 regval;
+
+	/* write the address to read */
+	regval = FIELD_PREP(MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, addr);
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, regval);
+
+	/* inform the hardware that we changed the address */
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET,
+				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET);
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET, 0);
+
+	/* start the read process */
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START);
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START, 0);
+
+	/*
+	 * perform a dummy read to ensure that the HW has the RD_BUSY bit set
+	 * when polling for the status below.
+	 */
+	readl(efuse->base + MESON_MX_EFUSE_CNTL1);
+
+	err = readl_poll_timeout_atomic(efuse->base + MESON_MX_EFUSE_CNTL1,
+			regval,
+			(!(regval & MESON_MX_EFUSE_CNTL1_AUTO_RD_BUSY)),
+			1, 1000);
+	if (err) {
+		dev_err(efuse->config.dev,
+			"Timeout while reading efuse address %u\n", addr);
+		return err;
+	}
+
+	*value = readl(efuse->base + MESON_MX_EFUSE_CNTL2);
+
+	return 0;
+}
+
+static int meson_mx_efuse_read(void *context, unsigned int offset,
+			       void *buf, size_t bytes)
+{
+	struct meson_mx_efuse *efuse = context;
+	u32 tmp;
+	int err, i, addr;
+
+	err = meson_mx_efuse_hw_enable(efuse);
+	if (err)
+		return err;
+
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
+
+	for (i = offset; i < offset + bytes; i += efuse->config.word_size) {
+		addr = i / efuse->config.word_size;
+
+		err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
+		if (err)
+			break;
+
+		memcpy(buf + i, &tmp, efuse->config.word_size);
+	}
+
+	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
+				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE, 0);
+
+	meson_mx_efuse_hw_disable(efuse);
+
+	return err;
+}
+
+static const struct meson_mx_efuse_platform_data meson6_efuse_data = {
+	.name = "meson6-efuse",
+	.word_size = 1,
+};
+
+static const struct meson_mx_efuse_platform_data meson8_efuse_data = {
+	.name = "meson8-efuse",
+	.word_size = 4,
+};
+
+static const struct meson_mx_efuse_platform_data meson8b_efuse_data = {
+	.name = "meson8b-efuse",
+	.word_size = 4,
+};
+
+static const struct of_device_id meson_mx_efuse_match[] = {
+	{ .compatible = "amlogic,meson6-efuse", .data = &meson6_efuse_data },
+	{ .compatible = "amlogic,meson8-efuse", .data = &meson8_efuse_data },
+	{ .compatible = "amlogic,meson8b-efuse", .data = &meson8b_efuse_data },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_mx_efuse_match);
+
+static int meson_mx_efuse_probe(struct platform_device *pdev)
+{
+	const struct meson_mx_efuse_platform_data *drvdata;
+	struct meson_mx_efuse *efuse;
+	struct resource *res;
+
+	drvdata = of_device_get_match_data(&pdev->dev);
+	if (!drvdata)
+		return -EINVAL;
+
+	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	efuse->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(efuse->base))
+		return PTR_ERR(efuse->base);
+
+	efuse->config.name = devm_kstrdup(&pdev->dev, drvdata->name,
+					  GFP_KERNEL);
+	efuse->config.owner = THIS_MODULE;
+	efuse->config.dev = &pdev->dev;
+	efuse->config.priv = efuse;
+	efuse->config.stride = drvdata->word_size;
+	efuse->config.word_size = drvdata->word_size;
+	efuse->config.size = SZ_512;
+	efuse->config.read_only = true;
+	efuse->config.reg_read = meson_mx_efuse_read;
+
+	efuse->core_clk = devm_clk_get(&pdev->dev, "core");
+	if (IS_ERR(efuse->core_clk)) {
+		dev_err(&pdev->dev, "Failed to get core clock\n");
+		return PTR_ERR(efuse->core_clk);
+	}
+
+	efuse->nvmem = nvmem_register(&efuse->config);
+	if (IS_ERR(efuse->nvmem)) {
+		clk_unprepare(efuse->core_clk);
+		return PTR_ERR(efuse->nvmem);
+	}
+
+	platform_set_drvdata(pdev, efuse);
+
+	return 0;
+}
+
+static int meson_mx_efuse_remove(struct platform_device *pdev)
+{
+	struct meson_mx_efuse *efuse = platform_get_drvdata(pdev);
+	int err;
+
+	err = nvmem_unregister(efuse->nvmem);
+
+	clk_unprepare(efuse->core_clk);
+
+	return err;
+}
+
+static struct platform_driver meson_mx_efuse_driver = {
+	.probe = meson_mx_efuse_probe,
+	.remove = meson_mx_efuse_remove,
+	.driver = {
+		.name = "meson-mx-efuse",
+		.of_match_table = meson_mx_efuse_match,
+	},
+};
+
+module_platform_driver(meson_mx_efuse_driver);
+
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_DESCRIPTION("Amlogic Meson MX eFuse NVMEM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.14.2

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Srinivas Kandagatla]

Minor comments!!

On 01/10/17 13:56, Martin Blumenstingl wrote:
> This adds a driver to access the efuse on Amlogic Meson6, Meson8 and
> Meson8b SoCs.
> These SoCs are accessing the efuse IP block directly through the
> registers in the "secbus" region. This makes it different from the Meson
> GX efuse driver which uses the "secure monitor" firmware to access the
> efuse.
> 
> The efuse on Meson6 can only read one byte at a time, while the efuse on
> Meson8 and Meson8b always reads 4 bytes at a time. The new driver
> supports both, but due to lack of hardware Meson6 support was not tested.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>   drivers/nvmem/Kconfig          |  10 ++
>   drivers/nvmem/Makefile         |   2 +
>   drivers/nvmem/meson-mx-efuse.c | 272 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 284 insertions(+)
>   create mode 100644 drivers/nvmem/meson-mx-efuse.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> +	
> +
> +static void meson_mx_efuse_mask_bits(struct meson_mx_efuse *efuse, u32 reg,
> +				     u32 mask, u32 set)
> +{
> +	u32 data;
> +
> +	data = readl(efuse->base + reg);
> +	data &= ~mask;
> +	data |= (set & mask);
> +
> +	writel(data, efuse->base + reg);
> +}
> +
> +static int meson_mx_efuse_hw_enable(struct meson_mx_efuse *efuse)
> +{
> +	int err;
> +
> +	err = clk_prepare_enable(efuse->core_clk);
> +	if (err)
> +		return err;
> +
> +	/* power up the efuse */
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_PD_ENABLE, 0);
> +
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL4,
> +				 MESON_MX_EFUSE_CNTL4_ENCRYPT_ENABLE, 0);
> +
> +	return 0;
> +}
> +
> +static void meson_mx_efuse_hw_disable(struct meson_mx_efuse *efuse)
> +{
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_PD_ENABLE,
> +				 MESON_MX_EFUSE_CNTL1_PD_ENABLE);
> +
> +	clk_disable_unprepare(efuse->core_clk);
> +}
> +
> +static int meson_mx_efuse_read_addr(struct meson_mx_efuse *efuse,
> +				    unsigned int addr, u32 *value)
> +{
> +	int err;
> +	u32 regval;
> +
> +	/* write the address to read */
> +	regval = FIELD_PREP(MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, addr);
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, regval);
> +
> +	/* inform the hardware that we changed the address */
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET,
> +				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET);
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET, 0);
> +
> +	/* start the read process */
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START);
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_START, 0);
> +
> +	/*
> +	 * perform a dummy read to ensure that the HW has the RD_BUSY bit set
> +	 * when polling for the status below.
> +	 */
> +	readl(efuse->base + MESON_MX_EFUSE_CNTL1);
> +
> +	err = readl_poll_timeout_atomic(efuse->base + MESON_MX_EFUSE_CNTL1,
> +			regval,
> +			(!(regval & MESON_MX_EFUSE_CNTL1_AUTO_RD_BUSY)),
> +			1, 1000);
> +	if (err) {
> +		dev_err(efuse->config.dev,
> +			"Timeout while reading efuse address %u\n", addr);
> +		return err;
> +	}
> +
> +	*value = readl(efuse->base + MESON_MX_EFUSE_CNTL2);
> +
> +	return 0;
> +}
> +
> +static int meson_mx_efuse_read(void *context, unsigned int offset,
> +			       void *buf, size_t bytes)
> +{
> +	struct meson_mx_efuse *efuse = context;
> +	u32 tmp;
> +	int err, i, addr;
> +
> +	err = meson_mx_efuse_hw_enable(efuse);
> +	if (err)
> +		return err;
> +
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
> +
> +	for (i = offset; i < offset + bytes; i += efuse->config.word_size) {
> +		addr = i / efuse->config.word_size;
> +
> +		err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
> +		if (err)
> +			break;
> +
> +		memcpy(buf + i, &tmp, efuse->config.word_size);
> +	}
> +
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE, 0);
> +
> +	meson_mx_efuse_hw_disable(efuse);
> +
> +	return err;
> +}
...
> +
> +static int meson_mx_efuse_probe(struct platform_device *pdev)
> +{
> +	const struct meson_mx_efuse_platform_data *drvdata;
> +	struct meson_mx_efuse *efuse;
> +	struct resource *res;
> +

...

> +
> +	efuse->core_clk = devm_clk_get(&pdev->dev, "core");
> +	if (IS_ERR(efuse->core_clk)) {
> +		dev_err(&pdev->dev, "Failed to get core clock\n");
> +		return PTR_ERR(efuse->core_clk);
> +	}
> +
> +	efuse->nvmem = nvmem_register(&efuse->config);
> +	if (IS_ERR(efuse->nvmem)) {
> +		clk_unprepare(efuse->core_clk);

Do you need this???

> +		return PTR_ERR(efuse->nvmem);
> +	}
> +
> +	platform_set_drvdata(pdev, efuse);
> +
> +	return 0;
> +}
> +
> +static int meson_mx_efuse_remove(struct platform_device *pdev)
> +{
> +	struct meson_mx_efuse *efuse = platform_get_drvdata(pdev);
> +	int err;
> +
> +	err = nvmem_unregister(efuse->nvmem);
> +
> +	clk_unprepare(efuse->core_clk);

Do you need this disable here? AFAIU, this driver would never leave the 
clk prepared unless we are in the middle of read.

> +
> +	return err;
> +}
> +

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Martin Blumenstingl]

On Mon, Oct 2, 2017 at 5:28 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Minor comments!!
that was a VERY quick code review (and by this I mean the time between
sending the patch and getting valuable feedback) - many thanks for
this!

>
> On 01/10/17 13:56, Martin Blumenstingl wrote:
>>
>> This adds a driver to access the efuse on Amlogic Meson6, Meson8 and
>> Meson8b SoCs.
>> These SoCs are accessing the efuse IP block directly through the
>> registers in the "secbus" region. This makes it different from the Meson
>> GX efuse driver which uses the "secure monitor" firmware to access the
>> efuse.
>>
>> The efuse on Meson6 can only read one byte at a time, while the efuse on
>> Meson8 and Meson8b always reads 4 bytes at a time. The new driver
>> supports both, but due to lack of hardware Meson6 support was not tested.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>   drivers/nvmem/Kconfig          |  10 ++
>>   drivers/nvmem/Makefile         |   2 +
>>   drivers/nvmem/meson-mx-efuse.c | 272
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 284 insertions(+)
>>   create mode 100644 drivers/nvmem/meson-mx-efuse.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> +
>> +
>> +static void meson_mx_efuse_mask_bits(struct meson_mx_efuse *efuse, u32
>> reg,
>> +                                    u32 mask, u32 set)
>> +{
>> +       u32 data;
>> +
>> +       data = readl(efuse->base + reg);
>> +       data &= ~mask;
>> +       data |= (set & mask);
>> +
>> +       writel(data, efuse->base + reg);
>> +}
>> +
>> +static int meson_mx_efuse_hw_enable(struct meson_mx_efuse *efuse)
>> +{
>> +       int err;
>> +
>> +       err = clk_prepare_enable(efuse->core_clk);
>> +       if (err)
>> +               return err;
>> +
>> +       /* power up the efuse */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE, 0);
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL4,
>> +                                MESON_MX_EFUSE_CNTL4_ENCRYPT_ENABLE, 0);
>> +
>> +       return 0;
>> +}
>> +
>> +static void meson_mx_efuse_hw_disable(struct meson_mx_efuse *efuse)
>> +{
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE,
>> +                                MESON_MX_EFUSE_CNTL1_PD_ENABLE);
>> +
>> +       clk_disable_unprepare(efuse->core_clk);
>> +}
>> +
>> +static int meson_mx_efuse_read_addr(struct meson_mx_efuse *efuse,
>> +                                   unsigned int addr, u32 *value)
>> +{
>> +       int err;
>> +       u32 regval;
>> +
>> +       /* write the address to read */
>> +       regval = FIELD_PREP(MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK, addr);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_MASK,
>> regval);
>> +
>> +       /* inform the hardware that we changed the address */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_BYTE_ADDR_SET, 0);
>> +
>> +       /* start the read process */
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START);
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_START, 0);
>> +
>> +       /*
>> +        * perform a dummy read to ensure that the HW has the RD_BUSY bit
>> set
>> +        * when polling for the status below.
>> +        */
>> +       readl(efuse->base + MESON_MX_EFUSE_CNTL1);
>> +
>> +       err = readl_poll_timeout_atomic(efuse->base +
>> MESON_MX_EFUSE_CNTL1,
>> +                       regval,
>> +                       (!(regval & MESON_MX_EFUSE_CNTL1_AUTO_RD_BUSY)),
>> +                       1, 1000);
>> +       if (err) {
>> +               dev_err(efuse->config.dev,
>> +                       "Timeout while reading efuse address %u\n", addr);
>> +               return err;
>> +       }
>> +
>> +       *value = readl(efuse->base + MESON_MX_EFUSE_CNTL2);
>> +
>> +       return 0;
>> +}
>> +
>> +static int meson_mx_efuse_read(void *context, unsigned int offset,
>> +                              void *buf, size_t bytes)
>> +{
>> +       struct meson_mx_efuse *efuse = context;
>> +       u32 tmp;
>> +       int err, i, addr;
>> +
>> +       err = meson_mx_efuse_hw_enable(efuse);
>> +       if (err)
>> +               return err;
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
>> +
>> +       for (i = offset; i < offset + bytes; i += efuse->config.word_size)
>> {
>> +               addr = i / efuse->config.word_size;
>> +
>> +               err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
>> +               if (err)
>> +                       break;
>> +
>> +               memcpy(buf + i, &tmp, efuse->config.word_size);
>> +       }
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE, 0);
>> +
>> +       meson_mx_efuse_hw_disable(efuse);
>> +
>> +       return err;
>> +}
>
> ...
>>
>> +
>> +static int meson_mx_efuse_probe(struct platform_device *pdev)
>> +{
>> +       const struct meson_mx_efuse_platform_data *drvdata;
>> +       struct meson_mx_efuse *efuse;
>> +       struct resource *res;
>> +
>
>
> ...
>
>> +
>> +       efuse->core_clk = devm_clk_get(&pdev->dev, "core");
>> +       if (IS_ERR(efuse->core_clk)) {
>> +               dev_err(&pdev->dev, "Failed to get core clock\n");
>> +               return PTR_ERR(efuse->core_clk);
>> +       }
>> +
>> +       efuse->nvmem = nvmem_register(&efuse->config);
>> +       if (IS_ERR(efuse->nvmem)) {
>> +               clk_unprepare(efuse->core_clk);
>
>
> Do you need this???
oh, this is a left-over from an earlier version where I only enabled
the clock in .probe (and disabled it again in .remove)
however, this could break during suspend and it keeps the clock
unnecessarily enabled when it's not needed

>
>> +               return PTR_ERR(efuse->nvmem);
>> +       }
>> +
>> +       platform_set_drvdata(pdev, efuse);
>> +
>> +       return 0;
>> +}
>> +
>> +static int meson_mx_efuse_remove(struct platform_device *pdev)
>> +{
>> +       struct meson_mx_efuse *efuse = platform_get_drvdata(pdev);
>> +       int err;
>> +
>> +       err = nvmem_unregister(efuse->nvmem);
>> +
>> +       clk_unprepare(efuse->core_clk);
>
>
> Do you need this disable here? AFAIU, this driver would never leave the clk
> prepared unless we are in the middle of read.
yes, good catch here as well - this clk_unprepare call has to be removed

>
>> +
>> +       return err;
>> +}
>> +

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Chris Moore]

Hi,

Sorry in advance if I am being extremely stupid here.

Le 01/10/2017 ? 14:56, Martin Blumenstingl a ?crit?:

[snip]

> +static int meson_mx_efuse_read(void *context, unsigned int offset,
> +			       void *buf, size_t bytes)
> +{
> +	struct meson_mx_efuse *efuse = context;
> +	u32 tmp;
> +	int err, i, addr;
> +
> +	err = meson_mx_efuse_hw_enable(efuse);
> +	if (err)
> +		return err;
> +
> +	meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
> +				 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
> +
> +	for (i = offset; i < offset + bytes; i += efuse->config.word_size) {
> +		addr = i / efuse->config.word_size;
> +
> +		err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
> +		if (err)
> +			break;
> +
> +		memcpy(buf + i, &tmp, efuse->config.word_size);
> +	}

This will not give the expected result if offset is not a multiple of 
efuse->config.word_size.
This will cause buffer overflow if bytes is not a multiple of 
efuse->config.word_size.
Shouldn't there at least be some sort of test on offset and bytes?

Also this doesn't look endian-safe, but maybe it is.

Old-timer remarks:
- I try to avoid divisions where possible but I suppose they are very 
low-cost on recent hardware.
- I don't like arithmetic on void pointers but I know it is supported by 
gcc (and possibly by recent C standards too).

Sorry again for the noise if I am being stupid.

Cheers,
Chris

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Martin Blumenstingl]

Hi Chris,

On Tue, Oct 3, 2017 at 6:48 AM, Chris Moore <moore@free.fr> wrote:
> Hi,
>
> Sorry in advance if I am being extremely stupid here.
>
> Le 01/10/2017 ? 14:56, Martin Blumenstingl a ?crit :
>
> [snip]
>
>> +static int meson_mx_efuse_read(void *context, unsigned int offset,
>> +                              void *buf, size_t bytes)
>> +{
>> +       struct meson_mx_efuse *efuse = context;
>> +       u32 tmp;
>> +       int err, i, addr;
>> +
>> +       err = meson_mx_efuse_hw_enable(efuse);
>> +       if (err)
>> +               return err;
>> +
>> +       meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
>> +                                MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
>> +
>> +       for (i = offset; i < offset + bytes; i += efuse->config.word_size)
>> {
>> +               addr = i / efuse->config.word_size;
>> +
>> +               err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
>> +               if (err)
>> +                       break;
>> +
>> +               memcpy(buf + i, &tmp, efuse->config.word_size);
>> +       }
>
>
> This will not give the expected result if offset is not a multiple of
> efuse->config.word_size.
I had a look at mtk-efuse.c and bcm-ocotp.c and both are doing similar
calculations
thus I assumed that the nvmem core takes care of handling this

> This will cause buffer overflow if bytes is not a multiple of
> efuse->config.word_size.
that would be very nasty indeed

> Shouldn't there at least be some sort of test on offset and bytes?
in the driver or in core? if possible these checks should be part of
the core nvmem code to prevent code duplication across various drivers
(and to prevent bugs if a specific driver forgets these checks)

> Also this doesn't look endian-safe, but maybe it is.
dumping the whole efuse via sysfs showed the same results as (the
vendor command in Amlogic's) u-boot.

> Old-timer remarks:
> - I try to avoid divisions where possible but I suppose they are very
> low-cost on recent hardware.
> - I don't like arithmetic on void pointers but I know it is supported by gcc
> (and possibly by recent C standards too).
>
> Sorry again for the noise if I am being stupid.
your questions are reasonable
this is my first nvmem driver, so there may be mistakes in the code -
maybe Srinivas could also comment on your feedback

also thank you for taking the time to review this!


Regards,
Martin

[PATCH 3/4] nvmem: add a driver for the Amlogic Meson6/Meson8/Meson8b SoCs

[Srinivas Kandagatla]

On 03/10/17 05:48, Chris Moore wrote:
> Hi,
> 
> Sorry in advance if I am being extremely stupid here.
> 
> Le 01/10/2017 ? 14:56, Martin Blumenstingl a ?crit :
> 
> [snip]
> 
>> +static int meson_mx_efuse_read(void *context, unsigned int offset,
>> +                   void *buf, size_t bytes)
>> +{
>> +    struct meson_mx_efuse *efuse = context;
>> +    u32 tmp;
>> +    int err, i, addr;
>> +
>> +    err = meson_mx_efuse_hw_enable(efuse);
>> +    if (err)
>> +        return err;
>> +
>> +    meson_mx_efuse_mask_bits(efuse, MESON_MX_EFUSE_CNTL1,
>> +                 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE,
>> +                 MESON_MX_EFUSE_CNTL1_AUTO_RD_ENABLE);
>> +
>> +    for (i = offset; i < offset + bytes; i += efuse->config.word_size) {
>> +        addr = i / efuse->config.word_size;
>> +
>> +        err = meson_mx_efuse_read_addr(efuse, addr, &tmp);
>> +        if (err)
>> +            break;
>> +
>> +        memcpy(buf + i, &tmp, efuse->config.word_size);
>> +    }
> 
> This will not give the expected result if offset is not a multiple of 
> efuse->config.word_size.

Offset should be always multiple of stride which is enforced in nvmem core.

nvmem_cell would return an -EINVAL if that is not the case.


--srini

> This will cause buffer overflow if bytes is not a multiple of 
> efuse->config.word_size.
> Shouldn't there at least be some sort of test on offset and bytes?
> 
> Also this doesn't look endian-safe, but maybe it is.
> 
> Old-timer remarks:
> - I try to avoid divisions where possible but I suppose they are very 
> low-cost on recent hardware.
> - I don't like arithmetic on void pointers but I know it is supported by 
> gcc (and possibly by recent C standards too).
> 
> Sorry again for the noise if I am being stupid.
> 
> Cheers,
> Chris
> 

[PATCH 4/4] ARM: dts: meson: add the efuse node

[Martin Blumenstingl]

Meson6, Meson8 and Meson8b use a similar IP block which has access to
512 bytes of efuse data.
During SoC manufacturing some calibration settings for the CVBS
connector and the internal temperature sensor are written to this efuse.
On some boards it additionally stores for example the MAC addresses.

The efuse is enabled on Meson8 and Meson8b but kept disabled on Meson6
since we do not have a clock driver there (which is required to read
data from the efuse).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson.dtsi   | 13 +++++++++++++
 arch/arm/boot/dts/meson6.dtsi  |  3 +++
 arch/arm/boot/dts/meson8.dtsi  |  6 ++++++
 arch/arm/boot/dts/meson8b.dtsi |  7 +++++++
 4 files changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/meson.dtsi b/arch/arm/boot/dts/meson.dtsi
index 163dac42bf2e..9c1b21be94f4 100644
--- a/arch/arm/boot/dts/meson.dtsi
+++ b/arch/arm/boot/dts/meson.dtsi
@@ -262,5 +262,18 @@
 			compatible = "amlogic,meson-mx-bootrom", "syscon";
 			reg = <0xd9040000 0x10000>;
 		};
+
+		secbus: secbus at da000000 {
+			compatible = "simple-bus";
+			reg = <0xda000000 0x6000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0xda000000 0x6000>;
+
+			efuse: nvmem at 0 {
+				compatible = "amlogic,meson6-efuse";
+				reg = <0x0 0x2000>;
+			};
+		};
 	};
 }; /* end of / */
diff --git a/arch/arm/boot/dts/meson6.dtsi b/arch/arm/boot/dts/meson6.dtsi
index ef281d290052..9b463211339f 100644
--- a/arch/arm/boot/dts/meson6.dtsi
+++ b/arch/arm/boot/dts/meson6.dtsi
@@ -84,6 +84,9 @@
 	};
 }; /* end of / */
 
+&efuse {
+	status = "disabled";
+};
 
 &uart_AO {
 	clocks = <&xtal>, <&clk81>, <&clk81>;
diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 2095a002ea8d..dad8b234c172 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -275,6 +275,12 @@
 	};
 };
 
+&efuse {
+	compatible = "amlogic,meson8-efuse";
+	clocks = <&clkc CLKID_EFUSE>;
+	clock-names = "core";
+};
+
 &ethmac {
 	clocks = <&clkc CLKID_ETH>;
 	clock-names = "stmmaceth";
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 111e01809ad7..4f8fc01ee0f0 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -183,6 +183,13 @@
 	};
 };
 
+
+&efuse {
+	compatible = "amlogic,meson8b-efuse";
+	clocks = <&clkc CLKID_EFUSE>;
+	clock-names = "core";
+};
+
 &ethmac {
 	clocks = <&clkc CLKID_ETH>;
 	clock-names = "stmmaceth";
-- 
2.14.2