Re: [PATCH 1/2] irqchip: gic: replace hex numbers with defines.

Re: [PATCH 1/2] irqchip: gic: replace hex numbers with defines.

[Marc Zyngier]

[adding Jason Cooper to the mix, as he's ramping up on the GIC
stuff... ;-)]

On Fri, May 16 2014 at 11:20:12 pm BST, Feng Kan <fkan@apm.com> wrote:
> This is to cleanup some hex numbers used in the code and replace
> then with defines to make the code cleaner.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Reviewed-by: Anup Patel <apatel@apm.com>
> ---
>  drivers/irqchip/irq-gic.c | 57 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57d165e..e6698a2 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -48,6 +48,20 @@
>  
>  #include "irqchip.h"
>  
> +#define GIC_DIST_ENABLE		0x1
> +#define GIC_DIST_DISABLE	0x0
> +#define GIC_CPU_ENABLE		0x1
> +#define GIC_CPU_DISABLE		0x0
> +#define GIC_INT_ID_MASK		0x3ff
> +#define GIC_INT_DEF_PRI		0xa0
> +#define GIC_INT_DEF_PRI_MASK	((GIC_INT_DEF_PRI << 24) |\
> +				(GIC_INT_DEF_PRI << 16) |\
> +				(GIC_INT_DEF_PRI << 8) | GIC_INT_DEF_PRI)

This is not a mask, but the interrupt priority for 4 interrupts.

> +#define GIC_INT_PRI_THRESHOLD	0xf0
> +#define GIC_INT_DISABLE_MASK	0xffffffff

This is not a mask to disable interrupt, but rather a mask for for 32
interrupts addressed by a single register. The action (enable or
disable) depends on the register you're writting to.

