[PATCH v2 0/4] Add support for NXP LPC18xx EEPROM using nvmem

[PATCH v2 0/4] Add support for NXP LPC18xx EEPROM using nvmem

[Ariel D'Alessandro]

This patch series adds support for NXP LPC18xx EEPROM memory found in
NXP LPC185x/3x and LPC435x/3x/2x/1x devices.

This patchset is based on tag next-20151019 of the linux-next
repository. It has been successfully tested on a LPC4337 CIAA-NXP
Board.

EEPROM notes:
------------

EEPROM size is 16384 bytes and it can be entirely read and 
written/erased with 1 word (4 bytes) granularity. The last page
(128 bytes) contains the EEPROM initialization data and is not writable.

Erase/program time is less than 3ms. The EEPROM device requires a
~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
the system bus clock by the division factor, contained in the divider
register (minus 1 encoded).

Changeset:
---------
v1 -> v2:
* Moved dt-bindings to be the first patch.
* Changed compatible name from lpc1850 to lpc1857 as the former doesn't have EEPROM.
* Fix hardware description which contained SoCs models without EEPROM.
* Disabled fast_io and changed mdelay for msleep in regmap writes.
* Replaced BUG_ON() in write function for an -EINVAL return.
* Add patches for defconfig and devicetree files.

Thanks,

Ariel D'Alessandro (4):
  DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation
  nvmem: NXP LPC18xx EEPROM memory NVMEM driver
  ARM: dts: lpc18xx: add EEPROM memory node
  ARM: configs: lpc18xx: enable EEPROM NVMEM driver

 .../devicetree/bindings/nvmem/lpc1857-eeprom.txt   |  26 ++
 arch/arm/boot/dts/lpc18xx.dtsi                     |  12 +
 arch/arm/configs/lpc18xx_defconfig                 |   2 +
 drivers/nvmem/Kconfig                              |   9 +
 drivers/nvmem/Makefile                             |   2 +
 drivers/nvmem/lpc18xx_eeprom.c                     | 266 +++++++++++++++++++++
 6 files changed, 317 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
 create mode 100644 drivers/nvmem/lpc18xx_eeprom.c

-- 
2.6.1

[PATCH v2 1/4] DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation

[Ariel D'Alessandro]

Add the devicetree binding document for NXP LPC18xx EEPROM memory.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 .../devicetree/bindings/nvmem/lpc1857-eeprom.txt   | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt

diff --git a/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
new file mode 100644
index 0000000..01cde0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
@@ -0,0 +1,26 @@
+* NXP LPC18xx EEPROM memory NVMEM driver
+
+Required properties:
+  - compatible: Should be "nxp,lpc1857-eeprom"
+  - reg: Must contain an entry with the physical base address and length
+    for each entry in reg-names.
+  - reg-names: Must include the following entries.
+    - reg: EEPROM registers.
+    - mem: EEPROM address space.
+  - clocks: Must contain an entry for each entry in clock-names.
+  - clock-names: Must include the following entries.
+    - eeprom: EEPROM operating clock.
+  - interrupts: Should contain EEPROM interrupt.
+
+Example:
+
+  eeprom: eeprom at 4000e000 {
+    compatible = "nxp,lpc1857-eeprom";
+    reg = <0x4000e000 0x1000>,
+          <0x20040000 0x4000>;
+    reg-names = "reg", "mem";
+    clocks = <&ccu1 CLK_CPU_EEPROM>;
+    clock-names = "eeprom";
+    resets = <&rgu 27>;
+    interrupts = <4>;
+  };
-- 
2.6.1

[PATCH v2 1/4] DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation

[Joachim Eastwood]

Hi Ariel,

On 19 October 2015 at 19:32, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> Add the devicetree binding document for NXP LPC18xx EEPROM memory.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
>  .../devicetree/bindings/nvmem/lpc1857-eeprom.txt   | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
> new file mode 100644
> index 0000000..01cde0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
> @@ -0,0 +1,26 @@
> +* NXP LPC18xx EEPROM memory NVMEM driver
> +
> +Required properties:
> +  - compatible: Should be "nxp,lpc1857-eeprom"
> +  - reg: Must contain an entry with the physical base address and length
> +    for each entry in reg-names.
> +  - reg-names: Must include the following entries.
> +    - reg: EEPROM registers.
> +    - mem: EEPROM address space.
> +  - clocks: Must contain an entry for each entry in clock-names.
> +  - clock-names: Must include the following entries.
> +    - eeprom: EEPROM operating clock.
> +  - interrupts: Should contain EEPROM interrupt.

You should document the 'reset' property as well.

> +Example:
> +
> +  eeprom: eeprom at 4000e000 {
> +    compatible = "nxp,lpc1857-eeprom";
> +    reg = <0x4000e000 0x1000>,
> +          <0x20040000 0x4000>;
> +    reg-names = "reg", "mem";
> +    clocks = <&ccu1 CLK_CPU_EEPROM>;
> +    clock-names = "eeprom";
> +    resets = <&rgu 27>;
> +    interrupts = <4>;
> +  };

Other than that this looks good to me.

Acked-by: Joachim Eastwood <manabian@gmail.com>


regards,
Joachim Eastwood

[PATCH v2 1/4] DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation

[Ariel D'Alessandro]

Joachim,

El 24/10/15 a las 18:44, Joachim Eastwood escribi?:
> Hi Ariel,
> 
> On 19 October 2015 at 19:32, Ariel D'Alessandro
> <ariel@vanguardiasur.com.ar> wrote:
>> Add the devicetree binding document for NXP LPC18xx EEPROM memory.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>  .../devicetree/bindings/nvmem/lpc1857-eeprom.txt   | 26 ++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
>> new file mode 100644
>> index 0000000..01cde0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
>> @@ -0,0 +1,26 @@
>> +* NXP LPC18xx EEPROM memory NVMEM driver
>> +
>> +Required properties:
>> +  - compatible: Should be "nxp,lpc1857-eeprom"
>> +  - reg: Must contain an entry with the physical base address and length
>> +    for each entry in reg-names.
>> +  - reg-names: Must include the following entries.
>> +    - reg: EEPROM registers.
>> +    - mem: EEPROM address space.
>> +  - clocks: Must contain an entry for each entry in clock-names.
>> +  - clock-names: Must include the following entries.
>> +    - eeprom: EEPROM operating clock.
>> +  - interrupts: Should contain EEPROM interrupt.
> 
> You should document the 'reset' property as well.

Ok. Will do.

> 
>> +Example:
>> +
>> +  eeprom: eeprom at 4000e000 {
>> +    compatible = "nxp,lpc1857-eeprom";
>> +    reg = <0x4000e000 0x1000>,
>> +          <0x20040000 0x4000>;
>> +    reg-names = "reg", "mem";
>> +    clocks = <&ccu1 CLK_CPU_EEPROM>;
>> +    clock-names = "eeprom";
>> +    resets = <&rgu 27>;
>> +    interrupts = <4>;
>> +  };
> 
> Other than that this looks good to me.
> 
> Acked-by: Joachim Eastwood <manabian@gmail.com>
> 

Great, thanks.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 1/4] DT: nvmem: Add NXP LPC18xx EEPROM memory binding documentation

[Rob Herring]

On Mon, Oct 19, 2015 at 12:32 PM, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> Add the devicetree binding document for NXP LPC18xx EEPROM memory.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  .../devicetree/bindings/nvmem/lpc1857-eeprom.txt   | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
> new file mode 100644
> index 0000000..01cde0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/lpc1857-eeprom.txt
> @@ -0,0 +1,26 @@
> +* NXP LPC18xx EEPROM memory NVMEM driver
> +
> +Required properties:
> +  - compatible: Should be "nxp,lpc1857-eeprom"
> +  - reg: Must contain an entry with the physical base address and length
> +    for each entry in reg-names.
> +  - reg-names: Must include the following entries.
> +    - reg: EEPROM registers.
> +    - mem: EEPROM address space.
> +  - clocks: Must contain an entry for each entry in clock-names.
> +  - clock-names: Must include the following entries.
> +    - eeprom: EEPROM operating clock.
> +  - interrupts: Should contain EEPROM interrupt.
> +
> +Example:
> +
> +  eeprom: eeprom at 4000e000 {
> +    compatible = "nxp,lpc1857-eeprom";
> +    reg = <0x4000e000 0x1000>,
> +          <0x20040000 0x4000>;
> +    reg-names = "reg", "mem";
> +    clocks = <&ccu1 CLK_CPU_EEPROM>;
> +    clock-names = "eeprom";
> +    resets = <&rgu 27>;
> +    interrupts = <4>;
> +  };
> --
> 2.6.1
>

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

This commit adds support for NXP LPC18xx EEPROM memory found in NXP
LPC185x/3x and LPC435x/3x/2x/1x devices.

EEPROM size is 16384 bytes and it can be entirely read and
written/erased with 1 word (4 bytes) granularity. The last page
(128 bytes) contains the EEPROM initialization data and is not writable.

Erase/program time is less than 3ms. The EEPROM device requires a
~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
the system bus clock by the division factor, contained in the divider
register (minus 1 encoded).

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 drivers/nvmem/Kconfig          |   9 ++
 drivers/nvmem/Makefile         |   2 +
 drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+)
 create mode 100644 drivers/nvmem/lpc18xx_eeprom.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index bc4ea58..6ff1b50 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -25,6 +25,15 @@ config NVMEM_IMX_OCOTP
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem-imx-ocotp.
 
+config NVMEM_LPC18XX_EEPROM
+	tristate "NXP LPC18XX EEPROM Memory Support"
+	depends on ARCH_LPC18XX || COMPILE_TEST
+	help
+	  Say Y here to include support for NXP LPC18xx EEPROM memory found in
+	  NXP LPC185x/3x and LPC435x/3x/2x/1x devices.
+	  To compile this driver as a module, choose M here: the module
+	  will be called nvmem_lpc18xx_eeprom.
+
 config NVMEM_MXS_OCOTP
 	tristate "Freescale MXS On-Chip OTP Memory Support"
 	depends on ARCH_MXS || COMPILE_TEST
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index 95dde3f..c14a556 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -8,6 +8,8 @@ nvmem_core-y			:= core.o
 # Devices
 obj-$(CONFIG_NVMEM_IMX_OCOTP)	+= nvmem-imx-ocotp.o
 nvmem-imx-ocotp-y		:= imx-ocotp.o
+obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
+nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
 obj-$(CONFIG_NVMEM_MXS_OCOTP)	+= nvmem-mxs-ocotp.o
 nvmem-mxs-ocotp-y		:= mxs-ocotp.o
 obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
diff --git a/drivers/nvmem/lpc18xx_eeprom.c b/drivers/nvmem/lpc18xx_eeprom.c
new file mode 100644
index 0000000..ccdda66
--- /dev/null
+++ b/drivers/nvmem/lpc18xx_eeprom.c
@@ -0,0 +1,266 @@
+/*
+ * NXP LPC18xx/LPC43xx EEPROM memory NVMEM driver
+ *
+ * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+/* Registers */
+#define LPC18XX_EEPROM_AUTOPROG		0x00c
+#define LPC18XX_EEPROM_AUTOPROG_WORD	0x1
+
+#define LPC18XX_EEPROM_CLKDIV		0x014
+
+#define LPC18XX_EEPROM_PWRDWN		0x018
+#define LPC18XX_EEPROM_PWRDWN_NO	0x0
+#define LPC18XX_EEPROM_PWRDWN_YES	0x1
+
+/* Fixed page size (bytes) */
+#define LPC18XX_EEPROM_PAGE_SIZE	0x80
+
+/* EEPROM device requires a ~1500 kHz clock (min 800 kHz, max 1600 kHz) */
+#define LPC18XX_EEPROM_CLOCK_HZ		1500000
+
+struct lpc18xx_eeprom_dev {
+	struct clk *clk;
+	void __iomem *reg_base;
+	void __iomem *mem_base;
+	struct nvmem_device *nvmem;
+	unsigned reg_bytes;
+	unsigned val_bytes;
+};
+
+static inline void lpc18xx_eeprom_writel(struct lpc18xx_eeprom_dev *eeprom,
+					 u32 reg, u32 val)
+{
+	writel(val, eeprom->reg_base + reg);
+}
+
+static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
+				       size_t reg_size, const void *val,
+				       size_t val_size)
+{
+	struct lpc18xx_eeprom_dev *eeprom = context;
+	unsigned int offset = *(u32 *)reg;
+
+	/* 3 ms of erase/program time between each writing */
+	while (val_size) {
+		writel(*(u32 *)val, eeprom->mem_base + offset);
+		usleep_range(3000, 4000);
+		val_size -= eeprom->val_bytes;
+		val += eeprom->val_bytes;
+		offset += eeprom->val_bytes;
+	}
+
+	return 0;
+}
+
+static int lpc18xx_eeprom_write(void *context, const void *data, size_t count)
+{
+	struct lpc18xx_eeprom_dev *eeprom = context;
+	unsigned int offset = eeprom->reg_bytes;
+
+	if (count <= offset)
+		return -EINVAL;
+
+
+	return lpc18xx_eeprom_gather_write(context, data, eeprom->reg_bytes,
+					   data + offset, count - offset);
+}
+
+static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size,
+			       void *val, size_t val_size)
+{
+	struct lpc18xx_eeprom_dev *eeprom = context;
+	unsigned int offset = *(u32 *)reg;
+
+	while (val_size) {
+		*(u32 *)val = readl(eeprom->mem_base + offset);
+		val_size -= eeprom->val_bytes;
+		val += eeprom->val_bytes;
+		offset += eeprom->val_bytes;
+	}
+
+	return 0;
+}
+
+static struct regmap_bus lpc18xx_eeprom_bus = {
+	.write = lpc18xx_eeprom_write,
+	.gather_write = lpc18xx_eeprom_gather_write,
+	.read = lpc18xx_eeprom_read,
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static struct regmap_config lpc18xx_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static bool lpc18xx_eeprom_writeable_reg(struct device *dev, unsigned int reg)
+{
+	/*
+	 * The last page contains the EEPROM initialization data and is not
+	 * writable.
+	 */
+	return reg <= lpc18xx_regmap_config.max_register -
+						LPC18XX_EEPROM_PAGE_SIZE;
+}
+
+static bool lpc18xx_eeprom_readable_reg(struct device *dev, unsigned int reg)
+{
+	return reg <= lpc18xx_regmap_config.max_register;
+}
+
+static struct nvmem_config lpc18xx_nvmem_config = {
+	.name = "lpc18xx-eeprom",
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id lpc18xx_eeprom_of_match[] = {
+	{ .compatible = "nxp,lpc1857-eeprom" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);
+
+static int lpc18xx_eeprom_probe(struct platform_device *pdev)
+{
+	struct lpc18xx_eeprom_dev *eeprom;
+	struct device *dev = &pdev->dev;
+	struct reset_control *rst;
+	unsigned long clk_rate;
+	struct regmap *regmap;
+	struct resource *res;
+	int ret;
+
+	eeprom = devm_kzalloc(dev, sizeof(*eeprom), GFP_KERNEL);
+	if (!eeprom)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
+	eeprom->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(eeprom->reg_base))
+		return PTR_ERR(eeprom->reg_base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem");
+	eeprom->mem_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(eeprom->mem_base))
+		return PTR_ERR(eeprom->mem_base);
+
+	eeprom->clk = devm_clk_get(&pdev->dev, "eeprom");
+	if (IS_ERR(eeprom->clk)) {
+		dev_err(&pdev->dev, "failed to get eeprom clock\n");
+		return PTR_ERR(eeprom->clk);
+	}
+
+	ret = clk_prepare_enable(eeprom->clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to prepare/enable eeprom clk: %d\n", ret);
+		return ret;
+	}
+
+	rst = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(rst)) {
+		dev_err(dev, "failed to get reset: %ld\n", PTR_ERR(rst));
+		ret = PTR_ERR(rst);
+		goto err_clk;
+	}
+
+	ret = reset_control_deassert(rst);
+	if (ret < 0) {
+		dev_err(dev, "failed to deassert reset: %d\n", ret);
+		goto err_clk;
+	}
+
+	eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
+	eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;
+
+	/*
+	 * Clock rate is generated by dividing the system bus clock by the
+	 * division factor, contained in the divider register (minus 1 encoded).
+	 */
+	clk_rate = clk_get_rate(eeprom->clk);
+	clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
+	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
+
+	/*
+	 * Writing a single word to the page will start the erase/program cycle
+	 * automatically
+	 */
+	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
+			      LPC18XX_EEPROM_AUTOPROG_WORD);
+
+	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
+			      LPC18XX_EEPROM_PWRDWN_NO);
+
+	lpc18xx_regmap_config.max_register = resource_size(res) - 1;
+	lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
+	lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
+
+	regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
+				  &lpc18xx_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
+		ret = PTR_ERR(regmap);
+		goto err_clk;
+	}
+
+	lpc18xx_nvmem_config.dev = dev;
+
+	eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
+	if (IS_ERR(eeprom->nvmem)) {
+		ret = PTR_ERR(eeprom->nvmem);
+		goto err_clk;
+	}
+
+	platform_set_drvdata(pdev, eeprom);
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(eeprom->clk);
+
+	return ret;
+}
+
+static int lpc18xx_eeprom_remove(struct platform_device *pdev)
+{
+	struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
+
+	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
+			      LPC18XX_EEPROM_PWRDWN_YES);
+
+	clk_disable_unprepare(eeprom->clk);
+
+	return nvmem_unregister(eeprom->nvmem);
+}
+
+static struct platform_driver lpc18xx_eeprom_driver = {
+	.probe = lpc18xx_eeprom_probe,
+	.remove = lpc18xx_eeprom_remove,
+	.driver = {
+		.name = "lpc18xx-eeprom",
+		.of_match_table = lpc18xx_eeprom_of_match,
+	},
+};
+
+module_platform_driver(lpc18xx_eeprom_driver);
+
+MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
+MODULE_DESCRIPTION("NXP LPC18xx EEPROM memory Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.6.1

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Joachim Eastwood]

