[PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Icenowy Zheng]

Sometimes the SID device have more memory address space than the real
NVMEM size (for the registers used to read/write the SID).

Fetch the NVMEM size from device compatible, rather than the memory
address space's length, in order to prepare for adding some
registers-based read support.

Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
---
This patch is splited from the [PATCH v2 1/2].

It defines a size property in the of_match data of the device, in
order to match compatible with SID length, as different SoCs' SID
have different length.

 drivers/nvmem/sunxi_sid.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 1567ccca8de3..69524b67007f 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -20,6 +20,7 @@
 #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/slab.h>
 #include <linux/random.h>
@@ -32,6 +33,10 @@ static struct nvmem_config econfig = {
 	.owner = THIS_MODULE,
 };
 
+struct sunxi_sid_cfg {
+	u32	size;
+};
+
 struct sunxi_sid {
 	void __iomem		*base;
 };
@@ -72,18 +77,24 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 	struct sunxi_sid *sid;
 	int ret, i, size;
 	char *randomness;
+	const struct sunxi_sid_cfg *cfg;
 
 	sid = devm_kzalloc(dev, sizeof(*sid), GFP_KERNEL);
 	if (!sid)
 		return -ENOMEM;
 
+	cfg = of_device_get_match_data(dev);
+	if (!cfg)
+		return -EINVAL;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sid->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(sid->base))
 		return PTR_ERR(sid->base);
 
-	size = resource_size(res) - 1;
-	econfig.size = resource_size(res);
+	size = cfg->size;
+
+	econfig.size = size;
 	econfig.dev = dev;
 	econfig.reg_read = sunxi_sid_read;
 	econfig.priv = sid;
@@ -119,9 +130,17 @@ static int sunxi_sid_remove(struct platform_device *pdev)
 	return nvmem_unregister(nvmem);
 }
 
+static const struct sunxi_sid_cfg sun4i_a10_cfg = {
+	.size = 0x10,
+};
+
+static const struct sunxi_sid_cfg sun7i_a20_cfg = {
+	.size = 0x200,
+};
+
 static const struct of_device_id sunxi_sid_of_match[] = {
-	{ .compatible = "allwinner,sun4i-a10-sid" },
-	{ .compatible = "allwinner,sun7i-a20-sid" },
+	{ .compatible = "allwinner,sun4i-a10-sid", .data = &sun4i_a10_cfg },
+	{ .compatible = "allwinner,sun7i-a20-sid", .data = &sun7i_a20_cfg },
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
-- 
2.11.0

[PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Icenowy Zheng]

Sometimes the SID device have more memory address space than the real
NVMEM size (for the registers used to read/write the SID).

Fetch the NVMEM size from device compatible, rather than the memory
address space's length, in order to prepare for adding some
registers-based read support.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
This patch is splited from the [PATCH v2 1/2].

It defines a size property in the of_match data of the device, in
order to match compatible with SID length, as different SoCs' SID
have different length.

 drivers/nvmem/sunxi_sid.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 1567ccca8de3..69524b67007f 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -20,6 +20,7 @@
 #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/slab.h>
 #include <linux/random.h>
@@ -32,6 +33,10 @@ static struct nvmem_config econfig = {
 	.owner = THIS_MODULE,
 };
 
+struct sunxi_sid_cfg {
+	u32	size;
+};
+
 struct sunxi_sid {
 	void __iomem		*base;
 };
@@ -72,18 +77,24 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 	struct sunxi_sid *sid;
 	int ret, i, size;
 	char *randomness;
+	const struct sunxi_sid_cfg *cfg;
 
 	sid = devm_kzalloc(dev, sizeof(*sid), GFP_KERNEL);
 	if (!sid)
 		return -ENOMEM;
 
+	cfg = of_device_get_match_data(dev);
+	if (!cfg)
+		return -EINVAL;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sid->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(sid->base))
 		return PTR_ERR(sid->base);
 
-	size = resource_size(res) - 1;
-	econfig.size = resource_size(res);
+	size = cfg->size;
+
+	econfig.size = size;
 	econfig.dev = dev;
 	econfig.reg_read = sunxi_sid_read;
 	econfig.priv = sid;
@@ -119,9 +130,17 @@ static int sunxi_sid_remove(struct platform_device *pdev)
 	return nvmem_unregister(nvmem);
 }
 
+static const struct sunxi_sid_cfg sun4i_a10_cfg = {
+	.size = 0x10,
+};
+
+static const struct sunxi_sid_cfg sun7i_a20_cfg = {
+	.size = 0x200,
+};
+
 static const struct of_device_id sunxi_sid_of_match[] = {
-	{ .compatible = "allwinner,sun4i-a10-sid" },
-	{ .compatible = "allwinner,sun7i-a20-sid" },
+	{ .compatible = "allwinner,sun4i-a10-sid", .data = &sun4i_a10_cfg },
+	{ .compatible = "allwinner,sun7i-a20-sid", .data = &sun7i_a20_cfg },
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
-- 
2.11.0

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

The H3 SoC have a bigger SID controller, which has its direct read
address at 0x200 position in the SID block, not 0x0.

Also, H3 SID controller has some silicon bug that makes the direct read
value wrong at cold boot, add code to workaround the bug. (This bug has
already been fixed on A64 and later SoCs)

Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
---
This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
controller.

 .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
 drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
index d543ed3f5363..9ab9e75a6351 100644
--- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
@@ -1,7 +1,11 @@
 Allwinner sunxi-sid
 
 Required properties:
-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
+- compatible: Should be one of the following (depending on your SoC):
+  "allwinner,sun4i-a10-sid"
+  "allwinner,sun7i-a20-sid"
+  "allwinner,sun8i-h3-sid"
+
 - reg: Should contain registers location and length
 
 = Data cells =
@@ -19,3 +23,9 @@ Example for sun7i:
 		compatible = "allwinner,sun7i-a20-sid";
 		reg = <0x01c23800 0x200>
 	};
+
+Example for sun8i-h3:
+	sid@01c14000 {
+		compatible = "allwinner,sun8i-h3-sid";
+		reg = <0x01c14000 0x400>;
+	};
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 69524b67007f..476a161ff23a 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -25,6 +25,16 @@
 #include <linux/slab.h>
 #include <linux/random.h>
 
+/* Registers and special values for doing register-based SID readout on H3 */
+#define SUN8I_SID_PRCTL		0x40
+#define SUN8I_SID_RDKEY		0x60
+
+#define SUN8I_SID_OP_LOCK	0xAC
+#define SUN8I_SID_OFFSET_MASK	0x1FF
+#define SUN8I_SID_OFFSET_SHIFT	16
+#define SUN8I_SID_LOCK_SHIFT	8
+#define SUN8I_SID_READ		BIT(1)
+
 static struct nvmem_config econfig = {
 	.name = "sunxi-sid",
 	.read_only = true,
@@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
 };
 
 struct sunxi_sid_cfg {
+	u32	value_offset;
 	u32	size;
+	bool	need_register_readout;
 };
 
 struct sunxi_sid {
 	void __iomem		*base;
+	u32			value_offset;
 };
 
 /* We read the entire key, due to a 32 bit read alignment requirement. Since we
@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
 {
 	u32 sid_key;
 
-	sid_key = ioread32be(sid->base + round_down(offset, 4));
+	sid_key = ioread32be(sid->base + sid->value_offset +
+			     round_down(offset, 4));
 	sid_key >>= (offset % 4) * 8;
 
 	return sid_key; /* Only return the last byte */
@@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
 	return 0;
 }
 
+static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
+				      const unsigned int word,
+				      u32 *out)
+{
+	u32 reg_val;
+	unsigned long expire = jiffies + msecs_to_jiffies(250);
+
+	/* Set word, lock access, and set read command */
+	reg_val = (word & SUN8I_SID_OFFSET_MASK)
+		  << SUN8I_SID_OFFSET_SHIFT;
+	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;
+	reg_val |= SUN8I_SID_READ;
+	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
+
+	do {
+		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
+	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));
+
+	if (reg_val & SUN8I_SID_READ)
+		return -EIO;
+
+	if (out)
+		*out = readl(sid->base + SUN8I_SID_RDKEY);
+	writel(0, sid->base + SUN8I_SID_PRCTL);
+	return 0;
+}
+
 static int sunxi_sid_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -86,6 +127,7 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 	cfg = of_device_get_match_data(dev);
 	if (!cfg)
 		return -EINVAL;
