Patchwork [v1,4/6] arm: mptimer: Remove WDT distinction

login
register
mail settings
Submitter Peter Crosthwaite
Date Feb. 8, 2013, 4:03 a.m.
Message ID <5e060187-7c24-4952-81e3-bcfafafae032@VA3EHSMHS037.ehs.local>
Download mbox | patch
Permalink /patch/219044/
State New
Headers show

Comments

Peter Crosthwaite - Feb. 8, 2013, 4:03 a.m.
In QEMU emulation, there is no functional difference between the ARM mpcore
private timers and watchdogs. Removed all the distinction between the two from
arm_mptimer.c and converted it to be just the mptimer. a9mpcore and arm11mpcore
just instantiate the same mptimer object twice to get both timer and WDT.

If in the future we want to make the WDT functionally different then we can use
either QOM heirachy to derive WDT from from mptimer, or we can add a property
"is-wdt" or some such.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/a9mpcore.c    |   17 +++++++++-----
 hw/arm11mpcore.c |   21 ++++++++++++-----
 hw/arm_mptimer.c |   63 ++++++++++++------------------------------------------
 3 files changed, 40 insertions(+), 61 deletions(-)
Peter Maydell - Feb. 18, 2013, 6:37 p.m.
On 8 February 2013 04:03, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> In QEMU emulation, there is no functional difference between the ARM mpcore
> private timers and watchdogs. Removed all the distinction between the two from
> arm_mptimer.c and converted it to be just the mptimer. a9mpcore and arm11mpcore
> just instantiate the same mptimer object twice to get both timer and WDT.
>
> If in the future we want to make the WDT functionally different then we can use
> either QOM heirachy to derive WDT from from mptimer, or we can add a property

"hierarchy".

> "is-wdt" or some such.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

I was sceptical about this change initially but you're right that it's much
cleaner this way.

One minor nit:

>  static const VMStateDescription vmstate_arm_mptimer = {
>      .name = "arm_mptimer",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_STRUCT_ARRAY(timerblock, ARMMPTimerState, (MAX_CPUS * 2),
> -                             1, vmstate_timerblock, TimerBlock),
> +        VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
> +                                     2, vmstate_timerblock, TimerBlock),
>          VMSTATE_END_OF_LIST()
>      }

This changes us from sending every timerblock to only the ones
that actually exist on this machine config, which renders the
comment in arm_mptimer_reset() irrelevant, so it should be deleted.

-- PMM
Peter Crosthwaite - Feb. 20, 2013, 12:24 a.m.
On Tue, Feb 19, 2013 at 4:37 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 February 2013 04:03, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> In QEMU emulation, there is no functional difference between the ARM mpcore
>> private timers and watchdogs. Removed all the distinction between the two from
>> arm_mptimer.c and converted it to be just the mptimer. a9mpcore and arm11mpcore
>> just instantiate the same mptimer object twice to get both timer and WDT.
>>
>> If in the future we want to make the WDT functionally different then we can use
>> either QOM heirachy to derive WDT from from mptimer, or we can add a property
>
> "hierarchy".
>

Fixed.

>> "is-wdt" or some such.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> I was sceptical about this change initially but you're right that it's much
> cleaner this way.
>
> One minor nit:
>
>>  static const VMStateDescription vmstate_arm_mptimer = {
>>      .name = "arm_mptimer",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_STRUCT_ARRAY(timerblock, ARMMPTimerState, (MAX_CPUS * 2),
>> -                             1, vmstate_timerblock, TimerBlock),
>> +        VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
>> +                                     2, vmstate_timerblock, TimerBlock),
>>          VMSTATE_END_OF_LIST()
>>      }
>
> This changes us from sending every timerblock to only the ones
> that actually exist on this machine config, which renders the
> comment in arm_mptimer_reset() irrelevant, so it should be deleted.
>

Fixed.

Regards,
Peter

> -- PMM
>

Patch

diff --git a/hw/a9mpcore.c b/hw/a9mpcore.c
index 6ad7c6d..6520c24 100644
--- a/hw/a9mpcore.c
+++ b/hw/a9mpcore.c
@@ -128,7 +128,7 @@  static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
-    SysBusDevice *busdev, *gicbusdev;
+    SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev;
     DeviceState *qdev;
     int i;
 
@@ -147,7 +147,12 @@  static int a9mp_priv_init(SysBusDevice *dev)
     qdev = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(qdev, "num-cpu", s->num_cpu);
     qdev_init_nofail(qdev);
-    busdev = SYS_BUS_DEVICE(qdev);
+    timerbusdev = SYS_BUS_DEVICE(qdev);
+
+    qdev = qdev_create(NULL, "arm_mptimer");
+    qdev_prop_set_uint32(qdev, "num-cpu", s->num_cpu);
+    qdev_init_nofail(qdev);
+    wdtbusdev = SYS_BUS_DEVICE(qdev);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
      *  0x0000-0x00ff -- Snoop Control Unit
