Message-Id: <1581950911.3.3@crapouillou.net>
Date:   Mon, 17 Feb 2020 11:48:31 -0300
From:   Paul Cercueil <paul@...pouillou.net>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>,
        Mathieu Malaterre <malat@...ian.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 Mailing List <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, linux-mips@...r.kernel.org,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com
Subject: Re: [RFC v3 1/9] nvmem: add driver for JZ4780 efuse

Hi Nikolaus,


Le lun., févr. 17, 2020 at 13:26, H. Nikolaus Schaller 
<hns@...delico.com> a écrit :
> Hi Srinivas Kandagatla,
> 
>>  Am 17.02.2020 um 12:36 schrieb Srinivas Kandagatla 
>> <srinivas.kandagatla@...aro.org>:
>> 
>> 
>> 
>>  On 16/02/2020 19:20, H. Nikolaus Schaller wrote:
>>>  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>)
> 
> @Paul: may I assume your signed off after squashing the patch 2/9?

Yes. As the Ingenic SoCs maintainer I'll still need to review though.

> 
>>>  ---
>>>   drivers/nvmem/Kconfig        |  10 ++
>>>   drivers/nvmem/Makefile       |   2 +
>>>   drivers/nvmem/jz4780-efuse.c | 249 
>>> +++++++++++++++++++++++++++++++++++
>>>   3 files changed, 261 insertions(+)
>>>   create mode 100644 drivers/nvmem/jz4780-efuse.c
>> 
>>  This patch along with 2/9 should be merged into single patch.
>>  Also please make sure you run checkpatch.pl before sending!
> 
> Sure. I had noted that in the [0/9].
> 
>> 
>>>  diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>>  index 35efab1ba8d9..10f8e08f5e31 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_JZ4780 || COMPILE_TEST
>> 
>>  what is that this driver depends on MACH_JZ4780 board?
> 
> Hm. Good question. Well, it is only useful for the jz4780 SoC
> but since we match drivers through DTS there is probably no reason
> to make it depend on. And depend on COMPILE_TEST is also not
> needed if I understand correctly.
> 
> On the other hand this makes the option disappear for non-jz4780 SoC.
> 
> So I leave it for the moment but keep in mind.

Make it MACH_INGENIC || COMPILE_TEST.

* You want COMPILE_TEST so that the driver can be compile-tested. It 
should always be there unless the driver really isn't buildable if 
MACH_JZ4780 is not set.
* If you depend on MACH_JZ4780, then you cannot create a kernel that 
will work on anything else than a JZ4780. Depend on MACH_INGENIC 
instead.