Hi Ariel,

On 19 October 2015 at 19:32, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
> LPC185x/3x and LPC435x/3x/2x/1x devices.
>
> EEPROM size is 16384 bytes and it can be entirely read and
> written/erased with 1 word (4 bytes) granularity. The last page
> (128 bytes) contains the EEPROM initialization data and is not writable.
>
> Erase/program time is less than 3ms. The EEPROM device requires a
> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
> the system bus clock by the division factor, contained in the divider
> register (minus 1 encoded).
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
>  drivers/nvmem/Kconfig          |   9 ++
>  drivers/nvmem/Makefile         |   2 +
>  drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 277 insertions(+)
>  create mode 100644 drivers/nvmem/lpc18xx_eeprom.c

> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
> +                                      size_t reg_size, const void *val,
> +                                      size_t val_size)
> +{
> +       struct lpc18xx_eeprom_dev *eeprom = context;
> +       unsigned int offset = *(u32 *)reg;
> +
> +       /* 3 ms of erase/program time between each writing */
> +       while (val_size) {
> +               writel(*(u32 *)val, eeprom->mem_base + offset);
> +               usleep_range(3000, 4000);
> +               val_size -= eeprom->val_bytes;
> +               val += eeprom->val_bytes;
> +               offset += eeprom->val_bytes;
> +       }

What happens here if 'val_size' is less than 4 or not dividable by 4?
Same thing for 'offset'.

I tested the driver from sysfs by writing strings into the nvmem-file
with echo. Writing a string not dividable by 4 seems to hang the
system.


> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size,
> +                              void *val, size_t val_size)
> +{
> +       struct lpc18xx_eeprom_dev *eeprom = context;
> +       unsigned int offset = *(u32 *)reg;
> +
> +       while (val_size) {
> +               *(u32 *)val = readl(eeprom->mem_base + offset);
> +               val_size -= eeprom->val_bytes;
> +               val += eeprom->val_bytes;
> +               offset += eeprom->val_bytes;
> +       }
> +
> +       return 0;
> +}

