enable HiP04 SoC

There's a little difference between ARM GIC and HiP04 GIC.

* HiP04 GIC could support 16 cores at most, and ARM GIC could support 8 cores at most. So the difination on GIC_DIST_TARGET registers are different since CPU interfaces are increased from 8-bit to 16-bit.

* HiP04 GIC could support 510 interrupts at most, and ARM GIC could support 1020 interrupts at most.

Signed-off-by Haojian Zhuang <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt |   1 +
 drivers/irqchip/irq-gic.c                     | 159 ++++++++++++++++++++------
 2 files changed, 124 insertions(+), 36 deletions(-)

Documentation/devicetree/bindings/arm/gic.txt +1 -0

index 5573c08..150f7d6 100644
@@ -16,6 +16,7 @@ Main node required properties:
        "arm,cortex-a9-gic"
        "arm,cortex-a7-gic"
        "arm,arm11mp-gic"
+       "hisilicon,hip04-gic"
 - interrupt-controller : Identifies the node as an interrupt controller
 - #interrupt-cells : Specifies the number of cells needed to encode an
   interrupt source.  The type shall be a <u32> and the value shall be 3.

drivers/irqchip/irq-gic.c +6 -2

index f711fb6..64af475 100644
@@ -68,6 +68,7 @@ struct gic_chip_data {
 #ifdef CONFIG_GIC_NON_BANKED
        void __iomem *(*get_base)(union gic_base *);
 #endif
+       u32 nr_cpu_if;
 };
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);

@@ -76,9 +77,11 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
  * by the GIC itself.
+ *
+ * Hisilicon HiP04 extends the number of CPU interface from 8 to 16.
  */
-#define NR_GIC_CPU_IF 8
-static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
+#define NR_GIC_CPU_IF  16
+static u16 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
 
 /*
  * Supported arch specific GIC irq extension.

@@ -241,20 +244,62 @@ static int gic_retrigger(struct irq_data *d)
        return 0;
 }
 
+static bool gic_is_standard(struct gic_chip_data *gic_data)

drivers/irqchip/irq-gic.c +48 -13

+       return (i + GIC_DIST_TARGET);
+}
+
 #ifdef CONFIG_SMP
+static u32 irq_to_core_shift(struct irq_data *d)
+{
+       struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+       unsigned int i = gic_irq(d);
+
+       if (gic_is_standard(gic_data))
+               return ((i % 4) << 3);
+       return ((i % 2) << 4);
+}
+
+static u32 irq_to_core_mask(struct irq_data *d)
+{
+       struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+       u32 mask;
+       /* ARM GIC, nr_cpu_if == 8; HiP04 GIC, nr_cpu_if == 16 */
+       mask = (1 << gic_data->nr_cpu_if) - 1;
+       return (mask << irq_to_core_shift(d));
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
                            bool force)
 {
-       void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-       unsigned int shift = (gic_irq(d) % 4) * 8;
+       void __iomem *reg;
+       struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+       unsigned int shift = irq_to_core_shift(d);
        unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
        u32 val, mask, bit;
 
-       if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
+       if (cpu >= gic_data->nr_cpu_if || cpu >= nr_cpu_ids)
                return -EINVAL;
 
+       reg = gic_dist_base(d) + irq_to_target_reg(gic_data, gic_irq(d));
+
        raw_spin_lock(&irq_controller_lock);
-       mask = 0xff << shift;
+       mask = irq_to_core_mask(d);
        bit = gic_cpu_map[cpu] << shift;
        val = readl_relaxed(reg) & ~mask;
        writel_relaxed(val | bit, reg);

@@ -354,15 +399,20 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
        irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
-static u8 gic_get_cpumask(struct gic_chip_data *gic)
+static u16 gic_get_cpumask(struct gic_chip_data *gic)
 {
        void __iomem *base = gic_data_dist_base(gic);
        u32 mask, i;
 
-       for (i = mask = 0; i < 32; i += 4) {
-               mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+       /*
+        * ARM GIC uses 8 registers for interrupt 0-31,
+        * HiP04 GIC uses 16 registers for interrupt 0-31.
+        */
+       for (i = mask = 0; i < 32; i++) {
+               mask = readl_relaxed(base + irq_to_target_reg(gic, i));
                mask |= mask >> 16;
-               mask |= mask >> 8;
+               if (gic_is_standard(gic))
+                       mask |= mask >> 8;
                if (mask)
                        break;
        }

@@ -370,6 +420,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
        if (!mask)
                pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
+       /* ARM GIC needs 8-bit cpu mask, HiP04 GIC needs 16-bit cpu mask. */
+       if (gic_is_standard(gic))
+               mask &= 0xff;
+
        return mask;
 }
 

@@ -392,10 +446,11 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
         * Set all global interrupts to this CPU only.
         */
        cpumask = gic_get_cpumask(gic);
-       cpumask |= cpumask << 8;
+       if (gic_is_standard(gic))
+               cpumask |= cpumask << 8;
        cpumask |= cpumask << 16;
-       for (i = 32; i < gic_irqs; i += 4)
-               writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
+       for (i = 32; i < gic_irqs; i++)
+               writel_relaxed(cpumask, base + irq_to_target_reg(gic, i));
 
        /*
         * Set priority on all global interrupts.

@@ -423,7 +478,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
        /*
         * Get what the GIC says our CPU mask is.
         */
-       BUG_ON(cpu >= NR_GIC_CPU_IF);
+       BUG_ON(cpu >= gic->nr_cpu_if);
        cpu_mask = gic_get_cpumask(gic);
        gic_cpu_map[cpu] = cpu_mask;
 

@@ -431,7 +486,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
         * Clear our mask from the other map entries in case they're
         * still undefined.
         */
-       for (i = 0; i < NR_GIC_CPU_IF; i++)
+       for (i = 0; i < gic->nr_cpu_if; i++)
                if (i != cpu)
                        gic_cpu_map[i] &= ~cpu_mask;
 

drivers/irqchip/irq-gic.c +2 -1

+       }
 
        for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
                gic_data[gic_nr].saved_spi_enable[i] =

@@ -502,7 +559,7 @@ static void gic_dist_save(unsigned int gic_nr)
  */
 static void gic_dist_restore(unsigned int gic_nr)
 {
-       unsigned int gic_irqs;
+       unsigned int gic_irqs, target_reg = 0;
        unsigned int i;
        void __iomem *dist_base;
 

drivers/irqchip/irq-gic.c +44 -16

+       }
 
        for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
                writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],

@@ -665,9 +724,19 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
         */
        dmb(ishst);
 
-       /* this always happens on GIC0 */
-       writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
-
+       /*
+        * CPUTargetList -- bit[23:16] in GIC_DIST_SOFTINT in ARM GIC.
+        *                  bit[23:8] in GIC_DIST_SOFTINT in HiP04 GIC.
+        * NSATT -- bit[15] in GIC_DIST_SOFTINT in ARM GIC.
+        *          bit[7] in GIC_DIST_SOFTINT in HiP04 GIC.
+        * this always happens on GIC0
+        */
+       if (gic_is_standard(&gic_data[0]))
+               map = map << 16;
+       else
+               map = map << 8;
+       writel_relaxed(map | irq,
+                      gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
        raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }
 #endif

@@ -681,10 +750,15 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
  */
 void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
 {
-       BUG_ON(cpu_id >= NR_GIC_CPU_IF);
+       BUG_ON(cpu_id >= gic_data[0].nr_cpu_if);
        cpu_id = 1 << cpu_id;
        /* this always happens on GIC0 */
-       writel_relaxed((cpu_id << 16) | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
+       if (gic_is_standard(&gic_data[0]))
+               cpu_id = cpu_id << 16;
+       else
+               cpu_id = cpu_id << 8;
+       writel_relaxed(cpu_id | irq,
+                      gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 }
 
 /*

@@ -700,7 +774,7 @@ int gic_get_cpu_id(unsigned int cpu)
 {
        unsigned int cpu_bit;
 
-       if (cpu >= NR_GIC_CPU_IF)
+       if (cpu >= gic_data[0].nr_cpu_if)
                return -1;
        cpu_bit = gic_cpu_map[cpu];
        if (cpu_bit & (cpu_bit - 1))

@@ -747,13 +821,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
         * CPU interface and migrate them to the new CPU interface.
         * We skip DIST_TARGET 0 to 7 as they are read-only.
         */
-       for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) {
-               val = readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+       for (i = 8; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+               target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
+               val = readl_relaxed(dist_base + target_reg);
                active_mask = val & cur_target_mask;
                if (active_mask) {
                        val &= ~active_mask;
                        val |= ror32(active_mask, ror_val);
-                       writel_relaxed(val, dist_base + GIC_DIST_TARGET + i*4);
+                       writel_relaxed(val, dist_base + target_reg);
                }
        }
 

@@ -931,7 +1006,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
        irq_hw_number_t hwirq_base;
        struct gic_chip_data *gic;
        int gic_irqs, irq_base, i;
-       int nr_routable_irqs;
+       int nr_routable_irqs, max_nr_irq;
 
        BUG_ON(gic_nr >= MAX_GIC_NR);
 

@@ -967,12 +1042,22 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
                gic_set_base_accessor(gic, gic_get_common_base);
        }
 
+       if (of_device_is_compatible(node, "hisilicon,hip04-gic")) {
+               /* HiP04 GIC supports 16 CPUs at most */
+               gic->nr_cpu_if = 16;
+               max_nr_irq = 510;
+       } else {
+               /* ARM/Qualcomm GIC supports 8 CPUs at most */
+               gic->nr_cpu_if = 8;
+               max_nr_irq = 1020;
+       }
+
        /*
         * Initialize the CPU interface map to all CPUs.
         * It will be refined as each CPU probes its ID.
         */
-       for (i = 0; i < NR_GIC_CPU_IF; i++)
-               gic_cpu_map[i] = 0xff;
+       for (i = 0; i < gic->nr_cpu_if; i++)
+               gic_cpu_map[i] = (1 << gic->nr_cpu_if) - 1;
 
        /*
         * For primary GICs, skip over SGIs.

@@ -988,12 +1073,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
        /*
         * Find out how many interrupts are supported.
-        * The GIC only supports up to 1020 interrupt sources.
+        * The ARM/Qualcomm GIC only supports up to 1020 interrupt sources.
+        * The HiP04 GIC only supports up to 510 interrupt sources.
         */
        gic_irqs = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_CTR) & 0x1f;
        gic_irqs = (gic_irqs + 1) * 32;
-       if (gic_irqs > 1020)
-               gic_irqs = 1020;
+       if (gic_irqs > max_nr_irq)
+               gic_irqs = max_nr_irq;
        gic->gic_irqs = gic_irqs;
 
        gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */

@@ -1069,6 +1155,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 }
 IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
+IRQCHIP_DECLARE(hip04_gic, "hisilicon,hip04-gic", gic_of_init);
 IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
 IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
 
--

1.9.1

There's a little difference between ARM GIC and HiP04 GIC.

* HiP04 GIC could support 16 cores at most, and ARM GIC could support 8 cores at most. So the difination on GIC_DIST_TARGET registers are different since CPU interfaces are increased from 8-bit to 16-bit.

