[PATCH] genirq: allow an alternative setup for the mask cache

[Holger Brunck]

On 03/14/2013 06:45 PM, Simon Guinot wrote:
> On Thu, Mar 14, 2013 at 05:10:30PM +0100, Holger Brunck wrote:
>> The same interrupt mask cache (stored within struct irq_chip_generic)
>> is shared between all the irq_chip_type instances by default. But this
>> does not work on Orion SOCs which have separate mask registers for edge
>> and level interrupts. Therefore refactor the code that we always use a
>> pointer to access the mask register. By default it points to
>> gc->mask_cache for Orion SOCs it points to ct->mask_cache which is
>> setup in irq_setup_alt_chip().
> 
> Thanks for patch. I was really willing to do it but you have been
> faster.
> 
>>
>> Signed-off-by: Holger Brunck <holger.brunck at keymile.com>
> 
> You may add here:
> 
> Reported-by: Joey Oravec <joravec at drewtech.com>
> 
> Joey took some time to report and describe this bug.
> 

ok will do.

>> ---
>>  include/linux/irq.h       |    3 +++
>>  kernel/irq/generic-chip.c |   19 ++++++++++++-------
>>  2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index bc4e066..6bf2447 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -654,6 +654,7 @@ struct irq_chip_type {
>>  	struct irq_chip_regs	regs;
>>  	irq_flow_handler_t	handler;
>>  	u32			type;
>> +	u32			mask_cache;
>>  };
>>  
>>  /**
>> @@ -663,6 +664,7 @@ struct irq_chip_type {
>>   * @irq_base:		Interrupt base nr for this chip
>>   * @irq_cnt:		Number of interrupts handled by this chip
>>   * @mask_cache:		Cached mask register
>> + * @pmask_cache:	pointer to cached mask register
>>   * @type_cache:		Cached type register
>>   * @polarity_cache:	Cached polarity register
>>   * @wake_enabled:	Interrupt can wakeup from suspend
>> @@ -684,6 +686,7 @@ struct irq_chip_generic {
>>  	unsigned int		irq_base;
>>  	unsigned int		irq_cnt;
>>  	u32			mask_cache;
>> +	u32			*pmask_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 c89295a..1be14a3 100644
>> --- a/kernel/irq/generic-chip.c
>> +++ b/kernel/irq/generic-chip.c
>> @@ -43,7 +43,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
>>  
>>  	irq_gc_lock(gc);
>>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->disable);
>> -	gc->mask_cache &= ~mask;
>> +	*gc->pmask_cache &= ~mask;
>>  	irq_gc_unlock(gc);
>>  }
>>  
>> @@ -60,8 +60,8 @@ void irq_gc_mask_set_bit(struct irq_data *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);
>> +	*gc->pmask_cache |= mask;
>> +	irq_reg_writel(*gc->pmask_cache, gc->reg_base + cur_regs(d)->mask);
>>  	irq_gc_unlock(gc);
>>  }
>>  
>> @@ -78,8 +78,8 @@ void irq_gc_mask_clr_bit(struct irq_data *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);
>> +	*gc->pmask_cache &= ~mask;
>> +	irq_reg_writel(*gc->pmask_cache, gc->reg_base + cur_regs(d)->mask);
>>  	irq_gc_unlock(gc);
>>  }
>>  > @@ -97,7 +97,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
>>  
>>  	irq_gc_lock(gc);
>>  	irq_reg_writel(mask, gc->reg_base + cur_regs(d)->enable);
>> -	gc->mask_cache |= mask;
>> +	*gc->pmask_cache |= mask;
>>  	irq_gc_unlock(gc);
>>  }
>>  
>> @@ -243,9 +243,12 @@ void irq_setup_generic_chip(struct irq_chip_generic *gc, u32 msk,
>>  	list_add_tail(&gc->list, &gc_list);
>>  	raw_spin_unlock(&gc_lock);
>>  
>> +	/* Setup pointer to mask_cache */
>> +	gc->pmask_cache = &gc->mask_cache;
> 
> You need a flag here to choose between gc->mask_cache and
> ct->mask_cache.
> 

hm, even here? Isn't it enough to do this if irq_setup_alt_chip is called?

>> +
>>  	/* Init mask cache ? */
>>  	if (flags & IRQ_GC_INIT_MASK_CACHE)
>> -		gc->mask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
>> +		*gc->pmask_cache = irq_reg_readl(gc->reg_base + ct->regs.mask);
>>  
>>  	for (i = gc->irq_base; msk; msk >>= 1, i++) {
>>  		if (!(msk & 0x01))
>> @@ -275,6 +278,8 @@ int irq_setup_alt_chip(struct irq_data *d, unsigned int type)
>>  	struct irq_chip_type *ct = gc->chip_types;
>>  	unsigned int i;
>>  
>> +	/* Setup pointer to the mask_cache */
>> +	gc->pmask_cache = &ct->mask_cache;
> 
> You also need to check the flag here, to know if you need to switch
> pmask_cache on ct->mask_cache.
>

yes. I'll fix this in v2.

>>  	for (i = 0; i < gc->num_ct; i++, ct++) {
>>  		if (ct->type & type) {
>>  			d->chip = &ct->chip;
> 
> Additionally, you need to update drivers/gpio/gpio-mvebu.c which use
> gc->mask_cache. The gpio-mvebu driver is also impacted by the bug. 
>

I have no chance to test this, so couldn't you or someone else do this on top of
my changes?

Thanks,
Holger