by Sanchayan

[permalink] [raw]

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

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 <[email protected]>
> >---
> > 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 <[email protected]>
> >+ *
> >+ * 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 <[email protected]>");
> >+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.

2015-06-24 08:40:10

[permalink] [raw]

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

Hello Stefan,

On 15-07-08 22:55:23, Stefan Wahren wrote:
> Hi Sanchayan,
>
> > [email protected] 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.