[RFC PATCH v6 0/2] Implement NVMEM/SoC bus support for Vybrid

[RFC PATCH v6 0/2] Implement NVMEM/SoC bus support for Vybrid

[Sanchayan Maity]

Hello,

This patchset is based on top of v5 of Srinivas's NVMEM framework patches.
Recently, a v6 was posted, however since I mean this patchset to be only
RFC I am not basing this on v6 for now. Applied the NVMEM framework patches
on shawn's tree along with Stefan's NAND patches and then tested on Colibri
VF61.

Would like to have some initial comments on the patchset. Maxime? Srinivas?

Thanks for the comments and feedback till now.

Version 5 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.0/03787.html

Version 4 of the patchset can be found here
https://lkml.org/lkml/2015/5/26/199

Version 3 of the patchset can be found here
http://www.spinics.net/lists/arm-kernel/msg420847.html

Version 2 of the patchset can be found here
http://www.spinics.net/lists/devicetree/msg80654.html

Version 1 of the patchset can be found here
http://www.spinics.net/lists/devicetree/msg80257.html

The RFC version can be found here
https://lkml.org/lkml/2015/5/11/13

Changes since v5:
- Use NVMEM framework by Srinivas and Maxime

Changes since v4:
- Use devm_* family of functions and use a struct to get rid of
  global variables (suggested by Joachim Eastwood)
- Make Kconfig govern the compilation with tristate, instead of
  earlier bool. Paul Bolle raised a valid point that perhaps this
  should have been built in with the bool, however I had not taken
  into consideration generic distro kernels and it makes sense to
  have this tristated. (comments from Paul Bolle and Andreas Farber)

Changes since v3:
- Instead of using the syscon_regmap_lookup_by_compatible function
  use a phandle in the device tree along with offsets specified in
  this phandle node and then read the offset along with the device
  node in the driver for reading from the required region.

Changes since v2:
- Implement the SoC bus code as a driver in drivers/soc
  by registering with fsl,mscm-cpucfg as per Arnd's feedback
  
Changes since v1:
- Sort the headers in alphabetical order

Changes since RFC:
- Use a DT entry for the ROM area while specifying it as syscon.

Notes same since v1:
Currently the required information is more or less read across the whole
SoC, but I guess we cannot change that since these are the locations
with the required information.

