Re: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Havard Skinnemoen <hskinnemoen@google.com>, peter.maydell@linaro.org
Cc: qemu-arm@nongnu.org, qemu-devel@nongnu.org,
	Avi.Fishman@nuvoton.com, kfting@nuvoton.com,
	Alexander Bulekov <alxndr@bu.edu>,
	Shengtan Mao <stmao@google.com>, Hao Wu <wuhaotsh@google.com>,
	Chris Rauer <crauer@google.com>, Tyrone Ting <kfting@nuvoton.com>
Subject: Re: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model
Date: Thu, 22 Dec 2022 16:03:26 +0100	[thread overview]
Message-ID: <6ccd925d-b965-4da0-13f2-365bd75abe88@linaro.org> (raw)
In-Reply-To: <20200911052101.2602693-9-hskinnemoen@google.com>

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.