On Sat, 19 Mar 2022 at 19:32, Zongyuan Li <zongyuan...@smartx.com> wrote:
>
> Signed-off-by: Zongyuan Li <zongyuan...@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
