[Qemu-devel] [PATCH v2 0/4] Xilinx-Zynq boot process patches

[Qemu-devel] [PATCH v2 0/4] Xilinx-Zynq boot process patches

[Peter Crosthwaite]

Patch 1 is dual core smp support.
Patches 2-4 Implement the SLCR reset functionality.

changed from v1:
Addressed PMM review (P1)
Patches 2-4 are completely new.
Removed previous patches 2-3 (no bootloader changes anymore)

Peter A. G. Crosthwaite (4):
  xilinx_zynq: added smp support
  zynq_slcr: Add links to the CPUs
  zynq_slcr: Fixed ResetValues enum
  zynq_slcr: Implement CPU reset and halting

 hw/xilinx_zynq.c |   68 ++++++++++++++++++++++++++++++++++++++++++++---------
 hw/zynq_slcr.c   |   29 ++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH v2 1/4] xilinx_zynq: added smp support

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

Added linux smp support for the xilinx zynq platform (2x cpus are supported)

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
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/xilinx_zynq.c |   57 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index 7e6c273..22a2bc5 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -30,6 +30,32 @@
 
 #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
 
+#define SMP_BOOT_ADDR 0x0fff0000
+#define SMP_BOOTREG_ADDR 0xfffffff0
+
+/* Entry point for secondary CPU */
+static uint32_t zynq_smpboot[] = {
+    0xe3e0000f, /* ldr r0, =0xfffffff0 (mvn r0, #15) */
+    0xe320f002, /* wfe */
+    0xe5901000, /* ldr     r1, [r0] */
+    0xe1110001, /* tst     r1, r1 */
+    0x0afffffb, /* beq     <wfe> */
+    0xe12fff11, /* bx      r1 */
+    0,
+};
+
+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)
@@ -50,7 +76,7 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
                         const char *kernel_filename, const char *kernel_cmdline,
                         const char *initrd_filename, const char *cpu_model)
 {
-    ARMCPU *cpu;
+    ARMCPU *cpus[2];
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
@@ -60,19 +86,21 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
     qemu_irq pic[64];
     NICInfo *nd;
     int n;
-    qemu_irq cpu_irq;
+    qemu_irq cpu_irq[2];
 
     if (!cpu_model) {
         cpu_model = "cortex-a9";
     }
 
-    cpu = cpu_arm_init(cpu_model);
-    if (!cpu) {
-        fprintf(stderr, "Unable to find CPU definition\n");
-        exit(1);
+    for (n = 0; n < smp_cpus; n++) {
+        cpus[n] = cpu_arm_init(cpu_model);
+        if (!cpus[n]) {
+            fprintf(stderr, "Unable to find CPU definition\n");
+            exit(1);
+        }
+        irqp = arm_pic_init_cpu(cpus[n]);
+        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
     }
-    irqp = arm_pic_init_cpu(cpu);
-    cpu_irq = irqp[ARM_PIC_CPU_IRQ];
 
     /* max 2GB ram */
     if (ram_size > 0x80000000) {
@@ -103,11 +131,13 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
     sysbus_mmio_map(sysbus_from_qdev(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 = sysbus_from_qdev(dev);
     sysbus_mmio_map(busdev, 0, 0xF8F00000);
-    sysbus_connect_irq(busdev, 0, cpu_irq);
+    for (n = 0; n < smp_cpus; n++) {
+        sysbus_connect_irq(busdev, n, cpu_irq[n]);
+    }
 
     for (n = 0; n < 64; n++) {
         pic[n] = qdev_get_gpio_in(dev, n);
@@ -134,7 +164,10 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
     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_env_get_cpu(first_cpu), &zynq_binfo);
@@ -145,7 +178,7 @@ static QEMUMachine zynq_machine = {
     .desc = "Xilinx Zynq Platform Baseboard for Cortex-A9",
     .init = zynq_init,
     .use_scsi = 1,
-    .max_cpus = 1,
+    .max_cpus = 2,
     .no_sdcard = 1
 };
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 1/4] xilinx_zynq: added smp support

[Peter Maydell]

On 4 October 2012 01:16, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>
> Added linux smp support for the xilinx zynq platform (2x cpus are supported)
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
> 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/xilinx_zynq.c |   57 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
> index 7e6c273..22a2bc5 100644
> --- a/hw/xilinx_zynq.c
> +++ b/hw/xilinx_zynq.c
> @@ -30,6 +30,32 @@
>
>  #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
>
> +#define SMP_BOOT_ADDR 0x0fff0000

What is at this address in real hardware? I can't see anything in the
machine model that maps at this address but I could be missing it.
(Generally we put the secondary bootloader in some place corresponding
to some kind of real RAM.)

> +#define SMP_BOOTREG_ADDR 0xfffffff0

...this is an address in the on-chip memory, right?

Other than those queries, patch looks OK.

-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] xilinx_zynq: added smp support

[Peter Crosthwaite]

On Tue, Oct 9, 2012 at 9:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 October 2012 01:16, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>>
>> Added linux smp support for the xilinx zynq platform (2x cpus are supported)
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>> 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/xilinx_zynq.c |   57 ++++++++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 45 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
>> index 7e6c273..22a2bc5 100644
>> --- a/hw/xilinx_zynq.c
>> +++ b/hw/xilinx_zynq.c
>> @@ -30,6 +30,32 @@
>>
>>  #define IRQ_OFFSET 32 /* pic interrupts start from index 32 */
>>
>> +#define SMP_BOOT_ADDR 0x0fff0000
>
> What is at this address in real hardware? I can't see anything in the
> machine model that maps at this address but I could be missing it.
> (Generally we put the secondary bootloader in some place corresponding
> to some kind of real RAM.)
>

Main RAM (based at 0). We always pass -M big enough for this to not be
a problem. I guess we could force to ram big enough in the machine
model to catch the case where memory is too small.

>> +#define SMP_BOOTREG_ADDR 0xfffffff0
>
> ...this is an address in the on-chip memory, right?
>

Yes, Our bootrom does something similar for secondary CPU kicking, so
we chose that address to bring the machine as close to the actual boot
process as possible.

Regards,
Peter

> Other than those queries, patch looks OK.
>
> -- PMM

[Qemu-devel] [PATCH v2 2/4] zynq_slcr: Add links to the CPUs

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

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

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---

 hw/xilinx_zynq.c |   11 +++++++++++
 hw/zynq_slcr.c   |    9 +++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index 22a2bc5..fc81521 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -100,6 +100,10 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
         }
         irqp = arm_pic_init_cpu(cpus[n]);
         cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
+        /* FIXME: handle this somewhere central */
+        object_property_add_child(container_get(qdev_get_machine(),
+                                  "/unattached"), g_strdup_printf("cpu[%d]", n),
+                                  OBJECT(cpus[n]), NULL);
     }
 
     /* max 2GB ram */
@@ -128,6 +132,13 @@ static void zynq_init(ram_addr_t ram_size, const char *boot_device,
 
     dev = qdev_create(NULL, "xilinx,zynq_slcr");
     qdev_init_nofail(dev);
+    Error *errp = NULL;
+    object_property_set_link(OBJECT(dev), OBJECT(cpus[0]), "cpu0", &errp);
+    assert_no_error(errp);
+    if (smp_cpus > 1) {
+        object_property_set_link(OBJECT(dev), OBJECT(cpus[1]), "cpu1", NULL);
+        assert_no_error(errp);
+    }
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, 0xF8000000);
 
     dev = qdev_create(NULL, "a9mpcore_priv");
diff --git a/hw/zynq_slcr.c b/hw/zynq_slcr.c
index 4f97575..468fb0e 100644
--- a/hw/zynq_slcr.c
+++ b/hw/zynq_slcr.c
@@ -19,6 +19,8 @@
 #include "sysbus.h"
 #include "sysemu.h"
 
+#define NUM_CPUS 2
+
 #ifdef ZYNQ_ARM_SLCR_ERR_DEBUG
 #define DB_PRINT(...) do { \
     fprintf(stderr,  ": %s: ", __func__); \
@@ -118,6 +120,8 @@ typedef struct {
     SysBusDevice busdev;
     MemoryRegion iomem;
 
+    ARMCPU *cpus[NUM_CPUS];
+
     union {
         struct {
             uint16_t scl;
@@ -496,6 +500,11 @@ static int zynq_slcr_init(SysBusDevice *dev)
     memory_region_init_io(&s->iomem, &slcr_ops, s, "slcr", 0x1000);
     sysbus_init_mmio(dev, &s->iomem);
 
+    object_property_add_link(OBJECT(dev), "cpu0", TYPE_ARM_CPU,
+                             (Object **) &s->cpus[0], NULL);
+    object_property_add_link(OBJECT(dev), "cpu1", TYPE_ARM_CPU,
+                             (Object **) &s->cpus[1], NULL);
+
     return 0;
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 2/4] zynq_slcr: Add links to the CPUs

[Peter Maydell]

On 4 October 2012 01:16, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>
> The SLCR needs to be able to reset the CPUs, so link the CPUs to the slcr.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

I think your patch 4/4 is the wrong approach to reset, so I'm not
going to review this one particularly (beyond the obvious that if
we need this it should be done globally, not specifically to Zynq).

-- PMM

[Qemu-devel] [PATCH v2 3/4] zynq_slcr: Fixed ResetValues enum

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

There is a gap in the reset region of the address space at offset 0x208. This
throws out all these enum values by one when translating them to address offsets.
Fixed by putting the corresponding gap in the enum as well.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---

 hw/zynq_slcr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/zynq_slcr.c b/hw/zynq_slcr.c
index 468fb0e..6eafad5 100644
--- a/hw/zynq_slcr.c
+++ b/hw/zynq_slcr.c
@@ -93,7 +93,7 @@ typedef enum {
 typedef enum {
   PSS,
   DDDR,
-  DMAC,
+  DMAC = 3,
   USB,
   GEM,
   SDIO,
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 3/4] zynq_slcr: Fixed ResetValues enum

[Peter Maydell]

On 4 October 2012 01:16, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>
> There is a gap in the reset region of the address space at offset 0x208. This
> throws out all these enum values by one when translating them to address offsets.
> Fixed by putting the corresponding gap in the enum as well.
>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

This fix isn't dependent on the rest of this patchset, right? I can put
it in arm-devs.next now if you like or you could send it via -trivial.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v2 3/4] zynq_slcr: Fixed ResetValues enum

[Peter Crosthwaite]

Yes it is independent.

Please queue to arm-devs,

Regards,
Peter

On Tue, Oct 9, 2012 at 9:53 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 October 2012 01:16, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>>
>> There is a gap in the reset region of the address space at offset 0x208. This
>> throws out all these enum values by one when translating them to address offsets.
>> Fixed by putting the corresponding gap in the enum as well.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>
> This fix isn't dependent on the rest of this patchset, right? I can put
> it in arm-devs.next now if you like or you could send it via -trivial.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> -- PMM

[Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Crosthwaite]

From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>

Implement the CPU reset and halt functions of the A9_CPU_RST_CTRL register
(offset 0x244).

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---

 hw/zynq_slcr.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/hw/zynq_slcr.c b/hw/zynq_slcr.c
index 6eafad5..c1922fc 100644
--- a/hw/zynq_slcr.c
+++ b/hw/zynq_slcr.c
@@ -116,6 +116,9 @@ typedef enum {
   RESET_MAX
 } ResetValues;
 
+#define A9_CPU_RST_CTRL_RST_SHIFT 0
+#define A9_CPU_RST_CTRL_CLKSTOP_SHIFT 4
+
 typedef struct {
     SysBusDevice busdev;
     MemoryRegion iomem;
@@ -346,6 +349,7 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t offset,
                           uint64_t val, unsigned size)
 {
     ZynqSLCRState *s = (ZynqSLCRState *)opaque;
+    int i;
 
     DB_PRINT("offset: %08x data: %08x\n", offset, (unsigned)val);
 
@@ -400,6 +404,20 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t 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 is_rst = val & (1 << (A9_CPU_RST_CTRL_RST_SHIFT + i));
+                    bool is_clkstop = val &
+                                    (1 << (A9_CPU_RST_CTRL_CLKSTOP_SHIFT + i));
+                    if (is_rst) {
+                        CPU_GET_CLASS(CPU(s->cpus[i]))->reset(CPU(s->cpus[i]));
+                        DB_PRINT("resetting cpu %d\n", i);
+                    }
+                    s->cpus[i]->env.halted = is_rst || is_clkstop;
+                    DB_PRINT("%shalting cpu %d\n", s->cpus[i]->env.halted ?
+                             "" : "un", i);
+                }
+            }
             break;
         case 0x300:
             s->apu_ctrl = val;
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Maydell]

On 4 October 2012 01:16, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> @@ -400,6 +404,20 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t 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 is_rst = val & (1 << (A9_CPU_RST_CTRL_RST_SHIFT + i));
> +                    bool is_clkstop = val &
> +                                    (1 << (A9_CPU_RST_CTRL_CLKSTOP_SHIFT + i));
> +                    if (is_rst) {
> +                        CPU_GET_CLASS(CPU(s->cpus[i]))->reset(CPU(s->cpus[i]));
> +                        DB_PRINT("resetting cpu %d\n", i);
> +                    }
> +                    s->cpus[i]->env.halted = is_rst || is_clkstop;
> +                    DB_PRINT("%shalting cpu %d\n", s->cpus[i]->env.halted ?
> +                             "" : "un", i);
> +                }
> +            }

