On 7/3/20 11:07 PM, Havard Skinnemoen wrote: > On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4...@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.