Fixes for DT CPU topology specification on Exynos

On SoCs with more than 2 cores there are more than 2 S5P_ARM_CORE_* registers that can be addressed with fixed stride of 0x80. This patch renames the definitions of S5P_ARM_CORE1_* registers to be S5P_ARM_CORE_* and make them take physical core ID as argument to calculate register address.

Signed-off-by Tomasz Figa <[email protected]>

---
 arch/arm/mach-exynos/hotplug.c  | 2 +-
 arch/arm/mach-exynos/platsmp.c  | 6 +++---
 arch/arm/mach-exynos/regs-pmu.h | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

arch/arm/mach-exynos/hotplug.c +0 -0

index 5eead53..7e0f31a 100644

arch/arm/mach-exynos/hotplug.c +0 -0

                /*
                 * here's the WFI

arch/arm/mach-exynos/platsmp.c +0 -0

index 03e5e9f..7b7de4b 100644

Hi Tomasz,

Quoted 18 lines · Tomasz Figa

On SoCs with more than 2 cores there are more than 2 S5P_ARM_CORE_* registers that can be addressed with fixed stride of 0x80. This patch renames the definitions of S5P_ARM_CORE1_* registers to be S5P_ARM_CORE_* and make them take physical core ID as argument to calculate register address.

Signed-off-by Tomasz Figa <[email protected]>

---
  arch/arm/mach-exynos/hotplug.c  | 2 +-
  arch/arm/mach-exynos/platsmp.c  | 6 +++---
  arch/arm/mach-exynos/regs-pmu.h | 4 ++--
  3 files changed, 6 insertions(+), 6 deletions(-)

arch/arm/mach-exynos/hotplug.c +0 -0

index 5eead53..7e0f31a 100644

arch/arm/mach-exynos/hotplug.c static inline void platform_do_lowpower(unsigned int cpu, int *spurious) · line 99

Quoted 5 lines

arch/arm/mach-exynos/hotplug.c +1 -1

@@ -96,7 +96,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 
                 /* make cpu1 to be turned off at next WFI command */
                 if (cpu == 1)
-                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));

Exynos4412 has quad cores. If turn off third/fourth core, this patch could not turn off third/fourth cores.

Why didn't you use cpu number for argument of S5P_ARM_CORE_CONFIGURATION() as following?
- __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpu));

Quoted 8 lines

arch/arm/mach-exynos/hotplug.c +0 -0

                 /*
                  * here's the WFI

arch/arm/mach-exynos/platsmp.c +0 -0

index 03e5e9f..7b7de4b 100644

arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) · line 112

Quoted 8 lines

arch/arm/mach-exynos/platsmp.c +2 -2

@@ -107,14 +107,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
          */
         write_pen_release(phys_cpu);
 
-       if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
+       if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
-                            S5P_ARM_CORE1_CONFIGURATION);
+                            S5P_ARM_CORE_CONFIGURATION(1));

ditto.

Quoted 1 line

arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) · line 117

Quoted 5 lines

                 timeout = 10;

                 /* wait max 10 ms until cpu1 is on */

arch/arm/mach-exynos/platsmp.c +1 -1

