Patchwork [v2,03/16] intc/arm_gic: Extract public header hw/intc/arm_gic.h

login
register
mail settings
Submitter Andreas Färber
Date July 23, 2013, 2:43 a.m.
Message ID <1374547404-11700-4-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/260955/
State New
Headers show

Comments

Andreas Färber - July 23, 2013, 2:43 a.m.
Rename NCPU to GIC_NCPU and move GICState away from gic_internal.h.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/intc/arm_gic_common.c  |  18 ++++----
 hw/intc/gic_internal.h    |  77 +--------------------------------
 include/hw/intc/arm_gic.h | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 84 deletions(-)
 create mode 100644 include/hw/intc/arm_gic.h
Peter Maydell - July 25, 2013, 3:40 p.m.
On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
> +typedef struct GICState {
> +    /*< private >*/
> +    SysBusDevice busdev;
> +    /*< public >*/
> +
> +    qemu_irq parent_irq[GIC_NCPU];
> +    bool enabled;
> +    bool cpu_enabled[GIC_NCPU];
> +
[etc]

Am I missing some reason why all these fields should be
in the section marked '< public >' ? I would have expected
that they would all be considered private, because only
the GIC implementation (and its subclasses) should be
touching them.

> +    MemoryRegion iomem; /* Distributor */
> +    /* This is just so we can have an opaque pointer which identifies
> +     * both this GIC and which CPU interface we should be accessing.
> +     */
> +    struct GICState *backref[GIC_NCPU];
> +    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    uint32_t num_irq;
> +    uint32_t revision;

...when we get down to here, should properties be
considered 'private' (ie "don't touch except via the
property APIs") or are they 'public' ?

> +} GICState;

(I noticed this because I thought I'd use these
patches as my demonstrator for the __private macro
I suggested in the other thread.)

thanks
-- PMM
Andreas Färber - July 26, 2013, 8:06 p.m.
Am 23.07.2013 04:43, schrieb Andreas Färber:
> diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
> new file mode 100644
> index 0000000..be945ec
> --- /dev/null
> +++ b/include/hw/intc/arm_gic.h
[...]
> +typedef struct GICState {
> +    /*< private >*/
> +    SysBusDevice busdev;

This missed to rename to parent_obj - therefore there's an occurrence
this patch missed. Addressed in the hw/arm/ FROM_SYSBUS() series.

Andreas

Patch

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 08560f2..a89c786 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -64,17 +64,17 @@  static const VMStateDescription vmstate_gic = {
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(enabled, GICState),
-        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, NCPU),
+        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
-        VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, NCPU),
+        VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
-        VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, NCPU),
-        VMSTATE_UINT16_ARRAY(priority_mask, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
+        VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -84,9 +84,9 @@  static void arm_gic_common_realize(DeviceState *dev, Error **errp)
     GICState *s = ARM_GIC_COMMON(dev);
     int num_irq = s->num_irq;
 
-    if (s->num_cpu > NCPU) {
+    if (s->num_cpu > GIC_NCPU) {
         error_setg(errp, "requested %u CPUs exceeds GIC maximum %d",
-                   s->num_cpu, NCPU);
+                   s->num_cpu, GIC_NCPU);
         return;
     }
     s->num_irq += GIC_BASE_IRQ;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 99a3bc3..3989fd1 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -21,16 +21,9 @@ 
 #ifndef QEMU_ARM_GIC_INTERNAL_H
 #define QEMU_ARM_GIC_INTERNAL_H
 
-#include "hw/sysbus.h"
+#include "hw/intc/arm_gic.h"
 
-/* Maximum number of possible interrupts, determined by the GIC architecture */
-#define GIC_MAXIRQ 1020
-/* First 32 are private to each CPU (SGIs and PPIs). */
-#define GIC_INTERNAL 32
-/* Maximum number of possible CPU interfaces, determined by GIC architecture */
-#define NCPU 8
-
-#define ALL_CPU_MASK ((unsigned)(((1 << NCPU) - 1)))
+#define ALL_CPU_MASK ((unsigned)(((1 << GIC_NCPU) - 1)))
 
 /* The NVIC has 16 internal vectors.  However these are not exposed
    through the normal GIC interface.  */
@@ -59,45 +52,6 @@ 
                                     s->priority2[(irq) - GIC_INTERNAL])
 #define GIC_TARGET(irq) s->irq_target[irq]
 
-typedef struct gic_irq_state {
-    /* The enable bits are only banked for per-cpu interrupts.  */
-    uint8_t enabled;
-    uint8_t pending;
-    uint8_t active;
-    uint8_t level;
-    bool model; /* 0 = N:N, 1 = 1:N */
-    bool trigger; /* nonzero = edge triggered.  */
-} gic_irq_state;
-
-typedef struct GICState {
-    SysBusDevice busdev;
-    qemu_irq parent_irq[NCPU];
-    bool enabled;
-    bool cpu_enabled[NCPU];
-
-    gic_irq_state irq_state[GIC_MAXIRQ];
-    uint8_t irq_target[GIC_MAXIRQ];
-    uint8_t priority1[GIC_INTERNAL][NCPU];
-    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
-    uint16_t last_active[GIC_MAXIRQ][NCPU];
-
-    uint16_t priority_mask[NCPU];
-    uint16_t running_irq[NCPU];
-    uint16_t running_priority[NCPU];
-    uint16_t current_pending[NCPU];
-
-    uint32_t num_cpu;
-
-    MemoryRegion iomem; /* Distributor */
-    /* This is just so we can have an opaque pointer which identifies
-     * both this GIC and which CPU interface we should be accessing.
-     */
-    struct GICState *backref[NCPU];
-    MemoryRegion cpuiomem[NCPU+1]; /* CPU interfaces */
-    uint32_t num_irq;
-    uint32_t revision;
-} GICState;
-
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
 #define REV_NVIC 0xffffffff