Same comments as for lpc18xx_eeprom_gather_write().


> +static const struct of_device_id lpc18xx_eeprom_of_match[] = {
> +       { .compatible = "nxp,lpc1857-eeprom" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);

nit: It's usual to place of_device_id struct just above the
platform_driver struct.


> +       eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
> +       eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;

There is a BITS_PER_BYTE define in bitops.h that you might want to use here.


> +       /*
> +        * Clock rate is generated by dividing the system bus clock by the
> +        * division factor, contained in the divider register (minus 1 encoded).
> +        */
> +       clk_rate = clk_get_rate(eeprom->clk);
> +       clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
> +
> +       /*
> +        * Writing a single word to the page will start the erase/program cycle
> +        * automatically
> +        */
> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
> +                             LPC18XX_EEPROM_AUTOPROG_WORD);
> +
> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
> +                             LPC18XX_EEPROM_PWRDWN_NO);
> +
> +       lpc18xx_regmap_config.max_register = resource_size(res) - 1;
> +       lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
> +       lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
> +
> +       regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
> +                                 &lpc18xx_regmap_config);
> +       if (IS_ERR(regmap)) {
> +               dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
> +               ret = PTR_ERR(regmap);
> +               goto err_clk;
> +       }
> +
> +       lpc18xx_nvmem_config.dev = dev;
> +
> +       eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
> +       if (IS_ERR(eeprom->nvmem)) {
> +               ret = PTR_ERR(eeprom->nvmem);
> +               goto err_clk;
> +       }
> +
> +       platform_set_drvdata(pdev, eeprom);
> +
> +       return 0;
> +
> +err_clk:
> +       clk_disable_unprepare(eeprom->clk);
> +
> +       return ret;
> +}
> +
> +static int lpc18xx_eeprom_remove(struct platform_device *pdev)
> +{
> +       struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
> +
> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
> +                             LPC18XX_EEPROM_PWRDWN_YES);
> +
> +       clk_disable_unprepare(eeprom->clk);
> +
> +       return nvmem_unregister(eeprom->nvmem);

Normally you do tear down in the reverse order of initialization.

Consider what happens here when you power down and disable the clock
while there still are nvmem users of the eeprom.


regards,
Joachim Eastwood

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Srinivas Kandagatla]

On 24/10/15 23:04, Joachim Eastwood wrote:
> Hi Ariel,
>
> On 19 October 2015 at 19:32, Ariel D'Alessandro
> <ariel@vanguardiasur.com.ar> wrote:
>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>
>> EEPROM size is 16384 bytes and it can be entirely read and
>> written/erased with 1 word (4 bytes) granularity. The last page
>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>
>> Erase/program time is less than 3ms. The EEPROM device requires a
>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>> the system bus clock by the division factor, contained in the divider
>> register (minus 1 encoded).
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>   drivers/nvmem/Kconfig          |   9 ++
>>   drivers/nvmem/Makefile         |   2 +
>>   drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 277 insertions(+)
>>   create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>
>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>> +                                      size_t reg_size, const void *val,
>> +                                      size_t val_size)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>> +       unsigned int offset = *(u32 *)reg;
>> +
>> +       /* 3 ms of erase/program time between each writing */
>> +       while (val_size) {
>> +               writel(*(u32 *)val, eeprom->mem_base + offset);
>> +               usleep_range(3000, 4000);
>> +               val_size -= eeprom->val_bytes;
>> +               val += eeprom->val_bytes;
>> +               offset += eeprom->val_bytes;
>> +       }
>
> What happens here if 'val_size' is less than 4 or not dividable by 4?
> Same thing for 'offset'.
>
> I tested the driver from sysfs by writing strings into the nvmem-file
> with echo. Writing a string not dividable by 4 seems to hang the
> system.
>

I think I know the issue here:
Could you try this patch:

-------------------------------->cut<----------------------------------
 From 8cae10eff8ea8da9c5a8058ff75abeeddd8a8224 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Mon, 26 Oct 2015 13:30:24 +0000
Subject: [PATCH] nvmem: core: return error for non word aligned bytes

nvmem providers have restrictions on register strides, so return error
code when users attempt to read/write buffers with sizes which are not
aligned to the word boundary.

Without this patch the userspace would continue to try as it does not
get any error from the nvmem core, resulting in a hang.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
  drivers/nvmem/core.c | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6fd4e5a..9d11d98 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -70,6 +70,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, 
struct kobject *kobj,
  	if (pos >= nvmem->size)
  		return 0;

+	if (count < nvmem->word_size)
+		return -EINVAL;
+
  	if (pos + count > nvmem->size)
  		count = nvmem->size - pos;

@@ -95,6 +98,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, 
struct kobject *kobj,
  	if (pos >= nvmem->size)
  		return 0;

+	if (count < nvmem->word_size)
+		return -EINVAL;
+
  	if (pos + count > nvmem->size)
  		count = nvmem->size - pos;

-- 
1.9.1

-------------------------------->cut<----------------------------------


--srini


>
>> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size,
>> +                              void *val, size_t val_size)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>> +       unsigned int offset = *(u32 *)reg;
>> +
>> +       while (val_size) {
>> +               *(u32 *)val = readl(eeprom->mem_base + offset);
>> +               val_size -= eeprom->val_bytes;
>> +               val += eeprom->val_bytes;
>> +               offset += eeprom->val_bytes;
>> +       }
>> +
>> +       return 0;
>> +}
>
> Same comments as for lpc18xx_eeprom_gather_write().
>
>
>> +static const struct of_device_id lpc18xx_eeprom_of_match[] = {
>> +       { .compatible = "nxp,lpc1857-eeprom" },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);
>
> nit: It's usual to place of_device_id struct just above the
> platform_driver struct.
>
>
>> +       eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
>> +       eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;
>
> There is a BITS_PER_BYTE define in bitops.h that you might want to use here.
>
>
>> +       /*
>> +        * Clock rate is generated by dividing the system bus clock by the
>> +        * division factor, contained in the divider register (minus 1 encoded).
>> +        */
>> +       clk_rate = clk_get_rate(eeprom->clk);
>> +       clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
>> +
>> +       /*
>> +        * Writing a single word to the page will start the erase/program cycle
>> +        * automatically
>> +        */
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
>> +                             LPC18XX_EEPROM_AUTOPROG_WORD);
>> +
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                             LPC18XX_EEPROM_PWRDWN_NO);
>> +
>> +       lpc18xx_regmap_config.max_register = resource_size(res) - 1;
>> +       lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
>> +       lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
>> +
>> +       regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
>> +                                 &lpc18xx_regmap_config);
>> +       if (IS_ERR(regmap)) {
>> +               dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
>> +               ret = PTR_ERR(regmap);
>> +               goto err_clk;
>> +       }
>> +
>> +       lpc18xx_nvmem_config.dev = dev;
>> +
>> +       eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
>> +       if (IS_ERR(eeprom->nvmem)) {
>> +               ret = PTR_ERR(eeprom->nvmem);
>> +               goto err_clk;
>> +       }
>> +
>> +       platform_set_drvdata(pdev, eeprom);
>> +
>> +       return 0;
>> +
>> +err_clk:
>> +       clk_disable_unprepare(eeprom->clk);
>> +
>> +       return ret;
>> +}
>> +
>> +static int lpc18xx_eeprom_remove(struct platform_device *pdev)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
>> +
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                             LPC18XX_EEPROM_PWRDWN_YES);
>> +
>> +       clk_disable_unprepare(eeprom->clk);
>> +
>> +       return nvmem_unregister(eeprom->nvmem);
>
> Normally you do tear down in the reverse order of initialization.
>
> Consider what happens here when you power down and disable the clock
> while there still are nvmem users of the eeprom.
>
>
> regards,
> Joachim Eastwood
>

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Srinivas,

