Re: [PATCH 1/6] irqchip: mips-gic: Add context saving for MIPS_REMOTEPROC

[Marc Zyngier]

Hi Matt,

On 02/09/16 10:59, Matt Redfearn wrote:
> The MIPS remote processor driver allows non-Linux firmware to take
> control of and execute on one of the systems VPEs. If that VPE is
> brought back under Linux, it is necessary to ensure that all GIC
> interrupts are routed and masked as Linux expects them, as the firmware
> can have done anything it likes with the GIC configuration (hopefully
> just for that VPEs local interrupt sources, but allow for shared
> external interrupts as well).
> 
> The configuration of shared and local CPU interrupts is maintained and
> updated every time a change is made. When a CPU is brought online, the
> saved configuration is restored.
> 
> These functions will also be useful for restoring GIC context after a
> suspend to RAM.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> ---
> 
>  drivers/irqchip/irq-mips-gic.c | 185 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 178 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 83f498393a7f..5ba1fe1468ce 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -8,6 +8,7 @@
>   */
>  #include <linux/bitmap.h>
>  #include <linux/clocksource.h>
> +#include <linux/cpu.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> @@ -56,6 +57,47 @@ static unsigned int timer_cpu_pin;
>  static struct irq_chip gic_level_irq_controller, gic_edge_irq_controller;
>  DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS);
>  
> +#if defined(CONFIG_MIPS_RPROC)
> +#define CONTEXT_SAVING
> +#endif
> +
> +#ifdef CONTEXT_SAVING

This looks really cumbersome. Why not make everything depend on
CONFIG_MIPS_RPROC instead?

> +static struct {
> +	unsigned mask:		GIC_NUM_LOCAL_INTRS;

nit/personal taste: can't you make this a normal type instead of a
bitfield? Considering that GIC_NUM_LOCAL_INTRS is hardcoded to 7, I'd
rather see a u8... or even an unsigned long if you have to use bitmap
operations on it.

> +} gic_local_state[NR_CPUS];

This looks like this really should be a percpu variable.

> +
> +#define gic_save_local_rmask(vpe, i)	(gic_local_state[vpe].mask &= i)
> +#define gic_save_local_smask(vpe, i)	(gic_local_state[vpe].mask |= i)
> +
> +static struct {
> +	unsigned vpe:		8;
> +	unsigned pin:		4;
> +
> +	unsigned polarity:	1;
> +	unsigned trigger:	1;
> +	unsigned dual_edge:	1;
> +	unsigned mask:		1;
> +} gic_shared_state[GIC_MAX_INTRS];
> +
> +#define gic_save_shared_vpe(i, v)	gic_shared_state[i].vpe = v
> +#define gic_save_shared_pin(i, p)	gic_shared_state[i].pin = p
> +#define gic_save_shared_polarity(i, p)	gic_shared_state[i].polarity = p
> +#define gic_save_shared_trigger(i, t)	gic_shared_state[i].trigger = t
> +#define gic_save_shared_dual_edge(i, d)	gic_shared_state[i].dual_edge = d
> +#define gic_save_shared_mask(i, m)	gic_shared_state[i].mask = m

Why don't you make these static functions? The compiler will inline them
nicely, and that will save you fixing them (they all miss proper
bracketing of arguments).

> +
> +#else
> +#define gic_save_local_rmask(vpe, i)
> +#define gic_save_local_smask(vpe, i)
> +
> +#define gic_save_shared_vpe(i, v)
> +#define gic_save_shared_pin(i, p)
> +#define gic_save_shared_polarity(i, p)
> +#define gic_save_shared_trigger(i, t)
> +#define gic_save_shared_dual_edge(i, d)
> +#define gic_save_shared_mask(i, m)

Please make those a "do { } while(0)" construct, so that the trailing
semi-colon is properly swallowed.

