[PATCH v2 0/2] Zynq 7000 Improvements

[PATCH v2 0/2] Zynq 7000 Improvements

[Sebastian Huber]

v2:

* Add Kconfig support

* Add array of CPUs to ZynqMachineState

* Add FIQ support

Sebastian Huber (2):
  hw/arm/xilinx_zynq: Add cache controller
  hw/arm/xilinx_zynq: Support up to two CPU cores

 hw/arm/Kconfig       |  1 +
 hw/arm/xilinx_zynq.c | 55 +++++++++++++++++++++++++++-----------------
 2 files changed, 35 insertions(+), 21 deletions(-)

-- 
2.35.3

[PATCH v2 1/2] hw/arm/xilinx_zynq: Add cache controller

[Sebastian Huber]

The Zynq 7000 SoCs contain a CoreLink L2C-310 cache controller.  Add the
corresponding Qemu device to the xilinx-zynq-a9 machine.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 hw/arm/Kconfig       | 1 +
 hw/arm/xilinx_zynq.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 8b97683a45..1ad60da7aa 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -370,6 +370,7 @@ config ZYNQ
     select A9MPCORE
     select CADENCE # UART
     select PFLASH_CFI02
+    select PL310 # cache controller
     select PL330
     select SDHCI
     select SSI_M25P80
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index fc3abcbe88..0abb62f131 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -241,6 +241,7 @@ static void zynq_init(MachineState *machine)
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(busdev, &error_fatal);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
+    sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL);
     sysbus_connect_irq(busdev, 0,
                        qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
     sysbus_connect_irq(busdev, 1,
-- 
2.35.3

[PATCH v2 2/2] hw/arm/xilinx_zynq: Support up to two CPU cores

[Sebastian Huber]

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 | 54 +++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 0abb62f131..ac30026040 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -84,9 +84,12 @@ static const int dma_irqs[8] = {
     0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
     0xe5801000 + (addr)
 
+#define ZYNQ_MAX_CPUS 2
+
 struct ZynqMachineState {
     MachineState parent;
     Clock *ps_clk;
+    ARMCPU *cpu[ZYNQ_MAX_CPUS];
 };
 
 static void zynq_write_board_setup(ARMCPU *cpu,
@@ -176,13 +179,13 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
 static void zynq_init(MachineState *machine)
 {
     ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
-    ARMCPU *cpu;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
     DeviceState *dev, *slcr;
     SysBusDevice *busdev;
     qemu_irq pic[64];
     int n;
+    unsigned int smp_cpus = machine->smp.cpus;
 
     /* max 2GB ram */
     if (machine->ram_size > 2 * GiB) {
@@ -190,21 +193,26 @@ 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(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);
+        object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR,
+                                &error_fatal);
+        object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE,
+                                &error_fatal);
+
+        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
+
+        zynq_machine->cpu[n] = ARM_CPU(cpuobj);
+    }
 
     /* DDR remapped to address zero.  */
     memory_region_add_subregion(address_space_mem, 0, machine->ram);
@@ -237,15 +245,19 @@ 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);
+    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));
-    sysbus_connect_irq(busdev, 1,
-                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_FIQ));
+    for (n = 0; n < smp_cpus; n++) {
+        DeviceState *cpudev = DEVICE(OBJECT(zynq_machine->cpu[n]));
+        sysbus_connect_irq(busdev, (2 * n) + 0,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+        sysbus_connect_irq(busdev, (2 * n) + 1,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
+    }
 
     for (n = 0; n < 64; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
@@ -350,7 +362,7 @@ static void zynq_init(MachineState *machine)
     zynq_binfo.board_setup_addr = BOARD_SETUP_ADDR;
     zynq_binfo.write_board_setup = zynq_write_board_setup;
 
-    arm_load_kernel(cpu, machine, &zynq_binfo);
+    arm_load_kernel(zynq_machine->cpu[0], machine, &zynq_binfo);
 }
 
 static void zynq_machine_class_init(ObjectClass *oc, void *data)
@@ -362,7 +374,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 = ZYNQ_MAX_CPUS;
     mc->no_sdcard = 1;
     mc->ignore_memory_transaction_failures = true;
     mc->valid_cpu_types = valid_cpu_types;
-- 
2.35.3

Re: [PATCH v2 2/2] hw/arm/xilinx_zynq: Support up to two CPU cores

[Peter Maydell]

On Fri, 24 May 2024 at 13:08, 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>

> +        DeviceState *cpudev = DEVICE(OBJECT(zynq_machine->cpu[n]));

You don't need to do a double-cast like this, you can say:

   DeviceState *cpudev = DEVICE(zynq_machine->cpu[n]);

(I'll make this minor fix when I apply the patchset to target-arm.)

-- PMM

Re: [PATCH v2 0/2] Zynq 7000 Improvements

[Peter Maydell]

On Fri, 24 May 2024 at 13:08, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> v2:
>
> * Add Kconfig support
>
> * Add array of CPUs to ZynqMachineState
>
> * Add FIQ support
>
> Sebastian Huber (2):
>   hw/arm/xilinx_zynq: Add cache controller
>   hw/arm/xilinx_zynq: Support up to two CPU cores



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH v2 0/2] Zynq 7000 Improvements

[Sebastian Huber]

On 30.05.24 12:30, Peter Maydell wrote:
> On Fri, 24 May 2024 at 13:08, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>>
>> v2:
>>
>> * Add Kconfig support
>>
>> * Add array of CPUs to ZynqMachineState
>>
>> * Add FIQ support
>>
>> Sebastian Huber (2):
>>    hw/arm/xilinx_zynq: Add cache controller
>>    hw/arm/xilinx_zynq: Support up to two CPU cores
> 
> 
> 
> Applied to target-arm.next, thanks.

Thanks, for the integration. I did some more tests and there is an issue 
with the IPI support. My test case worked with this change:

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 7f7a3d23fb..8d84eaf070 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -253,9 +253,9 @@ static void zynq_init(MachineState *machine)
      sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL);
      for (n = 0; n < smp_cpus; n++) {
          DeviceState *cpudev = DEVICE(zynq_machine->cpu[n]);
-        sysbus_connect_irq(busdev, (2 * n) + 0,
+        sysbus_connect_irq(busdev, n,
                             qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
-        sysbus_connect_irq(busdev, (2 * n) + 1,
+        sysbus_connect_irq(busdev, smp_cpus + n,
                             qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
      }

This is just guess work on my side since I have no idea how the second 
parameter relates to the pin.

Originally I used the hw/arm/realview.c as a reference, but his target 
doesn't use the ARM_CPU_FIQ at all.

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH v2 0/2] Zynq 7000 Improvements

[Peter Maydell]

On Fri, 7 Jun 2024 at 15:28, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> On 30.05.24 12:30, Peter Maydell wrote:
> > On Fri, 24 May 2024 at 13:08, Sebastian Huber
> > <sebastian.huber@embedded-brains.de> wrote:
> >>
> >> v2:
> >>
> >> * Add Kconfig support
> >>
> >> * Add array of CPUs to ZynqMachineState
> >>
> >> * Add FIQ support
> >>
> >> Sebastian Huber (2):
> >>    hw/arm/xilinx_zynq: Add cache controller
> >>    hw/arm/xilinx_zynq: Support up to two CPU cores
> >
> >
> >
> > Applied to target-arm.next, thanks.
>
> Thanks, for the integration. I did some more tests and there is an issue
> with the IPI support. My test case worked with this change:
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 7f7a3d23fb..8d84eaf070 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -253,9 +253,9 @@ static void zynq_init(MachineState *machine)
>       sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL);
>       for (n = 0; n < smp_cpus; n++) {
>           DeviceState *cpudev = DEVICE(zynq_machine->cpu[n]);
> -        sysbus_connect_irq(busdev, (2 * n) + 0,
> +        sysbus_connect_irq(busdev, n,
>                              qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> -        sysbus_connect_irq(busdev, (2 * n) + 1,
> +        sysbus_connect_irq(busdev, smp_cpus + n,
>                              qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
>       }
>
> This is just guess work on my side since I have no idea how the second
> parameter relates to the pin.
>
> Originally I used the hw/arm/realview.c as a reference, but his target
> doesn't use the ARM_CPU_FIQ at all.

(I suspect it's a bug that we don't wire up FIQ on realview,
but Linux doesn't care as it only uses IRQ anyway.)

The a9mpcore device's inbound IRQ lines are a passthrough of
the IRQ inputs to the GICv2 device. We don't document that
that's what a9mpcore does, unfortunately, but we do document
the GICv2 layout in include/hw/intc/arm_gic.h:

 *  + sysbus IRQs: (in order; number will vary depending on number of cores)
 *    - IRQ for CPU 0
 *    - IRQ for CPU 1
 *      ...
 *    - FIQ for CPU 0
 *    - FIQ for CPU 1
 *      ...
 *    - VIRQ for CPU 0 (exists even if virt extensions not present)
 *    - VIRQ for CPU 1 (exists even if virt extensions not present)
 *      ...
 *    - VFIQ for CPU 0 (exists even if virt extensions not present)
 *    - VFIQ for CPU 1 (exists even if virt extensions not present)
 *      ...
 *    - maintenance IRQ for CPU i/f 0 (only if virt extensions present)
 *    - maintenance IRQ for CPU i/f 1 (only if virt extensions present)

So yes, your change here is correct.

The original patch is in upstream git already, so would you
mind sending this fix as a proper patch email? Then I can
pick it up as a bugfix.

thanks
-- PMM