Thanks for your review!
On Mon, Feb 6, 2023 at 11:13 PM Philippe Mathieu-Daudé <phi...@linaro.org>
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 <wuhao...@google.com>
> > Reviewed-by: Chris Rauer <cra...@google.com>
> > ---
> > 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.
>
> > + 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é <phi...@linaro.org>
>
>
