[Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Krzysztof Kozlowski]

Hi,

This is a collection of three already sent patchsets [1] [2] [3].


Changes since previous versions (v2):
1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
2. Add reviewed-by tags.


The first patch in set is a bugfix which also exposes problem of cpuidle.
Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
poweroff working) but with CPU_IDLE it behaves bad due to low-power
mode (AFTR). This was discussed at [1].

There are no external dependencies.

Patchset is also available here:
https://github.com/krzk/qemu/tree/for-next/exynos4210-gic-qom-soc-poweroff-v3


Best regards,
Krzysztof

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg436902.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01562.html
[3] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01553.html


Krzysztof Kozlowski (11):
  hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
  hw/intc/exynos4210_gic: Use more meaningful name for local variable
  hw/timer/exynos4210_mct: Fix checkpatch style errors
  hw/timer/exynos4210_mct: Cleanup indentation and empty new lines
  hw/timer/exynos4210_mct: Remove unused defines
  hw/arm/exynos: Move DRAM initialization next boards
  hw/arm/exynos: Declare local variables in some order
  hw/arm/exynos: QOM-ify the SoC
  hw/arm/exynos: Use type define instead of hard-coded a9mpcore_priv
    string
  hw/intc/exynos4210_gic: Constify array of combiner interrupts
  hw/misc/exynos4210_pmu: Add support for system poweroff

 hw/arm/exynos4210.c         | 43 ++++++++++++++++--------------------
 hw/arm/exynos4_boards.c     | 53 +++++++++++++++++++++++++++++++++++++++------
 hw/intc/exynos4210_gic.c    | 35 ++++++++++++++++++------------
 hw/misc/exynos4210_pmu.c    | 20 ++++++++++++++++-
 hw/timer/exynos4210_mct.c   | 50 ++++++++++++++++++------------------------
 include/hw/arm/exynos4210.h | 11 +++++-----
 6 files changed, 132 insertions(+), 80 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 01/11] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU

[Krzysztof Kozlowski]

Recent Linux kernel (tested next-20170224) was complaining about missing
GIC mask and was unable to bring up secondary CPU:

    [    0.000000] NR_IRQS:16 nr_irqs:16 16
    [    0.000000] GIC CPU mask not found - kernel will fail to boot.
    ...
    [    0.400492] smp: Bringing up secondary CPUs ...
    [    1.413184] CPU1: failed to boot: -110
    [    1.423981] smp: Brought up 1 node, 1 CPU

In its instance_init() call, the Exynos GIC driver was setting GIC
memory mappings for each CPU, from 1 up to "num-cpu" property.  The
Exynos4210 machine init call on the other hand, first created Exynos GIC
device and then set the "num-cpu" property which was too late.  The init
already happened with default "num-cpu" value of 1 thus GIC mappings
were created only for the first CPU.

Split the Exynos GIC init code into realize function so the code will
see updated "num-cpu" property.  This fixes the warning and brings
second CPU:
    [    0.435780] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
    [    0.451838] smp: Brought up 1 node, 2 CPUs

