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

[Stefan Roese <sr@denx.de>]

Hi Lukas,

On 5/15/24 08:33, Lukas Funke wrote:
> Hi Stefan,
> 
> On 15.05.2024 08:12, Stefan Roese wrote:
>> Hi Lukas,
>>
>> On 5/14/24 16:04, 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@linaro.org/
>>>
>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com>
>>> ---
>>>
>>>   drivers/misc/Kconfig        |   8 ++
>>>   drivers/misc/Makefile       |   1 +
>>>   drivers/misc/zynqmp_efuse.c | 213 ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 222 insertions(+)
>>>   create mode 100644 drivers/misc/zynqmp_efuse.c
>>>
>>> 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..0cfc42a4f39
>>> --- /dev/null
>>> +++ b/drivers/misc/zynqmp_efuse.c
>>> @@ -0,0 +1,213 @@
>>> +// 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 EFUSE_MEMORY_SIZE    (0x177)
>>> +#define UNUSED_SPACE        (0x8)
>>> +#define ZYNQMP_NVMEM_SIZE    (SOC_VER_SIZE + UNUSED_SPACE + \
>>> +                 EFUSE_MEMORY_SIZE)
>>> +#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)
>>> +
>>> +/**
>>> + * 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`
>>> + * @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_access(struct udevice *dev, unsigned int 
>>> offset,
>>> +                   void *val, size_t bytes, unsigned int flag,
>>> +                   unsigned int pufflag)
>>> +{
>>> +    size_t words = bytes / WORD_INBYTES;
>>> +    ulong dma_addr, dma_buf;
>>> +    struct xilinx_efuse *efuse;
>>> +    char *data;
>>> +    int ret, value;
>>> +
>>> +    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));
>>
>> efuse and data are allocated via dma_alloc_coherent(). It should not be
>> necessary to use flush the cache here IIUTC.
> 
> If I understand correctly dma_alloc_coherent() maps to an aligned 
> malloc() which in turn just returns some physical memory without any 
> caching attributes (is this correct?). We have to ensure that the data 
> written here is *not* cached but written back to memory because the PMU 
> is running on a co-processor and data is exchanged via DRAM.

Frankly, I did not look into the U-Boot implementation of
dma_alloc_coherent() - I've rarely seen it here before. But the
original implementation in Linux guarantees that "DMA safe" (uncached)
memory is allocated AFAIU.

> Also: this is the way it was implemented in the other PMU calls as well.

Agreed. I also would copy such stuff from already existing code. Even
though this could be wrong from the beginning.

I just stumbled over this and wondered, if this really is needed
this way.

Thanks,
Stefan

>>
>>> +
>>> +    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:
>>
>> Nitpicking: Upper case label ist pretty uncommon AFAIK.
> 
> Since this is a port of the actual Linux driver I wanted to change as 
> little as possible. If this is absolutly not acceptable I'm open to 
> change this.
> 
> BTW: thanks for your review!
> 
>>
>>> +
>>> +    dma_free_coherent(efuse);
>>> +    dma_free_coherent(data);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int zynqmp_nvmem_read(struct udevice *dev, int offset,
>>> +                 void *val, int bytes)
>>> +{
>>> +    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)
>>> +{
>>> +    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);
>>> +}
>>> +
>>> +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,
>>> +};
>>
>> Viele Grüße,
>> Stefan Roese
>>
> 
> Best regards
>   - Lukas
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de