[PATCH v2 2/2] drivers: misc: Add driver to access ZynqMP efuses
Michal Simek
michal.simek at amd.com
Wed May 29 16:40:38 CEST 2024
On 5/15/24 14:41, lukas.funke-oss at weidmueller.com wrote:
> From: Lukas Funke <lukas.funke at weidmueller.com>
>
> Add driver to access ZynqMP efuses. This is a u-boot port of [1].
>
> [1] https://lore.kernel.org/all/20240224114516.86365-8-srinivas.kandagatla@linaro.org/
>
> Signed-off-by: Lukas Funke <lukas.funke at weidmueller.com>
> ---
>
> Changes in v2:
> - Drop vendor specific fuse cmd, use existing fuse cmd
> - Minor code refactoring (reverse x-mas tree)
>
> drivers/misc/Kconfig | 8 +
> drivers/misc/Makefile | 1 +
> drivers/misc/zynqmp_efuse.c | 324 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 333 insertions(+)
> create mode 100644 drivers/misc/zynqmp_efuse.c
would be good to get also 3/3 which enables
CONFIG_CMD_FUSE=y
CONFIG_ZYNQMP_EFUSE=y
in zynqmp defconfigs (virt and kria) to have it enabled.
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 6009d55f400..c07f50c9a76 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -298,6 +298,14 @@ config FSL_SEC_MON
> Security Monitor can be transitioned on any security failures,
> like software violations or hardware security violations.
>
> +config ZYNQMP_EFUSE
> + bool "Enable ZynqMP eFUSE Driver"
> + depends on ZYNQMP_FIRMWARE
> + help
> + Enable access to Zynq UltraScale (ZynqMP) eFUSEs thought PMU firmware
> + interface. ZnyqMP has 256 eFUSEs where some of them are security related
> + and cannot be read back (i.e. AES key).
> +
> choice
> prompt "Security monitor interaction endianess"
> depends on FSL_SEC_MON
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e53d52c47b3..68ba5648eab 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -92,3 +92,4 @@ obj-$(CONFIG_ESM_K3) += k3_esm.o
> obj-$(CONFIG_ESM_PMIC) += esm_pmic.o
> obj-$(CONFIG_SL28CPLD) += sl28cpld.o
> obj-$(CONFIG_SPL_SOCFPGA_DT_REG) += socfpga_dtreg.o
> +obj-$(CONFIG_ZYNQMP_EFUSE) += zynqmp_efuse.o
> diff --git a/drivers/misc/zynqmp_efuse.c b/drivers/misc/zynqmp_efuse.c
> new file mode 100644
> index 00000000000..98a39c1ebdd
> --- /dev/null
> +++ b/drivers/misc/zynqmp_efuse.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2014 - 2015 Xilinx, Inc.
> + * Michal Simek <michal.simek at amd.com>
> + *
> + * (C) Copyright 2024 Weidmueller Interface GmbH
> + * Lukas Funke <lukas.funke at weidmueller.com>
> + */
> +
> +#include <compiler.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <zynqmp_firmware.h>
> +#include <asm/dma-mapping.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <misc.h>
> +
> +#define SILICON_REVISION_MASK 0xF
> +#define P_USER_0_64_UPPER_MASK 0x5FFF0000
> +#define P_USER_127_LOWER_4_BIT_MASK 0xF
> +#define WORD_INBYTES (4)
> +#define SOC_VER_SIZE (0x4)
> +#define SOC_VERSION_OFFSET (0x0)
> +#define EFUSE_START_OFFSET (0xC)
> +#define EFUSE_END_OFFSET (0xFC)
> +#define EFUSE_PUF_START_OFFSET (0x100)
> +#define EFUSE_PUF_MID_OFFSET (0x140)
> +#define EFUSE_PUF_END_OFFSET (0x17F)
> +#define EFUSE_NOT_ENABLED (29)
> +#define EFUSE_READ (0)
> +#define EFUSE_WRITE (1)
I did compare it with the latest upstream version and there are some differences
which don't need to be there. Above macros for example
> +
> +/**
> + * struct efuse_map_entry - offset and length of zynqmp fuses
> + * @offset: offset of efuse to be read/write
> + * @length: length of efuse
> + */
> +struct efuse_map_entry {
> + u32 offset;
> + u32 length;
> +};
> +
> +struct efuse_map_entry zynqmp_efuse_table[] = {
> + {0x000, 0x04}, /* soc revision */
> + {0x00c, 0x0c}, /* SoC DNA */
> + {0x020, 0x04}, /* efuse-usr0 */
> + {0x024, 0x04}, /* efuse-usr1 */
> + {0x028, 0x04}, /* efuse-usr2 */
> + {0x02c, 0x04}, /* efuse-usr3 */
> + {0x030, 0x04}, /* efuse-usr4 */
> + {0x034, 0x04}, /* efuse-usr5 */
> + {0x038, 0x04}, /* efuse-usr6 */
> + {0x03c, 0x04}, /* efuse-usr7 */
> + {0x040, 0x04}, /* efuse-miscusr */
> + {0x050, 0x04}, /* efuse-chash */
> + {0x054, 0x04}, /* efuse-pufmisc */
> + {0x058, 0x04}, /* efuse-sec */
> + {0x05c, 0x04}, /* efuse-spkid */
> + {0x060, 0x30}, /* efuse-aeskey */
> + {0x0a0, 0x30}, /* ppk0-hash */
> + {0x0d0, 0x30}, /* ppk1-hash */
> + {0x100, 0x7f}, /* pufuser */
> +};
> +
> +/**
> + * struct xilinx_efuse - the basic structure
> + * @src: address of the buffer to store the data to be write/read
> + * @size: no of words to be read/write
> + * @offset: offset to be read/write`
a little bit different description compare to upsream linux
> + * @flag: 0 - represents efuse read and 1- represents efuse write
> + * @pufuserfuse:0 - represents non-puf efuses, offset is used for read/write
> + * 1 - represents puf user fuse row number.
> + *
> + * this structure stores all the required details to
> + * read/write efuse memory.
> + */
> +struct xilinx_efuse {
> + u64 src;
> + u32 size;
> + u32 offset;
> + u32 flag;
> + u32 pufuserfuse;
> +};
> +
> +static int zynqmp_efuse_get_length(u32 offset, u32 *base_offset, u32 *len)
> +{
> + struct efuse_map_entry *fuse;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(zynqmp_efuse_table); ++i) {
> + fuse = &zynqmp_efuse_table[i];
> + if (offset >= fuse->offset &&
> + offset < fuse->offset + fuse->length) {
> + *base_offset = fuse->offset;
> + *len = fuse->length;
> + return 0;
> + }
> + }
> +
> + return -ENOENT;
> +}
> +
> +static int zynqmp_efuse_access(struct udevice *dev, unsigned int offset,
> + void *val, size_t bytes, unsigned int flag,
> + unsigned int pufflag)
> +{
> + size_t words = bytes / WORD_INBYTES;
> + struct xilinx_efuse *efuse;
> + ulong dma_addr, dma_buf;
> + int ret, value;
> + char *data;
> +
> + if (bytes % WORD_INBYTES != 0) {
> + dev_err(dev, "Bytes requested should be word aligned\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (pufflag == 0 && offset % WORD_INBYTES) {
> + dev_err(dev, "Offset requested should be word aligned\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (pufflag == 1 && flag == EFUSE_WRITE) {
> + memcpy(&value, val, bytes);
> + if ((offset == EFUSE_PUF_START_OFFSET ||
> + offset == EFUSE_PUF_MID_OFFSET) &&
> + value & P_USER_0_64_UPPER_MASK) {
> + dev_err(dev, "Only lower 4 bytes are allowed to be programmed in P_USER_0 & P_USER_64\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (offset == EFUSE_PUF_END_OFFSET &&
> + (value & P_USER_127_LOWER_4_BIT_MASK)) {
> + dev_err(dev, "Only MSB 28 bits are allowed to be programmed for P_USER_127\n");
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + efuse = dma_alloc_coherent(sizeof(struct xilinx_efuse), &dma_addr);
> + if (!efuse)
> + return -ENOMEM;
> +
> + data = dma_alloc_coherent(bytes, &dma_buf);
> + if (!data) {
> + dma_free_coherent(efuse);
> + return -ENOMEM;
> + }
> +
> + if (flag == EFUSE_WRITE) {
> + memcpy(data, val, bytes);
> + efuse->flag = EFUSE_WRITE;
> + } else {
> + efuse->flag = EFUSE_READ;
> + }
> +
> + efuse->src = dma_buf;
> + efuse->size = words;
> + efuse->offset = offset;
> + efuse->pufuserfuse = pufflag;
> +
> + flush_dcache_range((ulong)efuse, (ulong)efuse +
> + roundup(sizeof(struct xilinx_efuse), ARCH_DMA_MINALIGN));
> + flush_dcache_range((ulong)data, (ulong)data +
> + roundup(sizeof(struct xilinx_efuse), ARCH_DMA_MINALIGN));
> +
> + zynqmp_pm_efuse_access(dma_addr, (u32 *)&ret);
> + if (ret != 0) {
> + if (ret == EFUSE_NOT_ENABLED) {
> + dev_err(dev, "efuse access is not enabled\n");
> + ret = -EOPNOTSUPP;
> + goto END;
> + }
> + dev_err(dev, "Error in efuse read %x\n", ret);
> + ret = -EPERM;
> + goto END;
> + }
> +
> + if (flag == EFUSE_READ)
> + memcpy(val, data, bytes);
> +END:
this has also changed and was commented by Stefan before.
efuse_access_err:
dma_free_coherent(dev, sizeof(bytes),
data, dma_buf);
efuse_data_fail:
dma_free_coherent(dev, sizeof(struct xilinx_efuse),
efuse, dma_addr);
> +
> + dma_free_coherent(efuse);
> + dma_free_coherent(data);
> +
> + return ret;
> +}
> +
> +static int zynqmp_nvmem_read(struct udevice *dev, int offset,
> + void *val, int bytes)
please check variables types.
> +{
> + int ret, pufflag = 0;
> + int idcode, version;
> +
> + if (offset >= EFUSE_PUF_START_OFFSET && offset <= EFUSE_PUF_END_OFFSET)
> + pufflag = 1;
> +
> + dev_dbg(dev, "reading from offset=0x%x, bytes=%d\n", offset, bytes);
> +
> + switch (offset) {
> + /* Soc version offset is zero */
> + case SOC_VERSION_OFFSET:
> + if (bytes != SOC_VER_SIZE)
> + return -EOPNOTSUPP;
> +
> + ret = zynqmp_pm_get_chipid((u32 *)&idcode, (u32 *)&version);
> + if (ret < 0)
> + return ret;
> +
> + *(int *)val = version & SILICON_REVISION_MASK;
> + break;
> + /* Efuse offset starts from 0xc */
> + case EFUSE_START_OFFSET ... EFUSE_END_OFFSET:
> + case EFUSE_PUF_START_OFFSET ... EFUSE_PUF_END_OFFSET:
> + ret = zynqmp_efuse_access(dev, offset, val,
> + bytes, EFUSE_READ, pufflag);
> + break;
> + default:
> + *(u32 *)val = 0xDEADBEEF;
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int zynqmp_nvmem_write(struct udevice *dev, int offset, const void *val,
> + int bytes)
here too. offset for example
> +{
> + int pufflag = 0;
> +
> + dev_dbg(dev, "writing to offset=0x%x, bytes=%d", offset, bytes);
> +
> + if (offset < EFUSE_START_OFFSET || offset > EFUSE_PUF_END_OFFSET)
> + return -EOPNOTSUPP;
> +
> + if (offset >= EFUSE_PUF_START_OFFSET && offset <= EFUSE_PUF_END_OFFSET)
> + pufflag = 1;
> +
> + return zynqmp_efuse_access(dev, offset,
> + (void *)val, bytes, EFUSE_WRITE, pufflag);
> +}
> +
> +int fuse_read(u32 bank, u32 word, u32 *val)
> +{
> + u32 base_offset, offset, len;
> + struct udevice *dev;
> + u8 buf[128];
128 is correct number but would be better to have connection where this number
is coming from. MAX size is 0x7f which is 127 but that connection is not visible
from it. I am fine with the comment to say it. Please look below.
> + int ret;
> +
> + memset(buf, 0, sizeof(buf));
I would call it before misc_read not to waste time in error case.
> +
> + ret = uclass_get_device_by_driver(UCLASS_MISC,
> + DM_DRIVER_GET(zynqmp_efuse), &dev);
> + if (ret)
> + return log_msg_ret("uclass_drv", ret);
Above c&p from Linux is using dev_dbg structure and here you are using log
infrastructure. Would be much better if you can use the same.
> +
> + ret = zynqmp_efuse_get_length(word, &base_offset, &len);
> + if (ret)
> + return log_msg_ret("efuse_offset", ret);
> +
And here please check that len is below 128 to make sure that we never overwrite
stack. Other entries can be added and it should be spotted.
> + ret = misc_read(dev, base_offset, (void *)buf, len);
> + if (ret)
> + return log_msg_ret("misc_read", ret);
> +
> + offset = word - base_offset;
> + *val = ((u32 *)buf)[offset];
> +
> + return 0;
> +}
> +
> +int fuse_prog(u32 bank, u32 word, u32 val)
> +{
> + u32 base_offset, len;
> + struct udevice *dev;
> + int ret;
> +
> + ret = zynqmp_efuse_get_length(word, &base_offset, &len);
> + if (ret)
> + return log_msg_ret("efuse_offset", ret);
> +
> + ret = uclass_get_device_by_driver(UCLASS_MISC,
> + DM_DRIVER_GET(zynqmp_efuse), &dev);
> + if (ret)
> + return log_msg_ret("uclass_drv", ret);
> +
> + if (word != base_offset || len != sizeof(val))
> + return -EINVAL;
> +
> + ret = misc_write(dev, word, (void *)&val, sizeof(val));
> + if (ret)
> + return log_msg_ret("misc_write", ret);
> +
> + return 0;
> +}
> +
> +int fuse_sense(u32 bank, u32 word, u32 *val)
> +{
> + /* We do not support sense */
avoid we - imperative mood please.
> + return -ENOSYS;
> +}
> +
> +int fuse_override(u32 bank, u32 word, u32 val)
> +{
> + /* We do not support overriding */
avoid we
> + return -ENOSYS;
> +}
> +
> +static const struct udevice_id zynqmp_efuse_match[] = {
> + { .compatible = "xlnx,zynqmp-nvmem-fw", },
> + { /* sentinel */ },
> +};
> +
> +static const struct misc_ops zynqmp_efuse_ops = {
> + .read = zynqmp_nvmem_read,
> + .write = zynqmp_nvmem_write,
> +};
> +
> +U_BOOT_DRIVER(zynqmp_efuse) = {
> + .name = "zynqmp_efuse",
> + .id = UCLASS_MISC,
> + .of_match = zynqmp_efuse_match,
> + .ops = &zynqmp_efuse_ops,
> +};
Thanks,
Michal
More information about the U-Boot
mailing list