Additionally this fixes missing Software Generated Interrupts (except
CPU wakeup, non of SGIs are coming) which are needed for example for IRQ
work.  Lack of IRQ work causes kernel to hang during system power off
because cpufreq_dbs_governor_stop() waits for completion with
irq_work_sync().

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/exynos4210_gic.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 2a55817b7660..222cfd6c6387 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -283,9 +283,20 @@ static void exynos4210_gic_set_irq(void *opaque, int irq, int level)
 
 static void exynos4210_gic_init(Object *obj)
 {
-    DeviceState *dev = DEVICE(obj);
     Exynos4210GicState *s = EXYNOS4210_GIC(obj);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init(&s->cpu_container, obj, "exynos4210-cpu-container",
+            EXYNOS4210_EXT_GIC_CPU_REGION_SIZE);
+    memory_region_init(&s->dist_container, obj, "exynos4210-dist-container",
+            EXYNOS4210_EXT_GIC_DIST_REGION_SIZE);
+
+}
+
+static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
+{
+    Exynos4210GicState *s = EXYNOS4210_GIC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    Object *obj = OBJECT(dev);
     uint32_t i;
     const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
     const char dist_prefix[] = "exynos4210-gic-alias_dist";
@@ -306,11 +317,6 @@ static void exynos4210_gic_init(Object *obj)
     qdev_init_gpio_in(dev, exynos4210_gic_set_irq,
                       EXYNOS4210_GIC_NIRQ - 32);
 
-    memory_region_init(&s->cpu_container, obj, "exynos4210-cpu-container",
-            EXYNOS4210_EXT_GIC_CPU_REGION_SIZE);
-    memory_region_init(&s->dist_container, obj, "exynos4210-dist-container",
-            EXYNOS4210_EXT_GIC_DIST_REGION_SIZE);
-
     for (i = 0; i < s->num_cpu; i++) {
         /* Map CPU interface per SMP Core */
         sprintf(cpu_alias_name, "%s%x", cpu_prefix, i);
@@ -346,6 +352,7 @@ static void exynos4210_gic_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->realize = exynos4210_gic_realize;
     dc->props = exynos4210_gic_properties;
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH v3 02/11] hw/intc/exynos4210_gic: Use more meaningful name for local variable

[Krzysztof Kozlowski]

There are to SysBusDevice variables in exynos4210_gic_realize()
function: one for the device itself and second for arm_gic device.  Add
a prefix "gic" to the second one so it will be easier to understand the
code.

While at it, put local uninitialized 'i' variable at the end, next to
other uninitialized ones.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/exynos4210_gic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 222cfd6c6387..9a2254f0b13c 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -297,21 +297,21 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
     Exynos4210GicState *s = EXYNOS4210_GIC(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     Object *obj = OBJECT(dev);
-    uint32_t i;
     const char cpu_prefix[] = "exynos4210-gic-alias_cpu";
     const char dist_prefix[] = "exynos4210-gic-alias_dist";
     char cpu_alias_name[sizeof(cpu_prefix) + 3];
     char dist_alias_name[sizeof(cpu_prefix) + 3];
-    SysBusDevice *busdev;
+    SysBusDevice *gicbusdev;
+    uint32_t i;
 
     s->gic = qdev_create(NULL, "arm_gic");
     qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(s->gic, "num-irq", EXYNOS4210_GIC_NIRQ);
     qdev_init_nofail(s->gic);
-    busdev = SYS_BUS_DEVICE(s->gic);
+    gicbusdev = SYS_BUS_DEVICE(s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
-    sysbus_pass_irq(sbd, busdev);
+    sysbus_pass_irq(sbd, gicbusdev);
 
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(dev, exynos4210_gic_set_irq,
@@ -322,7 +322,7 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
         sprintf(cpu_alias_name, "%s%x", cpu_prefix, i);
         memory_region_init_alias(&s->cpu_alias[i], obj,
                                  cpu_alias_name,
-                                 sysbus_mmio_get_region(busdev, 1),
+                                 sysbus_mmio_get_region(gicbusdev, 1),
                                  0,
                                  EXYNOS4210_GIC_CPU_REGION_SIZE);
         memory_region_add_subregion(&s->cpu_container,
@@ -332,7 +332,7 @@ static void exynos4210_gic_realize(DeviceState *dev, Error **errp)
         sprintf(dist_alias_name, "%s%x", dist_prefix, i);
         memory_region_init_alias(&s->dist_alias[i], obj,
                                  dist_alias_name,
-                                 sysbus_mmio_get_region(busdev, 0),
+                                 sysbus_mmio_get_region(gicbusdev, 0),
                                  0,
                                  EXYNOS4210_GIC_DIST_REGION_SIZE);
         memory_region_add_subregion(&s->dist_container,
-- 
2.9.3

[Qemu-devel] [PATCH v3 03/11] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Krzysztof Kozlowski]

Fix checkpatch errors:
1. ERROR: spaces required around that '+' (ctx:VxV)
2. ERROR: spaces required around that '&' (ctx:VxV)

No functional changes.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/exynos4210_mct.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index a2ec3920f82e..2404fb737ac4 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -937,7 +937,7 @@ static void exynos4210_mct_update_freq(Exynos4210MCTState *s)
 {
     uint32_t freq = s->freq;
     s->freq = 24000000 /
-            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg)+1) *
+            ((MCT_CFG_GET_PRESCALER(s->reg_mct_cfg) + 1) *
                     MCT_CFG_GET_DIVIDER(s->reg_mct_cfg));
 
     if (freq != s->freq) {
@@ -1162,7 +1162,7 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
 
     DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
 
-    if (offset&0x4) {
+    if (offset & 0x4) {
         s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
     } else {
         s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
-- 
2.9.3

[Qemu-devel] [PATCH v3 04/11] hw/timer/exynos4210_mct: Cleanup indentation and empty new lines

[Krzysztof Kozlowski]

Statements under 'case' were in some places wrongly indented bringing
confusion and making the code less readable.  Remove also few unneeded
blank lines.  No functional changes.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/exynos4210_mct.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index 2404fb737ac4..ea5f99d9a41b 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1016,9 +1016,9 @@ static uint64_t exynos4210_mct_read(void *opaque, hwaddr offset,
 
     case G_COMP_L(0): case G_COMP_L(1): case G_COMP_L(2): case G_COMP_L(3):
     case G_COMP_U(0): case G_COMP_U(1): case G_COMP_U(2): case G_COMP_U(3):
-    index = GET_G_COMP_IDX(offset);
-    shift = 8 * (offset & 0x4);
-    value = UINT32_MAX & (s->g_timer.reg.comp[index] >> shift);
+        index = GET_G_COMP_IDX(offset);
+        shift = 8 * (offset & 0x4);
+        value = UINT32_MAX & (s->g_timer.reg.comp[index] >> shift);
     break;
 
     case G_TCON:
@@ -1067,7 +1067,6 @@ static uint64_t exynos4210_mct_read(void *opaque, hwaddr offset,
         lt_i = GET_L_TIMER_IDX(offset);
 
         value = exynos4210_lfrc_get_count(&s->l_timer[lt_i]);
-
         break;
 
     case L0_TCON: case L1_TCON:
@@ -1153,23 +1152,23 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
 
     case G_COMP_L(0): case G_COMP_L(1): case G_COMP_L(2): case G_COMP_L(3):
     case G_COMP_U(0): case G_COMP_U(1): case G_COMP_U(2): case G_COMP_U(3):
-    index = GET_G_COMP_IDX(offset);
-    shift = 8 * (offset & 0x4);
-    s->g_timer.reg.comp[index] =
-            (s->g_timer.reg.comp[index] &
-            (((uint64_t)UINT32_MAX << 32) >> shift)) +
-            (value << shift);
+        index = GET_G_COMP_IDX(offset);
+        shift = 8 * (offset & 0x4);
+        s->g_timer.reg.comp[index] =
+                (s->g_timer.reg.comp[index] &
+                (((uint64_t)UINT32_MAX << 32) >> shift)) +
+                (value << shift);
 
-    DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
+        DPRINTF("comparator %d write 0x%llx val << %d\n", index, value, shift);
 
-    if (offset & 0x4) {
-        s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
-    } else {
-        s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
-    }
+        if (offset & 0x4) {
+            s->g_timer.reg.wstat |= G_WSTAT_COMP_U(index);
+        } else {
+            s->g_timer.reg.wstat |= G_WSTAT_COMP_L(index);
+        }
 
-    exynos4210_gfrc_restart(s);
-    break;
+        exynos4210_gfrc_restart(s);
+        break;
 
     case G_TCON:
         old_val = s->g_timer.reg.tcon;
@@ -1207,7 +1206,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case G_INT_ENB:
-
         /* Raise IRQ if transition from disabled to enabled and CSTAT pending */
         for (i = 0; i < MCT_GT_CMP_NUM; i++) {
             if ((value & G_INT_ENABLE(i)) > (s->g_timer.reg.tcon &
@@ -1288,7 +1286,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case L0_TCNTB: case L1_TCNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
@@ -1316,7 +1313,6 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         break;
 
     case L0_ICNTB: case L1_ICNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
@@ -1353,13 +1349,12 @@ static void exynos4210_mct_write(void *opaque, hwaddr offset,
         if (icntb_max[lt_i] < value) {
             icntb_max[lt_i] = value;
         }
-DPRINTF("local timer[%d] ICNTB write %llx; max=%x, min=%x\n\n",
-        lt_i, value, icntb_max[lt_i], icntb_min[lt_i]);
+        DPRINTF("local timer[%d] ICNTB write %llx; max=%x, min=%x\n\n",
+                lt_i, value, icntb_max[lt_i], icntb_min[lt_i]);
 #endif
-break;
+        break;
 
     case L0_FRCNTB: case L1_FRCNTB:
-
         lt_i = GET_L_TIMER_IDX(offset);
         index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
 
-- 
2.9.3

[Qemu-devel] [PATCH v3 05/11] hw/timer/exynos4210_mct: Remove unused defines

[Krzysztof Kozlowski]

Remove defines not used anywhere.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/exynos4210_mct.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index ea5f99d9a41b..e4ef4cfd3625 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -173,13 +173,10 @@ enum LocalTimerRegCntIndexes {
     L_REG_CNT_AMOUNT
 };
 
-#define MCT_NIRQ                6
 #define MCT_SFR_SIZE            0x444
 
 #define MCT_GT_CMP_NUM          4
 
-#define MCT_GT_MAX_VAL          UINT64_MAX
-
 #define MCT_GT_COUNTER_STEP     0x100000000ULL
 #define MCT_LT_COUNTER_STEP     0x100000000ULL
 #define MCT_LT_CNT_LOW_LIMIT    0x100
-- 
2.9.3

[Qemu-devel] [PATCH v3 06/11] hw/arm/exynos: Move DRAM initialization next boards

[Krzysztof Kozlowski]

Before QOM-ifying the Exynos4 SoC model, move the DRAM initialization
from exynos4210.c to exynos4_boards.c because DRAM is board specific,
not SoC.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/exynos4210.c         | 20 +-----------------
 hw/arm/exynos4_boards.c     | 50 ++++++++++++++++++++++++++++++++++++++-------
 include/hw/arm/exynos4210.h |  5 +----
 3 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 960f27e45a36..0da877f8db0a 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -160,13 +160,11 @@ static uint64_t exynos4210_calc_affinity(int cpu)
     return mp_affinity;
 }
 
-Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
-        unsigned long ram_size)
+Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
 {
     int i, n;
     Exynos4210State *s = g_new(Exynos4210State, 1);
     qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
-    unsigned long mem_size;
     DeviceState *dev;
     SysBusDevice *busdev;
     ObjectClass *cpu_oc;
@@ -299,22 +297,6 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
     memory_region_add_subregion(system_mem, EXYNOS4210_IRAM_BASE_ADDR,
                                 &s->iram_mem);
 
-    /* DRAM */
-    mem_size = ram_size;
-    if (mem_size > EXYNOS4210_DRAM_MAX_SIZE) {
-        memory_region_init_ram(&s->dram1_mem, NULL, "exynos4210.dram1",
-                mem_size - EXYNOS4210_DRAM_MAX_SIZE, &error_fatal);
-        vmstate_register_ram_global(&s->dram1_mem);
-        memory_region_add_subregion(system_mem, EXYNOS4210_DRAM1_BASE_ADDR,
-                &s->dram1_mem);
-        mem_size = EXYNOS4210_DRAM_MAX_SIZE;
-    }
-    memory_region_init_ram(&s->dram0_mem, NULL, "exynos4210.dram0", mem_size,
-                           &error_fatal);
-    vmstate_register_ram_global(&s->dram0_mem);
-    memory_region_add_subregion(system_mem, EXYNOS4210_DRAM0_BASE_ADDR,
-            &s->dram0_mem);
-
    /* PMU.
     * The only reason of existence at the moment is that secondary CPU boot
     * loader uses PMU INFORM5 register as a holding pen.
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 4853c318023c..6240b26839cd 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -22,6 +22,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu-common.h"
 #include "cpu.h"
@@ -56,6 +57,12 @@ typedef enum Exynos4BoardType {
     EXYNOS4_NUM_OF_BOARDS
 } Exynos4BoardType;
 
+typedef struct Exynos4BoardState {
+    Exynos4210State *soc;
+    MemoryRegion dram0_mem;
+    MemoryRegion dram1_mem;
+} Exynos4BoardState;
+
 static int exynos4_board_id[EXYNOS4_NUM_OF_BOARDS] = {
     [EXYNOS4_BOARD_NURI]     = 0xD33,
     [EXYNOS4_BOARD_SMDKC210] = 0xB16,
@@ -96,9 +103,34 @@ static void lan9215_init(uint32_t base, qemu_irq irq)
     }
 }
 
-static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
-                                                   Exynos4BoardType board_type)
+static void exynos4_boards_init_ram(Exynos4BoardState *s,
+                                    MemoryRegion *system_mem,
+                                    unsigned long ram_size)
+{
+    unsigned long mem_size = ram_size;
+
+    if (mem_size > EXYNOS4210_DRAM_MAX_SIZE) {
+        memory_region_init_ram(&s->dram1_mem, NULL, "exynos4210.dram1",
+                               mem_size - EXYNOS4210_DRAM_MAX_SIZE,
+                               &error_fatal);
+        vmstate_register_ram_global(&s->dram1_mem);
+        memory_region_add_subregion(system_mem, EXYNOS4210_DRAM1_BASE_ADDR,
+                                    &s->dram1_mem);
+        mem_size = EXYNOS4210_DRAM_MAX_SIZE;
+    }
+
+    memory_region_init_ram(&s->dram0_mem, NULL, "exynos4210.dram0", mem_size,
+                           &error_fatal);
+    vmstate_register_ram_global(&s->dram0_mem);
+    memory_region_add_subregion(system_mem, EXYNOS4210_DRAM0_BASE_ADDR,
+                                &s->dram0_mem);
+}
+
+static Exynos4BoardState *
+exynos4_boards_init_common(MachineState *machine,
+                           Exynos4BoardType board_type)
 {
+    Exynos4BoardState *s = g_new(Exynos4BoardState, 1);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
 
     if (smp_cpus != EXYNOS4210_NCPUS && !qtest_enabled()) {
@@ -127,8 +159,12 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
             machine->kernel_cmdline,
             machine->initrd_filename);
 
-    return exynos4210_init(get_system_memory(),
-            exynos4_board_ram_size[board_type]);
+    exynos4_boards_init_ram(s, get_system_memory(),
+                            exynos4_board_ram_size[board_type]);
+
+    s->soc = exynos4210_init(get_system_memory());
+
+    return s;
 }
 
 static void nuri_init(MachineState *machine)
@@ -140,11 +176,11 @@ static void nuri_init(MachineState *machine)
 
 static void smdkc210_init(MachineState *machine)
 {
-    Exynos4210State *s = exynos4_boards_init_common(machine,
-                                                    EXYNOS4_BOARD_SMDKC210);
+    Exynos4BoardState *s = exynos4_boards_init_common(machine,
+                                                      EXYNOS4_BOARD_SMDKC210);
 
     lan9215_init(SMDK_LAN9118_BASE_ADDR,
-            qemu_irq_invert(s->irq_table[exynos4210_get_irq(37, 1)]));
+            qemu_irq_invert(s->soc->irq_table[exynos4210_get_irq(37, 1)]));
     arm_load_kernel(ARM_CPU(first_cpu), &exynos4_board_binfo);
 }
 
diff --git a/include/hw/arm/exynos4210.h b/include/hw/arm/exynos4210.h
index d9e08014d6ff..098a69ec73d3 100644
--- a/include/hw/arm/exynos4210.h
+++ b/include/hw/arm/exynos4210.h
@@ -93,8 +93,6 @@ typedef struct Exynos4210State {
     MemoryRegion iram_mem;
     MemoryRegion irom_mem;
     MemoryRegion irom_alias_mem;
-    MemoryRegion dram0_mem;
-    MemoryRegion dram1_mem;
     MemoryRegion boot_secondary;
     MemoryRegion bootreg_mem;
     I2CBus *i2c_if[EXYNOS4210_I2C_NUMBER];
@@ -103,8 +101,7 @@ typedef struct Exynos4210State {
 void exynos4210_write_secondary(ARMCPU *cpu,
         const struct arm_boot_info *info);
 
-Exynos4210State *exynos4210_init(MemoryRegion *system_mem,
-        unsigned long ram_size);
+Exynos4210State *exynos4210_init(MemoryRegion *system_mem);
 
 /* Initialize exynos4210 IRQ subsystem stub */
 qemu_irq *exynos4210_init_irq(Exynos4210Irq *env);
-- 
2.9.3

[Qemu-devel] [PATCH v3 07/11] hw/arm/exynos: Declare local variables in some order

[Krzysztof Kozlowski]

Bring some more readability by declaring local function variables: first
initialized ones and then the rest (with reversed-christmas-tree order).

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/exynos4210.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 0da877f8db0a..27a7bf28a5a9 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -162,12 +162,12 @@ static uint64_t exynos4210_calc_affinity(int cpu)
 
 Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
 {
-    int i, n;
     Exynos4210State *s = g_new(Exynos4210State, 1);
     qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
-    DeviceState *dev;
     SysBusDevice *busdev;
     ObjectClass *cpu_oc;
+    DeviceState *dev;
+    int i, n;
 
     cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, "cortex-a9");
     assert(cpu_oc);
-- 
2.9.3

[Qemu-devel] [PATCH v3 08/11] hw/arm/exynos: QOM-ify the SoC

[Krzysztof Kozlowski]

Convert the Exynos4210 SoC code into a QOM model which is a preferred
approach instead of directly initializing SoC-related devices from the
board file.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/exynos4210.c         | 18 +++++++++++++++---
 hw/arm/exynos4_boards.c     |  9 ++++++---
 include/hw/arm/exynos4210.h |  8 ++++++--
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 27a7bf28a5a9..034fc8be9d76 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -160,9 +160,10 @@ static uint64_t exynos4210_calc_affinity(int cpu)
     return mp_affinity;
 }
 
-Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
+static void exynos4210_init(Object *obj)
 {
-    Exynos4210State *s = g_new(Exynos4210State, 1);
+    MemoryRegion *system_mem = get_system_memory();
+    Exynos4210State *s = EXYNOS4210(obj);
     qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
     SysBusDevice *busdev;
     ObjectClass *cpu_oc;
@@ -402,6 +403,17 @@ Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
 
     sysbus_create_simple(TYPE_EXYNOS4210_EHCI, EXYNOS4210_EHCI_BASE_ADDR,
             s->irq_table[exynos4210_get_irq(28, 3)]);
+}
+
+static const TypeInfo exynos4210_type_info = {
+    .name = TYPE_EXYNOS4210,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(Exynos4210State),
+    .instance_init = exynos4210_init,
+};
 
-    return s;
+static void exynos4210_register_types(void)
+{
+    type_register_static(&exynos4210_type_info);
 }
+type_init(exynos4210_register_types)
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 6240b26839cd..5e7c6b562ae2 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -58,7 +58,7 @@ typedef enum Exynos4BoardType {
 } Exynos4BoardType;
 
 typedef struct Exynos4BoardState {
-    Exynos4210State *soc;
+    Exynos4210State soc;
     MemoryRegion dram0_mem;
     MemoryRegion dram1_mem;
 } Exynos4BoardState;
@@ -162,7 +162,10 @@ exynos4_boards_init_common(MachineState *machine,
     exynos4_boards_init_ram(s, get_system_memory(),
                             exynos4_board_ram_size[board_type]);
 
-    s->soc = exynos4210_init(get_system_memory());
+    object_initialize(&s->soc, sizeof(s->soc), TYPE_EXYNOS4210);
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+                              &error_abort);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
 
     return s;
 }
@@ -180,7 +183,7 @@ static void smdkc210_init(MachineState *machine)
                                                       EXYNOS4_BOARD_SMDKC210);
 
     lan9215_init(SMDK_LAN9118_BASE_ADDR,
-            qemu_irq_invert(s->soc->irq_table[exynos4210_get_irq(37, 1)]));
+            qemu_irq_invert(s->soc.irq_table[exynos4210_get_irq(37, 1)]));
     arm_load_kernel(ARM_CPU(first_cpu), &exynos4_board_binfo);
 }
 
diff --git a/include/hw/arm/exynos4210.h b/include/hw/arm/exynos4210.h
index 098a69ec73d3..116eae62756b 100644
--- a/include/hw/arm/exynos4210.h
+++ b/include/hw/arm/exynos4210.h
@@ -29,6 +29,10 @@
 #include "exec/memory.h"
 #include "target/arm/cpu-qom.h"
 
+#define TYPE_EXYNOS4210 "exynos4210"
+#define EXYNOS4210(obj) \
+    OBJECT_CHECK(Exynos4210State, (obj), TYPE_EXYNOS4210)
+
 #define EXYNOS4210_NCPUS                    2
 
 #define EXYNOS4210_DRAM0_BASE_ADDR          0x40000000
@@ -85,6 +89,8 @@ typedef struct Exynos4210Irq {
 } Exynos4210Irq;
 
 typedef struct Exynos4210State {
+    DeviceState parent_obj;
+
     ARMCPU *cpu[EXYNOS4210_NCPUS];
     Exynos4210Irq irqs;
     qemu_irq *irq_table;
@@ -101,8 +107,6 @@ typedef struct Exynos4210State {
 void exynos4210_write_secondary(ARMCPU *cpu,
         const struct arm_boot_info *info);
 
-Exynos4210State *exynos4210_init(MemoryRegion *system_mem);
-
 /* Initialize exynos4210 IRQ subsystem stub */
 qemu_irq *exynos4210_init_irq(Exynos4210Irq *env);
 
-- 
2.9.3

[Qemu-devel] [PATCH v3 09/11] hw/arm/exynos: Use type define instead of hard-coded a9mpcore_priv string

[Krzysztof Kozlowski]

Use a define for a9mpcore_priv device type name instead of hard-coded
string.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/exynos4210.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index 034fc8be9d76..a9e221c5b7fe 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -26,6 +26,7 @@
 #include "qemu-common.h"
 #include "qemu/log.h"
 #include "cpu.h"
+#include "hw/cpu/a9mpcore.h"
 #include "hw/boards.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
@@ -212,7 +213,7 @@ static void exynos4210_init(Object *obj)
     }
 
     /* Private memory region and Internal GIC */
-    dev = qdev_create(NULL, "a9mpcore_priv");
+    dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
     qdev_init_nofail(dev);
     busdev = SYS_BUS_DEVICE(dev);
-- 
2.9.3

[Qemu-devel] [PATCH v3 10/11] hw/intc/exynos4210_gic: Constify array of combiner interrupts

[Krzysztof Kozlowski]

The static array of interrupt combiner mappings is not modified so it
can be made const for code safeness.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/intc/exynos4210_gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index 9a2254f0b13c..d79b4cfcc7c9 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -116,7 +116,7 @@ enum ExtInt {
  * which is INTG16 in Internal Interrupt Combiner.
  */
 
-static uint32_t
+static const uint32_t
 combiner_grp_to_gic_id[64-EXYNOS4210_MAX_EXT_COMBINER_OUT_IRQ][8] = {
     /* int combiner groups 16-19 */
     { }, { }, { }, { },
-- 
2.9.3

[Qemu-devel] [PATCH v3 11/11] hw/misc/exynos4210_pmu: Add support for system poweroff

[Krzysztof Kozlowski]

On all Exynos-based boards, the system powers down itself by driving
PS_HOLD signal low - eight bit in PS_HOLD_CONTROL register of PMU.
Handle writing to respective PMU register to fix power off failure:

    reboot: Power down
    Unable to poweroff system
    shutdown: 31 output lines suppressed due to ratelimiting
    Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000

    CPU: 0 PID: 1 Comm: shutdown Not tainted 4.11.0-rc8 #846
    Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
    [<c031050c>] (unwind_backtrace) from [<c030ba6c>] (show_stack+0x10/0x14)
    [<c030ba6c>] (show_stack) from [<c05b2800>] (dump_stack+0x88/0x9c)
    [<c05b2800>] (dump_stack) from [<c03d3140>] (panic+0xdc/0x268)
    [<c03d3140>] (panic) from [<c0343614>] (do_exit+0xa90/0xab4)
    [<c0343614>] (do_exit) from [<c035f2dc>] (SyS_reboot+0x164/0x1d0)
    [<c035f2dc>] (SyS_reboot) from [<c0307c80>] (ret_fast_syscall+0x0/0x3c)

Additionally the initial value of PS_HOLD has to be changed because
recent Linux kernel (v4.12-rc1) uses regmap cache for this access.
When the register is kept at reset value, the kernel will not issue a
write to it.  Usually the bootloader sets the eight bit of PS_HOLD high
so mimic its existence here.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 hw/misc/exynos4210_pmu.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/misc/exynos4210_pmu.c b/hw/misc/exynos4210_pmu.c
index 63a8ccd35559..f3f96b1f4889 100644
--- a/hw/misc/exynos4210_pmu.c
+++ b/hw/misc/exynos4210_pmu.c
@@ -26,6 +26,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
+#include "sysemu/sysemu.h"
 
 #ifndef DEBUG_PMU
 #define DEBUG_PMU           0
@@ -350,7 +351,11 @@ static const Exynos4210PmuReg exynos4210_pmu_regs[] = {
     {"PAD_RETENTION_MMCB_OPTION", PAD_RETENTION_MMCB_OPTION, 0x00000000},
     {"PAD_RETENTION_EBIA_OPTION", PAD_RETENTION_EBIA_OPTION, 0x00000000},
     {"PAD_RETENTION_EBIB_OPTION", PAD_RETENTION_EBIB_OPTION, 0x00000000},
-    {"PS_HOLD_CONTROL", PS_HOLD_CONTROL, 0x00005200},
+    /*
+     * PS_HOLD_CONTROL: reset value and manually toggle high the DATA bit.
+     * DATA bit high, set usually by bootloader, keeps system on.
+     */
+    {"PS_HOLD_CONTROL", PS_HOLD_CONTROL, 0x00005200 | BIT(8)},
     {"XUSBXTI_CONFIGURATION", XUSBXTI_CONFIGURATION, 0x00000001},
     {"XUSBXTI_STATUS", XUSBXTI_STATUS, 0x00000001},
     {"XUSBXTI_DURATION", XUSBXTI_DURATION, 0xFFF00000},
@@ -397,6 +402,12 @@ typedef struct Exynos4210PmuState {
     uint32_t reg[PMU_NUM_OF_REGISTERS];
 } Exynos4210PmuState;
 
+static void exynos4210_pmu_poweroff(void)
+{
+    PRINT_DEBUG("QEMU PMU: PS_HOLD bit down, powering off\n");
+    qemu_system_shutdown_request();
+}
+
 static uint64_t exynos4210_pmu_read(void *opaque, hwaddr offset,
                                     unsigned size)
 {
@@ -428,6 +439,13 @@ static void exynos4210_pmu_write(void *opaque, hwaddr offset,
             PRINT_DEBUG_EXTEND("%s <0x%04x> <- 0x%04x\n", reg_p->name,
                     (uint32_t)offset, (uint32_t)val);
             s->reg[i] = val;
+            if ((offset == PS_HOLD_CONTROL) && ((val & BIT(8)) == 0)) {
+                /*
+                 * We are interested only in setting data bit
+                 * of PS_HOLD_CONTROL register to indicate power off request.
+                 */
+                exynos4210_pmu_poweroff();
+            }
             return;
         }
         reg_p++;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Peter Maydell]

On 21 May 2017 at 16:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> This is a collection of three already sent patchsets [1] [2] [3].
>
>
> Changes since previous versions (v2):
> 1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
> 2. Add reviewed-by tags.
>
>
> The first patch in set is a bugfix which also exposes problem of cpuidle.
> Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
> poweroff working) but with CPU_IDLE it behaves bad due to low-power
> mode (AFTR). This was discussed at [1].

So is this a regression compared to our current model? I was
under the impression from the previous thread that it was,
which is why I didn't apply that patchset.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Krzysztof Kozlowski]

On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 May 2017 at 16:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> This is a collection of three already sent patchsets [1] [2] [3].
>>
>>
>> Changes since previous versions (v2):
>> 1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
>> 2. Add reviewed-by tags.
>>
>>
>> The first patch in set is a bugfix which also exposes problem of cpuidle.
>> Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
>> poweroff working) but with CPU_IDLE it behaves bad due to low-power
>> mode (AFTR). This was discussed at [1].
>
> So is this a regression compared to our current model? I was
> under the impression from the previous thread that it was,
> which is why I didn't apply that patchset.

It depends how wide understanding of regression you have. :)
1. Before this patch, second CPU could not boot. System was usable but
worked on only one CPU.
2. Before this patch, kernel's IRQ work is broken. None of IRQ work
are executed which is mostly visible during poweroff - system hangs on
syncing IRQ works.
3. With this patch, second CPU boots and IRQ works properly (so
poweroff is possible).
4. However with this patch, Linux kernel with cpuidle enabled (which
is also included in many kernel defconfigs), the system is very
unresponsive.

So overall... a lot of things were broken already. This patches fixes
them... but breaks different thing.

Best regards,
Krzysztof

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Peter Maydell]

On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> So is this a regression compared to our current model? I was
>> under the impression from the previous thread that it was,
>> which is why I didn't apply that patchset.
>
> It depends how wide understanding of regression you have. :)
> 1. Before this patch, second CPU could not boot. System was usable but
> worked on only one CPU.
> 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> are executed which is mostly visible during poweroff - system hangs on
> syncing IRQ works.
> 3. With this patch, second CPU boots and IRQ works properly (so
> poweroff is possible).
> 4. However with this patch, Linux kernel with cpuidle enabled (which
> is also included in many kernel defconfigs), the system is very
> unresponsive.
>
> So overall... a lot of things were broken already. This patches fixes
> them... but breaks different thing.

It sounds like it breaks a key thing, ie "boot a single cpu kernel
built from a defconfig", though. That's a regression I'd rather
not have. If there's a subset of these patches which don't break
that I'm happy to take those. Otherwise we need to investigate
and fix whatever the problem is that causes that unresponsiveness
before we can apply them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Krzysztof Kozlowski]

On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
> On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> So is this a regression compared to our current model? I was
> >> under the impression from the previous thread that it was,
> >> which is why I didn't apply that patchset.
> >
> > It depends how wide understanding of regression you have. :)
> > 1. Before this patch, second CPU could not boot. System was usable but
> > worked on only one CPU.
> > 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> > are executed which is mostly visible during poweroff - system hangs on
> > syncing IRQ works.
> > 3. With this patch, second CPU boots and IRQ works properly (so
> > poweroff is possible).
> > 4. However with this patch, Linux kernel with cpuidle enabled (which
> > is also included in many kernel defconfigs), the system is very
> > unresponsive.
> >
> > So overall... a lot of things were broken already. This patches fixes
> > them... but breaks different thing.
> 
> It sounds like it breaks a key thing, ie "boot a single cpu kernel
> built from a defconfig", though. That's a regression I'd rather
> not have.

I am not sure if I understand you correctly... the system previously
booted into one CPU mode because second CPU just could not boot. Now by
default, second CPU succeeds and this means that *default* settings are
indeed more broken than before.

On the other hand, after removing a line from hw/arm/exynos4210.c:
	qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
everything is okay. Booting of one CPU is unaffected.

> If there's a subset of these patches which don't break
> that I'm happy to take those. Otherwise we need to investigate
> and fix whatever the problem is that causes that unresponsiveness
> before we can apply them.

Yes, you can take everything except first patch
("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
and it should be fine. Rest of patches are just independent.

Best regards,
Krzysztof

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Krzysztof Kozlowski]

On Thu, Jun 01, 2017 at 07:10:19PM +0200, Krzysztof Kozlowski wrote:
> On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
> > On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >> So is this a regression compared to our current model? I was
> > >> under the impression from the previous thread that it was,
> > >> which is why I didn't apply that patchset.
> > >
> > > It depends how wide understanding of regression you have. :)
> > > 1. Before this patch, second CPU could not boot. System was usable but
> > > worked on only one CPU.
> > > 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> > > are executed which is mostly visible during poweroff - system hangs on
> > > syncing IRQ works.
> > > 3. With this patch, second CPU boots and IRQ works properly (so
> > > poweroff is possible).
> > > 4. However with this patch, Linux kernel with cpuidle enabled (which
> > > is also included in many kernel defconfigs), the system is very
> > > unresponsive.
> > >
> > > So overall... a lot of things were broken already. This patches fixes
> > > them... but breaks different thing.
> > 
> > It sounds like it breaks a key thing, ie "boot a single cpu kernel
> > built from a defconfig", though. That's a regression I'd rather
> > not have.
> 
> I am not sure if I understand you correctly... the system previously
> booted into one CPU mode because second CPU just could not boot. Now by
> default, second CPU succeeds and this means that *default* settings are
> indeed more broken than before.
> 
> On the other hand, after removing a line from hw/arm/exynos4210.c:
> 	qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
> and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
> everything is okay. Booting of one CPU is unaffected.

Ahh, and one more thing. For forced single CPU mode (mentioned above),
now it actually behaves better. Fixed GIC mappings means that SGIs are
working (execution of IRQ work is fixed) thus the system can properly
power off.

This means we could hard-code one-cpu for now and get better behavior
than previously (one CPU but with SGIs).

Best regards,
Krzysztof

> 
> > If there's a subset of these patches which don't break
> > that I'm happy to take those. Otherwise we need to investigate
> > and fix whatever the problem is that causes that unresponsiveness
> > before we can apply them.
> 
> Yes, you can take everything except first patch
> ("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
> and it should be fine. Rest of patches are just independent.

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

[Peter Maydell]

On 1 June 2017 at 18:10, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
>> It sounds like it breaks a key thing, ie "boot a single cpu kernel
>> built from a defconfig", though. That's a regression I'd rather
>> not have.
>
> I am not sure if I understand you correctly... the system previously
> booted into one CPU mode because second CPU just could not boot. Now by
> default, second CPU succeeds and this means that *default* settings are
> indeed more broken than before.
>
> On the other hand, after removing a line from hw/arm/exynos4210.c:
>         qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
> and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
> everything is okay. Booting of one CPU is unaffected.

Ah, I see now.

>> If there's a subset of these patches which don't break
>> that I'm happy to take those. Otherwise we need to investigate
>> and fix whatever the problem is that causes that unresponsiveness
>> before we can apply them.
>
> Yes, you can take everything except first patch
> ("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
> and it should be fine. Rest of patches are just independent.

Can you send a series which has just the patches that don't break,
please? I think this series needs a respin anyway because of the
make check problems (noted on the other thread).

thanks
-- PMM