+	sid->value_offset = cfg->value_offset;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sid->base = devm_ioremap_resource(dev, res);
@@ -94,6 +136,23 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 
 	size = cfg->size;
 
+	if (cfg->need_register_readout) {
+		/*
+		 * H3's SID controller have a bug that the value at 0x200
+		 * offset is not the correct value when the hardware is reset.
+		 * However, after doing a register-based read operation, the
+		 * value become right.
+		 * Do a full read operation here, but ignore its value
+		 * (as it's more fast to read by direct MMIO value than
+		 * with registers)
+		 */
+		for (i = 0; i < (size >> 2); i++) {
+			ret = sun8i_sid_register_readout(sid, i, NULL);
+			if (ret)
+				return ret;
+		}
+	}
+
 	econfig.size = size;
 	econfig.dev = dev;
 	econfig.reg_read = sunxi_sid_read;
@@ -131,16 +190,27 @@ static int sunxi_sid_remove(struct platform_device *pdev)
 }
 
 static const struct sunxi_sid_cfg sun4i_a10_cfg = {
+	.value_offset = 0,
 	.size = 0x10,
+	.need_register_readout = false,
 };
 
 static const struct sunxi_sid_cfg sun7i_a20_cfg = {
+	.value_offset = 0,
 	.size = 0x200,
+	.need_register_readout = false,
+};
+
+static const struct sunxi_sid_cfg sun8i_h3_cfg = {
+	.value_offset = 0x200,
+	.size = 0x100,
+	.need_register_readout = true,
 };
 
 static const struct of_device_id sunxi_sid_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-sid", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun7i-a20-sid", .data = &sun7i_a20_cfg },
+	{ .compatible = "allwinner,sun8i-h3-sid", .data = &sun8i_h3_cfg },
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
-- 
2.11.0

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

The H3 SoC have a bigger SID controller, which has its direct read
address at 0x200 position in the SID block, not 0x0.

Also, H3 SID controller has some silicon bug that makes the direct read
value wrong at cold boot, add code to workaround the bug. (This bug has
already been fixed on A64 and later SoCs)

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
controller.

 .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
 drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
index d543ed3f5363..9ab9e75a6351 100644
--- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
@@ -1,7 +1,11 @@
 Allwinner sunxi-sid
 
 Required properties:
-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
+- compatible: Should be one of the following (depending on your SoC):
+  "allwinner,sun4i-a10-sid"
+  "allwinner,sun7i-a20-sid"
+  "allwinner,sun8i-h3-sid"
+
 - reg: Should contain registers location and length
 
 = Data cells =
@@ -19,3 +23,9 @@ Example for sun7i:
 		compatible = "allwinner,sun7i-a20-sid";
 		reg = <0x01c23800 0x200>
 	};
+
+Example for sun8i-h3:
+	sid at 01c14000 {
+		compatible = "allwinner,sun8i-h3-sid";
+		reg = <0x01c14000 0x400>;
+	};
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index 69524b67007f..476a161ff23a 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -25,6 +25,16 @@
 #include <linux/slab.h>
 #include <linux/random.h>
 
+/* Registers and special values for doing register-based SID readout on H3 */
+#define SUN8I_SID_PRCTL		0x40
+#define SUN8I_SID_RDKEY		0x60
+
+#define SUN8I_SID_OP_LOCK	0xAC
+#define SUN8I_SID_OFFSET_MASK	0x1FF
+#define SUN8I_SID_OFFSET_SHIFT	16
+#define SUN8I_SID_LOCK_SHIFT	8
+#define SUN8I_SID_READ		BIT(1)
+
 static struct nvmem_config econfig = {
 	.name = "sunxi-sid",
 	.read_only = true,
@@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
 };
 
 struct sunxi_sid_cfg {
+	u32	value_offset;
 	u32	size;
+	bool	need_register_readout;
 };
 
 struct sunxi_sid {
 	void __iomem		*base;
+	u32			value_offset;
 };
 
 /* We read the entire key, due to a 32 bit read alignment requirement. Since we
@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
 {
 	u32 sid_key;
 
-	sid_key = ioread32be(sid->base + round_down(offset, 4));
+	sid_key = ioread32be(sid->base + sid->value_offset +
+			     round_down(offset, 4));
 	sid_key >>= (offset % 4) * 8;
 
 	return sid_key; /* Only return the last byte */
@@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
 	return 0;
 }
 
+static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
+				      const unsigned int word,
+				      u32 *out)
+{
+	u32 reg_val;
+	unsigned long expire = jiffies + msecs_to_jiffies(250);
+
+	/* Set word, lock access, and set read command */
+	reg_val = (word & SUN8I_SID_OFFSET_MASK)
+		  << SUN8I_SID_OFFSET_SHIFT;
+	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;
+	reg_val |= SUN8I_SID_READ;
+	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
+
+	do {
+		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
+	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));
+
+	if (reg_val & SUN8I_SID_READ)
+		return -EIO;
+
+	if (out)
+		*out = readl(sid->base + SUN8I_SID_RDKEY);
+	writel(0, sid->base + SUN8I_SID_PRCTL);
+	return 0;
+}
+
 static int sunxi_sid_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -86,6 +127,7 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 	cfg = of_device_get_match_data(dev);
 	if (!cfg)
 		return -EINVAL;
+	sid->value_offset = cfg->value_offset;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sid->base = devm_ioremap_resource(dev, res);
@@ -94,6 +136,23 @@ static int sunxi_sid_probe(struct platform_device *pdev)
 
 	size = cfg->size;
 
+	if (cfg->need_register_readout) {
+		/*
+		 * H3's SID controller have a bug that the value at 0x200
+		 * offset is not the correct value when the hardware is reset.
+		 * However, after doing a register-based read operation, the
+		 * value become right.
+		 * Do a full read operation here, but ignore its value
+		 * (as it's more fast to read by direct MMIO value than
+		 * with registers)
+		 */
+		for (i = 0; i < (size >> 2); i++) {
+			ret = sun8i_sid_register_readout(sid, i, NULL);
+			if (ret)
+				return ret;
+		}
+	}
+
 	econfig.size = size;
 	econfig.dev = dev;
 	econfig.reg_read = sunxi_sid_read;
@@ -131,16 +190,27 @@ static int sunxi_sid_remove(struct platform_device *pdev)
 }
 
 static const struct sunxi_sid_cfg sun4i_a10_cfg = {
+	.value_offset = 0,
 	.size = 0x10,
+	.need_register_readout = false,
 };
 
 static const struct sunxi_sid_cfg sun7i_a20_cfg = {
+	.value_offset = 0,
 	.size = 0x200,
+	.need_register_readout = false,
+};
+
+static const struct sunxi_sid_cfg sun8i_h3_cfg = {
+	.value_offset = 0x200,
+	.size = 0x100,
+	.need_register_readout = true,
 };
 
 static const struct of_device_id sunxi_sid_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-sid", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun7i-a20-sid", .data = &sun7i_a20_cfg },
+	{ .compatible = "allwinner,sun8i-h3-sid", .data = &sun8i_h3_cfg },
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
-- 
2.11.0

[PATCH v3 3/3] ARM: dts: sun8i: enable SID on Allwinner H3 SoC

[Icenowy Zheng]

As we have already made sunxi_sid driver support the SID controller on
H3, enable it.

Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 08fd0860bb6b..0a7a38a75ee4 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -206,6 +206,11 @@
 			#size-cells = <0>;
 		};
 
