Re: [RFC PATCH v1 1/1] hw/arm/realview: replace 'qemu_split_irq' with 'TYPE_SPLIT_IRQ'

[Peter Maydell]

On Sat, 19 Mar 2022 at 19:32, Zongyuan Li <zongyuan.li@smartx.com> wrote:
>
> Signed-off-by: Zongyuan Li <zongyuan.li@smartx.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/811

Thanks for this patch.

> +#include "hw/qdev-core.h"
>  #include "qemu/osdep.h"

The osdep.h include must always be first in every .c file.
Put new #include lines below it, not above it.

>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/boot.h"
>  #include "hw/arm/primecell.h"
> +#include "hw/core/split-irq.h"
>  #include "hw/net/lan9118.h"
>  #include "hw/net/smc91c111.h"
>  #include "hw/pci/pci.h"
>  #include "net/net.h"
> +#include "qom/object.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/boards.h"
>  #include "hw/i2c/i2c.h"
> @@ -27,6 +30,7 @@
>  #include "hw/irq.h"
>  #include "hw/i2c/arm_sbcon_i2c.h"
>  #include "hw/sd/sd.h"
> +#include <stdarg.h>

This include should not be necessary -- osdep.h provides it for you.

>
>  #define SMP_BOOT_ADDR 0xe0000000
>  #define SMP_BOOTREG_ADDR 0x10000030
> @@ -53,6 +57,30 @@ static const int realview_board_id[] = {
>      0x76d
>  };
>
> +static bool split_to_irq_varargs(Object *obj, int nums_of_output, ...) {
> +    int i;
> +    va_list va;
> +    qemu_irq output;
> +    Object *src_irq= object_new(TYPE_SPLIT_IRQ);
> +
> +    if (!object_property_set_int(src_irq, "num-lines", nums_of_output, 
> &error_fatal)) {
> +        return false;
> +    }
> +
> +    if (!qdev_realize(DEVICE(src_irq), NULL, &error_fatal)) {
> +        return false;
> +    }

In board code, prefer to create this device with
qdev_new() and then realize it with
qdev_realize_and_unref(). There's an example in
hw/openrisc/openrisc_sim.c. (NB: for splitters and other
devices created in SoC device objects rather than board code,
the pattern will be different.)

> +
> +    va_start(va, nums_of_output);
> +    for (i = 0; i < nums_of_output; i++) {
> +        output = va_arg(va, qemu_irq);
> +        qdev_connect_gpio_out(DEVICE(src_irq), i, output);
> +    }
> +       va_end(va);
> +
> +    return true;
> +}

I think this varargs function is unnecessarily complicated. We only
create two split devices here, and they are always with a fixed
number of output. Just write the "create splitter and wire it up"
code directly. Again, the openrisc_sim.c code is a good example.

> +
>  static void realview_init(MachineState *machine,
>                            enum realview_board_type board_type)
>  {
> @@ -67,6 +95,8 @@ static void realview_init(MachineState *machine,
>      SysBusDevice *busdev;
>      qemu_irq pic[64];
>      qemu_irq mmc_irq[2];
> +    Object *mmc_irq_for_ro = object_new(TYPE_SPLIT_IRQ);
> +    Object *mmc_irq_for_cardin = object_new(TYPE_SPLIT_IRQ);

You seem to be creating the splitter objects both here and also
in your split_to_irq_varargs() function...

>      PCIBus *pci_bus = NULL;
>      NICInfo *nd;
>      DriveInfo *dinfo;
> @@ -229,14 +259,20 @@ static void realview_init(MachineState *machine,
>       * and the PL061 has them the other way about. Also the card
>       * detect line is inverted.
>       */
> -    mmc_irq[0] = qemu_irq_split(
> -        qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT),
> -        qdev_get_gpio_in(gpio2, 1));
> -    mmc_irq[1] = qemu_irq_split(
> -        qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN),
> -        qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
> -    qdev_connect_gpio_out_named(dev, "card-read-only", 0, mmc_irq[0]);
> -    qdev_connect_gpio_out_named(dev, "card-inserted", 0, mmc_irq[1]);
> +    if (!split_to_irq_varargs(mmc_irq_for_ro, 2,
> +                              qdev_get_gpio_in(sysctl, 
> ARM_SYSCTL_GPIO_MMC_WPROT),
> +                              qdev_get_gpio_in(gpio2, 1))) {
> +        return;
> +    }
> +    qdev_connect_gpio_out_named(dev, "card-read-only", 0, 
> DEVICE(mmc_irq_for_ro));
> +
> +    if (!split_to_irq_varargs(mmc_irq_for_cardin, 2,
> +                              qdev_get_gpio_in(sysctl, 
> ARM_SYSCTL_GPIO_MMC_CARDIN),
> +                              qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)))) {
> +        return;
> +    }
> +    qdev_connect_gpio_out_named(dev, "card-inserted", 0, 
> DEVICE(mmc_irq_for_cardin));
> +
>      dinfo = drive_get(IF_SD, 0, 0);
>      if (dinfo) {
>          DeviceState *card;
> --
> 2.34.0

thanks
-- PMM