[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 5/8] hw/ppc/sam460ex: Drop use of ppcuic_init()
Date: Sat, 12 Dec 2020 21:35:02 +0100 (CET)

On Sat, 12 Dec 2020, Peter Maydell wrote:
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.

And disadvantage is that a typo can easier hide in there as we've just seen. Recent changes to simplify object creation did improve boiler plate somewhat but gpios still seem to be a bit obscure and hard to use so maybe if somebody has some idea to improve it that could help. I have no idea how it could be made simpler though. Maybe less verbose names or some helpers/macros for common ops that hide the verbosity could help readability.

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)

    /* 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 

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.

Yes, noticed that after I've answered this.

-    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 ?

Hah, you're the native English speaker so I hoped you could reformulate it in a simpler way. (But maybe that's what makes it more difficult because the current version already makes perfect sense to you.) Maybe something like "Interrupt numbers in uic[0] where INT outputs of uic[1]-uic[3] are connected for cascading. The CINT output is connected to the next interrupt number. The entry for uic[0] is ignored because it connects to the CPU."


+         * 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.

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]