Re: [PATCH v3 3/6] hw/openrisc/openrisc_sim: Use IRQ splitter when connecting UART

[Stafford Horne <shorne@gmail.com>]

On Sun, Feb 20, 2022 at 09:06:41PM +0100, Philippe Mathieu-Daudé wrote:
> On 19/2/22 07:42, Stafford Horne wrote:
> > Currently the OpenRISC SMP configuration only supports 2 cores due to
> > the UART IRQ routing being limited to 2 cores.  As was done in commit
> > 1eeffbeb11 ("hw/openrisc/openrisc_sim: Use IRQ splitter when connecting
> > IRQ to multiple CPUs") we can use a splitter to wire more than 2 CPUs.
> > 
> > This patch moves serial initialization out to it's own function and
> > uses a splitter to connect multiple CPU irq lines to the UART.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   hw/openrisc/openrisc_sim.c | 32 ++++++++++++++++++++++++--------
> >   1 file changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> > index d12b3e0c5e..5bfbac00f8 100644
> > --- a/hw/openrisc/openrisc_sim.c
> > +++ b/hw/openrisc/openrisc_sim.c
> > @@ -137,6 +137,28 @@ static void openrisc_sim_ompic_init(hwaddr base, int num_cpus,
> >       sysbus_mmio_map(s, 0, base);
> >   }
> > +static void openrisc_sim_serial_init(hwaddr base, int num_cpus,
> > +                                     OpenRISCCPU *cpus[], int irq_pin)
> > +{
> > +    qemu_irq serial_irq;
> > +    int i;
> > +
> > +    if (num_cpus > 1) {
> > +        DeviceState *splitter = qdev_new(TYPE_SPLIT_IRQ);
> > +        qdev_prop_set_uint32(splitter, "num-lines", num_cpus);
> > +        qdev_realize_and_unref(splitter, NULL, &error_fatal);
> > +        for (i = 0; i < num_cpus; i++) {
> > +            qdev_connect_gpio_out(splitter, i, get_cpu_irq(cpus, i, irq_pin));
> > +        }
> > +        serial_irq = qdev_get_gpio_in(splitter, 0);
> > +    } else {
> > +        serial_irq = get_cpu_irq(cpus, 0, irq_pin);
> > +    }
> 
> Up to here the code seems a generic helper:
> 
>   or1k_cpus_connect_device(OpenRISCCPU *cpus[],
>                            unsigned num_cpus,
>                            unsigned irq_pin);

Right, this is similar to that used in openrisc_sim_net_init.  I thought about
sharing the code but I didn't think it worth adding helper.

The main reason for me is that openrisc_sim_net_init doesn't expose the qemu_irq
and just does sysbus_connect_irq.  While openrisc_sim_serial_init exposes the
qemu_irq.

I think a generic function would have to look like:

    qemu_irq openrisc_cpus_irq_pin_init(OpenRISCCPU *cpus[],
                                        unsigned num_cpus,
                                        unsigned irq_pin);

I would like to leave this as is for now as.

> > +    serial_mm_init(get_system_memory(), base, 0, serial_irq, 115200,
> > +                   serial_hd(0), DEVICE_NATIVE_ENDIAN);
> 
> This part specific to UART.

Right.

> > +}
> 
> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thank you,

Adding this to the patch as is.

-Stafford