[Qemu-devel] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Qemu-devel] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

Hi,


The first patch fixes GIC and brings the secondary CPU. Unfortunately,
this brings up to light an annoying issue with CPUIDLE driver.


Changes since v1
================
1. Use one cast to obj in patch 1/5 (suggested by Peter).
2. Add Peter's review tags.
3. Re-order patches 3 and 4 to silence the checkpatch warning earlier.


Overview of the problem
=======================
On Exynos4210, by default Linux kernel uses cpuidle driver which tries
to enter low power mode, called AFTR (Arm Off, Top Running).  On real
hardware this brings some power savings.  This AFTR mode requires second
CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
1. Turns off second CPU,
2. Enters AFTR on CPU0.

However the QEMU system is then totally unresponsive (e.g. on serial console)
and RCU stalls appear from time to time.  I spent some time on it and did not
find the real cause behind the lag.  Maybe it is because the second CPU
does not really power down itself and system just burns the cycles under spin
locks?

Looking at recent stable kernels:
 - 3.10, 3.16 - works fine with two CPUs (no CPUIDLE driver),
 - 4.1 and newer - stalls and are unresponsive due to CPUIDLE being enabled.

The cpuidle driver is not relying on DTS. It is just enabled in exynos_defconfig
and works.

Workarounds
===========
1. Boot with only 1 cpu
2. Build a kernel with disabled CONFIG_CPU_IDLE



Best regards,
Krzysztof

Krzysztof Kozlowski (5):
  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/intc/exynos4210_gic.c  | 33 +++++++++++++++++++------------
 hw/timer/exynos4210_mct.c | 50 ++++++++++++++++++++---------------------------
 2 files changed, 41 insertions(+), 42 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/5] 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

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 v2 2/5] 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 v2 3/5] 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 0c189348ae04..cd290637f357 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -936,7 +936,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) {
@@ -1161,7 +1161,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 v2 4/5] 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 cd290637f357..4dd3e441e2e6 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1015,9 +1015,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:
@@ -1066,7 +1066,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:
@@ -1152,23 +1151,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;
@@ -1206,7 +1205,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 &
@@ -1287,7 +1285,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);
 
@@ -1315,7 +1312,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);
 
@@ -1352,13 +1348,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 v2 5/5] 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 4dd3e441e2e6..6069116942a4 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -172,13 +172,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

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 2/5] hw/intc/exynos4210_gic: Use more meaningful name for local variable

[Philippe Mathieu-Daudé]

On 03/13/2017 03:04 PM, Krzysztof Kozlowski wrote:
> 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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,
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 3/5] hw/timer/exynos4210_mct: Fix checkpatch style errors

[Philippe Mathieu-Daudé]

On 03/13/2017 03:04 PM, Krzysztof Kozlowski wrote:
> 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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 0c189348ae04..cd290637f357 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -936,7 +936,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) {
> @@ -1161,7 +1161,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);
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 4/5] hw/timer/exynos4210_mct: Cleanup indentation and empty new lines

[Philippe Mathieu-Daudé]

On 03/13/2017 03:04 PM, Krzysztof Kozlowski wrote:
> 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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 cd290637f357..4dd3e441e2e6 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -1015,9 +1015,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:
> @@ -1066,7 +1066,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:
> @@ -1152,23 +1151,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;
> @@ -1206,7 +1205,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 &
> @@ -1287,7 +1285,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);
>
> @@ -1315,7 +1312,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);
>
> @@ -1352,13 +1348,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);
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU

[Philippe Mathieu-Daudé]

On 03/13/2017 03:04 PM, Krzysztof Kozlowski wrote:
> 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
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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;
>  }
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Alex Bennée]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> Hi,
>
<snip>
>
> Overview of the problem
> =======================
> On Exynos4210, by default Linux kernel uses cpuidle driver which tries
> to enter low power mode, called AFTR (Arm Off, Top Running).  On real
> hardware this brings some power savings.  This AFTR mode requires second
> CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
> 1. Turns off second CPU,
> 2. Enters AFTR on CPU0.
>
> However the QEMU system is then totally unresponsive (e.g. on serial console)
> and RCU stalls appear from time to time.  I spent some time on it and did not
> find the real cause behind the lag.  Maybe it is because the second CPU
> does not really power down itself and system just burns the cycles under spin
> locks?

Was this tested post the MTTCG merge? Maybe there is an interaction
between power saving/halting the vCPU?

I mention this because I already made some minor tweaks for handing the
WFI instruction in MTTCG. See commit c22edfebff. You can test this by
forcing the original behaviour by adding:

  -accel tcg,thread=single

to your command line.