El 26/10/15 a las 10:37, Srinivas Kandagatla escribi?:
> 
> 
> On 24/10/15 23:04, Joachim Eastwood wrote:
>> Hi Ariel,
>>
>> On 19 October 2015 at 19:32, Ariel D'Alessandro
>> <ariel@vanguardiasur.com.ar> wrote:
>>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>>
>>> EEPROM size is 16384 bytes and it can be entirely read and
>>> written/erased with 1 word (4 bytes) granularity. The last page
>>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>>
>>> Erase/program time is less than 3ms. The EEPROM device requires a
>>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>>> the system bus clock by the division factor, contained in the divider
>>> register (minus 1 encoded).
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>> ---
>>>   drivers/nvmem/Kconfig          |   9 ++
>>>   drivers/nvmem/Makefile         |   2 +
>>>   drivers/nvmem/lpc18xx_eeprom.c | 266
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 277 insertions(+)
>>>   create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>>
>>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>>> +                                      size_t reg_size, const void *val,
>>> +                                      size_t val_size)
>>> +{
>>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>>> +       unsigned int offset = *(u32 *)reg;
>>> +
>>> +       /* 3 ms of erase/program time between each writing */
>>> +       while (val_size) {
>>> +               writel(*(u32 *)val, eeprom->mem_base + offset);
>>> +               usleep_range(3000, 4000);
>>> +               val_size -= eeprom->val_bytes;
>>> +               val += eeprom->val_bytes;
>>> +               offset += eeprom->val_bytes;
>>> +       }
>>
>> What happens here if 'val_size' is less than 4 or not dividable by 4?
>> Same thing for 'offset'.
>>
>> I tested the driver from sysfs by writing strings into the nvmem-file
>> with echo. Writing a string not dividable by 4 seems to hang the
>> system.
>>
> 
> I think I know the issue here:
> Could you try this patch:
> 
> -------------------------------->cut<----------------------------------
> From 8cae10eff8ea8da9c5a8058ff75abeeddd8a8224 Mon Sep 17 00:00:00 2001
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Mon, 26 Oct 2015 13:30:24 +0000
> Subject: [PATCH] nvmem: core: return error for non word aligned bytes
> 
> nvmem providers have restrictions on register strides, so return error
> code when users attempt to read/write buffers with sizes which are not
> aligned to the word boundary.
> 
> Without this patch the userspace would continue to try as it does not
> get any error from the nvmem core, resulting in a hang.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/nvmem/core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6fd4e5a..9d11d98 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -70,6 +70,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp,
> struct kobject *kobj,
>      if (pos >= nvmem->size)
>          return 0;
> 
> +    if (count < nvmem->word_size)
> +        return -EINVAL;
> +
>      if (pos + count > nvmem->size)
>          count = nvmem->size - pos;
> 
> @@ -95,6 +98,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp,
> struct kobject *kobj,
>      if (pos >= nvmem->size)
>          return 0;
> 
> +    if (count < nvmem->word_size)
> +        return -EINVAL;
> +
>      if (pos + count > nvmem->size)
>          count = nvmem->size - pos;
> 

Patch looks good to me. I think that it solves the issue.
I don't have the board here right now, so I'll check it ASAP and give
some feedback.

Thanks!

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Srinivas,

Sorry for the delay, I finally got the hardware again.

El 30/10/15 a las 11:58, Ariel D'Alessandro escribi?:
> Srinivas,
> 
> El 26/10/15 a las 10:37, Srinivas Kandagatla escribi?:
>>
>>
>> On 24/10/15 23:04, Joachim Eastwood wrote:
>>> Hi Ariel,
>>>
>>> On 19 October 2015 at 19:32, Ariel D'Alessandro
>>> <ariel@vanguardiasur.com.ar> wrote:
>>>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>>>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>>>
>>>> EEPROM size is 16384 bytes and it can be entirely read and
>>>> written/erased with 1 word (4 bytes) granularity. The last page
>>>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>>>
>>>> Erase/program time is less than 3ms. The EEPROM device requires a
>>>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>>>> the system bus clock by the division factor, contained in the divider
>>>> register (minus 1 encoded).
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>> ---
>>>>   drivers/nvmem/Kconfig          |   9 ++
>>>>   drivers/nvmem/Makefile         |   2 +
>>>>   drivers/nvmem/lpc18xx_eeprom.c | 266
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 277 insertions(+)
>>>>   create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>>>
>>>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>>>> +                                      size_t reg_size, const void *val,
>>>> +                                      size_t val_size)
>>>> +{
>>>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>>>> +       unsigned int offset = *(u32 *)reg;
>>>> +
>>>> +       /* 3 ms of erase/program time between each writing */
>>>> +       while (val_size) {
>>>> +               writel(*(u32 *)val, eeprom->mem_base + offset);
>>>> +               usleep_range(3000, 4000);
>>>> +               val_size -= eeprom->val_bytes;
>>>> +               val += eeprom->val_bytes;
>>>> +               offset += eeprom->val_bytes;
>>>> +       }
>>>
>>> What happens here if 'val_size' is less than 4 or not dividable by 4?
>>> Same thing for 'offset'.
>>>
>>> I tested the driver from sysfs by writing strings into the nvmem-file
>>> with echo. Writing a string not dividable by 4 seems to hang the
>>> system.
>>>
>>
>> I think I know the issue here:
>> Could you try this patch:
>>
>> -------------------------------->cut<----------------------------------
>> From 8cae10eff8ea8da9c5a8058ff75abeeddd8a8224 Mon Sep 17 00:00:00 2001
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Date: Mon, 26 Oct 2015 13:30:24 +0000
>> Subject: [PATCH] nvmem: core: return error for non word aligned bytes
>>
>> nvmem providers have restrictions on register strides, so return error
>> code when users attempt to read/write buffers with sizes which are not
>> aligned to the word boundary.
>>
>> Without this patch the userspace would continue to try as it does not
>> get any error from the nvmem core, resulting in a hang.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>  drivers/nvmem/core.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 6fd4e5a..9d11d98 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -70,6 +70,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp,
>> struct kobject *kobj,
>>      if (pos >= nvmem->size)
>>          return 0;
>>
>> +    if (count < nvmem->word_size)
>> +        return -EINVAL;
>> +
>>      if (pos + count > nvmem->size)
>>          count = nvmem->size - pos;
>>
>> @@ -95,6 +98,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp,
>> struct kobject *kobj,
>>      if (pos >= nvmem->size)
>>          return 0;
>>
>> +    if (count < nvmem->word_size)
>> +        return -EINVAL;
>> +
>>      if (pos + count > nvmem->size)
>>          count = nvmem->size - pos;
>>
> 
> Patch looks good to me. I think that it solves the issue.
> I don't have the board here right now, so I'll check it ASAP and give
> some feedback.

Finally tested this. As it seemed, it solved the issue.
Are you submitting a patch for this?

Thanks again.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Srinivas Kandagatla]

On 16/11/15 15:33, Ariel D'Alessandro wrote:
>>> >>-------------------------------->cut<----------------------------------
>>> >> From 8cae10eff8ea8da9c5a8058ff75abeeddd8a8224 Mon Sep 17 00:00:00 2001
>>> >>From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>> >>Date: Mon, 26 Oct 2015 13:30:24 +0000
>>> >>Subject: [PATCH] nvmem: core: return error for non word aligned bytes
>>> >>
>>> >>nvmem providers have restrictions on register strides, so return error
>>> >>code when users attempt to read/write buffers with sizes which are not
>>> >>aligned to the word boundary.
>>> >>
>>> >>Without this patch the userspace would continue to try as it does not
>>> >>get any error from the nvmem core, resulting in a hang.
>>> >>
>>> >>Signed-off-by: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>> >>---
>>> >>  drivers/nvmem/core.c | 6 ++++++
>>> >>  1 file changed, 6 insertions(+)
>>> >>
>>> >>diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> >>index 6fd4e5a..9d11d98 100644
>>> >>--- a/drivers/nvmem/core.c
>>> >>+++ b/drivers/nvmem/core.c
>>> >>@@ -70,6 +70,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp,
>>> >>struct kobject *kobj,
>>> >>      if (pos >= nvmem->size)
>>> >>          return 0;
>>> >>
>>> >>+    if (count < nvmem->word_size)
>>> >>+        return -EINVAL;
>>> >>+
>>> >>      if (pos + count > nvmem->size)
>>> >>          count = nvmem->size - pos;
>>> >>
>>> >>@@ -95,6 +98,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp,
>>> >>struct kobject *kobj,
>>> >>      if (pos >= nvmem->size)
>>> >>          return 0;
>>> >>
>>> >>+    if (count < nvmem->word_size)
>>> >>+        return -EINVAL;
>>> >>+
>>> >>      if (pos + count > nvmem->size)
>>> >>          count = nvmem->size - pos;
>>> >>
>> >
>> >Patch looks good to me. I think that it solves the issue.
>> >I don't have the board here right now, so I'll check it ASAP and give
>> >some feedback.
> Finally tested this. As it seemed, it solved the issue.
> Are you submitting a patch for this?
Yes, I will send a patch to the mailing list.

--srini
>
> Thanks again.

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ezequiel Garcia]

Hi Srinivas,

