Date:   Sat, 22 Feb 2020 18:16:50 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        Mathieu Malaterre <malat@...ian.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paulburton@...nel.org>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-mips@...r.kernel.org, letux-kernel@...nphoenux.org,
        kernel@...a-handheld.com
Subject: Re: [PATCH v5 1/6] nvmem: add driver for JZ4780 efuse

Hi Paul,

> Am 22.02.2020 um 16:38 schrieb Paul Cercueil <paul@...pouillou.net>:
> 
> Hi Nikolaus,
> 
> 
> Le sam., févr. 22, 2020 at 11:25, H. Nikolaus Schaller <hns@...delico.com> a écrit :
>> From: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
>> This patch brings support for the JZ4780 efuse. Currently it only exposes
>> a read only access to the entire 8K bits efuse memory and nvmem cells.
>> Tested-by: Mathieu Malaterre <malat@...ian.org>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
>> Signed-off-by: Mathieu Malaterre <malat@...ian.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> ---
>> drivers/nvmem/Kconfig        |  10 ++
>> drivers/nvmem/Makefile       |   2 +
>> drivers/nvmem/jz4780-efuse.c | 229 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 241 insertions(+)
>> create mode 100644 drivers/nvmem/jz4780-efuse.c
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 35efab1ba8d9..8143e6e1dd82 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -55,6 +55,16 @@ config NVMEM_IMX_OCOTP_SCU
>> 	  This is a driver for the SCU On-Chip OTP Controller (OCOTP)
>> 	  available on i.MX8 SoCs.
>> +config JZ4780_EFUSE
>> +	tristate "JZ4780 EFUSE Memory Support"
>> +	depends on MACH_INGENIC || COMPILE_TEST
>> +	depends on HAS_IOMEM
> 
> You're using regmap, so you need to select REGMAP_MMIO here.

Ok.

