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

Lukas Funke lukas.funke-oss at weidmueller.com
Mon Jun 3 08:48:02 CEST 2024


On 29.05.2024 16:40, Michal Simek wrote:
> 
> 
> 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);
> 
> 

I see.

BTW: Is 'sizeof(bytes)' correct here? Some efuse access will be greater 
than sizeof(bytes) I guess. That's why I changed the dma-alloc to 
'dma_alloc_coherent(bytes, &dma_buf)' in the u-boot port in contrast to 
the original code.

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

Can we use dev_dbg here? Since the return indicates that 'dev' is 
invalid here. My idea was to use the dev_dbg for the Linux port and 
log-infrastructure for u-boot related functions.

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