+		sid: eeprom@01c14000 {
+			compatible = "allwinner,sun8i-h3-sid";
+			reg = <0x01c14000 0x400>;
+		};
+
 		usbphy: phy@01c19400 {
 			compatible = "allwinner,sun8i-h3-usb-phy";
 			reg = <0x01c19400 0x2c>,
-- 
2.11.0

[PATCH v3 3/3] ARM: dts: sun8i: enable SID on Allwinner H3 SoC

[Icenowy Zheng]

As we have already made sunxi_sid driver support the SID controller on
H3, enable it.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 08fd0860bb6b..0a7a38a75ee4 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -206,6 +206,11 @@
 			#size-cells = <0>;
 		};
 
+		sid: eeprom at 01c14000 {
+			compatible = "allwinner,sun8i-h3-sid";
+			reg = <0x01c14000 0x400>;
+		};
+
 		usbphy: phy at 01c19400 {
 			compatible = "allwinner,sun8i-h3-usb-phy";
 			reg = <0x01c19400 0x2c>,
-- 
2.11.0

Re: [PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Thu, Feb 02, 2017 at 09:13:36PM +0800, Icenowy Zheng wrote:
> Sometimes the SID device have more memory address space than the real
> NVMEM size (for the registers used to read/write the SID).
> 
> Fetch the NVMEM size from device compatible, rather than the memory
> address space's length, in order to prepare for adding some
> registers-based read support.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Thu, Feb 02, 2017 at 09:13:36PM +0800, Icenowy Zheng wrote:
> Sometimes the SID device have more memory address space than the real
> NVMEM size (for the registers used to read/write the SID).
> 
> Fetch the NVMEM size from device compatible, rather than the memory
> address space's length, in order to prepare for adding some
> registers-based read support.
> 
> Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 1/3] nvmem: sunxi-sid: read NVMEM size from device compatible

[Maxime Ripard]

On Thu, Feb 02, 2017 at 09:13:36PM +0800, Icenowy Zheng wrote:
> Sometimes the SID device have more memory address space than the real
> NVMEM size (for the registers used to read/write the SID).
> 
> Fetch the NVMEM size from device compatible, rather than the memory
> address space's length, in order to prepare for adding some
> registers-based read support.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170206/c3412e37/attachment.sig>

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4415 bytes --]

On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> The H3 SoC have a bigger SID controller, which has its direct read
> address at 0x200 position in the SID block, not 0x0.
> 
> Also, H3 SID controller has some silicon bug that makes the direct read
> value wrong at cold boot, add code to workaround the bug. (This bug has
> already been fixed on A64 and later SoCs)
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> controller.
> 
>  .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
>  drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> index d543ed3f5363..9ab9e75a6351 100644
> --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> @@ -1,7 +1,11 @@
>  Allwinner sunxi-sid
>  
>  Required properties:
> -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> +- compatible: Should be one of the following (depending on your SoC):
> +  "allwinner,sun4i-a10-sid"
> +  "allwinner,sun7i-a20-sid"
> +  "allwinner,sun8i-h3-sid"
> +
>  - reg: Should contain registers location and length
>  
>  = Data cells =
> @@ -19,3 +23,9 @@ Example for sun7i:
>  		compatible = "allwinner,sun7i-a20-sid";
>  		reg = <0x01c23800 0x200>
>  	};
> +
> +Example for sun8i-h3:
> +	sid@01c14000 {
> +		compatible = "allwinner,sun8i-h3-sid";
> +		reg = <0x01c14000 0x400>;
> +	};
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 69524b67007f..476a161ff23a 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -25,6 +25,16 @@
>  #include <linux/slab.h>
>  #include <linux/random.h>
>  
> +/* Registers and special values for doing register-based SID readout on H3 */
> +#define SUN8I_SID_PRCTL		0x40
> +#define SUN8I_SID_RDKEY		0x60
> +
> +#define SUN8I_SID_OP_LOCK	0xAC
> +#define SUN8I_SID_OFFSET_MASK	0x1FF
> +#define SUN8I_SID_OFFSET_SHIFT	16
> +#define SUN8I_SID_LOCK_SHIFT	8
> +#define SUN8I_SID_READ		BIT(1)
> +
>  static struct nvmem_config econfig = {
>  	.name = "sunxi-sid",
>  	.read_only = true,
> @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>  };
>  
>  struct sunxi_sid_cfg {
> +	u32	value_offset;
>  	u32	size;
> +	bool	need_register_readout;
>  };
>  
>  struct sunxi_sid {
>  	void __iomem		*base;
> +	u32			value_offset;
>  };
>  
>  /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>  {
>  	u32 sid_key;
>  
> -	sid_key = ioread32be(sid->base + round_down(offset, 4));
> +	sid_key = ioread32be(sid->base + sid->value_offset +
> +			     round_down(offset, 4));

This would probably be more logical to have this in sunxi_sid_read.

>  	sid_key >>= (offset % 4) * 8;
>  
>  	return sid_key; /* Only return the last byte */
> @@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>  	return 0;
>  }
>  
> +static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
> +				      const unsigned int word,
> +				      u32 *out)
> +{
> +	u32 reg_val;
> +	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +
> +	/* Set word, lock access, and set read command */
> +	reg_val = (word & SUN8I_SID_OFFSET_MASK)
> +		  << SUN8I_SID_OFFSET_SHIFT;
> +	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;

You're not using those mask and shifts anywhere else, why not just
define the value / macro you need directly?

> +	reg_val |= SUN8I_SID_READ;
> +	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
> +
> +	do {
> +		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
> +	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));

readl_poll_timeout?

> +	if (reg_val & SUN8I_SID_READ)
> +		return -EIO;
> +
> +	if (out)
> +		*out = readl(sid->base + SUN8I_SID_RDKEY);

Why do you need that out parameter?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4298 bytes --]

On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> The H3 SoC have a bigger SID controller, which has its direct read
> address at 0x200 position in the SID block, not 0x0.
> 
> Also, H3 SID controller has some silicon bug that makes the direct read
> value wrong at cold boot, add code to workaround the bug. (This bug has
> already been fixed on A64 and later SoCs)
> 
> Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> ---
> This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> controller.
> 
>  .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
>  drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> index d543ed3f5363..9ab9e75a6351 100644
> --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> @@ -1,7 +1,11 @@
>  Allwinner sunxi-sid
>  
>  Required properties:
> -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> +- compatible: Should be one of the following (depending on your SoC):
> +  "allwinner,sun4i-a10-sid"
> +  "allwinner,sun7i-a20-sid"
> +  "allwinner,sun8i-h3-sid"
> +
>  - reg: Should contain registers location and length
>  
>  = Data cells =
> @@ -19,3 +23,9 @@ Example for sun7i:
>  		compatible = "allwinner,sun7i-a20-sid";
>  		reg = <0x01c23800 0x200>
>  	};
> +
> +Example for sun8i-h3:
> +	sid@01c14000 {
> +		compatible = "allwinner,sun8i-h3-sid";
> +		reg = <0x01c14000 0x400>;
> +	};
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 69524b67007f..476a161ff23a 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -25,6 +25,16 @@
>  #include <linux/slab.h>
>  #include <linux/random.h>
>  
> +/* Registers and special values for doing register-based SID readout on H3 */
> +#define SUN8I_SID_PRCTL		0x40
> +#define SUN8I_SID_RDKEY		0x60
> +
> +#define SUN8I_SID_OP_LOCK	0xAC
> +#define SUN8I_SID_OFFSET_MASK	0x1FF
> +#define SUN8I_SID_OFFSET_SHIFT	16
> +#define SUN8I_SID_LOCK_SHIFT	8
> +#define SUN8I_SID_READ		BIT(1)
> +
>  static struct nvmem_config econfig = {
>  	.name = "sunxi-sid",
>  	.read_only = true,
> @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>  };
>  
>  struct sunxi_sid_cfg {
> +	u32	value_offset;
>  	u32	size;
> +	bool	need_register_readout;
>  };
>  
>  struct sunxi_sid {
>  	void __iomem		*base;
> +	u32			value_offset;
>  };
>  
>  /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>  {
>  	u32 sid_key;
>  
> -	sid_key = ioread32be(sid->base + round_down(offset, 4));
> +	sid_key = ioread32be(sid->base + sid->value_offset +
> +			     round_down(offset, 4));

This would probably be more logical to have this in sunxi_sid_read.

>  	sid_key >>= (offset % 4) * 8;
>  
>  	return sid_key; /* Only return the last byte */
> @@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>  	return 0;
>  }
>  
> +static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
> +				      const unsigned int word,
> +				      u32 *out)
> +{
> +	u32 reg_val;
> +	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +
> +	/* Set word, lock access, and set read command */
> +	reg_val = (word & SUN8I_SID_OFFSET_MASK)
> +		  << SUN8I_SID_OFFSET_SHIFT;
> +	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;

You're not using those mask and shifts anywhere else, why not just
define the value / macro you need directly?

> +	reg_val |= SUN8I_SID_READ;
> +	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
> +
> +	do {
> +		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
> +	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));