> 
>> +	help
>> +	  Say Y here to include support for JZ4780 efuse memory found on
>> +	  all JZ4780 SoC based devices.
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called nvmem_jz4780_efuse.
>> +
>> config NVMEM_LPC18XX_EEPROM
>> 	tristate "NXP LPC18XX EEPROM Memory Support"
>> 	depends on ARCH_LPC18XX || COMPILE_TEST
>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>> index 6b466cd1427b..65a268d17807 100644
>> --- a/drivers/nvmem/Makefile
>> +++ b/drivers/nvmem/Makefile
>> @@ -18,6 +18,8 @@ obj-$(CONFIG_NVMEM_IMX_OCOTP)	+= nvmem-imx-ocotp.o
>> nvmem-imx-ocotp-y		:= imx-ocotp.o
>> obj-$(CONFIG_NVMEM_IMX_OCOTP_SCU)	+= nvmem-imx-ocotp-scu.o
>> nvmem-imx-ocotp-scu-y		:= imx-ocotp-scu.o
>> +obj-$(CONFIG_JZ4780_EFUSE)		+= nvmem_jz4780_efuse.o
>> +nvmem_jz4780_efuse-y		:= jz4780-efuse.o
>> obj-$(CONFIG_NVMEM_LPC18XX_EEPROM)	+= nvmem_lpc18xx_eeprom.o
>> nvmem_lpc18xx_eeprom-y	:= lpc18xx_eeprom.o
>> obj-$(CONFIG_NVMEM_LPC18XX_OTP)	+= nvmem_lpc18xx_otp.o
>> diff --git a/drivers/nvmem/jz4780-efuse.c b/drivers/nvmem/jz4780-efuse.c
>> new file mode 100644
>> index 000000000000..08b63de0e9cc
>> --- /dev/null
>> +++ b/drivers/nvmem/jz4780-efuse.c
>> @@ -0,0 +1,229 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * JZ4780 EFUSE Memory Support driver
>> + *
>> + * Copyright (c) 2017 PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
>> + * Copyright (c) 2020 H. Nikolaus Schaller <hns@...delico.com>
>> + */
>> +
>> +/*
>> + * Currently supports JZ4780 efuse which has 8K programmable bit.
>> + * Efuse is separated into seven segments as below:
>> + *
>> + * -----------------------------------------------------------------------
>> + * | 64 bit | 128 bit | 128 bit | 3520 bit | 8 bit | 2296 bit | 2048 bit |
>> + * -----------------------------------------------------------------------
>> + *
>> + * The rom itself is accessed using a 9 bit address line and an 8 word wide bus
>> + * which reads/writes based on strobes. The strobe is configured in the config
>> + * register and is based on number of cycles of the bus clock.
>> + *
>> + * Driver supports read only as the writes are done in the Factory.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/timer.h>
>> +
>> +#define JZ_EFUCTRL		(0x0)	/* Control Register */
>> +#define JZ_EFUCFG		(0x4)	/* Configure Register*/
>> +#define JZ_EFUSTATE		(0x8)	/* Status Register */
>> +#define JZ_EFUDATA(n)		(0xC + (n) * 4)
>> +
>> +#define EFUCTRL_ADDR_MASK	0x3FF
>> +#define EFUCTRL_ADDR_SHIFT	21
>> +#define EFUCTRL_LEN_MASK	0x1F
>> +#define EFUCTRL_LEN_SHIFT	16
>> +#define EFUCTRL_PG_EN		BIT(15)
>> +#define EFUCTRL_WR_EN		BIT(1)
>> +#define EFUCTRL_RD_EN		BIT(0)
>> +
>> +#define EFUCFG_INT_EN		BIT(31)
>> +#define EFUCFG_RD_ADJ_MASK	0xF
>> +#define EFUCFG_RD_ADJ_SHIFT	20
>> +#define EFUCFG_RD_STR_MASK	0xF
>> +#define EFUCFG_RD_STR_SHIFT	16
>> +#define EFUCFG_WR_ADJ_MASK	0xF
>> +#define EFUCFG_WR_ADJ_SHIFT	12
>> +#define EFUCFG_WR_STR_MASK	0xFFF
>> +#define EFUCFG_WR_STR_SHIFT	0
>> +
>> +#define EFUSTATE_WR_DONE	BIT(1)
>> +#define EFUSTATE_RD_DONE	BIT(0)
>> +
>> +struct jz4780_efuse {
>> +	struct device *dev;
>> +	struct regmap *map;
>> +	struct clk *clk;
>> +	unsigned int rd_adj;
>> +	unsigned int rd_strobe;
>> +};
>> +
>> +/* We read 32 byte chunks to avoid complexity in the driver. */
> 
> I don't see how reading 32 byte chunks avoids complexity in the driver; the 'size' variable here could very well be an argument.

I mixed your, mine and the original code.

One issue with a size argument is that regmap reads in 32bit words.

But reading the ethernet MAC address requires to fetch 6 bytes.
AFAIK regmap can only read in multiples of the word size.

It must also be possible to read starting at any byte address.