> +#endif /* CONTEXT_SAVING */
> +
>  static void __gic_irq_dispatch(void);
>  
>  static inline u32 gic_read32(unsigned int reg)
> @@ -105,52 +147,94 @@ static inline void gic_update_bits(unsigned int reg, unsigned long mask,
>  	gic_write(reg, regval);
>  }
>  
> -static inline void gic_reset_mask(unsigned int intr)
> +static inline void gic_write_reset_mask(unsigned int intr)
>  {
>  	gic_write(GIC_REG(SHARED, GIC_SH_RMASK) + GIC_INTR_OFS(intr),
>  		  1ul << GIC_INTR_BIT(intr));
>  }
>  
> -static inline void gic_set_mask(unsigned int intr)
> +static inline void gic_reset_mask(unsigned int intr)
> +{
> +	gic_save_shared_mask(intr, 0);
> +	gic_write_reset_mask(intr);
> +}
> +
> +static inline void gic_write_set_mask(unsigned int intr)
>  {
>  	gic_write(GIC_REG(SHARED, GIC_SH_SMASK) + GIC_INTR_OFS(intr),
>  		  1ul << GIC_INTR_BIT(intr));
>  }
>  
> -static inline void gic_set_polarity(unsigned int intr, unsigned int pol)
> +static inline void gic_set_mask(unsigned int intr)
> +{
> +	gic_save_shared_mask(intr, 1);
> +	gic_write_set_mask(intr);
> +}
> +
> +static inline void gic_write_polarity(unsigned int intr, unsigned int pol)
>  {
>  	gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_POLARITY) +
>  			GIC_INTR_OFS(intr), 1ul << GIC_INTR_BIT(intr),
>  			(unsigned long)pol << GIC_INTR_BIT(intr));
>  }
>  
> -static inline void gic_set_trigger(unsigned int intr, unsigned int trig)
> +static inline void gic_set_polarity(unsigned int intr, unsigned int pol)
> +{
> +	gic_save_shared_polarity(intr, pol);
> +	gic_write_polarity(intr, pol);
> +}
> +
> +static inline void gic_write_trigger(unsigned int intr, unsigned int trig)
>  {
>  	gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_TRIGGER) +
>  			GIC_INTR_OFS(intr), 1ul << GIC_INTR_BIT(intr),
>  			(unsigned long)trig << GIC_INTR_BIT(intr));
>  }
>  
> -static inline void gic_set_dual_edge(unsigned int intr, unsigned int dual)
> +static inline void gic_set_trigger(unsigned int intr, unsigned int trig)
> +{
> +	gic_save_shared_trigger(intr, trig);
> +	gic_write_trigger(intr, trig);
> +}
> +
> +static inline void gic_write_dual_edge(unsigned int intr, unsigned int dual)
>  {
>  	gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_DUAL) + GIC_INTR_OFS(intr),
>  			1ul << GIC_INTR_BIT(intr),
>  			(unsigned long)dual << GIC_INTR_BIT(intr));
>  }
>  
> -static inline void gic_map_to_pin(unsigned int intr, unsigned int pin)
> +static inline void gic_set_dual_edge(unsigned int intr, unsigned int dual)
> +{
> +	gic_save_shared_dual_edge(intr, dual);
> +	gic_write_dual_edge(intr, dual);
> +}
> +
> +static inline void gic_write_map_to_pin(unsigned int intr, unsigned int pin)
>  {
>  	gic_write32(GIC_REG(SHARED, GIC_SH_INTR_MAP_TO_PIN_BASE) +
>  		    GIC_SH_MAP_TO_PIN(intr), GIC_MAP_TO_PIN_MSK | pin);
>  }
>  
> -static inline void gic_map_to_vpe(unsigned int intr, unsigned int vpe)
> +static inline void gic_map_to_pin(unsigned int intr, unsigned int pin)
> +{
> +	gic_save_shared_pin(intr, pin);
> +	gic_write_map_to_pin(intr, pin);
> +}
> +
> +static inline void gic_write_map_to_vpe(unsigned int intr, unsigned int vpe)
>  {
>  	gic_write(GIC_REG(SHARED, GIC_SH_INTR_MAP_TO_VPE_BASE) +
>  		  GIC_SH_MAP_TO_VPE_REG_OFF(intr, vpe),
>  		  GIC_SH_MAP_TO_VPE_REG_BIT(vpe));
>  }
>  
> +static inline void gic_map_to_vpe(unsigned int intr, unsigned int vpe)
> +{
> +	gic_save_shared_vpe(intr, vpe);
> +	gic_write_map_to_vpe(intr, vpe);
> +}
> +
>  #ifdef CONFIG_CLKSRC_MIPS_GIC
>  cycle_t gic_read_count(void)
>  {
> @@ -537,6 +621,7 @@ static void gic_mask_local_irq(struct irq_data *d)
>  {
>  	int intr = GIC_HWIRQ_TO_LOCAL(d->hwirq);
>  
> +	gic_save_local_rmask(smp_processor_id(), (1 << intr));
>  	gic_write32(GIC_REG(VPE_LOCAL, GIC_VPE_RMASK), 1 << intr);
>  }
>  
> @@ -544,6 +629,7 @@ static void gic_unmask_local_irq(struct irq_data *d)
>  {
>  	int intr = GIC_HWIRQ_TO_LOCAL(d->hwirq);
>  
> +	gic_save_local_smask(smp_processor_id(), (1 << intr));
>  	gic_write32(GIC_REG(VPE_LOCAL, GIC_VPE_SMASK), 1 << intr);
>  }
>  
> @@ -561,6 +647,7 @@ static void gic_mask_local_irq_all_vpes(struct irq_data *d)
>  
>  	spin_lock_irqsave(&gic_lock, flags);
>  	for (i = 0; i < gic_vpes; i++) {
> +		gic_save_local_rmask(mips_cm_vp_id(i), 1 << intr);
>  		gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR),
>  			  mips_cm_vp_id(i));
>  		gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_RMASK), 1 << intr);
> @@ -576,6 +663,7 @@ static void gic_unmask_local_irq_all_vpes(struct irq_data *d)
>  
>  	spin_lock_irqsave(&gic_lock, flags);
>  	for (i = 0; i < gic_vpes; i++) {
> +		gic_save_local_smask(mips_cm_vp_id(i), 1 << intr);
>  		gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR),
>  			  mips_cm_vp_id(i));
>  		gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_SMASK), 1 << intr);
> @@ -983,6 +1071,85 @@ static struct irq_domain_ops gic_ipi_domain_ops = {
>  	.match = gic_ipi_domain_match,
>  };
>  
> +#ifdef CONTEXT_SAVING
> +static void gic_restore_shared(void)
> +{
> +	unsigned long flags;
> +	int i;
> +
> +	spin_lock_irqsave(&gic_lock, flags);
> +	for (i = 0; i < gic_shared_intrs; i++) {
> +		gic_write_polarity(i, gic_shared_state[i].polarity);
> +		gic_write_trigger(i, gic_shared_state[i].trigger);
> +		gic_write_dual_edge(i, gic_shared_state[i].dual_edge);
> +		gic_write_map_to_vpe(i, gic_shared_state[i].vpe);
> +		gic_write_map_to_pin(i, gic_shared_state[i].pin);
> +
> +		if (gic_shared_state[i].mask)
> +			gic_write_set_mask(i);
> +		else
> +			gic_write_reset_mask(i);
> +	}
> +	spin_unlock_irqrestore(&gic_lock, flags);
> +}
> +
> +static void gic_restore_local(unsigned int vpe)
> +{
> +	int hw, virq, intr, mask;
> +	unsigned long flags;
> +
> +	for (hw = 0; hw < GIC_NUM_LOCAL_INTRS; hw++) {
> +		intr = GIC_LOCAL_TO_HWIRQ(hw);
> +		virq = irq_linear_revmap(gic_irq_domain, intr);
> +		gic_local_irq_domain_map(gic_irq_domain, virq, hw);
> +	}
> +
> +	local_irq_save(flags);
> +	gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR), vpe);
> +
> +	/* Enable EIC mode if necessary */
> +	gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_CTL), cpu_has_veic);
> +
> +	/* Restore interrupt masks */
> +	mask = gic_local_state[vpe].mask;
> +	gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_RMASK), ~mask);
> +	gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_SMASK), mask);
> +
> +	local_irq_restore(flags);
> +}
> +#endif /* CONTEXT_SAVING */
> +
> +#ifdef CONFIG_MIPS_RPROC

