[v2,0/2] Zynq 7000 Improvements

Message

Sebastian Huber May 24, 2024, 12:08 p.m. UTC

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(-)

Comments

Peter Maydell May 30, 2024, 10:30 a.m. UTC | #1

On Fri, 24 May 2024 at 13:08, Sebastian Huber
<[e-mail]> 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

Sebastian Huber June 7, 2024, 2:28 p.m. UTC | #2

On 30.05.24 12:30, Peter Maydell wrote:
> On Fri, 24 May 2024 at 13:08, Sebastian Huber
> <[e-mail]> 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.

Peter Maydell June 7, 2024, 2:47 p.m. UTC | #3

On Fri, 7 Jun 2024 at 15:28, Sebastian Huber
<[e-mail]> wrote:
>
> On 30.05.24 12:30, Peter Maydell wrote:
> > On Fri, 24 May 2024 at 13:08, Sebastian Huber
> > <[e-mail]> 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