Re: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model

[Philippe Mathieu-Daudé]

Hi,

(old patch)

On 11/9/20 07:20, Havard Skinnemoen wrote:
> This supports reading and writing OTP fuses and keys. Only fuse reading
> has been tested. Protection is not implemented.
>
> Reviewed-by: Avi Fishman <avi.fishman@nuvoton.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>   include/hw/arm/npcm7xx.h       |   3 +
>   include/hw/nvram/npcm7xx_otp.h |  79 ++++++
>   hw/arm/npcm7xx.c               |  29 +++
>   hw/nvram/npcm7xx_otp.c         | 440 +++++++++++++++++++++++++++++++++
>   hw/nvram/meson.build           |   1 +
>   5 files changed, 552 insertions(+)
>   create mode 100644 include/hw/nvram/npcm7xx_otp.h
>   create mode 100644 hw/nvram/npcm7xx_otp.c

> +/**
> + * npcm7xx_otp_array_write - ECC encode and write data to OTP array.
> + * @s: OTP module.
> + * @data: Data to be encoded and written.
> + * @offset: Offset of first byte to be written in the OTP array.
> + * @len: Number of bytes before ECC encoding.
> + *
> + * Each nibble of data is encoded into a byte, so the number of bytes written
> + * to the array will be @len * 2.
> + */
> +extern void npcm7xx_otp_array_write(NPCM7xxOTPState *s, const void *data,
> +                                    unsigned int offset, unsigned int len);

> +static void npcm7xx_init_fuses(NPCM7xxState *s)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
> +    uint32_t value;
> +
> +    /*
> +     * The initial mask of disabled modules indicates the chip derivative (e.g.
> +     * NPCM750 or NPCM730).
> +     */
> +    value = tswap32(nc->disabled_modules);

In which endianness do you want this 32-bit fuse value to be written?

I suppose you used a little-endian host, so we want it big-endian in
the OTP? In that case it would be better to use cpu_to_be32(), to
be able to use the OTP on a big-endian host such s390x.

> +    npcm7xx_otp_array_write(&s->fuse_array, &value, NPCM7XX_FUSE_DERIVATIVE,
> +                            sizeof(value));
> +}

For completeness:

> +static void npcm730_class_init(ObjectClass *oc, void *data)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +    /* NPCM730 is optimized for data center use, so no graphics, etc. */
> +    nc->disabled_modules = 0x00300395;
> +    nc->num_cpus = 2;
> +}
> +
> +static void npcm750_class_init(ObjectClass *oc, void *data)
> +{
> +    NPCM7xxClass *nc = NPCM7XX_CLASS(oc);
> +
> +    /* NPCM750 has 2 cores and a full set of peripherals */
> +    nc->disabled_modules = 0x00000000;
> +    nc->num_cpus = 2;
> +}

Thanks,

Phil.