[PATCH 0/2] Zynq 7000 SoC improvements

[PATCH 0/2] Zynq 7000 SoC improvements

[Sebastian Huber]

Add support for the cache controller and up to two Cortex-A9 MPCore.

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

 hw/arm/xilinx_zynq.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

-- 
2.35.3

[PATCH 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/xilinx_zynq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8d..078abd77bd 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -242,6 +242,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));
 
-- 
2.35.3

[PATCH 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 | 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];
 
     /* 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");
-- 
2.35.3

Re: [PATCH 0/2] Zynq 7000 SoC improvements

[Sebastian Huber]

Hello,

is the mailing list the right place for contributions like this?

On 07.05.24 15:03, Sebastian Huber wrote:
> Add support for the cache controller and up to two Cortex-A9 MPCore.
> 
> Sebastian Huber (2):
>    hw/arm/xilinx_zynq: Add cache controller
>    hw/arm/xilinx_zynq: Support up to two CPU cores
> 
>   hw/arm/xilinx_zynq.c | 43 ++++++++++++++++++++++++++++---------------
>   1 file changed, 28 insertions(+), 15 deletions(-)
> 

-- 
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 0/2] Zynq 7000 SoC improvements

[Peter Maydell]

On Fri, 17 May 2024 at 09:31, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> Hello,
>
> is the mailing list the right place for contributions like this?

Yes it is, and this is on my todo list to review. Sorry for
not getting back to you earlier, but I was on holiday last
week and at a conference this week. I hope to be able to start
working through my code review backlog when I'm at my desk
again next week :-)

-- PMM

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

[Peter Maydell]

On Tue, 7 May 2024 at 14:04, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> 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/xilinx_zynq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8d..078abd77bd 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -242,6 +242,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));

If we add the cache controller to this board we also need to
update the board's entry in hw/arm/Kconfig to add a
"select PL310" line. This ensures that if the user asks
to build the zynq board type then we will also compile in
the PL310 device.

thanks
-- PMM

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

[Peter Maydell]

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

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

[Sebastian Huber]

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.

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