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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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!