On 16 November 2015 at 12:37, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> On 16/11/15 15:33, Ariel D'Alessandro wrote:
>>>>
>>>>
>>>> >> >>-------------------------------->cut<----------------------------------
>>>> >> From 8cae10eff8ea8da9c5a8058ff75abeeddd8a8224 Mon Sep 17 00:00:00
>>>> >> 2001
>>>> >>From: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>> >>Date: Mon, 26 Oct 2015 13:30:24 +0000
>>>> >>Subject: [PATCH] nvmem: core: return error for non word aligned bytes
>>>> >>
>>>> >>nvmem providers have restrictions on register strides, so return error
>>>> >>code when users attempt to read/write buffers with sizes which are not
>>>> >>aligned to the word boundary.
>>>> >>
>>>> >>Without this patch the userspace would continue to try as it does not
>>>> >>get any error from the nvmem core, resulting in a hang.
>>>> >>
>>>> >>Signed-off-by: Srinivas Kandagatla<srinivas.kandagatla@linaro.org>
>>>> >>---
>>>> >>  drivers/nvmem/core.c | 6 ++++++
>>>> >>  1 file changed, 6 insertions(+)
>>>> >>
>>>> >>diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>> >>index 6fd4e5a..9d11d98 100644
>>>> >>--- a/drivers/nvmem/core.c
>>>> >>+++ b/drivers/nvmem/core.c
>>>> >>@@ -70,6 +70,9 @@ static ssize_t bin_attr_nvmem_read(struct file
>>>> >> *filp,
>>>> >>struct kobject *kobj,
>>>> >>      if (pos >= nvmem->size)
>>>> >>          return 0;
>>>> >>
>>>> >>+    if (count < nvmem->word_size)
>>>> >>+        return -EINVAL;
>>>> >>+
>>>> >>      if (pos + count > nvmem->size)
>>>> >>          count = nvmem->size - pos;
>>>> >>
>>>> >>@@ -95,6 +98,9 @@ static ssize_t bin_attr_nvmem_write(struct file
>>>> >> *filp,
>>>> >>struct kobject *kobj,
>>>> >>      if (pos >= nvmem->size)
>>>> >>          return 0;
>>>> >>
>>>> >>+    if (count < nvmem->word_size)
>>>> >>+        return -EINVAL;
>>>> >>+
>>>> >>      if (pos + count > nvmem->size)
>>>> >>          count = nvmem->size - pos;
>>>> >>
>>>
>>> >
>>> >Patch looks good to me. I think that it solves the issue.
>>> >I don't have the board here right now, so I'll check it ASAP and give
>>> >some feedback.
>>
>> Finally tested this. As it seemed, it solved the issue.
>> Are you submitting a patch for this?
>
> Yes, I will send a patch to the mailing list.
>

Ping? It would be good to have that patch and be able to merge this
lpc18xx-nvmem driver in time for the next merge window.

Thanks!
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Joachim,

El 24/10/15 a las 19:04, Joachim Eastwood escribi?:
> Hi Ariel,
> 
> On 19 October 2015 at 19:32, Ariel D'Alessandro
> <ariel@vanguardiasur.com.ar> wrote:
>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>
>> EEPROM size is 16384 bytes and it can be entirely read and
>> written/erased with 1 word (4 bytes) granularity. The last page
>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>
>> Erase/program time is less than 3ms. The EEPROM device requires a
>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>> the system bus clock by the division factor, contained in the divider
>> register (minus 1 encoded).
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>  drivers/nvmem/Kconfig          |   9 ++
>>  drivers/nvmem/Makefile         |   2 +
>>  drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 277 insertions(+)
>>  create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
> 
>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>> +                                      size_t reg_size, const void *val,
>> +                                      size_t val_size)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>> +       unsigned int offset = *(u32 *)reg;
>> +
>> +       /* 3 ms of erase/program time between each writing */
>> +       while (val_size) {
>> +               writel(*(u32 *)val, eeprom->mem_base + offset);
>> +               usleep_range(3000, 4000);
>> +               val_size -= eeprom->val_bytes;
>> +               val += eeprom->val_bytes;
>> +               offset += eeprom->val_bytes;
>> +       }
> 
> What happens here if 'val_size' is less than 4 or not dividable by 4?

AFAIK, before calling to gather_write, the caller ensures that
'val_size' % map->format.val_bytes == 0

Like nvmem_device_write() do this in
drivers/base/regmap/regmap.c:regmap_raw_write()

Then, if val_size == 0, nothing will happen.

> Same thing for 'offset'.

Let me check this one.

> 
> I tested the driver from sysfs by writing strings into the nvmem-file
> with echo. Writing a string not dividable by 4 seems to hang the
> system.

Yeah, I saw that too. I think that's a nvmem core issue and Srinivas's
patch solves this. Will confirm that as soon as I have the board back.

> 
> 
>> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size,
>> +                              void *val, size_t val_size)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>> +       unsigned int offset = *(u32 *)reg;
>> +
>> +       while (val_size) {
>> +               *(u32 *)val = readl(eeprom->mem_base + offset);
>> +               val_size -= eeprom->val_bytes;
>> +               val += eeprom->val_bytes;
>> +               offset += eeprom->val_bytes;
>> +       }
>> +
>> +       return 0;
>> +}
> 
> Same comments as for lpc18xx_eeprom_gather_write().

Same answer.

> 
> 
>> +static const struct of_device_id lpc18xx_eeprom_of_match[] = {
>> +       { .compatible = "nxp,lpc1857-eeprom" },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);
> 
> nit: It's usual to place of_device_id struct just above the
> platform_driver struct.

I see.

> 
> 
>> +       eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
>> +       eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;
> 
> There is a BITS_PER_BYTE define in bitops.h that you might want to use here.

Right. Thanks.

> 
> 
>> +       /*
>> +        * Clock rate is generated by dividing the system bus clock by the
>> +        * division factor, contained in the divider register (minus 1 encoded).
>> +        */
>> +       clk_rate = clk_get_rate(eeprom->clk);
>> +       clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
>> +
>> +       /*
>> +        * Writing a single word to the page will start the erase/program cycle
>> +        * automatically
>> +        */
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
>> +                             LPC18XX_EEPROM_AUTOPROG_WORD);
>> +
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                             LPC18XX_EEPROM_PWRDWN_NO);
>> +
>> +       lpc18xx_regmap_config.max_register = resource_size(res) - 1;
>> +       lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
>> +       lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
>> +
>> +       regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
>> +                                 &lpc18xx_regmap_config);
>> +       if (IS_ERR(regmap)) {
>> +               dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
>> +               ret = PTR_ERR(regmap);
>> +               goto err_clk;
>> +       }
>> +
>> +       lpc18xx_nvmem_config.dev = dev;
>> +
>> +       eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
>> +       if (IS_ERR(eeprom->nvmem)) {
>> +               ret = PTR_ERR(eeprom->nvmem);
>> +               goto err_clk;
>> +       }
>> +
>> +       platform_set_drvdata(pdev, eeprom);
>> +
>> +       return 0;
>> +
>> +err_clk:
>> +       clk_disable_unprepare(eeprom->clk);
>> +
>> +       return ret;
>> +}
>> +
>> +static int lpc18xx_eeprom_remove(struct platform_device *pdev)
>> +{
>> +       struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
>> +
>> +       lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                             LPC18XX_EEPROM_PWRDWN_YES);
>> +
>> +       clk_disable_unprepare(eeprom->clk);
>> +
>> +       return nvmem_unregister(eeprom->nvmem);
> 
> Normally you do tear down in the reverse order of initialization.
> 
> Consider what happens here when you power down and disable the clock
> while there still are nvmem users of the eeprom.

You're right! Will fix this. Thanks again.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Joachim,

Sorry for the delay, I finally got the hardware again.

El 30/10/15 a las 11:55, Ariel D'Alessandro escribi?:
> Joachim,
> 
> El 24/10/15 a las 19:04, Joachim Eastwood escribi?:
>> Hi Ariel,
>>
>> On 19 October 2015 at 19:32, Ariel D'Alessandro
>> <ariel@vanguardiasur.com.ar> wrote:
>>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>>
>>> EEPROM size is 16384 bytes and it can be entirely read and
>>> written/erased with 1 word (4 bytes) granularity. The last page
>>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>>
>>> Erase/program time is less than 3ms. The EEPROM device requires a
>>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>>> the system bus clock by the division factor, contained in the divider
>>> register (minus 1 encoded).
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>> ---
>>>  drivers/nvmem/Kconfig          |   9 ++
>>>  drivers/nvmem/Makefile         |   2 +
>>>  drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 277 insertions(+)
>>>  create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>>
>>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>>> +                                      size_t reg_size, const void *val,
>>> +                                      size_t val_size)
>>> +{
>>> +       struct lpc18xx_eeprom_dev *eeprom = context;
>>> +       unsigned int offset = *(u32 *)reg;
>>> +
>>> +       /* 3 ms of erase/program time between each writing */
>>> +       while (val_size) {
>>> +               writel(*(u32 *)val, eeprom->mem_base + offset);
>>> +               usleep_range(3000, 4000);
>>> +               val_size -= eeprom->val_bytes;
>>> +               val += eeprom->val_bytes;
>>> +               offset += eeprom->val_bytes;
>>> +       }
>>
>> What happens here if 'val_size' is less than 4 or not dividable by 4?
> 
> AFAIK, before calling to gather_write, the caller ensures that
> 'val_size' % map->format.val_bytes == 0
> 
> Like nvmem_device_write() do this in
> drivers/base/regmap/regmap.c:regmap_raw_write()
> 
> Then, if val_size == 0, nothing will happen.
> 
>> Same thing for 'offset'.
> 
> Let me check this one.

As I said regmap will ensure that
  val_len % map->format.val_bytes == 0
in both read and write.

However, it will only ensure that
  reg % map->reg_stride == 0
in read calls, not in write.
A non-word aligned write to the EEPROM will fail, so I'm adding a check
for this.

Thanks for pointing this out.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Srinivas Kandagatla]

On 19/10/15 18:32, Ariel D'Alessandro wrote:
> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
s/commit/patch

> LPC185x/3x and LPC435x/3x/2x/1x devices.
>
> EEPROM size is 16384 bytes and it can be entirely read and
> written/erased with 1 word (4 bytes) granularity. The last page
> (128 bytes) contains the EEPROM initialization data and is not writable.
>
> Erase/program time is less than 3ms. The EEPROM device requires a
> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
> the system bus clock by the division factor, contained in the divider
> register (minus 1 encoded).
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
>   drivers/nvmem/Kconfig          |   9 ++
>   drivers/nvmem/Makefile         |   2 +
>   drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 277 insertions(+)
>   create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index bc4ea58..6ff1b50 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -25,6 +25,15 @@ config NVMEM_IMX_OCOTP
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-imx-ocotp.
>
> +config NVMEM_LPC18XX_EEPROM
> +	tristate "NXP LPC18XX EEPROM Memory Support"
> +	depends on ARCH_LPC18XX || COMPILE_TEST
> +	help
> +	  Say Y here to include support for NXP LPC18xx EEPROM memory found in
> +	  NXP LPC185x/3x and LPC435x/3x/2x/1x devices.
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called nvmem_lpc18xx_eeprom.
> +
>   config NVMEM_MXS_OCOTP
>   	tristate "Freescale MXS On-Chip OTP Memory Support"
>   	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 95dde3f..c14a556 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -8,6 +8,8 @@ nvmem_core-y			:= core.o
>   # Devices
>   obj-$(CONFIG_NVMEM_IMX_OCOTP)	+= nvmem-imx-ocotp.o
>   nvmem-imx-ocotp-y		:= imx-ocotp.o
> +obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
> +nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
>   obj-$(CONFIG_NVMEM_MXS_OCOTP)	+= nvmem-mxs-ocotp.o
>   nvmem-mxs-ocotp-y		:= mxs-ocotp.o
>   obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
> diff --git a/drivers/nvmem/lpc18xx_eeprom.c b/drivers/nvmem/lpc18xx_eeprom.c
> new file mode 100644
> index 0000000..ccdda66
> --- /dev/null
> +++ b/drivers/nvmem/lpc18xx_eeprom.c
> @@ -0,0 +1,266 @@
> +/*
> + * NXP LPC18xx/LPC43xx EEPROM memory NVMEM driver
> + *
> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of_device.h>
Why do you need above?

Also you should probably include

  #include <linux/platform_device.h>


> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +/* Registers */
> +#define LPC18XX_EEPROM_AUTOPROG		0x00c
> +#define LPC18XX_EEPROM_AUTOPROG_WORD	0x1
> +
> +#define LPC18XX_EEPROM_CLKDIV		0x014
> +
> +#define LPC18XX_EEPROM_PWRDWN		0x018
> +#define LPC18XX_EEPROM_PWRDWN_NO	0x0
> +#define LPC18XX_EEPROM_PWRDWN_YES	0x1
> +
> +/* Fixed page size (bytes) */
> +#define LPC18XX_EEPROM_PAGE_SIZE	0x80
> +
> +/* EEPROM device requires a ~1500 kHz clock (min 800 kHz, max 1600 kHz) */
> +#define LPC18XX_EEPROM_CLOCK_HZ		1500000
> +
> +struct lpc18xx_eeprom_dev {
> +	struct clk *clk;
> +	void __iomem *reg_base;
> +	void __iomem *mem_base;
> +	struct nvmem_device *nvmem;
> +	unsigned reg_bytes;
> +	unsigned val_bytes;
> +};
> +
> +static inline void lpc18xx_eeprom_writel(struct lpc18xx_eeprom_dev *eeprom,
> +					 u32 reg, u32 val)
> +{
> +	writel(val, eeprom->reg_base + reg);
> +}
I don't have a strong feeling but, I see no point to have a wrapper for 
writel which is only used in probe function.

