Reset and Halting modifications + Zynq SMP

PATCH Superseded Peter Crosthwaite 3 participants 10 messages Jan 2, 2014 – Jan 15, 2014 Arm/Zynq architecture

lore

Share & tools

lore

Peter Crosthwaite

author

2014-01-02 07:31 UTC #0 lore raw

v4 cover Reset and Halting modifications + Zynq SMP

Hi All,

The clock controller module in the Zynq platform has the ability to halt and reset arbitrary devices, including the CPU. We use this feature to implement SMP Linux - the kernel halts CPU1 then rewrites the vector table to the secondary entry point and the resets+unhalts.

This series adds SMP support to the Zynq machine, and patches the Zynq SLCR (the clock controller) to have DeviceState pointers to the CPUs. cpu_reset() is then called on the appropriate register access.

Only the reset side is implemented (which is good enough for SMP linux as it stands). Future work is to implement the halting behaviour as well.

changed since v3:
Removed halting patches
Reduced to minimal change needed for SMP Zynq

Peter Crosthwaite (3):
xilinx_zynq: added SMP support:
zynq_slcr: Add links to the CPUs
zynq_slcr: Implement CPU reset

 hw/arm/xilinx_zynq.c | 73 ++++++++++++++++++++++++++++++++++++++++------------
 hw/misc/zynq_slcr.c  | 23 +++++++++++++++++
 2 files changed, 80 insertions(+), 16 deletions(-)

Signature · 1 line

1.8.5.2

Peter Crosthwaite

author

2014-01-02 07:31 UTC #1 lore raw

PATCH v4 1/3 xilinx_zynq: added SMP support:

Added Linux SMP support for the Xilinx Zynq platform (2x CPUs are supported)

Signed-off-by Peter Crosthwaite <[email protected]>

---
Changed from v3:
Author reset
s/zynq_cpus/cpus
simplified custom secondary bootloader Rebased
Changed from v2:
macro defined the maximum number of CPUS Changed from v1:
Addressed PMM review
Shorted secondary bootloop using MVN instruction. Used default reset secondary instead of custom one. Rebased against QOM cpu developments.
Few whitespace fixes.

 hw/arm/xilinx_zynq.c | 69 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 16 deletions(-)

hw/arm/xilinx_zynq.c +53 ~~-16~~

index 17251c7..c09ff36 100644
@@ -27,6 +27,8 @@
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
 
+#define MAX_CPUS 2
+
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
 #define NUM_QSPI_BUSSES 2

@@ -38,10 +40,37 @@
 
 #define MPCORE_PERIPHBASE 0xF8F00000
 
+/* Dummy bootreg addr to keep ARM bootloader happy. Very top of OCM */
+#define SMP_BOOTREG_ADDR 0xfffffffc
+/* Put SMP bootloader up top of OCM under bootreg  */
+#define SMP_BOOT_ADDR (SMP_BOOTREG_ADDR - sizeof(zynq_smpboot))
+
 static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 
+/* Entry point for secondary CPU. Zynq Linux SMP protocol is to just reset
+ * the secondary to unpen, so any infinite loop will do the trick. Use a WFI
+ * loop as that will cause the emulated CPU to halt (and remove itself from
+ * the work queue pending an interrupt that never comes).
+ */
+static uint32_t zynq_smpboot[] = {
+    0xe320f003, /* wfi */
+    0xeafffffd, /* b       <b wfi> */
+};
+
+static void zynq_write_secondary_boot(ARMCPU *cpu,
+                                      const struct arm_boot_info *info)
+{
+    int n;
+
+    for (n = 0; n < ARRAY_SIZE(zynq_smpboot); n++) {
+        zynq_smpboot[n] = tswap32(zynq_smpboot[n]);
+    }
+    rom_add_blob_fixed("smpboot", zynq_smpboot, sizeof(zynq_smpboot),
+                       SMP_BOOT_ADDR);
+}
+
 static struct arm_boot_info zynq_binfo = {};
 
 static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)

@@ -106,7 +135,7 @@ static void zynq_init(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     ObjectClass *cpu_oc;
-    ARMCPU *cpu;
+    ARMCPU *cpu[MAX_CPUS];
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);

@@ -122,17 +151,20 @@ static void zynq_init(QEMUMachineInitArgs *args)
     }
     cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
 