There seem to be three options for the revision field:
- ROM revision (see https://community.freescale.com/docs/DOC-94802)
- ANADIG revision (ANADIG_DIGIPROC, as used for the i.MX SoC's)
- OCOTP revision

Some numbers:

Colibri VF61 1.1A (2N02G)
- 0x00000013
- 0x00610000
- 0x01000000
- 0x410000c8

Colibri VF61 V1.0B (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- 0x410000c8

Colibri VF61 V1.0A (which is actually a VF600 SoC, no L2 cache, since
that was the only one we could buy back then, 1N02G printed on it)
- 0x00000011
- 0x00610000
- 0x01000000
- none...

Colibri VF50 V1.0A (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- none...

Vybrid Tower Rev J (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- 0x410000c8

The ROM revision differs the most, so we would like to go with the
revision information from the ROM register 0x80.

Regards,
Sanchayan.

Sanchayan Maity (2):
  ARM: dts: vfxxx: Add OCOTP and OCROM nodes
  nvmem: Add Vybrid OCOTP and OCROM support

 arch/arm/boot/dts/vfxxx.dtsi | 18 +++++++++++++
 drivers/nvmem/Kconfig        | 11 ++++++++
 drivers/nvmem/Makefile       |  2 ++
 drivers/nvmem/vf610-ocotp.c  | 60 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

-- 
2.4.4

[RFC PATCH v6 0/2] Implement NVMEM/SoC bus support for Vybrid

[Sanchayan Maity]

Hello,

This patchset is based on top of v5 of Srinivas's NVMEM framework patches.
Recently, a v6 was posted, however since I mean this patchset to be only
RFC I am not basing this on v6 for now. Applied the NVMEM framework patches
on shawn's tree along with Stefan's NAND patches and then tested on Colibri
VF61.

Would like to have some initial comments on the patchset. Maxime? Srinivas?

Thanks for the comments and feedback till now.

Version 5 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.0/03787.html

Version 4 of the patchset can be found here
https://lkml.org/lkml/2015/5/26/199

Version 3 of the patchset can be found here
http://www.spinics.net/lists/arm-kernel/msg420847.html

Version 2 of the patchset can be found here
http://www.spinics.net/lists/devicetree/msg80654.html

Version 1 of the patchset can be found here
http://www.spinics.net/lists/devicetree/msg80257.html

The RFC version can be found here
https://lkml.org/lkml/2015/5/11/13

Changes since v5:
- Use NVMEM framework by Srinivas and Maxime

Changes since v4:
- Use devm_* family of functions and use a struct to get rid of
  global variables (suggested by Joachim Eastwood)
- Make Kconfig govern the compilation with tristate, instead of
  earlier bool. Paul Bolle raised a valid point that perhaps this
  should have been built in with the bool, however I had not taken
  into consideration generic distro kernels and it makes sense to
  have this tristated. (comments from Paul Bolle and Andreas Farber)

Changes since v3:
- Instead of using the syscon_regmap_lookup_by_compatible function
  use a phandle in the device tree along with offsets specified in
  this phandle node and then read the offset along with the device
  node in the driver for reading from the required region.

Changes since v2:
- Implement the SoC bus code as a driver in drivers/soc
  by registering with fsl,mscm-cpucfg as per Arnd's feedback
  
Changes since v1:
- Sort the headers in alphabetical order

Changes since RFC:
- Use a DT entry for the ROM area while specifying it as syscon.

Notes same since v1:
Currently the required information is more or less read across the whole
SoC, but I guess we cannot change that since these are the locations
with the required information.

There seem to be three options for the revision field:
- ROM revision (see https://community.freescale.com/docs/DOC-94802)
- ANADIG revision (ANADIG_DIGIPROC, as used for the i.MX SoC's)
- OCOTP revision

Some numbers:

Colibri VF61 1.1A (2N02G)
- 0x00000013
- 0x00610000
- 0x01000000
- 0x410000c8

Colibri VF61 V1.0B (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- 0x410000c8

Colibri VF61 V1.0A (which is actually a VF600 SoC, no L2 cache, since
that was the only one we could buy back then, 1N02G printed on it)
- 0x00000011
- 0x00610000
- 0x01000000
- none...

Colibri VF50 V1.0A (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- none...

Vybrid Tower Rev J (1N02G)
- 0x00000011
- 0x00610000
- 0x01000000
- 0x410000c8

The ROM revision differs the most, so we would like to go with the
revision information from the ROM register 0x80.

Regards,
Sanchayan.

Sanchayan Maity (2):
  ARM: dts: vfxxx: Add OCOTP and OCROM nodes
  nvmem: Add Vybrid OCOTP and OCROM support

 arch/arm/boot/dts/vfxxx.dtsi | 18 +++++++++++++
 drivers/nvmem/Kconfig        | 11 ++++++++
 drivers/nvmem/Makefile       |  2 ++
 drivers/nvmem/vf610-ocotp.c  | 60 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

-- 
2.4.4

[RFC PATCH v6 1/2] ARM: dts: vfxxx: Add OCOTP and OCROM nodes

[Sanchayan Maity]

Add a device tree node for the On-Chip One Time Programmable
Controller (OCOTP) and the On-Chip ROM.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vfxxx.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 2f4b04d..6efdf49 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -57,6 +57,13 @@
 		interrupt-parent = <&mscm_ir>;
 		ranges;
 
+		ocrom: ocrom@00000000 {
+			compatible = "fsl,vf610-ocrom";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x00000000 0x18000>;
+		};
+
 		aips0: aips-bus@40000000 {
 			compatible = "fsl,aips-bus", "simple-bus";
 			#address-cells = <1>;
@@ -403,6 +410,17 @@
 				status = "disabled";
 			};
 
+			ocotp: ocotp@400a5000 {
+				compatible = "fsl,vf610-ocotp";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x400a5000 0x1000>;
+
+				ocotp_cfg: cfg@410 {
+					reg = <0x410 0x2>;
+				};
+			};
+
 			snvs0: snvs@400a7000 {
 			    compatible = "fsl,sec-v4.0-mon", "simple-bus";
 				#address-cells = <1>;
-- 
2.4.4

[RFC PATCH v6 1/2] ARM: dts: vfxxx: Add OCOTP and OCROM nodes

[Sanchayan Maity]

Add a device tree node for the On-Chip One Time Programmable
Controller (OCOTP) and the On-Chip ROM.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vfxxx.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 2f4b04d..6efdf49 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -57,6 +57,13 @@
 		interrupt-parent = <&mscm_ir>;
 		ranges;
 
+		ocrom: ocrom at 00000000 {
+			compatible = "fsl,vf610-ocrom";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x00000000 0x18000>;
+		};
+
 		aips0: aips-bus at 40000000 {
 			compatible = "fsl,aips-bus", "simple-bus";
 			#address-cells = <1>;
@@ -403,6 +410,17 @@
 				status = "disabled";
 			};
 
+			ocotp: ocotp at 400a5000 {
+				compatible = "fsl,vf610-ocotp";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x400a5000 0x1000>;
+
+				ocotp_cfg: cfg at 410 {
+					reg = <0x410 0x2>;
+				};
+			};
+
 			snvs0: snvs at 400a7000 {
 			    compatible = "fsl,sec-v4.0-mon", "simple-bus";
 				#address-cells = <1>;
-- 
2.4.4

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Sanchayan Maity]

The patch adds support for the On Chip One Time Programmable Peripheral
(OCOTP) and On Chip ROM (OCROM) support.

On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
revision ID.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/nvmem/Kconfig       | 11 +++++++++
 drivers/nvmem/Makefile      |  2 ++
 drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 17f1a57..557c1e0 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
 	  This driver can also be built as a module. If so, the module
 	  will be called eeprom-sunxi-sid.
 
+config NVMEM_VF610_OCOTP
+	tristate "VF610 SoCs OCOTP support"
+	depends on SOC_VF610
+	select REGMAP_MMIO
+	help
+	  This is a driver for the 'OCOTP' available on various Vybrid
+	  devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-vf610-ocotp.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index cc46791..a9ed113 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
 nvmem_qfprom-y			:= qfprom.o
 obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
 nvmem-sunxi-sid-y		:= sunxi-sid.o
+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
new file mode 100644
index 0000000..d98772d
--- /dev/null
+++ b/drivers/nvmem/vf610-ocotp.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include "nvmem-mmio.h"
+
+static struct regmap_config regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static struct nvmem_config ocotp_config = {
+	.name = "soc_id",
+};
+
+static struct nvmem_config rom_config = {
+	.name = "rom_rev",
+};
+
+static struct nvmem_mmio_data ocotp_data = {
+	.nvmem_config = &ocotp_config,
+	.regmap_config = &regmap_config,
+};
+
+static struct nvmem_mmio_data rom_data = {
+	.nvmem_config = &rom_config,
+	.regmap_config = &regmap_config,
+};
+
+static const struct of_device_id ocotp_of_match[] = {
+	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
+	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, ocotp_of_match);
+
+static struct platform_driver vf610_ocotp_driver = {
+	.probe = nvmem_mmio_probe,
+	.remove = nvmem_mmio_remove,
+	.driver = {
+		.name = "vf610-nvmem",
+		.of_match_table = ocotp_of_match,
+	},
+};
+module_platform_driver(vf610_ocotp_driver);
+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
+MODULE_DESCRIPTION("Vybrid NVMEM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.4.4

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Sanchayan Maity]

The patch adds support for the On Chip One Time Programmable Peripheral
(OCOTP) and On Chip ROM (OCROM) support.

On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
revision ID.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/nvmem/Kconfig       | 11 +++++++++
 drivers/nvmem/Makefile      |  2 ++
 drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 17f1a57..557c1e0 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
 	  This driver can also be built as a module. If so, the module
 	  will be called eeprom-sunxi-sid.
 
+config NVMEM_VF610_OCOTP
+	tristate "VF610 SoCs OCOTP support"
+	depends on SOC_VF610
+	select REGMAP_MMIO
+	help
+	  This is a driver for the 'OCOTP' available on various Vybrid
+	  devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-vf610-ocotp.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index cc46791..a9ed113 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
 nvmem_qfprom-y			:= qfprom.o
 obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
 nvmem-sunxi-sid-y		:= sunxi-sid.o
+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
new file mode 100644
index 0000000..d98772d
--- /dev/null
+++ b/drivers/nvmem/vf610-ocotp.c
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include "nvmem-mmio.h"
+
+static struct regmap_config regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static struct nvmem_config ocotp_config = {
+	.name = "soc_id",
+};
+
+static struct nvmem_config rom_config = {
+	.name = "rom_rev",
+};
+
+static struct nvmem_mmio_data ocotp_data = {
+	.nvmem_config = &ocotp_config,
+	.regmap_config = &regmap_config,
+};
+
+static struct nvmem_mmio_data rom_data = {
+	.nvmem_config = &rom_config,
+	.regmap_config = &regmap_config,
+};
+
+static const struct of_device_id ocotp_of_match[] = {
+	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
+	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, ocotp_of_match);
+
+static struct platform_driver vf610_ocotp_driver = {
+	.probe = nvmem_mmio_probe,
+	.remove = nvmem_mmio_remove,
+	.driver = {
+		.name = "vf610-nvmem",
+		.of_match_table = ocotp_of_match,
+	},
+};
+module_platform_driver(vf610_ocotp_driver);
+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
+MODULE_DESCRIPTION("Vybrid NVMEM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.4.4

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

Hi Sanchayan,


On 23/06/15 14:44, Sanchayan Maity wrote:
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) and On Chip ROM (OCROM) support.
>
> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> revision ID.
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>   drivers/nvmem/Kconfig       | 11 +++++++++
>   drivers/nvmem/Makefile      |  2 ++
>   drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 73 insertions(+)
>   create mode 100644 drivers/nvmem/vf610-ocotp.c
>

Fantastic! diff is already looking good, when compared to v5 patches,
"drivers/soc/fsl/soc-vf610.c | 166 
+++++++++++++++++++++++++++++++++++++++++++"

I think, We could even do a better job by moving qfprom and this driver 
to a simple-mmio-nvmem provider, see below comments.

> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..557c1e0 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>   	  This driver can also be built as a module. If so, the module
>   	  will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> +	tristate "VF610 SoCs OCOTP support"
> +	depends on SOC_VF610
> +	select REGMAP_MMIO
> +	help
> +	  This is a driver for the 'OCOTP' available on various Vybrid
> +	  devices.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nvmem-vf610-ocotp.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index cc46791..a9ed113 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
>   nvmem_qfprom-y			:= qfprom.o
>   obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
>   nvmem-sunxi-sid-y		:= sunxi-sid.o
> +obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
> +nvmem-vf610-ocotp-y		:= vf610-ocotp.o
> diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> new file mode 100644
> index 0000000..d98772d
> --- /dev/null
> +++ b/drivers/nvmem/vf610-ocotp.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include "nvmem-mmio.h"
> +
> +static struct regmap_config regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,

Any particular reason why it cant support byte reads?

This driver looks exactly same as qcom,qfprom driver, except the 
regmap_config.

> +};
> +
> +static struct nvmem_config ocotp_config = {
> +	.name = "soc_id",
> +};
> +
> +static struct nvmem_config rom_config = {
> +	.name = "rom_rev",
> +};
> +
> +static struct nvmem_mmio_data ocotp_data = {
> +	.nvmem_config = &ocotp_config,
> +	.regmap_config = &regmap_config,
> +};
> +
> +static struct nvmem_mmio_data rom_data = {
> +	.nvmem_config = &rom_config,
> +	.regmap_config = &regmap_config,
> +};
> +
> +static const struct of_device_id ocotp_of_match[] = {
> +	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
> +	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> +
> +static struct platform_driver vf610_ocotp_driver = {
> +	.probe = nvmem_mmio_probe,
> +	.remove = nvmem_mmio_remove,
> +	.driver = {
> +		.name = "vf610-nvmem",
> +		.of_match_table = ocotp_of_match,
> +	},
> +};
> +module_platform_driver(vf610_ocotp_driver);
> +MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
> +MODULE_DESCRIPTION("Vybrid NVMEM driver");
> +MODULE_LICENSE("GPL v2");
>


I moved the nvmem_mmio to qfprom as I did not any other user for that in 
v6 patches.

Now that Vybrid can be another user I should probably put it back, or 
may be I should create a simple-mmio-nvmem driver which both qcom,qfprom 
and Vybrid can use.

--srini

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

Hi Sanchayan,


On 23/06/15 14:44, Sanchayan Maity wrote:
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) and On Chip ROM (OCROM) support.
>
> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> revision ID.
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>   drivers/nvmem/Kconfig       | 11 +++++++++
>   drivers/nvmem/Makefile      |  2 ++
>   drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 73 insertions(+)
>   create mode 100644 drivers/nvmem/vf610-ocotp.c
>

Fantastic! diff is already looking good, when compared to v5 patches,
"drivers/soc/fsl/soc-vf610.c | 166 
+++++++++++++++++++++++++++++++++++++++++++"

I think, We could even do a better job by moving qfprom and this driver 
to a simple-mmio-nvmem provider, see below comments.

> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..557c1e0 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>   	  This driver can also be built as a module. If so, the module
>   	  will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> +	tristate "VF610 SoCs OCOTP support"
> +	depends on SOC_VF610
> +	select REGMAP_MMIO
> +	help
> +	  This is a driver for the 'OCOTP' available on various Vybrid
> +	  devices.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nvmem-vf610-ocotp.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index cc46791..a9ed113 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
>   nvmem_qfprom-y			:= qfprom.o
>   obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
>   nvmem-sunxi-sid-y		:= sunxi-sid.o
> +obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
> +nvmem-vf610-ocotp-y		:= vf610-ocotp.o
> diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> new file mode 100644
> index 0000000..d98772d
> --- /dev/null
> +++ b/drivers/nvmem/vf610-ocotp.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include "nvmem-mmio.h"
> +
> +static struct regmap_config regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,

Any particular reason why it cant support byte reads?

This driver looks exactly same as qcom,qfprom driver, except the 
regmap_config.

> +};
> +
> +static struct nvmem_config ocotp_config = {
> +	.name = "soc_id",
> +};
> +
> +static struct nvmem_config rom_config = {
> +	.name = "rom_rev",
> +};
> +
> +static struct nvmem_mmio_data ocotp_data = {
> +	.nvmem_config = &ocotp_config,
> +	.regmap_config = &regmap_config,
> +};
> +
> +static struct nvmem_mmio_data rom_data = {
> +	.nvmem_config = &rom_config,
> +	.regmap_config = &regmap_config,
> +};
> +
> +static const struct of_device_id ocotp_of_match[] = {
> +	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
> +	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> +
> +static struct platform_driver vf610_ocotp_driver = {
> +	.probe = nvmem_mmio_probe,
> +	.remove = nvmem_mmio_remove,
> +	.driver = {
> +		.name = "vf610-nvmem",
> +		.of_match_table = ocotp_of_match,
> +	},
> +};
> +module_platform_driver(vf610_ocotp_driver);
> +MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
> +MODULE_DESCRIPTION("Vybrid NVMEM driver");
> +MODULE_LICENSE("GPL v2");
>


I moved the nvmem_mmio to qfprom as I did not any other user for that in 
v6 patches.

Now that Vybrid can be another user I should probably put it back, or 
may be I should create a simple-mmio-nvmem driver which both qcom,qfprom 
and Vybrid can use.

--srini

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Sanchayan,

> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> geschrieben:
>
>
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) and On Chip ROM (OCROM) support.
>
> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> revision ID.
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/nvmem/Kconfig | 11 +++++++++
> drivers/nvmem/Makefile | 2 ++
> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 73 insertions(+)
> create mode 100644 drivers/nvmem/vf610-ocotp.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..557c1e0 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> This driver can also be built as a module. If so, the module
> will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> + tristate "VF610 SoCs OCOTP support"
> + depends on SOC_VF610
> + select REGMAP_MMIO

how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
MMIO?

Regards
Stefan

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Sanchayan,

> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> geschrieben:
>
>
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) and On Chip ROM (OCROM) support.
>
> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> revision ID.
>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/nvmem/Kconfig | 11 +++++++++
> drivers/nvmem/Makefile | 2 ++
> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 73 insertions(+)
> create mode 100644 drivers/nvmem/vf610-ocotp.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..557c1e0 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> This driver can also be built as a module. If so, the module
> will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> + tristate "VF610 SoCs OCOTP support"
> + depends on SOC_VF610
> + select REGMAP_MMIO

how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
MMIO?

Regards
Stefan

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan]

On 15-06-23 21:31:41, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> > geschrieben:
> >
> >
> > The patch adds support for the On Chip One Time Programmable Peripheral
> > (OCOTP) and On Chip ROM (OCROM) support.
> >
> > On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> > revision ID.
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/nvmem/Kconfig | 11 +++++++++
> > drivers/nvmem/Makefile | 2 ++
> > drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 73 insertions(+)
> > create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > index 17f1a57..557c1e0 100644
> > --- a/drivers/nvmem/Kconfig
> > +++ b/drivers/nvmem/Kconfig
> > @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> > This driver can also be built as a module. If so, the module
> > will be called eeprom-sunxi-sid.
> >
> > +config NVMEM_VF610_OCOTP
> > + tristate "VF610 SoCs OCOTP support"
> > + depends on SOC_VF610
> > + select REGMAP_MMIO
> 
> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> MMIO?

Frankly speaking I just changed the naming conventions and followed the qfrom
and sunxi sid examples in Srinivas's patches.

I just tested it without the "select REGMAP_MMIO" and it works just fine.

- Sanchayan.

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan at gmail.com]

On 15-06-23 21:31:41, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> > geschrieben:
> >
> >
> > The patch adds support for the On Chip One Time Programmable Peripheral
> > (OCOTP) and On Chip ROM (OCROM) support.
> >
> > On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> > revision ID.
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/nvmem/Kconfig | 11 +++++++++
> > drivers/nvmem/Makefile | 2 ++
> > drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 73 insertions(+)
> > create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > index 17f1a57..557c1e0 100644
> > --- a/drivers/nvmem/Kconfig
> > +++ b/drivers/nvmem/Kconfig
> > @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> > This driver can also be built as a module. If so, the module
> > will be called eeprom-sunxi-sid.
> >
> > +config NVMEM_VF610_OCOTP
> > + tristate "VF610 SoCs OCOTP support"
> > + depends on SOC_VF610
> > + select REGMAP_MMIO
> 
> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> MMIO?

Frankly speaking I just changed the naming conventions and followed the qfrom
and sunxi sid examples in Srinivas's patches.

I just tested it without the "select REGMAP_MMIO" and it works just fine.

- Sanchayan.

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan]

Hello Srinivas,

On 15-06-23 20:03:31, Srinivas Kandagatla wrote:
> Hi Sanchayan,
> 
> 
> On 23/06/15 14:44, Sanchayan Maity wrote:
> >The patch adds support for the On Chip One Time Programmable Peripheral
> >(OCOTP) and On Chip ROM (OCROM) support.
> >
> >On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >revision ID.
> >
> >Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >---
> >  drivers/nvmem/Kconfig       | 11 +++++++++
> >  drivers/nvmem/Makefile      |  2 ++
> >  drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 73 insertions(+)
> >  create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> 
> Fantastic! diff is already looking good, when compared to v5 patches,
> "drivers/soc/fsl/soc-vf610.c | 166
> +++++++++++++++++++++++++++++++++++++++++++"

Yes :)

> 
> I think, We could even do a better job by moving qfprom and this driver to a
> simple-mmio-nvmem provider, see below comments.
> 
> >diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >index 17f1a57..557c1e0 100644
> >--- a/drivers/nvmem/Kconfig
> >+++ b/drivers/nvmem/Kconfig
> >@@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called eeprom-sunxi-sid.
> >
> >+config NVMEM_VF610_OCOTP
> >+	tristate "VF610 SoCs OCOTP support"
> >+	depends on SOC_VF610
> >+	select REGMAP_MMIO
> >+	help
> >+	  This is a driver for the 'OCOTP' available on various Vybrid
> >+	  devices.
> >+
> >+	  This driver can also be built as a module. If so, the module
> >+	  will be called nvmem-vf610-ocotp.
> >+
> >  endif
> >diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> >index cc46791..a9ed113 100644
> >--- a/drivers/nvmem/Makefile
> >+++ b/drivers/nvmem/Makefile
> >@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
> >  nvmem_qfprom-y			:= qfprom.o
> >  obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
> >  nvmem-sunxi-sid-y		:= sunxi-sid.o
> >+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
> >+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
> >diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> >new file mode 100644
> >index 0000000..d98772d
> >--- /dev/null
> >+++ b/drivers/nvmem/vf610-ocotp.c
> >@@ -0,0 +1,60 @@
> >+/*
> >+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> >+ * only version 2 as published by the Free Software Foundation.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ */
> >+
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+#include "nvmem-mmio.h"
> >+
> >+static struct regmap_config regmap_config = {
> >+	.reg_bits = 32,
> >+	.val_bits = 32,
> >+	.reg_stride = 4,
> 
> Any particular reason why it cant support byte reads?
> 
> This driver looks exactly same as qcom,qfprom driver, except the
> regmap_config.

It does support byte reads. This works as well

static struct regmap_config regmap_config = {
	   .reg_bits = 32,
	   .val_bits = 8,
	   .reg_stride = 1,
	};
}

Hmm can't recall why I went with that. Yes it is more or less the
same as qcom,qfprom driver, that's what I referred.

> 
> >+};
> >+
> >+static struct nvmem_config ocotp_config = {
> >+	.name = "soc_id",
> >+};
> >+
> >+static struct nvmem_config rom_config = {
> >+	.name = "rom_rev",
> >+};
> >+
> >+static struct nvmem_mmio_data ocotp_data = {
> >+	.nvmem_config = &ocotp_config,
> >+	.regmap_config = &regmap_config,
> >+};
> >+
> >+static struct nvmem_mmio_data rom_data = {
> >+	.nvmem_config = &rom_config,
> >+	.regmap_config = &regmap_config,
> >+};
> >+
> >+static const struct of_device_id ocotp_of_match[] = {
> >+	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
> >+	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
> >+	{/* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, ocotp_of_match);
> >+
> >+static struct platform_driver vf610_ocotp_driver = {
> >+	.probe = nvmem_mmio_probe,
> >+	.remove = nvmem_mmio_remove,
> >+	.driver = {
> >+		.name = "vf610-nvmem",
> >+		.of_match_table = ocotp_of_match,
> >+	},
> >+};
> >+module_platform_driver(vf610_ocotp_driver);
> >+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
> >+MODULE_DESCRIPTION("Vybrid NVMEM driver");
> >+MODULE_LICENSE("GPL v2");
> >
> 
> 
> I moved the nvmem_mmio to qfprom as I did not any other user for that in v6
> patches.
> 
> Now that Vybrid can be another user I should probably put it back, or may be
> I should create a simple-mmio-nvmem driver which both qcom,qfprom and Vybrid
> can use.

Nice, then that would simplify things further from what I understand at the moment.

- Sanchayan.

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan at gmail.com]

Hello Srinivas,

On 15-06-23 20:03:31, Srinivas Kandagatla wrote:
> Hi Sanchayan,
> 
> 
> On 23/06/15 14:44, Sanchayan Maity wrote:
> >The patch adds support for the On Chip One Time Programmable Peripheral
> >(OCOTP) and On Chip ROM (OCROM) support.
> >
> >On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >revision ID.
> >
> >Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >---
> >  drivers/nvmem/Kconfig       | 11 +++++++++
> >  drivers/nvmem/Makefile      |  2 ++
> >  drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 73 insertions(+)
> >  create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> 
> Fantastic! diff is already looking good, when compared to v5 patches,
> "drivers/soc/fsl/soc-vf610.c | 166
> +++++++++++++++++++++++++++++++++++++++++++"

Yes :)

> 
> I think, We could even do a better job by moving qfprom and this driver to a
> simple-mmio-nvmem provider, see below comments.
> 
> >diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >index 17f1a57..557c1e0 100644
> >--- a/drivers/nvmem/Kconfig
> >+++ b/drivers/nvmem/Kconfig
> >@@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called eeprom-sunxi-sid.
> >
> >+config NVMEM_VF610_OCOTP
> >+	tristate "VF610 SoCs OCOTP support"
> >+	depends on SOC_VF610
> >+	select REGMAP_MMIO
> >+	help
> >+	  This is a driver for the 'OCOTP' available on various Vybrid
> >+	  devices.
> >+
> >+	  This driver can also be built as a module. If so, the module
> >+	  will be called nvmem-vf610-ocotp.
> >+
> >  endif
> >diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> >index cc46791..a9ed113 100644
> >--- a/drivers/nvmem/Makefile
> >+++ b/drivers/nvmem/Makefile
> >@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
> >  nvmem_qfprom-y			:= qfprom.o
> >  obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
> >  nvmem-sunxi-sid-y		:= sunxi-sid.o
> >+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
> >+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
> >diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> >new file mode 100644
> >index 0000000..d98772d
> >--- /dev/null
> >+++ b/drivers/nvmem/vf610-ocotp.c
> >@@ -0,0 +1,60 @@
> >+/*
> >+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> >+ * only version 2 as published by the Free Software Foundation.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ */
> >+
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+#include "nvmem-mmio.h"
> >+
> >+static struct regmap_config regmap_config = {
> >+	.reg_bits = 32,
> >+	.val_bits = 32,
> >+	.reg_stride = 4,
> 
> Any particular reason why it cant support byte reads?
> 
> This driver looks exactly same as qcom,qfprom driver, except the
> regmap_config.

It does support byte reads. This works as well

static struct regmap_config regmap_config = {
	   .reg_bits = 32,
	   .val_bits = 8,
	   .reg_stride = 1,
	};
}

Hmm can't recall why I went with that. Yes it is more or less the
same as qcom,qfprom driver, that's what I referred.

> 
> >+};
> >+
> >+static struct nvmem_config ocotp_config = {
> >+	.name = "soc_id",
> >+};
> >+
> >+static struct nvmem_config rom_config = {
> >+	.name = "rom_rev",
> >+};
> >+
> >+static struct nvmem_mmio_data ocotp_data = {
> >+	.nvmem_config = &ocotp_config,
> >+	.regmap_config = &regmap_config,
> >+};
> >+
> >+static struct nvmem_mmio_data rom_data = {
> >+	.nvmem_config = &rom_config,
> >+	.regmap_config = &regmap_config,
> >+};
> >+
> >+static const struct of_device_id ocotp_of_match[] = {
> >+	{ .compatible = "fsl,vf610-ocotp", .data = &ocotp_data},
> >+	{ .compatible = "fsl,vf610-ocrom", .data = &rom_data},
> >+	{/* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, ocotp_of_match);
> >+
> >+static struct platform_driver vf610_ocotp_driver = {
> >+	.probe = nvmem_mmio_probe,
> >+	.remove = nvmem_mmio_remove,
> >+	.driver = {
> >+		.name = "vf610-nvmem",
> >+		.of_match_table = ocotp_of_match,
> >+	},
> >+};
> >+module_platform_driver(vf610_ocotp_driver);
> >+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
> >+MODULE_DESCRIPTION("Vybrid NVMEM driver");
> >+MODULE_LICENSE("GPL v2");
> >
> 
> 
> I moved the nvmem_mmio to qfprom as I did not any other user for that in v6
> patches.
> 
> Now that Vybrid can be another user I should probably put it back, or may be
> I should create a simple-mmio-nvmem driver which both qcom,qfprom and Vybrid
> can use.

Nice, then that would simplify things further from what I understand at the moment.

- Sanchayan.

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Maxime Ripard]

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

Hi, 

On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
> +static struct nvmem_config ocotp_config = {
> +	.name = "soc_id",
> +};
> +
> +static struct nvmem_config rom_config = {
> +	.name = "rom_rev",
> +};

Srinivas, shouldn't we use the DT to setup these names, just like
clock-output-names does for example?

This is very likely to change from one board to another, and defining
a new compatible and/or driver for each board seems a bit fishy.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Maxime Ripard]

Hi, 

On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
> +static struct nvmem_config ocotp_config = {
> +	.name = "soc_id",
> +};
> +
> +static struct nvmem_config rom_config = {
> +	.name = "rom_rev",
> +};

Srinivas, shouldn't we use the DT to setup these names, just like
clock-output-names does for example?

This is very likely to change from one board to another, and defining
a new compatible and/or driver for each board seems a bit fishy.

Maxime

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

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

On 24/06/15 09:35, Maxime Ripard wrote:
> Hi,
>
> On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
>> +static struct nvmem_config ocotp_config = {
>> +	.name = "soc_id",
>> +};
>> +
>> +static struct nvmem_config rom_config = {
>> +	.name = "rom_rev",
>> +};
>
> Srinivas, shouldn't we use the DT to setup these names, just like
> clock-output-names does for example?
These are the provider names, which would not change per board, I think. :-)

On the other hand if we are going to use generic drivers like 
"simple-mmio-nvmem" then having name DT bindings makes sense.

IMO, clock-output-names are analogous to nvmem consumers, which are 
obviously getting there names from cell node name ATM.


>
> This is very likely to change from one board to another, and defining
> a new compatible and/or driver for each board seems a bit fishy.
>
Do you have any particular example in mind, where the provider names 
would change per board?

--srini

> Maxime
>

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

On 24/06/15 09:35, Maxime Ripard wrote:
> Hi,
>
> On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
>> +static struct nvmem_config ocotp_config = {
>> +	.name = "soc_id",
>> +};
>> +
>> +static struct nvmem_config rom_config = {
>> +	.name = "rom_rev",
>> +};
>
> Srinivas, shouldn't we use the DT to setup these names, just like
> clock-output-names does for example?
These are the provider names, which would not change per board, I think. :-)

On the other hand if we are going to use generic drivers like 
"simple-mmio-nvmem" then having name DT bindings makes sense.

IMO, clock-output-names are analogous to nvmem consumers, which are 
obviously getting there names from cell node name ATM.


>
> This is very likely to change from one board to another, and defining
> a new compatible and/or driver for each board seems a bit fishy.
>
Do you have any particular example in mind, where the provider names 
would change per board?

--srini

> Maxime
>

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

On 24/06/15 06:19, maitysanchayan@gmail.com wrote:
> I just tested it without the "select REGMAP_MMIO" and it works just fine.
You just got lucky in this case, as REGMAP_MMIO is getting selected 
somewhere else.

Drivers should directly select the dependecies. In this case this driver 
actually is going to use REGMAP_MMIO, so keeping it selected is the 
right thing.

--srini

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Srinivas Kandagatla]

On 24/06/15 06:19, maitysanchayan at gmail.com wrote:
> I just tested it without the "select REGMAP_MMIO" and it works just fine.
You just got lucky in this case, as REGMAP_MMIO is getting selected 
somewhere else.

Drivers should directly select the dependecies. In this case this driver 
actually is going to use REGMAP_MMIO, so keeping it selected is the 
right thing.

--srini

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Sanchayan Maity]

On 15-06-24 10:37:43, Srinivas Kandagatla wrote:
> 
> 
> On 24/06/15 06:19, maitysanchayan@gmail.com wrote:
> >I just tested it without the "select REGMAP_MMIO" and it works just fine.
> You just got lucky in this case, as REGMAP_MMIO is getting selected
> somewhere else.
> 
> Drivers should directly select the dependecies. In this case this driver
> actually is going to use REGMAP_MMIO, so keeping it selected is the right
> thing.

Yes, you are right. Checking the .config file shows that this is enabled even
with the explicit selection removed in my driver's kconfig.

Sorry about missing that Stefan and Srinivas.

- Sanchayan.

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Sanchayan Maity]

On 15-06-24 10:37:43, Srinivas Kandagatla wrote:
> 
> 
> On 24/06/15 06:19, maitysanchayan at gmail.com wrote:
> >I just tested it without the "select REGMAP_MMIO" and it works just fine.
> You just got lucky in this case, as REGMAP_MMIO is getting selected
> somewhere else.
> 
> Drivers should directly select the dependecies. In this case this driver
> actually is going to use REGMAP_MMIO, so keeping it selected is the right
> thing.

Yes, you are right. Checking the .config file shows that this is enabled even
with the explicit selection removed in my driver's kconfig.

Sorry about missing that Stefan and Srinivas.

- Sanchayan.

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Sanchayan,

Am 24.06.2015 um 07:19 schrieb maitysanchayan@gmail.com:
> On 15-06-23 21:31:41, Stefan Wahren wrote:
>> Hi Sanchayan,
>>
>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>> geschrieben:
>>>
>>>
>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>
>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>> revision ID.
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>> drivers/nvmem/Kconfig | 11 +++++++++
>>> drivers/nvmem/Makefile | 2 ++
>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 73 insertions(+)
>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>
>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>> index 17f1a57..557c1e0 100644
>>> --- a/drivers/nvmem/Kconfig
>>> +++ b/drivers/nvmem/Kconfig
>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>> This driver can also be built as a module. If so, the module
>>> will be called eeprom-sunxi-sid.
>>>
>>> +config NVMEM_VF610_OCOTP
>>> + tristate "VF610 SoCs OCOTP support"
>>> + depends on SOC_VF610
>>> + select REGMAP_MMIO
>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>> MMIO?
> Frankly speaking I just changed the naming conventions and followed the qfrom
> and sunxi sid examples in Srinivas's patches.
>
> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>
> - Sanchayan.

sorry for the confusion. My question refers to the whole driver
implementation not only to the REGMAP_MMIO.

According to

Vybrid Reference Manual F-Series
Document Number: VYBRIDRM
Rev 7, 06/2014

35.5 OCOTP memory map/register definition

the memory region is organized in control and shadow registers. I'm very
sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.

It possible that it works in your case. But in the case the lock bits
are set the driver won't work correctly.

Stefan

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Sanchayan,

Am 24.06.2015 um 07:19 schrieb maitysanchayan at gmail.com:
> On 15-06-23 21:31:41, Stefan Wahren wrote:
>> Hi Sanchayan,
>>
>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>> geschrieben:
>>>
>>>
>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>
>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>> revision ID.
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>> drivers/nvmem/Kconfig | 11 +++++++++
>>> drivers/nvmem/Makefile | 2 ++
>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 73 insertions(+)
>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>
>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>> index 17f1a57..557c1e0 100644
>>> --- a/drivers/nvmem/Kconfig
>>> +++ b/drivers/nvmem/Kconfig
>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>> This driver can also be built as a module. If so, the module
>>> will be called eeprom-sunxi-sid.
>>>
>>> +config NVMEM_VF610_OCOTP
>>> + tristate "VF610 SoCs OCOTP support"
>>> + depends on SOC_VF610
>>> + select REGMAP_MMIO
>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>> MMIO?
> Frankly speaking I just changed the naming conventions and followed the qfrom
> and sunxi sid examples in Srinivas's patches.
>
> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>
> - Sanchayan.

sorry for the confusion. My question refers to the whole driver
implementation not only to the REGMAP_MMIO.

According to

Vybrid Reference Manual F-Series
Document Number: VYBRIDRM
Rev 7, 06/2014

35.5 OCOTP memory map/register definition

the memory region is organized in control and shadow registers. I'm very
sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.

It possible that it works in your case. But in the case the lock bits
are set the driver won't work correctly.

Stefan

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan]

Hello Stefan,

On 15-06-24 11:56:14, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> Am 24.06.2015 um 07:19 schrieb maitysanchayan@gmail.com:
> > On 15-06-23 21:31:41, Stefan Wahren wrote:
> >> Hi Sanchayan,
> >>
> >>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> >>> geschrieben:
> >>>
> >>>
> >>> The patch adds support for the On Chip One Time Programmable Peripheral
> >>> (OCOTP) and On Chip ROM (OCROM) support.
> >>>
> >>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >>> revision ID.
> >>>
> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>> ---
> >>> drivers/nvmem/Kconfig | 11 +++++++++
> >>> drivers/nvmem/Makefile | 2 ++
> >>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 73 insertions(+)
> >>> create mode 100644 drivers/nvmem/vf610-ocotp.c
> >>>
> >>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >>> index 17f1a57..557c1e0 100644
> >>> --- a/drivers/nvmem/Kconfig
> >>> +++ b/drivers/nvmem/Kconfig
> >>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >>> This driver can also be built as a module. If so, the module
> >>> will be called eeprom-sunxi-sid.
> >>>
> >>> +config NVMEM_VF610_OCOTP
> >>> + tristate "VF610 SoCs OCOTP support"
> >>> + depends on SOC_VF610
> >>> + select REGMAP_MMIO
> >> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> >> MMIO?
> > Frankly speaking I just changed the naming conventions and followed the qfrom
> > and sunxi sid examples in Srinivas's patches.
> >
> > I just tested it without the "select REGMAP_MMIO" and it works just fine.
> >
> > - Sanchayan.
> 
> sorry for the confusion. My question refers to the whole driver
> implementation not only to the REGMAP_MMIO.
> 
> According to
> 
> Vybrid Reference Manual F-Series
> Document Number: VYBRIDRM
> Rev 7, 06/2014
> 
> 35.5 OCOTP memory map/register definition
> 
> the memory region is organized in control and shadow registers. I'm very
> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.

Sorry I am not well versed with the regmap stuff. Can you please tell me why
REGMAP_MMIO is not the right way for accessing the OCOTP?

If this is not the right way, I assume you are referring to the procedures
in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?

> 
> It possible that it works in your case. But in the case the lock bits
> are set the driver won't work correctly.

As such the previous implementations were very different.

Currently I only expect to provide a way for users to read the SoC ID from
the OCOTP block. My understanding was even if the lock bits are set, reading
from the registers will return 0xBADABADA.

For example, currently for three locations I see this from ocotp block

*
0000080 bada bada bada bada bada bada bada bada
*
0000500 bada bada bada bada bada bada bada bada
*
0000c80 bada bada bada bada bada bada bada bada
*

- Sanchayan.

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan at gmail.com]

Hello Stefan,

On 15-06-24 11:56:14, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> Am 24.06.2015 um 07:19 schrieb maitysanchayan at gmail.com:
> > On 15-06-23 21:31:41, Stefan Wahren wrote:
> >> Hi Sanchayan,
> >>
> >>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> >>> geschrieben:
> >>>
> >>>
> >>> The patch adds support for the On Chip One Time Programmable Peripheral
> >>> (OCOTP) and On Chip ROM (OCROM) support.
> >>>
> >>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >>> revision ID.
> >>>
> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>> ---
> >>> drivers/nvmem/Kconfig | 11 +++++++++
> >>> drivers/nvmem/Makefile | 2 ++
> >>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 73 insertions(+)
> >>> create mode 100644 drivers/nvmem/vf610-ocotp.c
> >>>
> >>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >>> index 17f1a57..557c1e0 100644
> >>> --- a/drivers/nvmem/Kconfig
> >>> +++ b/drivers/nvmem/Kconfig
> >>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >>> This driver can also be built as a module. If so, the module
> >>> will be called eeprom-sunxi-sid.
> >>>
> >>> +config NVMEM_VF610_OCOTP
> >>> + tristate "VF610 SoCs OCOTP support"
> >>> + depends on SOC_VF610
> >>> + select REGMAP_MMIO
> >> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> >> MMIO?
> > Frankly speaking I just changed the naming conventions and followed the qfrom
> > and sunxi sid examples in Srinivas's patches.
> >
> > I just tested it without the "select REGMAP_MMIO" and it works just fine.
> >
> > - Sanchayan.
> 
> sorry for the confusion. My question refers to the whole driver
> implementation not only to the REGMAP_MMIO.
> 
> According to
> 
> Vybrid Reference Manual F-Series
> Document Number: VYBRIDRM
> Rev 7, 06/2014
> 
> 35.5 OCOTP memory map/register definition
> 
> the memory region is organized in control and shadow registers. I'm very
> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.

Sorry I am not well versed with the regmap stuff. Can you please tell me why
REGMAP_MMIO is not the right way for accessing the OCOTP?

If this is not the right way, I assume you are referring to the procedures
in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?

> 
> It possible that it works in your case. But in the case the lock bits
> are set the driver won't work correctly.

As such the previous implementations were very different.

Currently I only expect to provide a way for users to read the SoC ID from
the OCOTP block. My understanding was even if the lock bits are set, reading
from the registers will return 0xBADABADA.

For example, currently for three locations I see this from ocotp block

*
0000080 bada bada bada bada bada bada bada bada
*
0000500 bada bada bada bada bada bada bada bada
*
0000c80 bada bada bada bada bada bada bada bada
*

- Sanchayan.

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Agner]

On 2015-06-24 11:56, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> Am 24.06.2015 um 07:19 schrieb maitysanchayan@gmail.com:
>> On 15-06-23 21:31:41, Stefan Wahren wrote:
>>> Hi Sanchayan,
>>>
>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>>> geschrieben:
>>>>
>>>>
>>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>>
>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>>> revision ID.
>>>>
>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>> ---
>>>> drivers/nvmem/Kconfig | 11 +++++++++
>>>> drivers/nvmem/Makefile | 2 ++
>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 73 insertions(+)
>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>>
>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>>> index 17f1a57..557c1e0 100644
>>>> --- a/drivers/nvmem/Kconfig
>>>> +++ b/drivers/nvmem/Kconfig
>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>>> This driver can also be built as a module. If so, the module
>>>> will be called eeprom-sunxi-sid.
>>>>
>>>> +config NVMEM_VF610_OCOTP
>>>> + tristate "VF610 SoCs OCOTP support"
>>>> + depends on SOC_VF610
>>>> + select REGMAP_MMIO
>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>>> MMIO?
>> Frankly speaking I just changed the naming conventions and followed the qfrom
>> and sunxi sid examples in Srinivas's patches.
>>
>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>>
>> - Sanchayan.
> 
> sorry for the confusion. My question refers to the whole driver
> implementation not only to the REGMAP_MMIO.
> 
> According to
> 
> Vybrid Reference Manual F-Series
> Document Number: VYBRIDRM
> Rev 7, 06/2014
> 
> 35.5 OCOTP memory map/register definition
> 
> the memory region is organized in control and shadow registers. I'm very
> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> 
> It possible that it works in your case. But in the case the lock bits
> are set the driver won't work correctly.

The OCOTP represents fuses, starting from 0x400A5400 one can access the
shadowed registers containing the fuses values. The reference manual
also says: "All shadow registers are always readable through the APB bus
except some secret key"

The driver does not need write access. I guess it is fine to use only
reads through regmap...?

--
Stefan

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Agner]

On 2015-06-24 11:56, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> Am 24.06.2015 um 07:19 schrieb maitysanchayan at gmail.com:
>> On 15-06-23 21:31:41, Stefan Wahren wrote:
>>> Hi Sanchayan,
>>>
>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>>> geschrieben:
>>>>
>>>>
>>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>>
>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>>> revision ID.
>>>>
>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>> ---
>>>> drivers/nvmem/Kconfig | 11 +++++++++
>>>> drivers/nvmem/Makefile | 2 ++
>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 73 insertions(+)
>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>>
>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>>> index 17f1a57..557c1e0 100644
>>>> --- a/drivers/nvmem/Kconfig
>>>> +++ b/drivers/nvmem/Kconfig
>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>>> This driver can also be built as a module. If so, the module
>>>> will be called eeprom-sunxi-sid.
>>>>
>>>> +config NVMEM_VF610_OCOTP
>>>> + tristate "VF610 SoCs OCOTP support"
>>>> + depends on SOC_VF610
>>>> + select REGMAP_MMIO
>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>>> MMIO?
>> Frankly speaking I just changed the naming conventions and followed the qfrom
>> and sunxi sid examples in Srinivas's patches.
>>
>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>>
>> - Sanchayan.
> 
> sorry for the confusion. My question refers to the whole driver
> implementation not only to the REGMAP_MMIO.
> 
> According to
> 
> Vybrid Reference Manual F-Series
> Document Number: VYBRIDRM
> Rev 7, 06/2014
> 
> 35.5 OCOTP memory map/register definition
> 
> the memory region is organized in control and shadow registers. I'm very
> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> 
> It possible that it works in your case. But in the case the lock bits
> are set the driver won't work correctly.

The OCOTP represents fuses, starting from 0x400A5400 one can access the
shadowed registers containing the fuses values. The reference manual
also says: "All shadow registers are always readable through the APB bus
except some secret key"

The driver does not need write access. I guess it is fine to use only
reads through regmap...?

--
Stefan

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Srinivas,

Am 24.06.2015 um 12:45 schrieb maitysanchayan@gmail.com:
> Hello Stefan,
>
> On 15-06-24 11:56:14, Stefan Wahren wrote:
>> Hi Sanchayan,
>>
>> Am 24.06.2015 um 07:19 schrieb maitysanchayan@gmail.com:
>>> On 15-06-23 21:31:41, Stefan Wahren wrote:
>>>> Hi Sanchayan,
>>>>
>>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>>>
>>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>>>> revision ID.
>>>>>
>>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>>> ---
>>>>> drivers/nvmem/Kconfig | 11 +++++++++
>>>>> drivers/nvmem/Makefile | 2 ++
>>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 73 insertions(+)
>>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>>>
>>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>>>> index 17f1a57..557c1e0 100644
>>>>> --- a/drivers/nvmem/Kconfig
>>>>> +++ b/drivers/nvmem/Kconfig
>>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>>>> This driver can also be built as a module. If so, the module
>>>>> will be called eeprom-sunxi-sid.
>>>>>
>>>>> +config NVMEM_VF610_OCOTP
>>>>> + tristate "VF610 SoCs OCOTP support"
>>>>> + depends on SOC_VF610
>>>>> + select REGMAP_MMIO
>>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>>>> MMIO?
>>> Frankly speaking I just changed the naming conventions and followed the qfrom
>>> and sunxi sid examples in Srinivas's patches.
>>>
>>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>>>
>>> - Sanchayan.
>> sorry for the confusion. My question refers to the whole driver
>> implementation not only to the REGMAP_MMIO.
>>
>> According to
>>
>> Vybrid Reference Manual F-Series
>> Document Number: VYBRIDRM
>> Rev 7, 06/2014
>>
>> 35.5 OCOTP memory map/register definition
>>
>> the memory region is organized in control and shadow registers. I'm very
>> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> Sorry I am not well versed with the regmap stuff. Can you please tell me why
> REGMAP_MMIO is not the right way for accessing the OCOTP?

i think the implementation of OCOTP driver should be more like this:

https://git.linaro.org/people/srinivas.kandagatla/linux.git/commit/b4c3ad253747767511233687436f20144e850d67

It uses REGMAP with a specialized read function.

>
> If this is not the right way, I assume you are referring to the procedures
> in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?

Yes. I think writing isn't needed. But the read function should check
at least for OCOTP_CTRL[BUSY] and OCOTP_CTRL[ERROR]. If one of the
bits is set, the read function should return with an error.

This is the same behavior of my OCOTP driver for MXS platform.

Unfortunately i haven't push the driver to my github account.

>> It possible that it works in your case. But in the case the lock bits
>> are set the driver won't work correctly.
> As such the previous implementations were very different.
>
> Currently I only expect to provide a way for users to read the SoC ID from
> the OCOTP block. My understanding was even if the lock bits are set, reading
> from the registers will return 0xBADABADA.
>
> For example, currently for three locations I see this from ocotp block
>
> *
> 0000080 bada bada bada bada bada bada bada bada
> *
> 0000500 bada bada bada bada bada bada bada bada
> *
> 0000c80 bada bada bada bada bada bada bada bada
> *
>
> - Sanchayan.

Until somebody comes to the idea to fetch the MAC address from the OCOTP
block ...

How about doing it right at this point, instead of fixing it later?

Stefan

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Stefan Wahren]

Hi Srinivas,

Am 24.06.2015 um 12:45 schrieb maitysanchayan at gmail.com:
> Hello Stefan,
>
> On 15-06-24 11:56:14, Stefan Wahren wrote:
>> Hi Sanchayan,
>>
>> Am 24.06.2015 um 07:19 schrieb maitysanchayan at gmail.com:
>>> On 15-06-23 21:31:41, Stefan Wahren wrote:
>>>> Hi Sanchayan,
>>>>
>>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> The patch adds support for the On Chip One Time Programmable Peripheral
>>>>> (OCOTP) and On Chip ROM (OCROM) support.
>>>>>
>>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
>>>>> revision ID.
>>>>>
>>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>>> ---
>>>>> drivers/nvmem/Kconfig | 11 +++++++++
>>>>> drivers/nvmem/Makefile | 2 ++
>>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 73 insertions(+)
>>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
>>>>>
>>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>>>> index 17f1a57..557c1e0 100644
>>>>> --- a/drivers/nvmem/Kconfig
>>>>> +++ b/drivers/nvmem/Kconfig
>>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
>>>>> This driver can also be built as a module. If so, the module
>>>>> will be called eeprom-sunxi-sid.
>>>>>
>>>>> +config NVMEM_VF610_OCOTP
>>>>> + tristate "VF610 SoCs OCOTP support"
>>>>> + depends on SOC_VF610
>>>>> + select REGMAP_MMIO
>>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
>>>> MMIO?
>>> Frankly speaking I just changed the naming conventions and followed the qfrom
>>> and sunxi sid examples in Srinivas's patches.
>>>
>>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
>>>
>>> - Sanchayan.
>> sorry for the confusion. My question refers to the whole driver
>> implementation not only to the REGMAP_MMIO.
>>
>> According to
>>
>> Vybrid Reference Manual F-Series
>> Document Number: VYBRIDRM
>> Rev 7, 06/2014
>>
>> 35.5 OCOTP memory map/register definition
>>
>> the memory region is organized in control and shadow registers. I'm very
>> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> Sorry I am not well versed with the regmap stuff. Can you please tell me why
> REGMAP_MMIO is not the right way for accessing the OCOTP?

i think the implementation of OCOTP driver should be more like this:

https://git.linaro.org/people/srinivas.kandagatla/linux.git/commit/b4c3ad253747767511233687436f20144e850d67

It uses REGMAP with a specialized read function.

>
> If this is not the right way, I assume you are referring to the procedures
> in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?

Yes. I think writing isn't needed. But the read function should check
at least for OCOTP_CTRL[BUSY] and OCOTP_CTRL[ERROR]. If one of the
bits is set, the read function should return with an error.

This is the same behavior of my OCOTP driver for MXS platform.

Unfortunately i haven't push the driver to my github account.

>> It possible that it works in your case. But in the case the lock bits
>> are set the driver won't work correctly.
> As such the previous implementations were very different.
>
> Currently I only expect to provide a way for users to read the SoC ID from
> the OCOTP block. My understanding was even if the lock bits are set, reading
> from the registers will return 0xBADABADA.
>
> For example, currently for three locations I see this from ocotp block
>
> *
> 0000080 bada bada bada bada bada bada bada bada
> *
> 0000500 bada bada bada bada bada bada bada bada
> *
> 0000c80 bada bada bada bada bada bada bada bada
> *
>
> - Sanchayan.

Until somebody comes to the idea to fetch the MAC address from the OCOTP
block ...

How about doing it right at this point, instead of fixing it later?

Stefan

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan]

Hello Stefan,

On 15-06-24 13:52:28, Stefan Wahren wrote:
> Hi Srinivas,
> 
> Am 24.06.2015 um 12:45 schrieb maitysanchayan@gmail.com:
> > Hello Stefan,
> >
> > On 15-06-24 11:56:14, Stefan Wahren wrote:
> >> Hi Sanchayan,
> >>
> >> Am 24.06.2015 um 07:19 schrieb maitysanchayan@gmail.com:
> >>> On 15-06-23 21:31:41, Stefan Wahren wrote:
> >>>> Hi Sanchayan,
> >>>>
> >>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> >>>>> geschrieben:
> >>>>>
> >>>>>
> >>>>> The patch adds support for the On Chip One Time Programmable Peripheral
> >>>>> (OCOTP) and On Chip ROM (OCROM) support.
> >>>>>
> >>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >>>>> revision ID.
> >>>>>
> >>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>>>> ---
> >>>>> drivers/nvmem/Kconfig | 11 +++++++++
> >>>>> drivers/nvmem/Makefile | 2 ++
> >>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>> 3 files changed, 73 insertions(+)
> >>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
> >>>>>
> >>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >>>>> index 17f1a57..557c1e0 100644
> >>>>> --- a/drivers/nvmem/Kconfig
> >>>>> +++ b/drivers/nvmem/Kconfig
> >>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >>>>> This driver can also be built as a module. If so, the module
> >>>>> will be called eeprom-sunxi-sid.
> >>>>>
> >>>>> +config NVMEM_VF610_OCOTP
> >>>>> + tristate "VF610 SoCs OCOTP support"
> >>>>> + depends on SOC_VF610
> >>>>> + select REGMAP_MMIO
> >>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> >>>> MMIO?
> >>> Frankly speaking I just changed the naming conventions and followed the qfrom
> >>> and sunxi sid examples in Srinivas's patches.
> >>>
> >>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
> >>>
> >>> - Sanchayan.
> >> sorry for the confusion. My question refers to the whole driver
> >> implementation not only to the REGMAP_MMIO.
> >>
> >> According to
> >>
> >> Vybrid Reference Manual F-Series
> >> Document Number: VYBRIDRM
> >> Rev 7, 06/2014
> >>
> >> 35.5 OCOTP memory map/register definition
> >>
> >> the memory region is organized in control and shadow registers. I'm very
> >> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> > Sorry I am not well versed with the regmap stuff. Can you please tell me why
> > REGMAP_MMIO is not the right way for accessing the OCOTP?
> 
> i think the implementation of OCOTP driver should be more like this:
> 
> https://git.linaro.org/people/srinivas.kandagatla/linux.git/commit/b4c3ad253747767511233687436f20144e850d67
> 
> It uses REGMAP with a specialized read function.

Thank you very much for the link.

> 
> >
> > If this is not the right way, I assume you are referring to the procedures
> > in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?
> 
> Yes. I think writing isn't needed. But the read function should check
> at least for OCOTP_CTRL[BUSY] and OCOTP_CTRL[ERROR]. If one of the
> bits is set, the read function should return with an error.

Understood. Will incoporate this in the v6 patch.

> 
> This is the same behavior of my OCOTP driver for MXS platform.
> 
> Unfortunately i haven't push the driver to my github account.
> 
> >> It possible that it works in your case. But in the case the lock bits
> >> are set the driver won't work correctly.
> > As such the previous implementations were very different.
> >
> > Currently I only expect to provide a way for users to read the SoC ID from
> > the OCOTP block. My understanding was even if the lock bits are set, reading
> > from the registers will return 0xBADABADA.
> >
> > For example, currently for three locations I see this from ocotp block
> >
> > *
> > 0000080 bada bada bada bada bada bada bada bada
> > *
> > 0000500 bada bada bada bada bada bada bada bada
> > *
> > 0000c80 bada bada bada bada bada bada bada bada
> > *
> >
> > - Sanchayan.
> 
> Until somebody comes to the idea to fetch the MAC address from the OCOTP
> block ...
> 
> How about doing it right at this point, instead of fixing it later?

Of course. Thanks for the feedback Stefan.

- Sanchayan.

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[maitysanchayan at gmail.com]

Hello Stefan,

On 15-06-24 13:52:28, Stefan Wahren wrote:
> Hi Srinivas,
> 
> Am 24.06.2015 um 12:45 schrieb maitysanchayan at gmail.com:
> > Hello Stefan,
> >
> > On 15-06-24 11:56:14, Stefan Wahren wrote:
> >> Hi Sanchayan,
> >>
> >> Am 24.06.2015 um 07:19 schrieb maitysanchayan at gmail.com:
> >>> On 15-06-23 21:31:41, Stefan Wahren wrote:
> >>>> Hi Sanchayan,
> >>>>
> >>>>> Sanchayan Maity <maitysanchayan@gmail.com> hat am 23. Juni 2015 um 15:44
> >>>>> geschrieben:
> >>>>>
> >>>>>
> >>>>> The patch adds support for the On Chip One Time Programmable Peripheral
> >>>>> (OCOTP) and On Chip ROM (OCROM) support.
> >>>>>
> >>>>> On Vybrid OCOTP contain data like SoC ID, MAC address and OCROM has the
> >>>>> revision ID.
> >>>>>
> >>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >>>>> ---
> >>>>> drivers/nvmem/Kconfig | 11 +++++++++
> >>>>> drivers/nvmem/Makefile | 2 ++
> >>>>> drivers/nvmem/vf610-ocotp.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>> 3 files changed, 73 insertions(+)
> >>>>> create mode 100644 drivers/nvmem/vf610-ocotp.c
> >>>>>
> >>>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> >>>>> index 17f1a57..557c1e0 100644
> >>>>> --- a/drivers/nvmem/Kconfig
> >>>>> +++ b/drivers/nvmem/Kconfig
> >>>>> @@ -33,4 +33,15 @@ config NVMEM_SUNXI_SID
> >>>>> This driver can also be built as a module. If so, the module
> >>>>> will be called eeprom-sunxi-sid.
> >>>>>
> >>>>> +config NVMEM_VF610_OCOTP
> >>>>> + tristate "VF610 SoCs OCOTP support"
> >>>>> + depends on SOC_VF610
> >>>>> + select REGMAP_MMIO
> >>>> how do you come to the conclusion that Vybrid On-Chip OTP is accessable via
> >>>> MMIO?
> >>> Frankly speaking I just changed the naming conventions and followed the qfrom
> >>> and sunxi sid examples in Srinivas's patches.
> >>>
> >>> I just tested it without the "select REGMAP_MMIO" and it works just fine.
> >>>
> >>> - Sanchayan.
> >> sorry for the confusion. My question refers to the whole driver
> >> implementation not only to the REGMAP_MMIO.
> >>
> >> According to
> >>
> >> Vybrid Reference Manual F-Series
> >> Document Number: VYBRIDRM
> >> Rev 7, 06/2014
> >>
> >> 35.5 OCOTP memory map/register definition
> >>
> >> the memory region is organized in control and shadow registers. I'm very
> >> sceptical that using REGMAP_MMIO is the right way for accessing the OCOTP.
> > Sorry I am not well versed with the regmap stuff. Can you please tell me why
> > REGMAP_MMIO is not the right way for accessing the OCOTP?
> 
> i think the implementation of OCOTP driver should be more like this:
> 
> https://git.linaro.org/people/srinivas.kandagatla/linux.git/commit/b4c3ad253747767511233687436f20144e850d67
> 
> It uses REGMAP with a specialized read function.

Thank you very much for the link.

> 
> >
> > If this is not the right way, I assume you are referring to the procedures
> > in section 35.3.1.5 and 35.3.1.6 for reading and writing from these areas?
> 
> Yes. I think writing isn't needed. But the read function should check
> at least for OCOTP_CTRL[BUSY] and OCOTP_CTRL[ERROR]. If one of the
> bits is set, the read function should return with an error.

Understood. Will incoporate this in the v6 patch.

> 
> This is the same behavior of my OCOTP driver for MXS platform.
> 
> Unfortunately i haven't push the driver to my github account.
> 
> >> It possible that it works in your case. But in the case the lock bits
> >> are set the driver won't work correctly.
> > As such the previous implementations were very different.
> >
> > Currently I only expect to provide a way for users to read the SoC ID from
> > the OCOTP block. My understanding was even if the lock bits are set, reading
> > from the registers will return 0xBADABADA.
> >
> > For example, currently for three locations I see this from ocotp block
> >
> > *
> > 0000080 bada bada bada bada bada bada bada bada
> > *
> > 0000500 bada bada bada bada bada bada bada bada
> > *
> > 0000c80 bada bada bada bada bada bada bada bada
> > *
> >
> > - Sanchayan.
> 
> Until somebody comes to the idea to fetch the MAC address from the OCOTP
> block ...
> 
> How about doing it right at this point, instead of fixing it later?

Of course. Thanks for the feedback Stefan.

- Sanchayan.

Re: [RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Maxime Ripard]

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

Hi,

On Wed, Jun 24, 2015 at 10:34:43AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 24/06/15 09:35, Maxime Ripard wrote:
> >Hi,
> >
> >On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
> >>+static struct nvmem_config ocotp_config = {
> >>+	.name = "soc_id",
> >>+};
> >>+
> >>+static struct nvmem_config rom_config = {
> >>+	.name = "rom_rev",
> >>+};
> >
> >Srinivas, shouldn't we use the DT to setup these names, just like
> >clock-output-names does for example?
>
> These are the provider names, which would not change per board, I
> think. :-)

Except that my understanding is that it will fallback to the list of
these names if there's nothing in the DT.

> On the other hand if we are going to use generic drivers like
> "simple-mmio-nvmem" then having name DT bindings makes sense.
> 
> IMO, clock-output-names are analogous to nvmem consumers, which are
> obviously getting there names from cell node name ATM.

clock-output-names is the name of the clock itself within the
system. clock-names is the name of the clock to the consumer.

So, it looks like nvmem-names is strictly equivalent to clock-names
(local to the device, doesn't affect the name of the provider within
in the system) which makes perfect sense, but we don't really have an
equivalent to clock-output-names but this one (global range, defines
the name of the provider globally), which is fine for specific
devices, whose format and functions are very standardized, like those
EEPROMs, but is not so much for other more generic devices, like the
mmio-nvmem driver you were talking about, or ...

> >This is very likely to change from one board to another, and defining
> >a new compatible and/or driver for each board seems a bit fishy.
> >
> Do you have any particular example in mind, where the provider names would
> change per board?

... AT24, AT25, and all those generic, off-the-SoC EEPROMs, where each
board vendor is pretty much free to do whatever he wants with it.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC PATCH v6 2/2] nvmem: Add Vybrid OCOTP and OCROM support

[Maxime Ripard]

Hi,

On Wed, Jun 24, 2015 at 10:34:43AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 24/06/15 09:35, Maxime Ripard wrote:
> >Hi,
> >
> >On Tue, Jun 23, 2015 at 07:14:57PM +0530, Sanchayan Maity wrote:
> >>+static struct nvmem_config ocotp_config = {
> >>+	.name = "soc_id",
> >>+};
> >>+
> >>+static struct nvmem_config rom_config = {
> >>+	.name = "rom_rev",
> >>+};
> >
> >Srinivas, shouldn't we use the DT to setup these names, just like
> >clock-output-names does for example?
>
> These are the provider names, which would not change per board, I
> think. :-)

Except that my understanding is that it will fallback to the list of
these names if there's nothing in the DT.

> On the other hand if we are going to use generic drivers like
> "simple-mmio-nvmem" then having name DT bindings makes sense.
> 
> IMO, clock-output-names are analogous to nvmem consumers, which are
> obviously getting there names from cell node name ATM.

clock-output-names is the name of the clock itself within the
system. clock-names is the name of the clock to the consumer.

So, it looks like nvmem-names is strictly equivalent to clock-names
(local to the device, doesn't affect the name of the provider within
in the system) which makes perfect sense, but we don't really have an
equivalent to clock-output-names but this one (global range, defines
the name of the provider globally), which is fine for specific
devices, whose format and functions are very standardized, like those
EEPROMs, but is not so much for other more generic devices, like the
mmio-nvmem driver you were talking about, or ...

> >This is very likely to change from one board to another, and defining
> >a new compatible and/or driver for each board seems a bit fishy.
> >
> Do you have any particular example in mind, where the provider names would
> change per board?

... AT24, AT25, and all those generic, off-the-SoC EEPROMs, where each
board vendor is pretty much free to do whatever he wants with it.

Maxime

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

[RFC PATCH v6 0/3] Implement NVMEM/SoC bus support for Vybrid

[Sanchayan Maity]

Hello,

For what it's worth, a reworked implementation of the OCOTP driver
implementing the proper fuse and shadow register read sequence as
mentioned in the TRM.

I will avoid unnecessary churn after this and send the next v7 once
the nvmem framework gets merged.

Used the OCOTP code in barebox as a reference and the code is highly
similar to that. iMX6 OCOTP memory map is however different to Vybrid.

This set also does not contain code for exposing the ROM information,
have yet to check that.

Feedback and comments are most welcome. Thank you for all the reviews
till now.

Regards,
Sanchayan.

Sanchayan Maity (3):
  clk: clk-vf610: Add clock for Vybrid OCOTP controller
  ARM: dts: vfxxx: Add OCOTP node
  drivers: nvmem: Add Vybrid OCOTP support

 arch/arm/boot/dts/vfxxx.dtsi            |   7 +
 drivers/clk/imx/clk-vf610.c             |   1 +
 drivers/nvmem/Kconfig                   |  10 ++
 drivers/nvmem/Makefile                  |   2 +
 drivers/nvmem/vf610-ocotp.c             | 250 ++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/vf610-clock.h |   3 +-
 6 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

-- 
2.4.4

[RFC PATCH v6 0/3] Implement NVMEM/SoC bus support for Vybrid

[Sanchayan Maity]

Hello,

For what it's worth, a reworked implementation of the OCOTP driver
implementing the proper fuse and shadow register read sequence as
mentioned in the TRM.

I will avoid unnecessary churn after this and send the next v7 once
the nvmem framework gets merged.

Used the OCOTP code in barebox as a reference and the code is highly
similar to that. iMX6 OCOTP memory map is however different to Vybrid.

This set also does not contain code for exposing the ROM information,
have yet to check that.

Feedback and comments are most welcome. Thank you for all the reviews
till now.

Regards,
Sanchayan.

Sanchayan Maity (3):
  clk: clk-vf610: Add clock for Vybrid OCOTP controller
  ARM: dts: vfxxx: Add OCOTP node
  drivers: nvmem: Add Vybrid OCOTP support

 arch/arm/boot/dts/vfxxx.dtsi            |   7 +
 drivers/clk/imx/clk-vf610.c             |   1 +
 drivers/nvmem/Kconfig                   |  10 ++
 drivers/nvmem/Makefile                  |   2 +
 drivers/nvmem/vf610-ocotp.c             | 250 ++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/vf610-clock.h |   3 +-
 6 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

-- 
2.4.4

[RFC PATCH v6 1/3] clk: clk-vf610: Add clock for Vybrid OCOTP controller

[Sanchayan Maity]

Add clock support for Vybrid On-Chip One Time Programmable
(OCOTP) controller.

While the OCOTP block does not require explicit clock gating,
for programming the OCOTP timing register the clock rate of
ipg clock is required for timing calculations related to fuse
and shadow register read sequence. We explicitly specify the
ipg clock for OCOTP as a result.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/clk/imx/clk-vf610.c             | 1 +
 include/dt-bindings/clock/vf610-clock.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-vf610.c b/drivers/clk/imx/clk-vf610.c
index bff45ea..d1b1c95 100644
--- a/drivers/clk/imx/clk-vf610.c
+++ b/drivers/clk/imx/clk-vf610.c
@@ -387,6 +387,7 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)
 
 	clk[VF610_CLK_SNVS] = imx_clk_gate2("snvs-rtc", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));
 	clk[VF610_CLK_DAP] = imx_clk_gate("dap", "platform_bus", CCM_CCSR, 24);
+	clk[VF610_CLK_OCOTP] = imx_clk_gate("ocotp", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(5));
 
 	imx_check_clocks(clk, ARRAY_SIZE(clk));
 
diff --git a/include/dt-bindings/clock/vf610-clock.h b/include/dt-bindings/clock/vf610-clock.h
index d197634..56c16aa 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -194,6 +194,7 @@
 #define VF610_PLL7_BYPASS		181
 #define VF610_CLK_SNVS			182
 #define VF610_CLK_DAP			183
-#define VF610_CLK_END			184
+#define VF610_CLK_OCOTP         184
+#define VF610_CLK_END			185
 
 #endif /* __DT_BINDINGS_CLOCK_VF610_H */
-- 
2.4.4

[RFC PATCH v6 1/3] clk: clk-vf610: Add clock for Vybrid OCOTP controller

[Sanchayan Maity]

Add clock support for Vybrid On-Chip One Time Programmable
(OCOTP) controller.

While the OCOTP block does not require explicit clock gating,
for programming the OCOTP timing register the clock rate of
ipg clock is required for timing calculations related to fuse
and shadow register read sequence. We explicitly specify the
ipg clock for OCOTP as a result.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/clk/imx/clk-vf610.c             | 1 +
 include/dt-bindings/clock/vf610-clock.h | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-vf610.c b/drivers/clk/imx/clk-vf610.c
index bff45ea..d1b1c95 100644
--- a/drivers/clk/imx/clk-vf610.c
+++ b/drivers/clk/imx/clk-vf610.c
@@ -387,6 +387,7 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)
 
 	clk[VF610_CLK_SNVS] = imx_clk_gate2("snvs-rtc", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));
 	clk[VF610_CLK_DAP] = imx_clk_gate("dap", "platform_bus", CCM_CCSR, 24);
+	clk[VF610_CLK_OCOTP] = imx_clk_gate("ocotp", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(5));
 
 	imx_check_clocks(clk, ARRAY_SIZE(clk));
 
diff --git a/include/dt-bindings/clock/vf610-clock.h b/include/dt-bindings/clock/vf610-clock.h
index d197634..56c16aa 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -194,6 +194,7 @@
 #define VF610_PLL7_BYPASS		181
 #define VF610_CLK_SNVS			182
 #define VF610_CLK_DAP			183
-#define VF610_CLK_END			184
+#define VF610_CLK_OCOTP         184
+#define VF610_CLK_END			185
 
 #endif /* __DT_BINDINGS_CLOCK_VF610_H */
-- 
2.4.4

[RFC PATCH v6 2/3] ARM: dts: vfxxx: Add OCOTP node

[Sanchayan Maity]

Add device tree node for the On-Chip One Time Programmable
controller (OCOTP) on the Vybrid platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vfxxx.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 2f4b04d..05730d9 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -403,6 +403,13 @@
 				status = "disabled";
 			};
 
+			ocotp: ocotp@400a5000 {
+				compatible = "fsl,vf610-ocotp";
+				reg = <0x400a5000 0xD00>;
+				clocks = <&clks VF610_CLK_OCOTP>;
+				clock-names = "ocotp";
+			};
+
 			snvs0: snvs@400a7000 {
 			    compatible = "fsl,sec-v4.0-mon", "simple-bus";
 				#address-cells = <1>;
-- 
2.4.4

[RFC PATCH v6 2/3] ARM: dts: vfxxx: Add OCOTP node

[Sanchayan Maity]

Add device tree node for the On-Chip One Time Programmable
controller (OCOTP) on the Vybrid platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vfxxx.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 2f4b04d..05730d9 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -403,6 +403,13 @@
 				status = "disabled";
 			};
 
+			ocotp: ocotp at 400a5000 {
+				compatible = "fsl,vf610-ocotp";
+				reg = <0x400a5000 0xD00>;
+				clocks = <&clks VF610_CLK_OCOTP>;
+				clock-names = "ocotp";
+			};
+
 			snvs0: snvs at 400a7000 {
 			    compatible = "fsl,sec-v4.0-mon", "simple-bus";
 				#address-cells = <1>;
-- 
2.4.4

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Sanchayan Maity]

The patch adds support for the On Chip One Time Programmable Peripheral
(OCOTP) on the Vybrid platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/nvmem/Kconfig       |  10 ++
 drivers/nvmem/Makefile      |   2 +
 drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 262 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 17f1a57..84c830d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
 	  This driver can also be built as a module. If so, the module
 	  will be called eeprom-sunxi-sid.
 
+config NVMEM_VF610_OCOTP
+	tristate "VF610 SoCs OCOTP support"
+	depends on SOC_VF610
+	help
+	  This is a driver for the 'OCOTP' available on various Vybrid
+	  devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-vf610-ocotp.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index cc46791..a9ed113 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
 nvmem_qfprom-y			:= qfprom.o
 obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
 nvmem-sunxi-sid-y		:= sunxi-sid.o
+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
new file mode 100644
index 0000000..b7a782c
--- /dev/null
+++ b/drivers/nvmem/vf610-ocotp.c
@@ -0,0 +1,250 @@
+/*
+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/nvmem-provider.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* OCOTP Register Offsets */
+#define OCOTP_CTRL_REG          0x00
+#define OCOTP_CTRL_SET          0x04
+#define OCOTP_CTRL_CLR          0x08
+#define OCOTP_TIMING            0x10
+#define OCOTP_DATA              0x20
+#define OCOTP_READ_CTRL_REG     0x30
+#define OCOTP_READ_FUSE_DATA    0x40
+
+/* OCOTP Register bits and masks */
+#define OCOTP_CTRL_WR_UNLOCK         16
+#define OCOTP_CTRL_WR_UNLOCK_KEY     0x3E77
+#define OCOTP_CTRL_WR_UNLOCK_MASK    0xFFFF0000
+#define OCOTP_CTRL_ADDR              0
+#define OCOTP_CTRL_ADDR_MASK         0x7F
+#define OCOTP_CTRL_RELOAD_SHADOWS    (0x1 << 10)
+#define OCOTP_CTRL_ERROR             (0x1 << 9)
+#define OCOTP_CTRL_BUSY              (0x1 << 8)
+
+#define OCOTP_TIMING_STROBE_READ        16
+#define OCOTP_TIMING_STROBE_READ_MASK   0x003F0000
+#define OCOTP_TIMING_RELAX              12
+#define OCOTP_TIMING_RELAX_MASK         0x0000F000
+#define OCOTP_TIMING_STROBE_PROG        0
+#define OCOTP_TIMING_STROBE_PROG_MASK   0x00000FFF
+
+#define OCOTP_READ_CTRL_READ_FUSE    0x1
+
+#define VF610_OCOTP_TIMEOUT          100000
+
+#define BF(value, field)             (((value) << field) & field##_MASK)
+
+#define DEF_RELAX                    20
+
+struct vf610_ocotp_dev {
+	void __iomem *base;
+	struct clk *clk;
+	struct device *dev;
+	struct resource *res;
+	struct regmap *regmap;
+	struct nvmem_device *nvmem;
+};
+
+static int ocotp_timing;
+
+static u8 valid_fuse_addr[] = {
+	0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
+	0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
+	0x3D, 0x3E, 0x3F
+};
+
+static int vf610_ocotp_wait_busy(void __iomem *base)
+{
+	int timeout = VF610_OCOTP_TIMEOUT;
+
+	while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
+		udelay(10);
+
+	if (!timeout) {
+		writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
+		return -ETIMEDOUT;
+	}
+
+	udelay(10);
+
+	return 0;
+}
+
+static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
+{
+	u32 clk_rate;
+	u32 relax, strobe_read, strobe_prog;
+	u32 timing;
+
+	clk_rate = clk_get_rate(ocotp_dev->clk);
+
+	relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
+	strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
+	strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
+
+	timing = BF(relax, OCOTP_TIMING_RELAX);
+	timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
+	timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
+
+	return timing;
+}
+
+static int vf610_ocotp_set_timing(void __iomem *base, int timing)
+{
+	writel(timing, base + OCOTP_TIMING);
+
+	return 0;
+}
+
+static int vf610_ocotp_write(void *context, const void *data, size_t count)
+{
+	return 0;
+}
+
+static int vf610_ocotp_read(void *context,
+					const void *offset, size_t reg_size,
+					void *val, size_t val_size)
+{
+	void __iomem *ocotp_base = context;
+	u32 *buf = val;
+	u32 reg;
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
+		vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
+		ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
+		if (ret)
+			return ret;
+
+		reg = readl(ocotp_base + OCOTP_CTRL_REG);
+		reg &= ~OCOTP_CTRL_ADDR_MASK;
+		reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
+		reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
+		writel(reg, ocotp_base + OCOTP_CTRL_REG);
+
+		writel(OCOTP_READ_CTRL_READ_FUSE,
+			ocotp_base + OCOTP_READ_CTRL_REG);
+		ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
+		if (ret)
+			return ret;
+
+		if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
+			pr_err("Error reading from fuse address %d\n",
+				valid_fuse_addr[i]);
+			writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
+		}
+
+		*buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
+	}
+
+	return 0;
+}
+
+static struct regmap_bus vf610_ocotp_bus = {
+	.read = vf610_ocotp_read,
+	.write = vf610_ocotp_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static struct regmap_config ocotp_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static struct nvmem_config ocotp_config = {
+	.name = "ocotp",
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id ocotp_of_match[] = {
+	{ .compatible = "fsl,vf610-ocotp",},
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, ocotp_of_match);
+
+static int vf610_ocotp_remove(struct platform_device *pdev)
+{
+	struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
+
+	return nvmem_unregister(ocotp_dev->nvmem);
+}
+
+static int vf610_ocotp_probe(struct platform_device *pdev)
+{
+	struct vf610_ocotp_dev *ocotp_dev;
+
+	ocotp_dev = devm_kzalloc(&pdev->dev,
+			sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
+	if (!ocotp_dev)
+		return -ENOMEM;
+
+	ocotp_dev->dev = &pdev->dev;
+
+	ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
+	if (IS_ERR(ocotp_dev->base))
+		return PTR_ERR(ocotp_dev->base);
+
+	ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
+	if (IS_ERR(ocotp_dev->clk)) {
+		dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
+			PTR_ERR(ocotp_dev->clk));
+		return PTR_ERR(ocotp_dev->clk);
+	}
+
+	ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
+
+	ocotp_dev->regmap = devm_regmap_init(ocotp_dev->dev,
+		&vf610_ocotp_bus, ocotp_dev->base, &ocotp_regmap_config);
+	if (IS_ERR(ocotp_dev->regmap)) {
+		dev_err(ocotp_dev->dev, "regmap init failed\n");
+		return PTR_ERR(ocotp_dev->regmap);
+	}
+	ocotp_config.dev = ocotp_dev->dev;
+
+	ocotp_dev->nvmem = nvmem_register(&ocotp_config);
+	if (IS_ERR(ocotp_dev->nvmem))
+		return PTR_ERR(ocotp_dev->nvmem);
+
+	platform_set_drvdata(pdev, ocotp_dev);
+
+	ocotp_timing = vf610_ocotp_calculate_timing(ocotp_dev);
+
+	return 0;
+}
+
+static struct platform_driver vf610_ocotp_driver = {
+	.probe = vf610_ocotp_probe,
+	.remove = vf610_ocotp_remove,
+	.driver = {
+		.name = "vf610-ocotp",
+		.of_match_table = ocotp_of_match,
+	},
+};
+module_platform_driver(vf610_ocotp_driver);
+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
+MODULE_DESCRIPTION("Vybrid OCOTP driver");
+MODULE_LICENSE("GPL v2");
-- 
2.4.4

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Sanchayan Maity]

The patch adds support for the On Chip One Time Programmable Peripheral
(OCOTP) on the Vybrid platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/nvmem/Kconfig       |  10 ++
 drivers/nvmem/Makefile      |   2 +
 drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 262 insertions(+)
 create mode 100644 drivers/nvmem/vf610-ocotp.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 17f1a57..84c830d 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
 	  This driver can also be built as a module. If so, the module
 	  will be called eeprom-sunxi-sid.
 
+config NVMEM_VF610_OCOTP
+	tristate "VF610 SoCs OCOTP support"
+	depends on SOC_VF610
+	help
+	  This is a driver for the 'OCOTP' available on various Vybrid
+	  devices.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-vf610-ocotp.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index cc46791..a9ed113 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
 nvmem_qfprom-y			:= qfprom.o
 obj-$(CONFIG_NVMEM_SUNXI_SID)	+= nvmem-sunxi-sid.o
 nvmem-sunxi-sid-y		:= sunxi-sid.o
+obj-$(CONFIG_NVMEM_VF610_OCOTP)	+= nvmem-vf610-ocotp.o
+nvmem-vf610-ocotp-y		:= vf610-ocotp.o
diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
new file mode 100644
index 0000000..b7a782c
--- /dev/null
+++ b/drivers/nvmem/vf610-ocotp.c
@@ -0,0 +1,250 @@
+/*
+ * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/nvmem-provider.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* OCOTP Register Offsets */
+#define OCOTP_CTRL_REG          0x00
+#define OCOTP_CTRL_SET          0x04
+#define OCOTP_CTRL_CLR          0x08
+#define OCOTP_TIMING            0x10
+#define OCOTP_DATA              0x20
+#define OCOTP_READ_CTRL_REG     0x30
+#define OCOTP_READ_FUSE_DATA    0x40
+
+/* OCOTP Register bits and masks */
+#define OCOTP_CTRL_WR_UNLOCK         16
+#define OCOTP_CTRL_WR_UNLOCK_KEY     0x3E77
+#define OCOTP_CTRL_WR_UNLOCK_MASK    0xFFFF0000
+#define OCOTP_CTRL_ADDR              0
+#define OCOTP_CTRL_ADDR_MASK         0x7F
+#define OCOTP_CTRL_RELOAD_SHADOWS    (0x1 << 10)
+#define OCOTP_CTRL_ERROR             (0x1 << 9)
+#define OCOTP_CTRL_BUSY              (0x1 << 8)
+
+#define OCOTP_TIMING_STROBE_READ        16
+#define OCOTP_TIMING_STROBE_READ_MASK   0x003F0000
+#define OCOTP_TIMING_RELAX              12
+#define OCOTP_TIMING_RELAX_MASK         0x0000F000
+#define OCOTP_TIMING_STROBE_PROG        0
+#define OCOTP_TIMING_STROBE_PROG_MASK   0x00000FFF
+
+#define OCOTP_READ_CTRL_READ_FUSE    0x1
+
+#define VF610_OCOTP_TIMEOUT          100000
+
+#define BF(value, field)             (((value) << field) & field##_MASK)
+
+#define DEF_RELAX                    20
+
+struct vf610_ocotp_dev {
+	void __iomem *base;
+	struct clk *clk;
+	struct device *dev;
+	struct resource *res;
+	struct regmap *regmap;
+	struct nvmem_device *nvmem;
+};
+
+static int ocotp_timing;
+
+static u8 valid_fuse_addr[] = {
+	0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
+	0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
+	0x3D, 0x3E, 0x3F
+};
+
+static int vf610_ocotp_wait_busy(void __iomem *base)
+{
+	int timeout = VF610_OCOTP_TIMEOUT;
+
+	while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
+		udelay(10);
+
+	if (!timeout) {
+		writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
+		return -ETIMEDOUT;
+	}
+
+	udelay(10);
+
+	return 0;
+}
+
+static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
+{
+	u32 clk_rate;
+	u32 relax, strobe_read, strobe_prog;
+	u32 timing;
+
+	clk_rate = clk_get_rate(ocotp_dev->clk);
+
+	relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
+	strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
+	strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
+
+	timing = BF(relax, OCOTP_TIMING_RELAX);
+	timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
+	timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
+
+	return timing;
+}
+
+static int vf610_ocotp_set_timing(void __iomem *base, int timing)
+{
+	writel(timing, base + OCOTP_TIMING);
+
+	return 0;
+}
+
+static int vf610_ocotp_write(void *context, const void *data, size_t count)
+{
+	return 0;
+}
+
+static int vf610_ocotp_read(void *context,
+					const void *offset, size_t reg_size,
+					void *val, size_t val_size)
+{
+	void __iomem *ocotp_base = context;
+	u32 *buf = val;
+	u32 reg;
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
+		vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
+		ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
+		if (ret)
+			return ret;
+
+		reg = readl(ocotp_base + OCOTP_CTRL_REG);
+		reg &= ~OCOTP_CTRL_ADDR_MASK;
+		reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
+		reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
+		writel(reg, ocotp_base + OCOTP_CTRL_REG);
+
+		writel(OCOTP_READ_CTRL_READ_FUSE,
+			ocotp_base + OCOTP_READ_CTRL_REG);
+		ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
+		if (ret)
+			return ret;
+
+		if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
+			pr_err("Error reading from fuse address %d\n",
+				valid_fuse_addr[i]);
+			writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
+		}
+
+		*buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
+	}
+
+	return 0;
+}
+
+static struct regmap_bus vf610_ocotp_bus = {
+	.read = vf610_ocotp_read,
+	.write = vf610_ocotp_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static struct regmap_config ocotp_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static struct nvmem_config ocotp_config = {
+	.name = "ocotp",
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id ocotp_of_match[] = {
+	{ .compatible = "fsl,vf610-ocotp",},
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, ocotp_of_match);
+
+static int vf610_ocotp_remove(struct platform_device *pdev)
+{
+	struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
+
+	return nvmem_unregister(ocotp_dev->nvmem);
+}
+
+static int vf610_ocotp_probe(struct platform_device *pdev)
+{
+	struct vf610_ocotp_dev *ocotp_dev;
+
+	ocotp_dev = devm_kzalloc(&pdev->dev,
+			sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
+	if (!ocotp_dev)
+		return -ENOMEM;
+
+	ocotp_dev->dev = &pdev->dev;
+
+	ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
+	if (IS_ERR(ocotp_dev->base))
+		return PTR_ERR(ocotp_dev->base);
+
+	ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
+	if (IS_ERR(ocotp_dev->clk)) {
+		dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
+			PTR_ERR(ocotp_dev->clk));
+		return PTR_ERR(ocotp_dev->clk);
+	}
+
+	ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
+
+	ocotp_dev->regmap = devm_regmap_init(ocotp_dev->dev,
+		&vf610_ocotp_bus, ocotp_dev->base, &ocotp_regmap_config);
+	if (IS_ERR(ocotp_dev->regmap)) {
+		dev_err(ocotp_dev->dev, "regmap init failed\n");
+		return PTR_ERR(ocotp_dev->regmap);
+	}
+	ocotp_config.dev = ocotp_dev->dev;
+
+	ocotp_dev->nvmem = nvmem_register(&ocotp_config);
+	if (IS_ERR(ocotp_dev->nvmem))
+		return PTR_ERR(ocotp_dev->nvmem);
+
+	platform_set_drvdata(pdev, ocotp_dev);
+
+	ocotp_timing = vf610_ocotp_calculate_timing(ocotp_dev);
+
+	return 0;
+}
+
+static struct platform_driver vf610_ocotp_driver = {
+	.probe = vf610_ocotp_probe,
+	.remove = vf610_ocotp_remove,
+	.driver = {
+		.name = "vf610-ocotp",
+		.of_match_table = ocotp_of_match,
+	},
+};
+module_platform_driver(vf610_ocotp_driver);
+MODULE_AUTHOR("Sanchayan Maity <sanchayan.maity@toradex.com>");
+MODULE_DESCRIPTION("Vybrid OCOTP driver");
+MODULE_LICENSE("GPL v2");
-- 
2.4.4

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> Sanchayan Maity <maitysanchayan@gmail.com> hat am 29. Juni 2015 um 13:22
> geschrieben:
>
>
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) on the Vybrid platform.

please provide a changelog in your cover letter and a new version after changes.

I think it's important to note that the driver only support read-only access.

>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/nvmem/Kconfig | 10 ++
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 262 insertions(+)
> create mode 100644 drivers/nvmem/vf610-ocotp.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..84c830d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> This driver can also be built as a module. If so, the module
> will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> + tristate "VF610 SoCs OCOTP support"
> + depends on SOC_VF610
> + help
> + This is a driver for the 'OCOTP' available on various Vybrid
> + devices.

I don't know much about Vybrid. But this driver is specific for VF610, isn't it?

> +
> + This driver can also be built as a module. If so, the module
> + will be called nvmem-vf610-ocotp.
> +
> endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index cc46791..a9ed113 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM) += nvmem_qfprom.o
> nvmem_qfprom-y := qfprom.o
> obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem-sunxi-sid.o
> nvmem-sunxi-sid-y := sunxi-sid.o
> +obj-$(CONFIG_NVMEM_VF610_OCOTP) += nvmem-vf610-ocotp.o
> +nvmem-vf610-ocotp-y := vf610-ocotp.o
> diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> new file mode 100644
> index 0000000..b7a782c
> --- /dev/null
> +++ b/drivers/nvmem/vf610-ocotp.c
> @@ -0,0 +1,250 @@
> +/*
> + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* OCOTP Register Offsets */
> +#define OCOTP_CTRL_REG 0x00
> +#define OCOTP_CTRL_SET 0x04
> +#define OCOTP_CTRL_CLR 0x08
> +#define OCOTP_TIMING 0x10
> +#define OCOTP_DATA 0x20
> +#define OCOTP_READ_CTRL_REG 0x30
> +#define OCOTP_READ_FUSE_DATA 0x40
> +
> +/* OCOTP Register bits and masks */
> +#define OCOTP_CTRL_WR_UNLOCK 16
> +#define OCOTP_CTRL_WR_UNLOCK_KEY 0x3E77
> +#define OCOTP_CTRL_WR_UNLOCK_MASK 0xFFFF0000
> +#define OCOTP_CTRL_ADDR 0
> +#define OCOTP_CTRL_ADDR_MASK 0x7F
> +#define OCOTP_CTRL_RELOAD_SHADOWS (0x1 << 10)
> +#define OCOTP_CTRL_ERROR (0x1 << 9)
> +#define OCOTP_CTRL_BUSY (0x1 << 8)