> 
>> +static int jz4780_efuse_read_32bytes(struct jz4780_efuse *efuse, char *buf,
>> +				     unsigned int addr)
>> +{
>> +	unsigned int tmp;
>> +	u32 ctrl;
>> +	int ret;
>> +	const int size = 32;
>> +
>> +	ctrl = (addr << EFUCTRL_ADDR_SHIFT)
>> +		| ((size - 1) << EFUCTRL_LEN_SHIFT)
>> +		| EFUCTRL_RD_EN;
>> +
>> +	regmap_update_bits(efuse->map, JZ_EFUCTRL,
>> +			   (EFUCTRL_ADDR_MASK << EFUCTRL_ADDR_SHIFT) |
>> +			   (EFUCTRL_LEN_MASK << EFUCTRL_LEN_SHIFT) |
>> +			   EFUCTRL_PG_EN | EFUCTRL_WR_EN | EFUCTRL_RD_EN, ctrl);
>> +
>> +	ret = regmap_read_poll_timeout(efuse->map, JZ_EFUSTATE,
>> +				       tmp, tmp & EFUSTATE_RD_DONE,
>> +				       1 * MSEC_PER_SEC, 50 * MSEC_PER_SEC);
>> +	if (ret < 0) {
>> +		dev_err(efuse->dev, "Time out while reading efuse data");
>> +		return ret;
>> +	}
>> +
>> +	return regmap_bulk_read(efuse->map, JZ_EFUDATA(0),
>> +				buf, size / sizeof(u32));
>> +}
>> +
>> +/* main entry point */
>> +static int jz4780_efuse_read(void *context, unsigned int offset,
>> +			     void *val, size_t bytes)
>> +{
>> +	struct jz4780_efuse *efuse = context;
>> +	int ret;
>> +	const int size = 32;
>> +
>> +	while (bytes > 0) {
>> +		unsigned int start = offset & ~(size - 1);
>> +		unsigned int chunk = min(bytes, (start + size) - offset);
> 
> Why do you need to check for the address alignment?

This is to decide if jz4780_efuse_read_32bytes() can directly read to the
destination buffer or if we need a temporary buffer. This avoids to always
use a temporary buffer.

> 
>> +
>> +		if (start == offset && chunk == size) {
>> +			ret = jz4780_efuse_read_32bytes(efuse, val, start);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +		} else {
>> +			char buf[32];
>> +
>> +			ret = jz4780_efuse_read_32bytes(efuse, buf, start);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			memcpy(val, &buf[offset - start], chunk);
> 
> Why do you need this cumbersome process of reading 32 bytes if you need less than that? This looks over-complex to me.

MAC address must allow to read 6 Bytes. I am not sure if regmap can do that.

According to the Programming Manual there are 8 registers with 32 bits each
so it is probably wise to read all these 32 bytes in one big read into the
regmap. So that it is never attempted to read smaller or not 32bit aligned
registers. This also makes sure that bank address switching is not done for
every byte.

> 
>> +		}
>> +
>> +		val += chunk;
>> +		offset += chunk;
>> +		bytes -= chunk;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct nvmem_config jz4780_efuse_nvmem_config __initdata = {
>> +	.name = "jz4780-efuse",
>> +	.size = 1024,
> 
> The efuse is 8 KiB.

I read the Programming Manual as 8 kBit = 1024 Byte. See for example
Note 1. and 2. in section "26.2.4 EFUSE Data Register (EFUDATAn)"

> 
>> +	.word_size = 1,
>> +	.stride = 1,
>> +	.owner = THIS_MODULE,
>> +	.reg_read = jz4780_efuse_read,
>> +};
>> +
>> +static const struct regmap_config jz4780_efuse_regmap_config = {
>> +	.reg_bits = 32,
>> +	.val_bits = 32,
>> +	.reg_stride = 4,
>> +	.max_register = JZ_EFUDATA(7),
>> +};
>> +
>> +static int jz4780_efuse_probe(struct platform_device *pdev)
>> +{
>> +	struct nvmem_device *nvmem;
>> +	struct jz4780_efuse *efuse;
>> +	struct nvmem_config cfg;
>> +	unsigned long clk_rate;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *regs;
>> +
>> +	efuse = devm_kzalloc(dev, sizeof(*efuse), GFP_KERNEL);
>> +	if (!efuse)
>> +		return -ENOMEM;
>> +
>> +	regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +
>> +	efuse->map = devm_regmap_init_mmio(dev, regs,
>> +					   &jz4780_efuse_regmap_config);
>> +	if (IS_ERR(efuse->map))
>> +		return PTR_ERR(efuse->map);
>> +
>> +	efuse->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(efuse->clk))
>> +		return PTR_ERR(efuse->clk);
>> +
>> +	clk_rate = clk_get_rate(efuse->clk);
> 
> You didn't enable the clock before, so clk_get_rate can return a bogus value.

Well, I copied that from your proposal for jz4780_efuse_probe().
So I don't know if that is correct or not...

How should it be solved? clk_enable() before and clk_disable() after?
Or is it enabled somewhere else so that we break something if
we explicitly disable?

> 
>> +
>> +	efuse->dev = dev;
>> +	/*
>> +	 * rd_adj and rd_strobe are 4 bit values
>> +	 * bus clk period * (rd_adj + 1) > 6.5ns
>> +	 * bus clk period * (rd_adj + 5 + rd_strobe) > 35ns
>> +	 */
>> +	efuse->rd_adj = (((6500 * (clk_rate / 1000000)) / 1000000) + 1) - 1;
> 
> I can't get my head around this calculus. Try this:

I would read it as: multiply clk_rate (in MHz units) by 6.5 and divide by another 1000.

So if clock rate is e.g. 500 Mhz we calculate 6.5 * 500 / 1000 => 3(.25).
500 Mhz is equivalent to 2 ns so 2 ns * 3.25 = 6.5 ns.

But that said: there seems to be something wrong with rounding. To fulfill the
> 6.5 ns condition the 3.25 should be rounded up to 4.

But since the condition is defined for rd_adj + 1 it is ok to set rd_adj to 3 in this case.

> 
> /* 1 / 6.5ns == 153846154 Hz */
> efuse->rd_adj = clk_rate / 153846154;
> 
> The efuse read should be *at least* 6.5ns, so no need to bother with the -1/+1.

Well, the (X + 1) - 1 looks very redundant.

So what about:

#define RD_ADJ_FACTOR (unsigned long)(10 * NSEC_PER_SEC / 65)	/* 1 / 6.5ns == 153846154 Hz */
 
	efuse->rd_adj = clk_rate / RD_ADJ_FACTOR;

> 
>> +	efuse->rd_strobe = ((((35000 * (clk_rate / 1000000)) / 1000000) + 1)
>> +						- 5 - efuse->rd_adj);
> 
> /* 1 / 35ns == 28571429 Hz */
> efuse->rd_strobe = max(0, clk_rate / 28571429 + 1 - 5 - efuse->rd_adj);

I think the original author just wanted to make the magic values a little
more transparent. 35000 / 1000000 is easier to recognise as 35 ns in the
programming manual than 28571429.

> 
>> +
>> +	if (efuse->rd_adj > 0x1F || efuse->rd_strobe > 0x1F) {
>> +		dev_err(&pdev->dev, "Cannot set clock configuration\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_update_bits(efuse->map, JZ_EFUCFG,
>> +			   (EFUCFG_RD_ADJ_MASK << EFUCFG_RD_ADJ_SHIFT) |
>> +			   (EFUCFG_RD_STR_MASK << EFUCFG_RD_STR_SHIFT),
>> +			   (efuse->rd_adj << EFUCFG_RD_ADJ_SHIFT) |
>> +			   (efuse->rd_strobe << EFUCFG_RD_STR_SHIFT));
>> +
>> +	cfg = jz4780_efuse_nvmem_config;
>> +	cfg.dev = &pdev->dev;
>> +	cfg.priv = efuse;
>> +
>> +	nvmem = devm_nvmem_register(dev, &cfg);
>> +	if (IS_ERR(nvmem))
>> +		return PTR_ERR(nvmem);
>> +
>> +	platform_set_drvdata(pdev, nvmem);
> 
> This isn't used anywhere.

Also copied from your proposal...
But it is indeed not used anywhere.

A big question is why we need to store rd_adj and rd_strobe in the
struct jz4780_efuse? They are local variables inside jz4780_efuse_probe().

So the only values used are efuse->map 

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id jz4780_efuse_match[] = {
>> +	{ .compatible = "ingenic,jz4780-efuse" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, jz4780_efuse_match);
>> +
>> +static struct platform_driver jz4780_efuse_driver = {
>> +	.probe  = jz4780_efuse_probe,
>> +	.driver = {
>> +		.name = "jz4780-efuse",
>> +		.of_match_table = jz4780_efuse_match,
>> +	},
>> +};
>> +module_platform_driver(jz4780_efuse_driver);
>> +
>> +MODULE_AUTHOR("PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>");
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@...delico.com>");
>> +MODULE_AUTHOR("Paul Cercueil <paul@...pouillou.net>");
>> +MODULE_DESCRIPTION("Ingenic JZ4780 efuse driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.23.0


BR and thanks,
Nikolaus

Powered by blists - more mailing lists