[PATCH 1/2] dt-bindings: nvmem: add binding for I/O mapped NVMEM

[PATCH 1/2] dt-bindings: nvmem: add binding for I/O mapped NVMEM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

NVMEM on some devices can be accessed using I/O mapping. For example on
Broadcom MIPS and ARM Northstar platforms NVRAM can be accessed that
way.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/nvmem/iomap.yaml      | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/iomap.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/iomap.yaml b/Documentation/devicetree/bindings/nvmem/iomap.yaml
new file mode 100644
index 000000000000..ec8764598061
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/iomap.yaml
@@ -0,0 +1,29 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/iomap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I/O mapped NVMEM
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+allOf:
+  - $ref: "nvmem.yaml#"
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - brcm,nvram
+      - const: nvmem-iomap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    nvram@1eff0000 {
+            compatible = "brcm,nvram", "nvmem-iomap";
+            reg = <0x1eff0000 0x10000>;
+    };
-- 
2.26.2

[PATCH 1/2] dt-bindings: nvmem: add binding for I/O mapped NVMEM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

NVMEM on some devices can be accessed using I/O mapping. For example on
Broadcom MIPS and ARM Northstar platforms NVRAM can be accessed that
way.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/nvmem/iomap.yaml      | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/iomap.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/iomap.yaml b/Documentation/devicetree/bindings/nvmem/iomap.yaml
new file mode 100644
index 000000000000..ec8764598061
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/iomap.yaml
@@ -0,0 +1,29 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/iomap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I/O mapped NVMEM
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+allOf:
+  - $ref: "nvmem.yaml#"
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - brcm,nvram
+      - const: nvmem-iomap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    nvram@1eff0000 {
+            compatible = "brcm,nvram", "nvmem-iomap";
+            reg = <0x1eff0000 0x10000>;
+    };
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This is a generic NVMEM access method used e.g. by Broadcom for their
NVRAM on MIPS and Northstar devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/nvmem/Kconfig  |  7 +++
 drivers/nvmem/Makefile |  2 +
 drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+)
 create mode 100644 drivers/nvmem/iomap.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 75d2594c16e1..3d5c5684685d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -278,4 +278,11 @@ config NVMEM_RMEM
 
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem-rmem.
+
+config NVMEM_IOMAP
+	tristate "I/O mapped NVMEM support"
+	depends on HAS_IOMEM
+	help
+	  This driver supports NVMEM that can be accessed using I/O mapping.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 5376b8e0dae5..88a3b6979c53 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
 nvmem_sprd_efuse-y		:= sprd-efuse.o
 obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
 nvmem-rmem-y			:= rmem.o
+obj-$(CONFIG_NVMEM_IOMAP)	+= nvmem_iomap.o
+nvmem_iomap-y			:= iomap.o
diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
new file mode 100644
index 000000000000..ab6b40858a64
--- /dev/null
+++ b/drivers/nvmem/iomap.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+
+struct iomap {
+	struct device *dev;
+	void __iomem *base;
+};
+
+static int iomap_read(void *context, unsigned int offset, void *val,
+		      size_t bytes)
+{
+	struct iomap *priv = context;
+	u8 *src = priv->base + offset;
+	u8 *dst = val;
+	size_t tmp;
+
+	tmp = offset % 4;
+	memcpy_fromio(dst, src, tmp);
+	dst += tmp;
+	src += tmp;
+	bytes -= tmp;
+
+	tmp = rounddown(bytes, 4);
+	__ioread32_copy(dst, src, tmp / 4);
+	dst += tmp;
+	src += tmp;
+	bytes -= tmp;
+
+	memcpy_fromio(dst, src, bytes);
+
+	return 0;
+}
+
+static int iomap_probe(struct platform_device *pdev)
+{
+	struct nvmem_config config = {
+		.name = "iomap",
+		.reg_read = iomap_read,
+	};
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct iomap *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Failed to get resource\n");
+		return -ENODEV;
+	}
+
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "Failed to map resource: %ld\n", PTR_ERR(priv->base));
+		return PTR_ERR(priv->base);
+	}
+
+	config.dev = dev;
+	config.priv = priv;
+	config.size = resource_size(res);
+
+	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
+}
+
+static const struct of_device_id iomap_of_match_table[] = {
+	{ .compatible = "brcm,nvram", },
+	{ .compatible = "nvmem-iomap", },
+	{},
+};
+
+static struct platform_driver iomap_driver = {
+	.probe = iomap_probe,
+	.driver = {
+		.name = "nvmem_iomap",
+		.of_match_table = iomap_of_match_table,
+	},
+};
+
+static int __init iomap_init(void)
+{
+	return platform_driver_register(&iomap_driver);
+}
+
+subsys_initcall_sync(iomap_init);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, iomap_of_match_table);
-- 
2.26.2

[PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This is a generic NVMEM access method used e.g. by Broadcom for their
NVRAM on MIPS and Northstar devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/nvmem/Kconfig  |  7 +++
 drivers/nvmem/Makefile |  2 +
 drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+)
 create mode 100644 drivers/nvmem/iomap.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 75d2594c16e1..3d5c5684685d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -278,4 +278,11 @@ config NVMEM_RMEM
 
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem-rmem.
+
+config NVMEM_IOMAP
+	tristate "I/O mapped NVMEM support"
+	depends on HAS_IOMEM
+	help
+	  This driver supports NVMEM that can be accessed using I/O mapping.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 5376b8e0dae5..88a3b6979c53 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
 nvmem_sprd_efuse-y		:= sprd-efuse.o
 obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
 nvmem-rmem-y			:= rmem.o
+obj-$(CONFIG_NVMEM_IOMAP)	+= nvmem_iomap.o
+nvmem_iomap-y			:= iomap.o
diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
new file mode 100644
index 000000000000..ab6b40858a64
--- /dev/null
+++ b/drivers/nvmem/iomap.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
+ */
+
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+
+struct iomap {
+	struct device *dev;
+	void __iomem *base;
+};
+
+static int iomap_read(void *context, unsigned int offset, void *val,
+		      size_t bytes)
+{
+	struct iomap *priv = context;
+	u8 *src = priv->base + offset;
+	u8 *dst = val;
+	size_t tmp;
+
+	tmp = offset % 4;
+	memcpy_fromio(dst, src, tmp);
+	dst += tmp;
+	src += tmp;
+	bytes -= tmp;
+
+	tmp = rounddown(bytes, 4);
+	__ioread32_copy(dst, src, tmp / 4);
+	dst += tmp;
+	src += tmp;
+	bytes -= tmp;
+
+	memcpy_fromio(dst, src, bytes);
+
+	return 0;
+}
+
+static int iomap_probe(struct platform_device *pdev)
+{
+	struct nvmem_config config = {
+		.name = "iomap",
+		.reg_read = iomap_read,
+	};
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct iomap *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "Failed to get resource\n");
+		return -ENODEV;
+	}
+
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base)) {
+		dev_err(dev, "Failed to map resource: %ld\n", PTR_ERR(priv->base));
+		return PTR_ERR(priv->base);
+	}
+
+	config.dev = dev;
+	config.priv = priv;
+	config.size = resource_size(res);
+
+	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
+}
+
+static const struct of_device_id iomap_of_match_table[] = {
+	{ .compatible = "brcm,nvram", },
+	{ .compatible = "nvmem-iomap", },
+	{},
+};
+
+static struct platform_driver iomap_driver = {
+	.probe = iomap_probe,
+	.driver = {
+		.name = "nvmem_iomap",
+		.of_match_table = iomap_of_match_table,
+	},
+};
+
+static int __init iomap_init(void)
+{
+	return platform_driver_register(&iomap_driver);
+}
+
+subsys_initcall_sync(iomap_init);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, iomap_of_match_table);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Srinivas Kandagatla]

