| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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