readl_poll_timeout?

> +	if (reg_val & SUN8I_SID_READ)
> +		return -EIO;
> +
> +	if (out)
> +		*out = readl(sid->base + SUN8I_SID_RDKEY);

Why do you need that out parameter?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> The H3 SoC have a bigger SID controller, which has its direct read
> address at 0x200 position in the SID block, not 0x0.
> 
> Also, H3 SID controller has some silicon bug that makes the direct read
> value wrong at cold boot, add code to workaround the bug. (This bug has
> already been fixed on A64 and later SoCs)
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> controller.
> 
>  .../bindings/nvmem/allwinner,sunxi-sid.txt         | 12 +++-
>  drivers/nvmem/sunxi_sid.c                          | 72 +++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> index d543ed3f5363..9ab9e75a6351 100644
> --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> @@ -1,7 +1,11 @@
>  Allwinner sunxi-sid
>  
>  Required properties:
> -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> +- compatible: Should be one of the following (depending on your SoC):
> +  "allwinner,sun4i-a10-sid"
> +  "allwinner,sun7i-a20-sid"
> +  "allwinner,sun8i-h3-sid"
> +
>  - reg: Should contain registers location and length
>  
>  = Data cells =
> @@ -19,3 +23,9 @@ Example for sun7i:
>  		compatible = "allwinner,sun7i-a20-sid";
>  		reg = <0x01c23800 0x200>
>  	};
> +
> +Example for sun8i-h3:
> +	sid at 01c14000 {
> +		compatible = "allwinner,sun8i-h3-sid";
> +		reg = <0x01c14000 0x400>;
> +	};
> diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> index 69524b67007f..476a161ff23a 100644
> --- a/drivers/nvmem/sunxi_sid.c
> +++ b/drivers/nvmem/sunxi_sid.c
> @@ -25,6 +25,16 @@
>  #include <linux/slab.h>
>  #include <linux/random.h>
>  
> +/* Registers and special values for doing register-based SID readout on H3 */
> +#define SUN8I_SID_PRCTL		0x40
> +#define SUN8I_SID_RDKEY		0x60
> +
> +#define SUN8I_SID_OP_LOCK	0xAC
> +#define SUN8I_SID_OFFSET_MASK	0x1FF
> +#define SUN8I_SID_OFFSET_SHIFT	16
> +#define SUN8I_SID_LOCK_SHIFT	8
> +#define SUN8I_SID_READ		BIT(1)
> +
>  static struct nvmem_config econfig = {
>  	.name = "sunxi-sid",
>  	.read_only = true,
> @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>  };
>  
>  struct sunxi_sid_cfg {
> +	u32	value_offset;
>  	u32	size;
> +	bool	need_register_readout;
>  };
>  
>  struct sunxi_sid {
>  	void __iomem		*base;
> +	u32			value_offset;
>  };
>  
>  /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>  {
>  	u32 sid_key;
>  
> -	sid_key = ioread32be(sid->base + round_down(offset, 4));
> +	sid_key = ioread32be(sid->base + sid->value_offset +
> +			     round_down(offset, 4));

This would probably be more logical to have this in sunxi_sid_read.

>  	sid_key >>= (offset % 4) * 8;
>  
>  	return sid_key; /* Only return the last byte */
> @@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>  	return 0;
>  }
>  
> +static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
> +				      const unsigned int word,
> +				      u32 *out)
> +{
> +	u32 reg_val;
> +	unsigned long expire = jiffies + msecs_to_jiffies(250);
> +
> +	/* Set word, lock access, and set read command */
> +	reg_val = (word & SUN8I_SID_OFFSET_MASK)
> +		  << SUN8I_SID_OFFSET_SHIFT;
> +	reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;

You're not using those mask and shifts anywhere else, why not just
define the value / macro you need directly?

> +	reg_val |= SUN8I_SID_READ;
> +	writel(reg_val, sid->base + SUN8I_SID_PRCTL);
> +
> +	do {
> +		reg_val = readl(sid->base + SUN8I_SID_PRCTL);
> +	} while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));

readl_poll_timeout?

> +	if (reg_val & SUN8I_SID_READ)
> +		return -EIO;
> +
> +	if (out)
> +		*out = readl(sid->base + SUN8I_SID_RDKEY);

Why do you need that out parameter?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170206/2758c6cf/attachment.sig>

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
>>  The H3 SoC have a bigger SID controller, which has its direct read
>>  address at 0x200 position in the SID block, not 0x0.
>>
>>  Also, H3 SID controller has some silicon bug that makes the direct read
>>  value wrong at cold boot, add code to workaround the bug. (This bug has
>>  already been fixed on A64 and later SoCs)
>>
>>  Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
>>  ---
>>  This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
>>  controller.
>>
>>   .../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
>>   drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
>>   2 files changed, 82 insertions(+), 2 deletions(-)
>>
>>  diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  index d543ed3f5363..9ab9e75a6351 100644
>>  --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  @@ -1,7 +1,11 @@
>>   Allwinner sunxi-sid
>>
>>   Required properties:
>>  -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
>>  +- compatible: Should be one of the following (depending on your SoC):
>>  + "allwinner,sun4i-a10-sid"
>>  + "allwinner,sun7i-a20-sid"
>>  + "allwinner,sun8i-h3-sid"
>>  +
>>   - reg: Should contain registers location and length
>>
>>   = Data cells =
>>  @@ -19,3 +23,9 @@ Example for sun7i:
>>                   compatible = "allwinner,sun7i-a20-sid";
>>                   reg = <0x01c23800 0x200>
>>           };
>>  +
>>  +Example for sun8i-h3:
>>  + sid@01c14000 {
>>  + compatible = "allwinner,sun8i-h3-sid";
>>  + reg = <0x01c14000 0x400>;
>>  + };
>>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
>>  index 69524b67007f..476a161ff23a 100644
>>  --- a/drivers/nvmem/sunxi_sid.c
>>  +++ b/drivers/nvmem/sunxi_sid.c
>>  @@ -25,6 +25,16 @@
>>   #include <linux/slab.h>
>>   #include <linux/random.h>
>>
>>  +/* Registers and special values for doing register-based SID readout on H3 */
>>  +#define SUN8I_SID_PRCTL 0x40
>>  +#define SUN8I_SID_RDKEY 0x60
>>  +
>>  +#define SUN8I_SID_OP_LOCK 0xAC
>>  +#define SUN8I_SID_OFFSET_MASK 0x1FF
>>  +#define SUN8I_SID_OFFSET_SHIFT 16
>>  +#define SUN8I_SID_LOCK_SHIFT 8
>>  +#define SUN8I_SID_READ BIT(1)
>>  +
>>   static struct nvmem_config econfig = {
>>           .name = "sunxi-sid",
>>           .read_only = true,
>>  @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>>   };
>>
>>   struct sunxi_sid_cfg {
>>  + u32 value_offset;
>>           u32 size;
>>  + bool need_register_readout;
>>   };
>>
>>   struct sunxi_sid {
>>           void __iomem *base;
>>  + u32 value_offset;
>>   };
>>
>>   /* We read the entire key, due to a 32 bit read alignment requirement. Since we
>>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>>   {
>>           u32 sid_key;
>>
>>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
>>  + sid_key = ioread32be(sid->base + sid->value_offset +
>>  + round_down(offset, 4));
>
> This would probably be more logical to have this in sunxi_sid_read.

But it's here which really access the memory...

>
>>           sid_key >>= (offset % 4) * 8;
>>
>>           return sid_key; /* Only return the last byte */
>>  @@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>>           return 0;
>>   }
>>
>>  +static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
>>  + const unsigned int word,
>>  + u32 *out)
>>  +{
>>  + u32 reg_val;
>>  + unsigned long expire = jiffies + msecs_to_jiffies(250);
>>  +
>>  + /* Set word, lock access, and set read command */
>>  + reg_val = (word & SUN8I_SID_OFFSET_MASK)
>>  + << SUN8I_SID_OFFSET_SHIFT;
>>  + reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;
>
> You're not using those mask and shifts anywhere else, why not just
> define the value / macro you need directly?
>
>>  + reg_val |= SUN8I_SID_READ;
>>  + writel(reg_val, sid->base + SUN8I_SID_PRCTL);
>>  +
>>  + do {
>>  + reg_val = readl(sid->base + SUN8I_SID_PRCTL);
>>  + } while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));
>
> readl_poll_timeout?