-    cpu = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
+    for (n = 0; n < smp_cpus; n++) {
+        cpu[n] = ARM_CPU(object_new(object_class_get_name(cpu_oc)));
 
-    object_property_set_int(OBJECT(cpu), MPCORE_PERIPHBASE, "reset-cbar", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
-    }
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-    if (err) {
-        error_report("%s", error_get_pretty(err));
-        exit(1);
+        object_property_set_int(OBJECT(cpu[n]), MPCORE_PERIPHBASE, "reset-cbar",
+                                &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
+        object_property_set_bool(OBJECT(cpu[n]), true, "realized", &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            exit(1);
+        }
     }
 
     /* max 2GB ram */

@@ -164,12 +196,14 @@ static void zynq_init(QEMUMachineInitArgs *args)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
 
     dev = qdev_create(NULL, "a9mpcore_priv");
-    qdev_prop_set_uint32(dev, "num-cpu", 1);
+    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
     qdev_init_nofail(dev);
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
-    sysbus_connect_irq(busdev, 0,
-                       qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
+    for (n = 0; n < smp_cpus; n++) {
+        sysbus_connect_irq(busdev, n,
+                           qdev_get_gpio_in(DEVICE(cpu[n]), ARM_CPU_IRQ));
+    }
 
     for (n = 0; n < 64; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);

@@ -233,7 +267,10 @@ static void zynq_init(QEMUMachineInitArgs *args)
     zynq_binfo.kernel_filename = kernel_filename;
     zynq_binfo.kernel_cmdline = kernel_cmdline;
     zynq_binfo.initrd_filename = initrd_filename;
-    zynq_binfo.nb_cpus = 1;
+    zynq_binfo.nb_cpus = smp_cpus;
+    zynq_binfo.write_secondary_boot = zynq_write_secondary_boot;
+    zynq_binfo.smp_loader_start = SMP_BOOT_ADDR;
+    zynq_binfo.smp_bootreg_addr = SMP_BOOTREG_ADDR;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);

@@ -244,7 +281,7 @@ static QEMUMachine zynq_machine = {
     .desc = "Xilinx Zynq Platform Baseboard for Cortex-A9",
     .init = zynq_init,
     .block_default_type = IF_SCSI,
-    .max_cpus = 1,
+    .max_cpus = MAX_CPUS,
     .no_sdcard = 1,
 };

Signature · 1 line

1.8.5.2

Peter Maydell

2014-01-10 18:08 UTC #2 lore raw

Re: [Qemu-devel] [PATCH target-arm v4 1/3] xilinx_zynq: added SMP support:

Quoted 36 lines · Peter Crosthwaite

Added Linux SMP support for the Xilinx Zynq platform (2x CPUs are supported)
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed from v3:
Author reset
s/zynq_cpus/cpus
simplified custom secondary bootloader Rebased
Changed from v2:
macro defined the maximum number of CPUS Changed from v1:
Addressed PMM review
Shorted secondary bootloop using MVN instruction. Used default reset secondary instead of custom one. Rebased against QOM cpu developments.
Few whitespace fixes.
 hw/arm/xilinx_zynq.c | 69 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 16 deletions(-)
hw/arm/xilinx_zynq.c +2 -0
index 17251c7..c09ff36 100644
@@ -27,6 +27,8 @@
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
 
+#define MAX_CPUS 2
+
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
 #define NUM_QSPI_BUSSES 2

hw/arm/xilinx_zynq.c line 44

Quoted 5 lines

hw/arm/xilinx_zynq.c +2 -0

@@ -38,10 +40,37 @@
 
 #define MPCORE_PERIPHBASE 0xF8F00000
 
+/* Dummy bootreg addr to keep ARM bootloader happy. Very top of OCM */
+#define SMP_BOOTREG_ADDR 0xfffffffc

It would probably be nicer to provide your own reset_secondary hook, and then hw/arm/boot.c won't ever look at what you set in bootreg_addr.

Looks ok otherwise, though.

thanks
-- PMM

Peter Crosthwaite

author

2014-01-15 06:47 UTC #3 lore raw

Re: [Qemu-devel] [PATCH target-arm v4 1/3] xilinx_zynq: added SMP support:

Quoted 38 lines · Peter Maydell

Quoted 37 lines · Peter Crosthwaite
Added Linux SMP support for the Xilinx Zynq platform (2x CPUs are supported)
Signed-off-by Peter Crosthwaite <[email protected]>
---
Changed from v3:
Author reset
s/zynq_cpus/cpus
simplified custom secondary bootloader Rebased
Changed from v2:
macro defined the maximum number of CPUS Changed from v1:
Addressed PMM review
Shorted secondary bootloop using MVN instruction. Used default reset secondary instead of custom one. Rebased against QOM cpu developments.
Few whitespace fixes.
 hw/arm/xilinx_zynq.c | 69 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 16 deletions(-)
hw/arm/xilinx_zynq.c +2 -0
index 17251c7..c09ff36 100644
@@ -27,6 +27,8 @@
 #include "hw/ssi.h"
 #include "qemu/error-report.h"
 
+#define MAX_CPUS 2
+
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
 #define NUM_QSPI_BUSSES 2
@@ -38,10 +40,37 @@
 

Quoted 9 lines

hw/arm/xilinx_zynq.c line 44
Quoted 4 lines
hw/arm/xilinx_zynq.c +2 -0
 #define MPCORE_PERIPHBASE 0xF8F00000
 
+/* Dummy bootreg addr to keep ARM bootloader happy. Very top of OCM */
+#define SMP_BOOTREG_ADDR 0xfffffffc
It would probably be nicer to provide your own reset_secondary hook, and then hw/arm/boot.c won't ever look at what you set in bootreg_addr.

Yep, thatll be much cleaner. Thanks. All fixed in v2.

Regards,
Peter

Quoted 5 lines

Looks ok otherwise, though.
thanks
-- PMM

Peter Crosthwaite

author

2014-01-02 07:32 UTC #4 lore raw

PATCH v4 2/3 zynq_slcr: Add links to the CPUs

In reply to Peter Crosthwaite

The SLCR needs to be able to reset the CPUs, so link the CPUs to the SLCR.

Signed-off-by Peter Crosthwaite <[email protected]>

---
Changed since v3:
Revert back to using TYPE_CPU
Changed from v2:
Soften type of CPU to Device
Looped link creator

 hw/arm/xilinx_zynq.c |  4 ++++
 hw/misc/zynq_slcr.c  | 11 +++++++++++
 2 files changed, 15 insertions(+)

hw/arm/xilinx_zynq.c +4 -0

index c09ff36..1445c60 100644
@@ -193,6 +193,10 @@ static void zynq_init(QEMUMachineInitArgs *args)
 
     dev = qdev_create(NULL, "xilinx,zynq_slcr");
     qdev_init_nofail(dev);
+    object_property_set_link(OBJECT(dev), OBJECT(cpu[0]), "cpu0", NULL);
+    if (smp_cpus > 1) {
+        object_property_set_link(OBJECT(dev), OBJECT(cpu[1]), "cpu1", NULL);
+    }
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
 
     dev = qdev_create(NULL, "a9mpcore_priv");

hw/misc/zynq_slcr.c +11 -0

index e42a5b0..83ffc3b 100644
@@ -19,6 +19,8 @@
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 
+#define NUM_CPUS 2
+
 #ifdef ZYNQ_ARM_SLCR_ERR_DEBUG
 #define DB_PRINT(...) do { \
     fprintf(stderr,  ": %s: ", __func__); \

@@ -122,6 +124,8 @@ typedef struct ZynqSLCRState {
 
     MemoryRegion iomem;
 
+    CPUState *cpus[NUM_CPUS];
+
     union {
         struct {
             uint16_t scl;

@@ -496,10 +500,17 @@ static const MemoryRegionOps slcr_ops = {
 static int zynq_slcr_init(SysBusDevice *dev)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(dev);
+    int i;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
 
+    for (i = 0; i < NUM_CPUS; ++i) {
+        gchar *name = g_strdup_printf("cpu%d", i);
+        object_property_add_link(OBJECT(dev), name, TYPE_CPU,
+                                 (Object **)&s->cpus[i], NULL);
+        g_free(name);
+    }
     return 0;
 }

Signature · 1 line

1.8.5.2

Peter Maydell

2014-01-10 18:11 UTC #5 lore raw

Re: [Qemu-devel] [PATCH target-arm v4 2/3] zynq_slcr: Add links to the CPUs

Quoted 8 lines · Peter Crosthwaite

The SLCR needs to be able to reset the CPUs, so link the CPUs to the SLCR.
hw/misc/zynq_slcr.c +1 -0
@@ -496,10 +500,17 @@ static const MemoryRegionOps slcr_ops = {
 static int zynq_slcr_init(SysBusDevice *dev)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(dev);
+    int i;

hw/misc/zynq_slcr.c static const MemoryRegionOps slcr_ops = { · line 513

Quoted 10 lines

hw/misc/zynq_slcr.c +6 -0

 
     memory_region_init_io(&s->iomem, OBJECT(s), &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
 
+    for (i = 0; i < NUM_CPUS; ++i) {
+        gchar *name = g_strdup_printf("cpu%d", i);
+        object_property_add_link(OBJECT(dev), name, TYPE_CPU,
+                                 (Object **)&s->cpus[i], NULL);
+        g_free(name);
+    }

This is where we get into the nasty questions of how we ought to be modelling reset. I don't think that reset controllers ought to work by having direct links to a pile of QOM device objects. I'd much rather we tried to work towards modelling this the way the hardware does, ie a QOM device has one or more inbound GPIO lines corresponding to the hardware's reset signals, and the SoC or board wires those up to the reset controller appropriately.

thanks
-- PMM

Peter Crosthwaite

author

2014-01-10 20:21 UTC #6 lore raw

Re: [Qemu-devel] [PATCH target-arm v4 2/3] zynq_slcr: Add links to the CPUs

Quoted 13 lines · Peter Maydell

Quoted 12 lines · Peter Crosthwaite

The SLCR needs to be able to reset the CPUs, so link the CPUs to the SLCR.

hw/misc/zynq_slcr.c +1 -0

@@ -496,10 +500,17 @@ static const MemoryRegionOps slcr_ops = {
 static int zynq_slcr_init(SysBusDevice *dev)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(dev);
+    int i;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);

Quoted 17 lines

hw/misc/zynq_slcr.c static const MemoryRegionOps slcr_ops = { · line 513
Quoted 6 lines
hw/misc/zynq_slcr.c +6 -0
+    for (i = 0; i < NUM_CPUS; ++i) {
+        gchar *name = g_strdup_printf("cpu%d", i);
+        object_property_add_link(OBJECT(dev), name, TYPE_CPU,
+                                 (Object **)&s->cpus[i], NULL);
+        g_free(name);
+    }
This is where we get into the nasty questions of how we ought to be modelling reset. I don't think that reset controllers ought to work by having direct links to a pile of QOM device objects. I'd much rather we tried to work towards modelling this the way the hardware does, ie a QOM device has one or more inbound GPIO lines corresponding to the hardware's reset signals, and the SoC or board wires those up to the reset controller appropriately.

So all nice solutions to this really want named GPIOs which is something of a long term sore-point. Are you happy to take a simple addition of a reset GPIO to ARMCPU (which itself just calls cpu_reset) without the need for the big planned GPIO fixups (whether than be pins of Andreas' QOMification)?

Regards,
Peter

Quoted 3 lines

thanks
-- PMM

Andreas Färber

2014-01-10 20:54 UTC #7 lore raw

Re: [Qemu-devel] [PATCH target-arm v4 2/3] zynq_slcr: Add links to the CPUs

Am 10.01.2014 21:20, schrieb Peter Crosthwaite:

Quoted 32 lines · Peter Maydell

Quoted 13 lines · Peter Crosthwaite
Quoted 12 lines
The SLCR needs to be able to reset the CPUs, so link the CPUs to the SLCR.
hw/misc/zynq_slcr.c +1 -0
@@ -496,10 +500,17 @@ static const MemoryRegionOps slcr_ops = {
 static int zynq_slcr_init(SysBusDevice *dev)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(dev);
+    int i;
 
     memory_region_init_io(&s->iomem, OBJECT(s), &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
 
Quoted 17 lines
hw/misc/zynq_slcr.c static const MemoryRegionOps slcr_ops = { · line 513
Quoted 6 lines
hw/misc/zynq_slcr.c +6 -0
+    for (i = 0; i < NUM_CPUS; ++i) {
+        gchar *name = g_strdup_printf("cpu%d", i);
+        object_property_add_link(OBJECT(dev), name, TYPE_CPU,
+                                 (Object **)&s->cpus[i], NULL);
+        g_free(name);
+    }
This is where we get into the nasty questions of how we ought to be modelling reset. I don't think that reset controllers ought to work by having direct links to a pile of QOM device objects. I'd much rather we tried to work towards modelling this the way the hardware does, ie a QOM device has one or more inbound GPIO lines corresponding to the hardware's reset signals, and the SoC or board wires those up to the reset controller appropriately.

Quoted 5 lines

So all nice solutions to this really want named GPIOs which is something of a long term sore-point. Are you happy to take a simple addition of a reset GPIO to ARMCPU (which itself just calls cpu_reset) without the need for the big planned GPIO fixups (whether than be pins of Andreas' QOMification)?

Pins are Anthony's topic, not mine. :) I rather recently suggested to do a transparent QOM'ification. I thus have no objections against adding a reset IRQ! That had BTW been discussed as possible solution for partial/soft resets in PReP and x86 context.

Andreas

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Peter Crosthwaite

author

2014-01-02 07:32 UTC #8 lore raw

PATCH v4 3/3 zynq_slcr: Implement CPU reset

In reply to Peter Crosthwaite

Implement the CPU reset function of the A9_CPU_RST_CTRL register (offset 0x244).

Signed-off-by Peter Crosthwaite <[email protected]>

---
changed from v3:
Author reset
Use CPU reset rather than device reset
use extract32 rather than << &.
Removed halting functionality
changed from v2:
used device halting API instead of talking to the cpu.

 hw/misc/zynq_slcr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

hw/misc/zynq_slcr.c +12 -0

index 83ffc3b..0f94e03 100644
@@ -116,6 +116,8 @@ typedef enum {
   RESET_MAX
 } ResetValues;
 
+#define A9_CPU_RST_CTRL_RST_SHIFT 0
+
 #define TYPE_ZYNQ_SLCR "xilinx,zynq_slcr"
 #define ZYNQ_SLCR(obj) OBJECT_CHECK(ZynqSLCRState, (obj), TYPE_ZYNQ_SLCR)

@@ -349,6 +351,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
                           uint64_t val, unsigned size)
 {
     ZynqSLCRState *s = (ZynqSLCRState *)opaque;
+    int i;
 
     DB_PRINT("offset: %08x data: %08x\n", (unsigned)offset, (unsigned)val);

@@ -403,6 +406,15 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
                 goto bad_reg;
             }
             s->reset[(offset - 0x200) / 4] = val;
+            if (offset - 0x200 == A9_CPU * 4) { /* CPU Reset */
+                for (i = 0; i < NUM_CPUS && s->cpus[i]; ++i) {
+                    bool rst = extract32(val, A9_CPU_RST_CTRL_RST_SHIFT + i, 1);
+                    if (rst) {
+                        DB_PRINT("resetting cpu %d\n", i);
+                        cpu_reset(s->cpus[i]);
+                    }
+                }
+            }
             break;
         case 0x300:
             s->apu_ctrl = val;

Signature · 1 line

1.8.5.2

Peter Crosthwaite

author

2014-01-10 04:34 UTC #9 lore raw

In reply to Peter Crosthwaite

Ping!

Quoted 32 lines · Peter Crosthwaite

Hi All,
The clock controller module in the Zynq platform has the ability to halt and reset arbitrary devices, including the CPU. We use this feature to implement SMP Linux - the kernel halts CPU1 then rewrites the vector table to the secondary entry point and the resets+unhalts.
This series adds SMP support to the Zynq machine, and patches the Zynq SLCR (the clock controller) to have DeviceState pointers to the CPUs. cpu_reset() is then called on the appropriate register access.
Only the reset side is implemented (which is good enough for SMP linux as it stands). Future work is to implement the halting behaviour as well.
changed since v3:
Removed halting patches
Reduced to minimal change needed for SMP Zynq
Peter Crosthwaite (3):
xilinx_zynq: added SMP support:
zynq_slcr: Add links to the CPUs
zynq_slcr: Implement CPU reset
 hw/arm/xilinx_zynq.c | 73 ++++++++++++++++++++++++++++++++++++++++------------
 hw/misc/zynq_slcr.c  | 23 +++++++++++++++++
 2 files changed, 80 insertions(+), 16 deletions(-)
--
1.8.5.2