We don't support per-CPU reset right now, and I don't think this is the
right approach. This external device shouldn't be reaching into the
implementation of the CPU object like this.

My suggestion would be to expose an inbound GPIO line on the CPU object
(corresponding to the hardware's per-CPU reset lines) and then have an
appropriate implementation inside the CPU. (In general QEMU doesn't really
handle reset very cleanly IMHO.)

Not sure how much sense it makes to model the 'stop the CPU clock' stuff
within QEMU -- probably depends whether there are any programmer-visible
consequences.

-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Crosthwaite]

On Tue, Oct 9, 2012 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 October 2012 01:16, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> @@ -400,6 +404,20 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t 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 is_rst = val & (1 << (A9_CPU_RST_CTRL_RST_SHIFT + i));
>> +                    bool is_clkstop = val &
>> +                                    (1 << (A9_CPU_RST_CTRL_CLKSTOP_SHIFT + i));
>> +                    if (is_rst) {
>> +                        CPU_GET_CLASS(CPU(s->cpus[i]))->reset(CPU(s->cpus[i]));
>> +                        DB_PRINT("resetting cpu %d\n", i);
>> +                    }
>> +                    s->cpus[i]->env.halted = is_rst || is_clkstop;
>> +                    DB_PRINT("%shalting cpu %d\n", s->cpus[i]->env.halted ?
>> +                             "" : "un", i);
>> +                }
>> +            }
>
> We don't support per-CPU reset right now, and I don't think this is the
> right approach. This external device shouldn't be reaching into the
> implementation of the CPU object like this.
>