> 
>> 
>> 
>>>  +	depends on HAS_IOMEM
>>>  +	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..ac03e1900ef9
>>>  --- /dev/null
>>>  +++ b/drivers/nvmem/jz4780-efuse.c
>>>  @@ -0,0 +1,249 @@
>>>  +// 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>
>>  ?? why do we need these two headers in this patch
> 
> regmap will be used after squashing 2/9.
> 
>> 
>> 
>>>  +
>>>  +#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 JZ_EFUSE_START_ADDR		0x200
>>>  +
>>>  +#define JZ_EFUSE_EFUCTRL_CS		BIT(30)
>>>  +#define JZ_EFUSE_EFUCTRL_ADDR_MASK	0x1FF
>>>  +#define JZ_EFUSE_EFUCTRL_ADDR_SHIFT	21
>>>  +#define JZ_EFUSE_EFUCTRL_LEN_MASK	0x1F
>>>  +#define JZ_EFUSE_EFUCTRL_LEN_SHIFT	16
>>>  +#define JZ_EFUSE_EFUCTRL_PG_EN		BIT(15)
>>>  +#define JZ_EFUSE_EFUCTRL_WR_EN		BIT(1)
>>>  +#define JZ_EFUSE_EFUCTRL_RD_EN		BIT(0)
>>>  +
>>>  +#define JZ_EFUSE_EFUCFG_INT_EN		BIT(31)
>>>  +#define JZ_EFUSE_EFUCFG_RD_ADJ_MASK	0xF
>> 
>>  consider using GENMASK for these masks here.
>> 
>>>  +#define JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT	20
>>>  +#define JZ_EFUSE_EFUCFG_RD_STR_MASK	0xF
>>>  +#define JZ_EFUSE_EFUCFG_RD_STR_SHIFT	16
>>>  +#define JZ_EFUSE_EFUCFG_WR_ADJ_MASK	0xF
>>>  +#define JZ_EFUSE_EFUCFG_WR_ADJ_SHIFT	12
>>>  +#define JZ_EFUSE_EFUCFG_WR_STR_MASK	0xFFF
>>>  +#define JZ_EFUSE_EFUCFG_WR_STR_SHIFT	0
>>>  +
>>>  +#define JZ_EFUSE_EFUSTATE_WR_DONE	BIT(1)
>>>  +#define JZ_EFUSE_EFUSTATE_RD_DONE	BIT(0)
>>>  +
>>>  +struct jz4780_efuse {
>>>  +	struct device *dev;
>>>  +	void __iomem *iomem;
>>>  +	struct clk *clk;
>>>  +	unsigned int rd_adj;
>>>  +	unsigned int rd_strobe;
>>>  +};
>>>  +
>>>  +/* We read 32 byte chunks to avoid complexity in the driver. */
>>>  +static int jz4780_efuse_read_32bytes(struct jz4780_efuse *efuse, 
>>> char *buf,
>>>  +		unsigned int addr)
>>>  +{
>>>  +	unsigned int tmp = 0;
>>>  +	int i = 0;
>> 
>>  unnecessary initialization of both variables.
>> 
>>>  +	int timeout = 1000;
>>>  +	int size = 32;
>> 
>>  better to #define this STRIDE/CHUNK size. this driver seems to use 
>> this value in multiple places.
>> 
>>>  +
>>>  +	/* 1. Set config register */
>>>  +	tmp = readl(efuse->iomem + JZ_EFUCFG);
>>>  +	tmp &= ~((JZ_EFUSE_EFUCFG_RD_ADJ_MASK << 
>>> JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
>>>  +	| (JZ_EFUSE_EFUCFG_RD_STR_MASK << JZ_EFUSE_EFUCFG_RD_STR_SHIFT));
>>>  +	tmp |= (efuse->rd_adj << JZ_EFUSE_EFUCFG_RD_ADJ_SHIFT)
>>>  +	| (efuse->rd_strobe << JZ_EFUSE_EFUCFG_RD_STR_SHIFT);
>> 
>>  very odd indenting.
> 
> Will be replaced by 2/9.
> 
> Sorry for having you review this old version which is replaced by 2/9.
> 
> But it appeared more helpful to keep this as the 2018 original and 
> then update
> in a separate patch for this RFC stage.
> 
>> 
>>>  +	writel(tmp, efuse->iomem + JZ_EFUCFG);
>>>  +
>>>  +	/*
>>>  +	 * 2. Set control register to indicate what to read data address,
>>>  +	 * read data numbers and read enable.
>>>  +	 */
>>>  +	tmp = readl(efuse->iomem + JZ_EFUCTRL);
>>>  +	tmp &= ~(JZ_EFUSE_EFUCFG_RD_STR_SHIFT
>>>  +		| (JZ_EFUSE_EFUCTRL_ADDR_MASK << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
>>>  +		| JZ_EFUSE_EFUCTRL_PG_EN | JZ_EFUSE_EFUCTRL_WR_EN
>>>  +		| JZ_EFUSE_EFUCTRL_WR_EN);
>>>  +
>>>  +	/* Need to select CS bit if address accesses upper 4Kbits memory 
>>> */
>>>  +	if (addr >= (JZ_EFUSE_START_ADDR + 512))
>>>  +		tmp |= JZ_EFUSE_EFUCTRL_CS;
>>>  +
>>>  +	tmp |= (addr << JZ_EFUSE_EFUCTRL_ADDR_SHIFT)
>>>  +		| ((size - 1) << JZ_EFUSE_EFUCTRL_LEN_SHIFT)
>>>  +		| JZ_EFUSE_EFUCTRL_RD_EN;
>>>  +	writel(tmp, efuse->iomem + JZ_EFUCTRL);
>>>  +
>>>  +	/*
>>>  +	 * 3. Wait status register RD_DONE set to 1 or EFUSE interrupted,
>>>  +	 * software can read EFUSE data buffer 0 - 8 registers.
>>>  +	 */
>>>  +	do {
>>>  +		tmp = readl(efuse->iomem + JZ_EFUSTATE);
>>>  +		usleep_range(1000, 2000);
>>>  +		if (timeout--)
>>>  +			break;
>>>  +	} while (!(tmp & JZ_EFUSE_EFUSTATE_RD_DONE));
>>>  +
>>>  +	if (timeout <= 0) {
>>>  +		dev_err(efuse->dev, "Timed out while reading\n");
>>>  +		return -EAGAIN;
>>>  +	}
>>>  +
>>>  +	for (i = 0; i < (size / 4); i++)
>>>  +		*((unsigned int *)(buf + i * 4))
>> 
>>  make "unsigned int *buf32" a local variable and use it here, makes 
>> it much readable code.
>> 
>>>  +			 = readl(efuse->iomem + JZ_EFUDATA(i));
>>>  +
>>>  +	return 0;
>>>  +}
>>>  +
>>>  +/* 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;
>>>  +
>>>  +	while (bytes > 0) {
>>>  +		unsigned int start = offset & ~(32 - 1);
>>>  +		unsigned chunk = min(bytes, (start + 32 - offset));
>>>  +
>>>  +		if (start == offset && chunk == 32) {
>>>  +			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);
>>>  +		}
>>>  +
>>>  +		val += chunk;
>>>  +		offset += chunk;
>>>  +		bytes -= chunk;
>>>  +	}
>>>  +
>>>  +	return 0;
>>>  +}
>>>  +
>>>  +static struct nvmem_config jz4780_efuse_nvmem_config = {
>>>  +	.name = "jz4780-efuse",
>>>  +	.read_only = true,
>> 
>>  this value comes from device tree bindings, do we still need  to 
>> specify this here?
> 
> Ok, have to check what is automatically taken from DT.
> 
>> 
>> 
>>>  +	.size = 1024,
>>>  +	.word_size = 1,
>>>  +	.stride = 1,
>>>  +	.owner = THIS_MODULE,
>>>  +	.reg_read = jz4780_efuse_read,
>>>  +};
>>>  +
>>>  +static int jz4780_efuse_probe(struct platform_device *pdev)
>>>  +{
>>>  +	struct nvmem_device *nvmem;
>>>  +	struct jz4780_efuse *efuse;
>>>  +	struct resource *res;
>>>  +	unsigned long clk_rate;
>>>  +	struct device *dev = &pdev->dev;
>>>  +
>>>  +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
>>>  +	if (!efuse)
>>>  +		return -ENOMEM;
>>>  +
>>>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  +	efuse->iomem = devm_ioremap(&pdev->dev, res->start, 
>>> resource_size(res));
>>>  +	if (IS_ERR(efuse->iomem))
>>>  +		return PTR_ERR(efuse->iomem);
>>>  +
>>>  +	efuse->clk = devm_clk_get(&pdev->dev, "bus_clk");
>>>  +	if (IS_ERR(efuse->clk))
>>>  +		return PTR_ERR(efuse->clk);
>> 
>>  Who is enabling this clk?
>> 
>> 
>>>  +
>>>  +	clk_rate = clk_get_rate(efuse->clk);
>>>  +	/*
>>>  +	 * 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;
>>>  +	efuse->rd_strobe = ((((35000 * (clk_rate / 1000000)) / 1000000) 
>>> + 1)
>>>  +						- 5 - efuse->rd_adj);
>>>  +
>>>  +	if ((efuse->rd_adj > 0x1F) || (efuse->rd_strobe > 0x1F)) {
>>>  +		dev_err(&pdev->dev, "Cannot set clock configuration\n");
>>>  +		return -EINVAL;
>>>  +	}
>>>  +	efuse->dev = dev;
>>>  +
>>>  +	jz4780_efuse_nvmem_config.dev = &pdev->dev;
>>>  +	jz4780_efuse_nvmem_config.priv = efuse;
>>>  +
>>>  +	nvmem = nvmem_register(&jz4780_efuse_nvmem_config);
>> 
>>  devm variant here?
> 
> Good idea! I didn't know that it exists. Allows us to remove
> jz4780_efuse_remove() below.
> 
>> 
>> 
>>>  +	if (IS_ERR(nvmem))
>>>  +		return PTR_ERR(nvmem);
>>>  +
>>>  +	platform_set_drvdata(pdev, nvmem);
>>>  +
>>>  +	return 0;
>>>  +}
>>>  +
>>>  +static int jz4780_efuse_remove(struct platform_device *pdev)
>>>  +{
>>>  +	struct nvmem_device *nvmem = platform_get_drvdata(pdev);
>>>  +
>>>  +	nvmem_unregister(nvmem);
>>>  +
>>>  +	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,
>>>  +	.remove = jz4780_efuse_remove,
>>>  +	.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_DESCRIPTION("Ingenic JZ4780 efuse driver");
>>>  +MODULE_LICENSE("GPL v2");
> 
> BR and thanks,
> Nikolaus Schaller
> 
> 

Powered by blists - more mailing lists