On 04/03/2021 14:41, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This is a generic NVMEM access method used e.g. by Broadcom for their
> NVRAM on MIPS and Northstar devices.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   drivers/nvmem/Kconfig  |  7 +++
>   drivers/nvmem/Makefile |  2 +
>   drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 108 insertions(+)
>   create mode 100644 drivers/nvmem/iomap.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 75d2594c16e1..3d5c5684685d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -278,4 +278,11 @@ config NVMEM_RMEM
>   
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-rmem.
> +
> +config NVMEM_IOMAP
> +	tristate "I/O mapped NVMEM support"
> +	depends on HAS_IOMEM
> +	help
> +	  This driver supports NVMEM that can be accessed using I/O mapping.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 5376b8e0dae5..88a3b6979c53 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
>   nvmem_sprd_efuse-y		:= sprd-efuse.o
>   obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
>   nvmem-rmem-y			:= rmem.o
> +obj-$(CONFIG_NVMEM_IOMAP)	+= nvmem_iomap.o
> +nvmem_iomap-y			:= iomap.o
> diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
> new file mode 100644
> index 000000000000..ab6b40858a64
> --- /dev/null
> +++ b/drivers/nvmem/iomap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +
> +struct iomap {
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +static int iomap_read(void *context, unsigned int offset, void *val,
> +		      size_t bytes)
> +{
> +	struct iomap *priv = context;
> +	u8 *src = priv->base + offset;
> +	u8 *dst = val;
> +	size_t tmp;
> +
> +	tmp = offset % 4;
> +	memcpy_fromio(dst, src, tmp);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	tmp = rounddown(bytes, 4);
> +	__ioread32_copy(dst, src, tmp / 4);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	memcpy_fromio(dst, src, bytes);
> +


You could just do this!

	while (bytes--)
		*val++ = readb(priv->base + offset + i++);



> +	return 0;
> +}
> +
> +static int iomap_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config config = {
> +		.name = "iomap",
> +		.reg_read = iomap_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct iomap *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);


> +	if (!res) {
> +		dev_err(dev, "Failed to get resource\n");
> +		return -ENODEV;
> +	}

This is redundant check and error message!

You can just do :
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
priv->base = devm_ioremap_resource(dev, res);


> +
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "Failed to map resource: %ld\n", PTR_ERR(priv->base));
This message is redundant!

> +		return PTR_ERR(priv->base);
> +	}


> +
> +	config.dev = dev;
> +	config.priv = priv;
> +	config.size = resource_size(res);
> +
> +	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id iomap_of_match_table[] = {
> +	{ .compatible = "brcm,nvram", },
> +	{ .compatible = "nvmem-iomap", },

Generic compatible does not really makes sense at all, you can probably 
consider adding some generic functions rather than having a compatible, 
in case you want to reduce code duplication!


> +	{},
> +};
> +
> +static struct platform_driver iomap_driver = {
> +	.probe = iomap_probe,
> +	.driver = {
> +		.name = "nvmem_iomap",
> +		.of_match_table = iomap_of_match_table,
> +	},
> +};
> +
> +static int __init iomap_init(void)
> +{
> +	return platform_driver_register(&iomap_driver);
> +}
> +
> +subsys_initcall_sync(iomap_init);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL v2");

it should be
MODULE_LICENSE("GPL");


> +MODULE_DEVICE_TABLE(of, iomap_of_match_table);
> 

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Srinivas Kandagatla]