cpu reset is part of the QOM interface for TYPE_CPU. Theres no
privatisation of that so disagree that this is "reaching into the
implementation".

> My suggestion would be to expose an inbound GPIO line on the CPU object
> (corresponding to the hardware's per-CPU reset lines) and then have an
> appropriate implementation inside the CPU.

This would be awkward, considering the standard GPIO functionality
comes from TYPE_DEVICE, which is not a parent class of TYPE_ARM_CPU.
Would have to add GPIOs directly as properties. Does
object_property_set_gpio of something usable for that even exist?

Modelling internal signal as a GPIO is a slipperly slope as well. All
over QEMU there are assumptions made about what device connects to
what which abstract away the signalling layer, and I dont see how a
reset connection is different. We dont model an I2C bus as a wiggling
wire, so why do we have to model CPU resets like that?

(In general QEMU doesn't really
> handle reset very cleanly IMHO.)
>
> Not sure how much sense it makes to model the 'stop the CPU clock' stuff
> within QEMU -- probably depends whether there are any programmer-visible
> consequences.
>

Theres a race condition in my test vectors if the CPU immediately runs
after the reset. CPU0 holds CPU1 in reset, then rewrites the vector
table while holding reset. If CPU1 kicks immediately then it will use
out an out of date reset vector.

Regards,
Peter

> -- PMM

Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Maydell]

