Re: [PATCH v2 2/2] drivers: misc: Add driver to access ZynqMP efuses

On 5/15/24 14:41, lukas.funke-oss@weidmueller.com wrote:

From: Lukas Funke lukas.funke@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@linar...

Signed-off-by: Lukas Funke lukas.funke@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@amd.com
• • (C) Copyright 2024 Weidmueller Interface GmbH
• • Lukas Funke lukas.funke@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