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.