Thanks. I'll check it.

>
>>  + if (reg_val & SUN8I_SID_READ)
>>  + return -EIO;
>>  +
>>  + if (out)
>>  + *out = readl(sid->base + SUN8I_SID_RDKEY);
>
> Why do you need that out parameter?

The read operation by registers can really return a value --
in fact, the fix to the pre-read value is a side effect.

>
> Thanks,
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
>> ?The H3 SoC have a bigger SID controller, which has its direct read
>> ?address at 0x200 position in the SID block, not 0x0.
>>
>> ?Also, H3 SID controller has some silicon bug that makes the direct read
>> ?value wrong at cold boot, add code to workaround the bug. (This bug has
>> ?already been fixed on A64 and later SoCs)
>>
>> ?Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> ?---
>> ?This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
>> ?controller.
>>
>> ??.../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
>> ??drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
>> ??2 files changed, 82 insertions(+), 2 deletions(-)
>>
>> ?diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?index d543ed3f5363..9ab9e75a6351 100644
>> ?--- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?@@ -1,7 +1,11 @@
>> ??Allwinner sunxi-sid
>>
>> ??Required properties:
>> ?-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
>> ?+- compatible: Should be one of the following (depending on your SoC):
>> ?+ "allwinner,sun4i-a10-sid"
>> ?+ "allwinner,sun7i-a20-sid"
>> ?+ "allwinner,sun8i-h3-sid"
>> ?+
>> ??- reg: Should contain registers location and length
>>
>> ??= Data cells =
>> ?@@ -19,3 +23,9 @@ Example for sun7i:
>> ??????????????????compatible = "allwinner,sun7i-a20-sid";
>> ??????????????????reg = <0x01c23800 0x200>
>> ??????????};
>> ?+
>> ?+Example for sun8i-h3:
>> ?+ sid at 01c14000 {
>> ?+ compatible = "allwinner,sun8i-h3-sid";
>> ?+ reg = <0x01c14000 0x400>;
>> ?+ };
>> ?diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
>> ?index 69524b67007f..476a161ff23a 100644
>> ?--- a/drivers/nvmem/sunxi_sid.c
>> ?+++ b/drivers/nvmem/sunxi_sid.c
>> ?@@ -25,6 +25,16 @@
>> ??#include <linux/slab.h>
>> ??#include <linux/random.h>
>>
>> ?+/* Registers and special values for doing register-based SID readout on H3 */
>> ?+#define SUN8I_SID_PRCTL 0x40
>> ?+#define SUN8I_SID_RDKEY 0x60
>> ?+
>> ?+#define SUN8I_SID_OP_LOCK 0xAC
>> ?+#define SUN8I_SID_OFFSET_MASK 0x1FF
>> ?+#define SUN8I_SID_OFFSET_SHIFT 16
>> ?+#define SUN8I_SID_LOCK_SHIFT 8
>> ?+#define SUN8I_SID_READ BIT(1)
>> ?+
>> ??static struct nvmem_config econfig = {
>> ??????????.name = "sunxi-sid",
>> ??????????.read_only = true,
>> ?@@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>> ??};
>>
>> ??struct sunxi_sid_cfg {
>> ?+ u32 value_offset;
>> ??????????u32 size;
>> ?+ bool need_register_readout;
>> ??};
>>
>> ??struct sunxi_sid {
>> ??????????void __iomem *base;
>> ?+ u32 value_offset;
>> ??};
>>
>> ??/* We read the entire key, due to a 32 bit read alignment requirement. Since we
>> ?@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>> ??{
>> ??????????u32 sid_key;
>>
>> ?- sid_key = ioread32be(sid->base + round_down(offset, 4));
>> ?+ sid_key = ioread32be(sid->base + sid->value_offset +
>> ?+ round_down(offset, 4));
>
> This would probably be more logical to have this in sunxi_sid_read.

But it's here which really access the memory...

>
>> ??????????sid_key >>= (offset % 4) * 8;
>>
>> ??????????return sid_key; /* Only return the last byte */
>> ?@@ -69,6 +83,33 @@ static int sunxi_sid_read(void *context, unsigned int offset,
>> ??????????return 0;
>> ??}
>>
>> ?+static int sun8i_sid_register_readout(const struct sunxi_sid *sid,
>> ?+ const unsigned int word,
>> ?+ u32 *out)
>> ?+{
>> ?+ u32 reg_val;
>> ?+ unsigned long expire = jiffies + msecs_to_jiffies(250);
>> ?+
>> ?+ /* Set word, lock access, and set read command */
>> ?+ reg_val = (word & SUN8I_SID_OFFSET_MASK)
>> ?+ << SUN8I_SID_OFFSET_SHIFT;
>> ?+ reg_val |= SUN8I_SID_OP_LOCK << SUN8I_SID_LOCK_SHIFT;
>
> You're not using those mask and shifts anywhere else, why not just
> define the value / macro you need directly?
>
>> ?+ reg_val |= SUN8I_SID_READ;
>> ?+ writel(reg_val, sid->base + SUN8I_SID_PRCTL);
>> ?+
>> ?+ do {
>> ?+ reg_val = readl(sid->base + SUN8I_SID_PRCTL);
>> ?+ } while (time_before(jiffies, expire) && (reg_val & SUN8I_SID_READ));
>
> readl_poll_timeout?

Thanks. I'll check it.

>
>> ?+ if (reg_val & SUN8I_SID_READ)
>> ?+ return -EIO;
>> ?+
>> ?+ if (out)
>> ?+ *out = readl(sid->base + SUN8I_SID_RDKEY);
>
> Why do you need that out parameter?

The read operation by registers can really return a value --
in fact, the fix to the pre-read value is a side effect.

>
> Thanks,
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
> 06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> >>  The H3 SoC have a bigger SID controller, which has its direct read
> >>  address at 0x200 position in the SID block, not 0x0.
> >>
> >>  Also, H3 SID controller has some silicon bug that makes the direct read
> >>  value wrong at cold boot, add code to workaround the bug. (This bug has
> >>  already been fixed on A64 and later SoCs)
> >>
> >>  Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >>  ---
> >>  This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> >>  controller.
> >>
> >>   .../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
> >>   drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
> >>   2 files changed, 82 insertions(+), 2 deletions(-)
> >>
> >>  diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  index d543ed3f5363..9ab9e75a6351 100644
> >>  --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  @@ -1,7 +1,11 @@
> >>   Allwinner sunxi-sid
> >>
> >>   Required properties:
> >>  -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> >>  +- compatible: Should be one of the following (depending on your SoC):
> >>  + "allwinner,sun4i-a10-sid"
> >>  + "allwinner,sun7i-a20-sid"
> >>  + "allwinner,sun8i-h3-sid"
> >>  +
> >>   - reg: Should contain registers location and length
> >>
> >>   = Data cells =
> >>  @@ -19,3 +23,9 @@ Example for sun7i:
> >>                   compatible = "allwinner,sun7i-a20-sid";
> >>                   reg = <0x01c23800 0x200>
> >>           };
> >>  +
> >>  +Example for sun8i-h3:
> >>  + sid@01c14000 {
> >>  + compatible = "allwinner,sun8i-h3-sid";
> >>  + reg = <0x01c14000 0x400>;
> >>  + };
> >>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> >>  index 69524b67007f..476a161ff23a 100644
> >>  --- a/drivers/nvmem/sunxi_sid.c
> >>  +++ b/drivers/nvmem/sunxi_sid.c
> >>  @@ -25,6 +25,16 @@
> >>   #include <linux/slab.h>
> >>   #include <linux/random.h>
> >>
> >>  +/* Registers and special values for doing register-based SID readout on H3 */
> >>  +#define SUN8I_SID_PRCTL 0x40
> >>  +#define SUN8I_SID_RDKEY 0x60
> >>  +
> >>  +#define SUN8I_SID_OP_LOCK 0xAC
> >>  +#define SUN8I_SID_OFFSET_MASK 0x1FF
> >>  +#define SUN8I_SID_OFFSET_SHIFT 16
> >>  +#define SUN8I_SID_LOCK_SHIFT 8
> >>  +#define SUN8I_SID_READ BIT(1)
> >>  +
> >>   static struct nvmem_config econfig = {
> >>           .name = "sunxi-sid",
> >>           .read_only = true,
> >>  @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
> >>   };
> >>
> >>   struct sunxi_sid_cfg {
> >>  + u32 value_offset;
> >>           u32 size;
> >>  + bool need_register_readout;
> >>   };
> >>
> >>   struct sunxi_sid {
> >>           void __iomem *base;
> >>  + u32 value_offset;
> >>   };
> >>
> >>   /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>   {
> >>           u32 sid_key;
> >>
> >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  + round_down(offset, 4));
> >
> > This would probably be more logical to have this in sunxi_sid_read.
> 
> But it's here which really access the memory...

