mbox series

[RFC,v3,0/9] MIPS: CI20: Add efuse driver for Ingenic JZ4780 and attach to DM9000 for stable MAC addresses

Message ID cover.1581880851.git.hns@goldelico.com
Headers show
Series MIPS: CI20: Add efuse driver for Ingenic JZ4780 and attach to DM9000 for stable MAC addresses | expand

Message

H. Nikolaus Schaller Feb. 16, 2020, 7:20 p.m. UTC
This series is based on and a follow up for

https://lore.kernel.org/patchwork/cover/868157/

("[v2,0/2] Add efuse driver for Ingenic JZ4780 SoC")

Original authors were
PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
Mathieu Malaterre <malat@debian.org>

and there are additions / code improvements by
H. Nikolaus Schaller <hns@goldelico.com>
Paul Cercueil <paul@crapouillou.net>

This setup works, if the dm9000 driver is compiled
as a module.

Therefore it is all RFC level. It is also not completely
checkpatched.


H. Nikolaus Schaller (2):
  MIPS: DTS: CI20: make DM9000 Ethernet controller use NVMEM to find the
    default MAC address
  MIPS: CI20 defconfig: DEMO HACK: make DM9000 a module

Mathieu Malaterre (1):
  MIPS: CI20 defconfig: Probe efuse for CI20

Paul Cercueil (1):
  rework to use regmap

PrasannaKumar Muralidharan (5):
  nvmem: add driver for JZ4780 efuse
  Bindings: nvmem: add bindings for JZ4780 efuse
  Documentation: ABI: nvmem: add documentation for JZ4780 efuse ABI
  nvmem: MAINTAINERS: add maintainer for JZ4780 efuse driver
  MIPS: DTS: JZ4780: define node for JZ4780 efuse

 .../ABI/testing/sysfs-driver-jz4780-efuse     |  16 ++
 .../bindings/nvmem/ingenic,jz4780-efuse.txt   |  17 ++
 MAINTAINERS                                   |   5 +
 arch/mips/boot/dts/ingenic/ci20.dts           |   3 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  17 +-
 arch/mips/configs/ci20_defconfig              |   4 +-
 drivers/nvmem/Kconfig                         |  10 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/jz4780-efuse.c                  | 238 ++++++++++++++++++
 9 files changed, 310 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
 create mode 100644 Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
 create mode 100644 drivers/nvmem/jz4780-efuse.c

Comments

Srinivas Kandagatla Feb. 17, 2020, 11:36 a.m. UTC | #1
On 16/02/2020 19:20, H. Nikolaus Schaller wrote:
> From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.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@debian.org>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> (Signed-off-by: Paul Cercueil <paul@crapouillou.net>)
> ---
>   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!

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


> +	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@gmail.com>
> + * Copyright (c) 2020 H. Nikolaus Schaller <hns@goldelico.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


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

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


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


> +	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@gmail.com>");
> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
> +MODULE_DESCRIPTION("Ingenic JZ4780 efuse driver");
> +MODULE_LICENSE("GPL v2");
>
H. Nikolaus Schaller Feb. 17, 2020, 12:26 p.m. UTC | #2
Hi Srinivas Kandagatla,

> Am 17.02.2020 um 12:36 schrieb Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
> 
> 
> 
> On 16/02/2020 19:20, H. Nikolaus Schaller wrote:
>> From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.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@debian.org>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> (Signed-off-by: Paul Cercueil <paul@crapouillou.net>)

@Paul: may I assume your signed off after squashing the patch 2/9?

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

> 
> 
>> +	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@gmail.com>
>> + * Copyright (c) 2020 H. Nikolaus Schaller <hns@goldelico.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@gmail.com>");
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>> +MODULE_DESCRIPTION("Ingenic JZ4780 efuse driver");
>> +MODULE_LICENSE("GPL v2");

BR and thanks,
Nikolaus Schaller
Paul Cercueil Feb. 17, 2020, 2:48 p.m. UTC | #3
Hi Nikolaus,


Le lun., févr. 17, 2020 at 13:26, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Srinivas Kandagatla,
> 
>>  Am 17.02.2020 um 12:36 schrieb Srinivas Kandagatla 
>> <srinivas.kandagatla@linaro.org>:
>> 
>> 
>> 
>>  On 16/02/2020 19:20, H. Nikolaus Schaller wrote:
>>>  From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.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@debian.org>
>>>  Signed-off-by: PrasannaKumar Muralidharan 
>>> <prasannatsmkumar@gmail.com>
>>>  Signed-off-by: Mathieu Malaterre <malat@debian.org>
>>>  Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>  (Signed-off-by: Paul Cercueil <paul@crapouillou.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@gmail.com>
>>>  + * Copyright (c) 2020 H. Nikolaus Schaller <hns@goldelico.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@gmail.com>");
>>>  +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>>>  +MODULE_DESCRIPTION("Ingenic JZ4780 efuse driver");
>>>  +MODULE_LICENSE("GPL v2");
> 
> BR and thanks,
> Nikolaus Schaller
> 
>
Paul Cercueil Feb. 17, 2020, 2:58 p.m. UTC | #4
Hi Srinivas,


Le lun., févr. 17, 2020 at 11:36, Srinivas Kandagatla 
<srinivas.kandagatla@linaro.org> a écrit :
> 
> 
> On 16/02/2020 19:20, H. Nikolaus Schaller wrote:
>> From: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.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@debian.org>
>> Signed-off-by: PrasannaKumar Muralidharan 
>> <prasannatsmkumar@gmail.com>
>> Signed-off-by: Mathieu Malaterre <malat@debian.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> (Signed-off-by: Paul Cercueil <paul@crapouillou.net>)
>> ---
>>   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!
> 
>> 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?
> 
> 
>> +	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@gmail.com>
>> + * Copyright (c) 2020 H. Nikolaus Schaller <hns@goldelico.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
> 
> 
>> +
>> +#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.
> 
>> +	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?

I think its semantic is different - devicetree specifies that you want 
the nvmem to be readonly, this one specifies that the driver cannot do 
read-write anyway.

With that said, there is no .reg_write, so .read_only will be enforced 
to 'true' anyway, so it can be removed.

-Paul

> 
>> +	.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?
> 
> 
>> +	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@gmail.com>");
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>> +MODULE_DESCRIPTION("Ingenic JZ4780 efuse driver");
>> +MODULE_LICENSE("GPL v2");
>>