Date:   Sat, 22 Feb 2020 15:13:45 -0300
From:   Paul Cercueil <paul@...pouillou.net>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
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 Nikolaus,


Le sam., févr. 22, 2020 at 18:16, H. Nikolaus Schaller 
<hns@...delico.com> a écrit :
> 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.

I think you need to depend on OF too, since the driver only probes on 
devicetree.

>> 
>>>  +	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.

Right, I didn't think about byte access.

>> 
>>>  +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.

Just always use a temporary buffer. It's not like this will be called 
very often anyway. That makes the code simpler.

>> 
>>>  +
>>>  +		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.

Honestly, it doesn't have to be fast. You could very well read byte per 
byte. Then you wouldn't have to care about non-aligned addresses or 
sizes, and the driver would be much simpler.

>> 
>>>  +		}
>>>  +
>>>  +		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)"

Right. 8 Kib, not KiB.

>> 
>>>  +	.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?

clk_prepare_enable() here, and clk_disable_unprepare() in the remove 
callback or in a function passed to devm_add_action_or_reset().

>> 
>>>  +
>>>  +	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.

Yes, I'm not saying that the calculus is wrong. But it multiplies 
picoseconds with MHz, then divides by 1000000, so it's not easy to 
understand.

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

Well, you still need the comment after the #define, so why use a 
#define in the first place?

>> 
>>>  +	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.

35000 / 1000000 is 35ms, not 35ns.

28571429 Hz is easy to recognize as 35ns when there's a comment right 
above saying where that value comes from.

> 
>> 
>>>  +
>>>  +	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.

My code wasn't upstream-ready, and wasn't even tested on real hardware. 
I'm reviewing the driver for mainline inclusion now.

> 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().

Maybe move the calculation of rd_adj / rd_strobe to a new function 
that's called from the probe, and have these local variables within 
this function.

> 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