* HiP04 GIC could support 510 interrupts at most, and ARM GIC could support 1020 interrupts at most.

Signed-off-by Haojian Zhuang <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt |   1 +
 drivers/irqchip/irq-gic.c                     | 159 ++++++++++++++++++++------
 2 files changed, 124 insertions(+), 36 deletions(-)

drivers/irqchip/irq-gic.c +47 -4

index f711fb6..64af475 100644
@@ -68,6 +68,7 @@ struct gic_chip_data {
 #ifdef CONFIG_GIC_NON_BANKED
 	void __iomem *(*get_base)(union gic_base *);
 #endif
+	u32 nr_cpu_if;
 };
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);

@@ -76,9 +77,11 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
  * by the GIC itself.
+ *
+ * Hisilicon HiP04 extends the number of CPU interface from 8 to 16.
  */
-#define NR_GIC_CPU_IF 8
-static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
+#define NR_GIC_CPU_IF	16
+static u16 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
 
 /*
  * Supported arch specific GIC irq extension.

@@ -241,20 +244,62 @@ static int gic_retrigger(struct irq_data *d)
 	return 0;
 }
 
+static bool gic_is_standard(struct gic_chip_data *gic_data)
+{
+	return (gic_data->nr_cpu_if == 8);
+}
+
+static u32 irqs_per_target_reg(struct gic_chip_data *gic_data)
+{
+	return (32 / gic_data->nr_cpu_if);
+}
+
+/* i is the index of interrupt */
+static u32 irq_to_target_reg(struct gic_chip_data *gic_data, u32 i)
+{
+	if (gic_is_standard(gic_data))
+		i = i & ~3U;
+	else
+		i = (i << 1) & ~3U;
+	return (i + GIC_DIST_TARGET);
+}
+
 #ifdef CONFIG_SMP
+static u32 irq_to_core_shift(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	unsigned int i = gic_irq(d);
+
+	if (gic_is_standard(gic_data))
+		return ((i % 4) << 3);
+	return ((i % 2) << 4);
+}
+
+static u32 irq_to_core_mask(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	u32 mask;
+	/* ARM GIC, nr_cpu_if == 8; HiP04 GIC, nr_cpu_if == 16 */
+	mask = (1 << gic_data->nr_cpu_if) - 1;
+	return (mask << irq_to_core_shift(d));
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
-	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-	unsigned int shift = (gic_irq(d) % 4) * 8;
+	void __iomem *reg;
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	unsigned int shift = irq_to_core_shift(d);
 	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
 	u32 val, mask, bit;

drivers/irqchip/irq-gic.c +31 -18

 	bit = gic_cpu_map[cpu] << shift;
 	val = readl_relaxed(reg) & ~mask;
 	writel_relaxed(val | bit, reg);

@@ -354,15 +399,20 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
-static u8 gic_get_cpumask(struct gic_chip_data *gic)
+static u16 gic_get_cpumask(struct gic_chip_data *gic)
 {
 	void __iomem *base = gic_data_dist_base(gic);
 	u32 mask, i;
 
-	for (i = mask = 0; i < 32; i += 4) {
-		mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+	/*
+	 * ARM GIC uses 8 registers for interrupt 0-31,
+	 * HiP04 GIC uses 16 registers for interrupt 0-31.
+	 */
+	for (i = mask = 0; i < 32; i++) {
+		mask = readl_relaxed(base + irq_to_target_reg(gic, i));
 		mask |= mask >> 16;
-		mask |= mask >> 8;
+		if (gic_is_standard(gic))
+			mask |= mask >> 8;
 		if (mask)
 			break;
 	}

@@ -370,6 +420,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 	if (!mask)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
+	/* ARM GIC needs 8-bit cpu mask, HiP04 GIC needs 16-bit cpu mask. */
+	if (gic_is_standard(gic))
+		mask &= 0xff;
+
 	return mask;
 }
 

@@ -392,10 +446,11 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	 * Set all global interrupts to this CPU only.
 	 */
 	cpumask = gic_get_cpumask(gic);
-	cpumask |= cpumask << 8;
+	if (gic_is_standard(gic))
+		cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;
-	for (i = 32; i < gic_irqs; i += 4)
-		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
+	for (i = 32; i < gic_irqs; i++)
+		writel_relaxed(cpumask, base + irq_to_target_reg(gic, i));
 
 	/*
 	 * Set priority on all global interrupts.

@@ -423,7 +478,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 	/*
 	 * Get what the GIC says our CPU mask is.
 	 */
-	BUG_ON(cpu >= NR_GIC_CPU_IF);
+	BUG_ON(cpu >= gic->nr_cpu_if);
 	cpu_mask = gic_get_cpumask(gic);
 	gic_cpu_map[cpu] = cpu_mask;
 

@@ -431,7 +486,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 	 * Clear our mask from the other map entries in case they're
 	 * still undefined.
 	 */
-	for (i = 0; i < NR_GIC_CPU_IF; i++)
+	for (i = 0; i < gic->nr_cpu_if; i++)
 		if (i != cpu)
 			gic_cpu_map[i] &= ~cpu_mask;
 

@@ -467,7 +522,7 @@ void gic_cpu_if_down(void)
  */
 static void gic_dist_save(unsigned int gic_nr)
 {
-	unsigned int gic_irqs;
+	unsigned int gic_irqs, target_reg = 0;
 	void __iomem *dist_base;
 	int i;
 

@@ -484,9 +539,11 @@ static void gic_dist_save(unsigned int gic_nr)
 		gic_data[gic_nr].saved_spi_conf[i] =
 			readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
 
-	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
+	for (i = 0; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
 		gic_data[gic_nr].saved_spi_target[i] =
-			readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+			readl_relaxed(dist_base + target_reg);
+	}
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
 		gic_data[gic_nr].saved_spi_enable[i] =

@@ -502,7 +559,7 @@ static void gic_dist_save(unsigned int gic_nr)
  */
 static void gic_dist_restore(unsigned int gic_nr)
 {
-	unsigned int gic_irqs;
+	unsigned int gic_irqs, target_reg = 0;
 	unsigned int i;
 	void __iomem *dist_base;
 

@@ -525,9 +582,11 @@ static void gic_dist_restore(unsigned int gic_nr)
 		writel_relaxed(0xa0a0a0a0,
 			dist_base + GIC_DIST_PRI + i * 4);
 
-	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
+	for (i = 0; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
 		writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
-			dist_base + GIC_DIST_TARGET + i * 4);
+			dist_base + target_reg);
+	}
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
 		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],

@@ -665,9 +724,19 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	 */
 	dmb(ishst);
 
-	/* this always happens on GIC0 */
-	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
-
+	/*
+	 * CPUTargetList -- bit[23:16] in GIC_DIST_SOFTINT in ARM GIC.

drivers/irqchip/irq-gic.c +2 -0

+	writel_relaxed(map | irq,
+		       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }

#endif

drivers/irqchip/irq-gic.c +0 -0

@@ -681,10 +750,15 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
  */
 void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
 {

drivers/irqchip/irq-gic.c +2 -0

+	writel_relaxed(cpu_id | irq,
+		       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 }
 

/*

drivers/irqchip/irq-gic.c +22 -10

@@ -700,7 +774,7 @@ int gic_get_cpu_id(unsigned int cpu)
 {
 	unsigned int cpu_bit;
 
-	if (cpu >= NR_GIC_CPU_IF)
+	if (cpu >= gic_data[0].nr_cpu_if)
 		return -1;
 	cpu_bit = gic_cpu_map[cpu];
 	if (cpu_bit & (cpu_bit - 1))

@@ -747,13 +821,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
 	 * CPU interface and migrate them to the new CPU interface.
 	 * We skip DIST_TARGET 0 to 7 as they are read-only.
 	 */
-	for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) {
-		val = readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+	for (i = 8; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
+		val = readl_relaxed(dist_base + target_reg);
 		active_mask = val & cur_target_mask;
 		if (active_mask) {
 			val &= ~active_mask;
 			val |= ror32(active_mask, ror_val);
-			writel_relaxed(val, dist_base + GIC_DIST_TARGET + i*4);
+			writel_relaxed(val, dist_base + target_reg);
 		}
 	}
 

@@ -931,7 +1006,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
 	int gic_irqs, irq_base, i;
-	int nr_routable_irqs;
+	int nr_routable_irqs, max_nr_irq;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 

@@ -967,12 +1042,22 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_set_base_accessor(gic, gic_get_common_base);
 	}
 
+	if (of_device_is_compatible(node, "hisilicon,hip04-gic")) {
+		/* HiP04 GIC supports 16 CPUs at most */
+		gic->nr_cpu_if = 16;
+		max_nr_irq = 510;
+	} else {
+		/* ARM/Qualcomm GIC supports 8 CPUs at most */
+		gic->nr_cpu_if = 8;
+		max_nr_irq = 1020;
+	}
+
 	/*
 	 * Initialize the CPU interface map to all CPUs.
 	 * It will be refined as each CPU probes its ID.
 	 */
-	for (i = 0; i < NR_GIC_CPU_IF; i++)
-		gic_cpu_map[i] = 0xff;
+	for (i = 0; i < gic->nr_cpu_if; i++)
+		gic_cpu_map[i] = (1 << gic->nr_cpu_if) - 1;
 
 	/*
 	 * For primary GICs, skip over SGIs.

@@ -988,12 +1073,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	/*
 	 * Find out how many interrupts are supported.
-	 * The GIC only supports up to 1020 interrupt sources.
+	 * The ARM/Qualcomm GIC only supports up to 1020 interrupt sources.
+	 * The HiP04 GIC only supports up to 510 interrupt sources.
 	 */
 	gic_irqs = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_CTR) & 0x1f;
 	gic_irqs = (gic_irqs + 1) * 32;
-	if (gic_irqs > 1020)
-		gic_irqs = 1020;
+	if (gic_irqs > max_nr_irq)
+		gic_irqs = max_nr_irq;
 	gic->gic_irqs = gic_irqs;
 
 	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */

Hi Haojian,

I'm working through my backlog ...

Quoted 15 lines · Haojian Zhuang

There's a little difference between ARM GIC and HiP04 GIC.

* HiP04 GIC could support 16 cores at most, and ARM GIC could support 8 cores at most. So the difination on GIC_DIST_TARGET registers are different since CPU interfaces are increased from 8-bit to 16-bit.

* HiP04 GIC could support 510 interrupts at most, and ARM GIC could support 1020 interrupts at most.

Signed-off-by Haojian Zhuang <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt |   1 +
 drivers/irqchip/irq-gic.c                     | 159 ++++++++++++++++++++------
 2 files changed, 124 insertions(+), 36 deletions(-)

...

Quoted 80 lines

drivers/irqchip/irq-gic.c +47 -4

index f711fb6..64af475 100644
@@ -68,6 +68,7 @@ struct gic_chip_data {
 #ifdef CONFIG_GIC_NON_BANKED
 	void __iomem *(*get_base)(union gic_base *);
 #endif
+	u32 nr_cpu_if;
 };
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);

@@ -76,9 +77,11 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
  * The GIC mapping of CPU interfaces does not necessarily match
  * the logical CPU numbering.  Let's use a mapping as returned
  * by the GIC itself.
