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

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

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);
>> +}