| Message ID | 1311295758-27493-1-git-send-email-simon@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thomas, Nicolas, Lennert and Saeed, On Fri, Jul 22, 2011 at 02:49:18AM +0200, Simon Guinot wrote: > From: Simon Guinot <sguinot@lacie.com> > > This fixes a regression introduced by e59347a > "arm: orion: Use generic irq chip". > > The same interrupt mask cache (stored within struct irq_chip_generic) > is shared between all the irq_chip_type instances. As each irq_chip_type > can use a distinct mask register, share a single mask cache is not > correct. This bug affects Orion SoCs, which have separate mask registers > for edge and level interrupts. > > This patch move mask_cache from struct irq_chip_generic into struct > irq_chip_type. Note that the interrupt support for Samsung SoCs is also > slightly affected. > > Reported-by: Joey Oravec <joravec@drewtech.com> > Signed-off-by: Simon Guinot <sguinot@lacie.com> > --- > arch/arm/plat-samsung/irq-vic-timer.c | 6 +++- > include/linux/irq.h | 4 +- > kernel/irq/generic-chip.c | 38 +++++++++++++++++++------------- > 3 files changed, 28 insertions(+), 20 deletions(-) What do you think about this patch ? Even if the issue is not critical, this should be fixed. Regards, Simon > > diff --git a/arch/arm/plat-samsung/irq-vic-timer.c b/arch/arm/plat-samsung/irq-vic-timer.c > index f714d06..3bb0b26 100644 > --- a/arch/arm/plat-samsung/irq-vic-timer.c > +++ b/arch/arm/plat-samsung/irq-vic-timer.c > @@ -31,9 +31,11 @@ static void s3c_irq_demux_vic_timer(unsigned int irq, struct irq_desc *desc) > static void s3c_irq_timer_ack(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct irq_chip_type *ct = gc->chip_types; > + > u32 mask = (1 << 5) << (d->irq - gc->irq_base); > > - irq_reg_writel(mask | gc->mask_cache, gc->reg_base); > + irq_reg_writel(mask | ct->mask_cache, gc->reg_base); > } > > /** > @@ -68,7 +70,7 @@ void __init s3c_init_vic_timer_irq(unsigned int num, unsigned int timer_irq) > irq_setup_generic_chip(s3c_tgc, IRQ_MSK(num), IRQ_GC_INIT_MASK_CACHE, > IRQ_NOREQUEST | IRQ_NOPROBE, 0); > /* Clear the upper bits of the mask_cache*/ > - s3c_tgc->mask_cache &= 0x1f; > + ct->mask_cache &= 0x1f; > > for (i = 0; i < num; i++, timer_irq++) { > irq_set_chained_handler(pirq[i], s3c_irq_demux_vic_timer); > diff --git a/include/linux/irq.h b/include/linux/irq.h > index baa397e..5e5208e 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -608,6 +608,7 @@ struct irq_chip_regs { > * @regs: Register offsets for this chip > * @handler: Flow handler associated with this chip > * @type: Chip can handle these flow types > + * @mask_cache: Cached mask register > * > * A irq_generic_chip can have several instances of irq_chip_type when > * it requires different functions and register offsets for different > @@ -618,6 +619,7 @@ struct irq_chip_type { > struct irq_chip_regs regs; > irq_flow_handler_t handler; > u32 type; > + u32 mask_cache; > }; > > /** > @@ -626,7 +628,6 @@ struct irq_chip_type { > * @reg_base: Register base address (virtual) > * @irq_base: Interrupt base nr for this chip > * @irq_cnt: Number of interrupts handled by this chip > - * @mask_cache: Cached mask register > * @type_cache: Cached type register > * @polarity_cache: Cached polarity register > * @wake_enabled: Interrupt can wakeup from suspend > @@ -647,7 +648,6 @@ struct irq_chip_generic { > void __iomem *reg_base; > unsigned int irq_base; > unsigned int irq_cnt; > - u32 mask_cache; > u32 type_cache; > u32 polarity_cache; > u32 wake_enabled; > diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c > index 3a2cab4..e7d5c13 100644 > --- a/kernel/irq/generic-chip.c > +++ b/kernel/irq/generic-chip.c > @@ -15,9 +15,9 @@ > static LIST_HEAD(gc_list); > static DEFINE_RAW_SPINLOCK(gc_lock); > > -static inline struct irq_chip_regs *cur_regs(struct irq_data *d) > +static inline struct irq_chip_type *to_irq_chip_type(struct irq_data *d) > { > - return &container_of(d->chip, struct irq_chip_type, chip)->regs; > + return container_of(d->chip, struct irq_chip_type, chip); > } > > /** > @@ -38,11 +38,12 @@ void irq_gc_noop(struct irq_data *d) > void irq_gc_mask_disable_reg(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct irq_chip_type *ct = to_irq_chip_type(d); > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable); > - gc->mask_cache &= ~mask; > + irq_reg_writel(mask, gc->reg_base + ct->regs.disable); > + ct->mask_cache &= ~mask; > irq_gc_unlock(gc); > } > > @@ -56,11 +57,12 @@ void irq_gc_mask_disable_reg(struct irq_data *d) > void irq_gc_mask_set_bit(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct irq_chip_type *ct = to_irq_chip_type(d); > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - gc->mask_cache |= mask; > - irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask); > + ct->mask_cache |= mask; > + irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask); > irq_gc_unlock(gc); > } > > @@ -74,11 +76,12 @@ void irq_gc_mask_set_bit(struct irq_data *d) > void irq_gc_mask_clr_bit(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct irq_chip_type *ct = to_irq_chip_type(d); > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - gc->mask_cache &= ~mask; > - irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask); > + ct->mask_cache &= ~mask; > + irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask); > irq_gc_unlock(gc); > } > > @@ -92,11 +95,12 @@ void irq_gc_mask_clr_bit(struct irq_data *d) > void irq_gc_unmask_enable_reg(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + struct irq_chip_type *ct = to_irq_chip_type(d); > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable); > - gc->mask_cache |= mask; > + irq_reg_writel(mask, gc->reg_base + ct->regs.enable); > + ct->mask_cache |= mask; > irq_gc_unlock(gc); > } > > @@ -110,7 +114,7 @@ void irq_gc_ack_set_bit(struct irq_data *d) > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); > + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); > irq_gc_unlock(gc); > } > > @@ -124,7 +128,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d) > u32 mask = ~(1 << (d->irq - gc->irq_base)); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); > + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); > irq_gc_unlock(gc); > } > > @@ -138,8 +142,8 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d) > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); > + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.mask); > + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); > irq_gc_unlock(gc); > } > > @@ -153,7 +157,7 @@ void irq_gc_eoi(struct irq_data *d) > u32 mask = 1 << (d->irq - gc->irq_base); > > irq_gc_lock(gc); > - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi); > + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.eoi); > irq_gc_unlock(gc); > } > > @@ -243,7 +247,9 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk, > > /* Init mask cache ? */ > if (flags & IRQ_GC_INIT_MASK_CACHE) > - gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask); > + for (i = 0; i < gc->num_ct; i++) > + ct[i].mask_cache = > + irq_reg_readl(gc->reg_base + ct[i].regs.mask); > > for (i = gc->irq_base; msk; msk >>= 1, i++) { > if (!msk & 0x01) > -- > 1.7.5.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote: > From: Simon Guinot <sguinot@lacie.com> > > This fixes a regression introduced by e59347a > "arm: orion: Use generic irq chip". > > The same interrupt mask cache (stored within struct irq_chip_generic) > is shared between all the irq_chip_type instances. As each irq_chip_type > can use a distinct mask register, share a single mask cache is not > correct. This bug affects Orion SoCs, which have separate mask registers > for edge and level interrupts. > > This patch move mask_cache from struct irq_chip_generic into struct > irq_chip_type. Note that the interrupt support for Samsung SoCs is also > slightly affected. The patch looks to fix the issue with orion, but it seems that it won't work for SoC with multiple irq_chip_type that use one mask register. saeed
Hi Saeed, On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote: > On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote: > > From: Simon Guinot <sguinot@lacie.com> > > > > This fixes a regression introduced by e59347a > > "arm: orion: Use generic irq chip". > > > > The same interrupt mask cache (stored within struct irq_chip_generic) > > is shared between all the irq_chip_type instances. As each irq_chip_type > > can use a distinct mask register, share a single mask cache is not > > correct. This bug affects Orion SoCs, which have separate mask registers > > for edge and level interrupts. > > > > This patch move mask_cache from struct irq_chip_generic into struct > > irq_chip_type. Note that the interrupt support for Samsung SoCs is also > > slightly affected. > The patch looks to fix the issue with orion, but it seems that it > won't work for SoC with multiple irq_chip_type that use one mask > register. Yes indeed, but does such SoCs exists ? I mean that the only supported SoC using multiple irq_chip_type (for now) is Orion. What is the most generic case for edge/level interrupts ? shared or separated mask registers ? If Orion is the specific case, maybe we could provide a dedicated irq_mask() handler instead of using the generic one. It would be a little sad. Or we could find a way to make coexist this two mask cache policies (even if I don't see how to do that cleanly) ? Alternatively, we could allow to bypass this mask cache... Regards, Simon
On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote: > Hi Saeed, > > On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote: >> On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote: >> > From: Simon Guinot <sguinot@lacie.com> >> > >> > This fixes a regression introduced by e59347a >> > "arm: orion: Use generic irq chip". >> > >> > The same interrupt mask cache (stored within struct irq_chip_generic) >> > is shared between all the irq_chip_type instances. As each irq_chip_type >> > can use a distinct mask register, share a single mask cache is not >> > correct. This bug affects Orion SoCs, which have separate mask registers >> > for edge and level interrupts. >> > >> > This patch move mask_cache from struct irq_chip_generic into struct >> > irq_chip_type. Note that the interrupt support for Samsung SoCs is also >> > slightly affected. >> The patch looks to fix the issue with orion, but it seems that it >> won't work for SoC with multiple irq_chip_type that use one mask >> register. > > Yes indeed, but does such SoCs exists ? I mean that the only supported > SoC using multiple irq_chip_type (for now) is Orion. thats right, but as you code is generic, we should find some way to fix it or to prevent such devices to use this generic code. What is the most > generic case for edge/level interrupts ? shared or separated mask > registers ? orion gpios use separate mask. > > If Orion is the specific case, maybe we could provide a dedicated > irq_mask() handler instead of using the generic one. It would be a > little sad. I personally prefer this method, along with getting rid off the multiple irq_chip_types, my main consideration in this case is performance. the irq_gc_mask_clr_bit/irq_gc_ack_set_bit/irq_gc_unmask_enable_reg are critical since it get called for every (level) interrupt, and it would be great if you optimize those functions, I think you better do the following: 1. use pre-calculated offsets instead using gc->reg_base + cur_regs(d)->ack. 2. put all hot variables (lock, mask/ack/eoi register offset) in the same cache line if possible. regards saeed
Hi everyone, I know this issue is from two years ago... On 07/27/2011 10:45 AM, saeed bishara wrote: > On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote: >> Hi Saeed, >> >> On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote: >>> On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote: >>>> From: Simon Guinot <sguinot@lacie.com> >>>> >>>> This fixes a regression introduced by e59347a >>>> "arm: orion: Use generic irq chip". >>>> >>>> The same interrupt mask cache (stored within struct irq_chip_generic) >>>> is shared between all the irq_chip_type instances. As each irq_chip_type >>>> can use a distinct mask register, share a single mask cache is not >>>> correct. This bug affects Orion SoCs, which have separate mask registers >>>> for edge and level interrupts. >>>> >>>> This patch move mask_cache from struct irq_chip_generic into struct >>>> irq_chip_type. Note that the interrupt support for Samsung SoCs is also >>>> slightly affected. >>> The patch looks to fix the issue with orion, but it seems that it >>> won't work for SoC with multiple irq_chip_type that use one mask >>> register. >> >> Yes indeed, but does such SoCs exists ? I mean that the only supported >> SoC using multiple irq_chip_type (for now) is Orion. > thats right, but as you code is generic, we should find some way to > fix it or to prevent such devices to use this generic code. > What is the most >> generic case for edge/level interrupts ? shared or separated mask >> registers ? > orion gpios use separate mask. >> >> If Orion is the specific case, maybe we could provide a dedicated >> irq_mask() handler instead of using the generic one. It would be a >> little sad. > I personally prefer this method, along with getting rid off the > multiple irq_chip_types, my main consideration in this case is > performance. > the irq_gc_mask_clr_bit/irq_gc_ack_set_bit/irq_gc_unmask_enable_reg > are critical since it get called for every (level) interrupt, and it > would be great if you optimize those functions, > I think you better do the following: > 1. use pre-calculated offsets instead using gc->reg_base + cur_regs(d)->ack. > 2. put all hot variables (lock, mask/ack/eoi register offset) in the > same cache line if possible. > > regards > saeed I ran into the same problem (currently running 3.0.40)... but it looks like this patch was never applied anywhere. I also quickly scanned the log between now and then and could not find any reference to this problem. Was a fix ever committed or we still have this regression? Thanks a lot! Gerlando
On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote: > Hi everyone, > > I know this issue is from two years ago... > > On 07/27/2011 10:45 AM, saeed bishara wrote: > >On Tue, Jul 26, 2011 at 6:39 PM, Simon Guinot <simon@sequanux.org> wrote: > >>Hi Saeed, > >> > >>On Tue, Jul 26, 2011 at 05:35:50PM +0300, saeed bishara wrote: > >>>On Fri, Jul 22, 2011 at 3:49 AM, Simon Guinot <simon@sequanux.org> wrote: > >>>>From: Simon Guinot <sguinot@lacie.com> > >>>> > >>>>This fixes a regression introduced by e59347a > >>>>"arm: orion: Use generic irq chip". > >>>> > >>>>The same interrupt mask cache (stored within struct irq_chip_generic) > >>>>is shared between all the irq_chip_type instances. As each irq_chip_type > >>>>can use a distinct mask register, share a single mask cache is not > >>>>correct. This bug affects Orion SoCs, which have separate mask registers > >>>>for edge and level interrupts. > >>>> > >>>>This patch move mask_cache from struct irq_chip_generic into struct > >>>>irq_chip_type. Note that the interrupt support for Samsung SoCs is also > >>>>slightly affected. > >>>The patch looks to fix the issue with orion, but it seems that it > >>>won't work for SoC with multiple irq_chip_type that use one mask > >>>register. > >> > >>Yes indeed, but does such SoCs exists ? I mean that the only supported > >>SoC using multiple irq_chip_type (for now) is Orion. > >thats right, but as you code is generic, we should find some way to > >fix it or to prevent such devices to use this generic code. > > What is the most > >>generic case for edge/level interrupts ? shared or separated mask > >>registers ? > >orion gpios use separate mask. > >> > >>If Orion is the specific case, maybe we could provide a dedicated > >>irq_mask() handler instead of using the generic one. It would be a > >>little sad. > >I personally prefer this method, along with getting rid off the > >multiple irq_chip_types, my main consideration in this case is > >performance. > >the irq_gc_mask_clr_bit/irq_gc_ack_set_bit/irq_gc_unmask_enable_reg > >are critical since it get called for every (level) interrupt, and it > >would be great if you optimize those functions, > >I think you better do the following: > >1. use pre-calculated offsets instead using gc->reg_base + cur_regs(d)->ack. > >2. put all hot variables (lock, mask/ack/eoi register offset) in the > >same cache line if possible. > > > >regards > >saeed > > I ran into the same problem (currently running 3.0.40)... but it > looks like this patch was never applied anywhere. > I also quickly scanned the log between now and then and could not > find any reference to this problem. Was a fix ever committed or we > still have this regression? Hi Gerlando, AFAIK this issue is still here. I include the current Orion maintainers in the Cc list. Regards, Simon
Hi Simon, On 03/04/2013 04:44 PM, Simon Guinot wrote: > Hi Gerlando, > > AFAIK this issue is still here. I include the current Orion maintainers > in the Cc list. Thanks! And BTW, your patch solves the problem on my side. :-) Regards, Gerlando
On Mon, 4 Mar 2013, Simon Guinot wrote: > On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote: > > AFAIK this issue is still here. I include the current Orion maintainers > in the Cc list. Can you please resend the patch?
On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote: > On Mon, 4 Mar 2013, Simon Guinot wrote: > > On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote: > > > > AFAIK this issue is still here. I include the current Orion maintainers > > in the Cc list. > > Can you please resend the patch? Hi Thomas, Do you agree with the original patch concept: moving mask_cache from struct irq_chip_generic into struct irq_chip_type ? This allows to handle the irq_chips which have separate mask registers for the edge and level interrupts. At the time of this patch, this was the only existing case. Saeed objected that the patch was not correct because this was breaking "hypothetic" devices with multiple irq_chip_type and a single mask register. The other option was to provide some dedicated irq_mask handlers for the Orion irq chip. As I was comfortable with none of this solutions, I kind of let it lie. Let me know you advice. Simon > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 6 Mar 2013, Simon Guinot wrote: > On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote: > > On Mon, 4 Mar 2013, Simon Guinot wrote: > > > On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote: > > > > > > AFAIK this issue is still here. I include the current Orion maintainers > > > in the Cc list. > > > > Can you please resend the patch? > > Hi Thomas, > > Do you agree with the original patch concept: moving mask_cache from > struct irq_chip_generic into struct irq_chip_type ? Not really. See below. > This allows to handle the irq_chips which have separate mask registers > for the edge and level interrupts. At the time of this patch, this was > the only existing case. > > Saeed objected that the patch was not correct because this was breaking > "hypothetic" devices with multiple irq_chip_type and a single mask > register. Not so hypothetic. There _are_ irq controllers out there which use the same mask register for both level and edge type irqs. > The other option was to provide some dedicated irq_mask handlers for the > Orion irq chip. I'd rather refactor the core code so it uses a pointer to the mask_cache. The default would be to let it point to gc->mask_cache and optionally let it point to ct->mask_cache. We'd need to store the flag in the gc struct so we can redirect the pointer to the ct->mask_cache in irq_setup_alt_chip(). Thanks, tglx
On Wed, Mar 06, 2013 at 04:19:30PM +0100, Thomas Gleixner wrote: > On Wed, 6 Mar 2013, Simon Guinot wrote: > > > On Tue, Mar 05, 2013 at 11:15:04AM +0100, Thomas Gleixner wrote: > > > On Mon, 4 Mar 2013, Simon Guinot wrote: > > > > On Mon, Mar 04, 2013 at 03:28:14PM +0100, Gerlando Falauto wrote: > > > > > > > > AFAIK this issue is still here. I include the current Orion maintainers > > > > in the Cc list. > > > > > > Can you please resend the patch? > > > > Hi Thomas, > > > > Do you agree with the original patch concept: moving mask_cache from > > struct irq_chip_generic into struct irq_chip_type ? > > Not really. See below. > > > This allows to handle the irq_chips which have separate mask registers > > for the edge and level interrupts. At the time of this patch, this was > > the only existing case. > > > > Saeed objected that the patch was not correct because this was breaking > > "hypothetic" devices with multiple irq_chip_type and a single mask > > register. > > Not so hypothetic. There _are_ irq controllers out there which use the > same mask register for both level and edge type irqs. Sure, but if the same mask register is used, then a single irq_chip_type should be able to handle both level and edge interrupts ? I mean, if one needs to register different irq_chip_type for edge and level interrupts, it is most likely because the registers are not the same... Simon > > > The other option was to provide some dedicated irq_mask handlers for the > > Orion irq chip. > > I'd rather refactor the core code so it uses a pointer to the > mask_cache. The default would be to let it point to gc->mask_cache and > optionally let it point to ct->mask_cache. We'd need to store the flag > in the gc struct so we can redirect the pointer to the ct->mask_cache > in irq_setup_alt_chip(). > > Thanks, > > tglx
On Mon, 11 Mar 2013, Simon Guinot wrote: > On Wed, Mar 06, 2013 at 04:19:30PM +0100, Thomas Gleixner wrote: > > Not so hypothetic. There _are_ irq controllers out there which use the > > same mask register for both level and edge type irqs. > > Sure, but if the same mask register is used, then a single irq_chip_type > should be able to handle both level and edge interrupts ? I mean, if one > needs to register different irq_chip_type for edge and level interrupts, > it is most likely because the registers are not the same... No, it's because first of all the types have different flow handlers and because they can have a different irq_chip due to different callbacks while still having the necessarity to share a single mask_cache. Care to look at struct irq_chip_type as a whole and not just from the POV of your particular chip incarnation? Again. Here is the solution to the problem: > > I'd rather refactor the core code so it uses a pointer to the > > mask_cache. The default would be to let it point to gc->mask_cache and > > optionally let it point to ct->mask_cache. We'd need to store the flag > > in the gc struct so we can redirect the pointer to the ct->mask_cache > > in irq_setup_alt_chip(). Stop arguing in circles and implement the 20 lines of patch already. Thanks, tglx
diff --git a/arch/arm/plat-samsung/irq-vic-timer.c b/arch/arm/plat-samsung/irq-vic-timer.c index f714d06..3bb0b26 100644 --- a/arch/arm/plat-samsung/irq-vic-timer.c +++ b/arch/arm/plat-samsung/irq-vic-timer.c @@ -31,9 +31,11 @@ static void s3c_irq_demux_vic_timer(unsigned int irq, struct irq_desc *desc) static void s3c_irq_timer_ack(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct irq_chip_type *ct = gc->chip_types; + u32 mask = (1 << 5) << (d->irq - gc->irq_base); - irq_reg_writel(mask | gc->mask_cache, gc->reg_base); + irq_reg_writel(mask | ct->mask_cache, gc->reg_base); } /** @@ -68,7 +70,7 @@ void __init s3c_init_vic_timer_irq(unsigned int num, unsigned int timer_irq) irq_setup_generic_chip(s3c_tgc, IRQ_MSK(num), IRQ_GC_INIT_MASK_CACHE, IRQ_NOREQUEST | IRQ_NOPROBE, 0); /* Clear the upper bits of the mask_cache*/ - s3c_tgc->mask_cache &= 0x1f; + ct->mask_cache &= 0x1f; for (i = 0; i < num; i++, timer_irq++) { irq_set_chained_handler(pirq[i], s3c_irq_demux_vic_timer); diff --git a/include/linux/irq.h b/include/linux/irq.h index baa397e..5e5208e 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -608,6 +608,7 @@ struct irq_chip_regs { * @regs: Register offsets for this chip * @handler: Flow handler associated with this chip * @type: Chip can handle these flow types + * @mask_cache: Cached mask register * * A irq_generic_chip can have several instances of irq_chip_type when * it requires different functions and register offsets for different @@ -618,6 +619,7 @@ struct irq_chip_type { struct irq_chip_regs regs; irq_flow_handler_t handler; u32 type; + u32 mask_cache; }; /** @@ -626,7 +628,6 @@ struct irq_chip_type { * @reg_base: Register base address (virtual) * @irq_base: Interrupt base nr for this chip * @irq_cnt: Number of interrupts handled by this chip - * @mask_cache: Cached mask register * @type_cache: Cached type register * @polarity_cache: Cached polarity register * @wake_enabled: Interrupt can wakeup from suspend @@ -647,7 +648,6 @@ struct irq_chip_generic { void __iomem *reg_base; unsigned int irq_base; unsigned int irq_cnt; - u32 mask_cache; u32 type_cache; u32 polarity_cache; u32 wake_enabled; diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c index 3a2cab4..e7d5c13 100644 --- a/kernel/irq/generic-chip.c +++ b/kernel/irq/generic-chip.c @@ -15,9 +15,9 @@ static LIST_HEAD(gc_list); static DEFINE_RAW_SPINLOCK(gc_lock); -static inline struct irq_chip_regs *cur_regs(struct irq_data *d) +static inline struct irq_chip_type *to_irq_chip_type(struct irq_data *d) { - return &container_of(d->chip, struct irq_chip_type, chip)->regs; + return container_of(d->chip, struct irq_chip_type, chip); } /** @@ -38,11 +38,12 @@ void irq_gc_noop(struct irq_data *d) void irq_gc_mask_disable_reg(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct irq_chip_type *ct = to_irq_chip_type(d); u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable); - gc->mask_cache &= ~mask; + irq_reg_writel(mask, gc->reg_base + ct->regs.disable); + ct->mask_cache &= ~mask; irq_gc_unlock(gc); } @@ -56,11 +57,12 @@ void irq_gc_mask_disable_reg(struct irq_data *d) void irq_gc_mask_set_bit(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct irq_chip_type *ct = to_irq_chip_type(d); u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - gc->mask_cache |= mask; - irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask); + ct->mask_cache |= mask; + irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask); irq_gc_unlock(gc); } @@ -74,11 +76,12 @@ void irq_gc_mask_set_bit(struct irq_data *d) void irq_gc_mask_clr_bit(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct irq_chip_type *ct = to_irq_chip_type(d); u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - gc->mask_cache &= ~mask; - irq_reg_writel(gc->mask_cache, gc->reg_base + cur_regs(d)->mask); + ct->mask_cache &= ~mask; + irq_reg_writel(ct->mask_cache, gc->reg_base + ct->regs.mask); irq_gc_unlock(gc); } @@ -92,11 +95,12 @@ void irq_gc_mask_clr_bit(struct irq_data *d) void irq_gc_unmask_enable_reg(struct irq_data *d) { struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); + struct irq_chip_type *ct = to_irq_chip_type(d); u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable); - gc->mask_cache |= mask; + irq_reg_writel(mask, gc->reg_base + ct->regs.enable); + ct->mask_cache |= mask; irq_gc_unlock(gc); } @@ -110,7 +114,7 @@ void irq_gc_ack_set_bit(struct irq_data *d) u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); irq_gc_unlock(gc); } @@ -124,7 +128,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d) u32 mask = ~(1 << (d->irq - gc->irq_base)); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); irq_gc_unlock(gc); } @@ -138,8 +142,8 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d) u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->mask); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->ack); + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.mask); + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.ack); irq_gc_unlock(gc); } @@ -153,7 +157,7 @@ void irq_gc_eoi(struct irq_data *d) u32 mask = 1 << (d->irq - gc->irq_base); irq_gc_lock(gc); - irq_reg_writel(mask, gc->reg_base + cur_regs(d)->eoi); + irq_reg_writel(mask, gc->reg_base + to_irq_chip_type(d)->regs.eoi); irq_gc_unlock(gc); } @@ -243,7 +247,9 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk, /* Init mask cache ? */ if (flags & IRQ_GC_INIT_MASK_CACHE) - gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask); + for (i = 0; i < gc->num_ct; i++) + ct[i].mask_cache = + irq_reg_readl(gc->reg_base + ct[i].regs.mask); for (i = gc->irq_base; msk; msk >>= 1, i++) { if (!msk & 0x01)