On Tue, Feb 7, 2023 at 10:46 AM Hao Wu <wuhao...@google.com> wrote:
> 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.
>
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é <phi...@linaro.org>
>>
>>
