On Tue, Feb 7, 2023 at 10:46 AM Hao Wu wrote: > Thanks for your review! > > On Mon, Feb 6, 2023 at 11:13 PM Philippe Mathieu-Daudé > wrote: > >> On 7/2/23 00:34, Hao Wu wrote: >> > Nuvoton's PSPI is a general purpose SPI module which enables >> > connections to SPI-based peripheral devices. >> > >> > Signed-off-by: Hao Wu >> > Reviewed-by: Chris Rauer >> > --- >> > MAINTAINERS | 6 +- >> > hw/ssi/meson.build | 2 +- >> > hw/ssi/npcm_pspi.c | 216 +++++++++++++++++++++++++++++++++++++ >> > hw/ssi/trace-events | 5 + >> > include/hw/ssi/npcm_pspi.h | 53 +++++++++ >> > 5 files changed, 278 insertions(+), 4 deletions(-) >> > create mode 100644 hw/ssi/npcm_pspi.c >> > create mode 100644 include/hw/ssi/npcm_pspi.h >> >> >> > +static const MemoryRegionOps npcm_pspi_ctrl_ops = { >> > + .read = npcm_pspi_ctrl_read, >> > + .write = npcm_pspi_ctrl_write, >> > + .endianness = DEVICE_LITTLE_ENDIAN, >> > + .valid = { >> > + .min_access_size = 1, >> > + .max_access_size = 2, >> >> I'm not sure about ".max_access_size = 2". The datasheet does >> not seem public. Does that mean the CPU bus can not do a 32-bit >> access to read two consecutive 16-bit registers? (these fields >> restrict the guest accesses to the device). >> >> > + .unaligned = false, >> > + }, >> >> You might want instead (which is how you implemented the r/w >> handlers): >> >> .impl.min_access_size = 2, >> .impl.max_access_size = 2, >> > Thanks for the reminder. The datasheet suggests it's either 8-bit or > 16-bit accesses. But I think using your suggestion makes sense > and will be more widely adapted. > >> >> > +}; >> >> >> > +static void npcm_pspi_realize(DeviceState *dev, Error **errp) >> > +{ >> > + NPCMPSPIState *s = NPCM_PSPI(dev); >> > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> > + Object *obj = OBJECT(dev); >> > + >> > + s->spi = ssi_create_bus(dev, "pspi"); >> >> FYI there is an ongoing discussion about how to model QOM tree. If >> this bus isn't shared with another controller, the "embed QOM child >> in parent" style could be preferred. If so, the bus would be created >> as: >> >> object_initialize_child(obj, "pspi", &s->spi, TYPE_SSI_BUS); >> > I was just following some existing code here. I think I can use the new > style. > I've tried to use this and got the following error: ** ERROR:../qom/object.c:511:object_initialize_with_type: assertion failed: (size >= type->instance_size) Bail out! ERROR:../qom/object.c:511:object_initialize_with_type: assertion failed: (size >= type->instance_size) I think the problem is that we define s->spi as SSIBus* instead of SSIBus. But if we define it as SSIBus, we'll get an incomplete type error. Fixing it will require refactoring hw/ssi/ssi.c which I'm not sure if we want to do it right now. This code is consistent with other code in hw/ssi so I guess we can leave it here for now and wait for a future refactor. > >> > + memory_region_init_io(&s->mmio, obj, &npcm_pspi_ctrl_ops, s, >> > + "mmio", 4 * KiB); >> > + sysbus_init_mmio(sbd, &s->mmio); >> > + sysbus_init_irq(sbd, &s->irq); >> > +} >> >> >> > diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events >> > index c707d4aaba..16ea9954c4 100644 >> > --- a/hw/ssi/trace-events >> > +++ b/hw/ssi/trace-events >> >> > +# npcm_pspi.c >> > +npcm_pspi_enter_reset(const char *id, int reset_type) "%s reset type: >> %d" >> > +npcm_pspi_ctrl_read(const char *id, uint64_t addr, uint16_t data) "%s >> offset: 0x%04" PRIx64 " value: 0x%08" PRIx16 >> > +npcm_pspi_ctrl_write(const char *id, uint64_t addr, uint16_t data) "%s >> offset: 0x%04" PRIx64 " value: 0x%08" PRIx16 >> >> Since the region is 4KiB and the implementation is 16-bit, the formats >> could be simplified as offset 0x%03 and value 0x%04. The traces will >> then be more digestible to human eyes. >> > I'll do this. > >> >> Modulo the impl.access_size change: >> Reviewed-by: Philippe Mathieu-Daudé >> >>