diff mbox

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

Message ID b4a52f6c229bdabbfe9d21c03cb04593dde33ec4.1435066190.git.maitysanchayan@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sanchayan June 23, 2015, 1:44 p.m. UTC 
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

Comments

Srinivas Kandagatla June 23, 2015, 7:03 p.m. UTC | #1 
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
Stefan Wahren June 23, 2015, 7:31 p.m. UTC | #2 
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
Sanchayan June 24, 2015, 5:19 a.m. UTC | #3 
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.
Sanchayan June 24, 2015, 5:25 a.m. UTC | #4 
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.
Maxime Ripard June 24, 2015, 8:35 a.m. UTC | #5 
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
Srinivas Kandagatla June 24, 2015, 9:34 a.m. UTC | #6 
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
>
Srinivas Kandagatla June 24, 2015, 9:37 a.m. UTC | #7 
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
Sanchayan June 24, 2015, 9:44 a.m. UTC | #8 
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.
Stefan Wahren June 24, 2015, 9:56 a.m. UTC | #9 
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
Sanchayan June 24, 2015, 10:45 a.m. UTC | #10 
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.
Stefan Agner June 24, 2015, 11:20 a.m. UTC | #11 
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
Stefan Wahren June 24, 2015, 11:52 a.m. UTC | #12 
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
Sanchayan June 24, 2015, 12:05 p.m. UTC | #13 
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.
Maxime Ripard June 25, 2015, 5:49 p.m. UTC | #14 
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
Sanchayan June 29, 2015, 11:22 a.m. UTC | #15 
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
diff mbox

Patch

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");