You can use the BIT() macro here.

> +
> +#define OCOTP_TIMING_STROBE_READ 16
> +#define OCOTP_TIMING_STROBE_READ_MASK 0x003F0000
> +#define OCOTP_TIMING_RELAX 12
> +#define OCOTP_TIMING_RELAX_MASK 0x0000F000
> +#define OCOTP_TIMING_STROBE_PROG 0
> +#define OCOTP_TIMING_STROBE_PROG_MASK 0x00000FFF
> +
> +#define OCOTP_READ_CTRL_READ_FUSE 0x1
> +
> +#define VF610_OCOTP_TIMEOUT 100000
> +
> +#define BF(value, field) (((value) << field) & field##_MASK)
> +
> +#define DEF_RELAX 20
> +
> +struct vf610_ocotp_dev {
> + void __iomem *base;
> + struct clk *clk;
> + struct device *dev;
> + struct resource *res;
> + struct regmap *regmap;
> + struct nvmem_device *nvmem;
> +};

Some member of this struct seems unnecessary to me. Please
remove the unused one.

> +
> +static int ocotp_timing;

How about storing the timings in struct above?

> +
> +static u8 valid_fuse_addr[] = {
> + 0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
> + 0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
> + 0x3D, 0x3E, 0x3F
> +};

const?