> +#define GIC_INT_SGI_MASK	0x0000ffff
> +#define GIC_INT_LVL_TRIGGER	0x0
> +
>  union gic_base {
>  	void __iomem *common_base;
>  	void __percpu * __iomem *percpu_base;
> @@ -291,7 +305,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  
>  	do {
>  		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> -		irqnr = irqstat & ~0x1c00;
> +		irqnr = irqstat & GIC_INT_ID_MASK;
>  
>  		if (likely(irqnr > 15 && irqnr < 1021)) {
>  			irqnr = irq_find_mapping(gic->domain, irqnr);
> @@ -322,7 +336,7 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
>  	status = readl_relaxed(gic_data_cpu_base(chip_data) + GIC_CPU_INTACK);
>  	raw_spin_unlock(&irq_controller_lock);
>  
> -	gic_irq = (status & 0x3ff);
> +	gic_irq = (status & GIC_INT_ID_MASK);
>  	if (gic_irq == 1023)

You could define this one as GIC_INT_SPURIOUS.

>  		goto out;
>  
> @@ -384,13 +398,14 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	unsigned int gic_irqs = gic->gic_irqs;
>  	void __iomem *base = gic_data_dist_base(gic);
>  
> -	writel_relaxed(0, base + GIC_DIST_CTRL);
> +	writel_relaxed(GIC_DIST_DISABLE, base + GIC_DIST_CTRL);
>  
>  	/*
>  	 * Set all global interrupts to be level triggered, active low.
>  	 */
>  	for (i = 32; i < gic_irqs; i += 16)
> -		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
> +		writel_relaxed(GIC_INT_LVL_TRIGGER,
> +				base + GIC_DIST_CONFIG + i * 4 / 16);
>  
>  	/*
>  	 * Set all global interrupts to this CPU only.
> @@ -405,16 +420,18 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	 * Set priority on all global interrupts.
>  	 */
>  	for (i = 32; i < gic_irqs; i += 4)
> -		writel_relaxed(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
> +		writel_relaxed(GIC_INT_DEF_PRI_MASK,
> +					base + GIC_DIST_PRI + i * 4 / 4);
>  
>  	/*
>  	 * Disable all interrupts.  Leave the PPI and SGIs alone
>  	 * as these enables are banked registers.
>  	 */
>  	for (i = 32; i < gic_irqs; i += 32)
> -		writel_relaxed(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
> +		writel_relaxed(GIC_INT_DISABLE_MASK,
> +				base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
>  
> -	writel_relaxed(1, base + GIC_DIST_CTRL);
> +	writel_relaxed(GIC_DIST_ENABLE, base + GIC_DIST_CTRL);
>  }
>  
>  static void gic_cpu_init(struct gic_chip_data *gic)
> @@ -443,23 +460,24 @@ static void gic_cpu_init(struct gic_chip_data *gic)
>  	 * Deal with the banked PPI and SGI interrupts - disable all
>  	 * PPI interrupts, ensure all SGI interrupts are enabled.
>  	 */
> -	writel_relaxed(0xffff0000, dist_base + GIC_DIST_ENABLE_CLEAR);
> -	writel_relaxed(0x0000ffff, dist_base + GIC_DIST_ENABLE_SET);
> +	writel_relaxed(~GIC_INT_SGI_MASK, dist_base + GIC_DIST_ENABLE_CLEAR);

Not quite. 0xffff0000 descibe the PPIs. It is not the opposite of the
set of SGIs.

> +	writel_relaxed(GIC_INT_SGI_MASK, dist_base + GIC_DIST_ENABLE_SET);
>
>  	/*
>  	 * Set priority on PPI and SGI interrupts
>  	 */
>  	for (i = 0; i < 32; i += 4)
> -		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4);
> +		writel_relaxed(GIC_INT_DEF_PRI_MASK,
> +				dist_base + GIC_DIST_PRI + i * 4 / 4);
>  
> -	writel_relaxed(0xf0, base + GIC_CPU_PRIMASK);
> -	writel_relaxed(1, base + GIC_CPU_CTRL);
> +	writel_relaxed(GIC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
> +	writel_relaxed(GIC_CPU_ENABLE, base + GIC_CPU_CTRL);
>  }
>  
>  void gic_cpu_if_down(void)
>  {
>  	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
> -	writel_relaxed(0, cpu_base + GIC_CPU_CTRL);
> +	writel_relaxed(GIC_CPU_DISABLE, cpu_base + GIC_CPU_CTRL);
>  }
>  
>  #ifdef CONFIG_CPU_PM
> @@ -519,14 +537,14 @@ static void gic_dist_restore(unsigned int gic_nr)
>  	if (!dist_base)
>  		return;
>  
> -	writel_relaxed(0, dist_base + GIC_DIST_CTRL);
> +	writel_relaxed(GIC_DIST_DISABLE, dist_base + GIC_DIST_CTRL);
>  
>  	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
>  		writel_relaxed(gic_data[gic_nr].saved_spi_conf[i],
>  			dist_base + GIC_DIST_CONFIG + i * 4);
>  
>  	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
> -		writel_relaxed(0xa0a0a0a0,
> +		writel_relaxed(GIC_INT_DEF_PRI_MASK,
>  			dist_base + GIC_DIST_PRI + i * 4);
>  
>  	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
> @@ -537,7 +555,7 @@ 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);
> +	writel_relaxed(GIC_DIST_ENABLE, dist_base + GIC_DIST_CTRL);
>  }
>  
>  static void gic_cpu_save(unsigned int gic_nr)
> @@ -591,10 +609,11 @@ static void gic_cpu_restore(unsigned int gic_nr)
>  		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
>  
>  	for (i = 0; i < DIV_ROUND_UP(32, 4); i++)
> -		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
> +		writel_relaxed(GIC_INT_DEF_PRI_MASK,
> +					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(GIC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
> +	writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
>  }
>  
>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)

Thanks,

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

Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Marc Zyngier]

On Fri, May 16 2014 at 11:20:13 pm BST, Feng Kan <fkan@apm.com> wrote:
> This change is made to preserve the GIC v2 bypass bits in the
> GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
> This code will preserve all bits configured by the bootloader regarding
> v2 bypass group bits. In the X-Gene platform, the bypass functionality
> is not used and bypass bits should not be changed by the kernel gic
> code as it could lead to incorrect behavior.
>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>
> Reviewed-by: Anup Patel <apatel@apm.com>
> ---
>  drivers/irqchip/irq-gic.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index e6698a2..9840ddc 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -61,6 +61,7 @@
>  #define GIC_INT_DISABLE_MASK	0xffffffff
>  #define GIC_INT_SGI_MASK	0x0000ffff
>  #define GIC_INT_LVL_TRIGGER	0x0
> +#define GIC_DIS_BYPASS_MASK	0x1e0
>  
>  union gic_base {
>  	void __iomem *common_base;
> @@ -391,6 +392,20 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
>  	return mask;
>  }
>  
> +static void gic_cpu_if_up(void)
> +{
> +	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
> +	u32 bypass;
> +
> +	/*
> +	 * Preserve bypass disable bits to be written back later
> +	 */
> +	bypass = readl(cpu_base + GIC_CPU_CTRL);
> +	bypass &= GIC_DIS_BYPASS_MASK;
> +
> +	writel_relaxed(bypass | GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
> +}
> +
>  static void __init gic_dist_init(struct gic_chip_data *gic)
>  {
>  	unsigned int i;
> @@ -471,13 +486,21 @@ static void gic_cpu_init(struct gic_chip_data *gic)
>  				dist_base + GIC_DIST_PRI + i * 4 / 4);
>  
>  	writel_relaxed(GIC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
> -	writel_relaxed(GIC_CPU_ENABLE, base + GIC_CPU_CTRL);
> +	gic_cpu_if_up();
>  }
>  
>  void gic_cpu_if_down(void)
>  {
>  	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
> -	writel_relaxed(GIC_CPU_DISABLE, cpu_base + GIC_CPU_CTRL);
> +	u32 bypass;
> +
> +	/*
> +	 * Preserve bypass disable bits to be written back later
> +	 */
> +	bypass = readl(cpu_base + GIC_CPU_CTRL);
> +	bypass &= GIC_DIS_BYPASS_MASK;
> +
> +	writel_relaxed(bypass | GIC_CPU_DISABLE, cpu_base + GIC_CPU_CTRL);

Given that on the way down, you know that the configuration is sane, you
can write this as:

    val =  readl(cpu_base + GIC_CPU_CTRL);
    val &= ~GIC_CPU_ENABLE;
    writel_relaxed(val, cpu_base + GIC_CPU_CTRL);

I think it looks a bit better.

>  }
>  
>  #ifdef CONFIG_CPU_PM
> @@ -613,7 +636,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>  					dist_base + GIC_DIST_PRI + i * 4);
>  
>  	writel_relaxed(GIC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
> -	writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
> +	gic_cpu_if_up();

Have you tested the save/restore path? It seems that we dont save
GICC_CTLR, so it may not do what you think it will...

>  }
>  
>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)

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

Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Feng Kan]

