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

From: "Avi.Fishman@nuvoton.com" <Avi.Fishman@nuvoton.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Havard Skinnemoen" <hskinnemoen@google.com>,
	"peter.maydell@linaro.org" <peter.maydell@linaro.org>
Cc: "qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"KFTING@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>,
	"KFTING@nuvoton.com" <KFTING@nuvoton.com>
Subject: RE: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model
Date: Sun, 5 Mar 2023 07:41:01 +0000	[thread overview]
Message-ID: <TYZPR03MB5678AF48AA1C86E4C96521A7E4B19@TYZPR03MB5678.apcprd03.prod.outlook.com> (raw)
In-Reply-To: <6ccd925d-b965-4da0-13f2-365bd75abe88@linaro.org>

Hi,

Inside

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Thursday, December 22, 2022 5:03 PM
> To: Havard Skinnemoen <hskinnemoen@google.com>; peter.maydell@linaro.org
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; IS20 Avi Fishman
> <Avi.Fishman@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; Alexander
> Bulekov <alxndr@bu.edu>; Shengtan Mao <stmao@google.com>; Hao Wu
> <wuhaotsh@google.com>; Chris Rauer <crauer@google.com>; CS20 KFTing
> <KFTING@nuvoton.com>
> Subject: Re: [PATCH v9 08/14] hw/nvram: NPCM7xx OTP device model
>
> 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?

It should be little endian, I am not sure why there is a swap here.
Unless the nc->disabled_modules for some reason is coming swapped so we swap it back.

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

So according to what I said then use cpu_to_le32()

>
> > +    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.
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.