Re: [PATCH v2 1/2] hw/ssi: Add Ibex SPI device model

[Philippe Mathieu-Daudé]

Hi Wilfred,

On 28/2/22 10:13, Alistair Francis wrote:
> On Mon, Feb 28, 2022 at 1:41 PM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> >
> > Adds the SPI_HOST device model for ibex. The device specification is as per
> > [1]. The model has been tested on opentitan with spi_host unit tests
> > written for TockOS.
> >
> > [1] https://docs.opentitan.org/hw/ip/spi_host/doc/
> >
> > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > ---
> >   hw/ssi/ibex_spi_host.c         | 613 +++++++++++++++++++++++++++++++++
> >   hw/ssi/meson.build             |   1 +
> >   hw/ssi/trace-events            |   7 +
> >   include/hw/ssi/ibex_spi_host.h |  94 +++++
> >   4 files changed, 715 insertions(+)
> >   create mode 100644 hw/ssi/ibex_spi_host.c
> >   create mode 100644 include/hw/ssi/ibex_spi_host.h

> > +static void ibex_spi_host_realize(DeviceState *dev, Error **errp)
> > +{
> > +    IbexSPIHostState *s = IBEX_SPI_HOST(dev);
> > +    int i;
> > +
> > +    s->ssi = ssi_create_bus(dev, "ssi");
> > +    s->cs_lines = g_new0(qemu_irq, s->num_cs);
> > +
> > +    for (i = 0; i < s->num_cs; ++i) {
> > +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
> > +    }
> > +
> > +    /* Setup CONFIGOPTS Multi-register */
> > +    s->config_opts = malloc(sizeof(uint32_t) * s->num_cs);

This array is not zeroed. Clearer as:

        s->config_opts = g_new0(uint32_t, s->num_cs);

> > +
> > +    /* Setup FIFO Interrupt Timer */
> > +    s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > +                                          fifo_trigger_update, s);
> > +
> > +    /* FIFO sizes as per OT Spec */
> > +    fifo8_create(&s->tx_fifo, IBEX_SPI_HOST_TXFIFO_LEN);
> > +    fifo8_create(&s->rx_fifo, IBEX_SPI_HOST_RXFIFO_LEN);
> > +}
> > +
> > +static void ibex_spi_host_init(Object *obj)
> > +{
> > +    IbexSPIHostState *s = IBEX_SPI_HOST(obj);
> > +
> > +    s->init_status = true;

What about reset?

> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->host_err);
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->event);
> > +
> > +    memory_region_init_io(&s->mmio, obj, &ibex_spi_ops, s,
> > +                          TYPE_IBEX_SPI_HOST, 0x1000);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> > +}