| Message ID | 20240507130349.86851-3-sebastian.huber@embedded-brains.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Zynq 7000 SoC improvements | expand |
On Tue, 7 May 2024 at 14:04, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote: > > The Zynq 7000 SoCs contain two Arm Cortex-A9 MPCore (the Zynq 7000S have only > one core). Add support for up to two simulated cores. > > Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> > --- > hw/arm/xilinx_zynq.c | 42 +++++++++++++++++++++++++++--------------- > 1 file changed, 27 insertions(+), 15 deletions(-) > > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c > index 078abd77bd..3b858e3e9a 100644 > --- a/hw/arm/xilinx_zynq.c > +++ b/hw/arm/xilinx_zynq.c > @@ -184,6 +184,8 @@ static void zynq_init(MachineState *machine) > SysBusDevice *busdev; > qemu_irq pic[64]; > int n; > + unsigned int smp_cpus = machine->smp.cpus; > + qemu_irq cpu_irq[2]; We prefer not to have arrays of qemu_irq like this that are just passing qemu_irqs from one place to another. Instead at the point where you want the ARM_CPU_IRQ of a particular CPU, call qdev_get_gpio_in() on the CPU object there. I suggest dropping the "ARMCPU *cpu" local from this function and instead adding an "ARMCPU *cpu[ZYNQ_MAX_CPUS]" array to the ZynqMachineState struct. > /* max 2GB ram */ > if (machine->ram_size > 2 * GiB) { > @@ -191,21 +193,27 @@ static void zynq_init(MachineState *machine) > exit(EXIT_FAILURE); > } > > - cpu = ARM_CPU(object_new(machine->cpu_type)); > + for (n = 0; n < smp_cpus; n++) { > + Object *cpuobj = object_new(machine->cpu_type); > > - /* By default A9 CPUs have EL3 enabled. This board does not > - * currently support EL3 so the CPU EL3 property is disabled before > - * realization. > - */ > - if (object_property_find(OBJECT(cpu), "has_el3")) { > - object_property_set_bool(OBJECT(cpu), "has_el3", false, &error_fatal); > - } > + /* By default A9 CPUs have EL3 enabled. This board does not > + * currently support EL3 so the CPU EL3 property is disabled before > + * realization. > + */ If you're moving comment text around checkpatch will suggest that you fix it up to our current coding standard, which is that a multiline comment has the "/*" on a line of its own. > + if (object_property_find(cpuobj, "has_el3")) { > + object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); > + } > + > + object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, > + &error_fatal); > + object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE, > + &error_fatal); > > - object_property_set_int(OBJECT(cpu), "midr", ZYNQ_BOARD_MIDR, > - &error_fatal); > - object_property_set_int(OBJECT(cpu), "reset-cbar", MPCORE_PERIPHBASE, > - &error_fatal); > - qdev_realize(DEVICE(cpu), NULL, &error_fatal); > + qdev_realize(DEVICE(cpuobj), NULL, &error_fatal); > + > + cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpuobj), ARM_CPU_IRQ); > + } > + cpu = ARM_CPU(first_cpu); > > /* DDR remapped to address zero. */ > memory_region_add_subregion(address_space_mem, 0, machine->ram); > @@ -238,10 +246,14 @@ static void zynq_init(MachineState *machine) > sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000); > > dev = qdev_new(TYPE_A9MPCORE_PRIV); > - qdev_prop_set_uint32(dev, "num-cpu", 1); > + qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); > busdev = SYS_BUS_DEVICE(dev); > sysbus_realize_and_unref(busdev, &error_fatal); > sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE); > + for (n = 0; n < smp_cpus; n++) { > + sysbus_connect_irq(busdev, n, cpu_irq[n]); > + } Looks like you have based this on a version of QEMU which doesn't have commit 68a5827b80117973 which wires up the FIQ line of the A9MPCORE_PRIV device to the CPUs. > + zynq_binfo.gic_cpu_if_addr = MPCORE_PERIPHBASE + 0x100; > sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL); > sysbus_connect_irq(busdev, 0, > qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ)); > @@ -357,7 +369,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data) > MachineClass *mc = MACHINE_CLASS(oc); > mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9"; > mc->init = zynq_init; > - mc->max_cpus = 1; > + mc->max_cpus = 2; > mc->no_sdcard = 1; > mc->ignore_memory_transaction_failures = true; > mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9"); > -- I'm not making this a condition for accepting this patch, but since you're working on this board model would you consider writing up some documentation for it? It's one of the boards we do not currently have documented at all. This doesn't have to be very extensive: a few paragraphs describing what the board type is, maybe linking to a reference to the hardware, listing what is and isn't implemented, and (if there are some convenient examples available) perhaps listing some examples of use. docs/system/arm/xlnx-versal-virt.rst is the docs for the Xilinx Versal board; the rendered version of that is: https://www.qemu.org/docs/master/system/arm/xlnx-versal-virt.html or https://www.qemu.org/docs/master/system/arm/realview.html is an example of a more minimal board documentation page. thanks -- PMM
Hello Peter, thanks for the review. On 20.05.24 15:58, Peter Maydell wrote: > On Tue, 7 May 2024 at 14:04, Sebastian Huber > <sebastian.huber@embedded-brains.de> wrote: >> The Zynq 7000 SoCs contain two Arm Cortex-A9 MPCore (the Zynq 7000S have only >> one core). Add support for up to two simulated cores. >> >> Signed-off-by: Sebastian Huber<sebastian.huber@embedded-brains.de> >> --- >> hw/arm/xilinx_zynq.c | 42 +++++++++++++++++++++++++++--------------- >> 1 file changed, 27 insertions(+), 15 deletions(-) >> >> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c >> index 078abd77bd..3b858e3e9a 100644 >> --- a/hw/arm/xilinx_zynq.c >> +++ b/hw/arm/xilinx_zynq.c >> @@ -184,6 +184,8 @@ static void zynq_init(MachineState *machine) >> SysBusDevice *busdev; >> qemu_irq pic[64]; >> int n; >> + unsigned int smp_cpus = machine->smp.cpus; >> + qemu_irq cpu_irq[2]; > We prefer not to have arrays of qemu_irq like this that are > just passing qemu_irqs from one place to another. Instead > at the point where you want the ARM_CPU_IRQ of a particular > CPU, call qdev_get_gpio_in() on the CPU object there. > > I suggest dropping the "ARMCPU *cpu" local from this function > and instead adding an "ARMCPU *cpu[ZYNQ_MAX_CPUS]" array to > the ZynqMachineState struct. I used the hw/arm/realview.c as a template for this change. I will try to implement the suggested changes. > >> /* max 2GB ram */ >> if (machine->ram_size > 2 * GiB) { >> @@ -191,21 +193,27 @@ static void zynq_init(MachineState *machine) >> exit(EXIT_FAILURE); >> } >> >> - cpu = ARM_CPU(object_new(machine->cpu_type)); >> + for (n = 0; n < smp_cpus; n++) { >> + Object *cpuobj = object_new(machine->cpu_type); >> >> - /* By default A9 CPUs have EL3 enabled. This board does not >> - * currently support EL3 so the CPU EL3 property is disabled before >> - * realization. >> - */ >> - if (object_property_find(OBJECT(cpu), "has_el3")) { >> - object_property_set_bool(OBJECT(cpu), "has_el3", false, &error_fatal); >> - } >> + /* By default A9 CPUs have EL3 enabled. This board does not >> + * currently support EL3 so the CPU EL3 property is disabled before >> + * realization. >> + */ > If you're moving comment text around checkpatch will suggest that > you fix it up to our current coding standard, which is that > a multiline comment has the "/*" on a line of its own. Ok. > >> + if (object_property_find(cpuobj, "has_el3")) { >> + object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); >> + } >> + >> + object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, >> + &error_fatal); >> + object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE, >> + &error_fatal); >> >> - object_property_set_int(OBJECT(cpu), "midr", ZYNQ_BOARD_MIDR, >> - &error_fatal); >> - object_property_set_int(OBJECT(cpu), "reset-cbar", MPCORE_PERIPHBASE, >> - &error_fatal); >> - qdev_realize(DEVICE(cpu), NULL, &error_fatal); >> + qdev_realize(DEVICE(cpuobj), NULL, &error_fatal); >> + >> + cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpuobj), ARM_CPU_IRQ); >> + } >> + cpu = ARM_CPU(first_cpu); >> >> /* DDR remapped to address zero. */ >> memory_region_add_subregion(address_space_mem, 0, machine->ram); >> @@ -238,10 +246,14 @@ static void zynq_init(MachineState *machine) >> sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000); >> >> dev = qdev_new(TYPE_A9MPCORE_PRIV); >> - qdev_prop_set_uint32(dev, "num-cpu", 1); >> + qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); >> busdev = SYS_BUS_DEVICE(dev); >> sysbus_realize_and_unref(busdev, &error_fatal); >> sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE); >> + for (n = 0; n < smp_cpus; n++) { >> + sysbus_connect_irq(busdev, n, cpu_irq[n]); >> + } > Looks like you have based this on a version of QEMU which doesn't > have commit 68a5827b80117973 which wires up the FIQ line of the > A9MPCORE_PRIV device to the CPUs. Yes, indeed. I originally used a Qemu version from Xilinx. They have a huge set of patches which is not integrated in Qemu. > >> + zynq_binfo.gic_cpu_if_addr = MPCORE_PERIPHBASE + 0x100; >> sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL); >> sysbus_connect_irq(busdev, 0, >> qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ)); >> @@ -357,7 +369,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data) >> MachineClass *mc = MACHINE_CLASS(oc); >> mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9"; >> mc->init = zynq_init; >> - mc->max_cpus = 1; >> + mc->max_cpus = 2; >> mc->no_sdcard = 1; >> mc->ignore_memory_transaction_failures = true; >> mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9"); >> -- > I'm not making this a condition for accepting this patch, but > since you're working on this board model would you consider > writing up some documentation for it? It's one of the boards > we do not currently have documented at all. This doesn't have to > be very extensive: a few paragraphs describing what the board > type is, maybe linking to a reference to the hardware, listing > what is and isn't implemented, and (if there are some convenient > examples available) perhaps listing some examples of use. > docs/system/arm/xlnx-versal-virt.rst is the docs for the > Xilinx Versal board; the rendered version of that is: > https://www.qemu.org/docs/master/system/arm/xlnx-versal-virt.html > orhttps://www.qemu.org/docs/master/system/arm/realview.html > is an example of a more minimal board documentation page. Ok, I add this to my TODO list or try to delegate this, but it will take some time.
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c index 078abd77bd..3b858e3e9a 100644 --- a/hw/arm/xilinx_zynq.c +++ b/hw/arm/xilinx_zynq.c @@ -184,6 +184,8 @@ static void zynq_init(MachineState *machine) SysBusDevice *busdev; qemu_irq pic[64]; int n; + unsigned int smp_cpus = machine->smp.cpus; + qemu_irq cpu_irq[2]; /* max 2GB ram */ if (machine->ram_size > 2 * GiB) { @@ -191,21 +193,27 @@ static void zynq_init(MachineState *machine) exit(EXIT_FAILURE); } - cpu = ARM_CPU(object_new(machine->cpu_type)); + for (n = 0; n < smp_cpus; n++) { + Object *cpuobj = object_new(machine->cpu_type); - /* By default A9 CPUs have EL3 enabled. This board does not - * currently support EL3 so the CPU EL3 property is disabled before - * realization. - */ - if (object_property_find(OBJECT(cpu), "has_el3")) { - object_property_set_bool(OBJECT(cpu), "has_el3", false, &error_fatal); - } + /* By default A9 CPUs have EL3 enabled. This board does not + * currently support EL3 so the CPU EL3 property is disabled before + * realization. + */ + if (object_property_find(cpuobj, "has_el3")) { + object_property_set_bool(cpuobj, "has_el3", false, &error_fatal); + } + + object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR, + &error_fatal); + object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE, + &error_fatal); - object_property_set_int(OBJECT(cpu), "midr", ZYNQ_BOARD_MIDR, - &error_fatal); - object_property_set_int(OBJECT(cpu), "reset-cbar", MPCORE_PERIPHBASE, - &error_fatal); - qdev_realize(DEVICE(cpu), NULL, &error_fatal); + qdev_realize(DEVICE(cpuobj), NULL, &error_fatal); + + cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpuobj), ARM_CPU_IRQ); + } + cpu = ARM_CPU(first_cpu); /* DDR remapped to address zero. */ memory_region_add_subregion(address_space_mem, 0, machine->ram); @@ -238,10 +246,14 @@ static void zynq_init(MachineState *machine) sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000); dev = qdev_new(TYPE_A9MPCORE_PRIV); - qdev_prop_set_uint32(dev, "num-cpu", 1); + qdev_prop_set_uint32(dev, "num-cpu", smp_cpus); busdev = SYS_BUS_DEVICE(dev); sysbus_realize_and_unref(busdev, &error_fatal); sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE); + for (n = 0; n < smp_cpus; n++) { + sysbus_connect_irq(busdev, n, cpu_irq[n]); + } + zynq_binfo.gic_cpu_if_addr = MPCORE_PERIPHBASE + 0x100; sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL); sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ)); @@ -357,7 +369,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data) MachineClass *mc = MACHINE_CLASS(oc); mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9"; mc->init = zynq_init; - mc->max_cpus = 1; + mc->max_cpus = 2; mc->no_sdcard = 1; mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
The Zynq 7000 SoCs contain two Arm Cortex-A9 MPCore (the Zynq 7000S have only one core). Add support for up to two simulated cores. Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de> --- hw/arm/xilinx_zynq.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-)