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

[Philippe Mathieu-Daudé]

On 5/3/23 08:41, Avi.Fishman@nuvoton.com wrote:
> 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()

Thank you Avi!