On 04/03/2021 14:41, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This is a generic NVMEM access method used e.g. by Broadcom for their
> NVRAM on MIPS and Northstar devices.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   drivers/nvmem/Kconfig  |  7 +++
>   drivers/nvmem/Makefile |  2 +
>   drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 108 insertions(+)
>   create mode 100644 drivers/nvmem/iomap.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 75d2594c16e1..3d5c5684685d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -278,4 +278,11 @@ config NVMEM_RMEM
>   
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-rmem.
> +
> +config NVMEM_IOMAP
> +	tristate "I/O mapped NVMEM support"
> +	depends on HAS_IOMEM
> +	help
> +	  This driver supports NVMEM that can be accessed using I/O mapping.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 5376b8e0dae5..88a3b6979c53 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
>   nvmem_sprd_efuse-y		:= sprd-efuse.o
>   obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
>   nvmem-rmem-y			:= rmem.o
> +obj-$(CONFIG_NVMEM_IOMAP)	+= nvmem_iomap.o
> +nvmem_iomap-y			:= iomap.o
> diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
> new file mode 100644
> index 000000000000..ab6b40858a64
> --- /dev/null
> +++ b/drivers/nvmem/iomap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +
> +struct iomap {
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +static int iomap_read(void *context, unsigned int offset, void *val,
> +		      size_t bytes)
> +{
> +	struct iomap *priv = context;
> +	u8 *src = priv->base + offset;
> +	u8 *dst = val;
> +	size_t tmp;
> +
> +	tmp = offset % 4;
> +	memcpy_fromio(dst, src, tmp);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	tmp = rounddown(bytes, 4);
> +	__ioread32_copy(dst, src, tmp / 4);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	memcpy_fromio(dst, src, bytes);
> +


You could just do this!

	while (bytes--)
		*val++ = readb(priv->base + offset + i++);



> +	return 0;
> +}
> +
> +static int iomap_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config config = {
> +		.name = "iomap",
> +		.reg_read = iomap_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct iomap *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);


> +	if (!res) {
> +		dev_err(dev, "Failed to get resource\n");
> +		return -ENODEV;
> +	}

This is redundant check and error message!

You can just do :
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
priv->base = devm_ioremap_resource(dev, res);


> +
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "Failed to map resource: %ld\n", PTR_ERR(priv->base));
This message is redundant!

> +		return PTR_ERR(priv->base);
> +	}


> +
> +	config.dev = dev;
> +	config.priv = priv;
> +	config.size = resource_size(res);
> +
> +	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id iomap_of_match_table[] = {
> +	{ .compatible = "brcm,nvram", },
> +	{ .compatible = "nvmem-iomap", },

Generic compatible does not really makes sense at all, you can probably 
consider adding some generic functions rather than having a compatible, 
in case you want to reduce code duplication!


> +	{},
> +};
> +
> +static struct platform_driver iomap_driver = {
> +	.probe = iomap_probe,
> +	.driver = {
> +		.name = "nvmem_iomap",
> +		.of_match_table = iomap_of_match_table,
> +	},
> +};
> +
> +static int __init iomap_init(void)
> +{
> +	return platform_driver_register(&iomap_driver);
> +}
> +
> +subsys_initcall_sync(iomap_init);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL v2");

it should be
MODULE_LICENSE("GPL");


> +MODULE_DEVICE_TABLE(of, iomap_of_match_table);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