+ *
+ * Hisilicon HiP04 extends the number of CPU interface from 8 to 16.
  */
-#define NR_GIC_CPU_IF 8
-static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
+#define NR_GIC_CPU_IF	16
+static u16 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
 
 /*
  * Supported arch specific GIC irq extension.

@@ -241,20 +244,62 @@ static int gic_retrigger(struct irq_data *d)
 	return 0;
 }
 
+static bool gic_is_standard(struct gic_chip_data *gic_data)
+{
+	return (gic_data->nr_cpu_if == 8);
+}
+
+static u32 irqs_per_target_reg(struct gic_chip_data *gic_data)
+{
+	return (32 / gic_data->nr_cpu_if);
+}
+
+/* i is the index of interrupt */
+static u32 irq_to_target_reg(struct gic_chip_data *gic_data, u32 i)
+{
+	if (gic_is_standard(gic_data))
+		i = i & ~3U;
+	else
+		i = (i << 1) & ~3U;
+	return (i + GIC_DIST_TARGET);
+}
+
 #ifdef CONFIG_SMP
+static u32 irq_to_core_shift(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	unsigned int i = gic_irq(d);
+
+	if (gic_is_standard(gic_data))
+		return ((i % 4) << 3);
+	return ((i % 2) << 4);
+}
+
+static u32 irq_to_core_mask(struct irq_data *d)
+{
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	u32 mask;
+	/* ARM GIC, nr_cpu_if == 8; HiP04 GIC, nr_cpu_if == 16 */
+	mask = (1 << gic_data->nr_cpu_if) - 1;
+	return (mask << irq_to_core_shift(d));
+}
+
 static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
-	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-	unsigned int shift = (gic_irq(d) % 4) * 8;
+	void __iomem *reg;
+	struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+	unsigned int shift = irq_to_core_shift(d);
 	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
 	u32 val, mask, bit;

drivers/irqchip/irq-gic.c static int gic_retrigger(struct irq_data *d) · line 302

Quoted 10 lines

drivers/irqchip/irq-gic.c +4 -2

 
-	if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
+	if (cpu >= gic_data->nr_cpu_if || cpu >= nr_cpu_ids)
 		return -EINVAL;
 
+	reg = gic_dist_base(d) + irq_to_target_reg(gic_data, gic_irq(d));
+
 	raw_spin_lock(&irq_controller_lock);
-	mask = 0xff << shift;
+	mask = irq_to_core_mask(d);

You still haven't addressed my comment from your previous version of this series.

Please calculate the mask once at boottime and store it. Then you can use it in conjunction with the shift you calculate above. This will remove the needless calculation inside the spinlock:

  mask = (1 << gic_data->nr_cpu_if) - 1;

It will also remove the extra call to irq_data_get_irq_chip_data() since you already did that in this function outside the spinlock.

Quoted 127 lines

drivers/irqchip/irq-gic.c +31 -18

 	bit = gic_cpu_map[cpu] << shift;
 	val = readl_relaxed(reg) & ~mask;
 	writel_relaxed(val | bit, reg);

@@ -354,15 +399,20 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 	irq_set_chained_handler(irq, gic_handle_cascade_irq);
 }
 
-static u8 gic_get_cpumask(struct gic_chip_data *gic)
+static u16 gic_get_cpumask(struct gic_chip_data *gic)
 {
 	void __iomem *base = gic_data_dist_base(gic);
 	u32 mask, i;
 
-	for (i = mask = 0; i < 32; i += 4) {
-		mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+	/*
+	 * ARM GIC uses 8 registers for interrupt 0-31,
+	 * HiP04 GIC uses 16 registers for interrupt 0-31.
+	 */
+	for (i = mask = 0; i < 32; i++) {
+		mask = readl_relaxed(base + irq_to_target_reg(gic, i));
 		mask |= mask >> 16;
-		mask |= mask >> 8;
+		if (gic_is_standard(gic))
+			mask |= mask >> 8;
 		if (mask)
 			break;
 	}

@@ -370,6 +420,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 	if (!mask)
 		pr_crit("GIC CPU mask not found - kernel will fail to boot.\n");
 
+	/* ARM GIC needs 8-bit cpu mask, HiP04 GIC needs 16-bit cpu mask. */
+	if (gic_is_standard(gic))
+		mask &= 0xff;
+
 	return mask;
 }
 

@@ -392,10 +446,11 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	 * Set all global interrupts to this CPU only.
 	 */
 	cpumask = gic_get_cpumask(gic);
-	cpumask |= cpumask << 8;
+	if (gic_is_standard(gic))
+		cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;
-	for (i = 32; i < gic_irqs; i += 4)
-		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
+	for (i = 32; i < gic_irqs; i++)
+		writel_relaxed(cpumask, base + irq_to_target_reg(gic, i));
 
 	/*
 	 * Set priority on all global interrupts.

@@ -423,7 +478,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 	/*
 	 * Get what the GIC says our CPU mask is.
 	 */
-	BUG_ON(cpu >= NR_GIC_CPU_IF);
+	BUG_ON(cpu >= gic->nr_cpu_if);
 	cpu_mask = gic_get_cpumask(gic);
 	gic_cpu_map[cpu] = cpu_mask;
 

@@ -431,7 +486,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 	 * Clear our mask from the other map entries in case they're
 	 * still undefined.
 	 */
-	for (i = 0; i < NR_GIC_CPU_IF; i++)
+	for (i = 0; i < gic->nr_cpu_if; i++)
 		if (i != cpu)
 			gic_cpu_map[i] &= ~cpu_mask;
 

@@ -467,7 +522,7 @@ void gic_cpu_if_down(void)
  */
 static void gic_dist_save(unsigned int gic_nr)
 {
-	unsigned int gic_irqs;
+	unsigned int gic_irqs, target_reg = 0;
 	void __iomem *dist_base;
 	int i;
 

@@ -484,9 +539,11 @@ static void gic_dist_save(unsigned int gic_nr)
 		gic_data[gic_nr].saved_spi_conf[i] =
 			readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
 
-	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
+	for (i = 0; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
 		gic_data[gic_nr].saved_spi_target[i] =
-			readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+			readl_relaxed(dist_base + target_reg);
+	}
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
 		gic_data[gic_nr].saved_spi_enable[i] =

@@ -502,7 +559,7 @@ static void gic_dist_save(unsigned int gic_nr)
  */
 static void gic_dist_restore(unsigned int gic_nr)
 {
-	unsigned int gic_irqs;
+	unsigned int gic_irqs, target_reg = 0;
 	unsigned int i;
 	void __iomem *dist_base;
 

@@ -525,9 +582,11 @@ static void gic_dist_restore(unsigned int gic_nr)
 		writel_relaxed(0xa0a0a0a0,
 			dist_base + GIC_DIST_PRI + i * 4);
 
-	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
+	for (i = 0; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
 		writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
-			dist_base + GIC_DIST_TARGET + i * 4);
+			dist_base + target_reg);
+	}
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
 		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],

@@ -665,9 +724,19 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	 */
 	dmb(ishst);
 
-	/* this always happens on GIC0 */
-	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
-
+	/*
+	 * CPUTargetList -- bit[23:16] in GIC_DIST_SOFTINT in ARM GIC.

drivers/irqchip/irq-gic.c static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) · line 738

Quoted 10 lines

drivers/irqchip/irq-gic.c +9 -0

+	 *                  bit[23:8] in GIC_DIST_SOFTINT in HiP04 GIC.
+	 * NSATT -- bit[15] in GIC_DIST_SOFTINT in ARM GIC.
+	 *          bit[7] in GIC_DIST_SOFTINT in HiP04 GIC.
+	 * this always happens on GIC0
+	 */
 
+	if (gic_is_standard(&gic_data[0]))
+		map = map << 16;
+	else
+		map = map << 8;

Here's another place where you should store the shift and avoid the if() logic inside the lock.

Quoted 9 lines

drivers/irqchip/irq-gic.c +2 -0

+	writel_relaxed(map | irq,
+		       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 	raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
 }

#endif

drivers/irqchip/irq-gic.c +0 -0

@@ -681,10 +750,15 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
  */
 void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
 {

drivers/irqchip/irq-gic.c static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) · line 760

Quoted 10 lines

drivers/irqchip/irq-gic.c +5 -2

-	BUG_ON(cpu_id >= NR_GIC_CPU_IF);
+	BUG_ON(cpu_id >= gic_data[0].nr_cpu_if);
 	cpu_id = 1 << cpu_id;
 	/* this always happens on GIC0 */
-	writel_relaxed((cpu_id << 16) | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 
+	if (gic_is_standard(&gic_data[0]))
+		cpu_id = cpu_id << 16;
+	else
+		cpu_id = cpu_id << 8;

You can use the stored shift here as well.

Quoted 84 lines

drivers/irqchip/irq-gic.c +2 -0

+	writel_relaxed(cpu_id | irq,
+		       gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
 }
 

/*

drivers/irqchip/irq-gic.c +22 -10

@@ -700,7 +774,7 @@ int gic_get_cpu_id(unsigned int cpu)
 {
 	unsigned int cpu_bit;
 
-	if (cpu >= NR_GIC_CPU_IF)
+	if (cpu >= gic_data[0].nr_cpu_if)
 		return -1;
 	cpu_bit = gic_cpu_map[cpu];
 	if (cpu_bit & (cpu_bit - 1))

@@ -747,13 +821,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
 	 * CPU interface and migrate them to the new CPU interface.
 	 * We skip DIST_TARGET 0 to 7 as they are read-only.
 	 */
-	for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) {
-		val = readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
+	for (i = 8; i < gic_irqs; i += irqs_per_target_reg(&gic_data[gic_nr])) {
+		target_reg = irq_to_target_reg(&gic_data[gic_nr], i);
+		val = readl_relaxed(dist_base + target_reg);
 		active_mask = val & cur_target_mask;
 		if (active_mask) {
 			val &= ~active_mask;
 			val |= ror32(active_mask, ror_val);
-			writel_relaxed(val, dist_base + GIC_DIST_TARGET + i*4);
+			writel_relaxed(val, dist_base + target_reg);
 		}
 	}
 

@@ -931,7 +1006,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
 	int gic_irqs, irq_base, i;
-	int nr_routable_irqs;
+	int nr_routable_irqs, max_nr_irq;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 

@@ -967,12 +1042,22 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic_set_base_accessor(gic, gic_get_common_base);
 	}
 
+	if (of_device_is_compatible(node, "hisilicon,hip04-gic")) {
+		/* HiP04 GIC supports 16 CPUs at most */
+		gic->nr_cpu_if = 16;
+		max_nr_irq = 510;
+	} else {
+		/* ARM/Qualcomm GIC supports 8 CPUs at most */
+		gic->nr_cpu_if = 8;
+		max_nr_irq = 1020;
+	}
+
 	/*
 	 * Initialize the CPU interface map to all CPUs.
 	 * It will be refined as each CPU probes its ID.
 	 */
-	for (i = 0; i < NR_GIC_CPU_IF; i++)
-		gic_cpu_map[i] = 0xff;
+	for (i = 0; i < gic->nr_cpu_if; i++)
+		gic_cpu_map[i] = (1 << gic->nr_cpu_if) - 1;
 
 	/*
 	 * For primary GICs, skip over SGIs.

@@ -988,12 +1073,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	/*
 	 * Find out how many interrupts are supported.
-	 * The GIC only supports up to 1020 interrupt sources.
+	 * The ARM/Qualcomm GIC only supports up to 1020 interrupt sources.
+	 * The HiP04 GIC only supports up to 510 interrupt sources.
 	 */
 	gic_irqs = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_CTR) & 0x1f;
 	gic_irqs = (gic_irqs + 1) * 32;
-	if (gic_irqs > 1020)
-		gic_irqs = 1020;
+	if (gic_irqs > max_nr_irq)
+		gic_irqs = max_nr_irq;
 	gic->gic_irqs = gic_irqs;
 
 	gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */

drivers/irqchip/irq-gic.c gic_of_init(struct device_node *node, struct device_node *parent) · line 1160

Quoted 6 lines

drivers/irqchip/irq-gic.c +1 -0

@@ -1069,6 +1155,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 }
 IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
+IRQCHIP_DECLARE(hip04_gic, "hisilicon,hip04-gic", gic_of_init);
 IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
 IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);

thx,

Jason.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Haojian,

I've set up a branch for all the changes to the gic driver this time around. Please keep an eye on:

git://git.infradead.org/users/jcooper/linux.git irqchip/gic

And base your irqchip changes on top.

thx,

Jason.

Multiple CPU clusters are used in Hisilicon HiP04 SoC. Now use MCPM framework to manage power on HiP04 SoC.

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+	unsigned long data, mask;
+
+	if (!relocation || !sysctrl)
+		return -ENODEV;
+	if (cluster >= HIP04_MAX_CLUSTERS || cpu >= HIP04_MAX_CPUS_PER_CLUSTER)
+		return -EINVAL;
+
+	spin_lock_irq(&boot_lock);
+
+	if (hip04_cpu_table[cluster][cpu]) {
+		hip04_cpu_table[cluster][cpu]++;
+		spin_unlock_irq(&boot_lock);
+		return 0;
+	}
+
+	writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+	writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+	writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+	writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c new file +26 -0

+	return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+	unsigned int mpidr, cpu, cluster, data = 0;
+	bool skip_reset = false;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	__mcpm_cpu_going_down(cpu, cluster);
+
+	spin_lock(&boot_lock);
+	BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+	hip04_cpu_table[cluster][cpu]--;
+	if (hip04_cpu_table[cluster][cpu] == 1) {
+		/* A power_up request went ahead of us. */
+		skip_reset = true;
+	} else if (hip04_cpu_table[cluster][cpu] > 1) {
+		pr_err("Cluster %d CPU%d boots multiple times\n", cluster, cpu);
+		BUG();
+	}
+	spin_unlock(&boot_lock);
+

Quoted 2 lines · Haojian Zhuang

Multiple CPU clusters are used in Hisilicon HiP04 SoC. Now use MCPM framework to manage power on HiP04 SoC.

There are still unresolved issues with this patch.

[...]

Quoted 22 lines

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >= HIP04_MAX_CPUS_PER_CLUSTER)
+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+
+     if (hip04_cpu_table[cluster][cpu]) {
+             hip04_cpu_table[cluster][cpu]++;
+             spin_unlock_irq(&boot_lock);
+             return 0;
+     }
+
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Sorry to insist, but I want to repeat the question I asked during the previous review as I consider this is important, especially if you want to support deep C-States with cpuidle later. This also has implications if you ever want to turn off snoops in hip04_mcpm_power_down() when all CPUs in a cluster are down.

You said:

| But it fails on my platform if I execute snooping setup on the new | CPU.

I then asked:

| It fails how?  I want to make sure if the problem is with the hardware
| design or the code.
|
| The assembly code could be wrong.  Are you sure this is not theactual
| reason?
|
| Is there some documentation for this stuff?

I also see that the snoop filter is enabled from hip04_cpu_table_init() for the CPU that is actually executing that code. So that must work somehow...

arch/arm/mach-hisi/platmcpm.c line 137

Quoted 8 lines

arch/arm/mach-hisi/platmcpm.c new file +8 -0

+
+     hip04_cpu_table[cluster][cpu]++;
+
+     data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+            CORE_DEBUG_RESET_BIT(cpu);
+     writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+     spin_unlock_irq(&boot_lock);
+     msleep(POLL_MSEC);

Your cover letter for this series mentionned this:

| v9:
| * Remove delay workaround in mcpm implementation.

Why is it still there?

Quoted 26 lines

arch/arm/mach-hisi/platmcpm.c new file +26 -0

+     return 0;
+}
+
+static void hip04_mcpm_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster, data = 0;
+     bool skip_reset = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     spin_lock(&boot_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     hip04_cpu_table[cluster][cpu]--;
+     if (hip04_cpu_table[cluster][cpu] == 1) {
+             /* A power_up request went ahead of us. */
+             skip_reset = true;
+     } else if (hip04_cpu_table[cluster][cpu] > 1) {
+             pr_err("Cluster %d CPU%d boots multiple times\n", cluster, cpu);
+             BUG();
+     }
+     spin_unlock(&boot_lock);
+

arch/arm/mach-hisi/platmcpm.c line 174

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     v7_exit_coherency_flush(louis);
+
+     __mcpm_cpu_down(cpu, cluster);
+
+     if (!skip_reset) {
+             data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+                    CORE_DEBUG_RESET_BIT(cpu);
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_REQ(cluster));
+     }
+}

As I mentioned already this is going to race with the power_up() method. Let me illustrate the problem:

* CPU 0 * CPU 1
------- -------
* mcpm_cpu_power_down()
* hip04_mcpm_power_down()
* spin_lock(&boot_lock); [lock acquired]
* mcpm_cpu_power_up(cpu = 0)
* spin_lock(&boot_lock); [blocked]
* hip04_cpu_table[cluster][cpu]--; [value down to 0]
* skip_reset = false
* spin_unlock(&boot_lock);
* spin_lock(&boot_lock); [now succeeds]
* v7_exit_coherency_flush(louis); [takes a while to complete]
* hip04_cpu_table[cluster][cpu]++; [value back to 1]
* bring CPU0 out of reset
* put CPU0 into reset
* spin_unlock(&boot_lock);

Here you end up with CPU0 in reset while hip04_cpu_table[cluster][cpu] for CPU0 is equal to 1. The CPU will therefore never start again as further calls to power_up() won't see hip04_cpu_table equal to 0 anymore.

So... I'm asking again: are you absolutely certain that the CPU reset is applied at the very moment the corresponding bit is set? Isn't it applied only when the CPU does execute a WFI like most other platforms?

Quoted 62 lines · Nicolas Pitre

Quoted 2 lines · Haojian Zhuang

Multiple CPU clusters are used in Hisilicon HiP04 SoC. Now use MCPM framework to manage power on HiP04 SoC.

There are still unresolved issues with this patch.

[...]

Quoted 22 lines

arch/arm/mach-hisi/platmcpm.c new file +22 -0

+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+     unsigned long data, mask;
+
+     if (!relocation || !sysctrl)
+             return -ENODEV;
+     if (cluster >= HIP04_MAX_CLUSTERS || cpu >= HIP04_MAX_CPUS_PER_CLUSTER)
+             return -EINVAL;
+
+     spin_lock_irq(&boot_lock);
+
+     if (hip04_cpu_table[cluster][cpu]) {
+             hip04_cpu_table[cluster][cpu]++;
+             spin_unlock_irq(&boot_lock);
+             return 0;
+     }
+
+     writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
+     writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
+     writel_relaxed(virt_to_phys(mcpm_entry_point), relocation + 8);
+     writel_relaxed(0, relocation + 12);
+

arch/arm/mach-hisi/platmcpm.c line 129

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     if (hip04_cluster_down(cluster)) {
+             data = CLUSTER_DEBUG_RESET_BIT;
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
+             do {
+                     mask = CLUSTER_DEBUG_RESET_STATUS;
+                     data = readl_relaxed(sysctrl + \
+                                          SC_CPU_RESET_STATUS(cluster));
+             } while (data & mask);
+             hip04_set_snoop_filter(cluster, 1);
+     }

Sorry to insist, but I want to repeat the question I asked during the previous review as I consider this is important, especially if you want to support deep C-States with cpuidle later. This also has implications if you ever want to turn off snoops in hip04_mcpm_power_down() when all CPUs in a cluster are down.

You said:

| But it fails on my platform if I execute snooping setup on the new | CPU.

I then asked:

| It fails how?  I want to make sure if the problem is with the hardware
| design or the code.
|
| The assembly code could be wrong.  Are you sure this is not theactual
| reason?
|
| Is there some documentation for this stuff?

Quoted 4 lines

I also see that the snoop filter is enabled from hip04_cpu_table_init() for the CPU that is actually executing that code. So that must work somehow...

Cluster0 is very special. If I didn't enable snoop filter of cluster0, it also works. I'll check with Hisilicon guys why it's different. The configuration of snoop filter is a black box to me.

Quoted 59 lines

Quoted 23 lines

arch/arm/mach-hisi/platmcpm.c new file +23 -0

+static void hip04_mcpm_power_down(void)
+{
+     unsigned int mpidr, cpu, cluster, data = 0;
+     bool skip_reset = false;
+
+     mpidr = read_cpuid_mpidr();
+     cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+     cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+     __mcpm_cpu_going_down(cpu, cluster);
+
+     spin_lock(&boot_lock);
+     BUG_ON(__mcpm_cluster_state(cluster) != CLUSTER_UP);
+     hip04_cpu_table[cluster][cpu]--;
+     if (hip04_cpu_table[cluster][cpu] == 1) {
+             /* A power_up request went ahead of us. */
+             skip_reset = true;
+     } else if (hip04_cpu_table[cluster][cpu] > 1) {
+             pr_err("Cluster %d CPU%d boots multiple times\n", cluster, cpu);
+             BUG();
+     }
+     spin_unlock(&boot_lock);
+

arch/arm/mach-hisi/platmcpm.c line 174

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -0

+     v7_exit_coherency_flush(louis);
+
+     __mcpm_cpu_down(cpu, cluster);
+
+     if (!skip_reset) {
+             data = CORE_RESET_BIT(cpu) | NEON_RESET_BIT(cpu) | \
+                    CORE_DEBUG_RESET_BIT(cpu);
+             writel_relaxed(data, sysctrl + SC_CPU_RESET_REQ(cluster));
+     }
+}

As I mentioned already this is going to race with the power_up() method. Let me illustrate the problem:

* CPU 0 * CPU 1
------- -------
* mcpm_cpu_power_down()
* hip04_mcpm_power_down()
* spin_lock(&boot_lock); [lock acquired]
* mcpm_cpu_power_up(cpu = 0)
* spin_lock(&boot_lock); [blocked]
* hip04_cpu_table[cluster][cpu]--; [value down to 0]
* skip_reset = false
* spin_unlock(&boot_lock);
* spin_lock(&boot_lock); [now succeeds]
* v7_exit_coherency_flush(louis); [takes a while to complete]
* hip04_cpu_table[cluster][cpu]++; [value back to 1]
* bring CPU0 out of reset
* put CPU0 into reset
* spin_unlock(&boot_lock);

Here you end up with CPU0 in reset while hip04_cpu_table[cluster][cpu] for CPU0 is equal to 1. The CPU will therefore never start again as further calls to power_up() won't see hip04_cpu_table equal to 0 anymore.

Quoted 4 lines

So... I'm asking again: are you absolutely certain that the CPU reset is applied at the very moment the corresponding bit is set? Isn't it applied only when the CPU does execute a WFI like most other platforms?

If I put CPU0 into reset mode in wait_for_powerdown() that is executed in CPU1 or other CPU, this issue doesn't exist. Is it right?

Append multi_v7_lpae_config. In this default configuration, CONFIG_ARCH_MULTI_V6 is disabled. CONFIG_ARM_LPAE is enabled.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/configs/multi_v7_lpae_defconfig | 351 +++++++++++++++++++++++++++++++
 1 file changed, 351 insertions(+)
 create mode 100644 arch/arm/configs/multi_v7_lpae_defconfig

arch/arm/configs/multi_v7_lpae_defconfig new file +0 -0

new file mode 100644
index 0000000..59fcefc
@@ -0,0 +1,351 @@

Append multi_v7_lpae_config. In this default configuration, CONFIG_ARCH_MULTI_V6 is disabled. CONFIG_ARM_LPAE is enabled.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/configs/multi_v7_lpae_defconfig | 351 +++++++++++++++++++++++++++++++
 1 file changed, 351 insertions(+)
 create mode 100644 arch/arm/configs/multi_v7_lpae_defconfig

arch/arm/configs/multi_v7_lpae_defconfig new file +15 -0

new file mode 100644
index 0000000..59fcefc
@@ -0,0 +1,351 @@
+CONFIG_SYSVIPC=y
+CONFIG_FHANDLE=y
+CONFIG_IRQ_DOMAIN_DEBUG=y
+CONFIG_NO_HZ=y
+CONFIG_HIGH_RES_TIMERS=y
+CONFIG_BLK_DEV_INITRD=y
+CONFIG_EMBEDDED=y
+CONFIG_MODULES=y
+CONFIG_MODULE_UNLOAD=y
+CONFIG_PARTITION_ADVANCED=y
+# CONFIG_ARCH_MULTI_V6 is not set
+CONFIG_ARCH_MULTI_V7=y
+CONFIG_ARM_LPAE=y
+CONFIG_ARCH_MVEBU=y
+CONFIG_MACH_ARMADA_370=y

Append multi_v7_lpae_config. In this default configuration, CONFIG_ARCH_MULTI_V6 is disabled. CONFIG_ARM_LPAE is enabled.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/configs/multi_v7_lpae_defconfig | 351 +++++++++++++++++++++++++++++++
 1 file changed, 351 insertions(+)
 create mode 100644 arch/arm/configs/multi_v7_lpae_defconfig

In ARM standard GIC, GICH_APR offset is 0xf0 & GICH_LR0 offset is 0x100. In HiP04 GIC, GICH_APR offset is 0x70 & GICH_LR0 offset is 0x80.

Now reuse the nr_lr field in struct vgic_cpu. Bit[31:16] is used to store GICH_APR offset in HiP04, and bit[15:0] is used to store real nr_lr variable. In ARM standard GIC, don't set bit[31:16]. So we could avoid to change the VGIC implementation in arm64.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/kernel/asm-offsets.c   |  2 +-
 arch/arm/kvm/interrupts_head.S  | 29 +++++++++++++++++++------
 arch/arm64/kernel/asm-offsets.c |  2 +-
 arch/arm64/kvm/hyp.S            | 28 ++++++++++++++++++------
 include/kvm/arm_vgic.h          |  7 ++++--
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 48 +++++++++++++++++++++++++++++------------
 7 files changed, 92 insertions(+), 30 deletions(-)

arch/arm/kernel/asm-offsets.c +0 -0

index 85598b5..166cc98 100644

arch/arm64/kvm/hyp.S +10 -3

index 2c56012..a4a8b3d 100644
@@ -402,7 +402,9 @@ __kvm_hyp_code_start:
        ldr     w8, [x2, #GICH_EISR1]
        ldr     w9, [x2, #GICH_ELRSR0]
        ldr     w10, [x2, #GICH_ELRSR1]
-       ldr     w11, [x2, #GICH_APR]
+       ldr     w11, [x3, #VGIC_CPU_HW_CFG]
+       mov     w11, w11, lsr #HWCFG_APR_SHIFT
+       ldr     w11, [x2, w10]
 CPU_BE(        rev     w4,  w4  )
 CPU_BE(        rev     w5,  w5  )
 CPU_BE(        rev     w6,  w6  )

@@ -425,8 +427,13 @@ CPU_BE(    rev     w11, w11 )
        str     wzr, [x2, #GICH_HCR]
 
        /* Save list registers */
-       add     x2, x2, #GICH_LR0
-       ldr     w4, [x3, #VGIC_CPU_NR_LR]
+       ldr     w4, [x3, #VGIC_CPU_HW_CFG]
+       mov     w6, w4, lsr #HWCFG_APR_SHIFT
+       ldr     w7, =HWCFG_NR_LR_MASK
+       and     w4, w4, w7
+       /* the offset between GICH_APR and GICH_LR0 is 0x10 */
+       add     w6, w6, 0x10
+       add     x2, x2, w6
        add     x3, x3, #VGIC_CPU_LR
 1:     ldr     w5, [x2], #4
 CPU_BE(        rev     w5, w5 )

In ARM standard GIC, GICH_APR offset is 0xf0 & GICH_LR0 offset is 0x100. In HiP04 GIC, GICH_APR offset is 0x70 & GICH_LR0 offset is 0x80.

Now reuse the nr_lr field in struct vgic_cpu. Bit[31:16] is used to store GICH_APR offset in HiP04, and bit[15:0] is used to store real nr_lr variable. In ARM standard GIC, don't set bit[31:16]. So we could avoid to change the VGIC implementation in arm64.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/kernel/asm-offsets.c   |  2 +-
 arch/arm/kvm/interrupts_head.S  | 29 +++++++++++++++++++------
 arch/arm64/kernel/asm-offsets.c |  2 +-
 arch/arm64/kvm/hyp.S            | 28 ++++++++++++++++++------
 include/kvm/arm_vgic.h          |  7 ++++--
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 48 +++++++++++++++++++++++++++++------------
 7 files changed, 92 insertions(+), 30 deletions(-)

arch/arm/kernel/asm-offsets.c +1 -1

index 85598b5..166cc98 100644
@@ -189,7 +189,7 @@ int main(void)
   DEFINE(VGIC_CPU_ELRSR,	offsetof(struct vgic_cpu, vgic_elrsr));
   DEFINE(VGIC_CPU_APR,		offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,		offsetof(struct vgic_cpu, vgic_lr));
-  DEFINE(VGIC_CPU_NR_LR,	offsetof(struct vgic_cpu, nr_lr));
+  DEFINE(VGIC_CPU_HW_CFG,	offsetof(struct vgic_cpu, hw_cfg));
 #ifdef CONFIG_KVM_ARM_TIMER
   DEFINE(VCPU_TIMER_CNTV_CTL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
   DEFINE(VCPU_TIMER_CNTV_CVAL,	offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));

arch/arm/kvm/interrupts_head.S +23 -6

index 76af9302..9fbbf99 100644
@@ -419,7 +419,9 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	ldr	r7, [r2, #GICH_EISR1]
 	ldr	r8, [r2, #GICH_ELRSR0]
 	ldr	r9, [r2, #GICH_ELRSR1]
-	ldr	r10, [r2, #GICH_APR]
+	ldr	r10, [r11, #VGIC_CPU_HW_CFG]
+	mov	r10, r10, lsr #HWCFG_APR_SHIFT
+	ldr	r10, [r2, r10]
 
 	str	r3, [r11, #VGIC_CPU_HCR]
 	str	r4, [r11, #VGIC_CPU_VMCR]

@@ -435,9 +437,15 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 	str	r5, [r2, #GICH_HCR]
 
 	/* Save list registers */
-	add	r2, r2, #GICH_LR0
+	ldr	r4, [r11, #VGIC_CPU_HW_CFG]
+	mov	r10, r4, lsr #HWCFG_APR_SHIFT
+	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
+	add	r10, r10, #0x10
+	add	r2, r2, r10
 	add	r3, r11, #VGIC_CPU_LR
-	ldr	r4, [r11, #VGIC_CPU_NR_LR]
+	/* Get NR_LR from VGIC_CPU_HW_CFG */
+	ldr	r6, =HWCFG_NR_LR_MASK
+	and	r4, r4, r6
 1:	ldr	r6, [r2], #4
 	str	r6, [r3], #4
 	subs	r4, r4, #1

@@ -469,12 +477,21 @@ vcpu	.req	r0		@ vcpu pointer always in r0
 
 	str	r3, [r2, #GICH_HCR]
 	str	r4, [r2, #GICH_VMCR]
-	str	r8, [r2, #GICH_APR]
+	ldr	r6, [r11, #VGIC_CPU_HW_CFG]
+	mov	r6, r6, lsr #HWCFG_APR_SHIFT
+	str	r8, [r2, r6]
 
 	/* Restore list registers */
-	add	r2, r2, #GICH_LR0
+	ldr	r4, [r11, #VGIC_CPU_HW_CFG]
+	mov	r6, r4, lsr #HWCFG_APR_SHIFT
+	/* the offset between GICH_APR & GICH_LR0 is 0x10 */
+	add	r6, r6, #0x10
+	/* get offset of GICH_LR0 */
+	add	r2, r2, r6
+	/* Get NR_LR from VGIC_CPU_HW_CFG */
 	add	r3, r11, #VGIC_CPU_LR
-	ldr	r4, [r11, #VGIC_CPU_NR_LR]
+	ldr	r6, =HWCFG_NR_LR_MASK
+	and	r4, r4, r6
 1:	ldr	r6, [r3], #4
 	str	r6, [r2], #4
 	subs	r4, r4, #1

arch/arm64/kernel/asm-offsets.c +1 -1

index 646f888..2422358 100644
@@ -136,7 +136,7 @@ int main(void)
   DEFINE(VGIC_CPU_ELRSR,	offsetof(struct vgic_cpu, vgic_elrsr));
   DEFINE(VGIC_CPU_APR,		offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,		offsetof(struct vgic_cpu, vgic_lr));
-  DEFINE(VGIC_CPU_NR_LR,	offsetof(struct vgic_cpu, nr_lr));
+  DEFINE(VGIC_CPU_HW_CFG,	offsetof(struct vgic_cpu, hw_cfg));
   DEFINE(KVM_VTTBR,		offsetof(struct kvm, arch.vttbr));
   DEFINE(KVM_VGIC_VCTRL,	offsetof(struct kvm, arch.vgic.vctrl_base));
 #endif

arch/arm64/kvm/hyp.S +22 -6

index 2c56012..a4a8b3d 100644
@@ -402,7 +402,9 @@ __kvm_hyp_code_start:
 	ldr	w8, [x2, #GICH_EISR1]
 	ldr	w9, [x2, #GICH_ELRSR0]
 	ldr	w10, [x2, #GICH_ELRSR1]
-	ldr	w11, [x2, #GICH_APR]
+	ldr	w11, [x3, #VGIC_CPU_HW_CFG]
+	mov	w11, w11, lsr #HWCFG_APR_SHIFT
+	ldr	w11, [x2, w10]
 CPU_BE(	rev	w4,  w4  )
 CPU_BE(	rev	w5,  w5  )
 CPU_BE(	rev	w6,  w6  )

@@ -425,8 +427,13 @@ CPU_BE(	rev	w11, w11 )
 	str	wzr, [x2, #GICH_HCR]
 
 	/* Save list registers */
-	add	x2, x2, #GICH_LR0
-	ldr	w4, [x3, #VGIC_CPU_NR_LR]
+	ldr	w4, [x3, #VGIC_CPU_HW_CFG]
+	mov	w6, w4, lsr #HWCFG_APR_SHIFT
+	ldr	w7, =HWCFG_NR_LR_MASK
+	and	w4, w4, w7
+	/* the offset between GICH_APR and GICH_LR0 is 0x10 */
+	add	w6, w6, 0x10
+	add	x2, x2, w6
 	add	x3, x3, #VGIC_CPU_LR
 1:	ldr	w5, [x2], #4
 CPU_BE(	rev	w5, w5 )

@@ -461,11 +468,20 @@ CPU_BE(	rev	w6, w6 )
 
 	str	w4, [x2, #GICH_HCR]
 	str	w5, [x2, #GICH_VMCR]
-	str	w6, [x2, #GICH_APR]
+	ldr	w4, [x3, #VGIC_CPU_HW_CFG]
+	mov	w4, w4, #HWCFG_APR_SHIFT
+	str	w6, [x2, w4]
 
 	/* Restore list registers */
-	add	x2, x2, #GICH_LR0
-	ldr	w4, [x3, #VGIC_CPU_NR_LR]
+	ldr	w4, [x3, #VGIC_CPU_HW_CFG]
+	mov	w6, w4, #HWCFG_APR_SHIFT
+	/* the offset between GICH_APR and GICH_LR0 is 0x10 */
+	add	w6, w6, #0x10
+	/* get offset of GICH_LR0 */
+	add	x2, x2, w6
+	/* get NR_LR from VGIC_CPU_HW_CFG */
+	ldr	w6, =HWCFG_NR_LR_MASK
+	and	w4, w4, w6
 	add	x3, x3, #VGIC_CPU_LR
 1:	ldr	w5, [x3], #4
 CPU_BE(	rev	w5, w5 )

include/kvm/arm_vgic.h +5 -2

index f27000f..eba4b51 100644
@@ -122,8 +122,11 @@ struct vgic_cpu {
 	/* Bitmap of used/free list registers */
 	DECLARE_BITMAP(	lr_used, VGIC_MAX_LRS);
 
-	/* Number of list registers on this CPU */
-	int		nr_lr;
+	/*
+	 * bit[31:16]: GICH_APR offset
+	 * bit[15:0]:  Number of list registers on this CPU
+	 */
+	u32		hw_cfg;
 
 	/* CPU vif control registers for world switch */
 	u32		vgic_hcr;

include/linux/irqchip/arm-gic.h +6 -0

index 45e2d8c..b055f92 100644
@@ -49,6 +49,8 @@
 #define GICH_ELRSR1 			0x34
 #define GICH_APR			0xf0
 #define GICH_LR0			0x100
+#define HIP04_GICH_APR			0x70
+/* GICH_LR0 offset in HiP04 is 0x80 */
 
 #define GICH_HCR_EN			(1 << 0)
 #define GICH_HCR_UIE			(1 << 1)

@@ -73,6 +75,10 @@
 #define GICH_MISR_EOI			(1 << 0)
 #define GICH_MISR_U			(1 << 1)
 
+#define HWCFG_NR_LR_MASK	0xffff
+#define HWCFG_APR_SHIFT		16
+#define HWCFG_APR_MASK		(0xffff << HWCFG_APR_SHIFT)
+
 #ifndef __ASSEMBLY__
 
 struct device_node;

virt/kvm/arm/vgic.c +3 -1

index 47b2983..4c0c1e9 100644
@@ -76,6 +76,8 @@
 #define IMPLEMENTER_ARM		0x43b
 #define GICC_ARCH_VERSION_V2	0x2
 
+#define vgic_nr_lr(vcpu)	(vcpu->hw_cfg & HWCFG_NR_LR_MASK)
+
 /* Physical address of vgic virtual cpu interface */
 static phys_addr_t vgic_vcpu_base;
 

@@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
 static void vgic_kick_vcpus(struct kvm *kvm);
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
-static u32 vgic_nr_lr;
+static u32 vgic_hw_cfg;
 
 static unsigned int vgic_maint_irq;
 

Quoted 223 lines · Haojian Zhuang

In ARM standard GIC, GICH_APR offset is 0xf0 & GICH_LR0 offset is 0x100. In HiP04 GIC, GICH_APR offset is 0x70 & GICH_LR0 offset is 0x80.

Now reuse the nr_lr field in struct vgic_cpu. Bit[31:16] is used to store GICH_APR offset in HiP04, and bit[15:0] is used to store real nr_lr variable. In ARM standard GIC, don't set bit[31:16]. So we could avoid to change the VGIC implementation in arm64.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/kernel/asm-offsets.c   |  2 +-
 arch/arm/kvm/interrupts_head.S  | 29 +++++++++++++++++++------
 arch/arm64/kernel/asm-offsets.c |  2 +-
 arch/arm64/kvm/hyp.S            | 28 ++++++++++++++++++------
 include/kvm/arm_vgic.h          |  7 ++++--
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 48 +++++++++++++++++++++++++++++------------
 7 files changed, 92 insertions(+), 30 deletions(-)

arch/arm/kernel/asm-offsets.c +1 -1

index 85598b5..166cc98 100644
@@ -189,7 +189,7 @@ int main(void)
   DEFINE(VGIC_CPU_ELRSR,     offsetof(struct vgic_cpu, vgic_elrsr));
   DEFINE(VGIC_CPU_APR,               offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,                offsetof(struct vgic_cpu, vgic_lr));
-  DEFINE(VGIC_CPU_NR_LR,     offsetof(struct vgic_cpu, nr_lr));
+  DEFINE(VGIC_CPU_HW_CFG,    offsetof(struct vgic_cpu, hw_cfg));
 #ifdef CONFIG_KVM_ARM_TIMER
   DEFINE(VCPU_TIMER_CNTV_CTL,        offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
   DEFINE(VCPU_TIMER_CNTV_CVAL,       offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));

arch/arm/kvm/interrupts_head.S +23 -6

index 76af9302..9fbbf99 100644
@@ -419,7 +419,9 @@ vcpu      .req    r0              @ vcpu pointer always in r0
      ldr     r7, [r2, #GICH_EISR1]
      ldr     r8, [r2, #GICH_ELRSR0]
      ldr     r9, [r2, #GICH_ELRSR1]
-     ldr     r10, [r2, #GICH_APR]
+     ldr     r10, [r11, #VGIC_CPU_HW_CFG]
+     mov     r10, r10, lsr #HWCFG_APR_SHIFT
+     ldr     r10, [r2, r10]
 
      str     r3, [r11, #VGIC_CPU_HCR]
      str     r4, [r11, #VGIC_CPU_VMCR]

@@ -435,9 +437,15 @@ vcpu     .req    r0              @ vcpu pointer always in r0
      str     r5, [r2, #GICH_HCR]
 
      /* Save list registers */
-     add     r2, r2, #GICH_LR0
+     ldr     r4, [r11, #VGIC_CPU_HW_CFG]
+     mov     r10, r4, lsr #HWCFG_APR_SHIFT
+     /* the offset between GICH_APR & GICH_LR0 is 0x10 */
+     add     r10, r10, #0x10
+     add     r2, r2, r10
      add     r3, r11, #VGIC_CPU_LR
-     ldr     r4, [r11, #VGIC_CPU_NR_LR]
+     /* Get NR_LR from VGIC_CPU_HW_CFG */
+     ldr     r6, =HWCFG_NR_LR_MASK
+     and     r4, r4, r6
 1:   ldr     r6, [r2], #4
      str     r6, [r3], #4
      subs    r4, r4, #1

@@ -469,12 +477,21 @@ vcpu    .req    r0              @ vcpu pointer always in r0
 
      str     r3, [r2, #GICH_HCR]
      str     r4, [r2, #GICH_VMCR]
-     str     r8, [r2, #GICH_APR]
+     ldr     r6, [r11, #VGIC_CPU_HW_CFG]
+     mov     r6, r6, lsr #HWCFG_APR_SHIFT
+     str     r8, [r2, r6]
 
      /* Restore list registers */
-     add     r2, r2, #GICH_LR0
+     ldr     r4, [r11, #VGIC_CPU_HW_CFG]
+     mov     r6, r4, lsr #HWCFG_APR_SHIFT
+     /* the offset between GICH_APR & GICH_LR0 is 0x10 */
+     add     r6, r6, #0x10
+     /* get offset of GICH_LR0 */
+     add     r2, r2, r6
+     /* Get NR_LR from VGIC_CPU_HW_CFG */
      add     r3, r11, #VGIC_CPU_LR
-     ldr     r4, [r11, #VGIC_CPU_NR_LR]
+     ldr     r6, =HWCFG_NR_LR_MASK
+     and     r4, r4, r6
 1:   ldr     r6, [r3], #4
      str     r6, [r2], #4
      subs    r4, r4, #1

arch/arm64/kernel/asm-offsets.c +1 -1

index 646f888..2422358 100644
@@ -136,7 +136,7 @@ int main(void)
   DEFINE(VGIC_CPU_ELRSR,     offsetof(struct vgic_cpu, vgic_elrsr));
   DEFINE(VGIC_CPU_APR,               offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,                offsetof(struct vgic_cpu, vgic_lr));
-  DEFINE(VGIC_CPU_NR_LR,     offsetof(struct vgic_cpu, nr_lr));
+  DEFINE(VGIC_CPU_HW_CFG,    offsetof(struct vgic_cpu, hw_cfg));
   DEFINE(KVM_VTTBR,          offsetof(struct kvm, arch.vttbr));
   DEFINE(KVM_VGIC_VCTRL,     offsetof(struct kvm, arch.vgic.vctrl_base));
 #endif

arch/arm64/kvm/hyp.S +22 -6

index 2c56012..a4a8b3d 100644
@@ -402,7 +402,9 @@ __kvm_hyp_code_start:
      ldr     w8, [x2, #GICH_EISR1]
      ldr     w9, [x2, #GICH_ELRSR0]
      ldr     w10, [x2, #GICH_ELRSR1]
-     ldr     w11, [x2, #GICH_APR]
+     ldr     w11, [x3, #VGIC_CPU_HW_CFG]
+     mov     w11, w11, lsr #HWCFG_APR_SHIFT
+     ldr     w11, [x2, w10]
 CPU_BE(      rev     w4,  w4  )
 CPU_BE(      rev     w5,  w5  )
 CPU_BE(      rev     w6,  w6  )

@@ -425,8 +427,13 @@ CPU_BE(  rev     w11, w11 )
      str     wzr, [x2, #GICH_HCR]
 
      /* Save list registers */
-     add     x2, x2, #GICH_LR0
-     ldr     w4, [x3, #VGIC_CPU_NR_LR]
+     ldr     w4, [x3, #VGIC_CPU_HW_CFG]
+     mov     w6, w4, lsr #HWCFG_APR_SHIFT
+     ldr     w7, =HWCFG_NR_LR_MASK
+     and     w4, w4, w7
+     /* the offset between GICH_APR and GICH_LR0 is 0x10 */
+     add     w6, w6, 0x10
+     add     x2, x2, w6
      add     x3, x3, #VGIC_CPU_LR
 1:   ldr     w5, [x2], #4
 CPU_BE(      rev     w5, w5 )

@@ -461,11 +468,20 @@ CPU_BE( rev     w6, w6 )
 
      str     w4, [x2, #GICH_HCR]
      str     w5, [x2, #GICH_VMCR]
-     str     w6, [x2, #GICH_APR]
+     ldr     w4, [x3, #VGIC_CPU_HW_CFG]
+     mov     w4, w4, #HWCFG_APR_SHIFT
+     str     w6, [x2, w4]
 
      /* Restore list registers */
-     add     x2, x2, #GICH_LR0
-     ldr     w4, [x3, #VGIC_CPU_NR_LR]
+     ldr     w4, [x3, #VGIC_CPU_HW_CFG]
+     mov     w6, w4, #HWCFG_APR_SHIFT
+     /* the offset between GICH_APR and GICH_LR0 is 0x10 */
+     add     w6, w6, #0x10
+     /* get offset of GICH_LR0 */
+     add     x2, x2, w6
+     /* get NR_LR from VGIC_CPU_HW_CFG */
+     ldr     w6, =HWCFG_NR_LR_MASK
+     and     w4, w4, w6
      add     x3, x3, #VGIC_CPU_LR
 1:   ldr     w5, [x3], #4
 CPU_BE(      rev     w5, w5 )

include/kvm/arm_vgic.h +5 -2

index f27000f..eba4b51 100644
@@ -122,8 +122,11 @@ struct vgic_cpu {
      /* Bitmap of used/free list registers */
      DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
 
-     /* Number of list registers on this CPU */
-     int             nr_lr;
+     /*
+      * bit[31:16]: GICH_APR offset
+      * bit[15:0]:  Number of list registers on this CPU
+      */
+     u32             hw_cfg;
 
      /* CPU vif control registers for world switch */
      u32             vgic_hcr;

include/linux/irqchip/arm-gic.h +6 -0

index 45e2d8c..b055f92 100644
@@ -49,6 +49,8 @@
 #define GICH_ELRSR1                  0x34
 #define GICH_APR                     0xf0
 #define GICH_LR0                     0x100
+#define HIP04_GICH_APR                       0x70
+/* GICH_LR0 offset in HiP04 is 0x80 */
 
 #define GICH_HCR_EN                  (1 << 0)
 #define GICH_HCR_UIE                 (1 << 1)

@@ -73,6 +75,10 @@
 #define GICH_MISR_EOI                        (1 << 0)
 #define GICH_MISR_U                  (1 << 1)
 
+#define HWCFG_NR_LR_MASK     0xffff
+#define HWCFG_APR_SHIFT              16
+#define HWCFG_APR_MASK               (0xffff << HWCFG_APR_SHIFT)
+
 #ifndef __ASSEMBLY__
 
 struct device_node;

virt/kvm/arm/vgic.c +3 -1

index 47b2983..4c0c1e9 100644
@@ -76,6 +76,8 @@
 #define IMPLEMENTER_ARM              0x43b
 #define GICC_ARCH_VERSION_V2 0x2
 
+#define vgic_nr_lr(vcpu)     (vcpu->hw_cfg & HWCFG_NR_LR_MASK)
+
 /* Physical address of vgic virtual cpu interface */
 static phys_addr_t vgic_vcpu_base;
 

@@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
 static void vgic_kick_vcpus(struct kvm *kvm);
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
-static u32 vgic_nr_lr;
+static u32 vgic_hw_cfg;
 
 static unsigned int vgic_maint_irq;
 

virt/kvm/arm/vgic.c static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) · line 629

Quoted 5 lines

virt/kvm/arm/vgic.c +1 -1

@@ -624,9 +626,9 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
      struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
      int vcpu_id = vcpu->vcpu_id;
      int i, irq, source_cpu;
-     u32 *lr;
+     u32 *lr, nr_lr = vgic_nr_lr(vgic_cpu);

This is static for any system post-boot, right? Can't we set this global variable once like we did before instead of having to define these extra variables and do the bit manipulation all over the place?

-Christoffer

Quoted 1 line · Christoffer Dall

On Tue, May 20, 2014 at 09:10:27PM +0800, Haojian Zhuang wrote:

Quoted 33 lines

Quoted 23 lines

virt/kvm/arm/vgic.c +3 -1

index 47b2983..4c0c1e9 100644
@@ -76,6 +76,8 @@
 #define IMPLEMENTER_ARM              0x43b
 #define GICC_ARCH_VERSION_V2 0x2
 
+#define vgic_nr_lr(vcpu)     (vcpu->hw_cfg & HWCFG_NR_LR_MASK)
+
 /* Physical address of vgic virtual cpu interface */
 static phys_addr_t vgic_vcpu_base;
 

@@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
 static void vgic_kick_vcpus(struct kvm *kvm);
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
-static u32 vgic_nr_lr;
+static u32 vgic_hw_cfg;
 
 static unsigned int vgic_maint_irq;
 

virt/kvm/arm/vgic.c static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) · line 629

Quoted 5 lines

virt/kvm/arm/vgic.c +1 -1

@@ -624,9 +626,9 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
      struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
      int vcpu_id = vcpu->vcpu_id;
      int i, irq, source_cpu;
-     u32 *lr;
+     u32 *lr, nr_lr = vgic_nr_lr(vgic_cpu);

This is static for any system post-boot, right? Can't we set this global variable once like we did before instead of having to define these extra variables and do the bit manipulation all over the place?

Quoted 1 line

-Christoffer

I tried to define a global gich_apr variable before. But Marc didn't agree on that. He suggested to use vgic_cpu_nr_lr to save both GICH_APR offset and nr_lr.

Adding gich_apr variable should be the simpler implementation.

Quoted 2 lines · Haojian Zhuang

Quoted 1 line · Christoffer Dall

On Tue, May 20, 2014 at 09:10:27PM +0800, Haojian Zhuang wrote:

[...]

Quoted 39 lines

Quoted 33 lines

Quoted 23 lines

virt/kvm/arm/vgic.c +3 -1

index 47b2983..4c0c1e9 100644
@@ -76,6 +76,8 @@
 #define IMPLEMENTER_ARM              0x43b
 #define GICC_ARCH_VERSION_V2 0x2
 
+#define vgic_nr_lr(vcpu)     (vcpu->hw_cfg & HWCFG_NR_LR_MASK)
+
 /* Physical address of vgic virtual cpu interface */
 static phys_addr_t vgic_vcpu_base;
 

@@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
 static void vgic_kick_vcpus(struct kvm *kvm);
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
-static u32 vgic_nr_lr;
+static u32 vgic_hw_cfg;
 
 static unsigned int vgic_maint_irq;
 

virt/kvm/arm/vgic.c static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) · line 629

Quoted 5 lines

virt/kvm/arm/vgic.c +1 -1

@@ -624,9 +626,9 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
      struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
      int vcpu_id = vcpu->vcpu_id;
      int i, irq, source_cpu;
-     u32 *lr;
+     u32 *lr, nr_lr = vgic_nr_lr(vgic_cpu);

This is static for any system post-boot, right? Can't we set this global variable once like we did before instead of having to define these extra variables and do the bit manipulation all over the place?

Quoted 1 line

-Christoffer

I tried to define a global gich_apr variable before. But Marc didn't agree on that. He suggested to use vgic_cpu_nr_lr to save both GICH_APR offset and nr_lr.

Quoted 2 lines

Adding gich_apr variable should be the simpler implementation.

You're talking about storing this information on the vgic_cpu struct, which is accessed on every world-switch patch. There, you don't want two memory accesses.

Here, on the other hand, you're in host kernel land, and you can do your bit-shuffling once, and always access a single static variable like we did before, which will simplify the C-code.

Quoted 51 lines · Christoffer Dall

Quoted 2 lines · Haojian Zhuang

Quoted 1 line · Christoffer Dall

On Tue, May 20, 2014 at 09:10:27PM +0800, Haojian Zhuang wrote:

[...]

Quoted 33 lines

Quoted 33 lines

Quoted 23 lines

virt/kvm/arm/vgic.c +3 -1

index 47b2983..4c0c1e9 100644
@@ -76,6 +76,8 @@
 #define IMPLEMENTER_ARM              0x43b
 #define GICC_ARCH_VERSION_V2 0x2
 
+#define vgic_nr_lr(vcpu)     (vcpu->hw_cfg & HWCFG_NR_LR_MASK)
+
 /* Physical address of vgic virtual cpu interface */
 static phys_addr_t vgic_vcpu_base;
 

@@ -97,7 +99,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
 static void vgic_kick_vcpus(struct kvm *kvm);
 static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg);
-static u32 vgic_nr_lr;
+static u32 vgic_hw_cfg;
 
 static unsigned int vgic_maint_irq;
 

virt/kvm/arm/vgic.c static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu) · line 629

Quoted 5 lines

virt/kvm/arm/vgic.c +1 -1

@@ -624,9 +626,9 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
      struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
      int vcpu_id = vcpu->vcpu_id;
      int i, irq, source_cpu;
-     u32 *lr;
+     u32 *lr, nr_lr = vgic_nr_lr(vgic_cpu);

This is static for any system post-boot, right? Can't we set this global variable once like we did before instead of having to define these extra variables and do the bit manipulation all over the place?

Quoted 8 lines

Quoted 1 line

-Christoffer

I tried to define a global gich_apr variable before. But Marc didn't agree on that. He suggested to use vgic_cpu_nr_lr to save both GICH_APR offset and nr_lr.

Adding gich_apr variable should be the simpler implementation.

You're talking about storing this information on the vgic_cpu struct, which is accessed on every world-switch patch. There, you don't want two memory accesses.

arch/arm/kvm/interrupts_head.S +0 -0

index 76af9302..b27e43f 100644
@@ -419,7 +419,9 @@ vcpu        .req    r0              @ vcpu pointer

always in r0

        ldr     r7, [r2, #GICH_EISR1]
        ldr     r8, [r2, #GICH_ELRSR0]
        ldr     r9, [r2, #GICH_ELRSR1]

arch/arm/kvm/interrupts_head.S +3 -1

-       ldr     r10, [r2, #GICH_APR]
+       ldr     r10, =gich_apr
+       ldr     r10, [r10]
+       ldr     r10, [r2, r10]
 
        str     r3, [r11, #VGIC_CPU_HCR]
        str     r4, [r11, #VGIC_CPU_VMCR]

@@ -435,7 +437,11 @@ vcpu       .req    r0              @ vcpu pointer

always in r0

        str     r5, [r2, #GICH_HCR]

        /* Save list registers */

arch/arm/kvm/interrupts_head.S +5 -1

-       add     r2, r2, #GICH_LR0
+       ldr     r10, =gich_apr
+       ldr     r10, [r10]
+       /* the offset between GICH_APR & GICH_LR0 is 0x10 */
+       add     r10, r10, #0x10
+       add     r2, r2, r10
        add     r3, r11, #VGIC_CPU_LR
        ldr     r4, [r11, #VGIC_CPU_NR_LR]
 1:     ldr     r6, [r2], #4

@@ -469,10 +475,16 @@ vcpu      .req    r0              @ vcpu pointer

always in r0

        str     r3, [r2, #GICH_HCR]
        str     r4, [r2, #GICH_VMCR]

arch/arm/kvm/interrupts_head.S +8 -2

-       str     r8, [r2, #GICH_APR]
+       ldr     r6, =gich_apr
+       ldr     r6, [r6]
+       str     r8, [r2, r6]
 
        /* Restore list registers */
-       add     r2, r2, #GICH_LR0
+       ldr     r6, =gich_apr
+       ldr     r6, [r6]
+       /* the offset between GICH_APR & GICH_LR0 is 0x10 */
+       add     r6, r6, #0x10
+       add     r2, r2, r6
        add     r3, r11, #VGIC_CPU_LR
        ldr     r4, [r11, #VGIC_CPU_NR_LR]
 1:     ldr     r6, [r3], #4

@@ -618,3 +630,7 @@ vcpu        .req    r0              @ vcpu pointer

always in r0
.macro load_vcpu

        mrc     p15, 4, vcpu, c13, c0, 2        @ HTPIDR

.endm

arch/arm/kvm/interrupts_head.S +4 -0

+
+       .global gich_apr
+gich_apr:
+       .long   GICH_APR

virt/kvm/arm/vgic.c +14 -1

index 47b2983..6bf31db 100644
@@ -1470,17 +1470,30 @@ static struct notifier_block vgic_cpu_nb = {
        .notifier_call = vgic_cpu_notify,
 };
 
+static const struct of_device_id of_vgic_ids[] = {
+       {
+               .compatible = "arm,cortex-a15-gic",
+               .data = (void *)GICH_APR,
+       }, {
+               .compatible = "hisilicon,hip04-gic",
+               .data = (void *)HIP04_GICH_APR,
+       }, {
+       },
+};
+
 int kvm_vgic_hyp_init(void)
 {
        int ret;
        struct resource vctrl_res;
        struct resource vcpu_res;
+       const struct of_device_id *match;
 
-       vgic_node = of_find_compatible_node(NULL, NULL, "arm,cortex-a15-gic");
+       vgic_node = of_find_matching_node_and_match(NULL, of_vgic_ids, &match);
        if (!vgic_node) {
                kvm_err("error: no compatible vgic node in DT\n");
                return -ENODEV;
        }
+       gich_apr = (unsigned int)match->data;
 
        vgic_maint_irq = irq_of_parse_and_map(vgic_node, 0);
        if (!vgic_maint_irq) {

It's the implementation of gich_apr in arm32.

We needn't add or change anything in struct vgic_cpu. And both the assembly code and the code could be much easier.

Quoted 4 lines

Here, on the other hand, you're in host kernel land, and you can do your bit-shuffling once, and always access a single static variable like we did before, which will simplify the C-code.

No bit-shuffling in gich_apr implementation. Is it right?

Quoted 3 lines · Haojian Zhuang

On 20 May 2014 22:01, Christoffer Dall <[email protected]> wrote:
It's the implementation of gich_apr in arm32.

Quoted 3 lines

We needn't add or change anything in struct vgic_cpu. And both the assembly code and the code could be much easier.

But we do end up with an extra memory access from EL2 in the critical path, and I believe Marc's concern here is that if we cross a cache line, this might really hurt performance.

Quoted 4 lines · Christoffer Dall

Quoted 3 lines · Haojian Zhuang

On 20 May 2014 22:01, Christoffer Dall <[email protected]> wrote:
It's the implementation of gich_apr in arm32.

Quoted 8 lines

Quoted 3 lines

We needn't add or change anything in struct vgic_cpu. And both the assembly code and the code could be much easier.

But we do end up with an extra memory access from EL2 in the critical path, and I believe Marc's concern here is that if we cross a cache line, this might really hurt performance.

Sorry. Do we may cross a cache line or a TLB entry?

I think that you're concerning to cross TLB entries. The reason is in
below.

1. If the problem is on crossing cache line, it's caused by too much instructions. Either the packing nr_lr or the gich_apr adds some instructions. The packing nr_lr needs a little more instructions.

2. ldr instruction is a pseudo instruction. So it's parsed into operation on PC register.

Now I put gich_apr in interrupts_head.S, it results
in gich_apr variable before __kvm_hyp_code_start. It may cross the TLB entries.
How about to declare gich_apr after __kvm_cpu_return in interrupts.S? Since save_vgic_state & restore_vgic_state is only used once, declaring gich_apr just after the code could avoid crossing TLB entry.

Quoted 16 lines · Haojian Zhuang

Quoted 4 lines · Christoffer Dall

Quoted 3 lines · Haojian Zhuang

On 20 May 2014 22:01, Christoffer Dall <[email protected]> wrote:
It's the implementation of gich_apr in arm32.

Quoted 8 lines

Quoted 3 lines

We needn't add or change anything in struct vgic_cpu. And both the assembly code and the code could be much easier.

But we do end up with an extra memory access from EL2 in the critical path, and I believe Marc's concern here is that if we cross a cache line, this might really hurt performance.

Sorry. Do we may cross a cache line or a TLB entry?

Quoted 6 lines

I think that you're concerning to cross TLB entries. The reason is in
below.

1. If the problem is on crossing cache line, it's caused by too much instructions. Either the packing nr_lr or the gich_apr adds some instructions. The packing nr_lr needs a little more instructions.

I don't see why this argument is valid. If you have a separate

instruction and data cache, you may be loading from a different cache line when placing the static value close to your instructions. If you add a variable to the vcpu struct, all of the fields may no longer fit in a single data cache line and you may cause the memory subsystem to have to fetch another cache line. I believe the latter is Marc's

Quoted 17 lines · Christoffer Dall

Quoted 16 lines · Haojian Zhuang

Quoted 4 lines · Christoffer Dall

Quoted 3 lines · Haojian Zhuang

On 20 May 2014 22:01, Christoffer Dall <[email protected]> wrote:
It's the implementation of gich_apr in arm32.

Quoted 8 lines

Quoted 3 lines

We needn't add or change anything in struct vgic_cpu. And both the assembly code and the code could be much easier.

But we do end up with an extra memory access from EL2 in the critical path, and I believe Marc's concern here is that if we cross a cache line, this might really hurt performance.

Sorry. Do we may cross a cache line or a TLB entry?

Quoted 8 lines

Quoted 6 lines

I think that you're concerning to cross TLB entries. The reason is in
below.

1. If the problem is on crossing cache line, it's caused by too much instructions. Either the packing nr_lr or the gich_apr adds some instructions. The packing nr_lr needs a little more instructions.

I don't see why this argument is valid. If you have a separate

I want to make it clear what I missing.

Quoted 5 lines

instruction and data cache, you may be loading from a different cache line when placing the static value close to your instructions. If you add a variable to the vcpu struct, all of the fields may no longer fit in a single data cache line and you may cause the memory subsystem to have to fetch another cache line. I believe the latter is Marc's

Yes, I forgot new gich_apr is the only variable in the assembly code. So the gich_apr will be load from a different cache line.

Then let's come back to packing hw_cfg.

Now the high word is used to store the offset of GICH_APR. The unpacking operation is too complex to calculate the register offset, especially in arm64 implementation.

How about changing the packing mechanism?

1. Add the definition of enconding in arm-gic.h.

#define HIP04_GIC (1 << 16)
#define HIP04_GICH_APR 0x70
#define HIP04_GICH_LR0 0x80

2. The code in save_vgic_state could be changed in below.

ldr r9, [r2, #GICH_ELRSR1]

(quoted diff) +4 -0

+ldr     r10, [r3, #VGIC_CPU_HW_CFG]
+tst     r10, #HIP04_GIC
+ldreq  r10, [r2, #GICH_APR]
+ldrne  r10, [r2, #HIP04_GICH_APR]

Although I used the condition checking at here, the code could be easier.

I think that the executing time on "ldr" and "ldreq" should be same, because CPCS should be ready

Then calculation is avoid. Only three instructions are appended for both GICH_APR & GICH_LR0. The implementation in arm64 should be same & simple.

In ARM standard GIC, GICH_APR offset is 0xf0 & GICH_LR0 offset is 0x100. In HiP04 GIC, GICH_APR offset is 0x70 & GICH_LR0 offset is 0x80.

Now reuse the nr_lr field in struct vgic_cpu. Bit[31:16] is used to store GICH_APR offset in HiP04, and bit[15:0] is used to store real nr_lr variable. In ARM standard GIC, don't set bit[31:16]. So we could avoid to change the VGIC implementation in arm64.

Signed-off-by Haojian Zhuang <[email protected]>

---
 arch/arm/kernel/asm-offsets.c   |  2 +-
 arch/arm/kvm/interrupts_head.S  | 29 +++++++++++++++++++------
 arch/arm64/kernel/asm-offsets.c |  2 +-
 arch/arm64/kvm/hyp.S            | 28 ++++++++++++++++++------
 include/kvm/arm_vgic.h          |  7 ++++--
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 48 +++++++++++++++++++++++++++++------------
 7 files changed, 92 insertions(+), 30 deletions(-)

arch/arm/kernel/asm-offsets.c +1 -1

index 85598b5..166cc98 100644
@@ -189,7 +189,7 @@ int main(void)
   DEFINE(VGIC_CPU_ELRSR,       offsetof(struct vgic_cpu, vgic_elrsr));
   DEFINE(VGIC_CPU_APR,         offsetof(struct vgic_cpu, vgic_apr));
   DEFINE(VGIC_CPU_LR,          offsetof(struct vgic_cpu, vgic_lr));
-  DEFINE(VGIC_CPU_NR_LR,       offsetof(struct vgic_cpu, nr_lr));
+  DEFINE(VGIC_CPU_HW_CFG,      offsetof(struct vgic_cpu, hw_cfg));
 #ifdef CONFIG_KVM_ARM_TIMER
   DEFINE(VCPU_TIMER_CNTV_CTL,  offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl));
   DEFINE(VCPU_TIMER_CNTV_CVAL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval));

arch/arm/kvm/interrupts_head.S +3 -1

index 76af9302..9fbbf99 100644
@@ -419,7 +419,9 @@ vcpu        .req    r0              @ vcpu pointer always in r0
        ldr     r7, [r2, #GICH_EISR1]
        ldr     r8, [r2, #GICH_ELRSR0]
        ldr     r9, [r2, #GICH_ELRSR1]
-       ldr     r10, [r2, #GICH_APR]
+       ldr     r10, [r11, #VGIC_CPU_HW_CFG]
+       mov     r10, r10, lsr #HWCFG_APR_SHIFT
+       ldr     r10, [r2, r10]
 
        str     r3, [r11, #VGIC_CPU_HCR]
        str     r4, [r11, #VGIC_CPU_VMCR]