[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 08/12] hw/nvram: NPCM7xx OTP device model
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v3 08/12] hw/nvram: NPCM7xx OTP device model |
Date: |
Sat, 4 Jul 2020 00:24:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 7/3/20 11:07 PM, Havard Skinnemoen wrote:
> On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
>>> +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
>>> + NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
>>> + SysBusDevice *sbd = &s->parent;
>>> +
>>> + s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
>>
>> The possibility to reuse persistent OTP seems important.
>> This can be added later of course.
>
> Agree, it's an important part of the behavior of the module. But it's
> not essential to be able to boot a BMC firmware image, so I left it
> out initially.
This is not a problem until someone else try to use it. And OTP/NVRAM
are devices currently in hype. Meanwhile we have another technical debt.
I am just giving generic review comments.
What bugs me particularly is the OTP content zeroed at reset without
any warning. Maybe you can add something like:
qemu_log_mask(LOG_UNIMP,
"Persistence not supported, OTP content erased!\n");
in npcm7xx_otp_enter_reset().
>
>> See simple example in hw/nvram/eeprom_at24c.c which use
>> a BlockBackend so the OTP content is not lost after reset
>> or migration.
>
> I'll take a look at that, thanks!
>
>>> +
>>> + memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
>>> + NPCM7XX_OTP_REGS_SIZE);
>>> + sysbus_init_mmio(sbd, &s->mmio);
>>> +}
>>> +
>>> +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + ResettableClass *rc = RESETTABLE_CLASS(klass);
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>
>> Missing migration vmstate for NPCM7xxOTPState::regs[].
>
> Ah, you're right. This is probably true for most of the peripherals in
> this series. I'll see if I can get it sorted out for the next
> iteration.
IIUC Peter is ready to queue your series on Monday if you fix the
license, but he usually doesn't accept devices without vmstate via
his tree, you got lucky ;) See:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg643089.html
Adding the vmstate is not a hard task. If you need to respin,
I strongly recommend you to consider adding them.
Excellent work btw :)
Regards,
Phil.