>
> Looking at recent stable kernels:
>  - 3.10, 3.16 - works fine with two CPUs (no CPUIDLE driver),
>  - 4.1 and newer - stalls and are unresponsive due to CPUIDLE being enabled.
>
> The cpuidle driver is not relying on DTS. It is just enabled in exynos_defconfig
> and works.
>
> Workarounds
> ===========
> 1. Boot with only 1 cpu
> 2. Build a kernel with disabled CONFIG_CPU_IDLE
>
>
>
> Best regards,
> Krzysztof
>
> Krzysztof Kozlowski (5):
>   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/intc/exynos4210_gic.c  | 33 +++++++++++++++++++------------
>  hw/timer/exynos4210_mct.c | 50 ++++++++++++++++++++---------------------------
>  2 files changed, 41 insertions(+), 42 deletions(-)


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

On Tue, Mar 14, 2017 at 04:55:34PM +0000, Alex Bennée wrote:
> 
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
> > Hi,
> >
> <snip>
> >
> > Overview of the problem
> > =======================
> > On Exynos4210, by default Linux kernel uses cpuidle driver which tries
> > to enter low power mode, called AFTR (Arm Off, Top Running).  On real
> > hardware this brings some power savings.  This AFTR mode requires second
> > CPU to be off, so the driver (coupled cpuidle driver) when system is idle:
> > 1. Turns off second CPU,
> > 2. Enters AFTR on CPU0.
> >
> > However the QEMU system is then totally unresponsive (e.g. on serial console)
> > and RCU stalls appear from time to time.  I spent some time on it and did not
> > find the real cause behind the lag.  Maybe it is because the second CPU
> > does not really power down itself and system just burns the cycles under spin
> > locks?
> 
> Was this tested post the MTTCG merge? Maybe there is an interaction
> between power saving/halting the vCPU?
> 
> I mention this because I already made some minor tweaks for handing the
> WFI instruction in MTTCG. See commit c22edfebff. You can test this by
> forcing the original behaviour by adding:
> 
>   -accel tcg,thread=single
> 
> to your command line.
>

I am testing it on v2.8.0-2005-g17783ac828ad. Without any -accel, the
system boots but responsiveness of serial console is bad. Bad but at
least it is responding still a little bit.

With the -accel you mentioned, system does not even reach user-space
(initramfs).

I tried rebasing on newer (e.g. v2.8.0-2182-g5e2fb7c598c6) but
compilation fails all the time on:
/home/dev/qemu/qemu/vl.c: In function ‘main’:
/home/dev/qemu/qemu/vl.c:3131:18: error: ‘QEMU_OPTION_blockdev’ undeclared (first use in this function)
             case QEMU_OPTION_blockdev:


Best regards,
Krzysztof

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Alex Bennée]

That means you're on a pre-mttcg tree so that's not the reason.

On 14 Mar 2017 5:35 pm, "Krzysztof Kozlowski" <krzk@kernel.org> wrote:

> On Tue, Mar 14, 2017 at 04:55:34PM +0000, Alex Bennée wrote:
> >
> > Krzysztof Kozlowski <krzk@kernel.org> writes:
> >
> > > Hi,
> > >
> > <snip>
> > >
> > > Overview of the problem
> > > =======================
> > > On Exynos4210, by default Linux kernel uses cpuidle driver which tries
> > > to enter low power mode, called AFTR (Arm Off, Top Running).  On real
> > > hardware this brings some power savings.  This AFTR mode requires
> second
> > > CPU to be off, so the driver (coupled cpuidle driver) when system is
> idle:
> > > 1. Turns off second CPU,
> > > 2. Enters AFTR on CPU0.
> > >
> > > However the QEMU system is then totally unresponsive (e.g. on serial
> console)
> > > and RCU stalls appear from time to time.  I spent some time on it and
> did not
> > > find the real cause behind the lag.  Maybe it is because the second CPU
> > > does not really power down itself and system just burns the cycles
> under spin
> > > locks?
> >
> > Was this tested post the MTTCG merge? Maybe there is an interaction
> > between power saving/halting the vCPU?
> >
> > I mention this because I already made some minor tweaks for handing the
> > WFI instruction in MTTCG. See commit c22edfebff. You can test this by
> > forcing the original behaviour by adding:
> >
> >   -accel tcg,thread=single
> >
> > to your command line.
> >
>
> I am testing it on v2.8.0-2005-g17783ac828ad. Without any -accel, the
> system boots but responsiveness of serial console is bad. Bad but at
> least it is responding still a little bit.
>
> With the -accel you mentioned, system does not even reach user-space
> (initramfs).
>
> I tried rebasing on newer (e.g. v2.8.0-2182-g5e2fb7c598c6) but
> compilation fails all the time on:
> /home/dev/qemu/qemu/vl.c: In function ‘main’:
> /home/dev/qemu/qemu/vl.c:3131:18: error: ‘QEMU_OPTION_blockdev’
> undeclared (first use in this function)
>              case QEMU_OPTION_blockdev:
>
>
> Best regards,
> Krzysztof
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

