The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for CPU0. This is because for child GIC
controllers there is most likely only one recipient of the interrupt.
Fix this by moving the CPU mapping array to the GIC chip data structure
so that it is initialised for each GIC instance separately. It is assumed
that the bL switcher code is only interested in the root or primary GIC
instance.
Signed-off-by: Jon Hunter <[email protected]>
---
arch/arm/common/bL_switcher.c | 2 +-
drivers/irqchip/irq-gic.c | 41 ++++++++++++++++++++++-------------------
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c
index 37dc0fe1093f..4111d7b36077 100644
--- a/arch/arm/common/bL_switcher.c
+++ b/arch/arm/common/bL_switcher.c
@@ -488,7 +488,7 @@ static int bL_switcher_halve_cpus(void)
cluster = MPIDR_AFFINITY_LEVEL(cpu_logical_map(i), 1);
/* Let's take note of the GIC ID for this CPU */
- gic_id = gic_get_cpu_id(i);
+ gic_id = gic_get_cpu_id(i, 0);
if (gic_id < 0) {
pr_err("%s: bad GIC ID for CPU %d\n", __func__, i);
bL_switcher_restore_cpus();
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a9b810..78a7706c607e 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -50,6 +50,8 @@
#include "irq-gic-common.h"
+#define NR_GIC_CPU_IF 8
+
union gic_base {
void __iomem *common_base;
void __percpu * __iomem *percpu_base;
@@ -70,18 +72,16 @@ struct gic_chip_data {
#ifdef CONFIG_GIC_NON_BANKED
void __iomem *(*get_base)(union gic_base *);
#endif
+ /*
+ * 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.
+ */
+ u8 cpu_map[NR_GIC_CPU_IF];
};
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.
- */
-#define NR_GIC_CPU_IF 8
-static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
-
#ifndef MAX_GIC_NR
#define MAX_GIC_NR 1
#endif
@@ -237,6 +237,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
bool force)
{
+ struct gic_chip_data *gic = irq_data_get_irq_chip_data(d);
void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
u32 val, mask, bit;
@@ -252,7 +253,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
raw_spin_lock_irqsave(&irq_controller_lock, flags);
mask = 0xff << shift;
- bit = gic_cpu_map[cpu] << shift;
+ bit = gic->cpu_map[cpu] << shift;
val = readl_relaxed(reg) & ~mask;
writel_relaxed(val | bit, reg);
raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
@@ -420,7 +421,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
*/
BUG_ON(cpu >= NR_GIC_CPU_IF);
cpu_mask = gic_get_cpumask(gic);
- gic_cpu_map[cpu] = cpu_mask;
+ gic->cpu_map[cpu] = cpu_mask;
/*
* Clear our mask from the other map entries in case they're
@@ -428,7 +429,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
*/
for (i = 0; i < NR_GIC_CPU_IF; i++)
if (i != cpu)
- gic_cpu_map[i] &= ~cpu_mask;
+ gic->cpu_map[i] &= ~cpu_mask;
gic_cpu_config(dist_base, NULL);
@@ -677,7 +678,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
/* Convert our logical CPU mask into a physical one. */
for_each_cpu(cpu, mask)
- map |= gic_cpu_map[cpu];
+ map |= gic->cpu_map[cpu];
/*
* Ensure that stores to Normal memory are visible to the
@@ -722,15 +723,17 @@ void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
* or -1 if the CPU number is too large or the interface ID is
* unknown (more than one bit set).
*/
-int gic_get_cpu_id(unsigned int cpu)
+int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr)
{
unsigned int cpu_bit;
+ if (gic_nr >= MAX_GIC_NR)
+ return -EINVAL;
if (cpu >= NR_GIC_CPU_IF)
- return -1;
- cpu_bit = gic_cpu_map[cpu];
+ return -EINVAL;
+ cpu_bit = gic_data[gic_nr].cpu_map[cpu];
if (cpu_bit & (cpu_bit - 1))
- return -1;
+ return -EINVAL;
return __ffs(cpu_bit);
}
@@ -759,14 +762,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
return;
gic_irqs = gic_data[gic_nr].gic_irqs;
- cur_cpu_id = __ffs(gic_cpu_map[cpu]);
+ cur_cpu_id = __ffs(gic->cpu_map[cpu]);
cur_target_mask = 0x01010101 << cur_cpu_id;
ror_val = (cur_cpu_id - new_cpu_id) & 31;
raw_spin_lock(&irq_controller_lock);
/* Update the target interface for this logical CPU */
- gic_cpu_map[cpu] = 1 << new_cpu_id;
+ gic->cpu_map[cpu] = 1 << new_cpu_id;
/*
* Find all the peripheral interrupts targetting the current
@@ -981,7 +984,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
* It will be refined as each CPU probes its ID.
*/
for (i = 0; i < NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
+ gic->cpu_map[i] = 0xff;
/*
* Find out how many interrupts are supported.
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index f52a9024be9a..9a4d30564492 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -111,7 +111,7 @@ static inline void gic_init(unsigned int nr, int start,
int gicv2m_of_init(struct device_node *node, struct irq_domain *parent);
void gic_send_sgi(unsigned int cpu_id, unsigned int irq);
-int gic_get_cpu_id(unsigned int cpu);
+int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr);
void gic_migrate_target(unsigned int new_cpu_id);
unsigned long gic_get_sgir_physaddr(void);
--
2.1.4
On Wed, 29 Jul 2015, Jon Hunter wrote:
> The gic_init_bases() function initialises an array that stores the mapping
> between the GIC and CPUs. This array is a global array that is
> unconditionally initialised on every call to gic_init_bases(). Although,
> it is not common for there to be more than one GIC instance, there are
> some devices that do support nested GIC controllers and gic_init_bases()
> can be called more than once.
>
> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
> will only setup the mapping again for CPU0. This is because for child GIC
> controllers there is most likely only one recipient of the interrupt.
>
> Fix this by moving the CPU mapping array to the GIC chip data structure
> so that it is initialised for each GIC instance separately. It is assumed
> that the bL switcher code is only interested in the root or primary GIC
> instance.
>
> Signed-off-by: Jon Hunter <[email protected]>
Acked-by: Nicolas Pitre <[email protected]>
> ---
> arch/arm/common/bL_switcher.c | 2 +-
> drivers/irqchip/irq-gic.c | 41 ++++++++++++++++++++++-------------------
> include/linux/irqchip/arm-gic.h | 2 +-
> 3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c
> index 37dc0fe1093f..4111d7b36077 100644
> --- a/arch/arm/common/bL_switcher.c
> +++ b/arch/arm/common/bL_switcher.c
> @@ -488,7 +488,7 @@ static int bL_switcher_halve_cpus(void)
> cluster = MPIDR_AFFINITY_LEVEL(cpu_logical_map(i), 1);
>
> /* Let's take note of the GIC ID for this CPU */
> - gic_id = gic_get_cpu_id(i);
> + gic_id = gic_get_cpu_id(i, 0);
> if (gic_id < 0) {
> pr_err("%s: bad GIC ID for CPU %d\n", __func__, i);
> bL_switcher_restore_cpus();
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index a530d9a9b810..78a7706c607e 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -50,6 +50,8 @@
>
> #include "irq-gic-common.h"
>
> +#define NR_GIC_CPU_IF 8
> +
> union gic_base {
> void __iomem *common_base;
> void __percpu * __iomem *percpu_base;
> @@ -70,18 +72,16 @@ struct gic_chip_data {
> #ifdef CONFIG_GIC_NON_BANKED
> void __iomem *(*get_base)(union gic_base *);
> #endif
> + /*
> + * 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.
> + */
> + u8 cpu_map[NR_GIC_CPU_IF];
> };
>
> 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.
> - */
> -#define NR_GIC_CPU_IF 8
> -static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
> -
> #ifndef MAX_GIC_NR
> #define MAX_GIC_NR 1
> #endif
> @@ -237,6 +237,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> {
> + struct gic_chip_data *gic = irq_data_get_irq_chip_data(d);
> void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> u32 val, mask, bit;
> @@ -252,7 +253,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>
> raw_spin_lock_irqsave(&irq_controller_lock, flags);
> mask = 0xff << shift;
> - bit = gic_cpu_map[cpu] << shift;
> + bit = gic->cpu_map[cpu] << shift;
> val = readl_relaxed(reg) & ~mask;
> writel_relaxed(val | bit, reg);
> raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
> @@ -420,7 +421,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
> */
> BUG_ON(cpu >= NR_GIC_CPU_IF);
> cpu_mask = gic_get_cpumask(gic);
> - gic_cpu_map[cpu] = cpu_mask;
> + gic->cpu_map[cpu] = cpu_mask;
>
> /*
> * Clear our mask from the other map entries in case they're
> @@ -428,7 +429,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
> */
> for (i = 0; i < NR_GIC_CPU_IF; i++)
> if (i != cpu)
> - gic_cpu_map[i] &= ~cpu_mask;
> + gic->cpu_map[i] &= ~cpu_mask;
>
> gic_cpu_config(dist_base, NULL);
>
> @@ -677,7 +678,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>
> /* Convert our logical CPU mask into a physical one. */
> for_each_cpu(cpu, mask)
> - map |= gic_cpu_map[cpu];
> + map |= gic->cpu_map[cpu];
>
> /*
> * Ensure that stores to Normal memory are visible to the
> @@ -722,15 +723,17 @@ void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
> * or -1 if the CPU number is too large or the interface ID is
> * unknown (more than one bit set).
> */
> -int gic_get_cpu_id(unsigned int cpu)
> +int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr)
> {
> unsigned int cpu_bit;
>
> + if (gic_nr >= MAX_GIC_NR)
> + return -EINVAL;
> if (cpu >= NR_GIC_CPU_IF)
> - return -1;
> - cpu_bit = gic_cpu_map[cpu];
> + return -EINVAL;
> + cpu_bit = gic_data[gic_nr].cpu_map[cpu];
> if (cpu_bit & (cpu_bit - 1))
> - return -1;
> + return -EINVAL;
> return __ffs(cpu_bit);
> }
>
> @@ -759,14 +762,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
> return;
> gic_irqs = gic_data[gic_nr].gic_irqs;
>
> - cur_cpu_id = __ffs(gic_cpu_map[cpu]);
> + cur_cpu_id = __ffs(gic->cpu_map[cpu]);
> cur_target_mask = 0x01010101 << cur_cpu_id;
> ror_val = (cur_cpu_id - new_cpu_id) & 31;
>
> raw_spin_lock(&irq_controller_lock);
>
> /* Update the target interface for this logical CPU */
> - gic_cpu_map[cpu] = 1 << new_cpu_id;
> + gic->cpu_map[cpu] = 1 << new_cpu_id;
>
> /*
> * Find all the peripheral interrupts targetting the current
> @@ -981,7 +984,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> * It will be refined as each CPU probes its ID.
> */
> for (i = 0; i < NR_GIC_CPU_IF; i++)
> - gic_cpu_map[i] = 0xff;
> + gic->cpu_map[i] = 0xff;
>
> /*
> * Find out how many interrupts are supported.
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index f52a9024be9a..9a4d30564492 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -111,7 +111,7 @@ static inline void gic_init(unsigned int nr, int start,
> int gicv2m_of_init(struct device_node *node, struct irq_domain *parent);
>
> void gic_send_sgi(unsigned int cpu_id, unsigned int irq);
> -int gic_get_cpu_id(unsigned int cpu);
> +int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr);
> void gic_migrate_target(unsigned int new_cpu_id);
> unsigned long gic_get_sgir_physaddr(void);
>
> --
> 2.1.4
>
>
On Wed, 29 Jul 2015, Jon Hunter wrote:
Cc'ing Marc ...
> The gic_init_bases() function initialises an array that stores the mapping
> between the GIC and CPUs. This array is a global array that is
> unconditionally initialised on every call to gic_init_bases(). Although,
> it is not common for there to be more than one GIC instance, there are
> some devices that do support nested GIC controllers and gic_init_bases()
> can be called more than once.
>
> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
> will only setup the mapping again for CPU0. This is because for child GIC
> controllers there is most likely only one recipient of the interrupt.
>
> Fix this by moving the CPU mapping array to the GIC chip data structure
> so that it is initialised for each GIC instance separately. It is assumed
> that the bL switcher code is only interested in the root or primary GIC
> instance.
>
> Signed-off-by: Jon Hunter <[email protected]>
> ---
> arch/arm/common/bL_switcher.c | 2 +-
> drivers/irqchip/irq-gic.c | 41 ++++++++++++++++++++++-------------------
> include/linux/irqchip/arm-gic.h | 2 +-
> 3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c
> index 37dc0fe1093f..4111d7b36077 100644
> --- a/arch/arm/common/bL_switcher.c
> +++ b/arch/arm/common/bL_switcher.c
> @@ -488,7 +488,7 @@ static int bL_switcher_halve_cpus(void)
> cluster = MPIDR_AFFINITY_LEVEL(cpu_logical_map(i), 1);
>
> /* Let's take note of the GIC ID for this CPU */
> - gic_id = gic_get_cpu_id(i);
> + gic_id = gic_get_cpu_id(i, 0);
> if (gic_id < 0) {
> pr_err("%s: bad GIC ID for CPU %d\n", __func__, i);
> bL_switcher_restore_cpus();
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index a530d9a9b810..78a7706c607e 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -50,6 +50,8 @@
>
> #include "irq-gic-common.h"
>
> +#define NR_GIC_CPU_IF 8
> +
> union gic_base {
> void __iomem *common_base;
> void __percpu * __iomem *percpu_base;
> @@ -70,18 +72,16 @@ struct gic_chip_data {
> #ifdef CONFIG_GIC_NON_BANKED
> void __iomem *(*get_base)(union gic_base *);
> #endif
> + /*
> + * 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.
> + */
> + u8 cpu_map[NR_GIC_CPU_IF];
> };
>
> 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.
> - */
> -#define NR_GIC_CPU_IF 8
> -static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
> -
> #ifndef MAX_GIC_NR
> #define MAX_GIC_NR 1
> #endif
> @@ -237,6 +237,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> {
> + struct gic_chip_data *gic = irq_data_get_irq_chip_data(d);
> void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> u32 val, mask, bit;
> @@ -252,7 +253,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>
> raw_spin_lock_irqsave(&irq_controller_lock, flags);
> mask = 0xff << shift;
> - bit = gic_cpu_map[cpu] << shift;
> + bit = gic->cpu_map[cpu] << shift;
> val = readl_relaxed(reg) & ~mask;
> writel_relaxed(val | bit, reg);
> raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
> @@ -420,7 +421,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
> */
> BUG_ON(cpu >= NR_GIC_CPU_IF);
> cpu_mask = gic_get_cpumask(gic);
> - gic_cpu_map[cpu] = cpu_mask;
> + gic->cpu_map[cpu] = cpu_mask;
>
> /*
> * Clear our mask from the other map entries in case they're
> @@ -428,7 +429,7 @@ static void gic_cpu_init(struct gic_chip_data *gic)
> */
> for (i = 0; i < NR_GIC_CPU_IF; i++)
> if (i != cpu)
> - gic_cpu_map[i] &= ~cpu_mask;
> + gic->cpu_map[i] &= ~cpu_mask;
>
> gic_cpu_config(dist_base, NULL);
>
> @@ -677,7 +678,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>
> /* Convert our logical CPU mask into a physical one. */
> for_each_cpu(cpu, mask)
> - map |= gic_cpu_map[cpu];
> + map |= gic->cpu_map[cpu];
>
> /*
> * Ensure that stores to Normal memory are visible to the
> @@ -722,15 +723,17 @@ void gic_send_sgi(unsigned int cpu_id, unsigned int irq)
> * or -1 if the CPU number is too large or the interface ID is
> * unknown (more than one bit set).
> */
> -int gic_get_cpu_id(unsigned int cpu)
> +int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr)
> {
> unsigned int cpu_bit;
>
> + if (gic_nr >= MAX_GIC_NR)
> + return -EINVAL;
> if (cpu >= NR_GIC_CPU_IF)
> - return -1;
> - cpu_bit = gic_cpu_map[cpu];
> + return -EINVAL;
> + cpu_bit = gic_data[gic_nr].cpu_map[cpu];
> if (cpu_bit & (cpu_bit - 1))
> - return -1;
> + return -EINVAL;
> return __ffs(cpu_bit);
> }
>
> @@ -759,14 +762,14 @@ void gic_migrate_target(unsigned int new_cpu_id)
> return;
> gic_irqs = gic_data[gic_nr].gic_irqs;
>
> - cur_cpu_id = __ffs(gic_cpu_map[cpu]);
> + cur_cpu_id = __ffs(gic->cpu_map[cpu]);
> cur_target_mask = 0x01010101 << cur_cpu_id;
> ror_val = (cur_cpu_id - new_cpu_id) & 31;
>
> raw_spin_lock(&irq_controller_lock);
>
> /* Update the target interface for this logical CPU */
> - gic_cpu_map[cpu] = 1 << new_cpu_id;
> + gic->cpu_map[cpu] = 1 << new_cpu_id;
>
> /*
> * Find all the peripheral interrupts targetting the current
> @@ -981,7 +984,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> * It will be refined as each CPU probes its ID.
> */
> for (i = 0; i < NR_GIC_CPU_IF; i++)
> - gic_cpu_map[i] = 0xff;
> + gic->cpu_map[i] = 0xff;
>
> /*
> * Find out how many interrupts are supported.
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index f52a9024be9a..9a4d30564492 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -111,7 +111,7 @@ static inline void gic_init(unsigned int nr, int start,
> int gicv2m_of_init(struct device_node *node, struct irq_domain *parent);
>
> void gic_send_sgi(unsigned int cpu_id, unsigned int irq);
> -int gic_get_cpu_id(unsigned int cpu);
> +int gic_get_cpu_id(unsigned int cpu, unsigned int gic_nr);
> void gic_migrate_target(unsigned int new_cpu_id);
> unsigned long gic_get_sgir_physaddr(void);
>
> --
> 2.1.4
>
>
On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
> The gic_init_bases() function initialises an array that stores the mapping
> between the GIC and CPUs. This array is a global array that is
> unconditionally initialised on every call to gic_init_bases(). Although,
> it is not common for there to be more than one GIC instance, there are
> some devices that do support nested GIC controllers and gic_init_bases()
> can be called more than once.
>
> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
> will only setup the mapping again for CPU0. This is because for child GIC
> controllers there is most likely only one recipient of the interrupt.
>
> Fix this by moving the CPU mapping array to the GIC chip data structure
> so that it is initialised for each GIC instance separately. It is assumed
> that the bL switcher code is only interested in the root or primary GIC
> instance.
Does it make sense to expose the per-CPU-ness of the non-primary GIC?
If they are chained off a primary GIC SPI interrupt, then all IRQs on
the secondary GIC are routed to the same CPU that the SPI on the primary
GIC is routed to.
Other features like the PPIs and SGIs in the secondary CPU should also
be ignored - they probably aren't used anyway.
I have to say though... are the 1020 IRQs that the primary GIC provides
really not enough? What insane hardware needs more than 1020 IRQs?
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On 29/07/15 19:33, Russell King - ARM Linux wrote:
> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>> The gic_init_bases() function initialises an array that stores the mapping
>> between the GIC and CPUs. This array is a global array that is
>> unconditionally initialised on every call to gic_init_bases(). Although,
>> it is not common for there to be more than one GIC instance, there are
>> some devices that do support nested GIC controllers and gic_init_bases()
>> can be called more than once.
>>
>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>> will only setup the mapping again for CPU0. This is because for child GIC
>> controllers there is most likely only one recipient of the interrupt.
>>
>> Fix this by moving the CPU mapping array to the GIC chip data structure
>> so that it is initialised for each GIC instance separately. It is assumed
>> that the bL switcher code is only interested in the root or primary GIC
>> instance.
>
> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
> If they are chained off a primary GIC SPI interrupt, then all IRQs on
> the secondary GIC are routed to the same CPU that the SPI on the primary
> GIC is routed to.
I am looking at a use-case where there is a secondary GIC and the secondary
GIC is used as a interrupt router between the main CPU cluster and another
CPU. So in this case the mapping of a secondary is still of interest. This
patch does not address setting up the secondary mapping, but avoids a
secondary GIC overwriting the primary map (which we don't want).
> Other features like the PPIs and SGIs in the secondary CPU should also
> be ignored - they probably aren't used anyway.
Yes, agree.
> I have to say though... are the 1020 IRQs that the primary GIC provides
> really not enough? What insane hardware needs more than 1020 IRQs?
Ha. I guess some realview boards for a start ...
# grep -r "gic_init(1" arch/arm/
arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
Jon
On Wed, 29 Jul 2015 17:10:45 +0100
Thomas Gleixner <[email protected]> wrote:
> On Wed, 29 Jul 2015, Jon Hunter wrote:
>
> Cc'ing Marc ...
Thanks for looping me in.
>
> > The gic_init_bases() function initialises an array that stores the mapping
> > between the GIC and CPUs. This array is a global array that is
> > unconditionally initialised on every call to gic_init_bases(). Although,
> > it is not common for there to be more than one GIC instance, there are
> > some devices that do support nested GIC controllers and gic_init_bases()
> > can be called more than once.
> >
> > A 2nd call to gic_init_bases() will clear the previous CPU mapping and
> > will only setup the mapping again for CPU0. This is because for child GIC
> > controllers there is most likely only one recipient of the interrupt.
> >
> > Fix this by moving the CPU mapping array to the GIC chip data structure
> > so that it is initialised for each GIC instance separately. It is assumed
> > that the bL switcher code is only interested in the root or primary GIC
> > instance.
This feels very odd. If you have a secondary GIC, it is cascaded into
the primary one, and I don't see why you would need to manage the
affinity of the interrupt for the secondary GIC. The only thing that
matters is the affinity of interrupts in the primary one, and this is
what the bl switcher is dealing with.
To me, it looks like the bug is to even try to compute an affinity for
a GIC that is not the primary one, and keeping it around doesn't
seem to make much sense.
Or am I overlooking something?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 30/07/15 08:19, Marc Zyngier wrote:
> On Wed, 29 Jul 2015 17:10:45 +0100
> Thomas Gleixner <[email protected]> wrote:
>
>> On Wed, 29 Jul 2015, Jon Hunter wrote:
>>
>> Cc'ing Marc ...
>
> Thanks for looping me in.
>
>>
>>> The gic_init_bases() function initialises an array that stores the mapping
>>> between the GIC and CPUs. This array is a global array that is
>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>> it is not common for there to be more than one GIC instance, there are
>>> some devices that do support nested GIC controllers and gic_init_bases()
>>> can be called more than once.
>>>
>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>> will only setup the mapping again for CPU0. This is because for child GIC
>>> controllers there is most likely only one recipient of the interrupt.
>>>
>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>> so that it is initialised for each GIC instance separately. It is assumed
>>> that the bL switcher code is only interested in the root or primary GIC
>>> instance.
>
> This feels very odd. If you have a secondary GIC, it is cascaded into
> the primary one, and I don't see why you would need to manage the
> affinity of the interrupt for the secondary GIC. The only thing that
> matters is the affinity of interrupts in the primary one, and this is
> what the bl switcher is dealing with.
>
> To me, it looks like the bug is to even try to compute an affinity for
> a GIC that is not the primary one, and keeping it around doesn't
> seem to make much sense.
>
> Or am I overlooking something?
I mentioned to Russell (sorry you were not copied), that I am looking at
a use-case where the secondary GIC is actually a router that can route
interrupts to the main CPU cluster (via the primary GIC) or to another
CPU. So it turns out that I do see a use for being able to configure the
cpu map for the secondary as well. In this case the set_affinity would
be use to direct the interrupt to either the main cluster or the other CPU.
Please note that this patch does not address configuring the map for the
secondary GIC. I am trying to think about the best way to handle this. I
guess it could done via device-tree or we could have a API,
gic_set_cpu_map(), that would allow you to set it.
Cheers
Jon
On 29/07/15 20:27, Jon Hunter wrote:
>
> On 29/07/15 19:33, Russell King - ARM Linux wrote:
>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>>> The gic_init_bases() function initialises an array that stores the mapping
>>> between the GIC and CPUs. This array is a global array that is
>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>> it is not common for there to be more than one GIC instance, there are
>>> some devices that do support nested GIC controllers and gic_init_bases()
>>> can be called more than once.
>>>
>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>> will only setup the mapping again for CPU0. This is because for child GIC
>>> controllers there is most likely only one recipient of the interrupt.
>>>
>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>> so that it is initialised for each GIC instance separately. It is assumed
>>> that the bL switcher code is only interested in the root or primary GIC
>>> instance.
>>
>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
>> the secondary GIC are routed to the same CPU that the SPI on the primary
>> GIC is routed to.
>
> I am looking at a use-case where there is a secondary GIC and the secondary
> GIC is used as a interrupt router between the main CPU cluster and another
> CPU. So in this case the mapping of a secondary is still of interest. This
> patch does not address setting up the secondary mapping, but avoids a
> secondary GIC overwriting the primary map (which we don't want).
>
>> Other features like the PPIs and SGIs in the secondary CPU should also
>> be ignored - they probably aren't used anyway.
>
> Yes, agree.
>
>> I have to say though... are the 1020 IRQs that the primary GIC provides
>> really not enough? What insane hardware needs more than 1020 IRQs?
>
> Ha. I guess some realview boards for a start ...
>
> # grep -r "gic_init(1" arch/arm/
> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
Different use case. These are development boards with a relatively
modular design, where the SoC may or may not have a GIC by itself. When
it has one, you end up with the on-board GIC being a secondary one.
I never thought someone would quote these designs as an example to
follow... ;-)
M.
--
Jazz is not dead. It just smells funny...
On 30/07/15 09:20, Marc Zyngier wrote:
> On 29/07/15 20:27, Jon Hunter wrote:
>>
>> On 29/07/15 19:33, Russell King - ARM Linux wrote:
>>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>>>> The gic_init_bases() function initialises an array that stores the mapping
>>>> between the GIC and CPUs. This array is a global array that is
>>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>>> it is not common for there to be more than one GIC instance, there are
>>>> some devices that do support nested GIC controllers and gic_init_bases()
>>>> can be called more than once.
>>>>
>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>>> will only setup the mapping again for CPU0. This is because for child GIC
>>>> controllers there is most likely only one recipient of the interrupt.
>>>>
>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>>> so that it is initialised for each GIC instance separately. It is assumed
>>>> that the bL switcher code is only interested in the root or primary GIC
>>>> instance.
>>>
>>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
>>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
>>> the secondary GIC are routed to the same CPU that the SPI on the primary
>>> GIC is routed to.
>>
>> I am looking at a use-case where there is a secondary GIC and the secondary
>> GIC is used as a interrupt router between the main CPU cluster and another
>> CPU. So in this case the mapping of a secondary is still of interest. This
>> patch does not address setting up the secondary mapping, but avoids a
>> secondary GIC overwriting the primary map (which we don't want).
>>
>>> Other features like the PPIs and SGIs in the secondary CPU should also
>>> be ignored - they probably aren't used anyway.
>>
>> Yes, agree.
>>
>>> I have to say though... are the 1020 IRQs that the primary GIC provides
>>> really not enough? What insane hardware needs more than 1020 IRQs?
>>
>> Ha. I guess some realview boards for a start ...
>>
>> # grep -r "gic_init(1" arch/arm/
>> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
>> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
>> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
>
> Different use case. These are development boards with a relatively
> modular design, where the SoC may or may not have a GIC by itself. When
> it has one, you end up with the on-board GIC being a secondary one.
Yes, I understand that the use-case may be different, but I highlighted
this as a use where the primary map would be overwritten as the driver
is today.
> I never thought someone would quote these designs as an example to
> follow... ;-)
I can't say if this example was followed, but nevertheless hardware
designers certainly do get creative ;-)
So we need to ensure the primary cpu map does not get overwritten by a
secondary and I have a case where the secondary map is of interest. So
are ok with this?
Cheers
Jon
On 30/07/15 09:08, Jon Hunter wrote:
>
> On 30/07/15 08:19, Marc Zyngier wrote:
>> On Wed, 29 Jul 2015 17:10:45 +0100
>> Thomas Gleixner <[email protected]> wrote:
>>
>>> On Wed, 29 Jul 2015, Jon Hunter wrote:
>>>
>>> Cc'ing Marc ...
>>
>> Thanks for looping me in.
>>
>>>
>>>> The gic_init_bases() function initialises an array that stores the mapping
>>>> between the GIC and CPUs. This array is a global array that is
>>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>>> it is not common for there to be more than one GIC instance, there are
>>>> some devices that do support nested GIC controllers and gic_init_bases()
>>>> can be called more than once.
>>>>
>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>>> will only setup the mapping again for CPU0. This is because for child GIC
>>>> controllers there is most likely only one recipient of the interrupt.
>>>>
>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>>> so that it is initialised for each GIC instance separately. It is assumed
>>>> that the bL switcher code is only interested in the root or primary GIC
>>>> instance.
>>
>> This feels very odd. If you have a secondary GIC, it is cascaded into
>> the primary one, and I don't see why you would need to manage the
>> affinity of the interrupt for the secondary GIC. The only thing that
>> matters is the affinity of interrupts in the primary one, and this is
>> what the bl switcher is dealing with.
>>
>> To me, it looks like the bug is to even try to compute an affinity for
>> a GIC that is not the primary one, and keeping it around doesn't
>> seem to make much sense.
>>
>> Or am I overlooking something?
>
> I mentioned to Russell (sorry you were not copied), that I am looking at
> a use-case where the secondary GIC is actually a router that can route
> interrupts to the main CPU cluster (via the primary GIC) or to another
> CPU. So it turns out that I do see a use for being able to configure the
> cpu map for the secondary as well. In this case the set_affinity would
> be use to direct the interrupt to either the main cluster or the other CPU.
I'm afraid that's not exactly the same thing. This data structure maps
Linux view of the CPU number to the number of the CPU interface. What
you're describing is being able to route the interrupts to an entity
that is not under the kernel's control. I don't think you should
shoehorn the two concept together.
> Please note that this patch does not address configuring the map for the
> secondary GIC. I am trying to think about the best way to handle this. I
> guess it could done via device-tree or we could have a API,
> gic_set_cpu_map(), that would allow you to set it.
This is starting to look a lot like the Freescale Vybrid stuff, where
they share devices between two unrelated ARM CPUs, each running their
own OS.
You should sync up with these guys.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 30/07/15 09:33, Jon Hunter wrote:
>
> On 30/07/15 09:20, Marc Zyngier wrote:
>> On 29/07/15 20:27, Jon Hunter wrote:
>>>
>>> On 29/07/15 19:33, Russell King - ARM Linux wrote:
>>>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>>>>> The gic_init_bases() function initialises an array that stores the mapping
>>>>> between the GIC and CPUs. This array is a global array that is
>>>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>>>> it is not common for there to be more than one GIC instance, there are
>>>>> some devices that do support nested GIC controllers and gic_init_bases()
>>>>> can be called more than once.
>>>>>
>>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>>>> will only setup the mapping again for CPU0. This is because for child GIC
>>>>> controllers there is most likely only one recipient of the interrupt.
>>>>>
>>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>>>> so that it is initialised for each GIC instance separately. It is assumed
>>>>> that the bL switcher code is only interested in the root or primary GIC
>>>>> instance.
>>>>
>>>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
>>>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
>>>> the secondary GIC are routed to the same CPU that the SPI on the primary
>>>> GIC is routed to.
>>>
>>> I am looking at a use-case where there is a secondary GIC and the secondary
>>> GIC is used as a interrupt router between the main CPU cluster and another
>>> CPU. So in this case the mapping of a secondary is still of interest. This
>>> patch does not address setting up the secondary mapping, but avoids a
>>> secondary GIC overwriting the primary map (which we don't want).
>>>
>>>> Other features like the PPIs and SGIs in the secondary CPU should also
>>>> be ignored - they probably aren't used anyway.
>>>
>>> Yes, agree.
>>>
>>>> I have to say though... are the 1020 IRQs that the primary GIC provides
>>>> really not enough? What insane hardware needs more than 1020 IRQs?
>>>
>>> Ha. I guess some realview boards for a start ...
>>>
>>> # grep -r "gic_init(1" arch/arm/
>>> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
>>> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
>>> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
>>
>> Different use case. These are development boards with a relatively
>> modular design, where the SoC may or may not have a GIC by itself. When
>> it has one, you end up with the on-board GIC being a secondary one.
>
> Yes, I understand that the use-case may be different, but I highlighted
> this as a use where the primary map would be overwritten as the driver
> is today.
>
>> I never thought someone would quote these designs as an example to
>> follow... ;-)
>
> I can't say if this example was followed, but nevertheless hardware
> designers certainly do get creative ;-)
>
> So we need to ensure the primary cpu map does not get overwritten by a
> secondary and I have a case where the secondary map is of interest. So
> are ok with this?
My point is that there is no secondary map. That map should only be
written for the primary GIC, because that's the only place it makes
sense. Your "other CPU" is not under the control of Linux (at least, not
as a CPU), so this map is not the right data structure.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 30/07/15 10:04, Marc Zyngier wrote:
> On 30/07/15 09:33, Jon Hunter wrote:
>>
>> On 30/07/15 09:20, Marc Zyngier wrote:
>>> On 29/07/15 20:27, Jon Hunter wrote:
>>>>
>>>> On 29/07/15 19:33, Russell King - ARM Linux wrote:
>>>>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>>>>>> The gic_init_bases() function initialises an array that stores the mapping
>>>>>> between the GIC and CPUs. This array is a global array that is
>>>>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>>>>> it is not common for there to be more than one GIC instance, there are
>>>>>> some devices that do support nested GIC controllers and gic_init_bases()
>>>>>> can be called more than once.
>>>>>>
>>>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>>>>> will only setup the mapping again for CPU0. This is because for child GIC
>>>>>> controllers there is most likely only one recipient of the interrupt.
>>>>>>
>>>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>>>>> so that it is initialised for each GIC instance separately. It is assumed
>>>>>> that the bL switcher code is only interested in the root or primary GIC
>>>>>> instance.
>>>>>
>>>>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
>>>>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
>>>>> the secondary GIC are routed to the same CPU that the SPI on the primary
>>>>> GIC is routed to.
>>>>
>>>> I am looking at a use-case where there is a secondary GIC and the secondary
>>>> GIC is used as a interrupt router between the main CPU cluster and another
>>>> CPU. So in this case the mapping of a secondary is still of interest. This
>>>> patch does not address setting up the secondary mapping, but avoids a
>>>> secondary GIC overwriting the primary map (which we don't want).
>>>>
>>>>> Other features like the PPIs and SGIs in the secondary CPU should also
>>>>> be ignored - they probably aren't used anyway.
>>>>
>>>> Yes, agree.
>>>>
>>>>> I have to say though... are the 1020 IRQs that the primary GIC provides
>>>>> really not enough? What insane hardware needs more than 1020 IRQs?
>>>>
>>>> Ha. I guess some realview boards for a start ...
>>>>
>>>> # grep -r "gic_init(1" arch/arm/
>>>> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
>>>> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
>>>> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
>>>
>>> Different use case. These are development boards with a relatively
>>> modular design, where the SoC may or may not have a GIC by itself. When
>>> it has one, you end up with the on-board GIC being a secondary one.
>>
>> Yes, I understand that the use-case may be different, but I highlighted
>> this as a use where the primary map would be overwritten as the driver
>> is today.
>>
>>> I never thought someone would quote these designs as an example to
>>> follow... ;-)
>>
>> I can't say if this example was followed, but nevertheless hardware
>> designers certainly do get creative ;-)
>>
>> So we need to ensure the primary cpu map does not get overwritten by a
>> secondary and I have a case where the secondary map is of interest. So
>> are ok with this?
>
> My point is that there is no secondary map. That map should only be
> written for the primary GIC, because that's the only place it makes
> sense. Your "other CPU" is not under the control of Linux (at least, not
> as a CPU), so this map is not the right data structure.
Yes the "other CPU" may not run Linux, but it is certainly under the
control of Linux as a slave CPU. Linux will decide whether it wants to
receive the interrupts from this GIC or route them to the other CPU.
Yes, I see that this may not be technically a cpu map and may be that is
a mis-use. Following that I assume that using set_affinity here would
also not be correct and a custom API should be employed?
If that is the case, then may be I should just fix up the irq-gic driver
to only set the cpu map for the primary?
Cheers Jon
On 30/07/15 10:30, Jon Hunter wrote:
>
> On 30/07/15 10:04, Marc Zyngier wrote:
>> On 30/07/15 09:33, Jon Hunter wrote:
>>>
>>> On 30/07/15 09:20, Marc Zyngier wrote:
>>>> On 29/07/15 20:27, Jon Hunter wrote:
>>>>>
>>>>> On 29/07/15 19:33, Russell King - ARM Linux wrote:
>>>>>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
>>>>>>> The gic_init_bases() function initialises an array that stores the mapping
>>>>>>> between the GIC and CPUs. This array is a global array that is
>>>>>>> unconditionally initialised on every call to gic_init_bases(). Although,
>>>>>>> it is not common for there to be more than one GIC instance, there are
>>>>>>> some devices that do support nested GIC controllers and gic_init_bases()
>>>>>>> can be called more than once.
>>>>>>>
>>>>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
>>>>>>> will only setup the mapping again for CPU0. This is because for child GIC
>>>>>>> controllers there is most likely only one recipient of the interrupt.
>>>>>>>
>>>>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
>>>>>>> so that it is initialised for each GIC instance separately. It is assumed
>>>>>>> that the bL switcher code is only interested in the root or primary GIC
>>>>>>> instance.
>>>>>>
>>>>>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
>>>>>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
>>>>>> the secondary GIC are routed to the same CPU that the SPI on the primary
>>>>>> GIC is routed to.
>>>>>
>>>>> I am looking at a use-case where there is a secondary GIC and the secondary
>>>>> GIC is used as a interrupt router between the main CPU cluster and another
>>>>> CPU. So in this case the mapping of a secondary is still of interest. This
>>>>> patch does not address setting up the secondary mapping, but avoids a
>>>>> secondary GIC overwriting the primary map (which we don't want).
>>>>>
>>>>>> Other features like the PPIs and SGIs in the secondary CPU should also
>>>>>> be ignored - they probably aren't used anyway.
>>>>>
>>>>> Yes, agree.
>>>>>
>>>>>> I have to say though... are the 1020 IRQs that the primary GIC provides
>>>>>> really not enough? What insane hardware needs more than 1020 IRQs?
>>>>>
>>>>> Ha. I guess some realview boards for a start ...
>>>>>
>>>>> # grep -r "gic_init(1" arch/arm/
>>>>> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
>>>>> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
>>>>> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
>>>>
>>>> Different use case. These are development boards with a relatively
>>>> modular design, where the SoC may or may not have a GIC by itself. When
>>>> it has one, you end up with the on-board GIC being a secondary one.
>>>
>>> Yes, I understand that the use-case may be different, but I highlighted
>>> this as a use where the primary map would be overwritten as the driver
>>> is today.
>>>
>>>> I never thought someone would quote these designs as an example to
>>>> follow... ;-)
>>>
>>> I can't say if this example was followed, but nevertheless hardware
>>> designers certainly do get creative ;-)
>>>
>>> So we need to ensure the primary cpu map does not get overwritten by a
>>> secondary and I have a case where the secondary map is of interest. So
>>> are ok with this?
>>
>> My point is that there is no secondary map. That map should only be
>> written for the primary GIC, because that's the only place it makes
>> sense. Your "other CPU" is not under the control of Linux (at least, not
>> as a CPU), so this map is not the right data structure.
>
> Yes the "other CPU" may not run Linux, but it is certainly under the
> control of Linux as a slave CPU. Linux will decide whether it wants to
> receive the interrupts from this GIC or route them to the other CPU.
>
> Yes, I see that this may not be technically a cpu map and may be that is
> a mis-use. Following that I assume that using set_affinity here would
> also not be correct and a custom API should be employed?
Indeed. set_affinity is only concerned with delivering interrupts to the
CPUs, and not to other entities. It looks like we need an different API,
but I would refrain from declaring it "custom". There is at least one
other example of a platform doing similar things (vybrid), and I'd like
to see some collaboration on that (Stefan on CC).
> If that is the case, then may be I should just fix up the irq-gic driver
> to only set the cpu map for the primary?
Yes please.
M.
--
Jazz is not dead. It just smells funny...
On 2015-07-30 11:38, Marc Zyngier wrote:
> On 30/07/15 10:30, Jon Hunter wrote:
>> Yes the "other CPU" may not run Linux, but it is certainly under the
>> control of Linux as a slave CPU. Linux will decide whether it wants to
>> receive the interrupts from this GIC or route them to the other CPU.
>>
>> Yes, I see that this may not be technically a cpu map and may be that is
>> a mis-use. Following that I assume that using set_affinity here would
>> also not be correct and a custom API should be employed?
>
> Indeed. set_affinity is only concerned with delivering interrupts to the
> CPUs, and not to other entities. It looks like we need an different API,
> but I would refrain from declaring it "custom". There is at least one
> other example of a platform doing similar things (vybrid), and I'd like
> to see some collaboration on that (Stefan on CC).
The Vybrid SoC allows to assign CPU "complex" affinity through something
called interrupt router (which is part of a miscellaneous module, hence
the driver is called vf610-mscm-ir). Typically on the secondary core
(Cortex-M4) a RTOS has been used. So far each CPU complex configured the
affinity of the interrupts to the own CPU on interrupt enable (e.g.
under Linux handled the vf610-mscm-ir driver). In this mode, the whole
router is somewhat pointless since you could also only mask/unmask your
local interrupt controller. Either way, this can lead to conflicts,
hence it is up to the developer to configure the operating systems
peripheral access (and hence interrupts) orthogonal.
The question in those heterogeneous architectures is who should be
responsible for resource control? The vf610-mscm-ir driver only cares
about the local CPU, so no "resource control" for the other CPU complex.
I guess from a security standpoint one should be the master and restrict
access of the other CPU complex as much as possible. On Vybrid, there
are only limited options in that regard, and access to the interrupt
router could not be disabled without disabling other peripherals too.
Newer heterogeneous Freescale SoCs (e.g. i.MX 7) don't have this
interrupt router at all. However, there is now a Resource Domain
Controller which allow to restrict access of memory regions
(peripherals..). However, afaik there is no control over interrupts
there, so only the local interrupt controller which allow to
enable/disable the shared peripheral interrupts.
--
Stefan
On Thu, 30 Jul 2015, Marc Zyngier wrote:
> On 30/07/15 09:33, Jon Hunter wrote:
> >
> > On 30/07/15 09:20, Marc Zyngier wrote:
> >> On 29/07/15 20:27, Jon Hunter wrote:
> >>>
> >>> On 29/07/15 19:33, Russell King - ARM Linux wrote:
> >>>> On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote:
> >>>>> The gic_init_bases() function initialises an array that stores the mapping
> >>>>> between the GIC and CPUs. This array is a global array that is
> >>>>> unconditionally initialised on every call to gic_init_bases(). Although,
> >>>>> it is not common for there to be more than one GIC instance, there are
> >>>>> some devices that do support nested GIC controllers and gic_init_bases()
> >>>>> can be called more than once.
> >>>>>
> >>>>> A 2nd call to gic_init_bases() will clear the previous CPU mapping and
> >>>>> will only setup the mapping again for CPU0. This is because for child GIC
> >>>>> controllers there is most likely only one recipient of the interrupt.
> >>>>>
> >>>>> Fix this by moving the CPU mapping array to the GIC chip data structure
> >>>>> so that it is initialised for each GIC instance separately. It is assumed
> >>>>> that the bL switcher code is only interested in the root or primary GIC
> >>>>> instance.
> >>>>
> >>>> Does it make sense to expose the per-CPU-ness of the non-primary GIC?
> >>>> If they are chained off a primary GIC SPI interrupt, then all IRQs on
> >>>> the secondary GIC are routed to the same CPU that the SPI on the primary
> >>>> GIC is routed to.
> >>>
> >>> I am looking at a use-case where there is a secondary GIC and the secondary
> >>> GIC is used as a interrupt router between the main CPU cluster and another
> >>> CPU. So in this case the mapping of a secondary is still of interest. This
> >>> patch does not address setting up the secondary mapping, but avoids a
> >>> secondary GIC overwriting the primary map (which we don't want).
> >>>
> >>>> Other features like the PPIs and SGIs in the secondary CPU should also
> >>>> be ignored - they probably aren't used anyway.
> >>>
> >>> Yes, agree.
> >>>
> >>>> I have to say though... are the 1020 IRQs that the primary GIC provides
> >>>> really not enough? What insane hardware needs more than 1020 IRQs?
> >>>
> >>> Ha. I guess some realview boards for a start ...
> >>>
> >>> # grep -r "gic_init(1" arch/arm/
> >>> arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START,
> >>> arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE),
> >>> arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START,
> >>
> >> Different use case. These are development boards with a relatively
> >> modular design, where the SoC may or may not have a GIC by itself. When
> >> it has one, you end up with the on-board GIC being a secondary one.
> >
> > Yes, I understand that the use-case may be different, but I highlighted
> > this as a use where the primary map would be overwritten as the driver
> > is today.
> >
> >> I never thought someone would quote these designs as an example to
> >> follow... ;-)
> >
> > I can't say if this example was followed, but nevertheless hardware
> > designers certainly do get creative ;-)
> >
> > So we need to ensure the primary cpu map does not get overwritten by a
> > secondary and I have a case where the secondary map is of interest. So
> > are ok with this?
>
> My point is that there is no secondary map. That map should only be
> written for the primary GIC, because that's the only place it makes
> sense. Your "other CPU" is not under the control of Linux (at least, not
> as a CPU), so this map is not the right data structure.
Makes sense. I initially thought the patch was made to deal with, say,
2 GICs each routed to different CPU clusters. But even if that were the
case, the switcher code would need to keep track of both the CPU map
index and the GIC number. Therefore I revoke my ACK.
Nicolas