I'm even more confused now. How can you not have both CONFIG_MIPS_RPROC
and CONTEXT_SAVING defined at the same time?

> +/*
> + * The MIPS remote processor driver allows non-Linux firmware to take control
> + * of and execute on one of the systems VPEs. If that VPE is brought back under
> + * Linux, it is necessary to ensure that all GIC interrupts are routed and
> + * masked as Linux expects them, as the firmware can have done anything it
> + * likes with the GIC configuration (hopefully just for that VPEs local
> + * interrupt sources, but allow for shared external interrupts as well).
> + */
> +static int gic_cpu_notify(struct notifier_block *nfb, unsigned long action,
> +			       void *hcpu)
> +{
> +	unsigned int cpu = mips_cm_vp_id((unsigned long)hcpu);
> +
> +	switch (action) {
> +	case CPU_UP_PREPARE:
> +	case CPU_DOWN_FAILED:
> +		gic_restore_shared();
> +		gic_restore_local(cpu);
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block gic_cpu_notifier __refdata = {
> +	.notifier_call = gic_cpu_notify
> +};
> +#endif /* CONFIG_MIPS_RPROC */
> +
>  static void __init __gic_init(unsigned long gic_base_addr,
>  			      unsigned long gic_addrspace_size,
>  			      unsigned int cpu_vec, unsigned int irqbase,
> @@ -1082,6 +1249,10 @@ static void __init __gic_init(unsigned long gic_base_addr,
>  	}
>  
>  	gic_basic_init();
> +
> +#ifdef CONFIG_MIPS_RPROC
> +	register_hotcpu_notifier(&gic_cpu_notifier);
> +#endif /* CONFIG_MIPS_RPROC */
>  }
>  
>  void __init gic_init(unsigned long gic_base_addr,
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...