Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
On Tue, May 20 2014 at 2:26:33 am BST, Feng Kan 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
On Tue, May 20 2014 at 2:26:33 am BST, Feng Kan f...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
>> #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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
On Fri, May 16 2014 at 11:20:13 pm BST, Feng Kan 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
> Signed-off-by: Feng Kan
> Reviewed-by: Anup Patel
> ---
> 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 0x
> #define GIC_INT_SGI_MASK 0x
> #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(_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(_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
#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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register
On Fri, May 16 2014 at 11:20:13 pm BST, Feng Kan f...@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 vk...@apm.com
Signed-off-by: Feng Kan f...@apm.com
Reviewed-by: Anup Patel apa...@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 0x
#define GIC_INT_SGI_MASK 0x
#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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