On 9 October 2012 14:38, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Tue, Oct 9, 2012 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> We don't support per-CPU reset right now, and I don't think this is the
>> right approach. This external device shouldn't be reaching into the
>> implementation of the CPU object like this.
>>
>
> cpu reset is part of the QOM interface for TYPE_CPU. Theres no
> privatisation of that so disagree that this is "reaching into the
> implementation".

You're modifying s->cpus[i]->env.halted ...

Also, the QOM cpu reset function just does an instantaneous reset
of the CPU. What you actually want is "while I assert this then
please hold the CPU in reset indefinitely", which is functionality
we don't expose at the moment.

>> My suggestion would be to expose an inbound GPIO line on the CPU object
>> (corresponding to the hardware's per-CPU reset lines) and then have an
>> appropriate implementation inside the CPU.
>
> This would be awkward, considering the standard GPIO functionality
> comes from TYPE_DEVICE, which is not a parent class of TYPE_ARM_CPU.
> Would have to add GPIOs directly as properties. Does
> object_property_set_gpio of something usable for that even exist?
>
> Modelling internal signal as a GPIO is a slipperly slope as well. All
> over QEMU there are assumptions made about what device connects to
> what which abstract away the signalling layer, and I dont see how a
> reset connection is different. We dont model an I2C bus as a wiggling
> wire, so why do we have to model CPU resets like that?

We do (or should) model an I2C bus as a connection between two
QEMU objects, where the connection is made by the board model
as part of its initialisation. Similarly, we should model CPU
reset as a connection between two QEMU objects (the power controller
and the CPU) which is made by the board model as part of its
initialisation.

I2C is more complicated and so you want to bundle things up
into a more abstracted connection rather than modelling things
at the single-wire level. CPU reset really is a single wire though.

>> (In general QEMU doesn't really
>> handle reset very cleanly IMHO.)
>>
>> Not sure how much sense it makes to model the 'stop the CPU clock' stuff
>> within QEMU -- probably depends whether there are any programmer-visible
>> consequences.
>>
>
> Theres a race condition in my test vectors if the CPU immediately runs
> after the reset. CPU0 holds CPU1 in reset, then rewrites the vector
> table while holding reset. If CPU1 kicks immediately then it will use
> out an out of date reset vector.

Hmm, shouldn't CPU1 only start executing from the reset vector
when we come out of reset? (There is an issue like this for M
profile cores I expect, since they read the PC from the vector
table rather than just jumping into the vector table like A
profile cores.)

But either way the implementation of the reset gpio input
should be that while it's asserted we hold the CPU in reset,
and it doesn't execute until the GPIO line goes low.

-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Peter Crosthwaite]