On 05.03.2021 11:02, Srinivas Kandagatla wrote:
> On 04/03/2021 14:41, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This is a generic NVMEM access method used e.g. by Broadcom for their
>> NVRAM on MIPS and Northstar devices.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   drivers/nvmem/Kconfig  |  7 +++
>>   drivers/nvmem/Makefile |  2 +
>>   drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 108 insertions(+)
>>   create mode 100644 drivers/nvmem/iomap.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 75d2594c16e1..3d5c5684685d 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -278,4 +278,11 @@ config NVMEM_RMEM
>>         This driver can also be built as a module. If so, the module
>>         will be called nvmem-rmem.
>> +
>> +config NVMEM_IOMAP
>> +    tristate "I/O mapped NVMEM support"
>> +    depends on HAS_IOMEM
>> +    help
>> +      This driver supports NVMEM that can be accessed using I/O mapping.
>> +
>>   endif
>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>> index 5376b8e0dae5..88a3b6979c53 100644
>> --- a/drivers/nvmem/Makefile
>> +++ b/drivers/nvmem/Makefile
>> @@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)    += nvmem_sprd_efuse.o
>>   nvmem_sprd_efuse-y        := sprd-efuse.o
>>   obj-$(CONFIG_NVMEM_RMEM)     += nvmem-rmem.o
>>   nvmem-rmem-y            := rmem.o
>> +obj-$(CONFIG_NVMEM_IOMAP)    += nvmem_iomap.o
>> +nvmem_iomap-y            := iomap.o
>> diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
>> new file mode 100644
>> index 000000000000..ab6b40858a64
>> --- /dev/null
>> +++ b/drivers/nvmem/iomap.c
>> @@ -0,0 +1,99 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +
>> +struct iomap {
>> +    struct device *dev;
>> +    void __iomem *base;
>> +};
>> +
>> +static int iomap_read(void *context, unsigned int offset, void *val,
>> +              size_t bytes)
>> +{
>> +    struct iomap *priv = context;
>> +    u8 *src = priv->base + offset;
>> +    u8 *dst = val;
>> +    size_t tmp;
>> +
>> +    tmp = offset % 4;
>> +    memcpy_fromio(dst, src, tmp);
>> +    dst += tmp;
>> +    src += tmp;
>> +    bytes -= tmp;
>> +
>> +    tmp = rounddown(bytes, 4);
>> +    __ioread32_copy(dst, src, tmp / 4);
>> +    dst += tmp;
>> +    src += tmp;
>> +    bytes -= tmp;
>> +
>> +    memcpy_fromio(dst, src, bytes);
>> +
> 
> 
> You could just do this!
> 
>      while (bytes--)
>          *val++ = readb(priv->base + offset + i++);

Do you mean that as replacement for "memcpy_fromio" or the whole
function code?
The reason for using __ioread32_copy() was to improve reading
performance (using aligned 32 bit access where possible). I'm not sure
if that really matters?

P.S.
Please don't yell at me in every sentence :( Makes me a bit sad :(

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

On 05.03.2021 11:02, Srinivas Kandagatla wrote:
> On 04/03/2021 14:41, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This is a generic NVMEM access method used e.g. by Broadcom for their
>> NVRAM on MIPS and Northstar devices.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   drivers/nvmem/Kconfig  |  7 +++
>>   drivers/nvmem/Makefile |  2 +
>>   drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 108 insertions(+)
>>   create mode 100644 drivers/nvmem/iomap.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 75d2594c16e1..3d5c5684685d 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -278,4 +278,11 @@ config NVMEM_RMEM
>>         This driver can also be built as a module. If so, the module
>>         will be called nvmem-rmem.
>> +
>> +config NVMEM_IOMAP
>> +    tristate "I/O mapped NVMEM support"
>> +    depends on HAS_IOMEM
>> +    help
>> +      This driver supports NVMEM that can be accessed using I/O mapping.
>> +
>>   endif
>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>> index 5376b8e0dae5..88a3b6979c53 100644
>> --- a/drivers/nvmem/Makefile
>> +++ b/drivers/nvmem/Makefile
>> @@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)    += nvmem_sprd_efuse.o
>>   nvmem_sprd_efuse-y        := sprd-efuse.o
>>   obj-$(CONFIG_NVMEM_RMEM)     += nvmem-rmem.o
>>   nvmem-rmem-y            := rmem.o
>> +obj-$(CONFIG_NVMEM_IOMAP)    += nvmem_iomap.o
>> +nvmem_iomap-y            := iomap.o
>> diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
>> new file mode 100644
>> index 000000000000..ab6b40858a64
>> --- /dev/null
>> +++ b/drivers/nvmem/iomap.c
>> @@ -0,0 +1,99 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +
>> +struct iomap {
>> +    struct device *dev;
>> +    void __iomem *base;
>> +};
>> +
>> +static int iomap_read(void *context, unsigned int offset, void *val,
>> +              size_t bytes)
>> +{
>> +    struct iomap *priv = context;
>> +    u8 *src = priv->base + offset;
>> +    u8 *dst = val;
>> +    size_t tmp;
>> +
>> +    tmp = offset % 4;
>> +    memcpy_fromio(dst, src, tmp);
>> +    dst += tmp;
>> +    src += tmp;
>> +    bytes -= tmp;
>> +
>> +    tmp = rounddown(bytes, 4);
>> +    __ioread32_copy(dst, src, tmp / 4);
>> +    dst += tmp;
>> +    src += tmp;
>> +    bytes -= tmp;
>> +
>> +    memcpy_fromio(dst, src, bytes);
>> +
> 
> 
> You could just do this!
> 
>      while (bytes--)
>          *val++ = readb(priv->base + offset + i++);

Do you mean that as replacement for "memcpy_fromio" or the whole
function code?
The reason for using __ioread32_copy() was to improve reading
performance (using aligned 32 bit access where possible). I'm not sure
if that really matters?

P.S.
Please don't yell at me in every sentence :( Makes me a bit sad :(

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Srinivas Kandagatla]

