Re: [PATCH v7 2/7] nvmem: add driver for JZ4780 efuse

["H. Nikolaus Schaller" <hns@goldelico.com>]

> Am 28.02.2020 um 15:48 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le ven., févr. 28, 2020 at 14:58, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> 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.
>> To fetch for example the MAC address:
>> dd if=/sys/devices/platform/134100d0.efuse/jz4780-efuse0/nvmem bs=1 skip=34 count=6 status=none | xxd
>> 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        |  12 ++
>> drivers/nvmem/Makefile       |   2 +
>> drivers/nvmem/jz4780-efuse.c | 234 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 248 insertions(+)
>> create mode 100644 drivers/nvmem/jz4780-efuse.c
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 35efab1ba8d9..d7b7f6d688e7 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -55,6 +55,18 @@ 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
>> +	depends on OF
>> +	select REGMAP_MMIO
>> +	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..4e9dd340e33a
>> --- /dev/null
>> +++ b/drivers/nvmem/jz4780-efuse.c
>> @@ -0,0 +1,234 @@
>> +// 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>
>> +
>> +#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)
>> +
>> +/* We read 32 byte chunks to avoid complexity in the driver. */
>> +#define JZ_EFU_READ_SIZE 32
>> +
>> +#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;
>> +};
>> +
>> +/* main entry point */
>> +static int jz4780_efuse_read(void *context, unsigned int offset,
>> +			     void *val, size_t bytes)
>> +{
>> +	struct jz4780_efuse *efuse = context;
>> +
>> +	while (bytes > 0) {
>> +		unsigned int start = offset & ~(JZ_EFU_READ_SIZE - 1);
>> +		unsigned int chunk = min(bytes, (start + JZ_EFU_READ_SIZE)
>> +					 - offset);
>> +		char buf[JZ_EFU_READ_SIZE];
>> +		unsigned int tmp;
>> +		u32 ctrl;
>> +		int ret;
>> +
>> +		ctrl = (start << EFUCTRL_ADDR_SHIFT)
>> +			| ((JZ_EFU_READ_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;
>> +		}
>> +
>> +		ret = regmap_bulk_read(efuse->map, JZ_EFUDATA(0),
>> +				       buf, JZ_EFU_READ_SIZE / sizeof(u32));
>> +		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",
>> +	.size = 1024,
>> +	.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;
>> +	unsigned long rd_adj;
>> +	unsigned long rd_strobe;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *regs;
>> +	int ret;
>> +
>> +	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);
>> +
>> +	ret = clk_prepare_enable(efuse->clk);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(&pdev->dev,
>> +				       &clk_disable_unprepare,
>> +				       efuse->clk);
> 
> That's what I thought, this does not build:
> 
> CC      drivers/nvmem/jz4780-efuse.o
> drivers/nvmem/jz4780-efuse.c: In function 'jz4780_efuse_probe':
> drivers/nvmem/jz4780-efuse.c:168:12: error: passing argument 2 of 'devm_add_action_or_reset' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 168 |            &clk_disable_unprepare,
>     |            ^~~~~~~~~~~~~~~~~~~~~~
>     |            |
>     |            void (*)(struct clk *)
> In file included from ./include/linux/platform_device.h:13,
>                from drivers/nvmem/jz4780-efuse.c:29:
> ./include/linux/device.h:256:16: note: expected 'void (*)(void *)' but argument is of type 'void (*)(struct clk *)'
> 256 |         void (*action)(void *), void *data)
>     |         ~~~~~~~^~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[2]: *** [scripts/Makefile.build:268: drivers/nvmem/jz4780-efuse.o] Error 1
> make[1]: *** [scripts/Makefile.build:505: drivers/nvmem] Error 2
> make: *** [Makefile:1681: drivers] Error 2
> 
> 
> You need a local function of type 'void foo(void *data)' that just calls clk_disable_unprepare(data) and the compiler will be happy.
> (btw - no need to deference functions pointers, since they are already pointers)

Well, this asks for a devm_clk_enable_prepare but it was apparently rejected...

https://lore.kernel.org/patchwork/patch/755667/

> 
> With that said, I expect you to at least compile-test the patchset before sending it upstream...

I have compiled and tested it on real hardware. But missed the warning because my cross-compiler seems to emit a warning only.

Sorry, I don't have a good day and after 7 rounds one wants to get things through the door and with least effort as quick as possible :)

BR,
Nikolaus