arm/arm64: Turning IPIs into normal interrupts
Subsystems (9)
Marc Zyngier
maintainerv2 cover arm/arm64: Turning IPIs into normal interrupts
For as long as SMP ARM has existed, IPIs have been handled as something special. The arch code and the interrupt controller exchange a couple of hooks (one to generate an IPI, another to handle it).
Although this is perfectly manageable, it prevents the use of features that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It also means that each interrupt controller driver has to follow an architecture-specific interface instead of just implementing the base irqchip functionalities. The arch code also duplicates a number of things that the core irq code already does (such as calling set_irq_regs(), irq_enter()...).
This series tries to remedy this on arm/arm64 by offering a new registration interface where the irqchip gives the arch code a range of interrupts to use for IPIs. The arch code requests these as normal per-cpu interrupts.
The bulk of the work is at the interrupt controller level, where all 5 irqchips used on arm+SMP/arm64 get converted.
Finally, we drop the legacy registration interface as well as the custom statistics accounting.
Note that I have had a look at providing a "generic" interface by expanding the kernel/irq/ipi.c bag of helpers, but so far all irqchips have very different requirements, so there is hardly anything to consolidate for now. Maybe some as hip04 and the Marvell horror get cleaned up (the latter certainly could do with a good dusting).
This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3), as well as 64bit bare metal (GICv3). The RPi part has only been tested in QEMU as a 64bit guest, while the HiSi and Marvell parts have only been compile-tested.
* From v1:
- Clarified the effect of nesting irq_enter/exit (Russell)
- Changed the point where we tear IPIs down on (Valentin)
- IPIs are no longer accessible from DT
- HIP04 and Armada 370-XP have been converted, but are untested
- arch-specific kstat accounting is removed
- ARM's legacy interface is dropped
Marc Zyngier (17):
genirq: Add fasteoi IPI flow
genirq: Allow interrupts to be excluded from /proc/interrupts
arm64: Allow IPIs to be handled as normal interrupts
ARM: Allow IPIs to be handled as normal interrupts irqchip/gic-v3: Describe the SGI range
irqchip/gic-v3: Configure SGIs as standard interrupts irqchip/gic: Atomically update affinity
irqchip/gic: Refactor SMP configuration
irqchip/gic: Configure SGIs as standard interrupts irqchip/gic-common: Don't enable SGIs by default irqchip/bcm2836: Configure mailbox interrupts as standard interrupts irqchip/hip04: Configure IPIs as standard interrupts irqchip/armada-370-xp: Configure IPIs as standard interrupts
arm64: Kill __smp_cross_call and co
arm64: Remove custom IRQ stat accounting
ARM: Kill __smp_cross_call and co
ARM: Remove custom IRQ stat accounting
arch/arm/Kconfig | 1 + arch/arm/include/asm/hardirq.h | 17 -- arch/arm/include/asm/smp.h | 5 +- arch/arm/kernel/smp.c | 135 +++++++++----- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/hardirq.h | 9 - arch/arm64/include/asm/irq_work.h | 4 +- arch/arm64/include/asm/smp.h | 6 +- arch/arm64/kernel/smp.c | 119 ++++++++----- drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++--------- drivers/irqchip/irq-bcm2836.c | 151 +++++++++++++--- drivers/irqchip/irq-gic-common.c | 3 - drivers/irqchip/irq-gic-v3.c | 99 ++++++----- drivers/irqchip/irq-gic.c | 183 ++++++++++--------- drivers/irqchip/irq-hip04.c | 89 +++++----- include/linux/irq.h | 5 +- kernel/irq/chip.c | 27 +++ kernel/irq/debugfs.c | 1 + kernel/irq/proc.c | 2 +- kernel/irq/settings.h | 7 + 20 files changed, 713 insertions(+), 413 deletions(-)
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 1/17 genirq: Add fasteoi IPI flow
For irqchips using the fasteoi flow, IPIs are a bit special. They need to be EOI'd early (before calling the handler), as funny things may happen in the handler (they do not necessarily behave like a normal interrupt).
--- include/linux/irq.h | 1 + kernel/irq/chip.c | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
include/linux/irq.h +1 -0
@@ -621,6 +621,7 @@ static inline int irq_set_parent(int irq, int parent_irq)*/ extern void handle_level_irq (struct irq_desc *desc);extern void handle_fasteoi_irq (struct irq_desc *desc); +extern void handle_percpu_devid_fasteoi_ipi (struct irq_desc *desc);extern void handle_edge_irq (struct irq_desc *desc);extern void handle_edge_eoi_irq (struct irq_desc *desc);extern void handle_simple_irq (struct irq_desc *desc);
kernel/irq/chip.c +27 -0
@@ -955,6 +955,33 @@ void handle_percpu_devid_irq(struct irq_desc *desc) chip->irq_eoi (&desc->irq_data); } +/** +* handle_percpu_devid_fasteoi_ipi - Per CPU local IPI handler with per cpu +* dev ids +* @desc: the interrupt description structure for this irq +* +* The biggest difference with the IRQ version is that the interrupt is +* EOIed early, as the IPI could result in a context switch, and we need to +* make sure the IPI can fire again. We also assume that the arch code has +* registered an action. If not, we are positively doomed. +*/ +void handle_percpu_devid_fasteoi_ipi (struct irq_desc *desc) +{ +struct irq_chip *chip =irq_desc_get_chip (desc); +struct irqaction *action = desc->action; +unsigned int irq =irq_desc_get_irq (desc); +irqreturn_t res; + +__kstat_incr_irqs_this_cpu (desc); + +if (chip->irq_eoi) + chip->irq_eoi (&desc->irq_data); + +trace_irq_handler_entry (irq, action); + res = action->handler (irq,raw_cpu_ptr (action->percpu_dev_id)); +trace_irq_handler_exit (irq, action, res); +} +/** * handle_percpu_devid_fasteoi_nmi - Per CPU local NMI handler with per cpu * dev ids
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 2/17 genirq: Allow interrupts to be excluded from /proc/interrupts
In reply to Marc ZyngierA number of architectures implement IPI statistics directly, duplicating the core kstat_irqs accounting. As we move IPIs to being actual IRQs, we would end-up with a confusing display in /proc/interrupts (where the IPIs would appear twice).
In order to solve this, allow interrupts to be flagged as "hidden", which excludes them from /proc/interrupts.
--- include/linux/irq.h | 4 +++- kernel/irq/debugfs.c | 1 + kernel/irq/proc.c | 2 +- kernel/irq/settings.h | 7 +++++++ 4 files changed, 12 insertions(+), 2 deletions(-)
include/linux/irq.h +3 -1
@@ -71,6 +71,7 @@ enum irqchip_irq_state;* it from the spurious interrupt detection * mechanism and from core side polling. * IRQ_DISABLE_UNLAZY - Disable lazy irq disable +* IRQ_HIDDEN - Don't show up in /proc/interrupts */ enum {IRQ_TYPE_NONE =0x00000000 ,
@@ -97,13 +98,14 @@ enum {IRQ_PER_CPU_DEVID = (1 <<17 ),IRQ_IS_POLLED = (1 <<18 ),IRQ_DISABLE_UNLAZY = (1 <<19 ), + IRQ_HIDDEN = (1 << 20), };#define IRQF_MODIFY_MASK \ (IRQ_TYPE_SENSE_MASK | IRQ_NOPROBE | IRQ_NOREQUEST | \ IRQ_NOAUTOEN | IRQ_MOVE_PCNTXT | IRQ_LEVEL | IRQ_NO_BALANCING | \ IRQ_PER_CPU | IRQ_NESTED_THREAD | IRQ_NOTHREAD | IRQ_PER_CPU_DEVID | \ - IRQ_IS_POLLED | IRQ_DISABLE_UNLAZY) + IRQ_IS_POLLED | IRQ_DISABLE_UNLAZY | IRQ_HIDDEN)#define IRQ_NO_BALANCING_MASK (IRQ_PER_CPU | IRQ_NO_BALANCING)
kernel/irq/debugfs.c +1 -0
@@ -131,6 +131,7 @@ static const struct irq_bit_descr irqdesc_states[] = {BIT_MASK_DESCR (_IRQ_PER_CPU_DEVID),BIT_MASK_DESCR (_IRQ_IS_POLLED ),BIT_MASK_DESCR (_IRQ_DISABLE_UNLAZY ), + BIT_MASK_DESCR(_IRQ_HIDDEN), };static const struct irq_bit_descr irqdesc_istates[] = {
kernel/irq/proc.c +1 -1
@@ -485,7 +485,7 @@ int show_interrupts(struct seq_file *p, void *v)rcu_read_lock (); desc =irq_to_desc (i); -if (!desc) +if (!desc ||irq_settings_is_hidden (desc)) goto outsparse;if (desc->kstat_irqs)
kernel/irq/settings.h +7 -0
@@ -17,6 +17,7 @@ enum { _IRQ_PER_CPU_DEVID =IRQ_PER_CPU_DEVID , _IRQ_IS_POLLED =IRQ_IS_POLLED , _IRQ_DISABLE_UNLAZY =IRQ_DISABLE_UNLAZY , + _IRQ_HIDDEN =IRQ_HIDDEN , _IRQF_MODIFY_MASK = IRQF_MODIFY_MASK, };
@@ -31,6 +32,7 @@ enum {#define IRQ_PER_CPU_DEVID GOT_YOU_MORON#define IRQ_IS_POLLED GOT_YOU_MORON#define IRQ_DISABLE_UNLAZY GOT_YOU_MORON +#define IRQ_HIDDEN GOT_YOU_MORON#undef IRQF_MODIFY_MASK#define IRQF_MODIFY_MASK GOT_YOU_MORON
@@ -167,3 +169,8 @@ static inline void irq_settings_clr_disable_unlazy(struct irq_desc *desc) { desc->status_use_accessors &= ~_IRQ_DISABLE_UNLAZY; } + +static inline bool irq_settings_is_hidden (struct irq_desc *desc) +{ +return desc->status_use_accessors & _IRQ_HIDDEN; +}
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 3/17 arm64: Allow IPIs to be handled as normal interrupts
In reply to Marc ZyngierIn order to deal with IPIs as normal interrupts, let's add a new way to register them with the architecture code.
set_smp_ipi_range() takes a range of interrupts, and allows the arch code to request them as if the were normal interrupts. A standard handler is then called by the core IRQ code to deal with the IPI.
This means that we don't need to call irq_enter/irq_exit, and that we don't need to deal with set_irq_regs either. So let's move the dispatcher into its own function, and leave handle_IPI() as a compatibility function.
On the sending side, let's make use of ipi_send_mask, which already exists for this purpose.
One of the major difference is that we end up, in some cases (such as when performing IRQ time accounting on the scheduler IPI), end up with nested irq_enter()/irq_exit() pairs. Other than the (relatively small) overhead, there should be no consequences to it (these pairs are designed to nest correctly, and the accounting shouldn't be off).
--- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/smp.h | 5 ++ arch/arm64/kernel/smp.c | 91 +++++++++++++++++++++++++++++++----- 3 files changed, 85 insertions(+), 12 deletions(-)
arch/arm64/Kconfig +1 -0
@@ -106,6 +106,7 @@ config ARM64 select GENERIC_CPU_VULNERABILITIES select GENERIC_EARLY_IOREMAP select GENERIC_IDLE_POLL_SETUP + select GENERIC_IRQ_IPI select GENERIC_IRQ_MULTI_HANDLER select GENERIC_IRQ_PROBE select GENERIC_IRQ_SHOW
arch/arm64/include/asm/smp.h +5 -0
@@ -74,6 +74,11 @@ extern void set_smp_cross_call(void (*)(const struct cpumask *, unsigned int));extern void (*__smp_cross_call)(const struct cpumask *,unsigned int ); +/* +* Register IPI interrupts with the arch SMP code +*/ +extern void set_smp_ipi_range (int ipi_base,int nr_ipi); +/* * Called from the secondary holding pen, this is the secondary CPU entry point. */
arch/arm64/kernel/smp.c +79 -12
154 lines not on this page · raw message
Signature · 1 line
2.27.0
Re: [PATCH v2 03/17] arm64: Allow IPIs to be handled as normal interrupts
Quoted 28 lines · Marc Zyngier
arch/arm64/kernel/smp.c +21
-0@@ -958,9 +958,76 @@ void handle_IPI(int ipinr, struct pt_regs *regs)if ((unsigned )ipinr <NR_IPI )trace_ipi_exit_rcuidle (ipi_types[ipinr]); +} + +/* Legacy version, should go away once all irqchips have been converted */ +void handle_IPI (int ipinr,struct pt_regs *regs) +{ +struct pt_regs *old_regs =set_irq_regs (regs); + +irq_enter (); +do_handle_IPI (ipinr); +irq_exit (); +set_irq_regs (old_regs); } +static irqreturn_t ipi_handler (int irq,void *data) +{ +do_handle_IPI (irq - ipi_irq_base); +return IRQ_HANDLED ; +} + +static void ipi_send (const struct cpumask *target,unsigned int ipi) +{ +__ipi_send_mask (ipi_desc[ipi], target); +}
arch/arm64/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 994
Quoted 10 lines
arch/arm64/kernel/smp.c +10
-0+ +static void ipi_setup (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++) +enable_percpu_irq (ipi_irq_base + i,0 ); + } +}
Nit: Once we have the irqchip changes in, should we warn & bail out when !ipi_irq_base? Ditto for the teardown
Quoted 10 lines
arch/arm64/kernel/smp.c +10
-0+ +static void ipi_teardown (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++) +disable_percpu_irq (ipi_irq_base + i); + } +}
Re: [PATCH v2 03/17] arm64: Allow IPIs to be handled as normal interrupts
In reply to Marc ZyngierItsy bitsy nit below from re-reading this.
arch/arm64/kernel/smp.c enum ipi_msg_type { · line 80
Quoted 10 lines · Marc Zyngier
arch/arm64/kernel/smp.c +2
-0@@ -76,6 +76,13 @@ enum ipi_msg_type { IPI_WAKEUP }; +static int ipi_irq_base; +static int nr_ipi =NR_IPI ;
If we really care, these two could be marked as __read_mostly.
Quoted 8 lines
arch/arm64/kernel/smp.c +5
-0+static struct irq_desc *ipi_desc[NR_IPI ]; + +static void ipi_setup (int cpu); +static void ipi_teardown (int cpu); +#ifdef CONFIG_HOTPLUG_CPU static int op_cpu_kill (unsigned int cpu);#else
Marc Zyngier
maintainerPATCH v2 4/17 ARM: Allow IPIs to be handled as normal interrupts
In reply to Marc ZyngierIn order to deal with IPIs as normal interrupts, let's add a new way to register them with the architecture code.
set_smp_ipi_range() takes a range of interrupts, and allows the arch code to request them as if the were normal interrupts. A standard handler is then called by the core IRQ code to deal with the IPI.
This means that we don't need to call irq_enter/irq_exit, and that we don't need to deal with set_irq_regs either. So let's move the dispatcher into its own function, and leave handle_IPI() as a compatibility function.
On the sending side, let's make use of ipi_send_mask, which already exists for this purpose.
One of the major difference is that we end up, in some cases (such as when performing IRQ time accounting on the scheduler IPI), end up with nested irq_enter()/irq_exit() pairs. Other than the (relatively small) overhead, there should be no consequences to it (these pairs are designed to nest correctly, and the accounting shouldn't be off).
--- arch/arm/Kconfig | 1 + arch/arm/include/asm/smp.h | 5 ++ arch/arm/kernel/smp.c | 99 ++++++++++++++++++++++++++++++++------ 3 files changed, 89 insertions(+), 16 deletions(-)
arch/arm/Kconfig +1 -0
@@ -48,6 +48,7 @@ config ARM select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI select GENERIC_CLOCKEVENTS_BROADCAST if SMP + select GENERIC_IRQ_IPI if SMP select GENERIC_CPU_AUTOPROBE select GENERIC_EARLY_IOREMAP select GENERIC_IDLE_POLL_SETUP
arch/arm/include/asm/smp.h +5 -0
@@ -45,6 +45,11 @@ extern void smp_init_cpus(void);*/ extern void set_smp_cross_call (void (*)(const struct cpumask *,unsigned int )); +/* +* Register IPI interrupts with the arch SMP code +*/ +extern void set_smp_ipi_range (int ipi_base,int nr_ipi); +/* * Called from platform specific assembly code, this is the * secondary CPU entry point.
arch/arm/kernel/smp.c +83 -16
177 lines not on this page · raw message
Signature · 1 line
2.27.0
Re: [PATCH v2 04/17] ARM: Allow IPIs to be handled as normal interrupts
Quoted 45 lines · Marc Zyngier
arch/arm/kernel/smp.c +38
-0@@ -696,9 +696,76 @@ void handle_IPI(int ipinr, struct pt_regs *regs)if ((unsigned )ipinr <NR_IPI )trace_ipi_exit_rcuidle (ipi_types[ipinr]); +} + +/* Legacy version, should go away once all irqchips have been converted */ +void handle_IPI (int ipinr,struct pt_regs *regs) +{ +struct pt_regs *old_regs =set_irq_regs (regs); + +irq_enter (); +do_handle_IPI (ipinr); +irq_exit (); +set_irq_regs (old_regs); } +static irqreturn_t ipi_handler (int irq,void *data) +{ +do_handle_IPI (irq - ipi_irq_base); +return IRQ_HANDLED ; +} + +static void ipi_send (const struct cpumask *target,unsigned int ipi) +{ +__ipi_send_mask (ipi_desc[ipi], target); +} + +static void ipi_setup (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++) +enable_percpu_irq (ipi_irq_base + i,0 ); + } +} + +static void ipi_teardown (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++)
arch/arm/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 749
Quoted 10 lines
arch/arm/kernel/smp.c +10
-0+disable_percpu_irq (ipi_irq_base + i); + } +} + +void __init set_smp_ipi_range (int ipi_base,int n) +{ +int i; + +WARN_ON (n <MAX_IPI ); + nr_ipi =min (n,MAX_IPI );
I got confused by that backtrace thing and NR_IPI vs MAX_IPI. I think I got it now : we don't want to call trace_ipi_raise() for IPI_CPU_BACKTRACE *but* we still need to alloc the desc and route it through the generic IPI layers.
The only difference I can tell is that now we will get some trace events for it via the handler entry/exit tracepoints - that shouldn't cause any issue.
Quoted 22 lines
arch/arm/kernel/smp.c +19
-0+ +for (i =0 ; i < nr_ipi; i++) { +int err; + + err =request_percpu_irq (ipi_base + i, ipi_handler, +"IPI" , &irq_stat); +WARN_ON (err); + + ipi_desc[i] =irq_to_desc (ipi_base + i); +irq_set_status_flags (ipi_base + i,IRQ_HIDDEN ); + } + + ipi_irq_base = ipi_base; +set_smp_cross_call (ipi_send); + +/* Setup the boot CPU immediately */ +ipi_setup (smp_processor_id ()); +} +void smp_send_reschedule (int cpu) {smp_cross_call (cpumask_of (cpu),IPI_RESCHEDULE );
Marc Zyngier
maintainerRe: [PATCH v2 04/17] ARM: Allow IPIs to be handled as normal interrupts
Quoted 55 lines · Valentin Schneider
Quoted 54 lines · Marc Zyngier
arch/arm/kernel/smp.c +3
-0@@ -696,9 +696,76 @@ void handle_IPI(int ipinr, struct pt_regs *regs)if ((unsigned )ipinr <NR_IPI )trace_ipi_exit_rcuidle (ipi_types[ipinr]); +} + +/* Legacy version, should go away once all irqchips have been converted */
arch/arm/kernel/smp.c +43
-0+void handle_IPI (int ipinr,struct pt_regs *regs) +{ +struct pt_regs *old_regs =set_irq_regs (regs); + +irq_enter (); +do_handle_IPI (ipinr); +irq_exit (); +set_irq_regs (old_regs); } +static irqreturn_t ipi_handler (int irq,void *data) +{ +do_handle_IPI (irq - ipi_irq_base); +return IRQ_HANDLED ; +} + +static void ipi_send (const struct cpumask *target,unsigned int ipi) +{ +__ipi_send_mask (ipi_desc[ipi], target); +} + +static void ipi_setup (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++) +enable_percpu_irq (ipi_irq_base + i,0 ); + } +} + +static void ipi_teardown (int cpu) +{ +if (ipi_irq_base) { +int i; + +for (i =0 ; i < nr_ipi; i++) +disable_percpu_irq (ipi_irq_base + i); + } +} + +void __init set_smp_ipi_range (int ipi_base,int n) +{ +int i; +
Quoted 8 lines
arch/arm/kernel/smp.c void handle_IPI(int ipinr, struct pt_regs *regs) · line 749
Quoted 2 lines
arch/arm/kernel/smp.c +2
-0+WARN_ON (n <MAX_IPI ); + nr_ipi =min (n,MAX_IPI );I got confused by that backtrace thing and
NR_IPIvsMAX_IPI. I think I got it now : we don't want to calltrace_ipi_raise()forIPI_CPU_BACKTRACE*but* we still need to alloc the desc and route it through the generic IPI layers.
Indeed, and I didn't want to have a bizarre "+ 1" hanging about.
Quoted 3 lines
The only difference I can tell is that now we will get some trace events for it via the handler entry/exit tracepoints - that shouldn't cause any issue.
I hope so. I don't see how you can avoid all tracepoints anyway (if that was the intention).
Thanks,
M.
Signature · 6 lines
Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Marc Zyngier
maintainerPATCH v2 5/17 irqchip/gic-v3: Describe the SGI range
In reply to Marc ZyngierAs we are about to start making use of SGIs in a more conventional way, let's describe it is the GICv3 list of interrupt types.
--- drivers/irqchip/irq-gic-v3.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
drivers/irqchip/irq-gic-v3.c +15 -3
@@ -112,6 +112,7 @@ static DEFINE_PER_CPU(bool, has_rss);#define DEFAULT_PMR_VALUE 0xf0enum gic_intid_range { +SGI_RANGE ,PPI_RANGE ,SPI_RANGE ,EPPI_RANGE ,
@@ -123,6 +124,8 @@ enum gic_intid_range {static enum gic_intid_range __get_intid_range (irq_hw_number_t hwirq) {switch (hwirq) { + case 0 ... 15: +return SGI_RANGE ; case 16 ... 31:return PPI_RANGE ; case 32 ... 1019:
@@ -148,15 +151,22 @@ static inline unsigned int gic_irq(struct irq_data *d)return d->hwirq; } -static inline int gic_irq_in_rdist (struct irq_data *d) +static inline bool gic_irq_in_rdist (struct irq_data *d) { -enum gic_intid_range range =get_intid_range (d); -return range ==PPI_RANGE || range ==EPPI_RANGE ; +switch (get_intid_range (d)) { +case SGI_RANGE : +case PPI_RANGE : +case EPPI_RANGE : +return true; +default : +return false; + } }static inline void __iomem *gic_dist_base (struct irq_data *d) {switch (get_intid_range (d)) { +case SGI_RANGE :case PPI_RANGE :case EPPI_RANGE :/* SGI+PPI -> SGI_base for this CPU */
@@ -253,6 +263,7 @@ static void gic_enable_redist(bool enable)static u32 convert_offset_index (struct irq_data *d,u32 offset,u32 *index) {switch (get_intid_range (d)) { +case SGI_RANGE :case PPI_RANGE :case SPI_RANGE : *index = d->hwirq;
@@ -1277,6 +1288,7 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, chip = &gic_eoimode1_chip;switch (__get_intid_range (hw)) { +case SGI_RANGE :case PPI_RANGE :case EPPI_RANGE :irq_set_percpu_devid (irq);
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 6/17 irqchip/gic-v3: Configure SGIs as standard interrupts
In reply to Marc ZyngierChange the way we deal with GICv3 SGIs by turning them into proper IRQs, and calling into the arch code to register the interrupt range instead of a callback.
--- drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 38 deletions(-)
drivers/irqchip/irq-gic-v3.c +43 -38
@@ -36,6 +36,8 @@#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1) +#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) +struct redist_region {void __iomem *redist_base;phys_addr_t phys_base;
@@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regsif ((irqnr >=1020 && irqnr <=1023 ))return ; -/* Treat anything but SGIs in a uniform way */ -if (likely (irqnr >15 )) { -int err; - -if (static_branch_likely (&supports_deactivate_key)) -gic_write_eoir (irqnr); -else -isb (); - - err =handle_domain_irq (gic_data.domain, irqnr, regs); -if (err) { -WARN_ONCE (true,"Unexpected interrupt received!\n" ); -gic_deactivate_unhandled (irqnr); - } -return ; - } -if (irqnr <16 ) { +if (static_branch_likely (&supports_deactivate_key))gic_write_eoir (irqnr); -if (static_branch_likely (&supports_deactivate_key)) -gic_write_dir (irqnr); -#ifdef CONFIG_SMP -/* -* Unlike GICv2, we don't need an smp_rmb() here. -* The control dependency from gic_read_iar to -* the ISB in gic_write_eoir is enough to ensure -* that any shared data read by handle_IPI will -* be read after the ACK. -*/ -handle_IPI (irqnr, regs); -#else -WARN_ONCE (true,"Unexpected SGI received!\n" ); -#endif +else +isb (); + +if (handle_domain_irq (gic_data.domain, irqnr, regs)) { +WARN_ONCE (true,"Unexpected interrupt received!\n" ); +gic_deactivate_unhandled (irqnr); } }
@@ -1136,11 +1114,11 @@ static void gic_send_sgi(u64 cluster_id, u16 tlist, unsigned int irq)gic_write_sgi1r (val); } -static void gic_raise_softirq (const struct cpumask *mask,unsigned int irq) +static void gic_ipi_send_mask (struct irq_data *d,const struct cpumask *mask) {int cpu; -if (WARN_ON (irq >=16 )) +if (WARN_ON (d->hwirq >=16 ))return ;/*
@@ -1154,7 +1132,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)u16 tlist; tlist =gic_compute_target_list (&cpu, mask, cluster_id); -gic_send_sgi (cluster_id, tlist, irq); +gic_send_sgi (cluster_id, tlist, d->hwirq); }/* Force the above writes to ICC_SGI1R_EL1 to be executed */
@@ -1163,10 +1141,24 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)static void __init gic_smp_init (void ) { -set_smp_cross_call (gic_raise_softirq); +struct irq_fwspec sgi_fwspec = { + .fwnode = gic_data.fwnode, + .param_count =1 , + }; +int base_sgi; +cpuhp_setup_state_nocalls (CPUHP_AP_IRQ_GIC_STARTING ,"irqchip/arm/gicv3:starting" , gic_starting_cpu,NULL ); + +/* Register all 8 non-secure SGIs */ + base_sgi =__irq_domain_alloc_irqs (gic_data.domain,-1 ,8 , +NUMA_NO_NODE , &sgi_fwspec, + false,NULL ); +if (WARN_ON (base_sgi <=0 )) +return ; + +set_smp_ipi_range (base_sgi,8 ); } static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
@@ -1215,6 +1207,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, }#else #define gic_set_affinity NULL +#define gic_ipi_send_mask NULL#define gic_smp_init () do { } while(0)#endif
@@ -1257,6 +1250,7 @@ static struct irq_chip gic_chip = { .irq_set_irqchip_state = gic_irq_set_irqchip_state, .irq_nmi_setup = gic_irq_nmi_setup, .irq_nmi_teardown = gic_irq_nmi_teardown, + .ipi_send_mask = gic_ipi_send_mask, .flags =IRQCHIP_SET_TYPE_MASKED |IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
@@ -1274,6 +1268,7 @@ static struct irq_chip gic_eoimode1_chip = { .irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity, .irq_nmi_setup = gic_irq_nmi_setup, .irq_nmi_teardown = gic_irq_nmi_teardown, + .ipi_send_mask = gic_ipi_send_mask, .flags =IRQCHIP_SET_TYPE_MASKED |IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND,
@@ -1289,6 +1284,12 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,switch (__get_intid_range (hw)) {case SGI_RANGE : +irq_set_percpu_devid (irq); +irq_domain_set_info (d, irq, hw, chip, d->host_data, + handle_percpu_devid_fasteoi_ipi, +NULL ,NULL ); +break ; +case PPI_RANGE :case EPPI_RANGE :irq_set_percpu_devid (irq);
@@ -1318,13 +1319,17 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,return 0 ; } -#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) -static int gic_irq_domain_translate (struct irq_domain *d,struct irq_fwspec *fwspec,unsigned long *hwirq,unsigned int *type) { +if (fwspec->param_count ==1 && fwspec->param[0 ] <16 ) { + *hwirq = fwspec->param[0 ]; + *type =IRQ_TYPE_EDGE_RISING ; +return 0 ; + } +if (is_of_node (fwspec->fwnode)) {if (fwspec->param_count <3 )return -EINVAL ;
@@ -1656,9 +1661,9 @@ static int __init gic_init_bases(void __iomem *dist_base,gic_update_rdist_properties (); -gic_smp_init ();gic_dist_init ();gic_cpu_init (); +gic_smp_init ();gic_cpu_pm_init ();if (gic_dist_supports_lpis ()) {
Signature · 1 line
2.27.0
Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Quoted 14 lines · Marc Zyngier
Change the way we deal with GICv3 SGIs by turning them into proper IRQs, and calling into the arch code to register the interrupt range instead of a callback.
Signed-off-by Marc Zyngier <[email protected]>--- drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 38 deletions(-)drivers/irqchip/irq-gic-v3.c +0
-0
drivers/irqchip/irq-gic-v3.c line 40
Quoted 5 lines
drivers/irqchip/irq-gic-v3.c +2
-0@@ -36,6 +36,8 @@#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1) +#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) +
Nit: this piqued my interest but ended up being just a define shuffle; As a member of the git speleologists' guild, I'd be overjoyed with having a small notion of that in the changelog.
Quoted 26 lines
drivers/irqchip/irq-gic-v3.c +1
-17struct redist_region {void __iomem *redist_base;phys_addr_t phys_base;@@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regsif ((irqnr >=1020 && irqnr <=1023 ))return ; -/* Treat anything but SGIs in a uniform way */ -if (likely (irqnr >15 )) { -int err; - -if (static_branch_likely (&supports_deactivate_key)) -gic_write_eoir (irqnr); -else -isb (); - - err =handle_domain_irq (gic_data.domain, irqnr, regs); -if (err) { -WARN_ONCE (true,"Unexpected interrupt received!\n" ); -gic_deactivate_unhandled (irqnr); - } -return ; - } -if (irqnr <16 ) { +if (static_branch_likely (&supports_deactivate_key))gic_write_eoir (irqnr);
drivers/irqchip/irq-gic-v3.c static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs · line 687
Quoted 10 lines
drivers/irqchip/irq-gic-v3.c +0
-10-if (static_branch_likely (&supports_deactivate_key)) -gic_write_dir (irqnr); -#ifdef CONFIG_SMP -/* -* Unlike GICv2, we don't need an smp_rmb() here. -* The control dependency from gic_read_iar to -* the ISB in gic_write_eoir is enough to ensure -* that any shared data read by handle_IPI will -* be read after the ACK. -*/
Isn't that still relevant?
Also, while staring at this it dawned on me that IPI's don't need the eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll get a gic_eoi_irq() just before calling into the irqaction. Dunno how much we care about it.
Quoted 13 lines
drivers/irqchip/irq-gic-v3.c +6
-4-handle_IPI (irqnr, regs); -#else -WARN_ONCE (true,"Unexpected SGI received!\n" ); -#endif +else +isb (); + +if (handle_domain_irq (gic_data.domain, irqnr, regs)) { +WARN_ONCE (true,"Unexpected interrupt received!\n" ); +gic_deactivate_unhandled (irqnr); } }
Marc Zyngier
maintainerRe: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Quoted 19 lines · Valentin Schneider
Quoted 18 lines · Marc Zyngier
Change the way we deal with GICv3 SGIs by turning them into proper IRQs, and calling into the arch code to register the interrupt range instead of a callback.
Signed-off-by Marc Zyngier <[email protected]>--- drivers/irqchip/irq-gic-v3.c | 81 +++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 38 deletions(-)drivers/irqchip/irq-gic-v3.c +0
-0@@ -36,6 +36,8 @@#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
Quoted 6 lines
drivers/irqchip/irq-gic-v3.c line 40
Quoted 2 lines
drivers/irqchip/irq-gic-v3.c +2
-0+#define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1) +Nit: this piqued my interest but ended up being just a define shuffle; As a member of the git speleologists' guild, I'd be overjoyed with having a small notion of that in the changelog.
Fair enough.
Quoted 32 lines
Quoted 32 lines
drivers/irqchip/irq-gic-v3.c +0
-0struct redist_region {void __iomem *redist_base;phys_addr_t phys_base;@@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entrygic_handle_irq(struct
pt_regs*regsif ((irqnr >= 1020 && irqnr <= 1023)) return;drivers/irqchip/irq-gic-v3.c +1
-21-/* Treat anything but SGIs in a uniform way */ -if (likely (irqnr >15 )) { -int err; - -if (static_branch_likely (&supports_deactivate_key)) -gic_write_eoir (irqnr); -else -isb (); - - err =handle_domain_irq (gic_data.domain, irqnr, regs); -if (err) { -WARN_ONCE (true,"Unexpected interrupt received!\n" ); -gic_deactivate_unhandled (irqnr); - } -return ; - } -if (irqnr <16 ) { +if (static_branch_likely (&supports_deactivate_key))gic_write_eoir (irqnr); -if (static_branch_likely (&supports_deactivate_key)) -gic_write_dir (irqnr); -#ifdef CONFIG_SMP -/*
Quoted 8 lines
drivers/irqchip/irq-gic-v3.c static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs · line 687
Quoted 6 lines
drivers/irqchip/irq-gic-v3.c +0
-6-* Unlike GICv2, we don't need an smp_rmb() here. -* The control dependency from gic_read_iar to -* the ISB in gic_write_eoir is enough to ensure -* that any shared data read by handle_IPI will -* be read after the ACK. -*/ Isn't that still relevant?
It is. It is just that there is no really good place to put it. I may end-up just leaving it where it is.
Quoted 4 lines
Also, while staring at this it dawned on me that IPI's don't need the eoimode=0
isb(): due to how the IPI flow-handler is structured, we'll get agic_eoi_irq()just before calling into the irqaction. Dunno how much we care about it.
That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.
Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider
Also, while staring at this it dawned on me that IPI's don't need the eoimode=0
isb(): due to how the IPI flow-handler is structured, we'll get agic_eoi_irq()just before calling into the irqaction. Dunno how much we care about it.
Quoted 9 lines
That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.
Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.
If I got this one right:
39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and ->handle_irq")
you're describing case 2, which is indeed gone on gic-v3. However IIUC we also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the early gic_eoi_irq(), but we don't for the other flows.
Quoted 3 lines
Thanks,
M.
Marc Zyngier
maintainerRe: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Quoted 20 lines · Valentin Schneider
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider
Also, while staring at this it dawned on me that IPI's don't need the eoimode=0
isb(): due to how the IPI flow-handler is structured, we'll get agic_eoi_irq()just before calling into the irqaction. Dunno how much we care about it.Quoted 9 lines
That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.
Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.
If I got this one right:
Quoted 8 lines
39a06b67c2c1("irqchip/gic: Ensure we have an ISB between ack and
->handle_irq")you're describing case 2, which is indeed gone on gic-v3. However IIUC we also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the earlygic_eoi_irq(), but we don't
for the other flows.
You just made me realise something amazing: I've started to forget about all this crap. Which is wonderful! ;-)
More seriously, you are absolutely right. If we wanted to address this, we'd probably have to give IPIs their own irqchip so that they get their own eoi callback. Not sure that's worth it.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Quoted 33 lines · Marc Zyngier
Quoted 24 lines · Valentin Schneider
Quoted 8 lines · Marc Zyngier
Quoted 6 lines · Valentin Schneider
Also, while staring at this it dawned on me that IPI's don't need the eoimode=0
isb(): due to how the IPI flow-handler is structured, we'll get agic_eoi_irq()just before calling into the irqaction. Dunno how much we
care about it.Quoted 9 lines
That's interesting. This ISB is a leftover from the loop we had before the pseudo-NMI code, where we had to make sure the write to EOIR was ordered with the read from IAR.
Given that we have an exception return right after the interrupt handling, I *think* we could get rid of it (but that would need mode checking on broken systems such as TX1...). I don't think this is specific to IPIs though.
If I got this one right:
39a06b67c2c1("irqchip/gic: Ensure we have an ISB between ack and
->handle_irq")Quoted 7 lines
you're describing case 2, which is indeed gone on gic-v3. However IIUC we
also want an ISB between poking IAR and calling into the irqaction (case 1)
- we get just that with IPIs due to the earlygic_eoi_irq(), but we don't
for the other flows.
Quoted 3 lines
You just made me realise something amazing: I've started to forget about all this crap. Which is wonderful! ;-)
:)
Quoted 4 lines
More seriously, you are absolutely right. If we wanted to address this, we'd probably have to give IPIs their own irqchip so that they get their own eoi callback. Not sure that's worth it.
I was initially thinking of something like
if (static_branch_likely(&supports_deactivate_key))
gic_write_eoir(irqnr);
else if (__get_intid_range(irqnr) != SGI_RANGE)
isb();which is not particularly pretty, so maybe we should just slap this to the isb():
/* Superfluous for SGIs, but who cares */
Quoted 1 line
M.
Marc Zyngier
maintainerPATCH v2 7/17 irqchip/gic: Atomically update affinity
In reply to Marc ZyngierThe GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.
But these sequences only expend into anything meaningful if the BL_SWITCHER option is selected, which almost never happens.
It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the GICD_TARGETRn registers, which when used make the update atomic by definition.
Drop the terminally broken code and replace it by a byte write.
--- drivers/irqchip/irq-gic.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/irqchip/irq-gic.c +3 -11
@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)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 cpu, shift = (gic_irq (d) %4 ) *8 ; -u32 val, mask, bit; -unsigned long flags; +void __iomem *reg =gic_dist_base (d) +GIC_DIST_TARGET +gic_irq (d); +unsigned int cpu;if (!force) cpu =cpumask_any_and (mask_val, cpu_online_mask);
@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ; -gic_lock_irqsave (flags); - mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; - val =readl_relaxed (reg) & ~mask; -writel_relaxed (val | bit, reg); -gic_unlock_irqrestore (flags); - +writeb_relaxed (gic_cpu_map[cpu], reg);irq_data_update_effective_affinity (d,cpumask_of (cpu));return IRQ_SET_MASK_OK_DONE ;
Signature · 1 line
2.27.0
Sasha Levin
maintainerRe: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").
The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.
v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Signature · 2 lines
Thanks Sasha
Sasha Levin
maintainerRe: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
In reply to Marc ZyngierHi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature").
The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228.
v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Failed to apply! Possible dependencies:
0c9e498286ef9 ("irqchip/gic: Report that effective affinity is a single target")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Signature · 2 lines
Thanks Sasha
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
In reply to Marc ZyngierHi Marc, Russell,
Quoted 11 lines · Marc Zyngier
The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.
But these sequences only expend into anything meaningful if the
BL_SWITCHERoption is selected, which almost never happens.It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the
GICD_TARGETRnregisters, which when used make the update atomic by definition.
Quoted 5 lines
Drop the terminally broken code and replace it by a byte write.
Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")Signed-off-by Marc Zyngier <[email protected]>
Thanks for your patch, which is now commit 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.
Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.
Unfortunately I only have remote access to the board showing the issue. I did check that adding the writeb_relaxed() before the writel_relaxed() that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?
Since you said the locking was bogus before, due to the reliance on the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?
Thanks for your comments!
Quoted 20 lines
drivers/irqchip/irq-gic.c +2
-5@@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)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 cpu, shift = (gic_irq (d) %4 ) *8 ; -u32 val, mask, bit; -unsigned long flags; +void __iomem *reg =gic_dist_base (d) +GIC_DIST_TARGET +gic_irq (d); +unsigned int cpu;if (!force) cpu =cpumask_any_and (mask_val, cpu_online_mask);@@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ; -gic_lock_irqsave (flags);
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 346
Quoted 10 lines
drivers/irqchip/irq-gic.c +1
-6- mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; - val =readl_relaxed (reg) & ~mask; -writel_relaxed (val | bit, reg); -gic_unlock_irqrestore (flags); - +writeb_relaxed (gic_cpu_map[cpu], reg);irq_data_update_effective_affinity (d,cpumask_of (cpu));return IRQ_SET_MASK_OK_DONE ;
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Russell King (Oracle)
maintainerRe: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Quoted 5 lines · Geert Uytterhoeven
Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.
Fun. Seems someone can't read ARMs documentation. Even the old ARM IHI 0048B.b document I have for the GIC from 2013 states "In addition, the GICD_IPRIORITYRn, GICD_ITARGETSRn, GICD_CPENDSGIRn, and GICD_SPENDSGIRn registers support byte accesses."
However, this kind of thing is sadly not uncommon. There's been a similar issue with the PL011 UART driver as well - some platforms require 16-bit accesses instead of normal 32-bit accesses.
Quoted 9 lines
Unfortunately I only have remote access to the board showing the issue. I did check that adding the
writeb_relaxed()before thewritel_relaxed()that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?
Quoted 3 lines
Since you said the locking was bogus before, due to the reliance on the
BL_SWITCHERoption, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?
If byte accesses are not going to be workable, then the only answer _is_ a read-modify-write with working locking.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Marc Zyngier
maintainerRe: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
In reply to Geert UytterhoevenHi Geert,
Quoted 26 lines · Geert Uytterhoeven
Hi Marc, Russell,
Quoted 11 lines · Marc Zyngier
The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.
But these sequences only expend into anything meaningful if the
BL_SWITCHERoption is selected, which almost never happens.It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the
GICD_TARGETRnregisters, which when used make the update atomic by definition.Quoted 5 lines
Drop the terminally broken code and replace it by a byte write.
Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")Signed-off-by Marc Zyngier <[email protected]>Thanks for your patch, which is now commit
005c34ae4b44f085("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.
Quoted 5 lines
Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.
Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue...
Quoted 8 lines
Unfortunately I only have remote access to the board showing the issue. I did check that adding the
writeb_relaxed()before thewritel_relaxed()that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?
Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently.
Quoted 3 lines
Since you said the locking was bogus before, due to the reliance on the
BL_SWITCHERoption, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?
There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems.
I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps.
M.
drivers/irqchip/irq-gic.c +50 -1
@@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);#endif +static DEFINE_STATIC_KEY_FALSE (needs_rmw_access); +/* * The GIC mapping of CPU interfaces does not necessarily match * the logical CPU numbering. Let's use a mapping as returned
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)#endif #ifdef CONFIG_SMP +static void rmw_writeb (u8 bval,void __iomem *addr) +{ +static DEFINE_RAW_SPINLOCK (rmw_lock); +unsigned long offset = (unsigned long )addr & ~3UL ; +unsigned long shift = offset *8 ; +unsigned long flags; +u32 val; + +raw_spin_lock_irqsave (&rmw_lock, flags); + + addr -= offset; + val =readl_relaxed (addr); + val &= ~(0xffUL << shift); + val |= (u32 )bval << shift; +writel_relaxed (val, addr); + +raw_spin_unlock_irqrestore (&rmw_lock, flags); +} +static int gic_set_affinity (struct irq_data *d,const struct cpumask *mask_val,bool force) {
@@ -788,7 +809,10 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ; -writeb_relaxed (gic_cpu_map[cpu], reg); +if (static_branch_unlikely (&needs_rmw_access)) +rmw_writeb (gic_cpu_map[cpu], reg); +else +writeb_relaxed (gic_cpu_map[cpu], reg);irq_data_update_effective_affinity (d,cpumask_of (cpu));return IRQ_SET_MASK_OK_DONE ;
@@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)return true; } +static bool gic_enable_rmw_access (void *data) +{ +/* +* The EMEV2 class of machines has a broken interconnect, and +* locks up on accesses that are less than 32bit. So far, only +* the affinity setting requires it. +*/ +if (of_machine_is_compatible ("renesas,emev2" )) { +static_branch_enable (&needs_rmw_access); +return true; + } + +return false; +} + +static const struct gic_quirk gic_quirks[] = { + { + .desc ="Implementation with broken byte access" , + .compatible ="arm,pl390" , + .init = gic_enable_rmw_access, + }, +}; +static int gic_of_setup (struct gic_chip_data *gic,struct device_node *node) {if (!gic || !node)
@@ -1391,6 +1438,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)if (of_property_read_u32 (node,"cpu-offset" , &gic->percpu_offset)) gic->percpu_offset =0 ; +gic_enable_of_quirks (node, gic_quirks, gic); +return 0 ; error:
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Marc,
Quoted 25 lines · Marc Zyngier
Quoted 23 lines · Geert Uytterhoeven
Quoted 11 lines · Marc Zyngier
The GIC driver uses a RMW sequence to update the affinity, and relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences to update it atomically.
But these sequences only expend into anything meaningful if the
BL_SWITCHERoption is selected, which almost never happens.It also turns out that using a RMW and locks is just as silly, as the GIC distributor supports byte accesses for the
GICD_TARGETRnregisters, which when used make the update atomic by definition.Quoted 5 lines
Drop the terminally broken code and replace it by a byte write.
Fixes 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")Signed-off-by Marc Zyngier <[email protected]>Thanks for your patch, which is now commit
005c34ae4b44f085("irqchip/gic: Atomically update affinity"), to which I bisected a hard lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual board, which has a dual Cortex-A9 with PL390.
Quoted 8 lines
Quoted 5 lines
Despite the ARM Generic Interrupt Controller Architecture Specification (both version 1.0 and 2.0) stating that the Interrupt Processor Targets Registers are byte-accessible, the EMMA Mobile EV2 User's Manual states that the interrupt registers can be accessed via the APB bus, in 32-bit units. Using byte accesses locks up the system.
Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue...
Yes, they do.
Quoted 18 lines
Quoted 8 lines
Unfortunately I only have remote access to the board showing the issue. I did check that adding the
writeb_relaxed()before thewritel_relaxed()that was used before also causes a lock-up, so the issue is not an endian mismatch.
Looking at the driver history, these registers have always been accessed using 32-bit accesses before. As byte accesses lead indeed to simpler code, I'm wondering if they had been tried before, and caused issues before?Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently.
Quoted 3 lines
Since you said the locking was bogus before, due to the reliance on the
BL_SWITCHERoption, I'm not suggesting a plain revert, but I'm wondering what kind of locking you suggest to use instead?
Quoted 6 lines
There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems.
I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps.
Thanks for your quick response!
Your solution works, after making a few small modifications.
drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 782
Quoted 10 lines
drivers/irqchip/irq-gic.c +4
-0@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)#endif #ifdef CONFIG_SMP +static void rmw_writeb (u8 bval,void __iomem *addr) +{ +static DEFINE_RAW_SPINLOCK (rmw_lock); +unsigned long offset = (unsigned long )addr & ~3UL ;
Please drop the tilde.
drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 783
Quoted 1 line
drivers/irqchip/irq-gic.c +1
-0+unsigned long shift = offset *8 ;
"unsigned int" is sufficient for offset and size.
drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 791
Quoted 8 lines
drivers/irqchip/irq-gic.c +8
-0+unsigned long flags; +u32 val; + +raw_spin_lock_irqsave (&rmw_lock, flags); + + addr -= offset; + val =readl_relaxed (addr); + val &= ~(0xffUL << shift);
No need for the UL suffix.
drivers/irqchip/irq-gic.c static int gic_pm_init(struct gic_chip_data *gic) · line 792
Quoted 1 line
drivers/irqchip/irq-gic.c +1
-0+ val |= (u32 )bval << shift;
No need for the cast.
Quoted 21 lines
drivers/irqchip/irq-gic.c +13
-0+writel_relaxed (val, addr); + +raw_spin_unlock_irqrestore (&rmw_lock, flags); +} +static int gic_set_affinity (struct irq_data *d,const struct cpumask *mask_val,bool force) {@@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)return true; } +static bool gic_enable_rmw_access (void *data) +{ +/* +* The EMEV2 class of machines has a broken interconnect, and +* locks up on accesses that are less than 32bit. So far, only +* the affinity setting requires it. +*/ +if (of_machine_is_compatible ("renesas,emev2" )) {
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1419
Quoted 10 lines
drivers/irqchip/irq-gic.c +10
-0+static_branch_enable (&needs_rmw_access); +return true; + } + +return false; +} + +static const struct gic_quirk gic_quirks[] = { + { + .desc ="Implementation with broken byte access" ,
The output would look better without capitalizing the first word. I think you can drop the first two words, saving some space:
GIC: enabling workaround for broken byte access
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422
Quoted 3 lines
drivers/irqchip/irq-gic.c +3
-0+ .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + },
Missing "{ /* sentinel */ }".
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1427
Quoted 5 lines
drivers/irqchip/irq-gic.c +2
-0+}; +static int gic_of_setup (struct gic_chip_data *gic,struct device_node *node) {if (!gic || !node)
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Geert, Mark, RMK, everyone,
Thanks for your efforts. Let me just chime in with a few details and a question.
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier
On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:GIC: enabling workaround for broken byte access
Indeed, byte access is unsupported according to the EMEV2 documentation.
The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.
From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"
"ICDIPTR"
So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.
Quoted 3 lines
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422
Quoted 3 lines
drivers/irqchip/irq-gic.c +3
-0+ .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + },
May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:arch/arm/boot/dts/emev2.dts Documentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.
Cheers,
/ magnus
Marc Zyngier
maintainerRe: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Magnus,
Quoted 14 lines · Magnus Damm
Hi Geert, Mark, RMK, everyone,
Thanks for your efforts. Let me just chime in with a few details and a question.
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier
On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:GIC: enabling workaround for broken byte access
Indeed, byte access is unsupported according to the EMEV2 documentation.
Quoted 7 lines
The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.
This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?
Quoted 8 lines
From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"
Renamed to GICD_IPRIORITYRn in IHI0048B.
Quoted 1 line
"ICDIPTR"
Renamed to GICD_ITARGETRn in IHI0048B.
See IHI0048B_b ("B.1 Alternative register names" and specifically table B-1) for the translation table between GICv1 and GICv2 names.
Quoted 2 lines
So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.
Which is definitely an integration bug. Both set of registers *must* support byte accesses. This isn't optional and left to the appreciation of the integrator. This breaks the programming model badly, and prevents standard software from running unmodified.
One of the few things the GIC architecture got right is the absence of locking requirements, as all the registers can be accessed concurrently by multiple CPUs as long as they operate on distinct interrupts. This is why the enable and pending registers have both set and clear accessors, that the priority and target registers are byte accessible, and that everything else happens in CPU-private registers (the CPU interface).
This requirement has been there from day-1. Even the good old DIC (the GIC's ancestor) that was included with the 11MP-Core says: "All Interrupt Distributor Registers are byte accessible.", which is more than actually necessary for the GIC. See DDI 0360F for details. And yes, SW written for the GIC does work on the DIC.
Quoted 10 lines
Quoted 4 lines
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422
Quoted 4 lines
drivers/irqchip/irq-gic.c +3
-0+ .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + },May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:
arch/arm/boot/dts/emev2.dtsDocumentation/devicetree/bindings/interrupt-controller/arm,gic.yaml
Quoted 5 lines
On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.
GICv1 is an architecture specification. PL390 is an implementation of GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is simply the amalgamation of the CPU interface implemented by the A9 (with the prototype of the GICv2 virtualisation extensions) with a distributor (usually a PL390, but not necessarily). All of them require that the priority and target registers are byte accessible.
As for changing the compatibility string, I don't see the point. This will break existing setups, and doesn't change the core of the issue. As far as I can see, the EMEV2 DT is correct in the sense that it describes the actual implementation of the GIC used.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Marc,
Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier
On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:GIC: enabling workaround for broken byte access
Indeed, byte access is unsupported according to the EMEV2 documentation.
Quoted 7 lines
The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.
Quoted 2 lines
This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?
Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)
EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.
Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS) R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC)
R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC)
Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC)
SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)
The INTCA/INTCS interrupt controllers came from the SH architecture but were phased out once SMP became the norm. I've got the majority of the boards above hooked up for remote access if anyone wants to test something.
Quoted 8 lines
Quoted 7 lines
From my basic research, "ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" is documented in ARM IHI 0048A from 2008 (Non-Confidential) which contains:
"All GIC registers are 32-bit wide." and "All registers support 32-bit word access..."
"In addition, the following registers support byte accesses:" "ICDIPR"
Quoted 8 lines
Renamed to
GICD_IPRIORITYRnin IHI0048B.Quoted 1 line
"ICDIPTR"
Renamed to
GICD_ITARGETRnin IHI0048B.See
IHI0048B_b("B.1 Alternative register names" and specifically table B-1) for the translation table between GICv1 and GICv2 names.
Thanks.
Quoted 7 lines
Quoted 2 lines
So the GICv1 documentation says byte access is partially supported however EMEV2 documentation says 32-bit access is required.
Which is definitely an integration bug. Both set of registers *must* support byte accesses. This isn't optional and left to the appreciation of the integrator. This breaks the programming model badly, and prevents standard software from running unmodified.
This reminds me that on SH we used to fix up I/O access alignment for certain on-chip devices by trapping. The fast path worked well and the special case worked but was slow.
Quoted 7 lines
One of the few things the GIC architecture got right is the absence of locking requirements, as all the registers can be accessed concurrently by multiple CPUs as long as they operate on distinct interrupts. This is why the enable and pending registers have both set and clear accessors, that the priority and target registers are byte accessible, and that everything else happens in CPU-private registers (the CPU interface).
Yeah the GIC is quite nice IMO. The legacy INTC hardware often had separate registers for setting and clearing, however priority probably required read-modify-write. SMP wasn't an issue. =)
Quoted 5 lines
This requirement has been there from day-1. Even the good old DIC (the GIC's ancestor) that was included with the 11MP-Core says: "All Interrupt Distributor Registers are byte accessible.", which is more than actually necessary for the GIC. See DDI 0360F for details. And yes, SW written for the GIC does work on the DIC.
Interesting. For some not so big reason this makes me think of Monty Python. =)
Quoted 16 lines
Quoted 10 lines
Quoted 4 lines
drivers/irqchip/irq-gic.c static bool gic_check_eoimode(struct device_node *node, void __iomem **base) · line 1422
Quoted 4 lines
drivers/irqchip/irq-gic.c +3
-0+ .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + },May I ask about a clarification about the EMEV2 DTS and DT binding documentation in:
arch/arm/boot/dts/emev2.dtsDocumentation/devicetree/bindings/interrupt-controller/arm,gic.yamlQuoted 5 lines
On EMEV2 the DT compatible string currently seems to be the rather generic "arm,pl390". In the DT binding documentation GICv1 is listed in an example as "arm,cortex-a9-gic". Is there any reason for not using the GICv1 compatible string (and 32-bit access) for EMEV2? Just curious.
Quoted 6 lines
GICv1 is an architecture specification. PL390 is an implementation of GICv1. The so called "Cortex-A9 GIC" doesn't really exist. It is simply the amalgamation of the CPU interface implemented by the A9 (with the prototype of the GICv2 virtualisation extensions) with a distributor (usually a PL390, but not necessarily). All of them require that the priority and target registers are byte accessible.
Makes sense.
Quoted 4 lines
As for changing the compatibility string, I don't see the point. This will break existing setups, and doesn't change the core of the issue. As far as I can see, the EMEV2 DT is correct in the sense that it describes the actual implementation of the GIC used.
I'm all for not breaking existing setups, but EMEV2 is a pretty rare case. So my line of thinking was that instead of punishing all GIC platforms with this EMEV2-specific workaround it is completely fine from my side to to special case it in DT if it makes the rest of the code any cleaner. But it is really up to you guys.
Thanks,
/ magnus
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Magnus,
Quoted 23 lines · Magnus Damm
Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier
On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:GIC: enabling workaround for broken byte access
Indeed, byte access is unsupported according to the EMEV2 documentation.
Quoted 7 lines
The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.Quoted 2 lines
This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?
Quoted 8 lines
Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)
EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.
Indeed. I used to work on products using EMMA1 and EMMA2, and they were MIPS-based (vr4120A for EMMA2, IIRC). Later variants (EMMA2H and EMMA3?) did include a small ARM core for standby control.
Quoted 2 lines
Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)
This is no longer supported upstream (and not affected, as no GIC).
Quoted 1 line
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)
R-Mobile A1 has GIC (PL390), too, and is not affected.
Quoted 4 lines
R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC) R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC) Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC) SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)
All of these (except for EMEV2) are fine, too.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Hi Geert, everyone,
Quoted 36 lines · Geert Uytterhoeven
Hi Magnus,
Quoted 23 lines · Magnus Damm
Quoted 19 lines · Marc Zyngier
Quoted 9 lines · Magnus Damm
Quoted 4 lines · Geert Uytterhoeven
Quoted 2 lines · Marc Zyngier
On Thu, 09 Sep 2021 16:22:01 +0100,
Geert Uytterhoeven <[email protected]> wrote:GIC: enabling workaround for broken byte access
Indeed, byte access is unsupported according to the EMEV2 documentation.
Quoted 7 lines
The EMEV2 documentation R19UH0036EJ0600 Chapter 7 Interrupt Control on page 97 says:
"Interrupt registers can be accessed via the APB bus, in 32-bit units" "For details about register functions, see ARM Generic Interrupt Controller Architecture Specification Architecture version 1.0" The file "R19UH0036EJ0600_1Chip.pdf" is the 6th edition version published in 2010 and is not marked as confidential.Quoted 2 lines
This is as bad as it gets. Do you know if any other Renesas platform is affected by the same issue?
Quoted 8 lines
Next time we have a beer together I would be happy to show you some legacy interrupt controller code. =)
EMEV2 and the Emma Mobile product line came from the NEC Electronics side that got merged into Renesas Electronics in 2010. Historically NEC Electronics mainly used MIPS I've been told, and the Emma Mobile SoCs were one of the earlier Cortex-A9 adopters. That might have something to do with the rather loose interpretation of the spec.
Quoted 3 lines
Indeed. I used to work on products using EMMA1 and EMMA2, and they were MIPS-based (vr4120A for EMMA2, IIRC). Later variants (EMMA2H and EMMA3?) did include a small ARM core for standby control.
Thanks for sharing some more background!
Quoted 4 lines
Quoted 2 lines
Renesas SoCs from a similar era:
AP4 (sh7372) AP4EVB (Cortex-A8 + INTCA/INTCS)This is no longer supported upstream (and not affected, as no GIC).
Right. I might mix it up with the AP4.5 chip that I used for SMP prototyping back then. It had 4 x CA9 and obviously a GIC.
Quoted 10 lines
Quoted 1 line
R-Mobile A1 (r8a7740) Armadillo-800-EVA (Cortex-A9 + INTCA/INTCS)
R-Mobile A1 has GIC (PL390), too, and is not affected.
Quoted 4 lines
R-Car M1A (r8a7778) Bock-W (Cortex-A9 + GIC) R-Car H1 (r8a7779) Marzen (4 x Cortex-A9 + GIC) Emma Mobile EMEV2 KZM9D (2 x Cortex-A9 + GIC) SH-Mobile AG5 (sh73a0) KZM9G (2 x Cortex-A9 + GIC)
All of these (except for EMEV2) are fine, too.
Thanks for checking!
Cheers,
/ magnus
[irqchip: irq/irqchip-fixes] irqchip/gic: Work around broken Renesas integration
In reply to Geert UytterhoevenThe following commit has been merged into the irq/irqchip-fixes branch of irqchip:
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: b78f26926b17cc289e4f16b63363abe0aa2e8efc Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/b78f26926b17cc289e4f16b63363abe0aa2e8efc Author: Marc Zyngier <[email protected]> AuthorDate: Fri, 10 Sep 2021 18:29:25 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Wed, 22 Sep 2021 14:44:25 +01:00
irqchip/gic: Work around broken Renesas integration
Geert reported that the GIC driver locks up on a Renesas system since 005c34ae4b44f085 ("irqchip/gic: Atomically update affinity") fixed the driver to use writeb_relaxed() instead of writel_relaxed().
As it turns out, the interconnect used on this system mandates 32bit wide accesses for all MMIO transactions, even if the GIC architecture specifically mandates for some registers to be byte accessible. Gahhh...
Work around the issue by crudly detecting the offending system, and falling back to an inefficient RMW+lock implementation.
--- drivers/irqchip/irq-gic.c | 52 +++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)
drivers/irqchip/irq-gic.c +51 -1
@@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock);#endif +static DEFINE_STATIC_KEY_FALSE (needs_rmw_access); +/* * The GIC mapping of CPU interfaces does not necessarily match * the logical CPU numbering. Let's use a mapping as returned
@@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)#endif #ifdef CONFIG_SMP +static void rmw_writeb (u8 bval,void __iomem *addr) +{ +static DEFINE_RAW_SPINLOCK (rmw_lock); +unsigned long offset = (unsigned long )addr &3UL ; +unsigned long shift = offset *8 ; +unsigned long flags; +u32 val; + +raw_spin_lock_irqsave (&rmw_lock, flags); + + addr -= offset; + val =readl_relaxed (addr); + val &= ~GENMASK (shift +7 , shift); + val |= bval << shift; +writel_relaxed (val, addr); + +raw_spin_unlock_irqrestore (&rmw_lock, flags); +} +static int gic_set_affinity (struct irq_data *d,const struct cpumask *mask_val,bool force) {
@@ -788,7 +809,10 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ; -writeb_relaxed (gic_cpu_map[cpu], reg); +if (static_branch_unlikely (&needs_rmw_access)) +rmw_writeb (gic_cpu_map[cpu], reg); +else +writeb_relaxed (gic_cpu_map[cpu], reg);irq_data_update_effective_affinity (d,cpumask_of (cpu));return IRQ_SET_MASK_OK_DONE ;
@@ -1375,6 +1399,30 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)return true; } +static bool gic_enable_rmw_access (void *data) +{ +/* +* The EMEV2 class of machines has a broken interconnect, and +* locks up on accesses that are less than 32bit. So far, only +* the affinity setting requires it. +*/ +if (of_machine_is_compatible ("renesas,emev2" )) { +static_branch_enable (&needs_rmw_access); +return true; + } + +return false; +} + +static const struct gic_quirk gic_quirks[] = { + { + .desc ="broken byte access" , + .compatible ="arm,pl390" , + .init = gic_enable_rmw_access, + }, + { }, +}; +static int gic_of_setup (struct gic_chip_data *gic,struct device_node *node) {if (!gic || !node)
@@ -1391,6 +1439,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)if (of_property_read_u32 (node,"cpu-offset" , &gic->percpu_offset)) gic->percpu_offset =0 ; +gic_enable_of_quirks (node, gic_quirks, gic); +return 0 ; error:
Marc Zyngier
maintainerPATCH v2 8/17 irqchip/gic: Refactor SMP configuration
In reply to Marc ZyngierAs we are about to change quite a lot of the SMP support code, let's start by moving it around so that it minimizes the amount of #ifdefery.
--- drivers/irqchip/irq-gic.c | 76 ++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 36 deletions(-)
drivers/irqchip/irq-gic.c +40 -36
@@ -325,28 +325,6 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)return 0 ; } -#ifdef CONFIG_SMP -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); -unsigned int cpu; - -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else - cpu =cpumask_first (mask_val); - -if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) -return -EINVAL ; - -writeb_relaxed (gic_cpu_map[cpu], reg); -irq_data_update_effective_affinity (d,cpumask_of (cpu)); - -return IRQ_SET_MASK_OK_DONE ; -} -#endif - static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) {u32 irqstat, irqnr;
@@ -795,6 +773,26 @@ static int gic_pm_init(struct gic_chip_data *gic)#endif #ifdef CONFIG_SMP +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); +unsigned int cpu; + +if (!force) + cpu =cpumask_any_and (mask_val, cpu_online_mask); +else + cpu =cpumask_first (mask_val); + +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +return -EINVAL ; + +writeb_relaxed (gic_cpu_map[cpu], reg); +irq_data_update_effective_affinity (d,cpumask_of (cpu)); + +return IRQ_SET_MASK_OK_DONE ; +} +static void gic_raise_softirq (const struct cpumask *mask,unsigned int irq) {int cpu;
@@ -824,6 +822,23 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) gic_unlock_irqrestore(flags); } + +static int gic_starting_cpu (unsigned int cpu) +{ +gic_cpu_init (&gic_data[0 ]); +return 0 ; +} + +static __init void gic_smp_init (void ) +{ +set_smp_cross_call (gic_raise_softirq); +cpuhp_setup_state_nocalls (CPUHP_AP_IRQ_GIC_STARTING , +"irqchip/arm/gic:starting" , + gic_starting_cpu,NULL ); +} +#else +#define gic_smp_init () do { } while(0) +#define gic_set_affinity NULL#endif #ifdef CONFIG_BL_SWITCHER
@@ -1027,12 +1042,6 @@ static int gic_irq_domain_translate(struct irq_domain *d,return -EINVAL ; } -static int gic_starting_cpu (unsigned int cpu) -{ -gic_cpu_init (&gic_data[0 ]); -return 0 ; -} -static int gic_irq_domain_alloc (struct irq_domain *domain,unsigned int virq,unsigned int nr_irqs,void *arg) {
@@ -1079,10 +1088,8 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; } -#ifdef CONFIG_SMP if (gic == &gic_data[0 ]) gic->chip.irq_set_affinity = gic_set_affinity; -#endif } static int gic_init_bases(struct gic_chip_data *gic,
@@ -1199,12 +1206,7 @@ static int __init __gic_init_bases(struct gic_chip_data *gic,*/ for (i =0 ; i <NR_GIC_CPU_IF ; i++) gic_cpu_map[i] =0xff ; -#ifdef CONFIG_SMP -set_smp_cross_call (gic_raise_softirq); -#endif -cpuhp_setup_state_nocalls (CPUHP_AP_IRQ_GIC_STARTING , -"irqchip/arm/gic:starting" , - gic_starting_cpu,NULL ); +set_handle_irq (gic_handle_irq);if (static_branch_likely (&supports_deactivate_key))pr_info ("GIC: Using split EOI/Deactivate mode\n" );
@@ -1221,6 +1223,8 @@ static int __init __gic_init_bases(struct gic_chip_data *gic, ret =gic_init_bases (gic, handle);if (ret)kfree (name); +else if (gic == &gic_data[0 ]) +gic_smp_init ();return ret; }
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 9/17 irqchip/gic: Configure SGIs as standard interrupts
In reply to Marc ZyngierChange the way we deal with GIC SGIs by turning them into proper IRQs, and calling into the arch code to register the interrupt range instead of a callback.
--- drivers/irqchip/irq-gic.c | 101 +++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 39 deletions(-)
drivers/irqchip/irq-gic.c +62 -39
@@ -335,31 +335,22 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs) irqstat =readl_relaxed (cpu_base +GIC_CPU_INTACK ); irqnr = irqstat &GICC_IAR_INT_ID_MASK ; -if (likely (irqnr >15 && irqnr <1020 )) { -if (static_branch_likely (&supports_deactivate_key)) -writel_relaxed (irqstat, cpu_base +GIC_CPU_EOI ); -isb (); -handle_domain_irq (gic->domain, irqnr, regs); -continue ; - } -if (irqnr <16 ) { +if (unlikely (irqnr >=1020 )) +break ; + +if (static_branch_likely (&supports_deactivate_key))writel_relaxed (irqstat, cpu_base +GIC_CPU_EOI ); -if (static_branch_likely (&supports_deactivate_key)) -writel_relaxed (irqstat, cpu_base +GIC_CPU_DEACTIVATE ); -#ifdef CONFIG_SMP -/* -* Ensure any shared data written by the CPU sending -* the IPI is read after we've read the ACK register -* on the GIC. -* -* Pairs with the write barrier in gic_raise_softirq -*/ +isb (); + +/* +* Ensure any shared data written by the CPU sending the IPI +* is read after we've read the ACK register on the GIC. +* +* Pairs with the write barrier in gic_ipi_send_mask +*/ +if (irqnr <=15 )smp_rmb (); -handle_IPI (irqnr, regs); -#endif -continue ; - } -break ; +handle_domain_irq (gic->domain, irqnr, regs); }while (1 ); }
@@ -793,14 +784,14 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,return IRQ_SET_MASK_OK_DONE ; } -static void gic_raise_softirq (const struct cpumask *mask,unsigned int irq) +static void gic_ipi_send_mask (struct irq_data *d,const struct cpumask *mask) {int cpu;unsigned long flags, map =0 ;if (unlikely (nr_cpu_ids ==1 )) {/* Only one CPU? let's do a self-IPI... */ -writel_relaxed (2 <<24 | irq, +writel_relaxed (2 <<24 | d->hwirq,gic_data_dist_base (&gic_data[0 ]) +GIC_DIST_SOFTINT );return ; }
@@ -818,7 +809,7 @@ 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 ); +writel_relaxed (map <<16 | d->hwirq,gic_data_dist_base (&gic_data[0 ]) +GIC_DIST_SOFTINT );gic_unlock_irqrestore (flags); }
@@ -831,14 +822,28 @@ static int gic_starting_cpu(unsigned int cpu)static __init void gic_smp_init (void ) { -set_smp_cross_call (gic_raise_softirq); +struct irq_fwspec sgi_fwspec = { + .fwnode = gic_data[0 ].domain->fwnode, + .param_count =1 , + }; +int base_sgi; +cpuhp_setup_state_nocalls (CPUHP_AP_IRQ_GIC_STARTING ,"irqchip/arm/gic:starting" , gic_starting_cpu,NULL ); + + base_sgi =__irq_domain_alloc_irqs (gic_data[0 ].domain,-1 ,8 , +NUMA_NO_NODE , &sgi_fwspec, + false,NULL ); +if (WARN_ON (base_sgi <=0 )) +return ; + +set_smp_ipi_range (base_sgi,8 ); }#else #define gic_smp_init () do { } while(0)#define gic_set_affinity NULL +#define gic_ipi_send_mask NULL#endif #ifdef CONFIG_BL_SWITCHER
@@ -985,15 +990,24 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq, {struct gic_chip_data *gic = d->host_data; -if (hw <32 ) { +switch (hw) { + case 0 ... 15: +irq_set_percpu_devid (irq); +irq_domain_set_info (d, irq, hw, &gic->chip, d->host_data, + handle_percpu_devid_fasteoi_ipi, +NULL ,NULL ); +break ; + case 16 ... 31:irq_set_percpu_devid (irq);irq_domain_set_info (d, irq, hw, &gic->chip, d->host_data, handle_percpu_devid_irq,NULL ,NULL ); - }else { +break ; +default :irq_domain_set_info (d, irq, hw, &gic->chip, d->host_data, handle_fasteoi_irq,NULL ,NULL );irq_set_probe (irq);irqd_set_single_target (irq_desc_get_irq_data (irq_to_desc (irq))); +break ; }return 0 ; }
@@ -1007,19 +1021,26 @@ static int gic_irq_domain_translate(struct irq_domain *d, unsigned long *hwirq, unsigned int *type) { +if (fwspec->param_count ==1 && fwspec->param[0 ] <16 ) { + *hwirq = fwspec->param[0 ]; + *type =IRQ_TYPE_EDGE_RISING ; +return 0 ; + } +if (is_of_node (fwspec->fwnode)) {if (fwspec->param_count <3 )return -EINVAL ; -/* Get the interrupt number and add 16 to skip over SGIs */ - *hwirq = fwspec->param[1 ] +16 ; - -/* -* For SPIs, we need to add 16 more to get the GIC irq -* ID number -*/ -if (!fwspec->param[0 ]) - *hwirq +=16 ; +switch (fwspec->param[0 ]) { +case 0 :/* SPI */ + *hwirq = fwspec->param[1 ] +32 ; +break ; +case 1 :/* PPI */ + *hwirq = fwspec->param[1 ] +16 ; +break ; +default : +return -EINVAL ; + } *type = fwspec->param[2 ] &IRQ_TYPE_SENSE_MASK ;
@@ -1088,8 +1109,10 @@ static void gic_init_chip(struct gic_chip_data *gic, struct device *dev, gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity; } -if (gic == &gic_data[0 ]) +if (gic == &gic_data[0 ]) { gic->chip.irq_set_affinity = gic_set_affinity; + gic->chip.ipi_send_mask = gic_ipi_send_mask; + } } static int gic_init_bases(struct gic_chip_data *gic,
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 10/17 irqchip/gic-common: Don't enable SGIs by default
In reply to Marc ZyngierThe architecture code now enables the IPIs as required, so no need to enable SGIs by default in the GIC code.
--- drivers/irqchip/irq-gic-common.c | 3 --- 1 file changed, 3 deletions(-)
drivers/irqchip/irq-gic-common.c +0 -3
@@ -152,9 +152,6 @@ void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void))writel_relaxed (GICD_INT_DEF_PRI_X4 , base +GIC_DIST_PRI + i *4 /4 ); -/* Ensure all SGI interrupts are now enabled */ -writel_relaxed (GICD_INT_EN_SET_SGI , base +GIC_DIST_ENABLE_SET ); -if (sync_access)sync_access (); }
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 11/17 irqchip/bcm2836: Configure mailbox interrupts as standard interrupts
In reply to Marc ZyngierIn order to switch the bcm2836 driver to privide standard interrupts for IPIs, it first needs to stop lying about the way things work.
The mailbox interrupt is actually a multiplexer, with enough bits to store 32 pending interrupts per CPU. So let's turn it into a chained irqchip.
Once this is done, we can instanciate the corresponding IPIs, and pass them to the architecture code.
--- drivers/irqchip/irq-bcm2836.c | 151 ++++++++++++++++++++++++++++------ 1 file changed, 125 insertions(+), 26 deletions(-)
drivers/irqchip/irq-bcm2836.c +125 -26
@@ -10,6 +10,7 @@#include <linux/of_irq.h> #include <linux/irqchip.h> #include <linux/irqdomain.h> +#include <linux/irqchip/chained_irq.h> #include <linux/irqchip/irq-bcm2836.h> #include <asm/exception.h>
@@ -89,12 +90,24 @@ static struct irq_chip bcm2836_arm_irqchip_gpu = { .irq_unmask = bcm2836_arm_irqchip_unmask_gpu_irq, }; +static void bcm2836_arm_irqchip_dummy_op (struct irq_data *d) +{ +} + +static struct irq_chip bcm2836_arm_irqchip_dummy = { + .name ="bcm2836-dummy" , + .irq_eoi = bcm2836_arm_irqchip_dummy_op, +}; +static int bcm2836_map (struct irq_domain *d,unsigned int irq,irq_hw_number_t hw) {struct irq_chip *chip;switch (hw) { +case LOCAL_IRQ_MAILBOX0 : + chip = &bcm2836_arm_irqchip_dummy; +break ;case LOCAL_IRQ_CNTPSIRQ :case LOCAL_IRQ_CNTPNSIRQ :case LOCAL_IRQ_CNTHPIRQ :
@@ -127,17 +140,7 @@ __exception_irq_entry bcm2836_arm_irqchip_handle_irq(struct pt_regs *regs)u32 stat; stat =readl_relaxed (intc.base +LOCAL_IRQ_PENDING0 +4 * cpu); -if (stat &BIT (LOCAL_IRQ_MAILBOX0 )) { -#ifdef CONFIG_SMP -void __iomem *mailbox0 = (intc.base + -LOCAL_MAILBOX0_CLR0 +16 * cpu); -u32 mbox_val =readl (mailbox0); -u32 ipi =ffs (mbox_val) -1 ; - -writel (1 << ipi, mailbox0); -handle_IPI (ipi, regs); -#endif - }else if (stat) { +if (stat) {u32 hwirq =ffs (stat) -1 ;handle_domain_irq (intc.domain, hwirq, regs);
@@ -145,8 +148,35 @@ __exception_irq_entry bcm2836_arm_irqchip_handle_irq(struct pt_regs *regs) }#ifdef CONFIG_SMP -static void bcm2836_arm_irqchip_send_ipi (const struct cpumask *mask, -unsigned int ipi) +static struct irq_domain *ipi_domain; + +static void bcm2836_arm_irqchip_handle_ipi (struct irq_desc *desc) +{ +struct irq_chip *chip =irq_desc_get_chip (desc); +int cpu =smp_processor_id (); +u32 mbox_val; + +chained_irq_enter (chip, desc); + + mbox_val =readl_relaxed (intc.base +LOCAL_MAILBOX0_CLR0 +16 * cpu); +if (mbox_val) { +int hwirq =ffs (mbox_val) -1 ; +generic_handle_irq (irq_find_mapping (ipi_domain, hwirq)); + } + +chained_irq_exit (chip, desc); +} + +static void bcm2836_arm_irqchip_ipi_eoi (struct irq_data *d) +{ +int cpu =smp_processor_id (); + +writel_relaxed (BIT (d->hwirq), + intc.base +LOCAL_MAILBOX0_CLR0 +16 * cpu); +} + +static void bcm2836_arm_irqchip_ipi_send_mask (struct irq_data *d, +const struct cpumask *mask) {int cpu;void __iomem *mailbox0_base = intc.base +LOCAL_MAILBOX0_SET0 ;
@@ -157,11 +187,45 @@ static void bcm2836_arm_irqchip_send_ipi(const struct cpumask *mask,*/ smp_wmb (); -for_each_cpu (cpu, mask) { -writel (1 << ipi, mailbox0_base +16 * cpu); +for_each_cpu (cpu, mask) +writel_relaxed (BIT (d->hwirq), mailbox0_base +16 * cpu); +} + +static struct irq_chip bcm2836_arm_irqchip_ipi = { + .name ="IPI" , + .irq_eoi = bcm2836_arm_irqchip_ipi_eoi, + .ipi_send_mask = bcm2836_arm_irqchip_ipi_send_mask, +}; + +static int bcm2836_arm_irqchip_ipi_alloc (struct irq_domain *d, +unsigned int virq, +unsigned int nr_irqs,void *args) +{ +int i; + +for (i =0 ; i < nr_irqs; i++) { +irq_set_percpu_devid (virq + i); +irq_domain_set_info (d, virq + i, i, &bcm2836_arm_irqchip_ipi, + d->host_data, + handle_percpu_devid_fasteoi_ipi, +NULL ,NULL ); } + +return 0 ; } +static void bcm2836_arm_irqchip_ipi_free (struct irq_domain *d, +unsigned int virq, +unsigned int nr_irqs) +{ +/* Not freeing IPIs */ +} + +static const struct irq_domain_ops ipi_domain_ops = { + .alloc = bcm2836_arm_irqchip_ipi_alloc, + .free = bcm2836_arm_irqchip_ipi_free, +}; +static int bcm2836_cpu_starting (unsigned int cpu) { bcm2836_arm_irqchip_unmask_per_cpu_irq(LOCAL_MAILBOX_INT_CONTROL0, 0,
@@ -175,25 +239,58 @@ static int bcm2836_cpu_dying(unsigned int cpu) cpu);return 0 ; } -#endif -static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = { - .xlate = irq_domain_xlate_onetwocell, - .map = bcm2836_map, -}; +#define BITS_PER_MBOX 32 -static void -bcm2836_arm_irqchip_smp_init (void ) +static void bcm2836_arm_irqchip_smp_init (void ) { -#ifdef CONFIG_SMP +struct irq_fwspec ipi_fwspec = { + .fwnode = intc.domain->fwnode, + .param_count =1 , + .param = { + [0 ] =LOCAL_IRQ_MAILBOX0 , + }, + }; +int base_ipi, mux_irq; + + mux_irq =irq_create_fwspec_mapping (&ipi_fwspec); +if (WARN_ON (mux_irq <=0 )) +return ; + + ipi_domain =irq_domain_create_linear (intc.domain->fwnode, +BITS_PER_MBOX , &ipi_domain_ops, +NULL ); +if (WARN_ON (!ipi_domain)) +return ; + + ipi_domain->flags |=IRQ_DOMAIN_FLAG_IPI_SINGLE ; +irq_domain_update_bus_token (ipi_domain,DOMAIN_BUS_IPI ); + + base_ipi =__irq_domain_alloc_irqs (ipi_domain,-1 ,BITS_PER_MBOX , +NUMA_NO_NODE ,NULL , + false,NULL ); + +if (WARN_ON (!base_ipi)) +return ; + +set_smp_ipi_range (base_ipi,BITS_PER_MBOX ); + +irq_set_chained_handler_and_data (mux_irq, + bcm2836_arm_irqchip_handle_ipi,NULL ); +/* Unmask IPIs to the boot CPU. */ cpuhp_setup_state (CPUHP_AP_IRQ_BCM2836_STARTING ,"irqchip/bcm2836:starting" , bcm2836_cpu_starting, bcm2836_cpu_dying); - -set_smp_cross_call (bcm2836_arm_irqchip_send_ipi); -#endif } +#else +#define bcm2836_arm_irqchip_smp_init () do { } while(0) +#endif + +static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = { + .xlate = irq_domain_xlate_onetwocell, + .map = bcm2836_map, +};/* * The LOCAL_IRQ_CNT* timer firings are based off of the external
@@ -232,6 +329,8 @@ static int __init bcm2836_arm_irqchip_l1_intc_of_init(struct device_node *node,if (!intc.domain)panic ("%pOF: unable to create IRQ domain\n" , node); +irq_domain_update_bus_token (intc.domain,DOMAIN_BUS_WIRED ); +bcm2836_arm_irqchip_smp_init ();set_handle_irq (bcm2836_arm_irqchip_handle_irq);
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 12/17 irqchip/hip04: Configure IPIs as standard interrupts
In reply to Marc ZyngierIn order to switch the hip04 driver to provide standard interrupts for IPIs, rework the way interrupts are allocated, making sure the irqdomain covers the SGIs as well as the rest of the interrupt range.
The driver is otherwise so old-school that it creates all interrupts upfront (duh!), so there is hardly anything else to change, apart from communicating the IPIs to the arch code.
--- drivers/irqchip/irq-hip04.c | 89 +++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 49 deletions(-)
drivers/irqchip/irq-hip04.c +40 -49
@@ -171,6 +171,29 @@ static int hip04_irq_set_affinity(struct irq_data *d,return IRQ_SET_MASK_OK ; } + +static void hip04_ipi_send_mask (struct irq_data *d,const struct cpumask *mask) +{ +int cpu; +unsigned long flags, map =0 ; + +raw_spin_lock_irqsave (&irq_controller_lock, flags); + +/* Convert our logical CPU mask into a physical one. */ +for_each_cpu (cpu, mask) + map |= hip04_cpu_map[cpu]; + +/* +* Ensure that stores to Normal memory are visible to the +* other CPUs before they observe us issuing the IPI. +*/ +dmb (ishst); + +/* this always happens on GIC0 */ +writel_relaxed (map <<8 | d->hwirq, hip04_data.dist_base +GIC_DIST_SOFTINT ); + +raw_spin_unlock_irqrestore (&irq_controller_lock, flags); +}#endif static void __exception_irq_entry hip04_handle_irq(struct pt_regs *regs)
@@ -182,19 +205,9 @@ static void __exception_irq_entry hip04_handle_irq(struct pt_regs *regs) irqstat =readl_relaxed (cpu_base +GIC_CPU_INTACK ); irqnr = irqstat &GICC_IAR_INT_ID_MASK ; -if (likely (irqnr >15 && irqnr <=HIP04_MAX_IRQS )) { +if (irqnr <=HIP04_MAX_IRQS )handle_domain_irq (hip04_data.domain, irqnr, regs); -continue ; - } -if (irqnr <16 ) { -writel_relaxed (irqstat, cpu_base +GIC_CPU_EOI ); -#ifdef CONFIG_SMP -handle_IPI (irqnr, regs); -#endif -continue ; - } -break ; - }while (1 ); + }while (irqnr >HIP04_MAX_IRQS ); }static struct irq_chip hip04_irq_chip = {
@@ -205,6 +218,7 @@ static struct irq_chip hip04_irq_chip = { .irq_set_type = hip04_irq_set_type, #ifdef CONFIG_SMP .irq_set_affinity = hip04_irq_set_affinity, + .ipi_send_mask = hip04_ipi_send_mask, #endif .flags =IRQCHIP_SET_TYPE_MASKED | IRQCHIP_SKIP_SET_WAKE |
@@ -279,39 +293,17 @@ static void hip04_irq_cpu_init(struct hip04_irq_data *intc)writel_relaxed (1 , base +GIC_CPU_CTRL ); } -#ifdef CONFIG_SMP -static void hip04_raise_softirq (const struct cpumask *mask,unsigned int irq) -{ -int cpu; -unsigned long flags, map =0 ; - -raw_spin_lock_irqsave (&irq_controller_lock, flags); - -/* Convert our logical CPU mask into a physical one. */ -for_each_cpu (cpu, mask) - map |= hip04_cpu_map[cpu]; - -/* -* Ensure that stores to Normal memory are visible to the -* other CPUs before they observe us issuing the IPI. -*/ -dmb (ishst); - -/* this always happens on GIC0 */ -writel_relaxed (map <<8 | irq, hip04_data.dist_base +GIC_DIST_SOFTINT ); - -raw_spin_unlock_irqrestore (&irq_controller_lock, flags); -} -#endif -static int hip04_irq_domain_map (struct irq_domain *d,unsigned int irq,irq_hw_number_t hw) { -if (hw <32 ) { +if (hw <16 ) { +irq_set_percpu_devid (irq); +irq_set_chip_and_handler (irq, &hip04_irq_chip, + handle_percpu_devid_fasteoi_ipi); + }else if (hw <32 ) {irq_set_percpu_devid (irq);irq_set_chip_and_handler (irq, &hip04_irq_chip, handle_percpu_devid_irq); -irq_set_status_flags (irq,IRQ_NOAUTOEN ); }else {irq_set_chip_and_handler (irq, &hip04_irq_chip, handle_fasteoi_irq);
@@ -328,10 +320,13 @@ static int hip04_irq_domain_xlate(struct irq_domain *d, unsigned long *out_hwirq, unsigned int *out_type) { - unsigned long ret = 0; - if (irq_domain_get_of_node(d) != controller) return -EINVAL; +if (intsize ==1 && intspec[0 ] <16 ) { + *out_hwirq = intspec[0 ]; + *out_type =IRQ_TYPE_EDGE_RISING ; +return 0 ; + }if (intsize <3 )return -EINVAL ;
@@ -344,7 +339,7 @@ static int hip04_irq_domain_xlate(struct irq_domain *d, *out_type = intspec[2 ] &IRQ_TYPE_SENSE_MASK ; -return ret; +return 0 ; }static int hip04_irq_starting_cpu (unsigned int cpu)
@@ -361,7 +356,6 @@ static const struct irq_domain_ops hip04_irq_domain_ops = {static int __init hip04_of_init (struct device_node *node,struct device_node *parent) { -irq_hw_number_t hwirq_base =16 ;int nr_irqs, irq_base, i;if (WARN_ON (!node))
@@ -390,24 +384,21 @@ hip04_of_init(struct device_node *node, struct device_node *parent) nr_irqs =HIP04_MAX_IRQS ; hip04_data.nr_irqs = nr_irqs; - nr_irqs -= hwirq_base;/* calculate # of irqs to allocate */ - - irq_base =irq_alloc_descs (-1 , hwirq_base, nr_irqs,numa_node_id ()); + irq_base =irq_alloc_descs (-1 ,0 , nr_irqs,numa_node_id ());if (irq_base <0 ) {pr_err ("failed to allocate IRQ numbers\n" );return -EINVAL ; } hip04_data.domain =irq_domain_add_legacy (node, nr_irqs, irq_base, - hwirq_base, +0 , &hip04_irq_domain_ops, &hip04_data); -if (WARN_ON (!hip04_data.domain))return -EINVAL ;#ifdef CONFIG_SMP -set_smp_cross_call (hip04_raise_softirq); +set_smp_ipi_range (irq_base,16 );#endif set_handle_irq (hip04_handle_irq);
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 13/17 irqchip/armada-370-xp: Configure IPIs as standard interrupts
In reply to Marc ZyngierTo introduce IPIs as standard interrupts to the Armada 370-XP driver, let's allocate a completely separate irqdomain and irqchip combo that lives parallel to the "standard" one.
This effectively should be modelled as a chained interrupt controller, but the code is in such a state that it is pretty hard to shoehorn, as it would require the rewrite of the MSI layer as well.
--- drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++--------- 1 file changed, 178 insertions(+), 84 deletions(-)
drivers/irqchip/irq-armada-370-xp.c +178 -84
@@ -310,7 +310,134 @@ static inline int armada_370_xp_msi_init(struct device_node *node, }#endif +static void armada_xp_mpic_perf_init (void ) +{ +unsigned long cpuid =cpu_logical_map (smp_processor_id ()); + +/* Enable Performance Counter Overflow interrupts */ +writel (ARMADA_370_XP_INT_CAUSE_PERF (cpuid), + per_cpu_int_base +ARMADA_370_XP_INT_FABRIC_MASK_OFFS ); +} +#ifdef CONFIG_SMP +static struct irq_domain *ipi_domain; + +static void armada_370_xp_ipi_mask (struct irq_data *d) +{ +u32 reg; + reg =readl (per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); + reg &= ~BIT (d->hwirq); +writel (reg, per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); +} + +static void armada_370_xp_ipi_unmask (struct irq_data *d) +{ +u32 reg; + reg =readl (per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); + reg |=BIT (d->hwirq); +writel (reg, per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); +} + +static void armada_370_xp_ipi_send_mask (struct irq_data *d, +const struct cpumask *mask) +{ +unsigned long map =0 ; +int cpu; + +/* Convert our logical CPU mask into a physical one. */ +for_each_cpu (cpu, mask) + map |=1 <<cpu_logical_map (cpu); + +/* +* Ensure that stores to Normal memory are visible to the +* other CPUs before issuing the IPI. +*/ +dsb (); + +/* submit softirq */ +writel ((map <<8 ) | d->hwirq, main_int_base + +ARMADA_370_XP_SW_TRIG_INT_OFFS ); +} + +static void armada_370_xp_ipi_eoi (struct irq_data *d) +{ +writel (~BIT (d->hwirq), per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS ); +} + +static struct irq_chip ipi_irqchip = { + .name ="IPI" , + .irq_mask = armada_370_xp_ipi_mask, + .irq_unmask = armada_370_xp_ipi_unmask, + .irq_eoi = armada_370_xp_ipi_eoi, + .ipi_send_mask = armada_370_xp_ipi_send_mask, +}; + +static int armada_370_xp_ipi_alloc (struct irq_domain *d, +unsigned int virq, +unsigned int nr_irqs,void *args) +{ +int i; + +for (i =0 ; i < nr_irqs; i++) { +irq_set_percpu_devid (virq + i); +irq_domain_set_info (d, virq + i, i, &ipi_irqchip, + d->host_data, + handle_percpu_devid_fasteoi_ipi, +NULL ,NULL ); + } + +return 0 ; +} + +static void armada_370_xp_ipi_free (struct irq_domain *d, +unsigned int virq, +unsigned int nr_irqs) +{ +/* Not freeing IPIs */ +} + +static const struct irq_domain_ops ipi_domain_ops = { + .alloc = armada_370_xp_ipi_alloc, + .free = armada_370_xp_ipi_free, +}; + +static void ipi_resume (void ) +{ +int i; + +for (i =0 ; i <IPI_DOORBELL_END ; i++) { +int irq; + + irq =irq_find_mapping (ipi_domain, i); +if (irq <=0 ) +continue ; +if (irq_percpu_is_enabled (irq)) { +struct irq_data *d; + d =irq_domain_get_irq_data (ipi_domain, irq); +armada_370_xp_ipi_unmask (d); + } + } +} + +static __init void armada_xp_ipi_init (struct device_node *node) +{ +int base_ipi; + + ipi_domain =irq_domain_create_linear (of_node_to_fwnode (node), +IPI_DOORBELL_END , + &ipi_domain_ops,NULL ); +if (WARN_ON (!ipi_domain)) +return ; + +irq_domain_update_bus_token (ipi_domain,DOMAIN_BUS_IPI ); + base_ipi =__irq_domain_alloc_irqs (ipi_domain,-1 ,IPI_DOORBELL_END , +NUMA_NO_NODE ,NULL , false,NULL ); +if (WARN_ON (!base_ipi)) +return ; + +set_smp_ipi_range (base_ipi,IPI_DOORBELL_END ); +} +static DEFINE_RAW_SPINLOCK (irq_controller_lock); static int armada_xp_set_affinity(struct irq_data *d,
@@ -334,43 +461,6 @@ static int armada_xp_set_affinity(struct irq_data *d,return IRQ_SET_MASK_OK ; } -#endif - -static struct irq_chip armada_370_xp_irq_chip = { - .name ="MPIC" , - .irq_mask = armada_370_xp_irq_mask, - .irq_mask_ack = armada_370_xp_irq_mask, - .irq_unmask = armada_370_xp_irq_unmask, -#ifdef CONFIG_SMP - .irq_set_affinity = armada_xp_set_affinity, -#endif - .flags =IRQCHIP_SKIP_SET_WAKE |IRQCHIP_MASK_ON_SUSPEND , -}; - -static int armada_370_xp_mpic_irq_map (struct irq_domain *h, -unsigned int virq,irq_hw_number_t hw) -{ -armada_370_xp_irq_mask (irq_get_irq_data (virq)); -if (!is_percpu_irq (hw)) -writel (hw, per_cpu_int_base + -ARMADA_370_XP_INT_CLEAR_MASK_OFFS ); -else -writel (hw, main_int_base +ARMADA_370_XP_INT_SET_ENABLE_OFFS ); -irq_set_status_flags (virq,IRQ_LEVEL ); - -if (is_percpu_irq (hw)) { -irq_set_percpu_devid (virq); -irq_set_chip_and_handler (virq, &armada_370_xp_irq_chip, - handle_percpu_devid_irq); - }else { -irq_set_chip_and_handler (virq, &armada_370_xp_irq_chip, - handle_level_irq); -irqd_set_single_target (irq_desc_get_irq_data (irq_to_desc (virq))); - } -irq_set_probe (virq); - -return 0 ; -}static void armada_xp_mpic_smp_cpu_init (void ) {
@@ -383,48 +473,16 @@ static void armada_xp_mpic_smp_cpu_init(void)for (i =0 ; i < nr_irqs; i++)writel (i, per_cpu_int_base +ARMADA_370_XP_INT_SET_MASK_OFFS ); +/* Disable all IPIs */ +writel (0 , per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); +/* Clear pending IPIs */ writel (0 , per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS ); -/* Enable first 8 IPIs */ -writel (IPI_DOORBELL_MASK , per_cpu_int_base + -ARMADA_370_XP_IN_DRBEL_MSK_OFFS ); -/* Unmask IPI interrupt */ writel (0 , per_cpu_int_base +ARMADA_370_XP_INT_CLEAR_MASK_OFFS ); } -static void armada_xp_mpic_perf_init (void ) -{ -unsigned long cpuid =cpu_logical_map (smp_processor_id ()); - -/* Enable Performance Counter Overflow interrupts */ -writel (ARMADA_370_XP_INT_CAUSE_PERF (cpuid), - per_cpu_int_base +ARMADA_370_XP_INT_FABRIC_MASK_OFFS ); -} - -#ifdef CONFIG_SMP -static void armada_mpic_send_doorbell (const struct cpumask *mask, -unsigned int irq) -{ -int cpu; -unsigned long map =0 ; - -/* Convert our logical CPU mask into a physical one. */ -for_each_cpu (cpu, mask) - map |=1 <<cpu_logical_map (cpu); - -/* -* Ensure that stores to Normal memory are visible to the -* other CPUs before issuing the IPI. -*/ -dsb (); - -/* submit softirq */ -writel ((map <<8 ) | irq, main_int_base + -ARMADA_370_XP_SW_TRIG_INT_OFFS ); -} -static void armada_xp_mpic_reenable_percpu (void ) {unsigned int irq;
@@ -445,6 +503,8 @@ static void armada_xp_mpic_reenable_percpu(void)armada_370_xp_irq_unmask (data); } + +ipi_resume (); }static int armada_xp_mpic_starting_cpu (unsigned int cpu)
@@ -462,7 +522,46 @@ static int mpic_cascaded_starting_cpu(unsigned int cpu)enable_percpu_irq (parent_irq,IRQ_TYPE_NONE );return 0 ; } +#else +static void armada_xp_mpic_smp_cpu_init (void ) {} +static void ipi_resume (void ) {} +#endif + +static struct irq_chip armada_370_xp_irq_chip = { + .name ="MPIC" , + .irq_mask = armada_370_xp_irq_mask, + .irq_mask_ack = armada_370_xp_irq_mask, + .irq_unmask = armada_370_xp_irq_unmask, +#ifdef CONFIG_SMP + .irq_set_affinity = armada_xp_set_affinity, #endif + .flags =IRQCHIP_SKIP_SET_WAKE |IRQCHIP_MASK_ON_SUSPEND , +}; + +static int armada_370_xp_mpic_irq_map (struct irq_domain *h, +unsigned int virq,irq_hw_number_t hw) +{ +armada_370_xp_irq_mask (irq_get_irq_data (virq)); +if (!is_percpu_irq (hw)) +writel (hw, per_cpu_int_base + +ARMADA_370_XP_INT_CLEAR_MASK_OFFS ); +else +writel (hw, main_int_base +ARMADA_370_XP_INT_SET_ENABLE_OFFS ); +irq_set_status_flags (virq,IRQ_LEVEL ); + +if (is_percpu_irq (hw)) { +irq_set_percpu_devid (virq); +irq_set_chip_and_handler (virq, &armada_370_xp_irq_chip, + handle_percpu_devid_irq); + }else { +irq_set_chip_and_handler (virq, &armada_370_xp_irq_chip, + handle_level_irq); +irqd_set_single_target (irq_desc_get_irq_data (irq_to_desc (virq))); + } +irq_set_probe (virq); + +return 0 ; +}static const struct irq_domain_ops armada_370_xp_mpic_irq_ops = { .map = armada_370_xp_mpic_irq_map,
@@ -562,22 +661,15 @@ armada_370_xp_handle_irq(struct pt_regs *regs)#ifdef CONFIG_SMP /* IPI Handling */ if (irqnr ==0 ) { -u32 ipimask, ipinr; +unsigned long ipimask; +int ipi; ipimask =readl_relaxed (per_cpu_int_base +ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS ) &IPI_DOORBELL_MASK ; -writel (~ipimask, per_cpu_int_base + -ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS ); - -/* Handle all pending doorbells */ -for (ipinr =IPI_DOORBELL_START ; - ipinr <IPI_DOORBELL_END ; ipinr++) { -if (ipimask & (0x1 << ipinr)) -handle_IPI (ipinr, regs); - } -continue ; +for_each_set_bit (ipi, &ipimask,IPI_DOORBELL_END ) +handle_domain_irq (ipi_domain, ipi, regs); }#endif
@@ -636,6 +728,8 @@ static void armada_370_xp_mpic_resume(void)writel (0 , per_cpu_int_base +ARMADA_370_XP_INT_CLEAR_MASK_OFFS );if (doorbell_mask_reg &PCI_MSI_DOORBELL_MASK )writel (1 , per_cpu_int_base +ARMADA_370_XP_INT_CLEAR_MASK_OFFS ); + +ipi_resume (); }static struct syscore_ops armada_370_xp_mpic_syscore_ops = {
@@ -691,7 +785,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,irq_set_default_host (armada_370_xp_mpic_domain);set_handle_irq (armada_370_xp_handle_irq);#ifdef CONFIG_SMP -set_smp_cross_call (armada_mpic_send_doorbell); +armada_xp_ipi_init (node);cpuhp_setup_state_nocalls (CPUHP_AP_IRQ_ARMADA_XP_STARTING ,"irqchip/armada/ipi:starting" , armada_xp_mpic_starting_cpu,NULL );
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 14/17 arm64: Kill __smp_cross_call and co
In reply to Marc ZyngierThe old IPI registration interface is now unused on arm64, so let's get rid of it.
--- arch/arm64/include/asm/irq_work.h | 4 +--- arch/arm64/include/asm/smp.h | 7 ------- arch/arm64/kernel/smp.c | 34 +++++-------------------------- 3 files changed, 6 insertions(+), 39 deletions(-)
arch/arm64/include/asm/irq_work.h +1 -3
@@ -2,11 +2,9 @@#ifndef __ASM_IRQ_WORK_H#define __ASM_IRQ_WORK_H -#include <asm/smp.h> -static inline bool arch_irq_work_has_interrupt (void ) { -return !!__smp_cross_call; +return true; }#endif /* __ASM_IRQ_WORK_H */
arch/arm64/include/asm/smp.h +0 -7
@@ -67,13 +67,6 @@ extern void handle_IPI(int ipinr, struct pt_regs *regs);*/ extern void smp_init_cpus (void ); -/* -* Provide a function to raise an IPI cross call on CPUs in callmap. -*/ -extern void set_smp_cross_call (void (*)(const struct cpumask *,unsigned int )); - -extern void (*__smp_cross_call)(const struct cpumask *,unsigned int ); -/* * Register IPI interrupts with the arch SMP code */
arch/arm64/kernel/smp.c +5 -29
@@ -783,13 +783,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus) } } -void (*__smp_cross_call)(const struct cpumask *,unsigned int ); - -void __init set_smp_cross_call (void (*fn)(const struct cpumask *,unsigned int )) -{ - __smp_cross_call = fn; -} - static const char *ipi_types[NR_IPI] __tracepoint_string = { #define S(x,s) [x] = sS (IPI_RESCHEDULE ,"Rescheduling interrupts" ),
@@ -801,11 +794,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = { S(IPI_WAKEUP, "CPU wake-up interrupts"), }; -static void smp_cross_call (const struct cpumask *target,unsigned int ipinr) -{ -trace_ipi_raise (target, ipi_types[ipinr]); -__smp_cross_call (target, ipinr); -} +static void smp_cross_call (const struct cpumask *target,unsigned int ipinr);void show_ipi_list (struct seq_file *p,int prec) {
@@ -852,8 +841,7 @@ void arch_send_wakeup_ipi_mask(const struct cpumask *mask)#ifdef CONFIG_IRQ_WORK void arch_irq_work_raise (void ) { -if (__smp_cross_call) -smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK ); +smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK ); }#endif
@@ -960,27 +948,16 @@ static void do_handle_IPI(int ipinr)trace_ipi_exit_rcuidle (ipi_types[ipinr]); } -/* Legacy version, should go away once all irqchips have been converted */ -void handle_IPI (int ipinr,struct pt_regs *regs) -{ -struct pt_regs *old_regs =set_irq_regs (regs); - -irq_enter (); -do_handle_IPI (ipinr); -irq_exit (); - -set_irq_regs (old_regs); -} -static irqreturn_t ipi_handler (int irq,void *data) {do_handle_IPI (irq - ipi_irq_base);return IRQ_HANDLED ; } -static void ipi_send (const struct cpumask *target,unsigned int ipi) +static void smp_cross_call (const struct cpumask *target,unsigned int ipinr) { -__ipi_send_mask (ipi_desc[ipi], target); +trace_ipi_raise (target, ipi_types[ipinr]); +__ipi_send_mask (ipi_desc[ipinr], target); }static void ipi_setup (int cpu)
@@ -1022,7 +999,6 @@ void __init set_smp_ipi_range(int ipi_base, int n) } ipi_irq_base = ipi_base; - __smp_cross_call = ipi_send;/* Setup the boot CPU immediately */ ipi_setup (smp_processor_id ());
Signature · 1 line
2.27.0
Re: [PATCH v2 14/17] arm64: Kill __smp_cross_call and co
arch/arm64/kernel/smp.c void arch_send_wakeup_ipi_mask(const struct cpumask *mask) · line 844
Quoted 7 lines · Marc Zyngier
arch/arm64/kernel/smp.c +1
-2@@ -852,8 +841,7 @@ void arch_send_wakeup_ipi_mask(const struct cpumask *mask)#ifdef CONFIG_IRQ_WORK void arch_irq_work_raise (void ) { -if (__smp_cross_call) -smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK ); +smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK );
AIU the following commit:
eb631bb5bf5b ("arm64: Support arch_irq_work_raise() via self IPIs")
It seems arm64 hasn't needed that check since
4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant #ifdefs")
Did I get that right?
Quoted 3 lines
(quoted diff) +0
-0} #endif
Marc Zyngier
maintainerRe: [PATCH v2 14/17] arm64: Kill __smp_cross_call and co
Quoted 12 lines · Valentin Schneider
arch/arm64/kernel/smp.c void arch_send_wakeup_ipi_mask(const struct · line 845
Quoted 8 lines · Marc Zyngier
arch/arm64/kernel/smp.c +1
-2@@ -852,8 +841,7 @@ void arch_send_wakeup_ipi_mask(const struct cpumask *mask)#ifdef CONFIG_IRQ_WORK void arch_irq_work_raise (void ) { -if (__smp_cross_call) -smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK ); +smp_cross_call (cpumask_of (smp_processor_id ()),IPI_IRQ_WORK );AIU the following commit:
Quoted 8 lines
eb631bb5bf5b("arm64: Supportarch_irq_work_raise()via self IPIs")It seems arm64 hasn't needed that check since
4b3dc9679cf7("arm64: forceCONFIG_SMP=yand remove redundant
#ifdefs")Did I get that right?
Indeed. SMP implies being able to IPI, and thus the check has been irrelevant for some time now.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Marc Zyngier
maintainerPATCH v2 15/17 arm64: Remove custom IRQ stat accounting
In reply to Marc ZyngierLet's switch the arm64 code to the core accounting, which already does everything we need.
--- arch/arm64/include/asm/hardirq.h | 9 --------- arch/arm64/kernel/smp.c | 24 ++++++------------------ 2 files changed, 6 insertions(+), 27 deletions(-)
arch/arm64/include/asm/hardirq.h +0 -9
@@ -13,21 +13,12 @@#include <asm/kvm_arm.h> #include <asm/sysreg.h> -#define NR_IPI 7 -typedef struct {unsigned int __softirq_pending; -unsigned int ipi_irqs[NR_IPI ]; }____cacheline_aligned irq_cpustat_t ;#include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ -#define __inc_irq_stat (cpu, member) __IRQ_STAT(cpu, member)++ -#define __get_irq_stat (cpu, member) __IRQ_STAT(cpu, member) - -u64 smp_irq_stat_cpu (unsigned int cpu); -#define arch_irq_stat_cpu smp_irq_stat_cpu -#define __ARCH_IRQ_EXIT_IRQS_DISABLED 1struct nmi_ctx {
arch/arm64/kernel/smp.c +6 -18
@@ -73,7 +73,8 @@ enum ipi_msg_type {IPI_CPU_CRASH_STOP ,IPI_TIMER ,IPI_IRQ_WORK , - IPI_WAKEUP +IPI_WAKEUP , + NR_IPI };static int ipi_irq_base;
@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)unsigned int cpu, i;for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u:%s" , prec -1 ,"IPI" , i, prec >=4 ?" " :"" );for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]); } } -u64 smp_irq_stat_cpu (unsigned int cpu) -{ -u64 sum =0 ; -int i; - -for (i =0 ; i <NR_IPI ; i++) - sum +=__get_irq_stat (cpu, ipi_irqs[i]); - -return sum; -} -void arch_send_call_function_ipi_mask (const struct cpumask *mask) {smp_cross_call (mask,IPI_CALL_FUNC );
@@ -893,10 +883,8 @@ static void do_handle_IPI(int ipinr) {unsigned int cpu =smp_processor_id (); -if ((unsigned )ipinr <NR_IPI ) { +if ((unsigned )ipinr <NR_IPI )trace_ipi_entry_rcuidle (ipi_types[ipinr]); -__inc_irq_stat (cpu, ipi_irqs[ipinr]); - }switch (ipinr) {case IPI_RESCHEDULE :
@@ -991,7 +979,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)int err; err =request_percpu_irq (ipi_base + i, ipi_handler, -"IPI" , &irq_stat); +"IPI" , &cpu_number);WARN_ON (err); ipi_desc[i] =irq_to_desc (ipi_base + i);
Signature · 1 line
2.27.0
Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810
Quoted 12 lines · Marc Zyngier
arch/arm64/kernel/smp.c +2
-2@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)unsigned int cpu, i;for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u:%s" , prec -1 ,"IPI" , i, prec >=4 ?" " :"" );for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]);
How attached are we to that custom IPI printout? AIUI we *could* give them a "prettier" name in request_percpu_irq() and let the standard procfs printout take the wheel.
Marc Zyngier
maintainerRe: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
Quoted 4 lines · Valentin Schneider
Quoted 3 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0
-0@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)unsigned int cpu, i;
Quoted 13 lines
arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810
Quoted 9 lines
arch/arm64/kernel/smp.c +2
-2for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u:%s" , prec -1 ,"IPI" , i, prec >=4 ?" " :"" );for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]);How attached are we to that custom IPI printout? AIUI we *could* give them a "prettier" name in
request_percpu_irq()and let the standard procfs printout take the wheel.
I wish. Unfortunately, /proc/interrupts is likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update /proc/cpuinfo to be less braindead...).
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
Quoted 20 lines · Marc Zyngier
Quoted 4 lines · Valentin Schneider
Quoted 3 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0
-0@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec)unsigned int cpu, i;Quoted 14 lines
arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810
Quoted 9 lines
arch/arm64/kernel/smp.c +2
-2for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u:%s" , prec -1 ,"IPI" , i, prec >=4 ?" " :"" );for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]);How attached are we to that custom IPI printout? AIUI we *could* give them
a "prettier" name inrequest_percpu_irq()and let the standard procfs printout take the wheel.
Quoted 4 lines
I wish. Unfortunately,
/proc/interruptsis likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update/proc/cpuinfoto be less braindead...).
Hmph, it's borderline here I think: we're not introducing a new field or format in the file, so any tool that can parse /proc/interrupts can parse the IPIs if they are formatted like the other "regular" interrupts. But then said tool could die in flames when not seeing the special IPI fields because sturdiness is overrated :(
Oh well.
Quoted 1 line
M.
Marc Zyngier
maintainerRe: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
Quoted 27 lines · Valentin Schneider
Quoted 21 lines · Marc Zyngier
Quoted 5 lines · Valentin Schneider
Quoted 4 lines · Marc Zyngier
arch/arm64/kernel/smp.c +0
-0@@ -801,26 +802,15 @@ void show_ipi_list(struct seq_file *p, int prec) unsigned int cpu, i;Quoted 14 lines
arch/arm64/kernel/smp.c void show_ipi_list(struct seq_file *p, int prec) · line 810
Quoted 9 lines
arch/arm64/kernel/smp.c +2
-2for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u:%s" , prec -1 ,"IPI" , i, prec >=4 ?" " :"" );for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]);How attached are we to that custom IPI printout? AIUI we *could* give them
a "prettier" name inrequest_percpu_irq()and let the standard procfs printout take the wheel.Quoted 4 lines
I wish. Unfortunately,
/proc/interruptsis likely to be considered ABI, and I'm really worried to change this (see what happened last time we tried to update/proc/cpuinfoto be less braindead...).
Quoted 5 lines
Hmph, it's borderline here I think: we're not introducing a new field or format in the file, so any tool that can parse
/proc/interruptscan parse the IPIs if they are formatted like the other "regular" interrupts. But then said tool could die in flames when not seeing the special IPI fields because sturdiness is overrated :(
Which is exactly what I'm worried about. People do stupid things, and stupidity becomes ABI. I hate luserspace.
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting
In reply to Marc ZyngierQuoted 13 lines · Marc Zyngier
Let's switch the arm64 code to the core accounting, which already does everything we need.
Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/include/asm/hardirq.h | 9 --------- arch/arm64/kernel/smp.c | 24 ++++++------------------ 2 files changed, 6 insertions(+), 27 deletions(-)arch/arm64/kernel/smp.c +0
-0
arch/arm64/kernel/smp.c void __init set_smp_ipi_range(int ipi_base, int n) · line 982
Quoted 5 lines
arch/arm64/kernel/smp.c +1
-1@@ -991,7 +979,7 @@ void __init set_smp_ipi_range(int ipi_base, int n)int err; err =request_percpu_irq (ipi_base + i, ipi_handler, -"IPI" , &irq_stat); +"IPI" , &cpu_number);
That pointer remains unused in ipi_handler(); I suppose it's just a matter of cosmetics? I see that we leave it as irq_stat in arm, I suppose simply because we don't have a cpu_number equivalent there and need *some* percpu variable for the request to happen.
Quoted 3 lines
arch/arm64/kernel/smp.c +0
-0WARN_ON (err); ipi_desc[i] =irq_to_desc (ipi_base + i);
Marc Zyngier
maintainerPATCH v2 16/17 ARM: Kill __smp_cross_call and co
In reply to Marc ZyngierThe old IPI registration interface is now unused on arm, so let's get rid of it.
--- arch/arm/include/asm/smp.h | 6 ------ arch/arm/kernel/smp.c | 22 +++++----------------- 2 files changed, 5 insertions(+), 23 deletions(-)
arch/arm/include/asm/smp.h +0 -6
@@ -39,12 +39,6 @@ void handle_IPI(int ipinr, struct pt_regs *regs);*/ extern void smp_init_cpus (void ); - -/* -* Provide a function to raise an IPI cross call on CPUs in callmap. -*/ -extern void set_smp_cross_call (void (*)(const struct cpumask *,unsigned int )); -/* * Register IPI interrupts with the arch SMP code */
arch/arm/kernel/smp.c +5 -17
@@ -514,14 +514,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus) } } -static void (*__smp_cross_call)(const struct cpumask *,unsigned int ); - -void __init set_smp_cross_call (void (*fn)(const struct cpumask *,unsigned int )) -{ -if (!__smp_cross_call) - __smp_cross_call = fn; -} - static const char *ipi_types[NR_IPI] __tracepoint_string = { #define S(x,s) [x] = sS (IPI_WAKEUP ,"CPU wakeup interrupts" ),
@@ -533,11 +525,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = { S(IPI_COMPLETION, "completion interrupts"), }; -static void smp_cross_call (const struct cpumask *target,unsigned int ipinr) -{ -trace_ipi_raise_rcuidle (target, ipi_types[ipinr]); -__smp_cross_call (target, ipinr); -} +static void smp_cross_call (const struct cpumask *target,unsigned int ipinr);void show_ipi_list (struct seq_file *p,int prec) {
@@ -716,9 +704,10 @@ static irqreturn_t ipi_handler(int irq, void *data)return IRQ_HANDLED ; } -static void ipi_send (const struct cpumask *target,unsigned int ipi) +static void smp_cross_call (const struct cpumask *target,unsigned int ipinr) { -__ipi_send_mask (ipi_desc[ipi], target); +trace_ipi_raise_rcuidle (target, ipi_types[ipinr]); +__ipi_send_mask (ipi_desc[ipinr], target); }static void ipi_setup (int cpu)
@@ -760,7 +749,6 @@ void __init set_smp_ipi_range(int ipi_base, int n) } ipi_irq_base = ipi_base; -set_smp_cross_call (ipi_send);/* Setup the boot CPU immediately */ ipi_setup (smp_processor_id ());
@@ -873,7 +861,7 @@ core_initcall(register_cpufreq_notifier);static void raise_nmi (cpumask_t *mask) { -__smp_cross_call (mask,IPI_CPU_BACKTRACE ); +__ipi_send_mask (ipi_desc[IPI_CPU_BACKTRACE ], mask); }void arch_trigger_cpumask_backtrace (const cpumask_t *mask,bool exclude_self)
Signature · 1 line
2.27.0
Marc Zyngier
maintainerPATCH v2 17/17 ARM: Remove custom IRQ stat accounting
In reply to Marc ZyngierLet's switch the arm code to the core accounting, which already does everything we need.
--- arch/arm/include/asm/hardirq.h | 17 ----------------- arch/arm/kernel/smp.c | 20 ++++---------------- 2 files changed, 4 insertions(+), 33 deletions(-)
arch/arm/include/asm/hardirq.h +0 -17
@@ -6,29 +6,12 @@#include <linux/threads.h> #include <asm/irq.h> -/* number of IPIS _not_ including IPI_CPU_BACKTRACE */ -#define NR_IPI 7 -typedef struct {unsigned int __softirq_pending; -#ifdef CONFIG_SMP -unsigned int ipi_irqs[NR_IPI ]; -#endif }____cacheline_aligned irq_cpustat_t ;#include <linux/irq_cpustat.h> /* Standard mappings for irq_cpustat_t above */ -#define __inc_irq_stat (cpu, member) __IRQ_STAT(cpu, member)++ -#define __get_irq_stat (cpu, member) __IRQ_STAT(cpu, member) - -#ifdef CONFIG_SMP -u64 smp_irq_stat_cpu (unsigned int cpu); -#else -#define smp_irq_stat_cpu (cpu) 0 -#endif - -#define arch_irq_stat_cpu smp_irq_stat_cpu -#define __ARCH_IRQ_EXIT_IRQS_DISABLED 1#endif /* __ASM_HARDIRQ_H */
arch/arm/kernel/smp.c +4 -16
@@ -66,6 +66,7 @@ enum ipi_msg_type {IPI_CPU_STOP ,IPI_IRQ_WORK ,IPI_COMPLETION , +NR_IPI ,/* * CPU_BACKTRACE is special and not included in NR_IPI * or tracable with trace_ipi_*
@@ -532,27 +533,16 @@ void show_ipi_list(struct seq_file *p, int prec)unsigned int cpu, i;for (i =0 ; i <NR_IPI ; i++) { +unsigned int irq =irq_desc_get_irq (ipi_desc[i]);seq_printf (p,"%*s%u: " , prec -1 ,"IPI" , i);for_each_online_cpu (cpu) -seq_printf (p,"%10u " , -__get_irq_stat (cpu, ipi_irqs[i])); +seq_printf (p,"%10u " ,kstat_irqs_cpu (irq, cpu));seq_printf (p," %s\n" , ipi_types[i]); } } -u64 smp_irq_stat_cpu (unsigned int cpu) -{ -u64 sum =0 ; -int i; - -for (i =0 ; i <NR_IPI ; i++) - sum +=__get_irq_stat (cpu, ipi_irqs[i]); - -return sum; -} -void arch_send_call_function_ipi_mask (const struct cpumask *mask) {smp_cross_call (mask,IPI_CALL_FUNC );
@@ -633,10 +623,8 @@ static void do_handle_IPI(int ipinr) {unsigned int cpu =smp_processor_id (); -if ((unsigned )ipinr <NR_IPI ) { +if ((unsigned )ipinr <NR_IPI )trace_ipi_entry_rcuidle (ipi_types[ipinr]); -__inc_irq_stat (cpu, ipi_irqs[ipinr]); - }switch (ipinr) {case IPI_WAKEUP :
Signature · 1 line
2.27.0
Hi Marc,
Quoted 29 lines · Marc Zyngier
For as long as SMP ARM has existed, IPIs have been handled as something special. The arch code and the interrupt controller exchange a couple of hooks (one to generate an IPI, another to handle it).
Although this is perfectly manageable, it prevents the use of features that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It also means that each interrupt controller driver has to follow an architecture-specific interface instead of just implementing the base irqchip functionalities. The arch code also duplicates a number of things that the core irq code already does (such as calling
set_irq_regs(),irq_enter()...).This series tries to remedy this on arm/arm64 by offering a new registration interface where the irqchip gives the arch code a range of interrupts to use for IPIs. The arch code requests these as normal per-cpu interrupts.
The bulk of the work is at the interrupt controller level, where all 5 irqchips used on arm+SMP/arm64 get converted.
Finally, we drop the legacy registration interface as well as the custom statistics accounting.
Note that I have had a look at providing a "generic" interface by expanding the
kernel/irq/ipi.cbag of helpers, but so far all irqchips have very different requirements, so there is hardly anything to consolidate for now. Maybe some as hip04 and the Marvell horror get cleaned up (the latter certainly could do with a good dusting).
Quoted 5 lines
This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3), as well as 64bit bare metal (GICv3). The RPi part has only been tested in QEMU as a 64bit guest, while the HiSi and Marvell parts have only been compile-tested.
I gave that a spin on Juno r0 and HiKey960 (both GICv2), all good! I also wanted to try it out on my eMAG (to get some GICv3 airtime) but ran into "technical difficulties". I think I'll need to get someone to go poke it (most likely next week). I'm pretty sure I'm the one who should be asking you for hardware, but if there's anything specific you need me to test, please shout.
I have a few extra nits/comments in some patches, but it's all fairly minor so FWIW you can also add, for patches [01-10, 14-15]:
I haven't really looked at those other irqchips, but I can give it a shot if no one else shows up. Also I'll most likely look at the arm side, but I'm afraid I'm too well-done right now to pay much more attention to details.
Quoted 9 lines · Valentin Schneider
I have a few extra nits/comments in some patches, but it's all fairly minor so FWIW you can also add, for patches [01-10, 14-15]:
Reviewed-by Valentin Schneider <[email protected]>I haven't really looked at those other irqchips, but I can give it a shot if no one else shows up. Also I'll most likely look at the arm side, but I'm afraid I'm too well-done right now to pay much more attention to details.
Had a look at the arm side (and a took a refresher on the arm64 side); LGTM, so you can extend that to: [01-10, 14-17].
Hi Marc,
Quoted 30 lines · Marc Zyngier
For as long as SMP ARM has existed, IPIs have been handled as something special. The arch code and the interrupt controller exchange a couple of hooks (one to generate an IPI, another to handle it).
Although this is perfectly manageable, it prevents the use of features that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It also means that each interrupt controller driver has to follow an architecture-specific interface instead of just implementing the base irqchip functionalities. The arch code also duplicates a number of things that the core irq code already does (such as calling
set_irq_regs(),irq_enter()...).This series tries to remedy this on arm/arm64 by offering a new registration interface where the irqchip gives the arch code a range of interrupts to use for IPIs. The arch code requests these as normal per-cpu interrupts.
The bulk of the work is at the interrupt controller level, where all 5 irqchips used on arm+SMP/arm64 get converted.
Finally, we drop the legacy registration interface as well as the custom statistics accounting.
Note that I have had a look at providing a "generic" interface by expanding the
kernel/irq/ipi.cbag of helpers, but so far all irqchips have very different requirements, so there is hardly anything to consolidate for now. Maybe some as hip04 and the Marvell horror get cleaned up (the latter certainly could do with a good dusting).
Quoted 4 lines
This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3), as well as 64bit bare metal (GICv3). The RPi part has only been tested in QEMU as a 64bit guest, while the HiSi and Marvell parts have only been compile-tested.
This series works perfectly fine on Developerbox.
I just want to follow-up regarding when you are planning to push this series upstream? Are you waiting for other irqchips (apart from GIC) to be reviewed?
Actually mine work to turn IPI as a pseudo NMI [1] is dependent on this patch-set.
[1] https://lkml.org/lkml/2020/5/20/488
-Sumit
Quoted 53 lines
* From v1:
- Clarified the effect of nesting irq_enter/exit (Russell)
- Changed the point where we tear IPIs down on (Valentin)
- IPIs are no longer accessible from DT
- HIP04 and Armada 370-XP have been converted, but are untested
- arch-specific kstat accounting is removed
- ARM's legacy interface is droppedMarc Zyngier (17):
genirq: Add fasteoi IPI flow
genirq: Allow interrupts to be excluded from/proc/interrupts
arm64: Allow IPIs to be handled as normal interrupts
ARM: Allow IPIs to be handled as normal interrupts irqchip/gic-v3: Describe the SGI range
irqchip/gic-v3: Configure SGIs as standard interrupts irqchip/gic: Atomically update affinity
irqchip/gic: Refactor SMP configuration
irqchip/gic: Configure SGIs as standard interrupts irqchip/gic-common: Don't enable SGIs by default irqchip/bcm2836: Configure mailbox interrupts as standard interrupts irqchip/hip04: Configure IPIs as standard interrupts irqchip/armada-370-xp: Configure IPIs as standard interrupts
arm64: Kill__smp_cross_calland co
arm64: Remove custom IRQ stat accounting
ARM: Kill__smp_cross_calland co
ARM: Remove custom IRQ stat accountingarch/arm/Kconfig | 1 + arch/arm/include/asm/hardirq.h | 17 -- arch/arm/include/asm/smp.h | 5 +- arch/arm/kernel/smp.c | 135 +++++++++----- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/hardirq.h | 9 - arch/arm64/include/asm/irq_work.h | 4 +- arch/arm64/include/asm/smp.h | 6 +- arch/arm64/kernel/smp.c | 119 ++++++++----- drivers/irqchip/irq-armada-370-xp.c | 262 +++++++++++++++++++--------- drivers/irqchip/irq-bcm2836.c | 151 +++++++++++++--- drivers/irqchip/irq-gic-common.c | 3 - drivers/irqchip/irq-gic-v3.c | 99 ++++++----- drivers/irqchip/irq-gic.c | 183 ++++++++++--------- drivers/irqchip/irq-hip04.c | 89 +++++----- include/linux/irq.h | 5 +- kernel/irq/chip.c | 27 +++ kernel/irq/debugfs.c | 1 + kernel/irq/proc.c | 2 +- kernel/irq/settings.h | 7 + 20 files changed, 713 insertions(+), 413 deletions(-)--
2.27.0
Marc Zyngier
maintainerHi Sumit,
Quoted 38 lines · Sumit Garg
Hi Marc,
Quoted 30 lines · Marc Zyngier
For as long as SMP ARM has existed, IPIs have been handled as something special. The arch code and the interrupt controller exchange a couple of hooks (one to generate an IPI, another to handle it).
Although this is perfectly manageable, it prevents the use of features that we could use if IPIs were Linux IRQs (such as pseudo-NMIs). It also means that each interrupt controller driver has to follow an architecture-specific interface instead of just implementing the base irqchip functionalities. The arch code also duplicates a number of things that the core irq code already does (such as calling
set_irq_regs(),irq_enter()...).This series tries to remedy this on arm/arm64 by offering a new registration interface where the irqchip gives the arch code a range of interrupts to use for IPIs. The arch code requests these as normal per-cpu interrupts.
The bulk of the work is at the interrupt controller level, where all 5 irqchips used on arm+SMP/arm64 get converted.
Finally, we drop the legacy registration interface as well as the custom statistics accounting.
Note that I have had a look at providing a "generic" interface by expanding the
kernel/irq/ipi.cbag of helpers, but so far all irqchips have very different requirements, so there is hardly anything to consolidate for now. Maybe some as hip04 and the Marvell horror get cleaned up (the latter certainly could do with a good dusting).Quoted 4 lines
This has been tested on a bunch of 32 and 64bit guests (GICv2, GICv3), as well as 64bit bare metal (GICv3). The RPi part has only been tested in QEMU as a 64bit guest, while the HiSi and Marvell parts have only been compile-tested.
Quoted 5 lines
This series works perfectly fine on Developerbox.
I just want to follow-up regarding when you are planning to push this series upstream? Are you waiting for other irqchips (apart from GIC) to be reviewed?
I'd certainly like people to review (and maybe test if they have the HW at hand) the rest of the interrupt controller changes.
I'll probably repost the series around -rc1.
Quoted 4 lines
Actually mine work to turn IPI as a pseudo NMI [1] is dependent on this patch-set.
I'm aware of this.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...