> +
> +static int vf610_ocotp_wait_busy(void __iomem *base)
> +{
> + int timeout = VF610_OCOTP_TIMEOUT;
> +
> + while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
> + udelay(10);
> +
> + if (!timeout) {
> + writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
> + return -ETIMEDOUT;
> + }
> +
> + udelay(10);
> +
> + return 0;
> +}
> +
> +static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
> +{
> + u32 clk_rate;
> + u32 relax, strobe_read, strobe_prog;
> + u32 timing;
> +
> + clk_rate = clk_get_rate(ocotp_dev->clk);

If clk_get_rate() fails, then we should abort with a warning.

> +
> + relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> + strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
> + strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;

A little explanation of the calculation would be helpful.

> +
> + timing = BF(relax, OCOTP_TIMING_RELAX);
> + timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
> + timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
> +
> + return timing;
> +}
> +
> +static int vf610_ocotp_set_timing(void __iomem *base, int timing)
> +{
> + writel(timing, base + OCOTP_TIMING);
> +
> + return 0;
> +}
> +
> +static int vf610_ocotp_write(void *context, const void *data, size_t count)
> +{
> + return 0;
> +}
> +
> +static int vf610_ocotp_read(void *context,
> + const void *offset, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + void __iomem *ocotp_base = context;
> + u32 *buf = val;
> + u32 reg;
> + int ret;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> + if (ret)
> + return ret;

Is it really necessary to set the timing in the loop, instead before?

> +
> + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> + reg &= ~OCOTP_CTRL_ADDR_MASK;
> + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> +
> + writel(OCOTP_READ_CTRL_READ_FUSE,
> + ocotp_base + OCOTP_READ_CTRL_REG);
> + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> + if (ret)
> + return ret;
> +
> + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> + pr_err("Error reading from fuse address %d\n",
> + valid_fuse_addr[i]);

You could use dev_err() when storing vf610_ocotp_dev in the context.

> + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);