On Tue, Oct 9, 2012 at 11:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 October 2012 14:38, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> On Tue, Oct 9, 2012 at 9:41 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> We don't support per-CPU reset right now, and I don't think this is the
>>> right approach. This external device shouldn't be reaching into the
>>> implementation of the CPU object like this.
>>>
>>
>> cpu reset is part of the QOM interface for TYPE_CPU. Theres no
>> privatisation of that so disagree that this is "reaching into the
>> implementation".
>
> You're modifying s->cpus[i]->env.halted ...
>

My understanding is this is tangled with andreas's CPU refactorings:

http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg02882.html

And once that issue is surmounted we should be able to add APIs to do
this on the TYPE_CPU level rather than the TYPE_ARM_CPU level.

> Also, the QOM cpu reset function just does an instantaneous reset
> of the CPU. What you actually want is "while I assert this then
> please hold the CPU in reset indefinitely", which is functionality
> we don't expose at the moment.
>

I implemented this my just holding the halt while the reset bit is
asserted. Is this inaccurate?

>>> My suggestion would be to expose an inbound GPIO line on the CPU object
>>> (corresponding to the hardware's per-CPU reset lines) and then have an
>>> appropriate implementation inside the CPU.
>>
>> This would be awkward, considering the standard GPIO functionality
>> comes from TYPE_DEVICE, which is not a parent class of TYPE_ARM_CPU.
>> Would have to add GPIOs directly as properties. Does
>> object_property_set_gpio of something usable for that even exist?
>>

This is still my bigger concern. Can we actually do the GPIO thing
without major infrastructural developments?

>> Modelling internal signal as a GPIO is a slipperly slope as well. All
>> over QEMU there are assumptions made about what device connects to
>> what which abstract away the signalling layer, and I dont see how a
>> reset connection is different. We dont model an I2C bus as a wiggling
>> wire, so why do we have to model CPU resets like that?
>
> We do (or should) model an I2C bus as a connection between two
> QEMU objects, where the connection is made by the board model
> as part of its initialisation. Similarly, we should model CPU
> reset as a connection between two QEMU objects (the power controller
> and the CPU) which is made by the board model as part of its
> initialisation.
>

But the machine model does make the connection in this case as well.
Just using QOM links rather than GPIOs. The Machine model explicitly
sets the link (using object_property_set_link()) from the Zynq SLCR to
the CPU. The SLCR can be connected to any CPU. This also means that
when we come to implement the halting functionality, the interconnect
is handled as one link an not 2 GPIOs (or N gpios depending on how
many CPU control/status signals you want to hookup).

Regards,
Peter

> I2C is more complicated and so you want to bundle things up
> into a more abstracted connection rather than modelling things
> at the single-wire level. CPU reset really is a single wire though.
>
>>> (In general QEMU doesn't really
>>> handle reset very cleanly IMHO.)
>>>
>>> Not sure how much sense it makes to model the 'stop the CPU clock' stuff
>>> within QEMU -- probably depends whether there are any programmer-visible
>>> consequences.
>>>
>>
>> Theres a race condition in my test vectors if the CPU immediately runs
>> after the reset. CPU0 holds CPU1 in reset, then rewrites the vector
>> table while holding reset. If CPU1 kicks immediately then it will use
>> out an out of date reset vector.
>
> Hmm, shouldn't CPU1 only start executing from the reset vector
> when we come out of reset? (There is an issue like this for M
> profile cores I expect, since they read the PC from the vector
> table rather than just jumping into the vector table like A
> profile cores.)
>
> But either way the implementation of the reset gpio input
> should be that while it's asserted we hold the CPU in reset,
> and it doesn't execute until the GPIO line goes low.
>
> -- PMM

Re: [Qemu-devel] [PATCH v2 4/4] zynq_slcr: Implement CPU reset and halting

[Andreas Färber]

Am 04.10.2012 02:16, schrieb Peter Crosthwaite:
> From: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> 
> Implement the CPU reset and halt functions of the A9_CPU_RST_CTRL register
> (offset 0x244).
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
> 
>  hw/zynq_slcr.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/zynq_slcr.c b/hw/zynq_slcr.c
> index 6eafad5..c1922fc 100644
> --- a/hw/zynq_slcr.c
> +++ b/hw/zynq_slcr.c
> @@ -116,6 +116,9 @@ typedef enum {
>    RESET_MAX
>  } ResetValues;
>  
> +#define A9_CPU_RST_CTRL_RST_SHIFT 0
> +#define A9_CPU_RST_CTRL_CLKSTOP_SHIFT 4
> +
>  typedef struct {
>      SysBusDevice busdev;
>      MemoryRegion iomem;
> @@ -346,6 +349,7 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t offset,
>                            uint64_t val, unsigned size)
>  {
>      ZynqSLCRState *s = (ZynqSLCRState *)opaque;
> +    int i;
>  
>      DB_PRINT("offset: %08x data: %08x\n", offset, (unsigned)val);
>  
> @@ -400,6 +404,20 @@ static void zynq_slcr_write(void *opaque, target_phys_addr_t 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 is_rst = val & (1 << (A9_CPU_RST_CTRL_RST_SHIFT + i));
> +                    bool is_clkstop = val &
> +                                    (1 << (A9_CPU_RST_CTRL_CLKSTOP_SHIFT + i));
> +                    if (is_rst) {
> +                        CPU_GET_CLASS(CPU(s->cpus[i]))->reset(CPU(s->cpus[i]));

Isn't that just cpu_reset(CPU(s->cpus[i]));? Please prefer that over
open-coding.

Thanks,
Andreas

> +                        DB_PRINT("resetting cpu %d\n", i);
> +                    }
> +                    s->cpus[i]->env.halted = is_rst || is_clkstop;
> +                    DB_PRINT("%shalting cpu %d\n", s->cpus[i]->env.halted ?
> +                             "" : "un", i);
> +                }
> +            }
>              break;
>          case 0x300:
>              s->apu_ctrl = val;
> 


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

Re: [Qemu-devel] [PATCH v2 0/4] Xilinx-Zynq boot process patches

[Peter Crosthwaite]

Ping!

Any issues here? Should I [PULL] or can this go via arm-devs?

Regards,
Peter

On Thu, Oct 4, 2012 at 10:16 AM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Patch 1 is dual core smp support.
> Patches 2-4 Implement the SLCR reset functionality.
>
> changed from v1:
> Addressed PMM review (P1)
> Patches 2-4 are completely new.
> Removed previous patches 2-3 (no bootloader changes anymore)
>
> Peter A. G. Crosthwaite (4):
>   xilinx_zynq: added smp support
>   zynq_slcr: Add links to the CPUs
>   zynq_slcr: Fixed ResetValues enum
>   zynq_slcr: Implement CPU reset and halting
>
>  hw/xilinx_zynq.c |   68 ++++++++++++++++++++++++++++++++++++++++++++---------
>  hw/zynq_slcr.c   |   29 ++++++++++++++++++++++-
>  2 files changed, 84 insertions(+), 13 deletions(-)
>