@@ -170,9 +175,9 @@  static int a9mp_priv_init(SysBusDevice *dev)
      * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
      */
     memory_region_add_subregion(&s->container, 0x600,
-                                sysbus_mmio_get_region(busdev, 0));
+                                sysbus_mmio_get_region(timerbusdev, 0));
     memory_region_add_subregion(&s->container, 0x620,
-                                sysbus_mmio_get_region(busdev, 1));
+                                sysbus_mmio_get_region(wdtbusdev, 0));
     memory_region_add_subregion(&s->container, 0x1000,
                                 sysbus_mmio_get_region(gicbusdev, 0));
 
@@ -183,9 +188,9 @@  static int a9mp_priv_init(SysBusDevice *dev)
      */
     for (i = 0; i < s->num_cpu; i++) {
         int ppibase = (s->num_irq - 32) + i * 32;
-        sysbus_connect_irq(busdev, i * 2,
+        sysbus_connect_irq(timerbusdev, i,
                            qdev_get_gpio_in(s->gic, ppibase + 29));
-        sysbus_connect_irq(busdev, i * 2 + 1,
+        sysbus_connect_irq(wdtbusdev, i,
                            qdev_get_gpio_in(s->gic, ppibase + 30));
     }
     return 0;
diff --git a/hw/arm11mpcore.c b/hw/arm11mpcore.c
index 4e32dc1..c35911c 100644
--- a/hw/arm11mpcore.c
+++ b/hw/arm11mpcore.c
@@ -21,6 +21,7 @@  typedef struct MPCorePrivState {
     MemoryRegion iomem;
     MemoryRegion container;
     DeviceState *mptimer;
+    DeviceState *wdtimer;
     DeviceState *gic;
     uint32_t num_irq;
 } MPCorePrivState;
@@ -84,7 +85,8 @@  static void mpcore_priv_map_setup(MPCorePrivState *s)
 {
     int i;
     SysBusDevice *gicbusdev = SYS_BUS_DEVICE(s->gic);
-    SysBusDevice *busdev = SYS_BUS_DEVICE(s->mptimer);
+    SysBusDevice *timerbusdev = SYS_BUS_DEVICE(s->mptimer);
+    SysBusDevice *wdtbusdev = SYS_BUS_DEVICE(s->wdtimer);
     memory_region_init(&s->container, "mpcode-priv-container", 0x2000);
     memory_region_init_io(&s->iomem, &mpcore_scu_ops, s, "mpcore-scu", 0x100);
     memory_region_add_subregion(&s->container, 0, &s->iomem);
@@ -99,11 +101,13 @@  static void mpcore_priv_map_setup(MPCorePrivState *s)
     /* Add the regions for timer and watchdog for "current CPU" and
      * for each specific CPU.
      */
-    for (i = 0; i < (s->num_cpu + 1) * 2; i++) {
+    for (i = 0; i < (s->num_cpu + 1); i++) {
         /* Timers at 0x600, 0x700, ...; watchdogs at 0x620, 0x720, ... */
-        hwaddr offset = 0x600 + (i >> 1) * 0x100 + (i & 1) * 0x20;
+        hwaddr offset = 0x600 + i * 0x100;
         memory_region_add_subregion(&s->container, offset,
-                                    sysbus_mmio_get_region(busdev, i));
+                                    sysbus_mmio_get_region(timerbusdev, i));
+        memory_region_add_subregion(&s->container, offset + 0x20,
+                                    sysbus_mmio_get_region(wdtbusdev, i));
     }
     memory_region_add_subregion(&s->container, 0x1000,
                                 sysbus_mmio_get_region(gicbusdev, 0));
@@ -112,9 +116,9 @@  static void mpcore_priv_map_setup(MPCorePrivState *s)
      */
     for (i = 0; i < s->num_cpu; i++) {
         int ppibase = (s->num_irq - 32) + i * 32;
-        sysbus_connect_irq(busdev, i * 2,
+        sysbus_connect_irq(timerbusdev, i,
                            qdev_get_gpio_in(s->gic, ppibase + 29));
-        sysbus_connect_irq(busdev, i * 2 + 1,
+        sysbus_connect_irq(wdtbusdev, i,
                            qdev_get_gpio_in(s->gic, ppibase + 30));
     }
 }
@@ -139,6 +143,11 @@  static int mpcore_priv_init(SysBusDevice *dev)
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
     qdev_init_nofail(s->mptimer);
+
+    s->wdtimer = qdev_create(NULL, "arm_mptimer");
+    qdev_prop_set_uint32(s->wdtimer, "num-cpu", s->num_cpu);
+    qdev_init_nofail(s->wdtimer);
+
     mpcore_priv_map_setup(s);
     sysbus_init_mmio(dev, &s->container);
     return 0;
diff --git a/hw/arm_mptimer.c b/hw/arm_mptimer.c
index de0ef36..0059780 100644
--- a/hw/arm_mptimer.c
+++ b/hw/arm_mptimer.c
@@ -43,8 +43,8 @@  typedef struct {
 typedef struct {
     SysBusDevice busdev;
     uint32_t num_cpu;
-    TimerBlock timerblock[MAX_CPUS * 2];
-    MemoryRegion iomem[2];
+    TimerBlock timerblock[MAX_CPUS];
+    MemoryRegion iomem;
 } ARMMPTimerState;
 
 static inline int get_current_cpu(ARMMPTimerState *s)
@@ -166,7 +166,7 @@  static uint64_t arm_thistimer_read(void *opaque, hwaddr addr,
 {
     ARMMPTimerState *s = (ARMMPTimerState *)opaque;
     int id = get_current_cpu(s);
-    return timerblock_read(&s->timerblock[id * 2], addr, size);
+    return timerblock_read(&s->timerblock[id], addr, size);
 }
 
 static void arm_thistimer_write(void *opaque, hwaddr addr,
@@ -174,23 +174,7 @@  static void arm_thistimer_write(void *opaque, hwaddr addr,
 {
     ARMMPTimerState *s = (ARMMPTimerState *)opaque;
     int id = get_current_cpu(s);
-    timerblock_write(&s->timerblock[id * 2], addr, value, size);
-}
-
-static uint64_t arm_thiswdog_read(void *opaque, hwaddr addr,
-                                  unsigned size)
-{
-    ARMMPTimerState *s = (ARMMPTimerState *)opaque;
-    int id = get_current_cpu(s);
-    return timerblock_read(&s->timerblock[id * 2 + 1], addr, size);
-}
-
-static void arm_thiswdog_write(void *opaque, hwaddr addr,
-                               uint64_t value, unsigned size)
-{
-    ARMMPTimerState *s = (ARMMPTimerState *)opaque;
-    int id = get_current_cpu(s);
-    timerblock_write(&s->timerblock[id * 2 + 1], addr, value, size);
+    timerblock_write(&s->timerblock[id], addr, value, size);
 }
 
 static const MemoryRegionOps arm_thistimer_ops = {
@@ -203,16 +187,6 @@  static const MemoryRegionOps arm_thistimer_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static const MemoryRegionOps arm_thiswdog_ops = {
-    .read = arm_thiswdog_read,
-    .write = arm_thiswdog_write,
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4,
-    },
-    .endianness = DEVICE_NATIVE_ENDIAN,
-};
-
 static const MemoryRegionOps timerblock_ops = {
     .read = timerblock_read,
     .write = timerblock_write,
@@ -255,29 +229,20 @@  static int arm_mptimer_init(SysBusDevice *dev)
     if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
         hw_error("%s: num-cpu must be between 1 and %d\n", __func__, MAX_CPUS);
     }
-    /* We implement one timer and one watchdog block per CPU, and
-     * expose multiple MMIO regions:
+    /* We implement one timer block per CPU, and expose multiple MMIO regions:
      *  * region 0 is "timer for this core"
-     *  * region 1 is "watchdog for this core"
-     *  * region 2 is "timer for core 0"
-     *  * region 3 is "watchdog for core 0"
-     *  * region 4 is "timer for core 1"
-     *  * region 5 is "watchdog for core 1"
+     *  * region 1 is "timer for core 0"
+     *  * region 2 is "timer for core 1"
      * and so on.
      * The outgoing interrupt lines are
      *  * timer for core 0
-     *  * watchdog for core 0
      *  * timer for core 1
-     *  * watchdog for core 1
      * and so on.
      */
-    memory_region_init_io(&s->iomem[0], &arm_thistimer_ops, s,
+    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
                           "arm_mptimer_timer", 0x20);
-    sysbus_init_mmio(dev, &s->iomem[0]);
-    memory_region_init_io(&s->iomem[1], &arm_thiswdog_ops, s,
-                          "arm_mptimer_wdog", 0x20);
-    sysbus_init_mmio(dev, &s->iomem[1]);
-    for (i = 0; i < (s->num_cpu * 2); i++) {
+    sysbus_init_mmio(dev, &s->iomem);
+    for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
         tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb);
         sysbus_init_irq(dev, &tb->irq);
@@ -305,11 +270,11 @@  static const VMStateDescription vmstate_timerblock = {
 
 static const VMStateDescription vmstate_arm_mptimer = {
     .name = "arm_mptimer",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT_ARRAY(timerblock, ARMMPTimerState, (MAX_CPUS * 2),
-                             1, vmstate_timerblock, TimerBlock),
+        VMSTATE_STRUCT_VARRAY_UINT32(timerblock, ARMMPTimerState, num_cpu,
+                                     2, vmstate_timerblock, TimerBlock),
         VMSTATE_END_OF_LIST()
     }
 };