On Tue, Mar 14, 2017 at 06:24:33PM +0000, Alex Bennée wrote:
> That means you're on a pre-mttcg tree so that's not the reason.

I managed to build it on current master (v2.8.0-2182-g5e2fb7c598c6). It
behaves the same (with "-accel tcg,thread=single": cannot reach
switching to init).

Best regards,
Krzysztof

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Alex Bennée]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On Tue, Mar 14, 2017 at 06:24:33PM +0000, Alex Bennée wrote:
>> That means you're on a pre-mttcg tree so that's not the reason.
>
> I managed to build it on current master (v2.8.0-2182-g5e2fb7c598c6). It
> behaves the same (with "-accel tcg,thread=single": cannot reach
> switching to init).

Interesting. Can you post your exact qemu command line and I'll have a
go at reproducing.

In the meantime you can always --enable-debug-tcg your build and see if
any of the asserts fire.

>
> Best regards,
> Krzysztof


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/5] hw: arm: exynos: Bring up secondary CPU + CPUIDLE issue

[Krzysztof Kozlowski]

On Wed, Mar 15, 2017 at 08:05:56AM +0000, Alex Bennée wrote:
> 
> Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
> > On Tue, Mar 14, 2017 at 06:24:33PM +0000, Alex Bennée wrote:
> >> That means you're on a pre-mttcg tree so that's not the reason.
> >
> > I managed to build it on current master (v2.8.0-2182-g5e2fb7c598c6). It
> > behaves the same (with "-accel tcg,thread=single": cannot reach
> > switching to init).
> 
> Interesting. Can you post your exact qemu command line and I'll have a
> go at reproducing.

I build usually with:
../qemu/configure --cc="ccache cc" --cxx="ccache c++" --enable-debug --enable-fdt \
	--enable-kvm --enable-libusb --enable-libssh2 --enable-lzo \
	--enable-bzip2 --enable-curses --enable-gtk  --enable-cap-ng

Running with command:
arm-softmmu/qemu-system-arm -m 1024 -M smdkc210 -smp 2 -append \
	'console=ttySAC0,115200n8 console=ttyS0 earlyprintk' -d guest_errors \
	-serial stdio -D ..//log-smdkc210.log -kernel ../cur-linux/zImage
	-dtb ../cur-linux/dts/exynos4210-smdkv310.dtb
	-initrd ../armv7-odroidu3-exynos-v4.10-initramfs.cpio.gz

Plus the -accel line, depending on the needs.

The initramfs is a simple one from Arch Arm.

> 
> In the meantime you can always --enable-debug-tcg your build and see if
> any of the asserts fire.

enable-debug-tcg did not change much - no asserts. The last dmesg is:
[   72.616013] Registering SWP/SWPB emulation handler
[   74.484873] s3c64xx-spi 13940000.spi: spi bus clock parent not specified, using clock at index 0 as parent
[   74.485994] s3c64xx-spi 13940000.spi: number of chip select lines not specified, assuming 1 chip select line
[   74.493249] s3c64xx-spi 13940000.spi: Failed to get RX DMA channel
[   74.503885] hctosys: unable to open rtc device (rtc0)
[   74.523004] ALSA device list:
[   74.523711]   No soundcards found.
[   74.542278] samsung-uart 13800000.serial: DMA request failed, DMA will not be used


Best regards,
Krzysztof

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/5] hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU

[Krzysztof Kozlowski]

On Mon, Mar 13, 2017 at 04:32:40PM -0300, Philippe Mathieu-Daudé wrote:
> On 03/13/2017 03:04 PM, Krzysztof Kozlowski wrote:
> > 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
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Beside bringing secondary CPU, this patch fixes also Software Generated
Interrupts. Without it, none of the SGIs are coming (except CPU wakeup):
  IPI0:       6281  CPU wakeup interrupts
  IPI1:          0  Timer broadcast interrupts
  IPI2:          0  Rescheduling interrupts
  IPI3:          0  Function call interrupts
  IPI4:          0  CPU stop interrupts
  IPI5:          0  IRQ work interrupts
  IPI6:          0  completion interrupts

This is pretty annoying because lack of SGIs means lack of IPIs thus for
example IRQ work cannot be executed. Without IRQ work, the kernel hangs
on power down on cpufreq shutdown because in cpufreq_dbs_governor_stop()
there is irq_work_sync() but none of irq_work interrupts were handled.

I still did not solve the issue with cpuidle (AFTR). I am trying to
implement CPU power off (needed for AFTR) but it is one-way so far (no
wakeup). Anyway the workaround is to just disable cpuidle.

Overall, what is the status of this patch? Should I resend? I can also
extend the commit description with paragraph about SGI/IPI.

Best regards,
Krzysztof