genirq: move mask_cache into struct irq_chip_type
diff mbox

Message ID 1311295758-27493-1-git-send-email-simon@sequanux.org
State New, archived
Headers show

Commit Message

Simon Guinot July 22, 2011, 12:49 a.m. UTC
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(-)

Comments

Simon Guinot July 26, 2011, 2:11 p.m. UTC | #1
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
saeed bishara July 26, 2011, 2:35 p.m. UTC | #2
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
Simon Guinot July 26, 2011, 3:39 p.m. UTC | #3
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
saeed bishara July 27, 2011, 8:45 a.m. UTC | #4
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
Gerlando Falauto March 4, 2013, 2:28 p.m. UTC | #5
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
Simon Guinot March 4, 2013, 3:44 p.m. UTC | #6
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
Gerlando Falauto March 4, 2013, 5:20 p.m. UTC | #7
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
Thomas Gleixner March 5, 2013, 10:15 a.m. UTC | #8
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?
Simon Guinot March 6, 2013, 1:29 p.m. UTC | #9
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
Thomas Gleixner March 6, 2013, 3:19 p.m. UTC | #10
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
Simon Guinot March 11, 2013, 3:40 p.m. UTC | #11
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
Thomas Gleixner March 11, 2013, 9:08 p.m. UTC | #12
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

Patch
diff mbox

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)