> +
> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
> +				       size_t reg_size, const void *val,
> +				       size_t val_size)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom = context;
> +	unsigned int offset = *(u32 *)reg;
> +
> +	/* 3 ms of erase/program time between each writing */
> +	while (val_size) {
> +		writel(*(u32 *)val, eeprom->mem_base + offset);
> +		usleep_range(3000, 4000);
> +		val_size -= eeprom->val_bytes;
> +		val += eeprom->val_bytes;
> +		offset += eeprom->val_bytes;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpc18xx_eeprom_write(void *context, const void *data, size_t count)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom = context;
> +	unsigned int offset = eeprom->reg_bytes;
> +
> +	if (count <= offset)
> +		return -EINVAL;
> +
> +
unnecessary new line.

> +	return lpc18xx_eeprom_gather_write(context, data, eeprom->reg_bytes,
> +					   data + offset, count - offset);
> +}
> +
> +static int lpc18xx_eeprom_read(void *context, const void *reg, size_t reg_size,
> +			       void *val, size_t val_size)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom = context;
> +	unsigned int offset = *(u32 *)reg;
> +
> +	while (val_size) {
> +		*(u32 *)val = readl(eeprom->mem_base + offset);
> +		val_size -= eeprom->val_bytes;
> +		val += eeprom->val_bytes;
> +		offset += eeprom->val_bytes;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct regmap_bus lpc18xx_eeprom_bus = {
> +	.write = lpc18xx_eeprom_write,
> +	.gather_write = lpc18xx_eeprom_gather_write,
> +	.read = lpc18xx_eeprom_read,
> +	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> +	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +static struct regmap_config lpc18xx_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +};
> +
> +static bool lpc18xx_eeprom_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	/*
> +	 * The last page contains the EEPROM initialization data and is not
> +	 * writable.
> +	 */
> +	return reg <= lpc18xx_regmap_config.max_register -
> +						LPC18XX_EEPROM_PAGE_SIZE;
> +}
> +
> +static bool lpc18xx_eeprom_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg <= lpc18xx_regmap_config.max_register;
> +}
> +
> +static struct nvmem_config lpc18xx_nvmem_config = {
> +	.name = "lpc18xx-eeprom",
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id lpc18xx_eeprom_of_match[] = {
> +	{ .compatible = "nxp,lpc1857-eeprom" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lpc18xx_eeprom_of_match);
> +
> +static int lpc18xx_eeprom_probe(struct platform_device *pdev)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom;
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rst;
> +	unsigned long clk_rate;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	int ret;
> +
> +	eeprom = devm_kzalloc(dev, sizeof(*eeprom), GFP_KERNEL);
> +	if (!eeprom)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
> +	eeprom->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(eeprom->reg_base))
> +		return PTR_ERR(eeprom->reg_base);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem");
> +	eeprom->mem_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(eeprom->mem_base))
> +		return PTR_ERR(eeprom->mem_base);
> +
> +	eeprom->clk = devm_clk_get(&pdev->dev, "eeprom");
> +	if (IS_ERR(eeprom->clk)) {
> +		dev_err(&pdev->dev, "failed to get eeprom clock\n");
> +		return PTR_ERR(eeprom->clk);
> +	}
> +
> +	ret = clk_prepare_enable(eeprom->clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to prepare/enable eeprom clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	rst = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(rst)) {
> +		dev_err(dev, "failed to get reset: %ld\n", PTR_ERR(rst));
> +		ret = PTR_ERR(rst);
> +		goto err_clk;
> +	}
> +
> +	ret = reset_control_deassert(rst);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to deassert reset: %d\n", ret);
> +		goto err_clk;
> +	}
> +
> +	eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
> +	eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;
> +
> +	/*
> +	 * Clock rate is generated by dividing the system bus clock by the
> +	 * division factor, contained in the divider register (minus 1 encoded).
> +	 */
> +	clk_rate = clk_get_rate(eeprom->clk);
> +	clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
> +	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
> +
> +	/*
> +	 * Writing a single word to the page will start the erase/program cycle
> +	 * automatically
> +	 */
> +	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
> +			      LPC18XX_EEPROM_AUTOPROG_WORD);
> +
> +	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
> +			      LPC18XX_EEPROM_PWRDWN_NO);

Any reason not power up/dowm dynamically in 
lpc18xx_eeprom_write()/lpc18xx_eeprom_read().

This can potentially save some power.

> +
> +	lpc18xx_regmap_config.max_register = resource_size(res) - 1;
> +	lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
> +	lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
> +
> +	regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
> +				  &lpc18xx_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
> +		ret = PTR_ERR(regmap);
> +		goto err_clk;
> +	}
> +
> +	lpc18xx_nvmem_config.dev = dev;
> +
> +	eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
> +	if (IS_ERR(eeprom->nvmem)) {
> +		ret = PTR_ERR(eeprom->nvmem);
> +		goto err_clk;
> +	}
> +
> +	platform_set_drvdata(pdev, eeprom);
> +
> +	return 0;
> +
> +err_clk:

Should this error path also include power down/reset assert the eeprom?

> +	clk_disable_unprepare(eeprom->clk);
> +
> +	return ret;
> +}
> +
> +static int lpc18xx_eeprom_remove(struct platform_device *pdev)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
> +
> +	lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
> +			      LPC18XX_EEPROM_PWRDWN_YES);
> +
> +	clk_disable_unprepare(eeprom->clk);
> +
> +	return nvmem_unregister(eeprom->nvmem);

Same comment as Joachim, should the reset be asserted too?
> +}
> +
> +static struct platform_driver lpc18xx_eeprom_driver = {
> +	.probe = lpc18xx_eeprom_probe,
> +	.remove = lpc18xx_eeprom_remove,
> +	.driver = {
> +		.name = "lpc18xx-eeprom",
> +		.of_match_table = lpc18xx_eeprom_of_match,
> +	},
> +};
> +
> +module_platform_driver(lpc18xx_eeprom_driver);
> +
> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
> +MODULE_DESCRIPTION("NXP LPC18xx EEPROM memory Driver");
> +MODULE_LICENSE("GPL v2");
>

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Srinivas,

El 26/10/15 a las 11:23, Srinivas Kandagatla escribi?:
> 
> 
> On 19/10/15 18:32, Ariel D'Alessandro wrote:
>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
> s/commit/patch

OK.

> 
>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>
>> EEPROM size is 16384 bytes and it can be entirely read and
>> written/erased with 1 word (4 bytes) granularity. The last page
>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>
>> Erase/program time is less than 3ms. The EEPROM device requires a
>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>> the system bus clock by the division factor, contained in the divider
>> register (minus 1 encoded).
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>   drivers/nvmem/Kconfig          |   9 ++
>>   drivers/nvmem/Makefile         |   2 +
>>   drivers/nvmem/lpc18xx_eeprom.c | 266
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 277 insertions(+)
>>   create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>>
[snip]
>> diff --git a/drivers/nvmem/lpc18xx_eeprom.c
>> b/drivers/nvmem/lpc18xx_eeprom.c
>> new file mode 100644
>> index 0000000..ccdda66
>> --- /dev/null
>> +++ b/drivers/nvmem/lpc18xx_eeprom.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * NXP LPC18xx/LPC43xx EEPROM memory NVMEM driver
>> + *
>> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify it
>> + * under the terms of the GNU General Public License version 2 as
>> published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of_device.h>
> Why do you need above?
> 
> Also you should probably include
> 
>  #include <linux/platform_device.h>

You're right. No need for of_device.h, only platform_device.h.

> 
> 
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +/* Registers */
>> +#define LPC18XX_EEPROM_AUTOPROG        0x00c
>> +#define LPC18XX_EEPROM_AUTOPROG_WORD    0x1
>> +
>> +#define LPC18XX_EEPROM_CLKDIV        0x014
>> +
>> +#define LPC18XX_EEPROM_PWRDWN        0x018
>> +#define LPC18XX_EEPROM_PWRDWN_NO    0x0
>> +#define LPC18XX_EEPROM_PWRDWN_YES    0x1
>> +
>> +/* Fixed page size (bytes) */
>> +#define LPC18XX_EEPROM_PAGE_SIZE    0x80
>> +
>> +/* EEPROM device requires a ~1500 kHz clock (min 800 kHz, max 1600
>> kHz) */
>> +#define LPC18XX_EEPROM_CLOCK_HZ        1500000
>> +
>> +struct lpc18xx_eeprom_dev {
>> +    struct clk *clk;
>> +    void __iomem *reg_base;
>> +    void __iomem *mem_base;
>> +    struct nvmem_device *nvmem;
>> +    unsigned reg_bytes;
>> +    unsigned val_bytes;
>> +};
>> +
>> +static inline void lpc18xx_eeprom_writel(struct lpc18xx_eeprom_dev
>> *eeprom,
>> +                     u32 reg, u32 val)
>> +{
>> +    writel(val, eeprom->reg_base + reg);
>> +}
> I don't have a strong feeling but, I see no point to have a wrapper for
> writel which is only used in probe function.