-               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+               while ((__raw_readl(S5P_ARM_CORE_STATUS(1))

ditto.

When CPU topology is specified in device tree, cpu_logical_map() does not return core ID anymore, but rather full MPIDR value. This breaks existing calculation of PMU register offsets on Exynos SoCs.

This patch fixes the problem by adjusting the code to use only core ID bits of the value returned by cpu_logical_map() to allow CPU topology to be specified in device tree on Exynos SoCs.

Signed-off-by Tomasz Figa <[email protected]>

---
 arch/arm/mach-exynos/hotplug.c | 10 ++++++----
 arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
 2 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/mach-exynos/hotplug.c +0 -0

index 7e0f31a..8a5f07d 100644
@@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
 

arch/arm/mach-exynos/hotplug.c +3 -1

+               /* Turn the CPU off on next WFI instruction. */
+               __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(core_id));
 
                /*
                 * here's the WFI

@@ -106,7 +108,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
                    :
                    : "memory", "cc");
 
-               if (pen_release == cpu_logical_map(cpu)) {
+               if (pen_release == core_id) {
                        /*
                         * OK, proper wakeup, we're done
                         */

arch/arm/mach-exynos/platsmp.c +18 -14

index 7b7de4b..e08b2c5 100644
@@ -89,7 +89,9 @@ static void exynos_secondary_init(unsigned int cpu)
 static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
        unsigned long timeout;
-       unsigned long phys_cpu = cpu_logical_map(cpu);
+       u32 mpidr = cpu_logical_map(cpu);
+       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+       u32 reg;
 
        /*
         * Set synchronisation state between this boot processor

@@ -102,19 +104,20 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
         * the holding pen - release it, then wait for it to flag
         * that it has been released by resetting pen_release.
         *
-        * Note that "pen_release" is the hardware CPU ID, whereas
+        * Note that "pen_release" is the hardware CPU core ID, whereas
         * "cpu" is Linux's internal ID.
         */
-       write_pen_release(phys_cpu);
+       write_pen_release(core_id);
 
-       if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
+       reg = __raw_readl(S5P_ARM_CORE_STATUS(core_id));
+       if (!(reg & S5P_CORE_LOCAL_PWR_EN)) {
                __raw_writel(S5P_CORE_LOCAL_PWR_EN,
-                            S5P_ARM_CORE_CONFIGURATION(1));
+                            S5P_ARM_CORE_CONFIGURATION(core_id));
 
                timeout = 10;
 
                /* wait max 10 ms until cpu1 is on */
-               while ((__raw_readl(S5P_ARM_CORE_STATUS(1))
+               while ((__raw_readl(S5P_ARM_CORE_STATUS(core_id))
                        & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
                        if (timeout-- == 0)
                                break;

@@ -146,10 +149,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
                 * Try to set boot address using firmware first
                 * and fall back to boot register if it fails.
                 */
-               if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
-                       __raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
+               if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
+                       __raw_writel(boot_addr, cpu_boot_reg(core_id));
 
-               call_firmware_op(cpu_boot, phys_cpu);
+               call_firmware_op(cpu_boot, core_id);
 
                arch_send_wakeup_ipi_mask(cpumask_of(cpu));
 

@@ -215,14 +218,16 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
         * boot register if it fails.
         */
        for (i = 1; i < max_cpus; ++i) {
-               unsigned long phys_cpu;
                unsigned long boot_addr;
+               u32 mpidr;
+               u32 core_id;
 
-               phys_cpu = cpu_logical_map(i);
+               mpidr = cpu_logical_map(i);
+               core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
                boot_addr = virt_to_phys(exynos4_secondary_startup);
 
-               if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
-                       __raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
+               if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
+                       __raw_writel(boot_addr, cpu_boot_reg(core_id));
        }
 }
 
--

1.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tomasz,

Quoted 27 lines · Tomasz Figa

When CPU topology is specified in device tree, cpu_logical_map() does not return core ID anymore, but rather full MPIDR value. This breaks existing calculation of PMU register offsets on Exynos SoCs.

This patch fixes the problem by adjusting the code to use only core ID bits of the value returned by cpu_logical_map() to allow CPU topology to be specified in device tree on Exynos SoCs.

Signed-off-by Tomasz Figa <[email protected]>

---
  arch/arm/mach-exynos/hotplug.c | 10 ++++++----
  arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
  2 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/mach-exynos/hotplug.c +3 -0

index 7e0f31a..8a5f07d 100644
@@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
 
  static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
  {
+       u32 mpidr = cpu_logical_map(cpu);
+       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+
         for (;;) {
 

arch/arm/mach-exynos/hotplug.c static inline void cpu_leave_lowpower(void) · line 99

Quoted 3 lines

arch/arm/mach-exynos/hotplug.c +0 -3

-               /* make cpu1 to be turned off at next WFI command */
-               if (cpu == 1)
-                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));

I think this macro is not yet in ML code.

Hi Chander,

Quoted 34 lines · Chander Kashyap

Hi Tomasz,

Quoted 29 lines · Tomasz Figa

When CPU topology is specified in device tree, cpu_logical_map() does not return core ID anymore, but rather full MPIDR value. This breaks existing calculation of PMU register offsets on Exynos SoCs.

This patch fixes the problem by adjusting the code to use only core ID bits of the value returned by cpu_logical_map() to allow CPU topology to be specified in device tree on Exynos SoCs.

Signed-off-by Tomasz Figa <[email protected]>

---
  arch/arm/mach-exynos/hotplug.c | 10 ++++++----
  arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
  2 files changed, 24 insertions(+), 17 deletions(-)

arch/arm/mach-exynos/ renamed from arch/arm/mach-exynos/hotplug.c +0 -0

hotplug.c
index 7e0f31a..8a5f07d 100644

arch/arm/mach-exynos/hotplug.c +3 -0

@@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
 
  static inline void platform_do_lowpower(unsigned int cpu, int
 *spurious)
  {
+       u32 mpidr = cpu_logical_map(cpu);
+       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+
         for (;;) {
 

Quoted 7 lines

arch/arm/mach-exynos/hotplug.c static inline void cpu_leave_lowpower(void) · line 102

Quoted 4 lines

arch/arm/mach-exynos/hotplug.c +0 -3

-               /* make cpu1 to be turned off at next WFI command */
-               if (cpu == 1)
-                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));
 

I think this macro is not yet in ML code.

What do you mean? The S5P_ARM_CORE_CONFIGURATION() macro is being added by patch 1/4.

Best regards,
Tomasz

On most platforms GIC registers are banked, so each CPU can access its registers at the same address. However there is a small number of SoCs on which the banking is not implemented and each CPU has its GIC register set at different offset from GIC base address.

Originally the driver used simple maths to calculate the address, i.e. multiplying constant percpu_offset by cpu_logical_map(cpu). However this assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1, but if CPU topology is specified via DT, this changes to full ID in the same format as MPIDR register and thus breaks the assumption.

This patch adds support for per CPU GIC bank offset specification through device tree to separate SoC-internal core wiring from CPU multi-processor IDs.

Signed-off-by Tomasz Figa <[email protected]>

---
 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 3 files changed, 105 insertions(+), 30 deletions(-)

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

index 333f4ae..47654e6 100644

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

 Example 1 (dual-cluster big.LITTLE system 32-bit):
 

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

index 5573c08..2bd03406 100644
@@ -48,7 +48,7 @@ Optional
 
 - cpu-offset   : per-cpu offset within the distributor and cpu interface
   regions, used when the GIC doesn't have banked registers. The offset is
-  cpu-offset * cpu-nr.
+  cpu-offset * cpu-nr. (DEPRECATED, see per-CPU 'gic-offset' property.)
 
 - arm,routable-irqs : Total number of gic irq inputs which are not directly
                  connected from the peripherals, but are routed dynamically

@@ -67,6 +67,38 @@ Example:
                      <0xfff10100 0x100>;
        };
 
+* Per-CPU register offset specification for non-banked GIC
+
+On most platforms GIC registers are banked, so each CPU can access its
+registers at the same address. However there is a small number of SoCs
+on which the banking is not implemented and each CPU has its GIC
+register set at different offset from GIC base address. These offsets
+need to be be provided from device tree, as described below.
+
+Optional properties in node of each CPU in the system:
+
+ - gic-offset : A single u32 value that needs to be added to GIC base
+   address to calculate address of GIC registers for that CPU.
+
+See [1] for more details about ARM CPU bindings.
+
+Example:
+
+       cpus {
+               /* ... */
+
+               cpu at a00 {
+                       /* ... */
+                       gic-offset = <0x0000>;
+               };
+
+               cpu at a01 {
+                       /* ... */
+                       gic-offset = <0x8000>;
+               };
+       };
+
+[1] Documentation/devicetree/bindings/arm/cpus.txt
 
 * GIC virtualization extensions (VGIC)
 

drivers/irqchip/irq-gic.c +65 -30

index 4300b66..ad6f4fe 100644
@@ -924,6 +924,69 @@ const struct irq_domain_ops gic_default_routable_irq_domain_ops = {
 const struct irq_domain_ops *gic_routable_irq_domain_ops =
                                        &gic_default_routable_irq_domain_ops;
 
+static int gic_setup_bases(struct gic_chip_data *gic, void __iomem *dist_base,
+                          void __iomem *cpu_base, u32 percpu_offset)
+{
+       bool use_cpu_nodes = true;
+       u32 offset;
+       unsigned int cpu;
+
+       for_each_possible_cpu(cpu) {
+               struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+               if (!cpu_node
+                   || of_property_read_u32(cpu_node, "gic-offset", &offset)) {
+                       use_cpu_nodes = false;
+                       break;
+               }
+       }
+
+       if (!(percpu_offset || use_cpu_nodes)
+           || !IS_ENABLED(CONFIG_GIC_NON_BANKED)) {
+               /* Normal, sane GIC... (or non-banked unsupported) */
+               WARN(percpu_offset || use_cpu_nodes,
+                    "GIC_NON_BANKED not enabled, ignoring %08x offset!",
+                    percpu_offset);
+
+               gic->dist_base.common_base = dist_base;
+               gic->cpu_base.common_base = cpu_base;
+               gic_set_base_accessor(gic, gic_get_common_base);
+
+               return 0;
+       }
+
+       /* Frankein-GIC without banked registers... */
+       gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
+       gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
+       if (WARN_ON(!gic->dist_base.percpu_base ||
+                       !gic->cpu_base.percpu_base)) {
+               free_percpu(gic->dist_base.percpu_base);
+               free_percpu(gic->cpu_base.percpu_base);
+
+               return -ENOMEM;
+       }
+
+       for_each_possible_cpu(cpu) {
+               if (use_cpu_nodes) {
+                       struct device_node *cpu_node =
+                                               of_get_cpu_node(cpu, NULL);
+
+                       of_property_read_u32(cpu_node, "gic-offset", &offset);
+               } else {
+                       offset = percpu_offset * cpu_logical_map(cpu);
+               }
+
+               *per_cpu_ptr(gic->dist_base.percpu_base, cpu) =
+                                                       dist_base + offset;
+               *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) =
+                                                       cpu_base + offset;
+       }
+
+       gic_set_base_accessor(gic, gic_get_percpu_base);
+
+       return 0;
+}
+
 void __init gic_init_bases(unsigned int gic_nr, int irq_start,
                           void __iomem *dist_base, void __iomem *cpu_base,
                           u32 percpu_offset, struct device_node *node)

@@ -936,36 +999,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
        BUG_ON(gic_nr >= MAX_GIC_NR);
 
        gic = &gic_data[gic_nr];
-#ifdef CONFIG_GIC_NON_BANKED
-       if (percpu_offset) { /* Frankein-GIC without banked registers... */
-               unsigned int cpu;
-
-               gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
-               gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
-               if (WARN_ON(!gic->dist_base.percpu_base ||
-                           !gic->cpu_base.percpu_base)) {
-                       free_percpu(gic->dist_base.percpu_base);
-                       free_percpu(gic->cpu_base.percpu_base);
-                       return;
-               }
 
-               for_each_possible_cpu(cpu) {
-                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
-                       *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
-                       *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
-               }
-
-               gic_set_base_accessor(gic, gic_get_percpu_base);
-       } else
-#endif
-       {                       /* Normal, sane GIC... */
-               WARN(percpu_offset,
-                    "GIC_NON_BANKED not enabled, ignoring %08x offset!",
-                    percpu_offset);
-               gic->dist_base.common_base = dist_base;
-               gic->cpu_base.common_base = cpu_base;
-               gic_set_base_accessor(gic, gic_get_common_base);
-       }
+       if (gic_setup_bases(gic, dist_base, cpu_base, percpu_offset))
+               return;
 
        /*
         * Initialize the CPU interface map to all CPUs.
--

1.9.2

Quoted 30 lines · Tomasz Figa

On most platforms GIC registers are banked, so each CPU can access its registers at the same address. However there is a small number of SoCs on which the banking is not implemented and each CPU has its GIC register set at different offset from GIC base address.

Originally the driver used simple maths to calculate the address, i.e. multiplying constant percpu_offset by cpu_logical_map(cpu). However this assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1, but if CPU topology is specified via DT, this changes to full ID in the same format as MPIDR register and thus breaks the assumption.

This patch adds support for per CPU GIC bank offset specification through device tree to separate SoC-internal core wiring from CPU multi-processor IDs.

Signed-off-by Tomasz Figa <[email protected]>

---
 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 3 files changed, 105 insertions(+), 30 deletions(-)

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

index 333f4ae..47654e6 100644
@@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
                Value type: <phandle>
                Definition: Specifies the ACC[2] node associated with this CPU.
 

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 218

Quoted 7 lines

Documentation/devicetree/bindings/arm/cpus.txt +7 -0

+       - gic-offset
+               Usage: required for systems that have non-banked GIC
+                      implementation that requires each CPU to use different
+                      offset to access its set of GIC registers
+               Value type: <u32>
+               Definition: Specifies the offset of GIC registers specific to
+                           this CPU.

What if you have 1 distributor address and a per cpu address which is allowed in the gicv2 spec IIRC.

I think I would rather see this stay contained within the gic node and use reg property.

Quoted 31 lines · Rob Herring

Quoted 30 lines · Tomasz Figa

On most platforms GIC registers are banked, so each CPU can access its registers at the same address. However there is a small number of SoCs on which the banking is not implemented and each CPU has its GIC register set at different offset from GIC base address.

Originally the driver used simple maths to calculate the address, i.e. multiplying constant percpu_offset by cpu_logical_map(cpu). However this assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1, but if CPU topology is specified via DT, this changes to full ID in the same format as MPIDR register and thus breaks the assumption.

This patch adds support for per CPU GIC bank offset specification through device tree to separate SoC-internal core wiring from CPU multi-processor IDs.

Signed-off-by Tomasz Figa <[email protected]>

---
 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 3 files changed, 105 insertions(+), 30 deletions(-)

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

index 333f4ae..47654e6 100644
@@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
                Value type: <phandle>
                Definition: Specifies the ACC[2] node associated with this CPU.
 

Quoted 10 lines

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 218

Quoted 7 lines

Documentation/devicetree/bindings/arm/cpus.txt +7 -0

+       - gic-offset
+               Usage: required for systems that have non-banked GIC
+                      implementation that requires each CPU to use different
+                      offset to access its set of GIC registers
+               Value type: <u32>
+               Definition: Specifies the offset of GIC registers specific to
+                           this CPU.

What if you have 1 distributor address and a per cpu address which is allowed in the gicv2 spec IIRC.

Hmm, I need to take a look at GIC v2 spec... but I think my proposed binding would still cover this, as the implementation (if modified to support this) would simply ignore the offset for distributor in this case.

Quoted 2 lines

I think I would rather see this stay contained within the gic node and use reg property.

How do we match reg entries with CPUs then? The first idea that comes to my mind would be adding arm,cpu-map property that would list MPIDR values of CPUs in the same order as register banks are listed in reg property but I'm not sure this is a good idea.

Quoted 43 lines · Tomasz Figa

Quoted 41 lines · Rob Herring

Quoted 27 lines · Tomasz Figa

On most platforms GIC registers are banked, so each CPU can access its registers at the same address. However there is a small number of SoCs on which the banking is not implemented and each CPU has its GIC register set at different offset from GIC base address.

Originally the driver used simple maths to calculate the address, i.e. multiplying constant percpu_offset by cpu_logical_map(cpu). However this assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1, but if CPU topology is specified via DT, this changes to full ID in the same format as MPIDR register and thus breaks the assumption.

This patch adds support for per CPU GIC bank offset specification through device tree to separate SoC-internal core wiring from CPU multi-processor IDs.

Signed-off-by Tomasz Figa <[email protected]>

---
 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 3 files changed, 105 insertions(+), 30 deletions(-)

Documentation/devicetree/bindings/arm/cpus.txt +0 -0

index 333f4ae..47654e6 100644

Documentation/devicetree/bindings/arm/cpus.txt nodes to be present and contain the properties described below. · line 218

Quoted 10 lines

Documentation/devicetree/bindings/arm/cpus.txt +7 -0

@@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
                Value type: <phandle>
                Definition: Specifies the ACC[2] node associated with this CPU.
 
+       - gic-offset
+               Usage: required for systems that have non-banked GIC
+                      implementation that requires each CPU to use different
+                      offset to access its set of GIC registers
+               Value type: <u32>
+               Definition: Specifies the offset of GIC registers specific to
+                           this CPU.

What if you have 1 distributor address and a per cpu address which is allowed in the gicv2 spec IIRC.

Quoted 3 lines

Hmm, I need to take a look at GIC v2 spec... but I think my proposed binding would still cover this, as the implementation (if modified to support this) would simply ignore the offset for distributor in this case.

How does the implementation know whether to ignore the offset for the distributor or not?

Quoted 7 lines

Quoted 2 lines

I think I would rather see this stay contained within the gic node and use reg property.

How do we match reg entries with CPUs then? The first idea that comes to my mind would be adding arm,cpu-map property that would list MPIDR values of CPUs in the same order as register banks are listed in reg property but I'm not sure this is a good idea.

How do you do it currently?

The issue with putting properties in the cpu node is it does not scale. What if you had 10 per cpu properties on 10 different blocks.

Using the MPIDR directly is a bit specific. I would perhaps just do something like this:

gic-cpu-map = <&CPU0 &CPU1>;

Due to some parts of Exynos SoC support designed originally in a non-scalable way, relying on 1:1 mapping between value returned by cpu_logical_map() and CPU IDs as seen by PMU and GIC, trying to specify CPU topology in device tree caused various boot-up issues on Exynos SoCs, ranging from CPUs other than 0 failing to boot to crashes due to GIC driver accessing registers out of range.

This series attempts to fix aforementioned issues by removing incorrect assumptions from Exynos SoC core code and GIC driver and then adding CPU topology data to device tree sources of Exynos4.

[On Exynos4210-based TRATS and Exynos4412-based TRATS2 board]

Tested-by Tomasz Figa <[email protected]>

Tomasz Figa (4):
ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers
ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
irqchip: gic: Add support for per CPU bank offset specification in DT
ARM: dts: exynos4: Add CPU topology data

 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 arch/arm/boot/dts/exynos4210.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4212.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4412.dtsi              | 37 ++++++++--
 arch/arm/mach-exynos/hotplug.c                 | 10 +--
 arch/arm/mach-exynos/platsmp.c                 | 31 +++++----
 arch/arm/mach-exynos/regs-pmu.h                |  4 +-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 9 files changed, 202 insertions(+), 53 deletions(-)

Due to some parts of Exynos SoC support designed originally in a non-scalable way, relying on 1:1 mapping between value returned by cpu_logical_map() and CPU IDs as seen by PMU and GIC, trying to specify CPU topology in device tree caused various boot-up issues on Exynos SoCs, ranging from CPUs other than 0 failing to boot to crashes due to GIC driver accessing registers out of range.

This series attempts to fix aforementioned issues by removing incorrect assumptions from Exynos SoC core code and GIC driver and then adding CPU topology data to device tree sources of Exynos4.

[On Exynos4210-based TRATS and Exynos4412-based TRATS2 board]

Tested-by Tomasz Figa <[email protected]>

Tomasz Figa (4):
ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers
ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
irqchip: gic: Add support for per CPU bank offset specification in DT
ARM: dts: exynos4: Add CPU topology data

 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 arch/arm/boot/dts/exynos4210.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4212.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4412.dtsi              | 37 ++++++++--
 arch/arm/mach-exynos/hotplug.c                 | 10 +--
 arch/arm/mach-exynos/platsmp.c                 | 31 +++++----
 arch/arm/mach-exynos/regs-pmu.h                |  4 +-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 9 files changed, 202 insertions(+), 53 deletions(-)