* [PATCH v2 0/6] Migrate GIC to fasteoi flow control
@ 2011-02-28 13:33 Will Deacon
2011-02-28 13:33 ` [PATCH 1/6] ARM: gic: use handle_fasteoi_irq for SPIs Will Deacon
` (5 more replies)
0 siblings, 6 replies; 27+ messages in thread
From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
This is version 2 of the patch series originally posted here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/043083.html
Changes since v1:
* Added plat-nomadik changes after related patch from Rabin
* Fixed msm changes to call irq_eoi correctly
* No longer in quoted-printable encoding (thanks Colin)
This is going to be a PITA to merge so any thoughts relating to that
are welcome.
Thanks,
Will
Will Deacon (6):
ARM: gic: use handle_fasteoi_irq for SPIs
ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip
ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip
ARM: s5pv310: update IRQ combiner to use EOI in parent chip
ARM: msm: update GPIO chained IRQ handler to use EOI in parent chip
ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip
arch/arm/common/gic.c | 23 +++++++++--------------
arch/arm/mach-msm/gpio-v2.c | 3 ++-
arch/arm/mach-s5pv310/irq-combiner.c | 7 ++-----
arch/arm/mach-tegra/gpio.c | 17 +----------------
arch/arm/plat-nomadik/gpio.c | 2 ++
arch/arm/plat-omap/gpio.c | 5 ++++-
6 files changed, 20 insertions(+), 37 deletions(-)
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 1/6] ARM: gic: use handle_fasteoi_irq for SPIs 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-02-28 13:33 ` [PATCH 2/6] ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip Will Deacon ` (4 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel Currently, the gic uses handle_level_irq for handling SPIs (Shared Peripheral Interrupts), requiring active interrupts to be masked at the distributor level during IRQ handling. On a virtualised system, only the CPU interfaces are virtualised in hardware. Accesses to the distributor must be trapped by the hypervisor, adding latency to the critical interrupt path in Linux. This patch modifies the GIC code to use handle_fasteoi_irq for handling interrupts, which only requires us to signal EOI to the CPU interface when handling is complete. Cascaded IRQ handling is also updated so that EOI is signalled after handling. Note that commit 846afbd1 ("GIC: Dont disable INT in ack callback") broke cascading interrupts by forgetting to add IRQ masking. This is no longer an issue because the unmask call is now unnecessary. Tested on Versatile Express and Realview EB (1176 w/ cascaded GICs). Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk> Acked-by: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/common/gic.c | 23 +++++++++-------------- 1 files changed, 9 insertions(+), 14 deletions(-) diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c index 2243772..9def30b 100644 --- a/arch/arm/common/gic.c +++ b/arch/arm/common/gic.c @@ -71,13 +71,6 @@ static inline unsigned int gic_irq(struct irq_data *d) /* * Routines to acknowledge, disable and enable interrupts */ -static void gic_ack_irq(struct irq_data *d) -{ - spin_lock(&irq_controller_lock); - writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI); - spin_unlock(&irq_controller_lock); -} - static void gic_mask_irq(struct irq_data *d) { u32 mask = 1 << (d->irq % 32); @@ -96,6 +89,11 @@ static void gic_unmask_irq(struct irq_data *d) spin_unlock(&irq_controller_lock); } +static void gic_eoi_irq(struct irq_data *d) +{ + writel(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI); +} + static int gic_set_type(struct irq_data *d, unsigned int type) { void __iomem *base = gic_dist_base(d); @@ -174,9 +172,6 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) unsigned int cascade_irq, gic_irq; unsigned long status; - /* primary controller ack'ing */ - chip->irq_ack(&desc->irq_data); - spin_lock(&irq_controller_lock); status = readl(chip_data->cpu_base + GIC_CPU_INTACK); spin_unlock(&irq_controller_lock); @@ -192,15 +187,15 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) generic_handle_irq(cascade_irq); out: - /* primary controller unmasking */ - chip->irq_unmask(&desc->irq_data); + /* primary controller EOI */ + chip->irq_eoi(&desc->irq_data); } static struct irq_chip gic_chip = { .name = "GIC", - .irq_ack = gic_ack_irq, .irq_mask = gic_mask_irq, .irq_unmask = gic_unmask_irq, + .irq_eoi = gic_eoi_irq, .irq_set_type = gic_set_type, #ifdef CONFIG_SMP .irq_set_affinity = gic_set_cpu, @@ -275,7 +270,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic, for (i = irq_start; i < irq_limit; i++) { set_irq_chip(i, &gic_chip); set_irq_chip_data(i, gic); - set_irq_handler(i, handle_level_irq); + set_irq_handler(i, handle_fasteoi_irq); set_irq_flags(i, IRQF_VALID | IRQF_PROBE); } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/6] ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon 2011-02-28 13:33 ` [PATCH 1/6] ARM: gic: use handle_fasteoi_irq for SPIs Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-02-28 13:33 ` [PATCH 3/6] ARM: tegra: " Will Deacon ` (3 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel On OMAP4, gpio_irq_handler can be chained to a physical interrupt using the GIC as the primary IRQ chip. Now that the GIC uses the fasteoi flow model, the chained handler must invoke ->irq_eoi once handling is complete. This patch adds a conditional call to ->irq_eoi in the GPIO IRQ handler for OMAP platforms. For OMAP implementations using other primary interrupt controllers, the ->irq_ack call remains and is also made conditional on the support offered by the controller. Cc: Colin Cross <ccross@google.com> Acked-by: Tony Lindgren <tony@atomide.com> Acked-and-tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/plat-omap/gpio.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c index 971d186..1d2d1c7 100644 --- a/arch/arm/plat-omap/gpio.c +++ b/arch/arm/plat-omap/gpio.c @@ -1144,7 +1144,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) u32 retrigger = 0; int unmasked = 0; - desc->irq_data.chip->irq_ack(&desc->irq_data); + if (desc->irq_data.chip->irq_ack) + desc->irq_data.chip->irq_ack(&desc->irq_data); bank = get_irq_data(irq); #ifdef CONFIG_ARCH_OMAP1 @@ -1238,6 +1239,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) exit: if (!unmasked) desc->irq_data.chip->irq_unmask(&desc->irq_data); + if (desc->irq_data.chip->irq_eoi) + desc->irq_data.chip->irq_eoi(&desc->irq_data); } static void gpio_irq_shutdown(struct irq_data *d) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon 2011-02-28 13:33 ` [PATCH 1/6] ARM: gic: use handle_fasteoi_irq for SPIs Will Deacon 2011-02-28 13:33 ` [PATCH 2/6] ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-03-01 13:11 ` Sergei Shtylyov 2011-02-28 13:33 ` [PATCH 4/6] ARM: s5pv310: update IRQ combiner " Will Deacon ` (2 subsequent siblings) 5 siblings, 1 reply; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent chip prior to handling the interrupt. This patch updates the code to use ->irq_eoi now that the GIC has moved to using the fasteoi flow model. Acked-by: Colin Cross <ccross@android.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/mach-tegra/gpio.c | 17 +---------------- 1 files changed, 1 insertions(+), 16 deletions(-) diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c index ad80488..5019b01 100644 --- a/arch/arm/mach-tegra/gpio.c +++ b/arch/arm/mach-tegra/gpio.c @@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) struct tegra_gpio_bank *bank; int port; int pin; - int unmasked = 0; - - desc->irq_data.chip->irq_ack(&desc->irq_data); bank = get_irq_data(irq); @@ -233,23 +230,11 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) for_each_set_bit(pin, &sta, 8) { __raw_writel(1 << pin, GPIO_INT_CLR(gpio)); - - /* if gpio is edge triggered, clear condition - * before executing the hander so that we don't - * miss edges - */ - if (lvl & (0x100 << pin)) { - unmasked = 1; - desc->irq_data.chip->irq_unmask(&desc->irq_data); - } - generic_handle_irq(gpio_to_irq(gpio + pin)); } } - if (!unmasked) - desc->irq_data.chip->irq_unmask(&desc->irq_data); - + desc->irq_data.chip->irq_eoi(&desc->irq_data); } #ifdef CONFIG_PM -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 ` [PATCH 3/6] ARM: tegra: " Will Deacon @ 2011-03-01 13:11 ` Sergei Shtylyov 2011-03-01 13:24 ` Will Deacon 0 siblings, 1 reply; 27+ messages in thread From: Sergei Shtylyov @ 2011-03-01 13:11 UTC (permalink / raw) To: linux-arm-kernel Hello. On 28-02-2011 16:33, Will Deacon wrote: > The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent > chip prior to handling the interrupt. > This patch updates the code to use ->irq_eoi now that the GIC has moved > to using the fasteoi flow model. > Acked-by: Colin Cross<ccross@android.com> > Signed-off-by: Will Deacon<will.deacon@arm.com> > --- > arch/arm/mach-tegra/gpio.c | 17 +---------------- > 1 files changed, 1 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c > index ad80488..5019b01 100644 > --- a/arch/arm/mach-tegra/gpio.c > +++ b/arch/arm/mach-tegra/gpio.c > @@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > struct tegra_gpio_bank *bank; > int port; > int pin; > - int unmasked = 0; > - > - desc->irq_data.chip->irq_ack(&desc->irq_data); Won't this code break after the first patch as it removes irq_ack() method? I.e. shouldn't the patches be combined to keep them bisectable? WBR, Sergei ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] ARM: tegra: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 13:11 ` Sergei Shtylyov @ 2011-03-01 13:24 ` Will Deacon 0 siblings, 0 replies; 27+ messages in thread From: Will Deacon @ 2011-03-01 13:24 UTC (permalink / raw) To: linux-arm-kernel Sergei, > On 28-02-2011 16:33, Will Deacon wrote: > > > The chained GPIO IRQ handler on Tegra calls ->irq_ack on the parent > > chip prior to handling the interrupt. > > > This patch updates the code to use ->irq_eoi now that the GIC has moved > > to using the fasteoi flow model. > > > Acked-by: Colin Cross<ccross@android.com> > > Signed-off-by: Will Deacon<will.deacon@arm.com> > > --- > > arch/arm/mach-tegra/gpio.c | 17 +---------------- > > 1 files changed, 1 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm/mach-tegra/gpio.c b/arch/arm/mach-tegra/gpio.c > > index ad80488..5019b01 100644 > > --- a/arch/arm/mach-tegra/gpio.c > > +++ b/arch/arm/mach-tegra/gpio.c > > @@ -219,9 +219,6 @@ static void tegra_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > > struct tegra_gpio_bank *bank; > > int port; > > int pin; > > - int unmasked = 0; > > - > > - desc->irq_data.chip->irq_ack(&desc->irq_data); > > Won't this code break after the first patch as it removes irq_ack() > method? I.e. shouldn't the patches be combined to keep them bisectable? If you read the cover letter, I said: > This is going to be a PITA to merge so any thoughts relating to that > are welcome. I'm aware of the bisecting issues and Russell has come up with a solution using entry/exit functions to sort this out (with the GIC changes moved to the end of the series). But we have bigger fish to fry. The whole question about whether or not chained handlers should be written with multiple parent chips in mind is a hot topic. Thomas has some interesting ideas for the core code which will allow the chained handler not to care about flow control. Once we've sorted out the mechanism for this stuff, the merging should be fairly straightforward because the chip details will be irrelevant. Will ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/6] ARM: s5pv310: update IRQ combiner to use EOI in parent chip 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon ` (2 preceding siblings ...) 2011-02-28 13:33 ` [PATCH 3/6] ARM: tegra: " Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-03-01 13:12 ` Sergei Shtylyov 2011-02-28 13:33 ` [PATCH 5/6] ARM: msm: update GPIO chained IRQ handler " Will Deacon 2011-02-28 13:33 ` [PATCH 6/6] ARM: nmk: " Will Deacon 5 siblings, 1 reply; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel The IRQ combiner code invokes the ->irq_{un}mask routines of the parent chip. This patch updates the cascaded handler to use EOI now that the GIC has moved to using the fasteoi flow model. Tested-by: Kyungmin Park <kyungmin.park@samsung.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/mach-s5pv310/irq-combiner.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/arm/mach-s5pv310/irq-combiner.c b/arch/arm/mach-s5pv310/irq-combiner.c index 1ea4a9e..24d5604 100644 --- a/arch/arm/mach-s5pv310/irq-combiner.c +++ b/arch/arm/mach-s5pv310/irq-combiner.c @@ -59,9 +59,6 @@ static void combiner_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) unsigned int cascade_irq, combiner_irq; unsigned long status; - /* primary controller ack'ing */ - chip->irq_ack(&desc->irq_data); - spin_lock(&irq_controller_lock); status = __raw_readl(chip_data->base + COMBINER_INT_STATUS); spin_unlock(&irq_controller_lock); @@ -79,8 +76,8 @@ static void combiner_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) generic_handle_irq(cascade_irq); out: - /* primary controller unmasking */ - chip->irq_unmask(&desc->irq_data); + /* primary controller EOI */ + chip->irq_eoi(&desc->irq_data); } static struct irq_chip combiner_chip = { -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 4/6] ARM: s5pv310: update IRQ combiner to use EOI in parent chip 2011-02-28 13:33 ` [PATCH 4/6] ARM: s5pv310: update IRQ combiner " Will Deacon @ 2011-03-01 13:12 ` Sergei Shtylyov 0 siblings, 0 replies; 27+ messages in thread From: Sergei Shtylyov @ 2011-03-01 13:12 UTC (permalink / raw) To: linux-arm-kernel On 28-02-2011 16:33, Will Deacon wrote: > The IRQ combiner code invokes the ->irq_{un}mask routines of the parent > chip. > This patch updates the cascaded handler to use EOI now that the GIC has > moved to using the fasteoi flow model. > Tested-by: Kyungmin Park<kyungmin.park@samsung.com> > Signed-off-by: Will Deacon<will.deacon@arm.com> > --- > arch/arm/mach-s5pv310/irq-combiner.c | 7 ++----- > 1 files changed, 2 insertions(+), 5 deletions(-) > diff --git a/arch/arm/mach-s5pv310/irq-combiner.c b/arch/arm/mach-s5pv310/irq-combiner.c > index 1ea4a9e..24d5604 100644 > --- a/arch/arm/mach-s5pv310/irq-combiner.c > +++ b/arch/arm/mach-s5pv310/irq-combiner.c > @@ -59,9 +59,6 @@ static void combiner_handle_cascade_irq(unsigned int irq, struct irq_desc *desc) > unsigned int cascade_irq, combiner_irq; > unsigned long status; > > - /* primary controller ack'ing */ > - chip->irq_ack(&desc->irq_data); > - Same question about bisectability here... WBR, Sergei ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] ARM: msm: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon ` (3 preceding siblings ...) 2011-02-28 13:33 ` [PATCH 4/6] ARM: s5pv310: update IRQ combiner " Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-02-28 13:33 ` [PATCH 6/6] ARM: nmk: " Will Deacon 5 siblings, 0 replies; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel The chained GPIO IRQ handler on MSM8x60 calls ->ack on the parent chip after handling the interrupt. This patch updates the code to use ->irq_eoi now that the GIC has moved to using the fasteoi flow model. Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/mach-msm/gpio-v2.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/arch/arm/mach-msm/gpio-v2.c b/arch/arm/mach-msm/gpio-v2.c index 0de19ec..90a968f 100644 --- a/arch/arm/mach-msm/gpio-v2.c +++ b/arch/arm/mach-msm/gpio-v2.c @@ -310,6 +310,7 @@ static int msm_gpio_irq_set_type(unsigned int irq, unsigned int flow_type) static void msm_summary_irq_handler(unsigned int irq, struct irq_desc *desc) { unsigned long i; + struct irq_chip *chip = get_irq_desc_chip(desc); for (i = find_first_bit(msm_gpio.enabled_irqs, NR_GPIO_IRQS); i < NR_GPIO_IRQS; @@ -318,7 +319,7 @@ static void msm_summary_irq_handler(unsigned int irq, struct irq_desc *desc) generic_handle_irq(msm_gpio_to_irq(&msm_gpio.gpio_chip, i)); } - desc->chip->ack(irq); + chip->irq_eoi(&desc->irq_data); } static int msm_gpio_irq_set_wake(unsigned int irq, unsigned int on) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon ` (4 preceding siblings ...) 2011-02-28 13:33 ` [PATCH 5/6] ARM: msm: update GPIO chained IRQ handler " Will Deacon @ 2011-02-28 13:33 ` Will Deacon 2011-02-28 14:03 ` Russell King - ARM Linux 5 siblings, 1 reply; 27+ messages in thread From: Will Deacon @ 2011-02-28 13:33 UTC (permalink / raw) To: linux-arm-kernel The chained GPIO IRQ handler for the nomadik platform can be called with the GIC as its host chip on the mach-ux500 machines. This patch updates the code to use ->irq_eoi when it is available. Cc: Rabin Vincent <rabin@rab.in> Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/plat-nomadik/gpio.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c index 1e88ecb..51cc71b 100644 --- a/arch/arm/plat-nomadik/gpio.c +++ b/arch/arm/plat-nomadik/gpio.c @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) } host_chip->irq_unmask(&desc->irq_data); + if (host_chip->irq_eoi) + host_chip->irq_eoi(&desc->irq_data); } static int nmk_gpio_init_irq(struct nmk_gpio_chip *nmk_chip) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 13:33 ` [PATCH 6/6] ARM: nmk: " Will Deacon @ 2011-02-28 14:03 ` Russell King - ARM Linux 2011-02-28 18:09 ` Will Deacon 0 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-02-28 14:03 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote: > The chained GPIO IRQ handler for the nomadik platform can be called > with the GIC as its host chip on the mach-ux500 machines. > > This patch updates the code to use ->irq_eoi when it is available. > > Cc: Rabin Vincent <rabin@rab.in> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > arch/arm/plat-nomadik/gpio.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c > index 1e88ecb..51cc71b 100644 > --- a/arch/arm/plat-nomadik/gpio.c > +++ b/arch/arm/plat-nomadik/gpio.c > @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > } > > host_chip->irq_unmask(&desc->irq_data); > + if (host_chip->irq_eoi) > + host_chip->irq_eoi(&desc->irq_data); I notice in some you delete the irq_unmask, others you leave it. Shouldn't they all be similar? Maybe we do something like: static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc) { if (chip->irq_eoi) chip->irq_eoi(&desc->irq_data); else chip->irq_unmask(&desc->irq_data); } to cover these exit paths in a common way? ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 14:03 ` Russell King - ARM Linux @ 2011-02-28 18:09 ` Will Deacon 2011-02-28 19:16 ` Thomas Gleixner 0 siblings, 1 reply; 27+ messages in thread From: Will Deacon @ 2011-02-28 18:09 UTC (permalink / raw) To: linux-arm-kernel Hi Russell, > On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote: > > The chained GPIO IRQ handler for the nomadik platform can be called > > with the GIC as its host chip on the mach-ux500 machines. > > > > This patch updates the code to use ->irq_eoi when it is available. > > > > Cc: Rabin Vincent <rabin@rab.in> > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > arch/arm/plat-nomadik/gpio.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c > > index 1e88ecb..51cc71b 100644 > > --- a/arch/arm/plat-nomadik/gpio.c > > +++ b/arch/arm/plat-nomadik/gpio.c > > @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > > } > > > > host_chip->irq_unmask(&desc->irq_data); > > + if (host_chip->irq_eoi) > > + host_chip->irq_eoi(&desc->irq_data); > > I notice in some you delete the irq_unmask, others you leave it. Shouldn't > they all be similar? It depends on whether or not the parent chip is always the GIC. In some cases it can be a different IRQ chip, so we need to call the functions conditionally. > Maybe we do something like: > > static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc) > { > if (chip->irq_eoi) > chip->irq_eoi(&desc->irq_data); > else > chip->irq_unmask(&desc->irq_data); > } > > to cover these exit paths in a common way? Yes, that is cleaner and potentially less confusing. We'll also need an entry function which will do something like: if (!chip->irq_eoi) chip->irq_mask(&desc->irq_data); which does look pretty odd, so factoring it out (with a comment) will make it a little clearer. I'll see if I get any feedback about nomadik and msm and then post a v3 with the entry/exit functions. Will ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 18:09 ` Will Deacon @ 2011-02-28 19:16 ` Thomas Gleixner 2011-02-28 21:44 ` Russell King - ARM Linux 0 siblings, 1 reply; 27+ messages in thread From: Thomas Gleixner @ 2011-02-28 19:16 UTC (permalink / raw) To: linux-arm-kernel On Mon, 28 Feb 2011, Will Deacon wrote: > > On Mon, Feb 28, 2011 at 01:33:42PM +0000, Will Deacon wrote: > > > The chained GPIO IRQ handler for the nomadik platform can be called > > > with the GIC as its host chip on the mach-ux500 machines. > > > > > > This patch updates the code to use ->irq_eoi when it is available. > > > > > > Cc: Rabin Vincent <rabin@rab.in> > > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > > --- > > > arch/arm/plat-nomadik/gpio.c | 2 ++ > > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > > > diff --git a/arch/arm/plat-nomadik/gpio.c b/arch/arm/plat-nomadik/gpio.c > > > index 1e88ecb..51cc71b 100644 > > > --- a/arch/arm/plat-nomadik/gpio.c > > > +++ b/arch/arm/plat-nomadik/gpio.c > > > @@ -538,6 +538,8 @@ static void nmk_gpio_irq_handler(unsigned int irq, struct irq_desc *desc) > > > } > > > > > > host_chip->irq_unmask(&desc->irq_data); > > > + if (host_chip->irq_eoi) > > > + host_chip->irq_eoi(&desc->irq_data); > > > > I notice in some you delete the irq_unmask, others you leave it. Shouldn't > > they all be similar? > > It depends on whether or not the parent chip is always the GIC. In some cases > it can be a different IRQ chip, so we need to call the functions conditionally. > > > Maybe we do something like: > > > > static inline void chained_irq_exit(struct irq_chip *chip, struct irq_desc *desc) > > { > > if (chip->irq_eoi) > > chip->irq_eoi(&desc->irq_data); > > else > > chip->irq_unmask(&desc->irq_data); > > } > > > > to cover these exit paths in a common way? > > Yes, that is cleaner and potentially less confusing. We'll also need an entry > function which will do something like: > > if (!chip->irq_eoi) > chip->irq_mask(&desc->irq_data); > > which does look pretty odd, so factoring it out (with a comment) will > make it a little clearer. And that's the whole mess I was ranting about in the first place. A chained handler is tied to the underlying primary chip and not a half arsed flow handler which handles magically the availability or the absence of irq chip functions. That's just doomed. And for those cases I really want a reasonable explanation why the chained handler can't be implemented as a bog standard interrupt handler which relies on the underlying irq chip implementation to select the proper flow handler in the first place. Really folks, count the number of instructions which a normal flow handler adds. We talk about what? Not more than 100 instructions. So what's the gain of a barebone chained handler over a regular interrupt: - 100 instructions less - lack of statistics - lack of affinity setting via standard interfaces - making the chained handler aware of the underlying hardware While a bog standard interrupt handler which is installed via request/setup_irq() has only the 100 instructions more downside. There is no other difference. So please think about whether the 100 instructions are worth the trouble or not. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 19:16 ` Thomas Gleixner @ 2011-02-28 21:44 ` Russell King - ARM Linux 2011-03-01 10:57 ` Thomas Gleixner 0 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-02-28 21:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote: > So what's the gain of a barebone chained handler over a regular > interrupt: > > - 100 instructions less > - lack of statistics We don't want statistics. Don't care how many times we go to look at the interrupt controller - and actually it's really wrong to count them in the first place. Counting them means that you double-count these interrupt events, and the more layers of interrupt controllers there are the worse that problem gets. So no, that's a definite argument *for* chained handers. > - lack of affinity setting via standard interfaces Don't want affinity for them, as setting their affinity means that you then end up forcing the affinity for the sub-interrupts too. How you do cope with the high-level interrupt having affinity to CPU0 but a lower level interrupt being affine to CPU1 only? It's non-sensible, and is broken. So no, again this isn't an argument for not using chained handlers. It's an argument *for* them. Sorry, but I think this stuff is right, and chained handlers like these have their place. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-02-28 21:44 ` Russell King - ARM Linux @ 2011-03-01 10:57 ` Thomas Gleixner 2011-03-01 20:19 ` Russell King - ARM Linux 0 siblings, 1 reply; 27+ messages in thread From: Thomas Gleixner @ 2011-03-01 10:57 UTC (permalink / raw) To: linux-arm-kernel On Mon, 28 Feb 2011, Russell King - ARM Linux wrote: > On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote: > > So what's the gain of a barebone chained handler over a regular > > interrupt: > > > > - 100 instructions less > > - lack of statistics > > We don't want statistics. Don't care how many times we go to look > at the interrupt controller - and actually it's really wrong to count > them in the first place. Counting them means that you double-count > these interrupt events, and the more layers of interrupt controllers > there are the worse that problem gets. > > So no, that's a definite argument *for* chained handers. Errm, why do you double account them ? The accounting goes to different irq numbers, but we could exclude them from accounting and showing up in proc/interrupts easily. > > - lack of affinity setting via standard interfaces > > Don't want affinity for them, as setting their affinity means that > you then end up forcing the affinity for the sub-interrupts too. > How you do cope with the high-level interrupt having affinity to > CPU0 but a lower level interrupt being affine to CPU1 only? > > It's non-sensible, and is broken. So no, again this isn't an > argument for not using chained handlers. It's an argument *for* > them. Well, moving the whole group to a particular cpu is sensible and the sub interrupts don't have a set_affinity function anyway as they are always called on the cpu on which the primary interrupt is handled. > Sorry, but I think this stuff is right, and chained handlers like > these have their place. I'm not against chained handlers per se. I'm against creating a necessarity to make a chained handler deal with various different underlying primary irq chips and their oddities. That's simply wrong and broken. I don't say you have to use the existing flow handlers, if they are too heavy weight for your purpose, but pushing conditional flow handling into the chained handler is violating all principles of layering. The underlying primary chip implementation knows about the flow requirements, so it should install the correct handler so the chained handler just needs to deal with the sub interrupts which it knows how to handle. So if you don't want accounting and affinity setting (which I think is bad), then we still can solve it in an elegant and simple way. 1) Primary chip installs a rudimentary flow handler which deals with the requirements of the chip. primary_handler_eoi() { if (desc->action) action(); chip->irq_eoi(); } primary_handler_level() { chip->irq_mask(); if (desc->action) action(); chip->irq_unmask(); } The interupt descriptor is marked IRQ_PRIMARY or whatever 2) Demux handler is installed via: setup_irq(PRIMARY_IRQ, IRQ_DEMUX, demux_handler); That's a few lines in the core code to make this work with the same functionality than your current solution, but avoiding to touch any demux handler when you change the underlying irq chip implementation. It simply just works because the demux handler is agnostic of the primary chip. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 10:57 ` Thomas Gleixner @ 2011-03-01 20:19 ` Russell King - ARM Linux 2011-03-01 21:29 ` Thomas Gleixner 0 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-03-01 20:19 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 01, 2011 at 11:57:48AM +0100, Thomas Gleixner wrote: > On Mon, 28 Feb 2011, Russell King - ARM Linux wrote: > > > On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote: > > > So what's the gain of a barebone chained handler over a regular > > > interrupt: > > > > > > - 100 instructions less > > > - lack of statistics > > > > We don't want statistics. Don't care how many times we go to look > > at the interrupt controller - and actually it's really wrong to count > > them in the first place. Counting them means that you double-count > > these interrupt events, and the more layers of interrupt controllers > > there are the worse that problem gets. > > > > So no, that's a definite argument *for* chained handers. > > Errm, why do you double account them ? The accounting goes to > different irq numbers, but we could exclude them from accounting and > showing up in proc/interrupts easily. We don't want the parent interrupt counted because the interrupt sum which is exported from the kernel via the various statistics interface will be screwed by this. For N levels of classical interrupt handlers, causing I interrupts, you end up with I*N interrupts reported. So if you have a 100Hz interrupt coming from a timer on a 2nd level IRQ controller, you'll end up seeing a 200Hz interrupt rate, which is quite obviously wrong. > > Don't want affinity for them, as setting their affinity means that > > you then end up forcing the affinity for the sub-interrupts too. > > How you do cope with the high-level interrupt having affinity to > > CPU0 but a lower level interrupt being affine to CPU1 only? > > > > It's non-sensible, and is broken. So no, again this isn't an > > argument for not using chained handlers. It's an argument *for* > > them. > > Well, moving the whole group to a particular cpu is sensible and the > sub interrupts don't have a set_affinity function anyway as they are > always called on the cpu on which the primary interrupt is handled. Not quite correct - there's systems with two GICs chained one after each other. The second GIC will not have a NULL set_affinity function. However, it is meaningless to set the affinity for the second GIC as it is not multi-CPU aware. > > Sorry, but I think this stuff is right, and chained handlers like > > these have their place. > > I'm not against chained handlers per se. I'm against creating a > necessarity to make a chained handler deal with various different > underlying primary irq chips and their oddities. That's simply wrong > and broken. That wasn't a problem before genirq came along and invented many more different ways for 'flow' handlers to call into the lower level IRQ specific code. > I don't say you have to use the existing flow handlers, if they are > too heavy weight for your purpose, but pushing conditional flow > handling into the chained handler is violating all principles of > layering. Chained handlers *are* a *direct* replacement for flow handlers because the normal flow handler is *meaningless* for chained handlers. Trying to separate them into different layers is a mistake in itself. Chained handlers require the parent to behave differently. Eg, in the case of a level IRQ input, you don't want IRQs to be masked just because we're in the middle of servicing a down-stream interrupt. You want it left enabled so that if the ultimate device handler decides it needs to have IRQs enabled, then all interrupts are serviced in a fair manner. If you mask the parent interrupt, then only primary level interrupts get serviced and fairness goes out the window. Maybe you never got that point when you decided to create genirq from my work, but that's *your* problem, not mine. And it seems you're hell bent on breaking this for ARM. So, I say again - if you persist with this, I'll replace the ARM IRQ implementation and stop using genirq because you're taking it in a direction which will cause problems for ARM. And I *really* mean that. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 20:19 ` Russell King - ARM Linux @ 2011-03-01 21:29 ` Thomas Gleixner 2011-03-01 23:14 ` Russell King - ARM Linux ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Thomas Gleixner @ 2011-03-01 21:29 UTC (permalink / raw) To: linux-arm-kernel On Tue, 1 Mar 2011, Russell King - ARM Linux wrote: > On Tue, Mar 01, 2011 at 11:57:48AM +0100, Thomas Gleixner wrote: > > On Mon, 28 Feb 2011, Russell King - ARM Linux wrote: > > > > > On Mon, Feb 28, 2011 at 08:16:25PM +0100, Thomas Gleixner wrote: > > > > So what's the gain of a barebone chained handler over a regular > > > > interrupt: > > > > > > > > - 100 instructions less > > > > - lack of statistics > > > > > > We don't want statistics. Don't care how many times we go to look > > > at the interrupt controller - and actually it's really wrong to count > > > them in the first place. Counting them means that you double-count > > > these interrupt events, and the more layers of interrupt controllers > > > there are the worse that problem gets. > > > > > > So no, that's a definite argument *for* chained handers. > > > > Errm, why do you double account them ? The accounting goes to > > different irq numbers, but we could exclude them from accounting and > > showing up in proc/interrupts easily. > > We don't want the parent interrupt counted because the interrupt sum > which is exported from the kernel via the various statistics interface > will be screwed by this. For N levels of classical interrupt handlers, > causing I interrupts, you end up with I*N interrupts reported. > > So if you have a 100Hz interrupt coming from a timer on a 2nd level IRQ > controller, you'll end up seeing a 200Hz interrupt rate, which is quite > obviously wrong. Fair enough. > > > Don't want affinity for them, as setting their affinity means that > > > you then end up forcing the affinity for the sub-interrupts too. > > > How you do cope with the high-level interrupt having affinity to > > > CPU0 but a lower level interrupt being affine to CPU1 only? > > > > > > It's non-sensible, and is broken. So no, again this isn't an > > > argument for not using chained handlers. It's an argument *for* > > > them. > > > > Well, moving the whole group to a particular cpu is sensible and the > > sub interrupts don't have a set_affinity function anyway as they are > > always called on the cpu on which the primary interrupt is handled. > > Not quite correct - there's systems with two GICs chained one after each > other. The second GIC will not have a NULL set_affinity function. > However, it is meaningless to set the affinity for the second GIC as it > is not multi-CPU aware. Right. It's pointless for the secondary chip, but seeting the affinity of the primary interrupt can be still useful.. > > > Sorry, but I think this stuff is right, and chained handlers like > > > these have their place. > > > > I'm not against chained handlers per se. I'm against creating a > > necessarity to make a chained handler deal with various different > > underlying primary irq chips and their oddities. That's simply wrong > > and broken. > > That wasn't a problem before genirq came along and invented many more > different ways for 'flow' handlers to call into the lower level IRQ > specific code. It added merily two new flow handlers. One of them is fasteoi, which was not necessary in the original ARM implementation. So where is the point? > > I don't say you have to use the existing flow handlers, if they are > > too heavy weight for your purpose, but pushing conditional flow > > handling into the chained handler is violating all principles of > > layering. > > Chained handlers *are* a *direct* replacement for flow handlers because > the normal flow handler is *meaningless* for chained handlers. Trying > to separate them into different layers is a mistake in itself. Did you even look at the separation I suggested? > Chained handlers require the parent to behave differently. Eg, in the > case of a level IRQ input, you don't want IRQs to be masked just because > we're in the middle of servicing a down-stream interrupt. You want it > left enabled so that if the ultimate device handler decides it needs to > have IRQs enabled, then all interrupts are serviced in a fair manner. > > If you mask the parent interrupt, then only primary level interrupts > get serviced and fairness goes out the window. Errm. I did never say that we disable the parent interrupt by any means except when the chained handler explicitely wants to do that, which is pretty much pointlesss nowadays, as we run all interrupt handlers with interrupts disabled. > Maybe you never got that point when you decided to create genirq from > my work, but that's *your* problem, not mine. And it seems you're I pretty well understand the reasoning behind it and you know that. Sigh, I explained it more than once: The chained handlers as they are, are completely fine and not going away. The only point where we disagree is an implementation detail: The problem at hand which started this discussion wants to have chained handlers which are able to deal with different underlying primary irq chips. My point is that having a chained handler which does: if (chip->irq_ack) chip->irq_ack(); demux_interrupts(); if (chip->irq_eoi) chip->irq_eoi(); or whatever abnominations you come up with is wrong. Such a demux handler should be agnostic of the underlying chip to avoid wreckage like the one which is currently discussed which is caused by a change to the underlying primary chip. So my take on this is to have very simplistic primary flow handlers which are selected by the primary chip implementation to allow the underlying demux handler to be agnostic of the primary chip. They don't have anything to do with the normal handle_irq_* flow handlers and should be explicitely named different. They even should be implemented in the arch/ chip implmentation and not be part of the generic handlers as long as they are not identified as repeated patterns. Can you please take the time and explain me the difference of the following: irqchip1.c struct irq_chip1; handle_primary_irq(int irq, struct irq_desc *desc) { chip->irq_ack(); desc->demux(); } init() { irq_set_chip(PRIMARY_IRQ, &irq_chip1); irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); } irqchip2.c struct irq_chip2; handle_primary_irq(int irq, struct irq_desc *desc) { desc->demux(); chip->irq_eoi(); } init() { irq_set_chip(PRIMARY_IRQ, &irq_chip2); irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); } demux.c demux_handler(...) { for_each_secondary_irq(irq) generic_handle_irq(irq); } init() { irq_set_demux_handler(PRIMARY_IRQ, demux_handler); } versus: irqchip1.c struct irq_chip1; init() { irq_set_chip(PRIMARY_IRQ, &irq_chip1); } irqchip2.c struct irq_chip2; init() { irq_set_chip(PRIMARY_IRQ, &irq_chip2); } demux.c demux_handler(...) { if (chip->irq_ack()) chip->irq_ack(); for_each_secondary_irq(irq) generic_handle_irq(irq); if (chip->irq_eoi()) chip->irq_eoi(); } init() { irq_set_chained_handler(PRIMARY_IRQ, demux_handler); } Both are functionally equivivalent except that - my solution adds an indirect call - your solution requires explicit knowledge about the underlying possible primary irq chips Now go to the above examples and add irq_chip3.c which requires another different method of treating the primary interrupt. Lets assume irq_chip3 is some oddball hardware. My solution will look like this: irqchip3.c struct irq_chip3; handle_primary_irq(int irq, struct irq_desc *desc) { fiddle_with_some_special_register(); desc->demux(); chip->irq_eoi(); } init() { irq_set_chip(PRIMARY_IRQ, &irq_chip3); irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); } Yours will look like: irqchip3.c struct irq_chip3; init() { irq_set_chip(PRIMARY_IRQ, &irq_chip3); } And change all affected demux handlers to: demux_handler(...) { if (running_on_irq_chip3) fiddle_with_some_special_register(); if (chip->irq_ack()) chip->irq_ack(); for_each_secondary_irq(irq) generic_handle_irq(irq); if (chip->irq_eoi()) chip->irq_eoi(); } If you are still convinced that adding that extra stuff to the demux handlers is the right way to go, I'm not in the way. I merily tried to provide a generic solution to this, which avoid adding all this extra knowledge into the demux handlers. > hell bent on breaking this for ARM. So, I say again - if you persist > with this, I'll replace the ARM IRQ implementation and stop using genirq > because you're taking it in a direction which will cause problems for > ARM. And I *really* mean that. You are not threatening me with that. You are threatening your ARM users as they rely on much more than the f*cking chained handler details. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 21:29 ` Thomas Gleixner @ 2011-03-01 23:14 ` Russell King - ARM Linux 2011-03-01 23:44 ` Thomas Gleixner 2011-03-02 8:53 ` Russell King - ARM Linux 2011-03-02 15:33 ` Will Deacon 2 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-03-01 23:14 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > Can you please take the time and explain me the difference of the > following: Can you please take the time to realise that sometimes the combination between the primary and secondary interrupt controller is so tied that this kind of thing can't happen? Sometimes the secondary interrupt controller *must* mask the parent interrupt because it has no masking capability of its own. Sometimes the secondary interrupt controller *must* cause the primary controller to ack the interrupt status *before* the secondary handler reads the status. You can not say "the primary controller behaves like X so we always do X" because the action required on the primary is a combination of how the primary behaves *and* how the secondary behaves. Consider for example an edge triggered interrupt connected to a secondary controller - yes, we have that combination. When you read the secondary status register and clear those interrupts which are pending, the secondary controller resets to inactive its output. If there's still pending interrupts, it re-asserts the output causing a new edge. If your primary 'flow' handler were to acknowledge the interrupt *after* the demux, you'd lose interrupts. However, if the very same primary interrupt controller is connected to a FPGA based oring of several interrupt sources, this has to ack before reading the status, read the status, process *all* interrupts, re-ack, re-read the status and repeat until the status register indicates no further interrupts are pending. You can't deal with these two situations if you tie all the primary flow handling outside of the secondary demux handler. And it's *wrong* to do so. The behaviour required for the primary controller inherently depends on the secondary controller. It may not be clean from a software point of view, but that's hardware for you, and we have to make this work. You *can't* do that by separating the primary flow from the demux. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 23:14 ` Russell King - ARM Linux @ 2011-03-01 23:44 ` Thomas Gleixner 2011-03-01 23:50 ` Russell King - ARM Linux 0 siblings, 1 reply; 27+ messages in thread From: Thomas Gleixner @ 2011-03-01 23:44 UTC (permalink / raw) To: linux-arm-kernel On Tue, 1 Mar 2011, Russell King - ARM Linux wrote: > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > > Can you please take the time and explain me the difference of the > > following: > > Can you please take the time to realise that sometimes the combination > between the primary and secondary interrupt controller is so tied that > this kind of thing can't happen? > > Sometimes the secondary interrupt controller *must* mask the parent > interrupt because it has no masking capability of its own. > > Sometimes the secondary interrupt controller *must* cause the primary > controller to ack the interrupt status *before* the secondary handler > reads the status. > > You can not say "the primary controller behaves like X so we always do > X" because the action required on the primary is a combination of how > the primary behaves *and* how the secondary behaves. > > Consider for example an edge triggered interrupt connected to a secondary > controller - yes, we have that combination. When you read the secondary > status register and clear those interrupts which are pending, the > secondary controller resets to inactive its output. If there's still > pending interrupts, it re-asserts the output causing a new edge. > > If your primary 'flow' handler were to acknowledge the interrupt *after* > the demux, you'd lose interrupts. > > However, if the very same primary interrupt controller is connected to > a FPGA based oring of several interrupt sources, this has to ack before > reading the status, read the status, process *all* interrupts, re-ack, > re-read the status and repeat until the status register indicates no > further interrupts are pending. > > You can't deal with these two situations if you tie all the primary > flow handling outside of the secondary demux handler. And it's > *wrong* to do so. The behaviour required for the primary controller > inherently depends on the secondary controller. > > It may not be clean from a software point of view, but that's hardware > for you, and we have to make this work. You *can't* do that by > separating the primary flow from the demux. You are talking about very specific cases, where the demux handler is tighlty integrated into the primary chip implementation. No discussion about that. I do not want to change anything there. Period. The problem which caused that discussion has nothing to do with the above. It's about bog standard nested controllers which do not have a tight coupling to the the primary chip. But changing the primary chip from ack based to eoi based requires to touch multiple demux handlers while a primary chip agnostic solution for these cases would avoid that. W/O any negative side effects. Your oddball FPGA example has nothing to do with that and can happily use the existing model. Such a demux handler will never have to deal with different primary chips. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 23:44 ` Thomas Gleixner @ 2011-03-01 23:50 ` Russell King - ARM Linux 0 siblings, 0 replies; 27+ messages in thread From: Russell King - ARM Linux @ 2011-03-01 23:50 UTC (permalink / raw) To: linux-arm-kernel On Wed, Mar 02, 2011 at 12:44:05AM +0100, Thomas Gleixner wrote: > The problem which caused that discussion has nothing to do with the > above. It's about bog standard nested controllers which do not have a > tight coupling to the the primary chip. But changing the primary chip > from ack based to eoi based requires to touch multiple demux handlers > while a primary chip agnostic solution for these cases would avoid > that. W/O any negative side effects. If you think its soo damned simple, maybe you should put the effort in and fix all this up, get it tested as fully working on the affected platforms, and show the patches then. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 21:29 ` Thomas Gleixner 2011-03-01 23:14 ` Russell King - ARM Linux @ 2011-03-02 8:53 ` Russell King - ARM Linux 2011-03-02 9:25 ` Thomas Gleixner 2011-03-02 15:33 ` Will Deacon 2 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-03-02 8:53 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > Errm. I did never say that we disable the parent interrupt by any > means except when the chained handler explicitely wants to do that, > which is pretty much pointlesss nowadays, as we run all interrupt > handlers with interrupts disabled. And that's now why some platforms struggle to work, and we're having to bodge around this - like the ARM platforms with MMC support. Like some other platforms where having IRQs disabled during IDE prevents interrupts being recevied for long periods of time (longer than the 100Hz tick period). I *violently* disagree with the direction that genirq is heading. It's *actively* breaking stuff. What's really annoying is that problems like the above I did point out, but you seem happy to completely ignore them. The result is that more and more ARM platforms *are* becoming utterly useless, or requiring additional complexity being shoved into subsystems to cope with this crap. What we need is a *decent* IRQ support system. Not something created out of religious arguments which is what we have now. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-02 8:53 ` Russell King - ARM Linux @ 2011-03-02 9:25 ` Thomas Gleixner 2011-03-02 19:17 ` Russell King - ARM Linux 0 siblings, 1 reply; 27+ messages in thread From: Thomas Gleixner @ 2011-03-02 9:25 UTC (permalink / raw) To: linux-arm-kernel On Wed, 2 Mar 2011, Russell King - ARM Linux wrote: > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > > Errm. I did never say that we disable the parent interrupt by any > > means except when the chained handler explicitely wants to do that, > > which is pretty much pointlesss nowadays, as we run all interrupt > > handlers with interrupts disabled. > > And that's now why some platforms struggle to work, and we're having > to bodge around this - like the ARM platforms with MMC support. Like Whats the problem for MMC? > some other platforms where having IRQs disabled during IDE prevents > interrupts being recevied for long periods of time (longer than the > 100Hz tick period). That was discussed to death already and the general agreement was that those handlers should either enable interrupts themself, when it's required, or being converted to threaded handlers. An interrupt handler or any other code section which runs more than 10ms with interrupts disabled is a bug by definition. > I *violently* disagree with the direction that genirq is heading. It's > *actively* breaking stuff. What's really annoying is that problems like > the above I did point out, but you seem happy to completely ignore them. > The result is that more and more ARM platforms *are* becoming utterly > useless, or requiring additional complexity being shoved into subsystems > to cope with this crap. > > What we need is a *decent* IRQ support system. Not something created out > of religious arguments which is what we have now. I'm not religious about it, at least not more than you with your total refusement to distinguish between special case oddball FPGA demux and bog standard functional irq chips. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-02 9:25 ` Thomas Gleixner @ 2011-03-02 19:17 ` Russell King - ARM Linux 2011-03-02 20:44 ` Thomas Gleixner 0 siblings, 1 reply; 27+ messages in thread From: Russell King - ARM Linux @ 2011-03-02 19:17 UTC (permalink / raw) To: linux-arm-kernel On Wed, Mar 02, 2011 at 10:25:02AM +0100, Thomas Gleixner wrote: > On Wed, 2 Mar 2011, Russell King - ARM Linux wrote: > > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > > > Errm. I did never say that we disable the parent interrupt by any > > > means except when the chained handler explicitely wants to do that, > > > which is pretty much pointlesss nowadays, as we run all interrupt > > > handlers with interrupts disabled. > > > > And that's now why some platforms struggle to work, and we're having > > to bodge around this - like the ARM platforms with MMC support. Like > > Whats the problem for MMC? The FIFO interrupt gets completely buggered by USB interrupts such that we can't read data fast enough. With the old IRQF_DISABLED system, the FIFO interrupt was marked IRQF_DISABLED as it was *fast* and needed not to be interrupted. Other interrupts didn't have IRQF_DISABLED set. That means such interrupts can be handled while other longer interrupts are in progress. All that's gone out of the window now that all IRQ handlers are run with IRQs permanently disabled. And when this kind of behaviour becomes system specific (eg, the USB device driver doesn't have this problem on x86 so why should it change) then it all becomes impossible. There's hardware modifications in progress to add deeper FIFOs (which need to be extended to something idiotic like 512 32-bit words to work around the problem) and adaptive clocking schemes which retry transfers at slower clock rates in the hope that the IRQ handler can keep up. It's a complete mess. Another example - a platform I've worked on requires tight timings from I2C in order for the CPU not to be spontaneously rebooted. A timeout on I2C will cause this. We managed to get this platform to work reliably by using IRQF_DISABLED and having the standard behaviour (IRQs enabled during other interrupts). Now - god knows if it'll work reliably anymore, my guess is that with latest kernels it'll be impossibly unreliable. > > some other platforms where having IRQs disabled during IDE prevents > > interrupts being recevied for long periods of time (longer than the > > 100Hz tick period). > > That was discussed to death already and the general agreement was that > those handlers should either enable interrupts themself, when it's > required, or being converted to threaded handlers. An interrupt > handler or any other code section which runs more than 10ms with > interrupts disabled is a bug by definition. I haven't noticed PATA getting this support. So how do platforms force device drivers which turn out to be problematical to conform to their timing issues when there wasn't a problem with previous kernels? > > I *violently* disagree with the direction that genirq is heading. It's > > *actively* breaking stuff. What's really annoying is that problems like > > the above I did point out, but you seem happy to completely ignore them. > > The result is that more and more ARM platforms *are* becoming utterly > > useless, or requiring additional complexity being shoved into subsystems > > to cope with this crap. > > > > What we need is a *decent* IRQ support system. Not something created out > > of religious arguments which is what we have now. > > I'm not religious about it, at least not more than you with your total > refusement to distinguish between special case oddball FPGA demux and > bog standard functional irq chips. Clearly you don't want to know about the problems all this stuff is causing. Maybe you like being responsible for breaking the most ARM platforms with your changes - are you trying for an entry in the Guiness Book of World Records for this, because I think that's where you're heading. I don't agree with the distinction that you're trying to make. It only works for a simple subset of cases - but it seems you're overall attitude is that you only care about a small subset of easy cases and to hell with everything else. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-02 19:17 ` Russell King - ARM Linux @ 2011-03-02 20:44 ` Thomas Gleixner 0 siblings, 0 replies; 27+ messages in thread From: Thomas Gleixner @ 2011-03-02 20:44 UTC (permalink / raw) To: linux-arm-kernel On Wed, 2 Mar 2011, Russell King - ARM Linux wrote: > On Wed, Mar 02, 2011 at 10:25:02AM +0100, Thomas Gleixner wrote: > > On Wed, 2 Mar 2011, Russell King - ARM Linux wrote: > > > On Tue, Mar 01, 2011 at 10:29:37PM +0100, Thomas Gleixner wrote: > > > > Errm. I did never say that we disable the parent interrupt by any > > > > means except when the chained handler explicitely wants to do that, > > > > which is pretty much pointlesss nowadays, as we run all interrupt > > > > handlers with interrupts disabled. > > > > > > And that's now why some platforms struggle to work, and we're having > > > to bodge around this - like the ARM platforms with MMC support. Like > > > > Whats the problem for MMC? > > The FIFO interrupt gets completely buggered by USB interrupts such that > we can't read data fast enough. With the old IRQF_DISABLED system, > the FIFO interrupt was marked IRQF_DISABLED as it was *fast* and needed > not to be interrupted. Other interrupts didn't have IRQF_DISABLED set. > > That means such interrupts can be handled while other longer interrupts > are in progress. All that's gone out of the window now that all IRQ > handlers are run with IRQs permanently disabled. > > And when this kind of behaviour becomes system specific (eg, the USB > device driver doesn't have this problem on x86 so why should it change) I wouldn't say that. USB is known to be a trouble spot even on x86. And Linus argued for enforcing IRQ_DISABLED to weed out such crap. Anything which runs longer than a few micro seconds in the hard interrupt context needs to be fixed. > I haven't noticed PATA getting this support. So how do platforms force > device drivers which turn out to be problematical to conform to their > timing issues when there wasn't a problem with previous kernels? As I said above. PATA was talked about going threaded for years, but nobody cared as the irq enabled vs. IRQ_DISABLED mode made things magically work most of the time, except when you added some other random driver, which required IRQ_DISABLED as well and spent a millisecond and more in its handler. I know that pain and I wasted enough time with the wreckage caused by long running handlers. So at some point we had to make a decision: Either let the status quo, which is not satisfying at all and causes headache left and right, or create a situation which enforces that shit gets cleaned up. The general consensus was that enforcing the shit to be cleaned up is the better long term strategy even when it causes some short term irritation. If that hits ARM full force, sorry, but it's not my lonely decision in order to get an Guinness Book entry. I assume, you know which drivers are responsible for this kind of wreckage. Could you provide a list? > > > I *violently* disagree with the direction that genirq is heading. It's > > > *actively* breaking stuff. What's really annoying is that problems like > > > the above I did point out, but you seem happy to completely ignore them. > > > The result is that more and more ARM platforms *are* becoming utterly > > > useless, or requiring additional complexity being shoved into subsystems > > > to cope with this crap. > > > > > > What we need is a *decent* IRQ support system. Not something created out > > > of religious arguments which is what we have now. > > > > I'm not religious about it, at least not more than you with your total > > refusement to distinguish between special case oddball FPGA demux and > > bog standard functional irq chips. > > Clearly you don't want to know about the problems all this stuff is > causing. Maybe you like being responsible for breaking the most ARM I want to hear about problems and I'm the least person who refuses to help. And you are very well aware of that. Let's take it the other way around. Do you want to know about the pain of maintaining generic infrastructure like this and being blocked to change a few things in the core code just because nobody is talking to you and takes it for granted, that fiddling in the guts of the generic code by circumventing it (and I'm not talking about chained handlers here) is a good idea, just because the header files are public accessible via include/linux ? > platforms with your changes - are you trying for an entry in the > Guiness Book of World Records for this, because I think that's where > you're heading. Didn't know that they have a category for that. Can you point me to the website where to apply? > I don't agree with the distinction that you're trying to make. It only > works for a simple subset of cases - but it seems you're overall attitude > is that you only care about a small subset of easy cases and to hell > with everything else. That's not true at all. I said more than once that I understand why you want to keep the original concept of those chained handlers and I'm not going to change anything about them. I saw it a bit too simplistic at the beginning of the discussion and admitted that and corrected my POV right away. I'm merily trying to help and provide infrastructure for repeating patterns. If those patterns happen to be the simple ones, then we still want to avoid copied and duplicated code all over the place. Again, I'm not trying to kill the current chained handlers at all neither do I believe that everything is nail because we have a hammer. I'm trying to understand the problems instead of being fixated on a method which turned out to solve one of them. Thanks, tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-01 21:29 ` Thomas Gleixner 2011-03-01 23:14 ` Russell King - ARM Linux 2011-03-02 8:53 ` Russell King - ARM Linux @ 2011-03-02 15:33 ` Will Deacon 2011-03-02 17:10 ` Thomas Gleixner 2 siblings, 1 reply; 27+ messages in thread From: Will Deacon @ 2011-03-02 15:33 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, > Can you please take the time and explain me the difference of the > following: > > irqchip1.c > > struct irq_chip1; > > handle_primary_irq(int irq, struct irq_desc *desc) > { > chip->irq_ack(); > desc->demux(); > } > > init() > { > irq_set_chip(PRIMARY_IRQ, &irq_chip1); > irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); > } I think with this approach you get the exact opposite problem; that is the primary irq_chip doesn't know which IRQs are going to be demuxed so it cannot know at init time which IRQs need their primary handler set. Is the idea that you set_primary_handler for all IRQs, stash that in the descriptor somewhere and then replace handle_irq with the primary handler when a demux handler is registered? I guess I'm missing something here, Cheers, Will ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-02 15:33 ` Will Deacon @ 2011-03-02 17:10 ` Thomas Gleixner 2011-03-04 11:47 ` Will Deacon 0 siblings, 1 reply; 27+ messages in thread From: Thomas Gleixner @ 2011-03-02 17:10 UTC (permalink / raw) To: linux-arm-kernel On Wed, 2 Mar 2011, Will Deacon wrote: > Hi Thomas, > > > Can you please take the time and explain me the difference of the > > following: > > > > irqchip1.c > > > > struct irq_chip1; > > > > handle_primary_irq(int irq, struct irq_desc *desc) > > { > > chip->irq_ack(); > > desc->demux(); > > } > > > > init() > > { > > irq_set_chip(PRIMARY_IRQ, &irq_chip1); > > irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); > > } > > I think with this approach you get the exact opposite problem; that > is the primary irq_chip doesn't know which IRQs are going to be > demuxed so it cannot know at init time which IRQs need their primary > handler set. Is the idea that you set_primary_handler for all IRQs, > stash that in the descriptor somewhere and then replace handle_irq > with the primary handler when a demux handler is registered? > > I guess I'm missing something here, No, I missed that. Darn, yes this would need storing the primary_handler in some separate pointer or having a callback into the chip implementation to make this fully distangled. Grmbl. tglx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] ARM: nmk: update GPIO chained IRQ handler to use EOI in parent chip 2011-03-02 17:10 ` Thomas Gleixner @ 2011-03-04 11:47 ` Will Deacon 0 siblings, 0 replies; 27+ messages in thread From: Will Deacon @ 2011-03-04 11:47 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, > > > irqchip1.c > > > > > > struct irq_chip1; > > > > > > handle_primary_irq(int irq, struct irq_desc *desc) > > > { > > > chip->irq_ack(); > > > desc->demux(); > > > } > > > > > > init() > > > { > > > irq_set_chip(PRIMARY_IRQ, &irq_chip1); > > > irq_set_primary_handler(PRIMARY_IRQ, handle_primary_irq); > > > } > > > > I think with this approach you get the exact opposite problem; that > > is the primary irq_chip doesn't know which IRQs are going to be > > demuxed so it cannot know at init time which IRQs need their primary > > handler set. Is the idea that you set_primary_handler for all IRQs, > > stash that in the descriptor somewhere and then replace handle_irq > > with the primary handler when a demux handler is registered? > > > > I guess I'm missing something here, > > No, I missed that. Darn, yes this would need storing the > primary_handler in some separate pointer or having a callback into the > chip implementation to make this fully distangled. You could remove the need for pointer switching by having a bitmap indicating the flow control supported by the primary chip and then having a single primary handler implementation which checks the map. Not especially nice but it does allow for demux handlers to exist where they are sufficient. Will ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2011-03-04 11:47 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-02-28 13:33 [PATCH v2 0/6] Migrate GIC to fasteoi flow control Will Deacon 2011-02-28 13:33 ` [PATCH 1/6] ARM: gic: use handle_fasteoi_irq for SPIs Will Deacon 2011-02-28 13:33 ` [PATCH 2/6] ARM: omap: update GPIO chained IRQ handler to use EOI in parent chip Will Deacon 2011-02-28 13:33 ` [PATCH 3/6] ARM: tegra: " Will Deacon 2011-03-01 13:11 ` Sergei Shtylyov 2011-03-01 13:24 ` Will Deacon 2011-02-28 13:33 ` [PATCH 4/6] ARM: s5pv310: update IRQ combiner " Will Deacon 2011-03-01 13:12 ` Sergei Shtylyov 2011-02-28 13:33 ` [PATCH 5/6] ARM: msm: update GPIO chained IRQ handler " Will Deacon 2011-02-28 13:33 ` [PATCH 6/6] ARM: nmk: " Will Deacon 2011-02-28 14:03 ` Russell King - ARM Linux 2011-02-28 18:09 ` Will Deacon 2011-02-28 19:16 ` Thomas Gleixner 2011-02-28 21:44 ` Russell King - ARM Linux 2011-03-01 10:57 ` Thomas Gleixner 2011-03-01 20:19 ` Russell King - ARM Linux 2011-03-01 21:29 ` Thomas Gleixner 2011-03-01 23:14 ` Russell King - ARM Linux 2011-03-01 23:44 ` Thomas Gleixner 2011-03-01 23:50 ` Russell King - ARM Linux 2011-03-02 8:53 ` Russell King - ARM Linux 2011-03-02 9:25 ` Thomas Gleixner 2011-03-02 19:17 ` Russell King - ARM Linux 2011-03-02 20:44 ` Thomas Gleixner 2011-03-02 15:33 ` Will Deacon 2011-03-02 17:10 ` Thomas Gleixner 2011-03-04 11:47 ` Will Deacon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.