Well, to be correct it's also used in remove function. And I might call
it in read()/write() functions to allow dynamic power on/off.
I'd prefer keeping the wrapper.

> 
>> +
>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>> +                       size_t reg_size, const void *val,
>> +                       size_t val_size)
>> +{
>> +    struct lpc18xx_eeprom_dev *eeprom = context;
>> +    unsigned int offset = *(u32 *)reg;
>> +
>> +    /* 3 ms of erase/program time between each writing */
>> +    while (val_size) {
>> +        writel(*(u32 *)val, eeprom->mem_base + offset);
>> +        usleep_range(3000, 4000);
>> +        val_size -= eeprom->val_bytes;
>> +        val += eeprom->val_bytes;
>> +        offset += eeprom->val_bytes;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int lpc18xx_eeprom_write(void *context, const void *data,
>> size_t count)
>> +{
>> +    struct lpc18xx_eeprom_dev *eeprom = context;
>> +    unsigned int offset = eeprom->reg_bytes;
>> +
>> +    if (count <= offset)
>> +        return -EINVAL;
>> +
>> +
> unnecessary new line.

ACK.

> 
>> +    return lpc18xx_eeprom_gather_write(context, data, eeprom->reg_bytes,
>> +                       data + offset, count - offset);
>> +}
>> +
[snip]
>> +
>> +static int lpc18xx_eeprom_probe(struct platform_device *pdev)
>> +{
>> +    struct lpc18xx_eeprom_dev *eeprom;
>> +    struct device *dev = &pdev->dev;
>> +    struct reset_control *rst;
>> +    unsigned long clk_rate;
>> +    struct regmap *regmap;
>> +    struct resource *res;
>> +    int ret;
>> +
>> +    eeprom = devm_kzalloc(dev, sizeof(*eeprom), GFP_KERNEL);
>> +    if (!eeprom)
>> +        return -ENOMEM;
>> +
>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg");
>> +    eeprom->reg_base = devm_ioremap_resource(dev, res);
>> +    if (IS_ERR(eeprom->reg_base))
>> +        return PTR_ERR(eeprom->reg_base);
>> +
>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem");
>> +    eeprom->mem_base = devm_ioremap_resource(dev, res);
>> +    if (IS_ERR(eeprom->mem_base))
>> +        return PTR_ERR(eeprom->mem_base);
>> +
>> +    eeprom->clk = devm_clk_get(&pdev->dev, "eeprom");
>> +    if (IS_ERR(eeprom->clk)) {
>> +        dev_err(&pdev->dev, "failed to get eeprom clock\n");
>> +        return PTR_ERR(eeprom->clk);
>> +    }
>> +
>> +    ret = clk_prepare_enable(eeprom->clk);
>> +    if (ret < 0) {
>> +        dev_err(dev, "failed to prepare/enable eeprom clk: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    rst = devm_reset_control_get(dev, NULL);
>> +    if (IS_ERR(rst)) {
>> +        dev_err(dev, "failed to get reset: %ld\n", PTR_ERR(rst));
>> +        ret = PTR_ERR(rst);
>> +        goto err_clk;
>> +    }
>> +
>> +    ret = reset_control_deassert(rst);
>> +    if (ret < 0) {
>> +        dev_err(dev, "failed to deassert reset: %d\n", ret);
>> +        goto err_clk;
>> +    }
>> +
>> +    eeprom->val_bytes = lpc18xx_regmap_config.val_bits / 8;
>> +    eeprom->reg_bytes = lpc18xx_regmap_config.reg_bits / 8;
>> +
>> +    /*
>> +     * Clock rate is generated by dividing the system bus clock by the
>> +     * division factor, contained in the divider register (minus 1
>> encoded).
>> +     */
>> +    clk_rate = clk_get_rate(eeprom->clk);
>> +    clk_rate = DIV_ROUND_UP(clk_rate, LPC18XX_EEPROM_CLOCK_HZ) - 1;
>> +    lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_CLKDIV, clk_rate);
>> +
>> +    /*
>> +     * Writing a single word to the page will start the erase/program
>> cycle
>> +     * automatically
>> +     */
>> +    lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_AUTOPROG,
>> +                  LPC18XX_EEPROM_AUTOPROG_WORD);
>> +
>> +    lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                  LPC18XX_EEPROM_PWRDWN_NO);
> 
> Any reason not power up/dowm dynamically in
> lpc18xx_eeprom_write()/lpc18xx_eeprom_read().
> 
> This can potentially save some power.

That sounds good. I'll add this feature and check that it works properly.

> 
>> +
>> +    lpc18xx_regmap_config.max_register = resource_size(res) - 1;
>> +    lpc18xx_regmap_config.writeable_reg = lpc18xx_eeprom_writeable_reg;
>> +    lpc18xx_regmap_config.readable_reg = lpc18xx_eeprom_readable_reg;
>> +
>> +    regmap = devm_regmap_init(dev, &lpc18xx_eeprom_bus, eeprom,
>> +                  &lpc18xx_regmap_config);
>> +    if (IS_ERR(regmap)) {
>> +        dev_err(dev, "regmap init failed: %ld\n", PTR_ERR(regmap));
>> +        ret = PTR_ERR(regmap);
>> +        goto err_clk;
>> +    }
>> +
>> +    lpc18xx_nvmem_config.dev = dev;
>> +
>> +    eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
>> +    if (IS_ERR(eeprom->nvmem)) {
>> +        ret = PTR_ERR(eeprom->nvmem);
>> +        goto err_clk;
>> +    }
>> +
>> +    platform_set_drvdata(pdev, eeprom);
>> +
>> +    return 0;
>> +
>> +err_clk:
> 
> Should this error path also include power down/reset assert the eeprom?

Yes, I'll add another cleanup routine.

> 
>> +    clk_disable_unprepare(eeprom->clk);
>> +
>> +    return ret;
>> +}
>> +
>> +static int lpc18xx_eeprom_remove(struct platform_device *pdev)
>> +{
>> +    struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
>> +
>> +    lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
>> +                  LPC18XX_EEPROM_PWRDWN_YES);
>> +
>> +    clk_disable_unprepare(eeprom->clk);
>> +
>> +    return nvmem_unregister(eeprom->nvmem);
> 
> Same comment as Joachim, should the reset be asserted too?

No. That would cause a reset, disabling EEPROM power down mode.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ezequiel Garcia]

Hi Srinivas, Ariel,

On 30 October 2015 at 12:42, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> El 26/10/15 a las 11:23, Srinivas Kandagatla escribi?:
>>
>>
>> On 19/10/15 18:32, Ariel D'Alessandro wrote:
>>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>> s/commit/patch
>
> OK.
>

What's wrong with using "commit" ?

If the patch gets accepted and is merged, it will become a commit. Hence,
it's customary to use "commit" in the commit log.

I really don't think it's wrong to use either patch or commit.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Stefan Wahren]

Hi Ariel,

Am 19.10.2015 um 19:32 schrieb Ariel D'Alessandro:
> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
> LPC185x/3x and LPC435x/3x/2x/1x devices.
>
> EEPROM size is 16384 bytes and it can be entirely read and
> written/erased with 1 word (4 bytes) granularity. The last page
> (128 bytes) contains the EEPROM initialization data and is not writable.
>
> Erase/program time is less than 3ms. The EEPROM device requires a
> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
> the system bus clock by the division factor, contained in the divider
> register (minus 1 encoded).
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
>  drivers/nvmem/Kconfig          |   9 ++
>  drivers/nvmem/Makefile         |   2 +
>  drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 277 insertions(+)
>  create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
> [...]
> +
> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
> +				       size_t reg_size, const void *val,
> +				       size_t val_size)
> +{
> +	struct lpc18xx_eeprom_dev *eeprom = context;
> +	unsigned int offset = *(u32 *)reg;
> +
> +	/* 3 ms of erase/program time between each writing */
> +	while (val_size) {
> +		writel(*(u32 *)val, eeprom->mem_base + offset);
> +		usleep_range(3000, 4000);

i think it would be good to verify that the EEPROM write operation has
really finished.

Best regards
Stefan

> +		val_size -= eeprom->val_bytes;
> +		val += eeprom->val_bytes;
> +		offset += eeprom->val_bytes;
> +	}
> +
> +	return 0;
> +}

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Hi Stefan,

Sorry for the delay.

