Re: [PATCH v11 06/19] irqchip: gic: Provide support for interrupt grouping

[Daniel Thompson <daniel.thompson@linaro.org>]

On 02/09/14 20:33, Russell King - ARM Linux wrote:
> On Tue, Sep 02, 2014 at 02:00:40PM +0100, Daniel Thompson wrote:
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 4b959e6..423707c 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -41,6 +41,9 @@
>>  #include <linux/irqchip/arm-gic.h>
>>  
>>  #include <asm/cputype.h>
>> +#ifdef CONFIG_FIQ
>> +#include <asm/fiq.h>
>> +#endif
> 
> Is there much advantage to this ifdef over providing a dummy asm/fiq.h
> in ARM64?
> 
>>  #include <asm/irq.h>
>>  #include <asm/exception.h>
>>  #include <asm/smp_plat.h>
>> @@ -68,6 +71,9 @@ struct gic_chip_data {
>>  #ifdef CONFIG_GIC_NON_BANKED
>>  	void __iomem *(*get_base)(union gic_base *);
>>  #endif
>> +#ifdef CONFIG_FIQ
>> +	bool fiq_enable;
>> +#endif
>>  };
>>  
>>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>> @@ -131,6 +137,16 @@ static inline void gic_set_base_accessor(struct gic_chip_data *data,
>>  #define gic_set_base_accessor(d, f)
>>  #endif
>>  
>> +#ifdef CONFIG_FIQ
>> +static inline bool gic_data_fiq_enable(struct gic_chip_data *data)
>> +{
>> +	return data->fiq_enable;
>> +}
>> +#else
>> +static inline bool gic_data_fiq_enable(
>> +		struct gic_chip_data *data) { return false; }
> 
> I really hate this style.  Just lay it out as a normal function.

Will do.


>> +	/*
>> +	 * If grouping is not available (not implemented or prohibited by
>> +	 * security mode) these registers a read-as-zero/write-ignored.
>> +	 * However as a precaution we restore the reset default regardless of
>> +	 * the result of the test.
>> +	 */
> 
> Have we found that this additional complexity is actually necessary?
> If not, we're over-engineering the code, making it more complex (and
> hence more likely to be buggy) for very little reason.
>
> Last night, I booted an unconditional version of this on OMAP3430, and
> OMAP4430.  It's also been booted on the range of iMX6 CPUs.  Nothing
> here has shown any signs of problems to having these registers written.

No, I haven't proven that most of the conditional code based on
gic_data_fiq_enable() is required.

I should certainly be safe to remove the conditional code from registers
specified was RAZ/WI.

I suspect we could also remove it from registers with bits that are
reserved (although spec. doesn't not state this explicitly).

I could aggressively remove the conditionals and keep the current code
on a branch to make it quicker to support any reported regressions.


>> +	/*
>> +	 * Set EnableGrp1/EnableGrp0 (bit 1 and 0) or EnableGrp (bit 0 only,
>> +	 * bit 1 ignored)
>> +	 */
>> +	if (gic_data_fiq_enable(gic))
>> +		writel_relaxed(3, base + GIC_DIST_CTRL);
>> +	else
>> +		writel_relaxed(1, base + GIC_DIST_CTRL);
> 
> If we are going to do this conditionally, and the only thing which
> is variable is the value to be written, I much prefer the conditional
> bit to be on the value and not the write.  The compiler doesn't always
> optimise these things very well.  So:
> 
> 	writel_relaxed(gic_data_fiq_enable(gic) ? 3 : 1, base + GIC_DIST_CTRL);
> 
> works well enough for me.  If you feel better by using a temporary
> local, that works for me too.

Ternary is fine by me although I'm inclined to be more aggressive with
removal of conditional code.

> 
>> +	if (gic_data_fiq_enable(gic))
>> +		writel_relaxed(0x1f, base + GIC_CPU_CTRL);
>> +	else
>> +		writel_relaxed(1, base + GIC_CPU_CTRL);
> 
> Same here.

Ok.

>> @@ -485,7 +564,10 @@ static void gic_dist_restore(unsigned int gic_nr)
>>  		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
>>  			dist_base + GIC_DIST_ENABLE_SET + i * 4);
>>  
>> -	writel_relaxed(1, dist_base + GIC_DIST_CTRL);
>> +	if (gic_data_fiq_enable(&gic_data[gic_nr]))
>> +		writel_relaxed(3, dist_base + GIC_DIST_CTRL);
>> +	else
>> +		writel_relaxed(1, dist_base + GIC_DIST_CTRL);
> 
> And here.

Ok.

>> @@ -542,7 +624,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>>  		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
>>  
>>  	writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK);
>> -	writel_relaxed(1, cpu_base + GIC_CPU_CTRL);
>> +	writel_relaxed(0x1f, cpu_base + GIC_CPU_CTRL);
> 
> Interestingly, here you write 0x1f unconditionally.

Oops. Can I pretend this was was a premonition?


>>  }
>>  
>>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
>> @@ -604,6 +686,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>  {
>>  	int cpu;
>>  	unsigned long flags, map = 0;
>> +	unsigned long softint;
>>  
>>  	raw_spin_lock_irqsave(&irq_controller_lock, flags);
>>  
>> @@ -618,7 +701,11 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
>>  	dmb(ishst);
>>  
>>  	/* this always happens on GIC0 */
>> -	writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
>> +	softint = map << 16 | irq;
>> +	if (gic_data_fiq_enable(&gic_data[0]))
>> +		softint |= 0x8000;
> 
> I guess that this always has to be done conditionally.  I'd prefer this
> test to be done slightly differently (and we might as well wrap in a bit
> of patch 9 here):
> 
> 	if (sgi_is_nonsecure(irq, &gic_data[0]))
> 		softint |= 0x8000;
> 
> which follows the true purpose of that bit.  This bit only has effect if
> we are running in secure mode, where it must match the status of the
> target interrupt (as programmed into GIC_DIST_IGROUP).
> 
> We probably should do this based on a bitmask of SGIs in the
> gic_chip_data, which is initialised according to how we've been able
> to setup the GIC_DIST_IGROUP register(s).

Will do.