On 05/03/2021 10:24, Rafał Miłecki wrote:
>>>
>>> +static int iomap_read(void *context, unsigned int offset, void *val,
>>> +              size_t bytes)
>>> +{
>>> +    struct iomap *priv = context;
>>> +    u8 *src = priv->base + offset;
>>> +    u8 *dst = val;
>>> +    size_t tmp;
>>> +
>>> +    tmp = offset % 4;
>>> +    memcpy_fromio(dst, src, tmp);
>>> +    dst += tmp;
>>> +    src += tmp;
>>> +    bytes -= tmp;
>>> +
>>> +    tmp = rounddown(bytes, 4);
>>> +    __ioread32_copy(dst, src, tmp / 4);
>>> +    dst += tmp;
>>> +    src += tmp;
>>> +    bytes -= tmp;
>>> +
>>> +    memcpy_fromio(dst, src, bytes);
>>> +
>>
>>
>> You could just do this!
>>
>>      while (bytes--)
>>          *val++ = readb(priv->base + offset + i++);
> 
> Do you mean that as replacement for "memcpy_fromio" or the whole
> function code?

Yes please!

> The reason for using __ioread32_copy() was to improve reading
> performance (using aligned 32 bit access where possible). I'm not sure
> if that really matters?

Just simple while loop is much readable than the previous code TBH!

> 

> P.S.
> Please don't yell at me in every sentence :( Makes me a bit sad :(
Sorry!! I did not mean anything as such! :-)

--srini

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Srinivas Kandagatla]

On 05/03/2021 10:24, Rafał Miłecki wrote:
>>>
>>> +static int iomap_read(void *context, unsigned int offset, void *val,
>>> +              size_t bytes)
>>> +{
>>> +    struct iomap *priv = context;
>>> +    u8 *src = priv->base + offset;
>>> +    u8 *dst = val;
>>> +    size_t tmp;
>>> +
>>> +    tmp = offset % 4;
>>> +    memcpy_fromio(dst, src, tmp);
>>> +    dst += tmp;
>>> +    src += tmp;
>>> +    bytes -= tmp;
>>> +
>>> +    tmp = rounddown(bytes, 4);
>>> +    __ioread32_copy(dst, src, tmp / 4);
>>> +    dst += tmp;
>>> +    src += tmp;
>>> +    bytes -= tmp;
>>> +
>>> +    memcpy_fromio(dst, src, bytes);
>>> +
>>
>>
>> You could just do this!
>>
>>      while (bytes--)
>>          *val++ = readb(priv->base + offset + i++);
> 
> Do you mean that as replacement for "memcpy_fromio" or the whole
> function code?

Yes please!

> The reason for using __ioread32_copy() was to improve reading
> performance (using aligned 32 bit access where possible). I'm not sure
> if that really matters?

Just simple while loop is much readable than the previous code TBH!

> 

> P.S.
> Please don't yell at me in every sentence :( Makes me a bit sad :(
Sorry!! I did not mean anything as such! :-)

