Re: [Qemu-devel] [PATCH v1 2/4] hw/riscv/sifive_plic: Use gpios instead of irqs

[Alistair Francis]

On Wed, May 9, 2018 at 7:33 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi Alistair,
>
> On 05/04/2018 05:13 PM, Alistair Francis wrote:
>> Instead of creating the interrupt in lines with qemu_allocate_irq() use
>> qdev_init_gpio_in() as this gives us the ability to use the qdev*gpio*()
>> helpers later on.
>
> This is a good idea, but your patch is incomplete:
> The devices previously using plic->irqs[] now need to use those
> qdev*gpio*() helpers.
>
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>>  hw/riscv/sifive_plic.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
>> index a4ac910ca9..81b6b5245b 100644
>> --- a/hw/riscv/sifive_plic.c
>> +++ b/hw/riscv/sifive_plic.c
>> @@ -431,7 +431,6 @@ static void sifive_plic_irq_request(void *opaque, int 
>> irq, int level)
>>  static void sifive_plic_realize(DeviceState *dev, Error **errp)
>>  {
>>      SiFivePLICState *plic = SIFIVE_PLIC(dev);
>> -    int i;
>>
>>      memory_region_init_io(&plic->mmio, OBJECT(dev), &sifive_plic_ops, plic,
>>                            TYPE_SIFIVE_PLIC, plic->aperture_size);
>> @@ -444,9 +443,7 @@ static void sifive_plic_realize(DeviceState *dev, Error 
>> **errp)
>>      plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs);
>>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio);
>>      plic->irqs = g_new0(qemu_irq, plic->num_sources + 1);
>> -    for (i = 0; i <= plic->num_sources; i++) {
>> -        plic->irqs[i] = qemu_allocate_irq(sifive_plic_irq_request, plic, i);
>> -    }
>> +    qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources);
>>  }
>>
>>  static void sifive_plic_class_init(ObjectClass *klass, void *data)
>>
>
> The following patch completes yours:
>
> - access the irqs with qdev_get_gpio_in()
> - remove plic->irqs[] alloc in sifive_plic_realize()
> - remove plic->irqs[] from SiFivePLICState struct

Thanks for that.

I realised the same thing. This series is also missing some device
tree changes that are required. I'll fix all of that in the next
version.

Alistair

>
> -- >8 --
>  include/hw/riscv/sifive_plic.h | 1 -
>  hw/riscv/sifive_e.c            | 2 +-
>  hw/riscv/sifive_plic.c         | 1 -
>  hw/riscv/sifive_u.c            | 2 +-
>  hw/riscv/virt.c                | 4 ++--
>  5 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/riscv/sifive_plic.h b/include/hw/riscv/sifive_plic.h
> index 11a5a98df1..2f2af7e686 100644
> --- a/include/hw/riscv/sifive_plic.h
> +++ b/include/hw/riscv/sifive_plic.h
> @@ -56,7 +56,6 @@ typedef struct SiFivePLICState {
>      uint32_t *claimed;
>      uint32_t *enable;
>      QemuMutex lock;
> -    qemu_irq *irqs;
>
>      /* config */
>      char *hart_config;
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index 0ab5e3ca45..7b4f04adcf 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -190,7 +190,7 @@ static void riscv_sifive_e31_realize(DeviceState
> *dev, Error **errp)
>      sifive_mmio_emulate(sys_mem, "riscv.sifive.e.gpio0",
>          memmap[SIFIVE_E_GPIO0].base, memmap[SIFIVE_E_GPIO0].size);
>      sifive_uart_create(sys_mem, memmap[SIFIVE_E_UART0].base,
> -        serial_hd(0), SIFIVE_PLIC(s->plic)->irqs[SIFIVE_E_UART0_IRQ]);
> +        serial_hd(0), qdev_get_gpio_in(DEVICE(s->plic),
> SIFIVE_E_UART0_IRQ));
>      sifive_mmio_emulate(sys_mem, "riscv.sifive.e.qspi0",
>          memmap[SIFIVE_E_QSPI0].base, memmap[SIFIVE_E_QSPI0].size);
>      sifive_mmio_emulate(sys_mem, "riscv.sifive.e.pwm0",
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index c6ad17057d..a91aeb97ab 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -447,7 +447,6 @@ static void sifive_plic_realize(DeviceState *dev,
> Error **errp)
>      plic->claimed = g_new0(uint32_t, plic->bitfield_words);
>      plic->enable = g_new0(uint32_t, plic->bitfield_words *
> plic->num_addrs);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio);
> -    plic->irqs = g_new0(qemu_irq, plic->num_sources + 1);
>      qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources);
>  }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 54f33c39ae..b7936dcec5 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -330,7 +330,7 @@ static void riscv_sifive_u54_realize(DeviceState
> *dev, Error **errp)
>          SIFIVE_U_PLIC_CONTEXT_STRIDE,
>          memmap[SIFIVE_U_PLIC].size);
>      sifive_uart_create(system_memory, memmap[SIFIVE_U_UART0].base,
> -        serial_hd(0), SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART0_IRQ]);
> +        serial_hd(0), qdev_get_gpio_in(DEVICE(s->plic),
> SIFIVE_U_UART0_IRQ));
>      /* sifive_uart_create(system_memory, memmap[SIFIVE_U_UART1].base,
>          serial_hd(1), SIFIVE_PLIC(s->plic)->irqs[SIFIVE_U_UART1_IRQ]); */
>      sifive_clint_create(memmap[SIFIVE_U_CLINT].base,
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ad03113e0f..bdd75722eb 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -379,11 +379,11 @@ static void riscv_virt_board_init(MachineState
> *machine)
>      for (i = 0; i < VIRTIO_COUNT; i++) {
>          sysbus_create_simple("virtio-mmio",
>              memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size,
> -            SIFIVE_PLIC(s->plic)->irqs[VIRTIO_IRQ + i]);
> +            qdev_get_gpio_in(DEVICE(s->plic), VIRTIO_IRQ + i));
>      }
>
>      serial_mm_init(system_memory, memmap[VIRT_UART0].base,
> -        0, SIFIVE_PLIC(s->plic)->irqs[UART0_IRQ], 399193,
> +        0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
>          serial_hd(0), DEVICE_LITTLE_ENDIAN);
>  }
>
> --
>
> Regards,
>
> Phil.
>