Thanks for your review!
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 <wuhaotsh@google.com>
> Reviewed-by: Chris Rauer <crauer@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é <philmd@linaro.org>