@@ -108,31 +62,4 @@  void gic_complete_irq(GICState *s, int cpu, int irq);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 
-#define TYPE_ARM_GIC_COMMON "arm_gic_common"
-#define ARM_GIC_COMMON(obj) \
-     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC_COMMON)
-#define ARM_GIC_COMMON_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ARMGICCommonClass, (klass), TYPE_ARM_GIC_COMMON)
-#define ARM_GIC_COMMON_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ARMGICCommonClass, (obj), TYPE_ARM_GIC_COMMON)
-
-typedef struct ARMGICCommonClass {
-    SysBusDeviceClass parent_class;
-    void (*pre_save)(GICState *s);
-    void (*post_load)(GICState *s);
-} ARMGICCommonClass;
-
-#define TYPE_ARM_GIC "arm_gic"
-#define ARM_GIC(obj) \
-     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
-#define ARM_GIC_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ARMGICClass, (klass), TYPE_ARM_GIC)
-#define ARM_GIC_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ARMGICClass, (obj), TYPE_ARM_GIC)
-
-typedef struct ARMGICClass {
-    ARMGICCommonClass parent_class;
-    DeviceRealize parent_realize;
-} ARMGICClass;
-
 #endif /* !QEMU_ARM_GIC_INTERNAL_H */
diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
new file mode 100644
index 0000000..be945ec
--- /dev/null
+++ b/include/hw/intc/arm_gic.h
@@ -0,0 +1,108 @@ 
+/*
+ * ARM GIC support
+ *
+ * Copyright (c) 2012 Linaro Limited
+ * Written by Peter Maydell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_ARM_GIC_H
+#define HW_ARM_GIC_H
+
+#include "hw/sysbus.h"
+
+/* Maximum number of possible interrupts, determined by the GIC architecture */
+#define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
+/* Maximum number of possible CPU interfaces, determined by GIC architecture */
+#define GIC_NCPU 8
+
+typedef struct gic_irq_state {
+    /* The enable bits are only banked for per-cpu interrupts.  */
+    uint8_t enabled;
+    uint8_t pending;
+    uint8_t active;
+    uint8_t level;
+    bool model; /* 0 = N:N, 1 = 1:N */
+    bool trigger; /* nonzero = edge triggered.  */
+} gic_irq_state;
+
+typedef struct GICState {
+    /*< private >*/
+    SysBusDevice busdev;
+    /*< public >*/
+
+    qemu_irq parent_irq[GIC_NCPU];
+    bool enabled;
+    bool cpu_enabled[GIC_NCPU];
+
+    gic_irq_state irq_state[GIC_MAXIRQ];
+    uint8_t irq_target[GIC_MAXIRQ];
+    uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
+    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
+    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
+
+    uint16_t priority_mask[GIC_NCPU];
+    uint16_t running_irq[GIC_NCPU];
+    uint16_t running_priority[GIC_NCPU];
+    uint16_t current_pending[GIC_NCPU];
+
+    uint32_t num_cpu;
+
+    MemoryRegion iomem; /* Distributor */
+    /* This is just so we can have an opaque pointer which identifies
+     * both this GIC and which CPU interface we should be accessing.
+     */
+    struct GICState *backref[GIC_NCPU];
+    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
+    uint32_t num_irq;
+    uint32_t revision;
+} GICState;
+
+#define TYPE_ARM_GIC_COMMON "arm_gic_common"
+#define ARM_GIC_COMMON(obj) \
+     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC_COMMON)
+#define ARM_GIC_COMMON_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ARMGICCommonClass, (klass), TYPE_ARM_GIC_COMMON)
+#define ARM_GIC_COMMON_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ARMGICCommonClass, (obj), TYPE_ARM_GIC_COMMON)
+
+typedef struct ARMGICCommonClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    void (*pre_save)(GICState *s);
+    void (*post_load)(GICState *s);
+} ARMGICCommonClass;
+
+#define TYPE_ARM_GIC "arm_gic"
+#define ARM_GIC(obj) \
+     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
+#define ARM_GIC_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ARMGICClass, (klass), TYPE_ARM_GIC)
+#define ARM_GIC_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ARMGICClass, (obj), TYPE_ARM_GIC)
+
+typedef struct ARMGICClass {
+    /*< private >*/
+    ARMGICCommonClass parent_class;
+    /*< public >*/
+
+    DeviceRealize parent_realize;
+} ARMGICClass;
+
+#endif