Shouldn't the function abort here?

> + }
> +
> + *buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
> + }
> +
> + return 0;
> +}
> +
> +static struct regmap_bus vf610_ocotp_bus = {
> + .read = vf610_ocotp_read,
> + .write = vf610_ocotp_write,
> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +static struct regmap_config ocotp_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static struct nvmem_config ocotp_config = {
> + .name = "ocotp",
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id ocotp_of_match[] = {
> + { .compatible = "fsl,vf610-ocotp",},
> + {/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> +
> +static int vf610_ocotp_remove(struct platform_device *pdev)
> +{
> + struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
> +
> + return nvmem_unregister(ocotp_dev->nvmem);
> +}
> +
> +static int vf610_ocotp_probe(struct platform_device *pdev)
> +{
> + struct vf610_ocotp_dev *ocotp_dev;
> +
> + ocotp_dev = devm_kzalloc(&pdev->dev,
> + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> + if (!ocotp_dev)
> + return -ENOMEM;
> +
> + ocotp_dev->dev = &pdev->dev;
> +
> + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> + if (IS_ERR(ocotp_dev->base))
> + return PTR_ERR(ocotp_dev->base);
> +
> + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> + if (IS_ERR(ocotp_dev->clk)) {
> + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> + PTR_ERR(ocotp_dev->clk));
> + return PTR_ERR(ocotp_dev->clk);
> + }
> +
> + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;

Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access could be
uninitializied.

Regards
Stefan

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> Sanchayan Maity <maitysanchayan@gmail.com> hat am 29. Juni 2015 um 13:22
> geschrieben:
>
>
> The patch adds support for the On Chip One Time Programmable Peripheral
> (OCOTP) on the Vybrid platform.

please provide a changelog in your cover letter and a new version after changes.

I think it's important to note that the driver only support read-only access.

>
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
> drivers/nvmem/Kconfig | 10 ++
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 262 insertions(+)
> create mode 100644 drivers/nvmem/vf610-ocotp.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 17f1a57..84c830d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> This driver can also be built as a module. If so, the module
> will be called eeprom-sunxi-sid.
>
> +config NVMEM_VF610_OCOTP
> + tristate "VF610 SoCs OCOTP support"
> + depends on SOC_VF610
> + help
> + This is a driver for the 'OCOTP' available on various Vybrid
> + devices.

I don't know much about Vybrid. But this driver is specific for VF610, isn't it?

> +
> + This driver can also be built as a module. If so, the module
> + will be called nvmem-vf610-ocotp.
> +
> endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index cc46791..a9ed113 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM) += nvmem_qfprom.o
> nvmem_qfprom-y := qfprom.o
> obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem-sunxi-sid.o
> nvmem-sunxi-sid-y := sunxi-sid.o
> +obj-$(CONFIG_NVMEM_VF610_OCOTP) += nvmem-vf610-ocotp.o
> +nvmem-vf610-ocotp-y := vf610-ocotp.o
> diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> new file mode 100644
> index 0000000..b7a782c
> --- /dev/null
> +++ b/drivers/nvmem/vf610-ocotp.c
> @@ -0,0 +1,250 @@
> +/*
> + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* OCOTP Register Offsets */
> +#define OCOTP_CTRL_REG 0x00
> +#define OCOTP_CTRL_SET 0x04
> +#define OCOTP_CTRL_CLR 0x08
> +#define OCOTP_TIMING 0x10
> +#define OCOTP_DATA 0x20
> +#define OCOTP_READ_CTRL_REG 0x30
> +#define OCOTP_READ_FUSE_DATA 0x40
> +
> +/* OCOTP Register bits and masks */
> +#define OCOTP_CTRL_WR_UNLOCK 16
> +#define OCOTP_CTRL_WR_UNLOCK_KEY 0x3E77
> +#define OCOTP_CTRL_WR_UNLOCK_MASK 0xFFFF0000
> +#define OCOTP_CTRL_ADDR 0
> +#define OCOTP_CTRL_ADDR_MASK 0x7F
> +#define OCOTP_CTRL_RELOAD_SHADOWS (0x1 << 10)
> +#define OCOTP_CTRL_ERROR (0x1 << 9)
> +#define OCOTP_CTRL_BUSY (0x1 << 8)

You can use the BIT() macro here.

> +
> +#define OCOTP_TIMING_STROBE_READ 16
> +#define OCOTP_TIMING_STROBE_READ_MASK 0x003F0000
> +#define OCOTP_TIMING_RELAX 12
> +#define OCOTP_TIMING_RELAX_MASK 0x0000F000
> +#define OCOTP_TIMING_STROBE_PROG 0
> +#define OCOTP_TIMING_STROBE_PROG_MASK 0x00000FFF
> +
> +#define OCOTP_READ_CTRL_READ_FUSE 0x1
> +
> +#define VF610_OCOTP_TIMEOUT 100000
> +
> +#define BF(value, field) (((value) << field) & field##_MASK)
> +
> +#define DEF_RELAX 20
> +
> +struct vf610_ocotp_dev {
> + void __iomem *base;
> + struct clk *clk;
> + struct device *dev;
> + struct resource *res;
> + struct regmap *regmap;
> + struct nvmem_device *nvmem;
> +};

Some member of this struct seems unnecessary to me. Please
remove the unused one.

> +
> +static int ocotp_timing;

How about storing the timings in struct above?

> +
> +static u8 valid_fuse_addr[] = {
> + 0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
> + 0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
> + 0x3D, 0x3E, 0x3F
> +};

const?

> +
> +static int vf610_ocotp_wait_busy(void __iomem *base)
> +{
> + int timeout = VF610_OCOTP_TIMEOUT;
> +
> + while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
> + udelay(10);
> +
> + if (!timeout) {
> + writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
> + return -ETIMEDOUT;
> + }
> +
> + udelay(10);
> +
> + return 0;
> +}
> +
> +static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
> +{
> + u32 clk_rate;
> + u32 relax, strobe_read, strobe_prog;
> + u32 timing;
> +
> + clk_rate = clk_get_rate(ocotp_dev->clk);

If clk_get_rate() fails, then we should abort with a warning.

> +
> + relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> + strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
> + strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;

A little explanation of the calculation would be helpful.

> +
> + timing = BF(relax, OCOTP_TIMING_RELAX);
> + timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
> + timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
> +
> + return timing;
> +}
> +
> +static int vf610_ocotp_set_timing(void __iomem *base, int timing)
> +{
> + writel(timing, base + OCOTP_TIMING);
> +
> + return 0;
> +}
> +
> +static int vf610_ocotp_write(void *context, const void *data, size_t count)
> +{
> + return 0;
> +}
> +
> +static int vf610_ocotp_read(void *context,
> + const void *offset, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + void __iomem *ocotp_base = context;
> + u32 *buf = val;
> + u32 reg;
> + int ret;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> + if (ret)
> + return ret;

Is it really necessary to set the timing in the loop, instead before?

> +
> + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> + reg &= ~OCOTP_CTRL_ADDR_MASK;
> + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> +
> + writel(OCOTP_READ_CTRL_READ_FUSE,
> + ocotp_base + OCOTP_READ_CTRL_REG);
> + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> + if (ret)
> + return ret;
> +
> + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> + pr_err("Error reading from fuse address %d\n",
> + valid_fuse_addr[i]);

You could use dev_err() when storing vf610_ocotp_dev in the context.

> + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);

Shouldn't the function abort here?

> + }
> +
> + *buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
> + }
> +
> + return 0;
> +}
> +
> +static struct regmap_bus vf610_ocotp_bus = {
> + .read = vf610_ocotp_read,
> + .write = vf610_ocotp_write,
> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> +};
> +
> +static struct regmap_config ocotp_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static struct nvmem_config ocotp_config = {
> + .name = "ocotp",
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id ocotp_of_match[] = {
> + { .compatible = "fsl,vf610-ocotp",},
> + {/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> +
> +static int vf610_ocotp_remove(struct platform_device *pdev)
> +{
> + struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
> +
> + return nvmem_unregister(ocotp_dev->nvmem);
> +}
> +
> +static int vf610_ocotp_probe(struct platform_device *pdev)
> +{
> + struct vf610_ocotp_dev *ocotp_dev;
> +
> + ocotp_dev = devm_kzalloc(&pdev->dev,
> + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> + if (!ocotp_dev)
> + return -ENOMEM;
> +
> + ocotp_dev->dev = &pdev->dev;
> +
> + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> + if (IS_ERR(ocotp_dev->base))
> + return PTR_ERR(ocotp_dev->base);
> +
> + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> + if (IS_ERR(ocotp_dev->clk)) {
> + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> + PTR_ERR(ocotp_dev->clk));
> + return PTR_ERR(ocotp_dev->clk);
> + }
> +
> + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;

Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access could be
uninitializied.

Regards
Stefan

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan]

Hello Stefan,

On 15-07-06 12:16:34, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > Sanchayan Maity <maitysanchayan@gmail.com> hat am 29. Juni 2015 um 13:22
> > geschrieben:
> >
> >
> > The patch adds support for the On Chip One Time Programmable Peripheral
> > (OCOTP) on the Vybrid platform.
> 
> please provide a changelog in your cover letter and a new version after changes.
> 
> I think it's important to note that the driver only support read-only access.

Ok. Will mention this explicitly in the next version.

> 
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/nvmem/Kconfig | 10 ++
> > drivers/nvmem/Makefile | 2 +
> > drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 262 insertions(+)
> > create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > index 17f1a57..84c830d 100644
> > --- a/drivers/nvmem/Kconfig
> > +++ b/drivers/nvmem/Kconfig
> > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > This driver can also be built as a module. If so, the module
> > will be called eeprom-sunxi-sid.
> >
> > +config NVMEM_VF610_OCOTP
> > + tristate "VF610 SoCs OCOTP support"
> > + depends on SOC_VF610
> > + help
> > + This is a driver for the 'OCOTP' available on various Vybrid
> > + devices.
> 
> I don't know much about Vybrid. But this driver is specific for VF610, isn't it?

Sorry. I only checked on VF50 and VF61. Will check if is it available with the
other Vybrid devices and change if it is not so.

> 
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called nvmem-vf610-ocotp.
> > +
> > endif
> > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> > index cc46791..a9ed113 100644
> > --- a/drivers/nvmem/Makefile
> > +++ b/drivers/nvmem/Makefile
> > @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM) += nvmem_qfprom.o
> > nvmem_qfprom-y := qfprom.o
> > obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem-sunxi-sid.o
> > nvmem-sunxi-sid-y := sunxi-sid.o
> > +obj-$(CONFIG_NVMEM_VF610_OCOTP) += nvmem-vf610-ocotp.o
> > +nvmem-vf610-ocotp-y := vf610-ocotp.o
> > diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> > new file mode 100644
> > index 0000000..b7a782c
> > --- /dev/null
> > +++ b/drivers/nvmem/vf610-ocotp.c
> > @@ -0,0 +1,250 @@
> > +/*
> > + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +/* OCOTP Register Offsets */
> > +#define OCOTP_CTRL_REG 0x00
> > +#define OCOTP_CTRL_SET 0x04
> > +#define OCOTP_CTRL_CLR 0x08
> > +#define OCOTP_TIMING 0x10
> > +#define OCOTP_DATA 0x20
> > +#define OCOTP_READ_CTRL_REG 0x30
> > +#define OCOTP_READ_FUSE_DATA 0x40
> > +
> > +/* OCOTP Register bits and masks */
> > +#define OCOTP_CTRL_WR_UNLOCK 16
> > +#define OCOTP_CTRL_WR_UNLOCK_KEY 0x3E77
> > +#define OCOTP_CTRL_WR_UNLOCK_MASK 0xFFFF0000
> > +#define OCOTP_CTRL_ADDR 0
> > +#define OCOTP_CTRL_ADDR_MASK 0x7F
> > +#define OCOTP_CTRL_RELOAD_SHADOWS (0x1 << 10)
> > +#define OCOTP_CTRL_ERROR (0x1 << 9)
> > +#define OCOTP_CTRL_BUSY (0x1 << 8)
> 
> You can use the BIT() macro here.

Ok.

> 
> > +
> > +#define OCOTP_TIMING_STROBE_READ 16
> > +#define OCOTP_TIMING_STROBE_READ_MASK 0x003F0000
> > +#define OCOTP_TIMING_RELAX 12
> > +#define OCOTP_TIMING_RELAX_MASK 0x0000F000
> > +#define OCOTP_TIMING_STROBE_PROG 0
> > +#define OCOTP_TIMING_STROBE_PROG_MASK 0x00000FFF
> > +
> > +#define OCOTP_READ_CTRL_READ_FUSE 0x1
> > +
> > +#define VF610_OCOTP_TIMEOUT 100000
> > +
> > +#define BF(value, field) (((value) << field) & field##_MASK)
> > +
> > +#define DEF_RELAX 20
> > +
> > +struct vf610_ocotp_dev {
> > + void __iomem *base;
> > + struct clk *clk;
> > + struct device *dev;
> > + struct resource *res;
> > + struct regmap *regmap;
> > + struct nvmem_device *nvmem;
> > +};
> 
> Some member of this struct seems unnecessary to me. Please
> remove the unused one.
> 
> > +
> > +static int ocotp_timing;
> 
> How about storing the timings in struct above?

Ok.

> 
> > +
> > +static u8 valid_fuse_addr[] = {
> > + 0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
> > + 0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
> > + 0x3D, 0x3E, 0x3F
> > +};
> 
> const?

Right.

> 
> > +
> > +static int vf610_ocotp_wait_busy(void __iomem *base)
> > +{
> > + int timeout = VF610_OCOTP_TIMEOUT;
> > +
> > + while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
> > + udelay(10);
> > +
> > + if (!timeout) {
> > + writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + udelay(10);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
> > +{
> > + u32 clk_rate;
> > + u32 relax, strobe_read, strobe_prog;
> > + u32 timing;
> > +
> > + clk_rate = clk_get_rate(ocotp_dev->clk);
> 
> If clk_get_rate() fails, then we should abort with a warning.

Correct. Will fix this.

> 
> > +
> > + relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> > + strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
> > + strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
> 
> A little explanation of the calculation would be helpful.

Ok. Will add the relevant comments.

For now just to be clear, this is taken from from Barebox and as per Section 35.3.2.
DEF_RELAX is defined to be 20 as tRELAX is expected to be greated than 16.2ns with
the first primary equation being

tRELAX = tHP_PG = (OCOTP_TIMING[RELAX]+1)/ipg_frequency > 16.2ns

The rest of the equations follow the relax value defined.

> 
> > +
> > + timing = BF(relax, OCOTP_TIMING_RELAX);
> > + timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
> > + timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
> > +
> > + return timing;
> > +}
> > +
> > +static int vf610_ocotp_set_timing(void __iomem *base, int timing)
> > +{
> > + writel(timing, base + OCOTP_TIMING);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_write(void *context, const void *data, size_t count)
> > +{
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_read(void *context,
> > + const void *offset, size_t reg_size,
> > + void *val, size_t val_size)
> > +{
> > + void __iomem *ocotp_base = context;
> > + u32 *buf = val;
> > + u32 reg;
> > + int ret;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > + if (ret)
> > + return ret;
> 
> Is it really necessary to set the timing in the loop, instead before?

I will test it once. From my understanding of 35.3.1.5 I thought the timing
needs to explicitly programmed on every read. Perhaps I took it too literally.

> 
> > +
> > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > +
> > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > + ocotp_base + OCOTP_READ_CTRL_REG);
> > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > + if (ret)
> > + return ret;
> > +
> > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > + pr_err("Error reading from fuse address %d\n",
> > + valid_fuse_addr[i]);
> 
> You could use dev_err() when storing vf610_ocotp_dev in the context.

Ok.

> 
> > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> 
> Shouldn't the function abort here?

I am not sure about what usage I should follow here. I went for an
explicit error message and since 0xBADABADA is expected to be returned
on a read locked shadow register, the user would get the above for this
particular fuse address and the rest can still be valid since the TRM
mentions "subsequent reads to unlocked shadow locations will still work
successfully." So I did not abort on the error. Should I?

> 
> > + }
> > +
> > + *buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct regmap_bus vf610_ocotp_bus = {
> > + .read = vf610_ocotp_read,
> > + .write = vf610_ocotp_write,
> > + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> > + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> > +};
> > +
> > +static struct regmap_config ocotp_regmap_config = {
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > +};
> > +
> > +static struct nvmem_config ocotp_config = {
> > + .name = "ocotp",
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id ocotp_of_match[] = {
> > + { .compatible = "fsl,vf610-ocotp",},
> > + {/* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> > +
> > +static int vf610_ocotp_remove(struct platform_device *pdev)
> > +{
> > + struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
> > +
> > + return nvmem_unregister(ocotp_dev->nvmem);
> > +}
> > +
> > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > +{
> > + struct vf610_ocotp_dev *ocotp_dev;
> > +
> > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > + if (!ocotp_dev)
> > + return -ENOMEM;
> > +
> > + ocotp_dev->dev = &pdev->dev;
> > +
> > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > + if (IS_ERR(ocotp_dev->base))
> > + return PTR_ERR(ocotp_dev->base);
> > +
> > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > + if (IS_ERR(ocotp_dev->clk)) {
> > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > + PTR_ERR(ocotp_dev->clk));
> > + return PTR_ERR(ocotp_dev->clk);
> > + }
> > +
> > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> 
> Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access could be
> uninitializied.

Sorry I did not exactly get you here. The intention behind using the valid_fuse_addr
is to allow reading only from valid fuse addresses and avoid reading from all other
locations as per the Fuse map address table 35-1.

Thanks for the review.

Regards,
Sanchayan.

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan at gmail.com]

Hello Stefan,

On 15-07-06 12:16:34, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > Sanchayan Maity <maitysanchayan@gmail.com> hat am 29. Juni 2015 um 13:22
> > geschrieben:
> >
> >
> > The patch adds support for the On Chip One Time Programmable Peripheral
> > (OCOTP) on the Vybrid platform.
> 
> please provide a changelog in your cover letter and a new version after changes.
> 
> I think it's important to note that the driver only support read-only access.

Ok. Will mention this explicitly in the next version.

> 
> >
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> > drivers/nvmem/Kconfig | 10 ++
> > drivers/nvmem/Makefile | 2 +
> > drivers/nvmem/vf610-ocotp.c | 250 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 262 insertions(+)
> > create mode 100644 drivers/nvmem/vf610-ocotp.c
> >
> > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > index 17f1a57..84c830d 100644
> > --- a/drivers/nvmem/Kconfig
> > +++ b/drivers/nvmem/Kconfig
> > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > This driver can also be built as a module. If so, the module
> > will be called eeprom-sunxi-sid.
> >
> > +config NVMEM_VF610_OCOTP
> > + tristate "VF610 SoCs OCOTP support"
> > + depends on SOC_VF610
> > + help
> > + This is a driver for the 'OCOTP' available on various Vybrid
> > + devices.
> 
> I don't know much about Vybrid. But this driver is specific for VF610, isn't it?

Sorry. I only checked on VF50 and VF61. Will check if is it available with the
other Vybrid devices and change if it is not so.

> 
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called nvmem-vf610-ocotp.
> > +
> > endif
> > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> > index cc46791..a9ed113 100644
> > --- a/drivers/nvmem/Makefile
> > +++ b/drivers/nvmem/Makefile
> > @@ -11,3 +11,5 @@ obj-$(CONFIG_QCOM_QFPROM) += nvmem_qfprom.o
> > nvmem_qfprom-y := qfprom.o
> > obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem-sunxi-sid.o
> > nvmem-sunxi-sid-y := sunxi-sid.o
> > +obj-$(CONFIG_NVMEM_VF610_OCOTP) += nvmem-vf610-ocotp.o
> > +nvmem-vf610-ocotp-y := vf610-ocotp.o
> > diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
> > new file mode 100644
> > index 0000000..b7a782c
> > --- /dev/null
> > +++ b/drivers/nvmem/vf610-ocotp.c
> > @@ -0,0 +1,250 @@
> > +/*
> > + * Copyright (C) 2015 Sanchayan Maity <sanchayan.maity@toradex.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 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/nvmem-provider.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +/* OCOTP Register Offsets */
> > +#define OCOTP_CTRL_REG 0x00
> > +#define OCOTP_CTRL_SET 0x04
> > +#define OCOTP_CTRL_CLR 0x08
> > +#define OCOTP_TIMING 0x10
> > +#define OCOTP_DATA 0x20
> > +#define OCOTP_READ_CTRL_REG 0x30
> > +#define OCOTP_READ_FUSE_DATA 0x40
> > +
> > +/* OCOTP Register bits and masks */
> > +#define OCOTP_CTRL_WR_UNLOCK 16
> > +#define OCOTP_CTRL_WR_UNLOCK_KEY 0x3E77
> > +#define OCOTP_CTRL_WR_UNLOCK_MASK 0xFFFF0000
> > +#define OCOTP_CTRL_ADDR 0
> > +#define OCOTP_CTRL_ADDR_MASK 0x7F
> > +#define OCOTP_CTRL_RELOAD_SHADOWS (0x1 << 10)
> > +#define OCOTP_CTRL_ERROR (0x1 << 9)
> > +#define OCOTP_CTRL_BUSY (0x1 << 8)
> 
> You can use the BIT() macro here.

Ok.

> 
> > +
> > +#define OCOTP_TIMING_STROBE_READ 16
> > +#define OCOTP_TIMING_STROBE_READ_MASK 0x003F0000
> > +#define OCOTP_TIMING_RELAX 12
> > +#define OCOTP_TIMING_RELAX_MASK 0x0000F000
> > +#define OCOTP_TIMING_STROBE_PROG 0
> > +#define OCOTP_TIMING_STROBE_PROG_MASK 0x00000FFF
> > +
> > +#define OCOTP_READ_CTRL_READ_FUSE 0x1
> > +
> > +#define VF610_OCOTP_TIMEOUT 100000
> > +
> > +#define BF(value, field) (((value) << field) & field##_MASK)
> > +
> > +#define DEF_RELAX 20
> > +
> > +struct vf610_ocotp_dev {
> > + void __iomem *base;
> > + struct clk *clk;
> > + struct device *dev;
> > + struct resource *res;
> > + struct regmap *regmap;
> > + struct nvmem_device *nvmem;
> > +};
> 
> Some member of this struct seems unnecessary to me. Please
> remove the unused one.
> 
> > +
> > +static int ocotp_timing;
> 
> How about storing the timings in struct above?

Ok.

> 
> > +
> > +static u8 valid_fuse_addr[] = {
> > + 0x00, 0x01, 0x02, 0x04, 0x0F, 0x20, 0x21, 0x22, 0x23, 0x24,
> > + 0x25, 0x26, 0x27, 0x28, 0x2F, 0x38, 0x39, 0x3A, 0x3B, 0x3C,
> > + 0x3D, 0x3E, 0x3F
> > +};
> 
> const?

Right.

> 
> > +
> > +static int vf610_ocotp_wait_busy(void __iomem *base)
> > +{
> > + int timeout = VF610_OCOTP_TIMEOUT;
> > +
> > + while ((readl(base) & OCOTP_CTRL_BUSY) && --timeout)
> > + udelay(10);
> > +
> > + if (!timeout) {
> > + writel(OCOTP_CTRL_ERROR, base + OCOTP_CTRL_CLR);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + udelay(10);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_calculate_timing(struct vf610_ocotp_dev *ocotp_dev)
> > +{
> > + u32 clk_rate;
> > + u32 relax, strobe_read, strobe_prog;
> > + u32 timing;
> > +
> > + clk_rate = clk_get_rate(ocotp_dev->clk);
> 
> If clk_get_rate() fails, then we should abort with a warning.

Correct. Will fix this.

> 
> > +
> > + relax = clk_rate / (1000000000 / DEF_RELAX) - 1;
> > + strobe_prog = clk_rate / (1000000000 / 10000) + 2 * (DEF_RELAX + 1) - 1;
> > + strobe_read = clk_rate / (1000000000 / 40) + 2 * (DEF_RELAX + 1) - 1;
> 
> A little explanation of the calculation would be helpful.

Ok. Will add the relevant comments.

For now just to be clear, this is taken from from Barebox and as per Section 35.3.2.
DEF_RELAX is defined to be 20 as tRELAX is expected to be greated than 16.2ns with
the first primary equation being

tRELAX = tHP_PG = (OCOTP_TIMING[RELAX]+1)/ipg_frequency > 16.2ns

The rest of the equations follow the relax value defined.

> 
> > +
> > + timing = BF(relax, OCOTP_TIMING_RELAX);
> > + timing |= BF(strobe_read, OCOTP_TIMING_STROBE_READ);
> > + timing |= BF(strobe_prog, OCOTP_TIMING_STROBE_PROG);
> > +
> > + return timing;
> > +}
> > +
> > +static int vf610_ocotp_set_timing(void __iomem *base, int timing)
> > +{
> > + writel(timing, base + OCOTP_TIMING);
> > +
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_write(void *context, const void *data, size_t count)
> > +{
> > + return 0;
> > +}
> > +
> > +static int vf610_ocotp_read(void *context,
> > + const void *offset, size_t reg_size,
> > + void *val, size_t val_size)
> > +{
> > + void __iomem *ocotp_base = context;
> > + u32 *buf = val;
> > + u32 reg;
> > + int ret;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > + if (ret)
> > + return ret;
> 
> Is it really necessary to set the timing in the loop, instead before?

I will test it once. From my understanding of 35.3.1.5 I thought the timing
needs to explicitly programmed on every read. Perhaps I took it too literally.

> 
> > +
> > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > +
> > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > + ocotp_base + OCOTP_READ_CTRL_REG);
> > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > + if (ret)
> > + return ret;
> > +
> > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > + pr_err("Error reading from fuse address %d\n",
> > + valid_fuse_addr[i]);
> 
> You could use dev_err() when storing vf610_ocotp_dev in the context.

Ok.

> 
> > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> 
> Shouldn't the function abort here?

I am not sure about what usage I should follow here. I went for an
explicit error message and since 0xBADABADA is expected to be returned
on a read locked shadow register, the user would get the above for this
particular fuse address and the rest can still be valid since the TRM
mentions "subsequent reads to unlocked shadow locations will still work
successfully." So I did not abort on the error. Should I?

> 
> > + }
> > +
> > + *buf++ = readl(ocotp_base + OCOTP_READ_FUSE_DATA);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct regmap_bus vf610_ocotp_bus = {
> > + .read = vf610_ocotp_read,
> > + .write = vf610_ocotp_write,
> > + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> > + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> > +};
> > +
> > +static struct regmap_config ocotp_regmap_config = {
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > +};
> > +
> > +static struct nvmem_config ocotp_config = {
> > + .name = "ocotp",
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id ocotp_of_match[] = {
> > + { .compatible = "fsl,vf610-ocotp",},
> > + {/* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, ocotp_of_match);
> > +
> > +static int vf610_ocotp_remove(struct platform_device *pdev)
> > +{
> > + struct vf610_ocotp_dev *ocotp_dev = platform_get_drvdata(pdev);
> > +
> > + return nvmem_unregister(ocotp_dev->nvmem);
> > +}
> > +
> > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > +{
> > + struct vf610_ocotp_dev *ocotp_dev;
> > +
> > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > + if (!ocotp_dev)
> > + return -ENOMEM;
> > +
> > + ocotp_dev->dev = &pdev->dev;
> > +
> > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > + if (IS_ERR(ocotp_dev->base))
> > + return PTR_ERR(ocotp_dev->base);
> > +
> > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > + if (IS_ERR(ocotp_dev->clk)) {
> > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > + PTR_ERR(ocotp_dev->clk));
> > + return PTR_ERR(ocotp_dev->clk);
> > + }
> > +
> > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> 
> Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access could be
> uninitializied.

Sorry I did not exactly get you here. The intention behind using the valid_fuse_addr
is to allow reading only from valid fuse addresses and avoid reading from all other
locations as per the Fuse map address table 35-1.

Thanks for the review.

Regards,
Sanchayan.

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> maitysanchayan@gmail.com hat am 7. Juli 2015 um 07:19 geschrieben:
>
>
> [...]
> > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > > index 17f1a57..84c830d 100644
> > > --- a/drivers/nvmem/Kconfig
> > > +++ b/drivers/nvmem/Kconfig
> > > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > > This driver can also be built as a module. If so, the module
> > > will be called eeprom-sunxi-sid.
> > >
> > > +config NVMEM_VF610_OCOTP
> > > + tristate "VF610 SoCs OCOTP support"
> > > + depends on SOC_VF610
> > > + help
> > > + This is a driver for the 'OCOTP' available on various Vybrid
> > > + devices.
> >
> > I don't know much about Vybrid. But this driver is specific for VF610, isn't
> > it?
>
> Sorry. I only checked on VF50 and VF61. Will check if is it available with the
> other Vybrid devices and change if it is not so.

no problem. If you spend time in testing your driver with different devices, you
could mention
this in your patch description. The naming VF610 suggests that the driver is
very specific.
That confuses me a little bit.

>
> >
> > > +
> > > +
> > > +static int vf610_ocotp_write(void *context, const void *data, size_t
> > > count)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int vf610_ocotp_read(void *context,
> > > + const void *offset, size_t reg_size,
> > > + void *val, size_t val_size)
> > > +{
> > > + void __iomem *ocotp_base = context;
> > > + u32 *buf = val;
> > > + u32 reg;
> > > + int ret;
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > + if (ret)
> > > + return ret;
> >
> > Is it really necessary to set the timing in the loop, instead before?
>
> I will test it once. From my understanding of 35.3.1.5 I thought the timing
> needs to explicitly programmed on every read. Perhaps I took it too literally.

It was only a question. If barebox does the same, it should be okay.

>
> >
> > > +
> > > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > > +
> > > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > > + ocotp_base + OCOTP_READ_CTRL_REG);
> > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > > + pr_err("Error reading from fuse address %d\n",
> > > + valid_fuse_addr[i]);
> >
> > You could use dev_err() when storing vf610_ocotp_dev in the context.
>
> Ok.
>
> >
> > > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> >
> > Shouldn't the function abort here?
>
> I am not sure about what usage I should follow here. I went for an
> explicit error message and since 0xBADABADA is expected to be returned
> on a read locked shadow register, the user would get the above for this
> particular fuse address and the rest can still be valid since the TRM
> mentions "subsequent reads to unlocked shadow locations will still work
> successfully." So I did not abort on the error. Should I?

In case you don't want to abort, a comment about the 0xBADABADA behavior would
be helpful.

>
> >
> > > +
> > > +
> > > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > > +{
> > > + struct vf610_ocotp_dev *ocotp_dev;
> > > +
> > > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > > + if (!ocotp_dev)
> > > + return -ENOMEM;
> > > +
> > > + ocotp_dev->dev = &pdev->dev;
> > > +
> > > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > > + if (IS_ERR(ocotp_dev->base))
> > > + return PTR_ERR(ocotp_dev->base);
> > > +
> > > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > > + if (IS_ERR(ocotp_dev->clk)) {
> > > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > > + PTR_ERR(ocotp_dev->clk));
> > > + return PTR_ERR(ocotp_dev->clk);
> > > + }
> > > +
> > > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> >
> > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > could be
> > uninitializied.
>
> Sorry I did not exactly get you here. The intention behind using the
> valid_fuse_addr
> is to allow reading only from valid fuse addresses and avoid reading from all
> other
> locations as per the Fuse map address table 35-1.

Yes, i got your intention. But from my unterstand the read function should fill
up
the complete buffer with defined values. My MXS OCOTP driver have the same
problem
and fill up the invalid registers with zero.

Regards
Stefan

>
> Thanks for the review.
>
> Regards,
> Sanchayan.

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> maitysanchayan at gmail.com hat am 7. Juli 2015 um 07:19 geschrieben:
>
>
> [...]
> > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > > index 17f1a57..84c830d 100644
> > > --- a/drivers/nvmem/Kconfig
> > > +++ b/drivers/nvmem/Kconfig
> > > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > > This driver can also be built as a module. If so, the module
> > > will be called eeprom-sunxi-sid.
> > >
> > > +config NVMEM_VF610_OCOTP
> > > + tristate "VF610 SoCs OCOTP support"
> > > + depends on SOC_VF610
> > > + help
> > > + This is a driver for the 'OCOTP' available on various Vybrid
> > > + devices.
> >
> > I don't know much about Vybrid. But this driver is specific for VF610, isn't
> > it?
>
> Sorry. I only checked on VF50 and VF61. Will check if is it available with the
> other Vybrid devices and change if it is not so.

no problem. If you spend time in testing your driver with different devices, you
could mention
this in your patch description. The naming VF610 suggests that the driver is
very specific.
That confuses me a little bit.

>
> >
> > > +
> > > +
> > > +static int vf610_ocotp_write(void *context, const void *data, size_t
> > > count)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int vf610_ocotp_read(void *context,
> > > + const void *offset, size_t reg_size,
> > > + void *val, size_t val_size)
> > > +{
> > > + void __iomem *ocotp_base = context;
> > > + u32 *buf = val;
> > > + u32 reg;
> > > + int ret;
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > + if (ret)
> > > + return ret;
> >
> > Is it really necessary to set the timing in the loop, instead before?
>
> I will test it once. From my understanding of 35.3.1.5 I thought the timing
> needs to explicitly programmed on every read. Perhaps I took it too literally.

It was only a question. If barebox does the same, it should be okay.

>
> >
> > > +
> > > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > > +
> > > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > > + ocotp_base + OCOTP_READ_CTRL_REG);
> > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > > + pr_err("Error reading from fuse address %d\n",
> > > + valid_fuse_addr[i]);
> >
> > You could use dev_err() when storing vf610_ocotp_dev in the context.
>
> Ok.
>
> >
> > > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> >
> > Shouldn't the function abort here?
>
> I am not sure about what usage I should follow here. I went for an
> explicit error message and since 0xBADABADA is expected to be returned
> on a read locked shadow register, the user would get the above for this
> particular fuse address and the rest can still be valid since the TRM
> mentions "subsequent reads to unlocked shadow locations will still work
> successfully." So I did not abort on the error. Should I?

In case you don't want to abort, a comment about the 0xBADABADA behavior would
be helpful.

>
> >
> > > +
> > > +
> > > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > > +{
> > > + struct vf610_ocotp_dev *ocotp_dev;
> > > +
> > > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > > + if (!ocotp_dev)
> > > + return -ENOMEM;
> > > +
> > > + ocotp_dev->dev = &pdev->dev;
> > > +
> > > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > > + if (IS_ERR(ocotp_dev->base))
> > > + return PTR_ERR(ocotp_dev->base);
> > > +
> > > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > > + if (IS_ERR(ocotp_dev->clk)) {
> > > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > > + PTR_ERR(ocotp_dev->clk));
> > > + return PTR_ERR(ocotp_dev->clk);
> > > + }
> > > +
> > > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> >
> > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > could be
> > uninitializied.
>
> Sorry I did not exactly get you here. The intention behind using the
> valid_fuse_addr
> is to allow reading only from valid fuse addresses and avoid reading from all
> other
> locations as per the Fuse map address table 35-1.

Yes, i got your intention. But from my unterstand the read function should fill
up
the complete buffer with defined values. My MXS OCOTP driver have the same
problem
and fill up the invalid registers with zero.

Regards
Stefan

>
> Thanks for the review.
>
> Regards,
> Sanchayan.

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan]

Hi Stefan,

On 15-07-07 14:49:06, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > maitysanchayan@gmail.com hat am 7. Juli 2015 um 07:19 geschrieben:
> >
> >
> > [...]
> > > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > > > index 17f1a57..84c830d 100644
> > > > --- a/drivers/nvmem/Kconfig
> > > > +++ b/drivers/nvmem/Kconfig
> > > > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > > > This driver can also be built as a module. If so, the module
> > > > will be called eeprom-sunxi-sid.
> > > >
> > > > +config NVMEM_VF610_OCOTP
> > > > + tristate "VF610 SoCs OCOTP support"
> > > > + depends on SOC_VF610
> > > > + help
> > > > + This is a driver for the 'OCOTP' available on various Vybrid
> > > > + devices.
> > >
> > > I don't know much about Vybrid. But this driver is specific for VF610, isn't
> > > it?
> >
> > Sorry. I only checked on VF50 and VF61. Will check if is it available with the
> > other Vybrid devices and change if it is not so.
> 
> no problem. If you spend time in testing your driver with different devices, you
> could mention
> this in your patch description. The naming VF610 suggests that the driver is
> very specific.
> That confuses me a little bit.

Sorry about that. Will add the necessary information in the next respin for sure.

> 
> >
> > >
> > > > +
> > > > +
> > > > +static int vf610_ocotp_write(void *context, const void *data, size_t
> > > > count)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int vf610_ocotp_read(void *context,
> > > > + const void *offset, size_t reg_size,
> > > > + void *val, size_t val_size)
> > > > +{
> > > > + void __iomem *ocotp_base = context;
> > > > + u32 *buf = val;
> > > > + u32 reg;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > > > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > Is it really necessary to set the timing in the loop, instead before?
> >
> > I will test it once. From my understanding of 35.3.1.5 I thought the timing
> > needs to explicitly programmed on every read. Perhaps I took it too literally.
> 
> It was only a question. If barebox does the same, it should be okay.
>

Yes, the code logic is completely similar to barebox except for changes like
how timeout is handled here in the wait busy function.

> >
> > >
> > > > +
> > > > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > > > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > > > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > > > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > > > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > > > +
> > > > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > > > + ocotp_base + OCOTP_READ_CTRL_REG);
> > > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > > > + pr_err("Error reading from fuse address %d\n",
> > > > + valid_fuse_addr[i]);
> > >
> > > You could use dev_err() when storing vf610_ocotp_dev in the context.
> >
> > Ok.
> >
> > >
> > > > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> > >
> > > Shouldn't the function abort here?
> >
> > I am not sure about what usage I should follow here. I went for an
> > explicit error message and since 0xBADABADA is expected to be returned
> > on a read locked shadow register, the user would get the above for this
> > particular fuse address and the rest can still be valid since the TRM
> > mentions "subsequent reads to unlocked shadow locations will still work
> > successfully." So I did not abort on the error. Should I?
> 
> In case you don't want to abort, a comment about the 0xBADABADA behavior would
> be helpful.

Ok. Will add the same.

> 
> >
> > >
> > > > +
> > > > +
> > > > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct vf610_ocotp_dev *ocotp_dev;
> > > > +
> > > > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > > > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > > > + if (!ocotp_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + ocotp_dev->dev = &pdev->dev;
> > > > +
> > > > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > > > + if (IS_ERR(ocotp_dev->base))
> > > > + return PTR_ERR(ocotp_dev->base);
> > > > +
> > > > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > > > + if (IS_ERR(ocotp_dev->clk)) {
> > > > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > > > + PTR_ERR(ocotp_dev->clk));
> > > > + return PTR_ERR(ocotp_dev->clk);
> > > > + }
> > > > +
> > > > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> > >
> > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > could be
> > > uninitializied.
> >
> > Sorry I did not exactly get you here. The intention behind using the
> > valid_fuse_addr
> > is to allow reading only from valid fuse addresses and avoid reading from all
> > other
> > locations as per the Fuse map address table 35-1.
> 
> Yes, i got your intention. But from my unterstand the read function should fill
> up
> the complete buffer with defined values. My MXS OCOTP driver have the same
> problem
> and fill up the invalid registers with zero.

I must admit I had not thought of that. Thats a valid point. I dont know at the
moment however what would be the correct approach here. Perhaps filling locations
other than the valid fuse addresses as per the fuse map table with 0xBADABADA is
the right thing? Anyways that's what is going to be returned if we were to read
a location which is read locked or reserved.

However since the fuse address and base address mapping is not exactly linear,
this would require some additional logic than the simple thing I did. Also OCOTP
requires the fuse address to be written. May be using the nvmem consumer DT property
gets something?! Not sure atm.

- Sanchayan.

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan at gmail.com]

Hi Stefan,

On 15-07-07 14:49:06, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > maitysanchayan at gmail.com hat am 7. Juli 2015 um 07:19 geschrieben:
> >
> >
> > [...]
> > > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > > > index 17f1a57..84c830d 100644
> > > > --- a/drivers/nvmem/Kconfig
> > > > +++ b/drivers/nvmem/Kconfig
> > > > @@ -33,4 +33,14 @@ config NVMEM_SUNXI_SID
> > > > This driver can also be built as a module. If so, the module
> > > > will be called eeprom-sunxi-sid.
> > > >
> > > > +config NVMEM_VF610_OCOTP
> > > > + tristate "VF610 SoCs OCOTP support"
> > > > + depends on SOC_VF610
> > > > + help
> > > > + This is a driver for the 'OCOTP' available on various Vybrid
> > > > + devices.
> > >
> > > I don't know much about Vybrid. But this driver is specific for VF610, isn't
> > > it?
> >
> > Sorry. I only checked on VF50 and VF61. Will check if is it available with the
> > other Vybrid devices and change if it is not so.
> 
> no problem. If you spend time in testing your driver with different devices, you
> could mention
> this in your patch description. The naming VF610 suggests that the driver is
> very specific.
> That confuses me a little bit.

Sorry about that. Will add the necessary information in the next respin for sure.

> 
> >
> > >
> > > > +
> > > > +
> > > > +static int vf610_ocotp_write(void *context, const void *data, size_t
> > > > count)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int vf610_ocotp_read(void *context,
> > > > + const void *offset, size_t reg_size,
> > > > + void *val, size_t val_size)
> > > > +{
> > > > + void __iomem *ocotp_base = context;
> > > > + u32 *buf = val;
> > > > + u32 reg;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(valid_fuse_addr); i++) {
> > > > + vf610_ocotp_set_timing(ocotp_base, ocotp_timing);
> > > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > Is it really necessary to set the timing in the loop, instead before?
> >
> > I will test it once. From my understanding of 35.3.1.5 I thought the timing
> > needs to explicitly programmed on every read. Perhaps I took it too literally.
> 
> It was only a question. If barebox does the same, it should be okay.
>

Yes, the code logic is completely similar to barebox except for changes like
how timeout is handled here in the wait busy function.

> >
> > >
> > > > +
> > > > + reg = readl(ocotp_base + OCOTP_CTRL_REG);
> > > > + reg &= ~OCOTP_CTRL_ADDR_MASK;
> > > > + reg &= ~OCOTP_CTRL_WR_UNLOCK_MASK;
> > > > + reg |= BF(valid_fuse_addr[i], OCOTP_CTRL_ADDR);
> > > > + writel(reg, ocotp_base + OCOTP_CTRL_REG);
> > > > +
> > > > + writel(OCOTP_READ_CTRL_READ_FUSE,
> > > > + ocotp_base + OCOTP_READ_CTRL_REG);
> > > > + ret = vf610_ocotp_wait_busy(ocotp_base + OCOTP_CTRL_REG);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + if (readl(ocotp_base) & OCOTP_CTRL_ERROR) {
> > > > + pr_err("Error reading from fuse address %d\n",
> > > > + valid_fuse_addr[i]);
> > >
> > > You could use dev_err() when storing vf610_ocotp_dev in the context.
> >
> > Ok.
> >
> > >
> > > > + writel(OCOTP_CTRL_ERROR, ocotp_base + OCOTP_CTRL_CLR);
> > >
> > > Shouldn't the function abort here?
> >
> > I am not sure about what usage I should follow here. I went for an
> > explicit error message and since 0xBADABADA is expected to be returned
> > on a read locked shadow register, the user would get the above for this
> > particular fuse address and the rest can still be valid since the TRM
> > mentions "subsequent reads to unlocked shadow locations will still work
> > successfully." So I did not abort on the error. Should I?
> 
> In case you don't want to abort, a comment about the 0xBADABADA behavior would
> be helpful.

Ok. Will add the same.

> 
> >
> > >
> > > > +
> > > > +
> > > > +static int vf610_ocotp_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct vf610_ocotp_dev *ocotp_dev;
> > > > +
> > > > + ocotp_dev = devm_kzalloc(&pdev->dev,
> > > > + sizeof(struct vf610_ocotp_dev), GFP_KERNEL);
> > > > + if (!ocotp_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + ocotp_dev->dev = &pdev->dev;
> > > > +
> > > > + ocotp_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + ocotp_dev->base = devm_ioremap_resource(ocotp_dev->dev, ocotp_dev->res);
> > > > + if (IS_ERR(ocotp_dev->base))
> > > > + return PTR_ERR(ocotp_dev->base);
> > > > +
> > > > + ocotp_dev->clk = devm_clk_get(ocotp_dev->dev, "ocotp");
> > > > + if (IS_ERR(ocotp_dev->clk)) {
> > > > + dev_err(ocotp_dev->dev, "failed getting clock, err = %ld\n",
> > > > + PTR_ERR(ocotp_dev->clk));
> > > > + return PTR_ERR(ocotp_dev->clk);
> > > > + }
> > > > +
> > > > + ocotp_regmap_config.max_register = resource_size(ocotp_dev->res) - 1;
> > >
> > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the rest
> > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > could be
> > > uninitializied.
> >
> > Sorry I did not exactly get you here. The intention behind using the
> > valid_fuse_addr
> > is to allow reading only from valid fuse addresses and avoid reading from all
> > other
> > locations as per the Fuse map address table 35-1.
> 
> Yes, i got your intention. But from my unterstand the read function should fill
> up
> the complete buffer with defined values. My MXS OCOTP driver have the same
> problem
> and fill up the invalid registers with zero.

I must admit I had not thought of that. Thats a valid point. I dont know at the
moment however what would be the correct approach here. Perhaps filling locations
other than the valid fuse addresses as per the fuse map table with 0xBADABADA is
the right thing? Anyways that's what is going to be returned if we were to read
a location which is read locked or reserved.

However since the fuse address and base address mapping is not exactly linear,
this would require some additional logic than the simple thing I did. Also OCOTP
requires the fuse address to be written. May be using the nvmem consumer DT property
gets something?! Not sure atm.

- Sanchayan.

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> maitysanchayan@gmail.com hat am 8. Juli 2015 um 07:39 geschrieben:
>
>
> [...]
> > > >
> > > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the
> > > > rest
> > > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > > could be
> > > > uninitializied.
> > >
> > > Sorry I did not exactly get you here. The intention behind using the
> > > valid_fuse_addr
> > > is to allow reading only from valid fuse addresses and avoid reading from
> > > all
> > > other
> > > locations as per the Fuse map address table 35-1.
> >
> > Yes, i got your intention. But from my unterstand the read function should
> > fill
> > up
> > the complete buffer with defined values. My MXS OCOTP driver have the same
> > problem
> > and fill up the invalid registers with zero.
>
> I must admit I had not thought of that. Thats a valid point. I dont know at
> the
> moment however what would be the correct approach here. Perhaps filling
> locations
> other than the valid fuse addresses as per the fuse map table with 0xBADABADA
> is
> the right thing? Anyways that's what is going to be returned if we were to
> read
> a location which is read locked or reserved.

since we are operating in userspace the behavior shouldn't be specific to the
device.
It would be good when all drivers behave the same. But i think it would be much
better
as the nvmem framework take care of it and preinit the buffer with a defined
value.

>
> However since the fuse address and base address mapping is not exactly linear,
> this would require some additional logic than the simple thing I did. 

I defined a regmap_access_table for valid read registers in my case. But i think
in your case
an implementation of the readable_reg callback in the regmap_config would be
more elegant.

You can validate your implementation under /sys/kernel/debug/regmap/

Sorry, i'm a newbie to regmap.

Stefan

> Also OCOTP requires the fuse address to be written. May be using the nvmem
> consumer DT property
> gets something?! Not sure atm.
>
> - Sanchayan.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[Stefan Wahren]

Hi Sanchayan,

> maitysanchayan at gmail.com hat am 8. Juli 2015 um 07:39 geschrieben:
>
>
> [...]
> > > >
> > > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the
> > > > rest
> > > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > > could be
> > > > uninitializied.
> > >
> > > Sorry I did not exactly get you here. The intention behind using the
> > > valid_fuse_addr
> > > is to allow reading only from valid fuse addresses and avoid reading from
> > > all
> > > other
> > > locations as per the Fuse map address table 35-1.
> >
> > Yes, i got your intention. But from my unterstand the read function should
> > fill
> > up
> > the complete buffer with defined values. My MXS OCOTP driver have the same
> > problem
> > and fill up the invalid registers with zero.
>
> I must admit I had not thought of that. Thats a valid point. I dont know at
> the
> moment however what would be the correct approach here. Perhaps filling
> locations
> other than the valid fuse addresses as per the fuse map table with 0xBADABADA
> is
> the right thing? Anyways that's what is going to be returned if we were to
> read
> a location which is read locked or reserved.

since we are operating in userspace the behavior shouldn't be specific to the
device.
It would be good when all drivers behave the same. But i think it would be much
better
as the nvmem framework take care of it and preinit the buffer with a defined
value.

>
> However since the fuse address and base address mapping is not exactly linear,
> this would require some additional logic than the simple thing I did. 

I defined a regmap_access_table for valid read registers in my case. But i think
in your case
an implementation of the readable_reg callback in the regmap_config would be
more elegant.

You can validate your implementation under /sys/kernel/debug/regmap/

Sorry, i'm a newbie to regmap.

Stefan

> Also OCOTP requires the fuse address to be written. May be using the nvmem
> consumer DT property
> gets something?! Not sure atm.
>
> - Sanchayan.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan]

Hello Stefan,

On 15-07-08 22:55:23, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > maitysanchayan@gmail.com hat am 8. Juli 2015 um 07:39 geschrieben:
> >
> >
> > [...]
> > > > >
> > > > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the
> > > > > rest
> > > > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > > > could be
> > > > > uninitializied.
> > > >
> > > > Sorry I did not exactly get you here. The intention behind using the
> > > > valid_fuse_addr
> > > > is to allow reading only from valid fuse addresses and avoid reading from
> > > > all
> > > > other
> > > > locations as per the Fuse map address table 35-1.
> > >
> > > Yes, i got your intention. But from my unterstand the read function should
> > > fill
> > > up
> > > the complete buffer with defined values. My MXS OCOTP driver have the same
> > > problem
> > > and fill up the invalid registers with zero.
> >
> > I must admit I had not thought of that. Thats a valid point. I dont know at
> > the
> > moment however what would be the correct approach here. Perhaps filling
> > locations
> > other than the valid fuse addresses as per the fuse map table with 0xBADABADA
> > is
> > the right thing? Anyways that's what is going to be returned if we were to
> > read
> > a location which is read locked or reserved.
> 
> since we are operating in userspace the behavior shouldn't be specific to the
> device.
> It would be good when all drivers behave the same. But i think it would be much
> better
> as the nvmem framework take care of it and preinit the buffer with a defined
> value.

There is a v7 now. Yet to give that a try or look at it.

> 
> >
> > However since the fuse address and base address mapping is not exactly linear,
> > this would require some additional logic than the simple thing I did. 
> 
> I defined a regmap_access_table for valid read registers in my case. But i think
> in your case
> an implementation of the readable_reg callback in the regmap_config would be
> more elegant.
> 
> You can validate your implementation under /sys/kernel/debug/regmap/

Thank you for the input. I will have a look at it and give it a try.

> 
> Sorry, i'm a newbie to regmap.

Same here as well :)

Regards,
Sanchayan.

[RFC PATCH v6 3/3] drivers: nvmem: Add Vybrid OCOTP support

[maitysanchayan at gmail.com]

Hello Stefan,

On 15-07-08 22:55:23, Stefan Wahren wrote:
> Hi Sanchayan,
> 
> > maitysanchayan at gmail.com hat am 8. Juli 2015 um 07:39 geschrieben:
> >
> >
> > [...]
> > > > >
> > > > > Looking at valid_fuse_addr shows me 0x3F as last valid register. So the
> > > > > rest
> > > > > of the buffer ( 0xD00 - sizeof(valid_fuse_addr) ) in case of raw access
> > > > > could be
> > > > > uninitializied.
> > > >
> > > > Sorry I did not exactly get you here. The intention behind using the
> > > > valid_fuse_addr
> > > > is to allow reading only from valid fuse addresses and avoid reading from
> > > > all
> > > > other
> > > > locations as per the Fuse map address table 35-1.
> > >
> > > Yes, i got your intention. But from my unterstand the read function should
> > > fill
> > > up
> > > the complete buffer with defined values. My MXS OCOTP driver have the same
> > > problem
> > > and fill up the invalid registers with zero.
> >
> > I must admit I had not thought of that. Thats a valid point. I dont know at
> > the
> > moment however what would be the correct approach here. Perhaps filling
> > locations
> > other than the valid fuse addresses as per the fuse map table with 0xBADABADA
> > is
> > the right thing? Anyways that's what is going to be returned if we were to
> > read
> > a location which is read locked or reserved.
> 
> since we are operating in userspace the behavior shouldn't be specific to the
> device.
> It would be good when all drivers behave the same. But i think it would be much
> better
> as the nvmem framework take care of it and preinit the buffer with a defined
> value.

There is a v7 now. Yet to give that a try or look at it.

> 
> >
> > However since the fuse address and base address mapping is not exactly linear,
> > this would require some additional logic than the simple thing I did. 
> 
> I defined a regmap_access_table for valid read registers in my case. But i think
> in your case
> an implementation of the readable_reg callback in the regmap_config would be
> more elegant.
> 
> You can validate your implementation under /sys/kernel/debug/regmap/

Thank you for the input. I will have a look at it and give it a try.

> 
> Sorry, i'm a newbie to regmap.

Same here as well :)

Regards,
Sanchayan.