This function is made to read a single register. What you want is to
offset all reads, and all the reads are made in sunxi_sid_read.

> >>  + if (reg_val & SUN8I_SID_READ)
> >>  + return -EIO;
> >>  +
> >>  + if (out)
> >>  + *out = readl(sid->base + SUN8I_SID_RDKEY);
> >
> > Why do you need that out parameter?
> 
> The read operation by registers can really return a value --
> in fact, the fix to the pre-read value is a side effect.

Yet, you're not using it at all, so this is dead code.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4861 bytes --]

On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
> 06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> >>  The H3 SoC have a bigger SID controller, which has its direct read
> >>  address at 0x200 position in the SID block, not 0x0.
> >>
> >>  Also, H3 SID controller has some silicon bug that makes the direct read
> >>  value wrong at cold boot, add code to workaround the bug. (This bug has
> >>  already been fixed on A64 and later SoCs)
> >>
> >>  Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> >>  ---
> >>  This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> >>  controller.
> >>
> >>   .../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
> >>   drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
> >>   2 files changed, 82 insertions(+), 2 deletions(-)
> >>
> >>  diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  index d543ed3f5363..9ab9e75a6351 100644
> >>  --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >>  @@ -1,7 +1,11 @@
> >>   Allwinner sunxi-sid
> >>
> >>   Required properties:
> >>  -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> >>  +- compatible: Should be one of the following (depending on your SoC):
> >>  + "allwinner,sun4i-a10-sid"
> >>  + "allwinner,sun7i-a20-sid"
> >>  + "allwinner,sun8i-h3-sid"
> >>  +
> >>   - reg: Should contain registers location and length
> >>
> >>   = Data cells =
> >>  @@ -19,3 +23,9 @@ Example for sun7i:
> >>                   compatible = "allwinner,sun7i-a20-sid";
> >>                   reg = <0x01c23800 0x200>
> >>           };
> >>  +
> >>  +Example for sun8i-h3:
> >>  + sid@01c14000 {
> >>  + compatible = "allwinner,sun8i-h3-sid";
> >>  + reg = <0x01c14000 0x400>;
> >>  + };
> >>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> >>  index 69524b67007f..476a161ff23a 100644
> >>  --- a/drivers/nvmem/sunxi_sid.c
> >>  +++ b/drivers/nvmem/sunxi_sid.c
> >>  @@ -25,6 +25,16 @@
> >>   #include <linux/slab.h>
> >>   #include <linux/random.h>
> >>
> >>  +/* Registers and special values for doing register-based SID readout on H3 */
> >>  +#define SUN8I_SID_PRCTL 0x40
> >>  +#define SUN8I_SID_RDKEY 0x60
> >>  +
> >>  +#define SUN8I_SID_OP_LOCK 0xAC
> >>  +#define SUN8I_SID_OFFSET_MASK 0x1FF
> >>  +#define SUN8I_SID_OFFSET_SHIFT 16
> >>  +#define SUN8I_SID_LOCK_SHIFT 8
> >>  +#define SUN8I_SID_READ BIT(1)
> >>  +
> >>   static struct nvmem_config econfig = {
> >>           .name = "sunxi-sid",
> >>           .read_only = true,
> >>  @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
> >>   };
> >>
> >>   struct sunxi_sid_cfg {
> >>  + u32 value_offset;
> >>           u32 size;
> >>  + bool need_register_readout;
> >>   };
> >>
> >>   struct sunxi_sid {
> >>           void __iomem *base;
> >>  + u32 value_offset;
> >>   };
> >>
> >>   /* We read the entire key, due to a 32 bit read alignment requirement. Since we
> >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>   {
> >>           u32 sid_key;
> >>
> >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  + round_down(offset, 4));
> >
> > This would probably be more logical to have this in sunxi_sid_read.
> 
> But it's here which really access the memory...

This function is made to read a single register. What you want is to
offset all reads, and all the reads are made in sunxi_sid_read.

> >>  + if (reg_val & SUN8I_SID_READ)
> >>  + return -EIO;
> >>  +
> >>  + if (out)
> >>  + *out = readl(sid->base + SUN8I_SID_RDKEY);
> >
> > Why do you need that out parameter?
> 
> The read operation by registers can really return a value --
> in fact, the fix to the pre-read value is a side effect.

Yet, you're not using it at all, so this is dead code.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
> 06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
> >> ?The H3 SoC have a bigger SID controller, which has its direct read
> >> ?address at 0x200 position in the SID block, not 0x0.
> >>
> >> ?Also, H3 SID controller has some silicon bug that makes the direct read
> >> ?value wrong at cold boot, add code to workaround the bug. (This bug has
> >> ?already been fixed on A64 and later SoCs)
> >>
> >> ?Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >> ?---
> >> ?This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
> >> ?controller.
> >>
> >> ??.../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
> >> ??drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
> >> ??2 files changed, 82 insertions(+), 2 deletions(-)
> >>
> >> ?diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >> ?index d543ed3f5363..9ab9e75a6351 100644
> >> ?--- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >> ?+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
> >> ?@@ -1,7 +1,11 @@
> >> ??Allwinner sunxi-sid
> >>
> >> ??Required properties:
> >> ?-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
> >> ?+- compatible: Should be one of the following (depending on your SoC):
> >> ?+ "allwinner,sun4i-a10-sid"
> >> ?+ "allwinner,sun7i-a20-sid"
> >> ?+ "allwinner,sun8i-h3-sid"
> >> ?+
> >> ??- reg: Should contain registers location and length
> >>
> >> ??= Data cells =
> >> ?@@ -19,3 +23,9 @@ Example for sun7i:
> >> ??????????????????compatible = "allwinner,sun7i-a20-sid";
> >> ??????????????????reg = <0x01c23800 0x200>
> >> ??????????};
> >> ?+
> >> ?+Example for sun8i-h3:
> >> ?+ sid at 01c14000 {
> >> ?+ compatible = "allwinner,sun8i-h3-sid";
> >> ?+ reg = <0x01c14000 0x400>;
> >> ?+ };
> >> ?diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
> >> ?index 69524b67007f..476a161ff23a 100644
> >> ?--- a/drivers/nvmem/sunxi_sid.c
> >> ?+++ b/drivers/nvmem/sunxi_sid.c
> >> ?@@ -25,6 +25,16 @@
> >> ??#include <linux/slab.h>
> >> ??#include <linux/random.h>
> >>
> >> ?+/* Registers and special values for doing register-based SID readout on H3 */
> >> ?+#define SUN8I_SID_PRCTL 0x40
> >> ?+#define SUN8I_SID_RDKEY 0x60
> >> ?+
> >> ?+#define SUN8I_SID_OP_LOCK 0xAC
> >> ?+#define SUN8I_SID_OFFSET_MASK 0x1FF
> >> ?+#define SUN8I_SID_OFFSET_SHIFT 16
> >> ?+#define SUN8I_SID_LOCK_SHIFT 8
> >> ?+#define SUN8I_SID_READ BIT(1)
> >> ?+
> >> ??static struct nvmem_config econfig = {
> >> ??????????.name = "sunxi-sid",
> >> ??????????.read_only = true,
> >> ?@@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
> >> ??};
> >>
> >> ??struct sunxi_sid_cfg {
> >> ?+ u32 value_offset;
> >> ??????????u32 size;
> >> ?+ bool need_register_readout;
> >> ??};
> >>
> >> ??struct sunxi_sid {
> >> ??????????void __iomem *base;
> >> ?+ u32 value_offset;
> >> ??};
> >>
> >> ??/* We read the entire key, due to a 32 bit read alignment requirement. Since we
> >> ?@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >> ??{
> >> ??????????u32 sid_key;
> >>
> >> ?- sid_key = ioread32be(sid->base + round_down(offset, 4));
> >> ?+ sid_key = ioread32be(sid->base + sid->value_offset +
> >> ?+ round_down(offset, 4));
> >
> > This would probably be more logical to have this in sunxi_sid_read.
> 
> But it's here which really access the memory...

This function is made to read a single register. What you want is to
offset all reads, and all the reads are made in sunxi_sid_read.

> >> ?+ if (reg_val & SUN8I_SID_READ)
> >> ?+ return -EIO;
> >> ?+
> >> ?+ if (out)
> >> ?+ *out = readl(sid->base + SUN8I_SID_RDKEY);
> >
> > Why do you need that out parameter?
> 
> The read operation by registers can really return a value --
> in fact, the fix to the pre-read value is a side effect.

Yet, you're not using it at all, so this is dead code.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170207/99252249/attachment.sig>

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

07.02.2017, 17:25, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
>>  06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>>  > On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
>>  >>  The H3 SoC have a bigger SID controller, which has its direct read
>>  >>  address at 0x200 position in the SID block, not 0x0.
>>  >>
>>  >>  Also, H3 SID controller has some silicon bug that makes the direct read
>>  >>  value wrong at cold boot, add code to workaround the bug. (This bug has
>>  >>  already been fixed on A64 and later SoCs)
>>  >>
>>  >>  Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
>>  >>  ---
>>  >>  This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
>>  >>  controller.
>>  >>
>>  >>   .../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
>>  >>   drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
>>  >>   2 files changed, 82 insertions(+), 2 deletions(-)
>>  >>
>>  >>  diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  >>  index d543ed3f5363..9ab9e75a6351 100644
>>  >>  --- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  >>  +++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>>  >>  @@ -1,7 +1,11 @@
>>  >>   Allwinner sunxi-sid
>>  >>
>>  >>   Required properties:
>>  >>  -- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
>>  >>  +- compatible: Should be one of the following (depending on your SoC):
>>  >>  + "allwinner,sun4i-a10-sid"
>>  >>  + "allwinner,sun7i-a20-sid"
>>  >>  + "allwinner,sun8i-h3-sid"
>>  >>  +
>>  >>   - reg: Should contain registers location and length
>>  >>
>>  >>   = Data cells =
>>  >>  @@ -19,3 +23,9 @@ Example for sun7i:
>>  >>                   compatible = "allwinner,sun7i-a20-sid";
>>  >>                   reg = <0x01c23800 0x200>
>>  >>           };
>>  >>  +
>>  >>  +Example for sun8i-h3:
>>  >>  + sid@01c14000 {
>>  >>  + compatible = "allwinner,sun8i-h3-sid";
>>  >>  + reg = <0x01c14000 0x400>;
>>  >>  + };
>>  >>  diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
>>  >>  index 69524b67007f..476a161ff23a 100644
>>  >>  --- a/drivers/nvmem/sunxi_sid.c
>>  >>  +++ b/drivers/nvmem/sunxi_sid.c
>>  >>  @@ -25,6 +25,16 @@
>>  >>   #include <linux/slab.h>
>>  >>   #include <linux/random.h>
>>  >>
>>  >>  +/* Registers and special values for doing register-based SID readout on H3 */
>>  >>  +#define SUN8I_SID_PRCTL 0x40
>>  >>  +#define SUN8I_SID_RDKEY 0x60
>>  >>  +
>>  >>  +#define SUN8I_SID_OP_LOCK 0xAC
>>  >>  +#define SUN8I_SID_OFFSET_MASK 0x1FF
>>  >>  +#define SUN8I_SID_OFFSET_SHIFT 16
>>  >>  +#define SUN8I_SID_LOCK_SHIFT 8
>>  >>  +#define SUN8I_SID_READ BIT(1)
>>  >>  +
>>  >>   static struct nvmem_config econfig = {
>>  >>           .name = "sunxi-sid",
>>  >>           .read_only = true,
>>  >>  @@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>>  >>   };
>>  >>
>>  >>   struct sunxi_sid_cfg {
>>  >>  + u32 value_offset;
>>  >>           u32 size;
>>  >>  + bool need_register_readout;
>>  >>   };
>>  >>
>>  >>   struct sunxi_sid {
>>  >>           void __iomem *base;
>>  >>  + u32 value_offset;
>>  >>   };
>>  >>
>>  >>   /* We read the entire key, due to a 32 bit read alignment requirement. Since we
>>  >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>>  >>   {
>>  >>           u32 sid_key;
>>  >>
>>  >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
>>  >>  + sid_key = ioread32be(sid->base + sid->value_offset +
>>  >>  + round_down(offset, 4));
>>  >
>>  > This would probably be more logical to have this in sunxi_sid_read.
>>
>>  But it's here which really access the memory...
>
> This function is made to read a single register. What you want is to
> offset all reads, and all the reads are made in sunxi_sid_read.

I think the semantic of this function is to read out one byte from SID,
not read out a single register from SID; the parameter passed into it is
also a const struct *sunxi_sid, so I think make the offset here is right.

>
>>  >>  + if (reg_val & SUN8I_SID_READ)
>>  >>  + return -EIO;
>>  >>  +
>>  >>  + if (out)
>>  >>  + *out = readl(sid->base + SUN8I_SID_RDKEY);
>>  >
>>  > Why do you need that out parameter?
>>
>>  The read operation by registers can really return a value --
>>  in fact, the fix to the pre-read value is a side effect.
>
> Yet, you're not using it at all, so this is dead code.

Removed it.

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Icenowy Zheng]

07.02.2017, 17:25, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Mon, Feb 06, 2017 at 04:56:55PM +0800, Icenowy Zheng wrote:
>> ?06.02.2017, 16:54, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> ?> On Thu, Feb 02, 2017 at 09:13:37PM +0800, Icenowy Zheng wrote:
>> ?>> ?The H3 SoC have a bigger SID controller, which has its direct read
>> ?>> ?address at 0x200 position in the SID block, not 0x0.
>> ?>>
>> ?>> ?Also, H3 SID controller has some silicon bug that makes the direct read
>> ?>> ?value wrong at cold boot, add code to workaround the bug. (This bug has
>> ?>> ?already been fixed on A64 and later SoCs)
>> ?>>
>> ?>> ?Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> ?>> ?---
>> ?>> ?This patch is the part of [PATCH v2 1/1] that adds support for H3 SID
>> ?>> ?controller.
>> ?>>
>> ?>> ??.../bindings/nvmem/allwinner,sunxi-sid.txt | 12 +++-
>> ?>> ??drivers/nvmem/sunxi_sid.c | 72 +++++++++++++++++++++-
>> ?>> ??2 files changed, 82 insertions(+), 2 deletions(-)
>> ?>>
>> ?>> ?diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?>> ?index d543ed3f5363..9ab9e75a6351 100644
>> ?>> ?--- a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?>> ?+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
>> ?>> ?@@ -1,7 +1,11 @@
>> ?>> ??Allwinner sunxi-sid
>> ?>>
>> ?>> ??Required properties:
>> ?>> ?-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
>> ?>> ?+- compatible: Should be one of the following (depending on your SoC):
>> ?>> ?+ "allwinner,sun4i-a10-sid"
>> ?>> ?+ "allwinner,sun7i-a20-sid"
>> ?>> ?+ "allwinner,sun8i-h3-sid"
>> ?>> ?+
>> ?>> ??- reg: Should contain registers location and length
>> ?>>
>> ?>> ??= Data cells =
>> ?>> ?@@ -19,3 +23,9 @@ Example for sun7i:
>> ?>> ??????????????????compatible = "allwinner,sun7i-a20-sid";
>> ?>> ??????????????????reg = <0x01c23800 0x200>
>> ?>> ??????????};
>> ?>> ?+
>> ?>> ?+Example for sun8i-h3:
>> ?>> ?+ sid at 01c14000 {
>> ?>> ?+ compatible = "allwinner,sun8i-h3-sid";
>> ?>> ?+ reg = <0x01c14000 0x400>;
>> ?>> ?+ };
>> ?>> ?diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
>> ?>> ?index 69524b67007f..476a161ff23a 100644
>> ?>> ?--- a/drivers/nvmem/sunxi_sid.c
>> ?>> ?+++ b/drivers/nvmem/sunxi_sid.c
>> ?>> ?@@ -25,6 +25,16 @@
>> ?>> ??#include <linux/slab.h>
>> ?>> ??#include <linux/random.h>
>> ?>>
>> ?>> ?+/* Registers and special values for doing register-based SID readout on H3 */
>> ?>> ?+#define SUN8I_SID_PRCTL 0x40
>> ?>> ?+#define SUN8I_SID_RDKEY 0x60
>> ?>> ?+
>> ?>> ?+#define SUN8I_SID_OP_LOCK 0xAC
>> ?>> ?+#define SUN8I_SID_OFFSET_MASK 0x1FF
>> ?>> ?+#define SUN8I_SID_OFFSET_SHIFT 16
>> ?>> ?+#define SUN8I_SID_LOCK_SHIFT 8
>> ?>> ?+#define SUN8I_SID_READ BIT(1)
>> ?>> ?+
>> ?>> ??static struct nvmem_config econfig = {
>> ?>> ??????????.name = "sunxi-sid",
>> ?>> ??????????.read_only = true,
>> ?>> ?@@ -34,11 +44,14 @@ static struct nvmem_config econfig = {
>> ?>> ??};
>> ?>>
>> ?>> ??struct sunxi_sid_cfg {
>> ?>> ?+ u32 value_offset;
>> ?>> ??????????u32 size;
>> ?>> ?+ bool need_register_readout;
>> ?>> ??};
>> ?>>
>> ?>> ??struct sunxi_sid {
>> ?>> ??????????void __iomem *base;
>> ?>> ?+ u32 value_offset;
>> ?>> ??};
>> ?>>
>> ?>> ??/* We read the entire key, due to a 32 bit read alignment requirement. Since we
>> ?>> ?@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
>> ?>> ??{
>> ?>> ??????????u32 sid_key;
>> ?>>
>> ?>> ?- sid_key = ioread32be(sid->base + round_down(offset, 4));
>> ?>> ?+ sid_key = ioread32be(sid->base + sid->value_offset +
>> ?>> ?+ round_down(offset, 4));
>> ?>
>> ?> This would probably be more logical to have this in sunxi_sid_read.
>>
>> ?But it's here which really access the memory...
>
> This function is made to read a single register. What you want is to
> offset all reads, and all the reads are made in sunxi_sid_read.

I think the semantic of this function is to read out one byte from SID,
not read out a single register from SID; the parameter passed into it is
also a const struct *sunxi_sid, so I think make the offset here is right.

>
>> ?>> ?+ if (reg_val & SUN8I_SID_READ)
>> ?>> ?+ return -EIO;
>> ?>> ?+
>> ?>> ?+ if (out)
>> ?>> ?+ *out = readl(sid->base + SUN8I_SID_RDKEY);
>> ?>
>> ?> Why do you need that out parameter?
>>
>> ?The read operation by registers can really return a value --
>> ?in fact, the fix to the pre-read value is a side effect.
>
> Yet, you're not using it at all, so this is dead code.

Removed it.

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

On Tue, Feb 07, 2017 at 09:36:35PM +0800, Icenowy Zheng wrote:
> >>  >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>  >>   {
> >>  >>           u32 sid_key;
> >>  >>
> >>  >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  >>  + round_down(offset, 4));
> >>  >
> >>  > This would probably be more logical to have this in sunxi_sid_read.
> >>
> >>  But it's here which really access the memory...
> >
> > This function is made to read a single register. What you want is to
> > offset all reads, and all the reads are made in sunxi_sid_read.
> 
> I think the semantic of this function is to read out one byte from SID,
> not read out a single register from SID; the parameter passed into it is
> also a const struct *sunxi_sid, so I think make the offset here is right.

You need to offset *all* register reads, it makes much more sense to
do that offset in the function that reads all the registers, and not
just one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

On Tue, Feb 07, 2017 at 09:36:35PM +0800, Icenowy Zheng wrote:
> >>  >>  @@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >>  >>   {
> >>  >>           u32 sid_key;
> >>  >>
> >>  >>  - sid_key = ioread32be(sid->base + round_down(offset, 4));
> >>  >>  + sid_key = ioread32be(sid->base + sid->value_offset +
> >>  >>  + round_down(offset, 4));
> >>  >
> >>  > This would probably be more logical to have this in sunxi_sid_read.
> >>
> >>  But it's here which really access the memory...
> >
> > This function is made to read a single register. What you want is to
> > offset all reads, and all the reads are made in sunxi_sid_read.
> 
> I think the semantic of this function is to read out one byte from SID,
> not read out a single register from SID; the parameter passed into it is
> also a const struct *sunxi_sid, so I think make the offset here is right.

You need to offset *all* register reads, it makes much more sense to
do that offset in the function that reads all the registers, and not
just one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v3 2/3] nvmem: sunxi-sid: add support for H3's SID controller

[Maxime Ripard]

On Tue, Feb 07, 2017 at 09:36:35PM +0800, Icenowy Zheng wrote:
> >> ?>> ?@@ -51,7 +64,8 @@ static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
> >> ?>> ??{
> >> ?>> ??????????u32 sid_key;
> >> ?>>
> >> ?>> ?- sid_key = ioread32be(sid->base + round_down(offset, 4));
> >> ?>> ?+ sid_key = ioread32be(sid->base + sid->value_offset +
> >> ?>> ?+ round_down(offset, 4));
> >> ?>
> >> ?> This would probably be more logical to have this in sunxi_sid_read.
> >>
> >> ?But it's here which really access the memory...
> >
> > This function is made to read a single register. What you want is to
> > offset all reads, and all the reads are made in sunxi_sid_read.
> 
> I think the semantic of this function is to read out one byte from SID,
> not read out a single register from SID; the parameter passed into it is
> also a const struct *sunxi_sid, so I think make the offset here is right.

You need to offset *all* register reads, it makes much more sense to
do that offset in the function that reads all the registers, and not
just one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170210/616ea9da/attachment.sig>