Re: [PATCH 5/8] hw/ppc/sam460ex: Drop use of ppcuic_init()

[Peter Maydell]

On Sat, 12 Dec 2020 at 17:17, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 12 Dec 2020, Peter Maydell wrote:
> > Switch the sam460ex board to directly creating and configuring the
> > UIC, rather than doing it via the old ppcuic_init() helper function.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > hw/ppc/sam460ex.c | 70 ++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 54 insertions(+), 16 deletions(-)
>
> More than 3 times as much than before, qdev seems to be overly verbose and
> less readable but if that's the preferred way then be it.

Yeah, the boilerplate is sometimes a bit clunky; but the benefits
come from devices all behaving in the same way, being introspectable,
having support for things like VM state save/load, etc.

> > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> > index 14e6583eb0d..9cf7aad3833 100644
> > --- a/hw/ppc/sam460ex.c
> > +++ b/hw/ppc/sam460ex.c
> > @@ -39,6 +39,7 @@
> > #include "hw/usb/hcd-ehci.h"
> > #include "hw/ppc/fdt.h"
> > #include "hw/qdev-properties.h"
> > +#include "hw/intc/ppc-uic.h"
> >
> > #include <libfdt.h>
> >
> > @@ -281,7 +282,6 @@ static void sam460ex_init(MachineState *machine)
> >     hwaddr ram_bases[SDRAM_NR_BANKS] = {0};
> >     hwaddr ram_sizes[SDRAM_NR_BANKS] = {0};
> >     MemoryRegion *l2cache_ram = g_new(MemoryRegion, 1);
> > -    qemu_irq *irqs, *uic[4];
> >     PCIBus *pci_bus;
> >     PowerPCCPU *cpu;
> >     CPUPPCState *env;
> > @@ -293,6 +293,9 @@ static void sam460ex_init(MachineState *machine)
> >     struct boot_info *boot_info;
> >     uint8_t *spd_data;
> >     int success;
> > +    qemu_irq mal_irqs[4];
> > +    DeviceState *uic[4];
> > +    int i;
>
> Maybe keep this where it was above instead of moving to the end and keep
> DeviceState *uic[4]; first so the two others that would be removed later
> are next to each other (just to make patches simpler and keep the order of
> variables which may be roughly as they appear below).

Sure, I can do that.

> >     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> >     env = &cpu->env;
> > @@ -312,13 +315,35 @@ static void sam460ex_init(MachineState *machine)
> >     ppc4xx_plb_init(env);
> >
> >     /* interrupt controllers */
> > -    irqs = g_new0(qemu_irq, PPCUIC_OUTPUT_NB);
> > -    irqs[PPCUIC_OUTPUT_INT] = ((qemu_irq 
> > *)env->irq_inputs)[PPC40x_INPUT_INT];
> > -    irqs[PPCUIC_OUTPUT_CINT] = ((qemu_irq 
> > *)env->irq_inputs)[PPC40x_INPUT_CINT];
>
> Unrelated to this, but I wonder why do we need these casts? Could we just
> define env->irq_inputs as qemu_irq array in the first place? It's now void
> ** which according to the comment next to it may be because once it may
> have been used for different implementations but by now maybe it's only
> used for what its name implies? I haven't checked though if it could be
> cleaned up just raising it if anyone's interested to have a look as there
> are such casts at a lot of other places too.

I mentioned this in the cover letter. The irq_inputs stuff seems
to be an old workaround for not being able to have gpio inputs
to the CPU object. Now that CPUs inherit from TYPE_DEVICE, they
can just create gpio inputs like any other device, and this
code would be able to wire them up without having to dig into
the internals of the CPUPPCState structure.

> > -    uic[0] = ppcuic_init(env, irqs, 0xc0, 0, 1);
> > -    uic[1] = ppcuic_init(env, &uic[0][30], 0xd0, 0, 1);
> > -    uic[2] = ppcuic_init(env, &uic[0][10], 0xe0, 0, 1);
> > -    uic[3] = ppcuic_init(env, &uic[0][16], 0xf0, 0, 1);
> > +    for (i = 0; i < ARRAY_SIZE(uic); i++) {
> > +        SysBusDevice *sbd;
>
> There's already a SysBusDevice *sbdev; defined for similar cases that you
> could reuse here.
>
> > +        /*
> > +         * Number of the first of the two consecutive IRQ inputs on UIC 0
> > +         * to connect the INT and CINT outputs of UIC n to. The entry
>
> This comment confused me a bit, while it's precise is it possible to say
> it in a simpler way? I think these are how uic[1-3] are cascaded through
> uic[0] similar to how the PICs in a PC are cascaded.

Yes, it's the cascading -- it's saying "which inputs on UIC 0 should
UIC n's outputs connect to". What would be a helpful way to phrase
this more clearly ?

> > +         * for UIC 0 is ignored, because it connects to the CPU.
> > +         */
> > +        const int input_ints[] = { -1, 30, 10, 16 };

> >     /* MAL */
> > -    ppc4xx_mal_init(env, 4, 16, &uic[2][3]);
> > +    /*
> > +     * TODO if the MAL were a proper QOM device we would not need to
> > +     * copy its qemu_irqs into an array for ppc4xx_mal_init()'s benefit.
> > +     */
>
> It's not a todo for sam460ex so maybe put it in the file where mal is if
> you want to note it somewhere? (Other sites using the mal may also need
> updating not just this one when this is cleaned up.)

Yeah. I discovered later that one of the other files that creates
the MAL is doing exactly the same thing with a local mal_irqs[]
type array. So I think we could just drop this TODO comment.
As and when somebody QOMifies the MAL device they'll naturally
come back and fix up all the callsites.

thanks
-- PMM