--srini

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

On 05.03.2021 11:33, Srinivas Kandagatla wrote:
> On 05/03/2021 10:24, Rafał Miłecki wrote:
>>>>
>>>> +static int iomap_read(void *context, unsigned int offset, void *val,
>>>> +              size_t bytes)
>>>> +{
>>>> +    struct iomap *priv = context;
>>>> +    u8 *src = priv->base + offset;
>>>> +    u8 *dst = val;
>>>> +    size_t tmp;
>>>> +
>>>> +    tmp = offset % 4;
>>>> +    memcpy_fromio(dst, src, tmp);
>>>> +    dst += tmp;
>>>> +    src += tmp;
>>>> +    bytes -= tmp;
>>>> +
>>>> +    tmp = rounddown(bytes, 4);
>>>> +    __ioread32_copy(dst, src, tmp / 4);
>>>> +    dst += tmp;
>>>> +    src += tmp;
>>>> +    bytes -= tmp;
>>>> +
>>>> +    memcpy_fromio(dst, src, bytes);
>>>> +
>>>
>>>
>>> You could just do this!
>>>
>>>      while (bytes--)
>>>          *val++ = readb(priv->base + offset + i++);
>>
>> Do you mean that as replacement for "memcpy_fromio" or the whole
>> function code?
> 
> Yes please!
> 
>> The reason for using __ioread32_copy() was to improve reading
>> performance (using aligned 32 bit access where possible). I'm not sure
>> if that really matters?
> 
> Just simple while loop is much readable than the previous code TBH!
> 
>>
> 
>> P.S.
>> Please don't yell at me in every sentence :( Makes me a bit sad :(
> Sorry!! I did not mean anything as such! :-)

All clear (I hope)! Thanks for your review!

Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible using I/O mapping

[Rafał Miłecki]

On 05.03.2021 11:33, Srinivas Kandagatla wrote:
> On 05/03/2021 10:24, Rafał Miłecki wrote:
>>>>
>>>> +static int iomap_read(void *context, unsigned int offset, void *val,
>>>> +              size_t bytes)
>>>> +{
>>>> +    struct iomap *priv = context;
>>>> +    u8 *src = priv->base + offset;
>>>> +    u8 *dst = val;
>>>> +    size_t tmp;
>>>> +
>>>> +    tmp = offset % 4;
>>>> +    memcpy_fromio(dst, src, tmp);
>>>> +    dst += tmp;
>>>> +    src += tmp;
>>>> +    bytes -= tmp;
>>>> +
>>>> +    tmp = rounddown(bytes, 4);
>>>> +    __ioread32_copy(dst, src, tmp / 4);
>>>> +    dst += tmp;
>>>> +    src += tmp;
>>>> +    bytes -= tmp;
>>>> +
>>>> +    memcpy_fromio(dst, src, bytes);
>>>> +
>>>
>>>
>>> You could just do this!
>>>
>>>      while (bytes--)
>>>          *val++ = readb(priv->base + offset + i++);
>>
>> Do you mean that as replacement for "memcpy_fromio" or the whole
>> function code?
> 
> Yes please!
> 
>> The reason for using __ioread32_copy() was to improve reading
>> performance (using aligned 32 bit access where possible). I'm not sure
>> if that really matters?
> 
> Just simple while loop is much readable than the previous code TBH!
> 
>>
> 
>> P.S.
>> Please don't yell at me in every sentence :( Makes me a bit sad :(
> Sorry!! I did not mean anything as such! :-)

All clear (I hope)! Thanks for your review!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel