enable HiP04 SoC

Bit[9:0] is interrupt ID field in GICC_IAR. Bit[12:10] is CPU ID field, and others are reserved.

So we should use GICC_IAR_INT_ID_MASK to get interrupt ID. It's not a good way to use ~0x1c00 (CPU ID field) to get interrupt ID.

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

---
 drivers/irqchip/irq-gic.c       | 2 +-
 include/linux/irqchip/arm-gic.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

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                     | 160 ++++++++++++++++++++------
 2 files changed, 129 insertions(+), 32 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 +5 -2

index f711fb6..d1d1430 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.

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

+{
+       return (gic->nr_cpu_if == 8);
+}
+
+static inline int gic_irqs_per_target_reg(struct gic_chip_data *gic)
+{
+       return 32 / gic->nr_cpu_if;
+}
+
 #ifdef CONFIG_SMP
+static inline u32 irq_to_target_reg(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))
+               i = i & ~3U;
+       else
+               i = (i << 1) & ~3U;
+       return (i + GIC_DIST_TARGET);
+}
+
+static inline 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 inline 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(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 +401,24 @@ 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);
-               mask |= mask >> 16;
-               mask |= mask >> 8;
+       /*
+        * ARM GIC uses 8 registers for interrupt 0-31,
+        * HiP04 GIC uses 16 registers for interrupt 0-31.

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

                if (mask)
                        break;
        }

@@ -370,6 +426,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 +452,17 @@ 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;

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

        /*
         * Set priority on all global interrupts.

@@ -423,7 +490,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 +498,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 +534,7 @@ void gic_cpu_if_down(void)
  */
 static void gic_dist_save(unsigned int gic_nr)
 {
-       unsigned int gic_irqs;
+       unsigned int gic_irqs, nr_irq_per_target;
        void __iomem *dist_base;
        int i;
 

@@ -476,6 +543,7 @@ static void gic_dist_save(unsigned int gic_nr)
 
        gic_irqs = gic_data[gic_nr].gic_irqs;
        dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+       nr_irq_per_target = gic_irqs_per_target_reg(&gic_data[gic_nr]);
 
        if (!dist_base)
                return;

@@ -484,7 +552,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
                gic_data[gic_nr].saved_spi_target[i] =
                        readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -502,7 +570,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, nr_irq_per_target;
        unsigned int i;
        void __iomem *dist_base;
 

@@ -511,6 +579,7 @@ static void gic_dist_restore(unsigned int gic_nr)
 
        gic_irqs = gic_data[gic_nr].gic_irqs;
        dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+       nr_irq_per_target = gic_irqs_per_target_reg(&gic_data[gic_nr]);
 
        if (!dist_base)
                return;

@@ -525,7 +594,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
                writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
                        dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -665,9 +734,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 +760,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 +784,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))

@@ -931,7 +1015,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 +1051,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 +1082,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 +1164,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

Quoted 1 line

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

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

Quoted 4 lines

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

+{
+       return (gic->nr_cpu_if == 8);
+}
+

Quoted 4 lines · On Thursday 15 May 2014 10:34:35 Marc Zyngier wrote:

Quoted 1 line

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Quoted 4 lines

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

Quoted 4 lines

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

+{
+       return (gic->nr_cpu_if == 8);
+}
+

I think it's actually the common style to spell out the 'inline' for trivial helpers like this. The compiler doesn't need it, but it also doesn't hurt and most people write it.

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 15 May 2014 10:34:35 Marc Zyngier wrote:

Quoted 1 line

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Quoted 9 lines

Quoted 4 lines

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

Quoted 4 lines

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

+{
+       return (gic->nr_cpu_if == 8);
+}
+

I think it's actually the common style to spell out the 'inline' for trivial helpers like this. The compiler doesn't need it, but it also doesn't hurt and most people write it.

Only in header files, no?

I've been chastised previously for putting this in .c files.

Quoted 17 lines · On Thursday 15 May 2014 13:08:17 Christoffer Dall wrote:

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 15 May 2014 10:34:35 Marc Zyngier wrote:

Quoted 1 line

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Quoted 9 lines

Quoted 4 lines

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

Quoted 4 lines

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

+{
+       return (gic->nr_cpu_if == 8);
+}
+

I think it's actually the common style to spell out the 'inline' for trivial helpers like this. The compiler doesn't need it, but it also doesn't hurt and most people write it.

Only in header files, no?

Quoted 1 line

I've been chastised previously for putting this in .c files.

It's crazy to make it a hard rule for C files one way or the other, given that the compiler doesn't care.

I normally spell out the the inline part in my code because I find that clearer to read.

Quoted 2 lines · Haojian Zhuang

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Quoted 18 lines

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

-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);
-               mask |= mask >> 16;
-               mask |= mask >> 8;
+       /*
+        * 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic)) {
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+                       mask |= mask >> 16;

drivers/irqchip/irq-gic.c void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) · line 421

Quoted 5 lines

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

+                       mask |= mask >> 8;
+               } else {                        /* HiP04 GIC */
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+                       mask |= mask >> 16;
+               }

You have irq_to_target_reg now, and you can rewrite most of this without duplication (see my previous review comment).

Quoted 12 lines

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

@@ -392,10 +452,17 @@ 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic))

drivers/irqchip/irq-gic.c static void __init gic_dist_init(struct gic_chip_data *gic) · line 465

Quoted 6 lines

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

+                       writel_relaxed(cpumask,
+                                      base + GIC_DIST_TARGET + i / 4 * 4);
+               else
+                       writel_relaxed(cpumask,
+                                      base + GIC_DIST_TARGET + i / 2 * 4);
+       }

Same here.

Quoted 6 lines · Marc Zyngier

Quoted 2 lines · Haojian Zhuang

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Since others also agree on inline, I'll keep to use it.

Quoted 18 lines

Quoted 18 lines

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

-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);
-               mask |= mask >> 16;
-               mask |= mask >> 8;
+       /*
+        * 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic)) {
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+                       mask |= mask >> 16;

Quoted 9 lines

drivers/irqchip/irq-gic.c void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) · line 421

Quoted 5 lines

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

+                       mask |= mask >> 8;
+               } else {                        /* HiP04 GIC */
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+                       mask |= mask >> 16;
+               }

You have irq_to_target_reg now, and you can rewrite most of this without duplication (see my previous review comment).

At here, the offset from GIC_DIST_TARGET is got directly. In irq_to_target_reg(), the parameter is struct irq_data. These two cases are different.

How could I reuse the irq_to_target_reg() at here?

Quoted 13 lines

Quoted 13 lines

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

@@ -392,10 +452,17 @@ 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic))

Quoted 9 lines

drivers/irqchip/irq-gic.c static void __init gic_dist_init(struct gic_chip_data *gic) · line 465

Quoted 6 lines

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

+                       writel_relaxed(cpumask,
+                                      base + GIC_DIST_TARGET + i / 4 * 4);
+               else
+                       writel_relaxed(cpumask,
+                                      base + GIC_DIST_TARGET + i / 2 * 4);
+       }

Same here.

Same reason that I can't use irq_to_target_reg(). There's no struct irq_data at here.

Quoted 37 lines · Haojian Zhuang

Quoted 6 lines · Marc Zyngier

Quoted 2 lines · Haojian Zhuang

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Since others also agree on inline, I'll keep to use it.

Quoted 18 lines

Quoted 18 lines

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

-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);
-               mask |= mask >> 16;
-               mask |= mask >> 8;
+       /*
+        * 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic)) {
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+                       mask |= mask >> 16;

Quoted 9 lines

drivers/irqchip/irq-gic.c void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) · line 421

Quoted 5 lines

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

+                       mask |= mask >> 8;
+               } else {                        /* HiP04 GIC */
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+                       mask |= mask >> 16;
+               }

You have irq_to_target_reg now, and you can rewrite most of this without duplication (see my previous review comment).

Quoted 5 lines

At here, the offset from GIC_DIST_TARGET is got directly. In irq_to_target_reg(), the parameter is struct irq_data. These two cases are different.

How could I reuse the irq_to_target_reg() at here?

By using your imagination, and redefining irq_to_target_reg to this:

static inline u32 irq_to_target_reg(struct gic_chip_data *gic, int irq)

{
       if (!gic_is_standard(gic))
               i *= 2;
       irq &= ~3U;
       return (i + GIC_DIST_TARGET);
}

You could then try modifying the only existing caller, and then rewrite the above hunk as such:

       for (i = mask = 0; i < 32; i += gic_irqs_per_target_reg(gic)) {
               mask = readl_relaxed(base + irq_to_target_reg(gic, i));
               mask |= mask >> 16;
               if (gic_is_standard(gic))
                       mask |= mask >> 8;
       }

Quoted 58 lines · Marc Zyngier

Quoted 37 lines · Haojian Zhuang

Quoted 6 lines · Marc Zyngier

Quoted 2 lines · Haojian Zhuang

+static inline bool gic_is_standard(struct gic_chip_data *gic)

Please loose all of the inlines. The compiler can do this by itself.

Since others also agree on inline, I'll keep to use it.

Quoted 18 lines

Quoted 18 lines

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

-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);
-               mask |= mask >> 16;
-               mask |= mask >> 8;
+       /*
+        * 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 += gic_irqs_per_target_reg(gic)) {
+               if (gic_is_standard(gic)) {
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+                       mask |= mask >> 16;

Quoted 9 lines

drivers/irqchip/irq-gic.c void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq) · line 421

Quoted 5 lines

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

+                       mask |= mask >> 8;
+               } else {                        /* HiP04 GIC */
+                       mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+                       mask |= mask >> 16;
+               }

You have irq_to_target_reg now, and you can rewrite most of this without duplication (see my previous review comment).

Quoted 5 lines

At here, the offset from GIC_DIST_TARGET is got directly. In irq_to_target_reg(), the parameter is struct irq_data. These two cases are different.

How could I reuse the irq_to_target_reg() at here?

By using your imagination, and redefining irq_to_target_reg to this:

static inline u32 irq_to_target_reg(struct gic_chip_data *gic, int irq)

{
       if (!gic_is_standard(gic))
               i *= 2;
       irq &= ~3U;
       return (i + GIC_DIST_TARGET);
}

You could then try modifying the only existing caller, and then rewrite the above hunk as such:

Quoted 7 lines

       for (i = mask = 0; i < 32; i += gic_irqs_per_target_reg(gic)) {
               mask = readl_relaxed(base + irq_to_target_reg(gic, i));
               mask |= mask >> 16;
               if (gic_is_standard(gic))
                       mask |= mask >> 8;
       }

But why should I name it as irq_to_target_reg()? There's nothing related to irq at here.

So I'll append a new function to handle it.

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                     | 160 ++++++++++++++++++++------
 2 files changed, 129 insertions(+), 32 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 +49 -4

index f711fb6..d1d1430 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,64 @@ static int gic_retrigger(struct irq_data *d)
 	return 0;
 }
 
+static inline bool gic_is_standard(struct gic_chip_data *gic)
+{
+	return (gic->nr_cpu_if == 8);
+}
+
+static inline int gic_irqs_per_target_reg(struct gic_chip_data *gic)
+{
+	return 32 / gic->nr_cpu_if;
+}
+
 #ifdef CONFIG_SMP
+static inline u32 irq_to_target_reg(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))
+		i = i & ~3U;
+	else
+		i = (i << 1) & ~3U;
+	return (i + GIC_DIST_TARGET);
+}
+
+static inline 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 inline 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 -11

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

@@ -354,15 +401,24 @@ 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);
-		mask |= mask >> 16;
-		mask |= mask >> 8;
+	/*
+	 * 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 += gic_irqs_per_target_reg(gic)) {
+		if (gic_is_standard(gic)) {
+			mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+			mask |= mask >> 16;
+			mask |= mask >> 8;
+		} else {			/* HiP04 GIC */
+			mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+			mask |= mask >> 16;
+		}
 		if (mask)
 			break;
 	}

@@ -370,6 +426,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 +452,17 @@ 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 += gic_irqs_per_target_reg(gic)) {
+		if (gic_is_standard(gic))
+			writel_relaxed(cpumask,
+				       base + GIC_DIST_TARGET + i / 4 * 4);
+		else
+			writel_relaxed(cpumask,
+				       base + GIC_DIST_TARGET + i / 2 * 4);
+	}
 
 	/*
 	 * Set priority on all global interrupts.

@@ -423,7 +490,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 +498,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 +534,7 @@ void gic_cpu_if_down(void)
  */
 static void gic_dist_save(unsigned int gic_nr)
 {
-	unsigned int gic_irqs;
+	unsigned int gic_irqs, nr_irq_per_target;
 	void __iomem *dist_base;
 	int i;
 

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

 	if (!dist_base)
 		return;

@@ -484,7 +552,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
 		gic_data[gic_nr].saved_spi_target[i] =
 			readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -502,7 +570,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, nr_irq_per_target;
 	unsigned int i;
 	void __iomem *dist_base;
 

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

 	if (!dist_base)
 		return;

@@ -525,7 +594,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
 		writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
 			dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -665,9 +734,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);
-
+	/*

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

@@ -681,10 +760,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 +20 -7

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

@@ -700,7 +784,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))

@@ -931,7 +1015,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 +1051,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 +1082,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 */

Haojian,

Quoted 6 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.

Quoted 8 lines

* 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                     | 160 ++++++++++++++++++++------
 2 files changed, 129 insertions(+), 32 deletions(-)

As I'm still coming up to speed on this driver, I'm just going to make some general comments about this patch. If I appear to be off-base on something, please don't assume I know better ;-)

I have no strong preference regarding using 'inline'. In my own code, I prefer to be explicit, but I also prefer to use macros in some of the situations below.

I would try to avoid runtime calculations of known values (register offsets, masks, etc) where possible. I've tried to highlight some of them below:

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

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(d);
+
      raw_spin_lock(&irq_controller_lock);
-     mask = 0xff << shift;
+     mask = irq_to_core_mask(d);

You're inside a spinlock here, calculating a mask value (in irq_to_core_mask()) that never changes after boot. It, in turn, calls irq_to_core_shift() and checks gic_is_standard() to use two values that also never change after boot.

Sure, these functions are inlined, but the compiler can't optimize for something that's determined at runtime. eg the mask, and X and Y in ((i % X) << Y).

Quoted 93 lines

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

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

@@ -354,15 +401,24 @@ 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);
-             mask |= mask >> 16;
-             mask |= mask >> 8;
+     /*
+      * 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 += gic_irqs_per_target_reg(gic)) {
+             if (gic_is_standard(gic)) {
+                     mask = readl_relaxed(base + GIC_DIST_TARGET + i);
+                     mask |= mask >> 16;
+                     mask |= mask >> 8;
+             } else {                        /* HiP04 GIC */
+                     mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
+                     mask |= mask >> 16;
+             }
              if (mask)
                      break;
      }

@@ -370,6 +426,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 +452,17 @@ 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 += gic_irqs_per_target_reg(gic)) {
+             if (gic_is_standard(gic))
+                     writel_relaxed(cpumask,
+                                    base + GIC_DIST_TARGET + i / 4 * 4);
+             else
+                     writel_relaxed(cpumask,
+                                    base + GIC_DIST_TARGET + i / 2 * 4);
+     }
 
      /*
       * Set priority on all global interrupts.

@@ -423,7 +490,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 +498,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 +534,7 @@ void gic_cpu_if_down(void)
  */
 static void gic_dist_save(unsigned int gic_nr)
 {
-     unsigned int gic_irqs;
+     unsigned int gic_irqs, nr_irq_per_target;
      void __iomem *dist_base;
      int i;
 

drivers/irqchip/irq-gic.c static void gic_dist_save(unsigned int gic_nr) · line 546

Quoted 4 lines

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

@@ -476,6 +543,7 @@ static void gic_dist_save(unsigned int gic_nr)
 
      gic_irqs = gic_data[gic_nr].gic_irqs;
      dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+     nr_irq_per_target = gic_irqs_per_target_reg(&gic_data[gic_nr]);

gic_dist_save() isn't called too often (suspend/idle), but this is another example of calculating something that doesn't change after boot.

Quoted 22 lines

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

      if (!dist_base)
              return;

@@ -484,7 +552,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
              gic_data[gic_nr].saved_spi_target[i] =
                      readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -502,7 +570,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, nr_irq_per_target;
      unsigned int i;
      void __iomem *dist_base;
 

drivers/irqchip/irq-gic.c static void gic_dist_restore(unsigned int gic_nr) · line 582

Quoted 4 lines

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

@@ -511,6 +579,7 @@ static void gic_dist_restore(unsigned int gic_nr)
 
      gic_irqs = gic_data[gic_nr].gic_irqs;
      dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+     nr_irq_per_target = gic_irqs_per_target_reg(&gic_data[gic_nr]);

Same.

Quoted 20 lines

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

      if (!dist_base)
              return;

@@ -525,7 +594,7 @@ 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 < DIV_ROUND_UP(gic_irqs, nr_irq_per_target); i++)
              writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
                      dist_base + GIC_DIST_TARGET + i * 4);
 

@@ -665,9 +734,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);
-
+     /*

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

Quoted 10 lines

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

+      * 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;

ditto. Inside a spinlock as well.

Quoted 8 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

@@ -681,10 +760,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 769

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;

ditto.

Quoted 64 lines

drivers/irqchip/irq-gic.c +20 -7

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

@@ -700,7 +784,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))

@@ -931,7 +1015,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 +1051,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 +1082,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;
 

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

Quoted 8 lines

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

      gic_irqs -= hwirq_base; /* calculate # of irqs to allocate */

@@ -1069,6 +1164,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);

I would re-evaluate your use of gic_is_standard() and attempt to eliminate all post-bootup callers of it.

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

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

---
 arch/arm/mach-hisi/Makefile   |   1 +
 arch/arm/mach-hisi/platmcpm.c | 309 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 310 insertions(+)
 create mode 100644 arch/arm/mach-hisi/platmcpm.c

arch/arm/mach-hisi/Makefile +1 -0

index 2ae1b59..e7a8640 100644
@@ -3,4 +3,5 @@
 #
 
 obj-y	+= hisilicon.o
+obj-$(CONFIG_MCPM)		+= platmcpm.o
 obj-$(CONFIG_SMP)		+= platsmp.o hotplug.o

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

new file mode 100644
index 0000000..6208bc6
@@ -0,0 +1,309 @@

arch/arm/mach-hisi/platmcpm.c new file +14 -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);
+	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);

Quoted 26 lines · Haojian Zhuang

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

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

---
 arch/arm/mach-hisi/Makefile   |   1 +
 arch/arm/mach-hisi/platmcpm.c | 309 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 310 insertions(+)
 create mode 100644 arch/arm/mach-hisi/platmcpm.c

arch/arm/mach-hisi/Makefile +1 -0

index 2ae1b59..e7a8640 100644
@@ -3,4 +3,5 @@
 #
 
 obj-y        += hisilicon.o
+obj-$(CONFIG_MCPM)           += platmcpm.o
 obj-$(CONFIG_SMP)            += platsmp.o hotplug.o

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

new file mode 100644
index 0000000..6208bc6
@@ -0,0 +1,309 @@

[...]

Quoted 14 lines

arch/arm/mach-hisi/platmcpm.c new file +14 -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);
+     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 121

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);

Did you find the answer regarding whether it's safe to enable snoops to a powered-down CPU?

Does HiP04 use CCI, or some different interconnect? As Nico pointed out, the above code would definitely not be safe for CCI.

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

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

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

+static int hip04_mcpm_power_up(unsigned int cpu, unsigned int cluster)
+{
+	unsigned long data, mask;
+

arch/arm/mach-hisi/platmcpm.c new file +31 -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);
+	}
+
+	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);
+
+	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);
+

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

+}
+
+static int hip04_mcpm_wait_for_powerdown(unsigned int cpu, unsigned int cluster)
+{
+	unsigned int data, tries;
+
+	BUG_ON(cluster >= HIP04_MAX_CLUSTERS ||
+	       cpu >= HIP04_MAX_CPUS_PER_CLUSTER);
+
+	for (tries = 0; tries < TIMEOUT_MSEC / POLL_MSEC; tries++) {
+		data = readl_relaxed(sysctrl + SC_CPU_RESET_STATUS(cluster));
+		if (!(data & CORE_RESET_STATUS(cpu))) {
+			msleep(POLL_MSEC);
+			continue;
+		}
+		return 0;
+	}
+	return -ETIMEDOUT;
+}
+
+static void hip04_mcpm_powered_up(void)
+{
+	if (!relocation)
+		return;
+	spin_lock(&boot_lock);
+	writel_relaxed(0, relocation);
+	writel_relaxed(0, relocation + 4);
+	writel_relaxed(0, relocation + 8);
+	writel_relaxed(0, relocation + 12);
+	spin_unlock(&boot_lock);
+}
+
+static const struct mcpm_platform_ops hip04_mcpm_ops = {
+	.power_up		= hip04_mcpm_power_up,
+	.power_down		= hip04_mcpm_power_down,
+	.wait_for_powerdown	= hip04_mcpm_wait_for_powerdown,
+	.powered_up		= hip04_mcpm_powered_up,
+};
+
+static bool __init hip04_cpu_table_init(void)
+{
+	unsigned int mpidr, cpu, cluster;
+
+	mpidr = read_cpuid_mpidr();
+	cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	if (cluster >= HIP04_MAX_CLUSTERS ||
+	    cpu >= HIP04_MAX_CPUS_PER_CLUSTER) {
+		pr_err("%s: boot CPU is out of bound!\n", __func__);
+		return false;
+	}
+	hip04_set_snoop_filter(cluster, 1);
+	hip04_cpu_table[cluster][cpu] = 1;
+	return true;
+}
+
+static int __init hip04_mcpm_init(void)
+{
+	struct device_node *np, *np_fab;
+	int ret = -ENODEV;
+
+	np = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
+	if (!np)
+		goto err;
+	np_fab = of_find_compatible_node(NULL, NULL, "hisilicon,hip04-fabric");
+	if (!np_fab)
+		goto err;
+
+	if (of_property_read_u32(np, "bootwrapper-phys",
+				 &hip04_boot.bootwrapper_phys)) {
+		pr_err("failed to get bootwrapper-phys\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "bootwrapper-size",
+				 &hip04_boot.bootwrapper_size)) {
+		pr_err("failed to get bootwrapper-size\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "bootwrapper-magic",
+				 &hip04_boot.bootwrapper_magic)) {
+		pr_err("failed to get bootwrapper-magic\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "relocation-entry",
+				 &hip04_boot.relocation_entry)) {
+		pr_err("failed to get relocation-entry\n");
+		ret = -EINVAL;
+		goto err;
+	}
+	if (of_property_read_u32(np, "relocation-size",
+				 &hip04_boot.relocation_size)) {
+		pr_err("failed to get relocation-size\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	relocation = ioremap(hip04_boot.relocation_entry,
+			     hip04_boot.relocation_size);
+	if (!relocation) {
+		pr_err("failed to map relocation space\n");
+		ret = -ENOMEM;
+		goto err;
+	}
+	sysctrl = of_iomap(np, 0);
+	if (!sysctrl) {
+		pr_err("failed to get sysctrl base\n");
+		ret = -ENOMEM;
+		goto err_sysctrl;
+	}
+	fabric = of_iomap(np_fab, 0);
+	if (!fabric) {
+		pr_err("failed to get fabric base\n");
+		ret = -ENOMEM;
+		goto err_fabric;
+	}
+
+	if (!hip04_cpu_table_init())
+		return -EINVAL;
+	ret = mcpm_platform_register(&hip04_mcpm_ops);
+	if (!ret) {
+		mcpm_sync_init(NULL);
+		pr_info("HiP04 MCPM initialized\n");
+	}
+	mcpm_smp_set_ops();
+	return ret;
+err_fabric:
+	iounmap(sysctrl);
+err_sysctrl:
+	iounmap(relocation);
+err:
+	return ret;
+}
+early_initcall(hip04_mcpm_init);

Signature · 1 line

1.9.1

Quoted 4 lines · Haojian Zhuang

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

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

Some more comments...

[...]

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

Quoted 6 lines

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

+static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
+{
+     unsigned long data;
+
+     if (!fabric)
+             return;

How could this validly be NULL?

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

Quoted 11 lines

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

+     data = readl_relaxed(fabric + FAB_SF_MODE);
+     if (on)
+             data |= 1 << cluster;
+     else
+             data &= ~(1 << cluster);
+     writel_relaxed(data, fabric + FAB_SF_MODE);
+     while (1) {
+             if (data == readl_relaxed(fabric + FAB_SF_MODE))
+                     break;
+     }
+}

The above could be easily coded in assembly for the power_up_setup callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

bx lr

:LC0: .word .
.word fabric_phys_addr - .LC0

That should be it.

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -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);

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

Quoted 4 lines

arch/arm/mach-hisi/platmcpm.c new file +4 -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);

Shouldn't you do the above writes only when hip04_cpu_table[cluster][cpu] is zero? Please see the comment in mcpm_cpu_power_down() about unordered calls.

Quoted 37 lines

arch/arm/mach-hisi/platmcpm.c new file +37 -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);
+     }
+
+     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);
+
+     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) {

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

Quoted 4 lines

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

+             /* 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 is still running\n", cluster, cpu);

This message is misleading. If execution gets here, that means mcpm_cpu_power_up() was called more than twice in a row for the same CPU which should never happen.

Quoted 9 lines

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

+             BUG();
+     }
+
+     spin_unlock(&boot_lock);
+
+     v7_exit_coherency_flush(louis);
+
+     __mcpm_cpu_down(cpu, cluster);
+

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

Quoted 4 lines

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

+     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));

You should not perform this outside the lock protected region as this could race with hip04_mcpm_power_up(). Instead, this should be done above when hip04_cpu_table[cluster][cpu] == 0 after being decremented.

Quoted 43 lines · Nicolas Pitre

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

Quoted 11 lines · Haojian Zhuang

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

+     data = readl_relaxed(fabric + FAB_SF_MODE);
+     if (on)
+             data |= 1 << cluster;
+     else
+             data &= ~(1 << cluster);
+     writel_relaxed(data, fabric + FAB_SF_MODE);
+     while (1) {
+             if (data == readl_relaxed(fabric + FAB_SF_MODE))
+                     break;
+     }
+}

The above could be easily coded in assembly for the power_up_setup callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

bx lr

:LC0: .word .
.word fabric_phys_addr - .LC0

Quoted 2 lines

That should be it.

No. These code should be executed before new CPU on. If I transfer them to assembler code, it means that code will be executed after new CPU on.

Then it results me failing to make new CPU online.

Quoted 10 lines

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -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);

Quoted 9 lines

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

Quoted 4 lines

arch/arm/mach-hisi/platmcpm.c new file +4 -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);

Shouldn't you do the above writes only when hip04_cpu_table[cluster][cpu] is zero? Please see the comment in mcpm_cpu_power_down() about unordered calls.

OK. I can add the check.

Quoted 37 lines

Quoted 37 lines

arch/arm/mach-hisi/platmcpm.c new file +37 -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);
+     }
+
+     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);
+
+     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) {

Quoted 9 lines

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

Quoted 4 lines

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

+             /* 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 is still running\n", cluster, cpu);

This message is misleading. If execution gets here, that means mcpm_cpu_power_up() was called more than twice in a row for the same CPU which should never happen.

OK. I'll replace the comments.

Quoted 9 lines

Quoted 9 lines

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

+             BUG();
+     }
+
+     spin_unlock(&boot_lock);
+
+     v7_exit_coherency_flush(louis);
+
+     __mcpm_cpu_down(cpu, cluster);
+

Quoted 9 lines

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

Quoted 4 lines

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

+     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));

You should not perform this outside the lock protected region as this could race with hip04_mcpm_power_up(). Instead, this should be done above when hip04_cpu_table[cluster][cpu] == 0 after being decremented.

No. power_down() is executed on the specified CPU. If spin_unlock() is placed after reset operation, it means that there's no chance to execute the spin_unlock(). Because CPU is already in reset mode at this time.

Quoted 44 lines · Haojian Zhuang

Quoted 43 lines · Nicolas Pitre

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

Quoted 11 lines · Haojian Zhuang

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

+     data = readl_relaxed(fabric + FAB_SF_MODE);
+     if (on)
+             data |= 1 << cluster;
+     else
+             data &= ~(1 << cluster);
+     writel_relaxed(data, fabric + FAB_SF_MODE);
+     while (1) {
+             if (data == readl_relaxed(fabric + FAB_SF_MODE))
+                     break;
+     }
+}

The above could be easily coded in assembly for the power_up_setup callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

bx lr

:LC0: .word .
.word fabric_phys_addr - .LC0

Quoted 6 lines

Quoted 2 lines

That should be it.

No. These code should be executed before new CPU on. If I transfer them to assembler code, it means that code will be executed after new CPU on.

Exact.

Quoted 1 line

Then it results me failing to make new CPU online.

The assembly code could be wrong as well. Are you sure this is not the actual reason?

Is there some documentation for this stuff?

Quoted 88 lines

Quoted 10 lines

Quoted 10 lines

arch/arm/mach-hisi/platmcpm.c new file +10 -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);

Quoted 9 lines

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

Quoted 4 lines

arch/arm/mach-hisi/platmcpm.c new file +4 -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);

Shouldn't you do the above writes only when hip04_cpu_table[cluster][cpu] is zero? Please see the comment in mcpm_cpu_power_down() about unordered calls.

OK. I can add the check.

Quoted 37 lines

Quoted 37 lines

arch/arm/mach-hisi/platmcpm.c new file +37 -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);
+     }
+
+     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);
+
+     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) {

Quoted 9 lines

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

Quoted 4 lines

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

+             /* 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 is still running\n", cluster, cpu);

This message is misleading. If execution gets here, that means mcpm_cpu_power_up() was called more than twice in a row for the same CPU which should never happen.

OK. I'll replace the comments.

Quoted 18 lines

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

Quoted 13 lines

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

+             BUG();
+     }
+
+     spin_unlock(&boot_lock);
+
+     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));

You should not perform this outside the lock protected region as this could race with hip04_mcpm_power_up(). Instead, this should be done above when hip04_cpu_table[cluster][cpu] == 0 after being decremented.

Quoted 4 lines

No. power_down() is executed on the specified CPU. If spin_unlock() is placed after reset operation, it means that there's no chance to execute the spin_unlock(). Because CPU is already in reset mode at this time.

Normally, reset is effective only when WFI is later executed. Are you sure this is not the case on hip04 as well?

Quoted 53 lines · Nicolas Pitre

Quoted 44 lines · Haojian Zhuang

Quoted 43 lines · Nicolas Pitre

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

Quoted 11 lines · Haojian Zhuang

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

+     data = readl_relaxed(fabric + FAB_SF_MODE);
+     if (on)
+             data |= 1 << cluster;
+     else
+             data &= ~(1 << cluster);
+     writel_relaxed(data, fabric + FAB_SF_MODE);
+     while (1) {
+             if (data == readl_relaxed(fabric + FAB_SF_MODE))
+                     break;
+     }
+}

The above could be easily coded in assembly for the power_up_setup callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

bx lr

:LC0: .word .
.word fabric_phys_addr - .LC0

Quoted 6 lines

Quoted 2 lines

That should be it.

No. These code should be executed before new CPU on. If I transfer them to assembler code, it means that code will be executed after new CPU on.

Quoted 9 lines

Exact.

Quoted 1 line

Then it results me failing to make new CPU online.

The assembly code could be wrong as well. Are you sure this is not the actual reason?

Is there some documentation for this stuff?

There's no problem in assembly code. I even rewrite your assembly code.

If I keep my c code with assembly code, new CPU could be online right. If I only use assembly code, I only get the kernel panic. So it's not caused by assembly code. It's caused by executing code after new CPU on.

There's no documentation on this. They didn't prepare well on documents. I think they'll improve it in the future.

Quoted 53 lines · Nicolas Pitre

Quoted 44 lines · Haojian Zhuang

Quoted 43 lines · Nicolas Pitre

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

Quoted 11 lines · Haojian Zhuang

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

+     data = readl_relaxed(fabric + FAB_SF_MODE);
+     if (on)
+             data |= 1 << cluster;
+     else
+             data &= ~(1 << cluster);
+     writel_relaxed(data, fabric + FAB_SF_MODE);
+     while (1) {
+             if (data == readl_relaxed(fabric + FAB_SF_MODE))
+                     break;
+     }
+}

The above could be easily coded in assembly for the power_up_setup callback thusly:

hip04_power_up_setup:

        cmp     r0, #0                  @ check affinity level
        bxeq    lr                      @ nothing to do at CPU level

        mrc     p15, 0, r0, c0, c0, 5   @ get MPIDR
        ubfx    r0, r0, #8, #8          @ extract cluster number

        adr     r1, .LC0
        ldmia   r1, {r2, r3}
        sub     r2, r2, r1              @ virt_addr - phys_addr
        ldr     r1, [r2, r3]            @ get fabric_phys_addr
        mov     r2, #1
        ldr     r3, [r1, #FAB_SF_MODE]  @ read "data"
        orr     r3, r3, r2, lsl r0      @ set cluster bit
        str     r3, [r1, #FAB_SF_MODE]  @ write it back

1:      ldr     r2, [r1, #FAB_SF_MODE]  @ read register content
        cmp     r2, r3                  @ make sure it matches
        bne     1b                      @ otherwise retry

bx lr

:LC0: .word .
.word fabric_phys_addr - .LC0

Quoted 6 lines

Quoted 2 lines

That should be it.

No. These code should be executed before new CPU on. If I transfer them to assembler code, it means that code will be executed after new CPU on.

Quoted 9 lines

Exact.

Quoted 1 line

Then it results me failing to make new CPU online.

The assembly code could be wrong as well. Are you sure this is not the actual reason?

Is there some documentation for this stuff?

There's no problem in assembly code. I even rewrite your assembly code.

If I keep my c code with assembly code, new CPU could be online right. If I only use assembly code, I only get the kernel panic. So it's not caused by assembly code. It's caused by executing code after new CPU on.

cpu_v7_reset() likes to give a reset pulse signal to CPU core logic. The operation on SC_CPU_RESET_REQ register likes make CPU out of reset mode. After system is power on, all CPUs except for CPU0 stay in reset mode.

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/kvm/interrupts_head.S  | 32 ++++++++++++++++++++++------
 include/kvm/arm_vgic.h          |  5 ++++-
 include/linux/irqchip/arm-gic.h |  6 ++++++
 virt/kvm/arm/vgic.c             | 47 +++++++++++++++++++++++++++++------------
 4 files changed, 69 insertions(+), 21 deletions(-)

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

index 76af9302..7aacaff 100644