El 03/11/15 a las 05:20, Stefan Wahren escribi?:
> Hi Ariel,
> 
> Am 19.10.2015 um 19:32 schrieb Ariel D'Alessandro:
>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>
>> EEPROM size is 16384 bytes and it can be entirely read and
>> written/erased with 1 word (4 bytes) granularity. The last page
>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>
>> Erase/program time is less than 3ms. The EEPROM device requires a
>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>> the system bus clock by the division factor, contained in the divider
>> register (minus 1 encoded).
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>>  drivers/nvmem/Kconfig          |   9 ++
>>  drivers/nvmem/Makefile         |   2 +
>>  drivers/nvmem/lpc18xx_eeprom.c | 266 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 277 insertions(+)
>>  create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>> [...]
>> +
>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>> +				       size_t reg_size, const void *val,
>> +				       size_t val_size)
>> +{
>> +	struct lpc18xx_eeprom_dev *eeprom = context;
>> +	unsigned int offset = *(u32 *)reg;
>> +
>> +	/* 3 ms of erase/program time between each writing */
>> +	while (val_size) {
>> +		writel(*(u32 *)val, eeprom->mem_base + offset);
>> +		usleep_range(3000, 4000);
> 
> i think it would be good to verify that the EEPROM write operation has
> really finished.

I'm not sure what are you proposing. Why could the write operation not
finish?

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Stefan Wahren]

Hi Ariel,

> Ariel D'Alessandro <ariel@vanguardiasur.com.ar> hat am 16. November 2015 um
> 16:29 geschrieben:
>
>
> Hi Stefan,
>
> Sorry for the delay.
>
> El 03/11/15 a las 05:20, Stefan Wahren escribi?:
> > Hi Ariel,
> >
> > Am 19.10.2015 um 19:32 schrieb Ariel D'Alessandro:
> >> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
> >> LPC185x/3x and LPC435x/3x/2x/1x devices.
> >>
> >> EEPROM size is 16384 bytes and it can be entirely read and
> >> written/erased with 1 word (4 bytes) granularity. The last page
> >> (128 bytes) contains the EEPROM initialization data and is not writable.
> >>
> >> Erase/program time is less than 3ms. The EEPROM device requires a
> >> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
> >> the system bus clock by the division factor, contained in the divider
> >> register (minus 1 encoded).
> >>
> >> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> >> ---
> >> drivers/nvmem/Kconfig | 9 ++
> >> drivers/nvmem/Makefile | 2 +
> >> drivers/nvmem/lpc18xx_eeprom.c | 266
> >> +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 277 insertions(+)
> >> create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
> >> [...]
> >> +
> >> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
> >> + size_t reg_size, const void *val,
> >> + size_t val_size)
> >> +{
> >> + struct lpc18xx_eeprom_dev *eeprom = context;
> >> + unsigned int offset = *(u32 *)reg;
> >> +
> >> + /* 3 ms of erase/program time between each writing */
> >> + while (val_size) {
> >> + writel(*(u32 *)val, eeprom->mem_base + offset);
> >> + usleep_range(3000, 4000);
> >
> > i think it would be good to verify that the EEPROM write operation has
> > really finished.
>
> I'm not sure what are you proposing. Why could the write operation not
> finish?

it's always good to keep in sync with the hardware. Here is an extract of
chapter 
"46.6.2.1 Writing and erase/programming" from the datasheet [1]:

  During programming, the EEPROM is not available for other operations. To
prevent
  undesired loss in performance which would be caused by stalling the bus, the
EEPROM
  instead generates an error for AHB read/writes and APB writes when programming
is
  busy. In order to prevent the error response, the program operation finished
interrupt
  can be enabled or the interrupt status bit can be polled.

Please blame me if it doesn't apply.

It's only a suggestion: How about checking the interrupt status bit for
END_OF_PROG after the 3 ms sleep?

Best regards
Stefan

[1] - http://www.nxp.com/documents/user_manual/UM10430.pdf

>
> --
> Ariel D'Alessandro, VanguardiaSur
> www.vanguardiasur.com.ar

[PATCH v2 2/4] nvmem: NXP LPC18xx EEPROM memory NVMEM driver

[Ariel D'Alessandro]

Stefan,

El 17/11/15 a las 07:01, Stefan Wahren escribi?:
> Hi Ariel,
> 
>> Ariel D'Alessandro <ariel@vanguardiasur.com.ar> hat am 16. November 2015 um
>> 16:29 geschrieben:
>>
>>
>> Hi Stefan,
>>
>> Sorry for the delay.
>>
>> El 03/11/15 a las 05:20, Stefan Wahren escribi?:
>>> Hi Ariel,
>>>
>>> Am 19.10.2015 um 19:32 schrieb Ariel D'Alessandro:
>>>> This commit adds support for NXP LPC18xx EEPROM memory found in NXP
>>>> LPC185x/3x and LPC435x/3x/2x/1x devices.
>>>>
>>>> EEPROM size is 16384 bytes and it can be entirely read and
>>>> written/erased with 1 word (4 bytes) granularity. The last page
>>>> (128 bytes) contains the EEPROM initialization data and is not writable.
>>>>
>>>> Erase/program time is less than 3ms. The EEPROM device requires a
>>>> ~1500 kHz clock (min 800 kHz, max 1600 kHz) that is generated dividing
>>>> the system bus clock by the division factor, contained in the divider
>>>> register (minus 1 encoded).
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>> ---
>>>> drivers/nvmem/Kconfig | 9 ++
>>>> drivers/nvmem/Makefile | 2 +
>>>> drivers/nvmem/lpc18xx_eeprom.c | 266
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 277 insertions(+)
>>>> create mode 100644 drivers/nvmem/lpc18xx_eeprom.c
>>>> [...]
>>>> +
>>>> +static int lpc18xx_eeprom_gather_write(void *context, const void *reg,
>>>> + size_t reg_size, const void *val,
>>>> + size_t val_size)
>>>> +{
>>>> + struct lpc18xx_eeprom_dev *eeprom = context;
>>>> + unsigned int offset = *(u32 *)reg;
>>>> +
>>>> + /* 3 ms of erase/program time between each writing */
>>>> + while (val_size) {
>>>> + writel(*(u32 *)val, eeprom->mem_base + offset);
>>>> + usleep_range(3000, 4000);
>>>
>>> i think it would be good to verify that the EEPROM write operation has
>>> really finished.
>>
>> I'm not sure what are you proposing. Why could the write operation not
>> finish?
> 
> it's always good to keep in sync with the hardware. Here is an extract of
> chapter 
> "46.6.2.1 Writing and erase/programming" from the datasheet [1]:
> 
>   During programming, the EEPROM is not available for other operations. To
> prevent
>   undesired loss in performance which would be caused by stalling the bus, the
> EEPROM
>   instead generates an error for AHB read/writes and APB writes when programming
> is
>   busy. In order to prevent the error response, the program operation finished
> interrupt
>   can be enabled or the interrupt status bit can be polled.
> 
> Please blame me if it doesn't apply.
> 
> It's only a suggestion: How about checking the interrupt status bit for
> END_OF_PROG after the 3 ms sleep?

Yeah, I think that's a good idea, pretty simple and it will ensure
(although 3ms should be enough) erase/program operation has effectively
finished.

Thanks for the explanation and suggestion. I'm submitting patchset v4
with this.

-- 
Ariel D'Alessandro, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2 3/4] ARM: dts: lpc18xx: add EEPROM memory node

[Ariel D'Alessandro]

Add node for the NXP LPC18xx EEPROM memory which can be found in
NXP LPC185x/3x and LPC435x/3x/2x/1x devices.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 arch/arm/boot/dts/lpc18xx.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/lpc18xx.dtsi b/arch/arm/boot/dts/lpc18xx.dtsi
index 52591d8..43e99cf 100644
--- a/arch/arm/boot/dts/lpc18xx.dtsi
+++ b/arch/arm/boot/dts/lpc18xx.dtsi
@@ -166,6 +166,18 @@
 			status = "disabled";
 		};
 
+		eeprom: eeprom at 4000e000 {
+			compatible = "nxp,lpc1857-eeprom";
+			reg =	<0x4000e000 0x1000>,
+				<0x20040000 0x4000>;
+			reg-names = "reg", "mem";
+			clocks = <&ccu1 CLK_CPU_EEPROM>;
+			clock-names = "eeprom";
+			resets = <&rgu 27>;
+			interrupts = <4>;
+			status = "disabled";
+		};
+
 		mac: ethernet at 40010000 {
 			compatible = "nxp,lpc1850-dwmac", "snps,dwmac-3.611", "snps,dwmac";
 			reg = <0x40010000 0x2000>;
-- 
2.6.1

[PATCH v2 3/4] ARM: dts: lpc18xx: add EEPROM memory node

[Joachim Eastwood]

Hi Ariel,

On 19 October 2015 at 19:32, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> Add node for the NXP LPC18xx EEPROM memory which can be found in
> NXP LPC185x/3x and LPC435x/3x/2x/1x devices.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>

Acked-by: Joachim Eastwood <manabian@gmail.com>

I'll pick it up once the driver hits next.

regards,
Joachim Eastwood

[PATCH v2 4/4] ARM: configs: lpc18xx: enable EEPROM NVMEM driver

[Ariel D'Alessandro]

Enable NVMEM driver for NXP LPC18xx EEPROM, which can be found in
NXP LPC185x/3x and LPC435x/3x/2x/1x devices.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
 arch/arm/configs/lpc18xx_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/lpc18xx_defconfig b/arch/arm/configs/lpc18xx_defconfig
index b758a80..2c7e0da 100644
--- a/arch/arm/configs/lpc18xx_defconfig
+++ b/arch/arm/configs/lpc18xx_defconfig
@@ -149,6 +149,8 @@ CONFIG_ARM_PL172_MPMC=y
 CONFIG_PWM=y
 CONFIG_PWM_LPC18XX_SCT=y
 CONFIG_PHY_LPC18XX_USB_OTG=y
+CONFIG_NVMEM=y
+CONFIG_NVMEM_LPC18XX_EEPROM=y
 CONFIG_EXT2_FS=y
 # CONFIG_FILE_LOCKING is not set
 # CONFIG_DNOTIFY is not set
-- 
2.6.1

[PATCH v2 4/4] ARM: configs: lpc18xx: enable EEPROM NVMEM driver

[Joachim Eastwood]

Hi Ariel,

On 19 October 2015 at 19:32, Ariel D'Alessandro
<ariel@vanguardiasur.com.ar> wrote:
> Enable NVMEM driver for NXP LPC18xx EEPROM, which can be found in
> NXP LPC185x/3x and LPC435x/3x/2x/1x devices.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>

Acked-by: Joachim Eastwood <manabian@gmail.com>

I'll pick it up once the driver hits next.

regards,
Joachim Eastwood