>>  #ifdef CONFIG_CPU_PM
>> @@ -613,7 +636,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>>                                       dist_base + GIC_DIST_PRI + i * 4);
>>
>>       writel_relaxed(GIC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
>> -     writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
>> +     gic_cpu_if_up();
>
> Have you tested the save/restore path? It seems that we dont save
> GICC_CTLR, so it may not do what you think it will...

We are debating which is the better method. Currently we are only
disabling the GIC distributor so it is not a problem. Later on, with
more
aggressive PM we could have the helper core to setup the GIC CTLR prior to
releasing out of the PM state. However, it seems it would be more
cleaner if we save off the GIC_CTLR bits in the gic_cpu_save. This
would add additional items in to the gic_chip_data. Would you be open
to that?

>
>>  }
>>
>>  static int gic_notifier(struct notifier_block *self, unsigned long cmd,      void *v)
>
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Marc Zyngier]

On Tue, May 20 2014 at  2:26:33 am BST, Feng Kan <fkan@apm.com> wrote:
>>>  #ifdef CONFIG_CPU_PM
>>> @@ -613,7 +636,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
>>>                                       dist_base + GIC_DIST_PRI + i * 4);
>>>
>>>       writel_relaxed(GIC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
>>> -     writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
>>> +     gic_cpu_if_up();
>>
>> Have you tested the save/restore path? It seems that we dont save
>> GICC_CTLR, so it may not do what you think it will...
>
> We are debating which is the better method. Currently we are only
> disabling the GIC distributor so it is not a problem. Later on, with
> more aggressive PM we could have the helper core to setup the GIC CTLR
> prior to releasing out of the PM state. However, it seems it would be
> more cleaner if we save off the GIC_CTLR bits in the
> gic_cpu_save. This would add additional items in to the
> gic_chip_data. Would you be open to that?

I'm open to anything that looks reasonable and doesn't introduce
regressions. Saving/restoring the CPU interface state should be fine.

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