[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

The Exynos interrupt combiner IP loses its state when the SoC enters
into a low power state during a Suspend-to-RAM. This means that if a
IRQ is used as a source, the interrupts for the devices are disabled
when the system is resumed from a sleep state so are not triggered.

Save the interrupt enable set register for each combiner group and
restore it after resume to make sure that the interrupts are enabled.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Hello,

I noticed this issue because after a S2R, IRQs for some devices didn't
trigger anymore but others continued working and all of them had lines
from a GPIO chip as their interrupt source.

The only difference was that the GPIO pins that were not working after
a resume, were the ones that had the interrupt combiner as interrupt
parent.

With this patch now all perhiperals are working correctly after a resume.
Tested on an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi
Chromebooks.

Best regards,
Javier

Changes since v1:
 - Clear masking bits before of the COMBINER_ENABLE_CLEAR register before
   restore IRQ enable set the Suggested by Chanho Park.
 - Fixes a typo in the commit message. Suggested by Peter Chubb.

 drivers/irqchip/exynos-combiner.c | 64 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/exynos-combiner.c b/drivers/irqchip/exynos-combiner.c
index 5945223b73fa..639e59c8eed3 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/interrupt.h>
@@ -34,9 +35,14 @@ struct combiner_chip_data {
 	unsigned int irq_mask;
 	void __iomem *base;
 	unsigned int parent_irq;
+#ifdef CONFIG_PM
+	u32 pm_save;
+#endif
 };
 
+static struct combiner_chip_data *combiner_data;
 static struct irq_domain *combiner_irq_domain;
+static unsigned int max_nr = 20;
 
 static inline void __iomem *combiner_base(struct irq_data *data)
 {
@@ -170,12 +176,10 @@ static struct irq_domain_ops combiner_irq_domain_ops = {
 };
 
 static void __init combiner_init(void __iomem *combiner_base,
-				 struct device_node *np,
-				 unsigned int max_nr)
+				 struct device_node *np)
 {
 	int i, irq;
 	unsigned int nr_irq;
-	struct combiner_chip_data *combiner_data;
 
 	nr_irq = max_nr * IRQ_IN_COMBINER;
 
@@ -201,11 +205,59 @@ static void __init combiner_init(void __iomem *combiner_base,
 	}
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * combiner_suspend - save interrupt combiner state before suspend
+ *
+ * Save the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state.
+ *
+ */
+static int combiner_suspend(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++)
+		combiner_data[i].pm_save =
+			__raw_readl(combiner_data[i].base + COMBINER_ENABLE_SET);
+
+	return 0;
+}
+
+/**
+ * combiner_resume - restore interrupt combiner state after resume
+ *
+ * Restore the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state on suspend.
+ *
+ */
+static void combiner_resume(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++) {
+		__raw_writel(combiner_data[i].irq_mask,
+			     combiner_data[i].base + COMBINER_ENABLE_CLEAR);
+		__raw_writel(combiner_data[i].pm_save,
+			     combiner_data[i].base + COMBINER_ENABLE_SET);
+	}
+}
+
+#else
+#define combiner_suspend	NULL
+#define combiner_resume		NULL
+#endif
+
+static struct syscore_ops combiner_syscore_ops = {
+	.suspend	= combiner_suspend,
+	.resume		= combiner_resume,
+};
+
 static int __init combiner_of_init(struct device_node *np,
 				   struct device_node *parent)
 {
 	void __iomem *combiner_base;
-	unsigned int max_nr = 20;
 
 	combiner_base = of_iomap(np, 0);
 	if (!combiner_base) {
@@ -219,7 +271,9 @@ static int __init combiner_of_init(struct device_node *np,
 			__func__, max_nr);
 	}
 
-	combiner_init(combiner_base, np, max_nr);
+	combiner_init(combiner_base, np);
+
+	register_syscore_ops(&combiner_syscore_ops);
 
 	return 0;
 }
-- 
2.1.4

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

The Exynos interrupt combiner IP loses its state when the SoC enters
into a low power state during a Suspend-to-RAM. This means that if a
IRQ is used as a source, the interrupts for the devices are disabled
when the system is resumed from a sleep state so are not triggered.

Save the interrupt enable set register for each combiner group and
restore it after resume to make sure that the interrupts are enabled.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Hello,

I noticed this issue because after a S2R, IRQs for some devices didn't
trigger anymore but others continued working and all of them had lines
from a GPIO chip as their interrupt source.

The only difference was that the GPIO pins that were not working after
a resume, were the ones that had the interrupt combiner as interrupt
parent.

With this patch now all perhiperals are working correctly after a resume.
Tested on an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi
Chromebooks.

Best regards,
Javier

Changes since v1:
 - Clear masking bits before of the COMBINER_ENABLE_CLEAR register before
   restore IRQ enable set the Suggested by Chanho Park.
 - Fixes a typo in the commit message. Suggested by Peter Chubb.

 drivers/irqchip/exynos-combiner.c | 64 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/exynos-combiner.c b/drivers/irqchip/exynos-combiner.c
index 5945223b73fa..639e59c8eed3 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/interrupt.h>
@@ -34,9 +35,14 @@ struct combiner_chip_data {
 	unsigned int irq_mask;
 	void __iomem *base;
 	unsigned int parent_irq;
+#ifdef CONFIG_PM
+	u32 pm_save;
+#endif
 };
 
+static struct combiner_chip_data *combiner_data;
 static struct irq_domain *combiner_irq_domain;
+static unsigned int max_nr = 20;
 
 static inline void __iomem *combiner_base(struct irq_data *data)
 {
@@ -170,12 +176,10 @@ static struct irq_domain_ops combiner_irq_domain_ops = {
 };
 
 static void __init combiner_init(void __iomem *combiner_base,
-				 struct device_node *np,
-				 unsigned int max_nr)
+				 struct device_node *np)
 {
 	int i, irq;
 	unsigned int nr_irq;
-	struct combiner_chip_data *combiner_data;
 
 	nr_irq = max_nr * IRQ_IN_COMBINER;
 
@@ -201,11 +205,59 @@ static void __init combiner_init(void __iomem *combiner_base,
 	}
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * combiner_suspend - save interrupt combiner state before suspend
+ *
+ * Save the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state.
+ *
+ */
+static int combiner_suspend(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++)
+		combiner_data[i].pm_save =
+			__raw_readl(combiner_data[i].base + COMBINER_ENABLE_SET);
+
+	return 0;
+}
+
+/**
+ * combiner_resume - restore interrupt combiner state after resume
+ *
+ * Restore the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state on suspend.
+ *
+ */
+static void combiner_resume(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++) {
+		__raw_writel(combiner_data[i].irq_mask,
+			     combiner_data[i].base + COMBINER_ENABLE_CLEAR);
+		__raw_writel(combiner_data[i].pm_save,
+			     combiner_data[i].base + COMBINER_ENABLE_SET);
+	}
+}
+
+#else
+#define combiner_suspend	NULL
+#define combiner_resume		NULL
+#endif
+
+static struct syscore_ops combiner_syscore_ops = {
+	.suspend	= combiner_suspend,
+	.resume		= combiner_resume,
+};
+
 static int __init combiner_of_init(struct device_node *np,
 				   struct device_node *parent)
 {
 	void __iomem *combiner_base;
-	unsigned int max_nr = 20;
 
 	combiner_base = of_iomap(np, 0);
 	if (!combiner_base) {
@@ -219,7 +271,9 @@ static int __init combiner_of_init(struct device_node *np,
 			__func__, max_nr);
 	}
 
-	combiner_init(combiner_base, np, max_nr);
+	combiner_init(combiner_base, np);
+
+	register_syscore_ops(&combiner_syscore_ops);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 12.06.2015 14:43, Javier Martinez Canillas wrote:
> The Exynos interrupt combiner IP loses its state when the SoC enters
> into a low power state during a Suspend-to-RAM. This means that if a
> IRQ is used as a source, the interrupts for the devices are disabled
> when the system is resumed from a sleep state so are not triggered.
> 
> Save the interrupt enable set register for each combiner group and
> restore it after resume to make sure that the interrupts are enabled.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Hello,
> 
> I noticed this issue because after a S2R, IRQs for some devices didn't
> trigger anymore but others continued working and all of them had lines
> from a GPIO chip as their interrupt source.
> 
> The only difference was that the GPIO pins that were not working after
> a resume, were the ones that had the interrupt combiner as interrupt
> parent.
> 
> With this patch now all perhiperals are working correctly after a resume.
> Tested on an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi
> Chromebooks.
> 
> Best regards,
> Javier
> 
> Changes since v1:
>  - Clear masking bits before of the COMBINER_ENABLE_CLEAR register before
>    restore IRQ enable set the Suggested by Chanho Park.
>  - Fixes a typo in the commit message. Suggested by Peter Chubb.

Looks okay,
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 12.06.2015 14:43, Javier Martinez Canillas wrote:
> The Exynos interrupt combiner IP loses its state when the SoC enters
> into a low power state during a Suspend-to-RAM. This means that if a
> IRQ is used as a source, the interrupts for the devices are disabled
> when the system is resumed from a sleep state so are not triggered.
> 
> Save the interrupt enable set register for each combiner group and
> restore it after resume to make sure that the interrupts are enabled.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
> 
> Hello,
> 
> I noticed this issue because after a S2R, IRQs for some devices didn't
> trigger anymore but others continued working and all of them had lines
> from a GPIO chip as their interrupt source.
> 
> The only difference was that the GPIO pins that were not working after
> a resume, were the ones that had the interrupt combiner as interrupt
> parent.
> 
> With this patch now all perhiperals are working correctly after a resume.
> Tested on an Exynos5250 Snow, Exynos5420 Peach Pit and Exynos5800 Peach Pi
> Chromebooks.
> 
> Best regards,
> Javier
> 
> Changes since v1:
>  - Clear masking bits before of the COMBINER_ENABLE_CLEAR register before
>    restore IRQ enable set the Suggested by Chanho Park.
>  - Fixes a typo in the commit message. Suggested by Peter Chubb.

Looks okay,
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 06:43, Javier Martinez Canillas wrote:
> The Exynos interrupt combiner IP loses its state when the SoC enters
> into a low power state during a Suspend-to-RAM. This means that if a
> IRQ is used as a source, the interrupts for the devices are disabled
> when the system is resumed from a sleep state so are not triggered.
>
> Save the interrupt enable set register for each combiner group and
> restore it after resume to make sure that the interrupts are enabled.
>

Not sure if you need this. IMO it's not clean and redundant though I
admit many drivers do exactly same thing. I am trying to remove or point
out those redundant code as irqchip core has options/flags to do what
you need. I assume there are no wakeup sources connected to this
combiner. Setting irqchip flags should solve this problem. A simple
patch below should do the job ?

-->8

diff --git a/drivers/irqchip/exynos-combiner.c 
b/drivers/irqchip/exynos-combiner.c
index 5945223b73fa..c0bcec59f829 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
  #ifdef CONFIG_SMP
         .irq_set_affinity       = combiner_set_affinity,
  #endif
+       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
  };

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 06:43, Javier Martinez Canillas wrote:
> The Exynos interrupt combiner IP loses its state when the SoC enters
> into a low power state during a Suspend-to-RAM. This means that if a
> IRQ is used as a source, the interrupts for the devices are disabled
> when the system is resumed from a sleep state so are not triggered.
>
> Save the interrupt enable set register for each combiner group and
> restore it after resume to make sure that the interrupts are enabled.
>

Not sure if you need this. IMO it's not clean and redundant though I
admit many drivers do exactly same thing. I am trying to remove or point
out those redundant code as irqchip core has options/flags to do what
you need. I assume there are no wakeup sources connected to this
combiner. Setting irqchip flags should solve this problem. A simple
patch below should do the job ?

-->8

diff --git a/drivers/irqchip/exynos-combiner.c 
b/drivers/irqchip/exynos-combiner.c
index 5945223b73fa..c0bcec59f829 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
  #ifdef CONFIG_SMP
         .irq_set_affinity       = combiner_set_affinity,
  #endif
+       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
  };

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 12.06.2015 19:10, Sudeep Holla wrote:
> 
> 
> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>> The Exynos interrupt combiner IP loses its state when the SoC enters
>> into a low power state during a Suspend-to-RAM. This means that if a
>> IRQ is used as a source, the interrupts for the devices are disabled
>> when the system is resumed from a sleep state so are not triggered.
>>
>> Save the interrupt enable set register for each combiner group and
>> restore it after resume to make sure that the interrupts are enabled.
>>
> 
> Not sure if you need this. IMO it's not clean and redundant though I
> admit many drivers do exactly same thing. I am trying to remove or point
> out those redundant code as irqchip core has options/flags to do what
> you need. I assume there are no wakeup sources connected to this
> combiner.

It may have wake up sources connected. Correct me if I am wrong but (at
least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be
external interrupts (e.g. power key on Exynos5250 Snow). I didn't check
other boards.

Best regards,
Krzysztof

> Setting irqchip flags should solve this problem. A simple
> patch below should do the job ?
> 
> -->8
> 
> diff --git a/drivers/irqchip/exynos-combiner.c
> b/drivers/irqchip/exynos-combiner.c
> index 5945223b73fa..c0bcec59f829 100644
> --- a/drivers/irqchip/exynos-combiner.c
> +++ b/drivers/irqchip/exynos-combiner.c
> @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
>  #ifdef CONFIG_SMP
>         .irq_set_affinity       = combiner_set_affinity,
>  #endif
> +       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
>  };
> 
> 
> 

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 12.06.2015 19:10, Sudeep Holla wrote:
> 
> 
> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>> The Exynos interrupt combiner IP loses its state when the SoC enters
>> into a low power state during a Suspend-to-RAM. This means that if a
>> IRQ is used as a source, the interrupts for the devices are disabled
>> when the system is resumed from a sleep state so are not triggered.
>>
>> Save the interrupt enable set register for each combiner group and
>> restore it after resume to make sure that the interrupts are enabled.
>>
> 
> Not sure if you need this. IMO it's not clean and redundant though I
> admit many drivers do exactly same thing. I am trying to remove or point
> out those redundant code as irqchip core has options/flags to do what
> you need. I assume there are no wakeup sources connected to this
> combiner.

It may have wake up sources connected. Correct me if I am wrong but (at
least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be
external interrupts (e.g. power key on Exynos5250 Snow). I didn't check
other boards.

Best regards,
Krzysztof

> Setting irqchip flags should solve this problem. A simple
> patch below should do the job ?
> 
> -->8
> 
> diff --git a/drivers/irqchip/exynos-combiner.c
> b/drivers/irqchip/exynos-combiner.c
> index 5945223b73fa..c0bcec59f829 100644
> --- a/drivers/irqchip/exynos-combiner.c
> +++ b/drivers/irqchip/exynos-combiner.c
> @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
>  #ifdef CONFIG_SMP
>         .irq_set_affinity       = combiner_set_affinity,
>  #endif
> +       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
>  };
> 
> 
> 

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 11:42, Krzysztof Kozlowski wrote:
> On 12.06.2015 19:10, Sudeep Holla wrote:
>>
>>
>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>> into a low power state during a Suspend-to-RAM. This means that if a
>>> IRQ is used as a source, the interrupts for the devices are disabled
>>> when the system is resumed from a sleep state so are not triggered.
>>>
>>> Save the interrupt enable set register for each combiner group and
>>> restore it after resume to make sure that the interrupts are enabled.
>>>
>>
>> Not sure if you need this. IMO it's not clean and redundant though I
>> admit many drivers do exactly same thing. I am trying to remove or point
>> out those redundant code as irqchip core has options/flags to do what
>> you need. I assume there are no wakeup sources connected to this
>> combiner.
>
> It may have wake up sources connected. Correct me if I am wrong but (at
> least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be
> external interrupts (e.g. power key on Exynos5250 Snow). I didn't check
> other boards.
>

In that case, this irqchip should implement irq_set_wake and the driver
implementing power key should use enable_irq_wake in the suspend path.
Just saving all the mask/enable registers is not scalable solution and
also useless if it's just one or to interrupts that are very few IRQs
registered/actively being used.

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 11:42, Krzysztof Kozlowski wrote:
> On 12.06.2015 19:10, Sudeep Holla wrote:
>>
>>
>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>> into a low power state during a Suspend-to-RAM. This means that if a
>>> IRQ is used as a source, the interrupts for the devices are disabled
>>> when the system is resumed from a sleep state so are not triggered.
>>>
>>> Save the interrupt enable set register for each combiner group and
>>> restore it after resume to make sure that the interrupts are enabled.
>>>
>>
>> Not sure if you need this. IMO it's not clean and redundant though I
>> admit many drivers do exactly same thing. I am trying to remove or point
>> out those redundant code as irqchip core has options/flags to do what
>> you need. I assume there are no wakeup sources connected to this
>> combiner.
>
> It may have wake up sources connected. Correct me if I am wrong but (at
> least) on Exynos5250 combiner takes care of gpx1 GPIO pins which may be
> external interrupts (e.g. power key on Exynos5250 Snow). I didn't check
> other boards.
>

In that case, this irqchip should implement irq_set_wake and the driver
implementing power key should use enable_irq_wake in the suspend path.
Just saving all the mask/enable registers is not scalable solution and
also useless if it's just one or to interrupts that are very few IRQs
registered/actively being used.

Regards,
Sudeep

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

Thanks a lot for the feedback.

On 06/12/2015 12:10 PM, Sudeep Holla wrote:
> 
> 
> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>> The Exynos interrupt combiner IP loses its state when the SoC enters
>> into a low power state during a Suspend-to-RAM. This means that if a
>> IRQ is used as a source, the interrupts for the devices are disabled
>> when the system is resumed from a sleep state so are not triggered.
>>
>> Save the interrupt enable set register for each combiner group and
>> restore it after resume to make sure that the interrupts are enabled.
>>
> 
> Not sure if you need this. IMO it's not clean and redundant though I
> admit many drivers do exactly same thing. I am trying to remove or point
> out those redundant code as irqchip core has options/flags to do what
> you need. I assume there are no wakeup sources connected to this

Yes, there are wakeup sources connected to this combiner. As Krzysztof
said some wakeup sources use a GPIO line as IRQ whose interrupt parent
is the combiner. As an example the Power gpio-key and cros_ec keyboard
for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.

Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
right thing though and call {enable,disable}_irq_wake() on the S2R path.

And in fact, the peripherals resume the system after a suspend but after
resume, interrupts are not triggered anymore.

> combiner. Setting irqchip flags should solve this problem. A simple
> patch below should do the job ?
>

I don't see how the below patch is going to work for the case I'm trying to
solve. If I understand correctly, the IRQCHIP_MASK_ON_SUSPEND flag is used
to force masking non wakeup interrupt in the suspend path (instead of just
disabling the interrupts on suspend and not masking at the hardware level)

But my problem is not about interrupts needed to be masked on suspend but
the opposite, that interrupts have to be unmasked on resume since the IP
loses its state so after a resume, interrupts are not triggered anymore.

So I also don't see how implementing irq_set_wake() as you suggested is
going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for this case?

> -->8
> 
> diff --git a/drivers/irqchip/exynos-combiner.c 
> b/drivers/irqchip/exynos-combiner.c
> index 5945223b73fa..c0bcec59f829 100644
> --- a/drivers/irqchip/exynos-combiner.c
> +++ b/drivers/irqchip/exynos-combiner.c
> @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
>   #ifdef CONFIG_SMP
>          .irq_set_affinity       = combiner_set_affinity,
>   #endif
> +       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
>   };
> 
> 

Best regards,
Javier

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

Thanks a lot for the feedback.

On 06/12/2015 12:10 PM, Sudeep Holla wrote:
> 
> 
> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>> The Exynos interrupt combiner IP loses its state when the SoC enters
>> into a low power state during a Suspend-to-RAM. This means that if a
>> IRQ is used as a source, the interrupts for the devices are disabled
>> when the system is resumed from a sleep state so are not triggered.
>>
>> Save the interrupt enable set register for each combiner group and
>> restore it after resume to make sure that the interrupts are enabled.
>>
> 
> Not sure if you need this. IMO it's not clean and redundant though I
> admit many drivers do exactly same thing. I am trying to remove or point
> out those redundant code as irqchip core has options/flags to do what
> you need. I assume there are no wakeup sources connected to this

Yes, there are wakeup sources connected to this combiner. As Krzysztof
said some wakeup sources use a GPIO line as IRQ whose interrupt parent
is the combiner. As an example the Power gpio-key and cros_ec keyboard
for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.

Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
right thing though and call {enable,disable}_irq_wake() on the S2R path.

And in fact, the peripherals resume the system after a suspend but after
resume, interrupts are not triggered anymore.

> combiner. Setting irqchip flags should solve this problem. A simple
> patch below should do the job ?
>

I don't see how the below patch is going to work for the case I'm trying to
solve. If I understand correctly, the IRQCHIP_MASK_ON_SUSPEND flag is used
to force masking non wakeup interrupt in the suspend path (instead of just
disabling the interrupts on suspend and not masking at the hardware level)

But my problem is not about interrupts needed to be masked on suspend but
the opposite, that interrupts have to be unmasked on resume since the IP
loses its state so after a resume, interrupts are not triggered anymore.

So I also don't see how implementing irq_set_wake() as you suggested is
going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for this case?

> -->8
> 
> diff --git a/drivers/irqchip/exynos-combiner.c 
> b/drivers/irqchip/exynos-combiner.c
> index 5945223b73fa..c0bcec59f829 100644
> --- a/drivers/irqchip/exynos-combiner.c
> +++ b/drivers/irqchip/exynos-combiner.c
> @@ -111,6 +111,7 @@ static struct irq_chip combiner_chip = {
>   #ifdef CONFIG_SMP
>          .irq_set_affinity       = combiner_set_affinity,
>   #endif
> +       .flags                  = IRQCHIP_MASK_ON_SUSPEND,
>   };
> 
> 

Best regards,
Javier

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 12:27, Javier Martinez Canillas wrote:
> Hello Sudeep,
>
> Thanks a lot for the feedback.
>
> On 06/12/2015 12:10 PM, Sudeep Holla wrote:
>>
>>
>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>> into a low power state during a Suspend-to-RAM. This means that if a
>>> IRQ is used as a source, the interrupts for the devices are disabled
>>> when the system is resumed from a sleep state so are not triggered.
>>>
>>> Save the interrupt enable set register for each combiner group and
>>> restore it after resume to make sure that the interrupts are enabled.
>>>
>>
>> Not sure if you need this. IMO it's not clean and redundant though I
>> admit many drivers do exactly same thing. I am trying to remove or point
>> out those redundant code as irqchip core has options/flags to do what
>> you need. I assume there are no wakeup sources connected to this
>
> Yes, there are wakeup sources connected to this combiner. As Krzysztof
> said some wakeup sources use a GPIO line as IRQ whose interrupt parent
> is the combiner. As an example the Power gpio-key and cros_ec keyboard
> for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.
>
> Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
> right thing though and call {enable,disable}_irq_wake() on the S2R path.
>
> And in fact, the peripherals resume the system after a suspend but after
> resume, interrupts are not triggered anymore.
>

I agree for the wakeup source, but I need to go through the irqchip core
again to understand this better.

>> combiner. Setting irqchip flags should solve this problem. A
>> simple patch below should do the job ?
>>
>
> I don't see how the below patch is going to work for the case I'm
> trying to solve. If I understand correctly, the
> IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup
> interrupt in the suspend path (instead of just disabling the
> interrupts on suspend and not masking at the hardware level)
>

It also takes re-enables all the IRQs in the resume path that were
disabled on the suspend path.

> But my problem is not about interrupts needed to be masked on suspend
> but the opposite, that interrupts have to be unmasked on resume since
> the IP loses its state so after a resume, interrupts are not
> triggered anymore.
>

As I mentioned above this happens for all non-wake up interrupts.
However this needs to done for wake up interrupts IIUC. BTW if these
registers are lost assuming the combiner was powered down, even the
status register will be lost and you will not know exactly the wakeup
reason right ?

> So I also don't see how implementing irq_set_wake() as you suggested
> is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for
> this case?
>

IIRC these combiner feeds to main interrupt controller and you need to
make sure that interrupt is set as wakeup if required. Right ? so you
may still need irq_set_wake IMO.

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 12:27, Javier Martinez Canillas wrote:
> Hello Sudeep,
>
> Thanks a lot for the feedback.
>
> On 06/12/2015 12:10 PM, Sudeep Holla wrote:
>>
>>
>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>> into a low power state during a Suspend-to-RAM. This means that if a
>>> IRQ is used as a source, the interrupts for the devices are disabled
>>> when the system is resumed from a sleep state so are not triggered.
>>>
>>> Save the interrupt enable set register for each combiner group and
>>> restore it after resume to make sure that the interrupts are enabled.
>>>
>>
>> Not sure if you need this. IMO it's not clean and redundant though I
>> admit many drivers do exactly same thing. I am trying to remove or point
>> out those redundant code as irqchip core has options/flags to do what
>> you need. I assume there are no wakeup sources connected to this
>
> Yes, there are wakeup sources connected to this combiner. As Krzysztof
> said some wakeup sources use a GPIO line as IRQ whose interrupt parent
> is the combiner. As an example the Power gpio-key and cros_ec keyboard
> for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.
>
> Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
> right thing though and call {enable,disable}_irq_wake() on the S2R path.
>
> And in fact, the peripherals resume the system after a suspend but after
> resume, interrupts are not triggered anymore.
>

I agree for the wakeup source, but I need to go through the irqchip core
again to understand this better.

>> combiner. Setting irqchip flags should solve this problem. A
>> simple patch below should do the job ?
>>
>
> I don't see how the below patch is going to work for the case I'm
> trying to solve. If I understand correctly, the
> IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup
> interrupt in the suspend path (instead of just disabling the
> interrupts on suspend and not masking at the hardware level)
>

It also takes re-enables all the IRQs in the resume path that were
disabled on the suspend path.

> But my problem is not about interrupts needed to be masked on suspend
> but the opposite, that interrupts have to be unmasked on resume since
> the IP loses its state so after a resume, interrupts are not
> triggered anymore.
>

As I mentioned above this happens for all non-wake up interrupts.
However this needs to done for wake up interrupts IIUC. BTW if these
registers are lost assuming the combiner was powered down, even the
status register will be lost and you will not know exactly the wakeup
reason right ?

> So I also don't see how implementing irq_set_wake() as you suggested
> is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for
> this case?
>

IIRC these combiner feeds to main interrupt controller and you need to
make sure that interrupt is set as wakeup if required. Right ? so you
may still need irq_set_wake IMO.

Regards,
Sudeep

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/12/2015 01:54 PM, Sudeep Holla wrote:
> 
> 
> On 12/06/15 12:27, Javier Martinez Canillas wrote:
>> Hello Sudeep,
>>
>> Thanks a lot for the feedback.
>>
>> On 06/12/2015 12:10 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>>> into a low power state during a Suspend-to-RAM. This means that if a
>>>> IRQ is used as a source, the interrupts for the devices are disabled
>>>> when the system is resumed from a sleep state so are not triggered.
>>>>
>>>> Save the interrupt enable set register for each combiner group and
>>>> restore it after resume to make sure that the interrupts are enabled.
>>>>
>>>
>>> Not sure if you need this. IMO it's not clean and redundant though I
>>> admit many drivers do exactly same thing. I am trying to remove or point
>>> out those redundant code as irqchip core has options/flags to do what
>>> you need. I assume there are no wakeup sources connected to this
>>
>> Yes, there are wakeup sources connected to this combiner. As Krzysztof
>> said some wakeup sources use a GPIO line as IRQ whose interrupt parent
>> is the combiner. As an example the Power gpio-key and cros_ec keyboard
>> for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.
>>
>> Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
>> right thing though and call {enable,disable}_irq_wake() on the S2R path.
>>
>> And in fact, the peripherals resume the system after a suspend but after
>> resume, interrupts are not triggered anymore.
>>
> 
> I agree for the wakeup source, but I need to go through the irqchip core
> again to understand this better.
> 
>>> combiner. Setting irqchip flags should solve this problem. A
>>> simple patch below should do the job ?
>>>
>>
>> I don't see how the below patch is going to work for the case I'm
>> trying to solve. If I understand correctly, the
>> IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup
>> interrupt in the suspend path (instead of just disabling the
>> interrupts on suspend and not masking at the hardware level)
>>
> 
> It also takes re-enables all the IRQs in the resume path that were
> disabled on the suspend path.
>

Ok, I only saw that a call to mask_irq was made in suspend_device_irq [0]
if IRQCHIP_MASK_ON_SUSPEND was set but I didn't see the inverse operation
in the resume path. But now looking at irq_enable() I see that the IRQ is
unmasked if it was disabled so that's why is not needed.
 
>> But my problem is not about interrupts needed to be masked on suspend
>> but the opposite, that interrupts have to be unmasked on resume since
>> the IP loses its state so after a resume, interrupts are not
>> triggered anymore.
>>
> 
> As I mentioned above this happens for all non-wake up interrupts.
> However this needs to done for wake up interrupts IIUC. BTW if these

Well both interrupts that are a wakeup source and those that are not,
don't work after a resume. IOW, all the interrupts whose source is the
combiner are not working.

> registers are lost assuming the combiner was powered down, even the
> status register will be lost and you will not know exactly the wakeup
> reason right ?
>

Good question, I didn't find in the documentation I've access to that
this happen but just found through experimentation that the IRQ enable
set register values are lost after a resume and that saving/restoring
the values makes the interrupts to be triggered again.

>> So I also don't see how implementing irq_set_wake() as you suggested
>> is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for
>> this case?
>>
> 
> IIRC these combiner feeds to main interrupt controller and you need to
> make sure that interrupt is set as wakeup if required. Right ? so you
> may still need irq_set_wake IMO.
>

Yes, I agree that is good to have a irq_set_wake callback, what is still
not clear to me is if is needed to solve this particular issue or not.

> Regards,
> Sudeep
>

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/kernel/irq/pm.c#L90

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/12/2015 01:54 PM, Sudeep Holla wrote:
> 
> 
> On 12/06/15 12:27, Javier Martinez Canillas wrote:
>> Hello Sudeep,
>>
>> Thanks a lot for the feedback.
>>
>> On 06/12/2015 12:10 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 12/06/15 06:43, Javier Martinez Canillas wrote:
>>>> The Exynos interrupt combiner IP loses its state when the SoC enters
>>>> into a low power state during a Suspend-to-RAM. This means that if a
>>>> IRQ is used as a source, the interrupts for the devices are disabled
>>>> when the system is resumed from a sleep state so are not triggered.
>>>>
>>>> Save the interrupt enable set register for each combiner group and
>>>> restore it after resume to make sure that the interrupts are enabled.
>>>>
>>>
>>> Not sure if you need this. IMO it's not clean and redundant though I
>>> admit many drivers do exactly same thing. I am trying to remove or point
>>> out those redundant code as irqchip core has options/flags to do what
>>> you need. I assume there are no wakeup sources connected to this
>>
>> Yes, there are wakeup sources connected to this combiner. As Krzysztof
>> said some wakeup sources use a GPIO line as IRQ whose interrupt parent
>> is the combiner. As an example the Power gpio-key and cros_ec keyboard
>> for both Exynos5250 Snow and Exynos5420 Peach Pit/Pi.
>>
>> Both drivers/input/keyboard/gpio_keysc and drivers/mfd/cros_ec.c do the
>> right thing though and call {enable,disable}_irq_wake() on the S2R path.
>>
>> And in fact, the peripherals resume the system after a suspend but after
>> resume, interrupts are not triggered anymore.
>>
> 
> I agree for the wakeup source, but I need to go through the irqchip core
> again to understand this better.
> 
>>> combiner. Setting irqchip flags should solve this problem. A
>>> simple patch below should do the job ?
>>>
>>
>> I don't see how the below patch is going to work for the case I'm
>> trying to solve. If I understand correctly, the
>> IRQCHIP_MASK_ON_SUSPEND flag is used to force masking non wakeup
>> interrupt in the suspend path (instead of just disabling the
>> interrupts on suspend and not masking at the hardware level)
>>
> 
> It also takes re-enables all the IRQs in the resume path that were
> disabled on the suspend path.
>

Ok, I only saw that a call to mask_irq was made in suspend_device_irq [0]
if IRQCHIP_MASK_ON_SUSPEND was set but I didn't see the inverse operation
in the resume path. But now looking at irq_enable() I see that the IRQ is
unmasked if it was disabled so that's why is not needed.
 
>> But my problem is not about interrupts needed to be masked on suspend
>> but the opposite, that interrupts have to be unmasked on resume since
>> the IP loses its state so after a resume, interrupts are not
>> triggered anymore.
>>
> 
> As I mentioned above this happens for all non-wake up interrupts.
> However this needs to done for wake up interrupts IIUC. BTW if these

Well both interrupts that are a wakeup source and those that are not,
don't work after a resume. IOW, all the interrupts whose source is the
combiner are not working.

> registers are lost assuming the combiner was powered down, even the
> status register will be lost and you will not know exactly the wakeup
> reason right ?
>

Good question, I didn't find in the documentation I've access to that
this happen but just found through experimentation that the IRQ enable
set register values are lost after a resume and that saving/restoring
the values makes the interrupts to be triggered again.

>> So I also don't see how implementing irq_set_wake() as you suggested
>> is going to work. Maybe we need a IRQCHIP_UNMASK_ON_RESUME flag for
>> this case?
>>
> 
> IIRC these combiner feeds to main interrupt controller and you need to
> make sure that interrupt is set as wakeup if required. Right ? so you
> may still need irq_set_wake IMO.
>

Yes, I agree that is good to have a irq_set_wake callback, what is still
not clear to me is if is needed to solve this particular issue or not.

> Regards,
> Sudeep
>

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/kernel/irq/pm.c#L90

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/12/2015 02:57 PM, Javier Martinez Canillas wrote:
> On 06/12/2015 01:54 PM, Sudeep Holla wrote:

[snip]

> 
>> registers are lost assuming the combiner was powered down, even the
>> status register will be lost and you will not know exactly the wakeup
>> reason right ?
>>
> 
> Good question, I didn't find in the documentation I've access to that
> this happen but just found through experimentation that the IRQ enable
> set register values are lost after a resume and that saving/restoring
> the values makes the interrupts to be triggered again.
>

I'll share here too the findings I mentioned over IRC. As you suggested I
add some printouts and noticed that the ISTRn (Interrupt Status) registers
values are indeed preserved on resume so I can know for example if the
wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
values of the registers against the Exynos manual and they corresponds to
the interrupt sources in each case so the values are correct.

So as you said, it seems that is not that the IP block loses its state on
S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
registers.

To reduce the possible s/w components that could be doing this, I booted a
signed FIT image directly using the RO U-Boot instead of chain loading a
mainline nv-uboot. In this configuration I've the same issue so it seems
that if something is zeroing those registers on S2R, this can't be changed
without void the warranty of these machines.

I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
they have a very similar solution than my patch, the IESRn are also saved
but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
or registering a syscore ops but the idea is basically the same.

I have to take a look to the U-boot that is shipped on the device, I think
the correct branch is [1] but I'm not sure if that is the correct one.

Best regards,
Javier

[0]: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/arch/arm/mach-exynos/common.c#657
[1]: https://chromium.googlesource.com/chromiumos/third_party/u-boot firmware-pit-4482.B

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/12/2015 02:57 PM, Javier Martinez Canillas wrote:
> On 06/12/2015 01:54 PM, Sudeep Holla wrote:

[snip]

> 
>> registers are lost assuming the combiner was powered down, even the
>> status register will be lost and you will not know exactly the wakeup
>> reason right ?
>>
> 
> Good question, I didn't find in the documentation I've access to that
> this happen but just found through experimentation that the IRQ enable
> set register values are lost after a resume and that saving/restoring
> the values makes the interrupts to be triggered again.
>

I'll share here too the findings I mentioned over IRC. As you suggested I
add some printouts and noticed that the ISTRn (Interrupt Status) registers
values are indeed preserved on resume so I can know for example if the
wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
values of the registers against the Exynos manual and they corresponds to
the interrupt sources in each case so the values are correct.

So as you said, it seems that is not that the IP block loses its state on
S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
registers.

To reduce the possible s/w components that could be doing this, I booted a
signed FIT image directly using the RO U-Boot instead of chain loading a
mainline nv-uboot. In this configuration I've the same issue so it seems
that if something is zeroing those registers on S2R, this can't be changed
without void the warranty of these machines.

I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
they have a very similar solution than my patch, the IESRn are also saved
but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
or registering a syscore ops but the idea is basically the same.

I have to take a look to the U-boot that is shipped on the device, I think
the correct branch is [1] but I'm not sure if that is the correct one.

Best regards,
Javier

[0]: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/arch/arm/mach-exynos/common.c#657
[1]: https://chromium.googlesource.com/chromiumos/third_party/u-boot firmware-pit-4482.B

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Doug Anderson]

Hi,

On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>>> registers are lost assuming the combiner was powered down, even the
>>> status register will be lost and you will not know exactly the wakeup
>>> reason right ?
>>>
>>
>> Good question, I didn't find in the documentation I've access to that
>> this happen but just found through experimentation that the IRQ enable
>> set register values are lost after a resume and that saving/restoring
>> the values makes the interrupts to be triggered again.
>>
>
> I'll share here too the findings I mentioned over IRC. As you suggested I
> add some printouts and noticed that the ISTRn (Interrupt Status) registers
> values are indeed preserved on resume so I can know for example if the
> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
> values of the registers against the Exynos manual and they corresponds to
> the interrupt sources in each case so the values are correct.
>
> So as you said, it seems that is not that the IP block loses its state on
> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
> registers.

I'll postulate an alternate theory here.  You can tell me if you buy
it or if you think I've been out in the sun too long.

Let's say that the interrupt combiner's status registers show the raw
status as asserted by whatever is hooked up to the combiner.  This
means that even if the combiner got reset we could still read the
status register and get the status of the source.  Imagine that the
combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
all kinds of config (input vs. output, edge interrupt status, maybe
pulls, etc).  ...but you can still read the state asserted by an
external source, right?

In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
pretty sure that the controller managine 'EINT 11' _doesn't_ lose
power across suspend/resume...


I'll further bolster my theory by saying that _almost nothing_ in the
exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
might be nice, but you're out of luck.  ...so it would make me
terribly surprised to see the combiner keep power.


> To reduce the possible s/w components that could be doing this, I booted a
> signed FIT image directly using the RO U-Boot instead of chain loading a
> mainline nv-uboot. In this configuration I've the same issue so it seems
> that if something is zeroing those registers on S2R, this can't be changed
> without void the warranty of these machines.
>
> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
> they have a very similar solution than my patch, the IESRn are also saved
> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
> or registering a syscore ops but the idea is basically the same.

Yup, you can see where kliegs added it in
<https://chromium-review.googlesource.com/#/c/27964/>.  As per the
comments in that CL, this was probably broken in:

063bd6f ARM: EXYNOS: Remove GIC save & restore function


> I have to take a look to the U-boot that is shipped on the device, I think
> the correct branch is [1] but I'm not sure if that is the correct one.

It is the right one.  If U-Boot were touching this (which would
greatly surprise me) it should be here:

arch/arm/include/asm/arch-exynos/cpu.h

...and it's not.  Doing a grep for '10440000' (the combiner base
address) doesn't find anything in U-Boot either, which makes it less
likely.  ...and it's even less likely since the amount of code that is
in U-Boot that runs at resume time is a very small subset and I'm
fairly familiar with it and I would have remembered it touching the
combiner.

It's POSSIBLE that the internal ROM in exynos is clobbering this, but
as per above it seems crazy unlikely and I think it's just losing
power.


-Doug

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Doug Anderson]

Hi,

On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>>> registers are lost assuming the combiner was powered down, even the
>>> status register will be lost and you will not know exactly the wakeup
>>> reason right ?
>>>
>>
>> Good question, I didn't find in the documentation I've access to that
>> this happen but just found through experimentation that the IRQ enable
>> set register values are lost after a resume and that saving/restoring
>> the values makes the interrupts to be triggered again.
>>
>
> I'll share here too the findings I mentioned over IRC. As you suggested I
> add some printouts and noticed that the ISTRn (Interrupt Status) registers
> values are indeed preserved on resume so I can know for example if the
> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
> values of the registers against the Exynos manual and they corresponds to
> the interrupt sources in each case so the values are correct.
>
> So as you said, it seems that is not that the IP block loses its state on
> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
> registers.

I'll postulate an alternate theory here.  You can tell me if you buy
it or if you think I've been out in the sun too long.

Let's say that the interrupt combiner's status registers show the raw
status as asserted by whatever is hooked up to the combiner.  This
means that even if the combiner got reset we could still read the
status register and get the status of the source.  Imagine that the
combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
all kinds of config (input vs. output, edge interrupt status, maybe
pulls, etc).  ...but you can still read the state asserted by an
external source, right?

In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
pretty sure that the controller managine 'EINT 11' _doesn't_ lose
power across suspend/resume...


I'll further bolster my theory by saying that _almost nothing_ in the
exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
might be nice, but you're out of luck.  ...so it would make me
terribly surprised to see the combiner keep power.


> To reduce the possible s/w components that could be doing this, I booted a
> signed FIT image directly using the RO U-Boot instead of chain loading a
> mainline nv-uboot. In this configuration I've the same issue so it seems
> that if something is zeroing those registers on S2R, this can't be changed
> without void the warranty of these machines.
>
> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
> they have a very similar solution than my patch, the IESRn are also saved
> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
> or registering a syscore ops but the idea is basically the same.

Yup, you can see where kliegs added it in
<https://chromium-review.googlesource.com/#/c/27964/>.  As per the
comments in that CL, this was probably broken in:

063bd6f ARM: EXYNOS: Remove GIC save & restore function


> I have to take a look to the U-boot that is shipped on the device, I think
> the correct branch is [1] but I'm not sure if that is the correct one.

It is the right one.  If U-Boot were touching this (which would
greatly surprise me) it should be here:

arch/arm/include/asm/arch-exynos/cpu.h

...and it's not.  Doing a grep for '10440000' (the combiner base
address) doesn't find anything in U-Boot either, which makes it less
likely.  ...and it's even less likely since the amount of code that is
in U-Boot that runs at resume time is a very small subset and I'm
fairly familiar with it and I would have remembered it touching the
combiner.

It's POSSIBLE that the internal ROM in exynos is clobbering this, but
as per above it seems crazy unlikely and I think it's just losing
power.


-Doug

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Doug,

Thanks a lot for your comments.

On 06/12/2015 10:17 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>>>> registers are lost assuming the combiner was powered down, even the
>>>> status register will be lost and you will not know exactly the wakeup
>>>> reason right ?
>>>>
>>>
>>> Good question, I didn't find in the documentation I've access to that
>>> this happen but just found through experimentation that the IRQ enable
>>> set register values are lost after a resume and that saving/restoring
>>> the values makes the interrupts to be triggered again.
>>>
>>
>> I'll share here too the findings I mentioned over IRC. As you suggested I
>> add some printouts and noticed that the ISTRn (Interrupt Status) registers
>> values are indeed preserved on resume so I can know for example if the
>> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
>> values of the registers against the Exynos manual and they corresponds to
>> the interrupt sources in each case so the values are correct.
>>
>> So as you said, it seems that is not that the IP block loses its state on
>> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
>> registers.
> 
> I'll postulate an alternate theory here.  You can tell me if you buy
> it or if you think I've been out in the sun too long.
> 
> Let's say that the interrupt combiner's status registers show the raw
> status as asserted by whatever is hooked up to the combiner.  This
> means that even if the combiner got reset we could still read the
> status register and get the status of the source.  Imagine that the
> combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
> all kinds of config (input vs. output, edge interrupt status, maybe
> pulls, etc).  ...but you can still read the state asserted by an
> external source, right?
> 

That definitely makes a lot of sense to me.

> In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
> pretty sure that the controller managine 'EINT 11' _doesn't_ lose
> power across suspend/resume...
>

Yes, the External Interrupt sources come from the GPIO controller and the
driver has a similar logic than $subject to save the registers state.
 
> 
> I'll further bolster my theory by saying that _almost nothing_ in the
> exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
> controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
> might be nice, but you're out of luck.  ...so it would make me
> terribly surprised to see the combiner keep power.
>

It surprised me as well but I didn't think about what you said that the
combiner interrupt source was restored so the status register could just
be showing the combiner's input raw status even after the chip was reset.

> 
>> To reduce the possible s/w components that could be doing this, I booted a
>> signed FIT image directly using the RO U-Boot instead of chain loading a
>> mainline nv-uboot. In this configuration I've the same issue so it seems
>> that if something is zeroing those registers on S2R, this can't be changed
>> without void the warranty of these machines.
>>
>> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
>> they have a very similar solution than my patch, the IESRn are also saved
>> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
>> or registering a syscore ops but the idea is basically the same.
> 
> Yup, you can see where kliegs added it in
> <https://chromium-review.googlesource.com/#/c/27964/>.  As per the
> comments in that CL, this was probably broken in:
> 
> 063bd6f ARM: EXYNOS: Remove GIC save & restore function
> 
>

Nod, thanks for the pointer.

>> I have to take a look to the U-boot that is shipped on the device, I think
>> the correct branch is [1] but I'm not sure if that is the correct one.
> 
> It is the right one.  If U-Boot were touching this (which would
> greatly surprise me) it should be here:
> 
> arch/arm/include/asm/arch-exynos/cpu.h
> 
> ...and it's not.  Doing a grep for '10440000' (the combiner base
> address) doesn't find anything in U-Boot either, which makes it less
> likely.  ...and it's even less likely since the amount of code that is
> in U-Boot that runs at resume time is a very small subset and I'm
> fairly familiar with it and I would have remembered it touching the
> combiner.
>

That explains why I was also not able to find were U-Boot was overwriting
those registers.

> It's POSSIBLE that the internal ROM in exynos is clobbering this, but
> as per above it seems crazy unlikely and I think it's just losing
> power.
>

Agreed.

> 
> -Doug
>

Sudeep, so we may need something like $subject after all from Doug's
explanations since the combiner chip state is lost during a S2R. I know
that it adds more duplicated code (others irqchip drivers do the same)
and it may not scale well if a chip has many registers but is the best
solution I could came with.

If you have a suggestion for a better alternative, I can give a try and
write the patch. But I think $subject could also land to fix this issue
since is a very non intrusive change and later can be changed once the
irqchip core supports this use case.

Best regards,
Javier

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Doug,

Thanks a lot for your comments.

On 06/12/2015 10:17 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>>>> registers are lost assuming the combiner was powered down, even the
>>>> status register will be lost and you will not know exactly the wakeup
>>>> reason right ?
>>>>
>>>
>>> Good question, I didn't find in the documentation I've access to that
>>> this happen but just found through experimentation that the IRQ enable
>>> set register values are lost after a resume and that saving/restoring
>>> the values makes the interrupts to be triggered again.
>>>
>>
>> I'll share here too the findings I mentioned over IRC. As you suggested I
>> add some printouts and noticed that the ISTRn (Interrupt Status) registers
>> values are indeed preserved on resume so I can know for example if the
>> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
>> values of the registers against the Exynos manual and they corresponds to
>> the interrupt sources in each case so the values are correct.
>>
>> So as you said, it seems that is not that the IP block loses its state on
>> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
>> registers.
> 
> I'll postulate an alternate theory here.  You can tell me if you buy
> it or if you think I've been out in the sun too long.
> 
> Let's say that the interrupt combiner's status registers show the raw
> status as asserted by whatever is hooked up to the combiner.  This
> means that even if the combiner got reset we could still read the
> status register and get the status of the source.  Imagine that the
> combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
> all kinds of config (input vs. output, edge interrupt status, maybe
> pulls, etc).  ...but you can still read the state asserted by an
> external source, right?
> 

That definitely makes a lot of sense to me.

> In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
> pretty sure that the controller managine 'EINT 11' _doesn't_ lose
> power across suspend/resume...
>

Yes, the External Interrupt sources come from the GPIO controller and the
driver has a similar logic than $subject to save the registers state.
 
> 
> I'll further bolster my theory by saying that _almost nothing_ in the
> exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
> controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
> might be nice, but you're out of luck.  ...so it would make me
> terribly surprised to see the combiner keep power.
>

It surprised me as well but I didn't think about what you said that the
combiner interrupt source was restored so the status register could just
be showing the combiner's input raw status even after the chip was reset.

> 
>> To reduce the possible s/w components that could be doing this, I booted a
>> signed FIT image directly using the RO U-Boot instead of chain loading a
>> mainline nv-uboot. In this configuration I've the same issue so it seems
>> that if something is zeroing those registers on S2R, this can't be changed
>> without void the warranty of these machines.
>>
>> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
>> they have a very similar solution than my patch, the IESRn are also saved
>> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
>> or registering a syscore ops but the idea is basically the same.
> 
> Yup, you can see where kliegs added it in
> <https://chromium-review.googlesource.com/#/c/27964/>.  As per the
> comments in that CL, this was probably broken in:
> 
> 063bd6f ARM: EXYNOS: Remove GIC save & restore function
> 
>

Nod, thanks for the pointer.

>> I have to take a look to the U-boot that is shipped on the device, I think
>> the correct branch is [1] but I'm not sure if that is the correct one.
> 
> It is the right one.  If U-Boot were touching this (which would
> greatly surprise me) it should be here:
> 
> arch/arm/include/asm/arch-exynos/cpu.h
> 
> ...and it's not.  Doing a grep for '10440000' (the combiner base
> address) doesn't find anything in U-Boot either, which makes it less
> likely.  ...and it's even less likely since the amount of code that is
> in U-Boot that runs at resume time is a very small subset and I'm
> fairly familiar with it and I would have remembered it touching the
> combiner.
>

That explains why I was also not able to find were U-Boot was overwriting
those registers.

> It's POSSIBLE that the internal ROM in exynos is clobbering this, but
> as per above it seems crazy unlikely and I think it's just losing
> power.
>

Agreed.

> 
> -Doug
>

Sudeep, so we may need something like $subject after all from Doug's
explanations since the combiner chip state is lost during a S2R. I know
that it adds more duplicated code (others irqchip drivers do the same)
and it may not scale well if a chip has many registers but is the best
solution I could came with.

If you have a suggestion for a better alternative, I can give a try and
write the patch. But I think $subject could also land to fix this issue
since is a very non intrusive change and later can be changed once the
irqchip core supports this use case.

Best regards,
Javier

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 15/06/15 08:46, Javier Martinez Canillas wrote:
[...]

>
> Sudeep, so we may need something like $subject after all from Doug's
> explanations since the combiner chip state is lost during a S2R. I know
> that it adds more duplicated code (others irqchip drivers do the same)
> and it may not scale well if a chip has many registers but is the best
> solution I could came with.
>

OK

> If you have a suggestion for a better alternative, I can give a try and
> write the patch. But I think $subject could also land to fix this issue
> since is a very non intrusive change and later can be changed once the
> irqchip core supports this use case.
>

Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
also and then you can restore iff it's non-zero as irq core will take
care of most of the non-wakeup sources. Because I am planning to push
MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
combiner interrupts are always on in GIC. Implement set_irq_wake as
enable_irq_wake (comb_irq_to_GIC).

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 15/06/15 08:46, Javier Martinez Canillas wrote:
[...]

>
> Sudeep, so we may need something like $subject after all from Doug's
> explanations since the combiner chip state is lost during a S2R. I know
> that it adds more duplicated code (others irqchip drivers do the same)
> and it may not scale well if a chip has many registers but is the best
> solution I could came with.
>

OK

> If you have a suggestion for a better alternative, I can give a try and
> write the patch. But I think $subject could also land to fix this issue
> since is a very non intrusive change and later can be changed once the
> irqchip core supports this use case.
>

Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
also and then you can restore iff it's non-zero as irq core will take
care of most of the non-wakeup sources. Because I am planning to push
MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
combiner interrupts are always on in GIC. Implement set_irq_wake as
enable_irq_wake (comb_irq_to_GIC).

Regards,
Sudeep

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/15/2015 11:01 AM, Sudeep Holla wrote:
> 
> 
> On 15/06/15 08:46, Javier Martinez Canillas wrote:
> [...]
> 
>>
>> Sudeep, so we may need something like $subject after all from Doug's
>> explanations since the combiner chip state is lost during a S2R. I know
>> that it adds more duplicated code (others irqchip drivers do the same)
>> and it may not scale well if a chip has many registers but is the best
>> solution I could came with.
>>
> 
> OK
> 
>> If you have a suggestion for a better alternative, I can give a try and
>> write the patch. But I think $subject could also land to fix this issue
>> since is a very non intrusive change and later can be changed once the
>> irqchip core supports this use case.
>>
> 
> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
> also and then you can restore iff it's non-zero as irq core will take
> care of most of the non-wakeup sources. Because I am planning to push

I've looking at this and a problem I found is that IIUC the set_irq_wake
is not propagated from the the Exynos pinctrl / GPIO driver which is the
combiner's external interrupt source so the callback is never called.
Which means that right now only the state of the wakeup source IRQs can't
be saved since that information is not present.

The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
the combiner interrupts but its .irq_set_wake handler only updates the
wakeup source mask for the external interrupts but does not call the
combiner .set_irq_wake so that should be changed as well.

But even for non-wakeup interrupts, I found that they are not enabled when
adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
to the pinctrl driver missing something else though.

> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
> combiner interrupts are always on in GIC. Implement set_irq_wake as
> enable_irq_wake (comb_irq_to_GIC).
>

Right, but can we do this as a follow-up? S2R is currently broken on these
machines and $subject is I think a reasonable small fix that won't introduce
any regressions.

To do a more intrusive change, I should better understand the interactions
between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
meantime S2R will continue to be broken on these platforms unless someone
more familiar with all this could point me in the right direction.

> Regards,
> Sudeep
> 

Best regards,
Javier

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/15/2015 11:01 AM, Sudeep Holla wrote:
> 
> 
> On 15/06/15 08:46, Javier Martinez Canillas wrote:
> [...]
> 
>>
>> Sudeep, so we may need something like $subject after all from Doug's
>> explanations since the combiner chip state is lost during a S2R. I know
>> that it adds more duplicated code (others irqchip drivers do the same)
>> and it may not scale well if a chip has many registers but is the best
>> solution I could came with.
>>
> 
> OK
> 
>> If you have a suggestion for a better alternative, I can give a try and
>> write the patch. But I think $subject could also land to fix this issue
>> since is a very non intrusive change and later can be changed once the
>> irqchip core supports this use case.
>>
> 
> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
> also and then you can restore iff it's non-zero as irq core will take
> care of most of the non-wakeup sources. Because I am planning to push

I've looking at this and a problem I found is that IIUC the set_irq_wake
is not propagated from the the Exynos pinctrl / GPIO driver which is the
combiner's external interrupt source so the callback is never called.
Which means that right now only the state of the wakeup source IRQs can't
be saved since that information is not present.

The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
the combiner interrupts but its .irq_set_wake handler only updates the
wakeup source mask for the external interrupts but does not call the
combiner .set_irq_wake so that should be changed as well.

But even for non-wakeup interrupts, I found that they are not enabled when
adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
to the pinctrl driver missing something else though.

> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
> combiner interrupts are always on in GIC. Implement set_irq_wake as
> enable_irq_wake (comb_irq_to_GIC).
>

Right, but can we do this as a follow-up? S2R is currently broken on these
machines and $subject is I think a reasonable small fix that won't introduce
any regressions.

To do a more intrusive change, I should better understand the interactions
between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
meantime S2R will continue to be broken on these platforms unless someone
more familiar with all this could point me in the right direction.

> Regards,
> Sudeep
> 

Best regards,
Javier

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 15/06/15 16:00, Javier Martinez Canillas wrote:
> Hello Sudeep,
>
> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>
>>
>> On 15/06/15 08:46, Javier Martinez Canillas wrote:
>> [...]
>>
>>>
>>> Sudeep, so we may need something like $subject after all from Doug's
>>> explanations since the combiner chip state is lost during a S2R. I know
>>> that it adds more duplicated code (others irqchip drivers do the same)
>>> and it may not scale well if a chip has many registers but is the best
>>> solution I could came with.
>>>
>>
>> OK
>>
>>> If you have a suggestion for a better alternative, I can give a try and
>>> write the patch. But I think $subject could also land to fix this issue
>>> since is a very non intrusive change and later can be changed once the
>>> irqchip core supports this use case.
>>>
>>
>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>> also and then you can restore iff it's non-zero as irq core will take
>> care of most of the non-wakeup sources. Because I am planning to push
>
> I've looking at this and a problem I found is that IIUC the set_irq_wake
> is not propagated from the the Exynos pinctrl / GPIO driver which is the
> combiner's external interrupt source so the callback is never called.
> Which means that right now only the state of the wakeup source IRQs can't
> be saved since that information is not present.
>
> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
> the combiner interrupts but its .irq_set_wake handler only updates the
> wakeup source mask for the external interrupts but does not call the
> combiner .set_irq_wake so that should be changed as well.
>

Thanks for the looking at this.

> But even for non-wakeup interrupts, I found that they are not enabled when
> adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
> to the pinctrl driver missing something else though.
>

Even GIC is not masking any interrupt on suspend and if other irqchip or
drivers are assuming that, it will work fine for now. But once I
introduce that change in GIC it will break.

>> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
>> combiner interrupts are always on in GIC. Implement set_irq_wake as
>> enable_irq_wake (comb_irq_to_GIC).
>>
>
> Right, but can we do this as a follow-up? S2R is currently broken on these
> machines and $subject is I think a reasonable small fix that won't introduce
> any regressions.
>

No issues, I just wanted to understand the issue better and also make
sure we understand that things might break in future once that GIC
change is introduced. So we already know the reason or atleast have some
basic understanding as why would it break if it breaks :)

> To do a more intrusive change, I should better understand the interactions
> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
> meantime S2R will continue to be broken on these platforms unless someone
> more familiar with all this could point me in the right direction.
>

As I said I am fine with this patch for now and I don't want to block it.

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 15/06/15 16:00, Javier Martinez Canillas wrote:
> Hello Sudeep,
>
> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>
>>
>> On 15/06/15 08:46, Javier Martinez Canillas wrote:
>> [...]
>>
>>>
>>> Sudeep, so we may need something like $subject after all from Doug's
>>> explanations since the combiner chip state is lost during a S2R. I know
>>> that it adds more duplicated code (others irqchip drivers do the same)
>>> and it may not scale well if a chip has many registers but is the best
>>> solution I could came with.
>>>
>>
>> OK
>>
>>> If you have a suggestion for a better alternative, I can give a try and
>>> write the patch. But I think $subject could also land to fix this issue
>>> since is a very non intrusive change and later can be changed once the
>>> irqchip core supports this use case.
>>>
>>
>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>> also and then you can restore iff it's non-zero as irq core will take
>> care of most of the non-wakeup sources. Because I am planning to push
>
> I've looking at this and a problem I found is that IIUC the set_irq_wake
> is not propagated from the the Exynos pinctrl / GPIO driver which is the
> combiner's external interrupt source so the callback is never called.
> Which means that right now only the state of the wakeup source IRQs can't
> be saved since that information is not present.
>
> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
> the combiner interrupts but its .irq_set_wake handler only updates the
> wakeup source mask for the external interrupts but does not call the
> combiner .set_irq_wake so that should be changed as well.
>

Thanks for the looking at this.

> But even for non-wakeup interrupts, I found that they are not enabled when
> adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
> to the pinctrl driver missing something else though.
>

Even GIC is not masking any interrupt on suspend and if other irqchip or
drivers are assuming that, it will work fine for now. But once I
introduce that change in GIC it will break.

>> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
>> combiner interrupts are always on in GIC. Implement set_irq_wake as
>> enable_irq_wake (comb_irq_to_GIC).
>>
>
> Right, but can we do this as a follow-up? S2R is currently broken on these
> machines and $subject is I think a reasonable small fix that won't introduce
> any regressions.
>

No issues, I just wanted to understand the issue better and also make
sure we understand that things might break in future once that GIC
change is introduced. So we already know the reason or atleast have some
basic understanding as why would it break if it breaks :)

> To do a more intrusive change, I should better understand the interactions
> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
> meantime S2R will continue to be broken on these platforms unless someone
> more familiar with all this could point me in the right direction.
>

As I said I am fine with this patch for now and I don't want to block it.

Regards,
Sudeep

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/15/2015 05:08 PM, Sudeep Holla wrote:
> On 15/06/15 16:00, Javier Martinez Canillas wrote:
>> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>> On 15/06/15 08:46, Javier Martinez Canillas wrote:

[...]

>>>
>>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>>> also and then you can restore iff it's non-zero as irq core will take
>>> care of most of the non-wakeup sources. Because I am planning to push
>>
>> I've looking at this and a problem I found is that IIUC the set_irq_wake
>> is not propagated from the the Exynos pinctrl / GPIO driver which is the
>> combiner's external interrupt source so the callback is never called.
>> Which means that right now only the state of the wakeup source IRQs can't
>> be saved since that information is not present.
>>
>> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
>> the combiner interrupts but its .irq_set_wake handler only updates the
>> wakeup source mask for the external interrupts but does not call the
>> combiner .set_irq_wake so that should be changed as well.
>>
> 
> Thanks for the looking at this.
>

You are welcome and thanks to you for pointing this out.
 
>> But even for non-wakeup interrupts, I found that they are not enabled when
>> adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
>> to the pinctrl driver missing something else though.
>>
> 
> Even GIC is not masking any interrupt on suspend and if other irqchip or
> drivers are assuming that, it will work fine for now. But once I
> introduce that change in GIC it will break.
> 
>>> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
>>> combiner interrupts are always on in GIC. Implement set_irq_wake as
>>> enable_irq_wake (comb_irq_to_GIC).
>>>
>>
>> Right, but can we do this as a follow-up? S2R is currently broken on these
>> machines and $subject is I think a reasonable small fix that won't introduce
>> any regressions.
>>
> 
> No issues, I just wanted to understand the issue better and also make
> sure we understand that things might break in future once that GIC
> change is introduced. So we already know the reason or atleast have some
> basic understanding as why would it break if it breaks :)
>

Indeed, in the meantime I will continue looking at this to better understand
all the interactions so if something breaks with your changes I may be able
to test / debug and hopefully fix it :)
 
>> To do a more intrusive change, I should better understand the interactions
>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
>> meantime S2R will continue to be broken on these platforms unless someone
>> more familiar with all this could point me in the right direction.
>>
> 
> As I said I am fine with this patch for now and I don't want to block it.
>

Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
with the patch so hopefully this can land sooner rather than later.

> Regards,
> Sudeep
> 

Best regards,
Javier

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Sudeep,

On 06/15/2015 05:08 PM, Sudeep Holla wrote:
> On 15/06/15 16:00, Javier Martinez Canillas wrote:
>> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>> On 15/06/15 08:46, Javier Martinez Canillas wrote:

[...]

>>>
>>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>>> also and then you can restore iff it's non-zero as irq core will take
>>> care of most of the non-wakeup sources. Because I am planning to push
>>
>> I've looking at this and a problem I found is that IIUC the set_irq_wake
>> is not propagated from the the Exynos pinctrl / GPIO driver which is the
>> combiner's external interrupt source so the callback is never called.
>> Which means that right now only the state of the wakeup source IRQs can't
>> be saved since that information is not present.
>>
>> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
>> the combiner interrupts but its .irq_set_wake handler only updates the
>> wakeup source mask for the external interrupts but does not call the
>> combiner .set_irq_wake so that should be changed as well.
>>
> 
> Thanks for the looking at this.
>

You are welcome and thanks to you for pointing this out.
 
>> But even for non-wakeup interrupts, I found that they are not enabled when
>> adding the MASK_ON_SUSPEND on my tests. I don't know if that is related
>> to the pinctrl driver missing something else though.
>>
> 
> Even GIC is not masking any interrupt on suspend and if other irqchip or
> drivers are assuming that, it will work fine for now. But once I
> introduce that change in GIC it will break.
> 
>>> MASK_ON_SUSPEND to GIC and it will break this combiner if it assumes the
>>> combiner interrupts are always on in GIC. Implement set_irq_wake as
>>> enable_irq_wake (comb_irq_to_GIC).
>>>
>>
>> Right, but can we do this as a follow-up? S2R is currently broken on these
>> machines and $subject is I think a reasonable small fix that won't introduce
>> any regressions.
>>
> 
> No issues, I just wanted to understand the issue better and also make
> sure we understand that things might break in future once that GIC
> change is introduced. So we already know the reason or atleast have some
> basic understanding as why would it break if it breaks :)
>

Indeed, in the meantime I will continue looking at this to better understand
all the interactions so if something breaks with your changes I may be able
to test / debug and hopefully fix it :)
 
>> To do a more intrusive change, I should better understand the interactions
>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
>> meantime S2R will continue to be broken on these platforms unless someone
>> more familiar with all this could point me in the right direction.
>>
> 
> As I said I am fine with this patch for now and I don't want to block it.
>

Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
with the patch so hopefully this can land sooner rather than later.

> Regards,
> Sudeep
> 

Best regards,
Javier

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 16.06.2015 00:23, Javier Martinez Canillas wrote:
(...)
>>> To do a more intrusive change, I should better understand the interactions
>>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
>>> meantime S2R will continue to be broken on these platforms unless someone
>>> more familiar with all this could point me in the right direction.
>>>
>>
>> As I said I am fine with this patch for now and I don't want to block it.
>>
> 
> Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
> with the patch so hopefully this can land sooner rather than later.

I assume this will go through irqchip tree, right?

Best regards,
Krzysztof

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Krzysztof Kozlowski]

On 16.06.2015 00:23, Javier Martinez Canillas wrote:
(...)
>>> To do a more intrusive change, I should better understand the interactions
>>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
>>> meantime S2R will continue to be broken on these platforms unless someone
>>> more familiar with all this could point me in the right direction.
>>>
>>
>> As I said I am fine with this patch for now and I don't want to block it.
>>
> 
> Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
> with the patch so hopefully this can land sooner rather than later.

I assume this will go through irqchip tree, right?

Best regards,
Krzysztof

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Krzysztof,

On 06/16/2015 01:57 AM, Krzysztof Kozlowski wrote:
> On 16.06.2015 00:23, Javier Martinez Canillas wrote: (...)
>>>> To do a more intrusive change, I should better understand the
>>>> interactions between the Exynos pinctrl / GPIO, interrupt
>>>> combiner and the GIC and in the meantime S2R will continue to
>>>> be broken on these platforms unless someone more familiar with
>>>> all this could point me in the right direction.
>>>> 
>>> 
>>> As I said I am fine with this patch for now and I don't want to
>>> block it.
>>> 
>> 
>> Thanks a lot, Krzysztof who is one of the Exynos maintainers has
>> also agreed with the patch so hopefully this can land sooner rather
>> than later.
> 
> I assume this will go through irqchip tree, right?
> 

That is my assumption as well. I meant the fact that you as an Exynos
maintainer thinks $subject is a sensible solution to the issue, would
give confidence to the irqchip maintainers to pick this patch soon.

> Best regards, Krzysztof
> 

Best regards,
Javier

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Javier Martinez Canillas]

Hello Krzysztof,

On 06/16/2015 01:57 AM, Krzysztof Kozlowski wrote:
> On 16.06.2015 00:23, Javier Martinez Canillas wrote: (...)
>>>> To do a more intrusive change, I should better understand the
>>>> interactions between the Exynos pinctrl / GPIO, interrupt
>>>> combiner and the GIC and in the meantime S2R will continue to
>>>> be broken on these platforms unless someone more familiar with
>>>> all this could point me in the right direction.
>>>> 
>>> 
>>> As I said I am fine with this patch for now and I don't want to
>>> block it.
>>> 
>> 
>> Thanks a lot, Krzysztof who is one of the Exynos maintainers has
>> also agreed with the patch so hopefully this can land sooner rather
>> than later.
> 
> I assume this will go through irqchip tree, right?
> 

That is my assumption as well. I meant the fact that you as an Exynos
maintainer thinks $subject is a sensible solution to the issue, would
give confidence to the irqchip maintainers to pick this patch soon.

> Best regards, Krzysztof
> 

Best regards,
Javier

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Thomas Gleixner]

On Tue, 16 Jun 2015, Krzysztof Kozlowski wrote:

> On 16.06.2015 00:23, Javier Martinez Canillas wrote:
> (...)
> >>> To do a more intrusive change, I should better understand the interactions
> >>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
> >>> meantime S2R will continue to be broken on these platforms unless someone
> >>> more familiar with all this could point me in the right direction.
> >>>
> >>
> >> As I said I am fine with this patch for now and I don't want to block it.
> >>
> > 
> > Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
> > with the patch so hopefully this can land sooner rather than later.
> 
> I assume this will go through irqchip tree, right?

yes, picking it up now

Thanks,

	tglx

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Thomas Gleixner]

On Tue, 16 Jun 2015, Krzysztof Kozlowski wrote:

> On 16.06.2015 00:23, Javier Martinez Canillas wrote:
> (...)
> >>> To do a more intrusive change, I should better understand the interactions
> >>> between the Exynos pinctrl / GPIO, interrupt combiner and the GIC and in the
> >>> meantime S2R will continue to be broken on these platforms unless someone
> >>> more familiar with all this could point me in the right direction.
> >>>
> >>
> >> As I said I am fine with this patch for now and I don't want to block it.
> >>
> > 
> > Thanks a lot, Krzysztof who is one of the Exynos maintainers has also agreed
> > with the patch so hopefully this can land sooner rather than later.
> 
> I assume this will go through irqchip tree, right?

yes, picking it up now

Thanks,

	tglx

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Tomasz Figa]

2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas
<javier.martinez@collabora.co.uk>:
> Hello Sudeep,
>
> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>
>>
>> On 15/06/15 08:46, Javier Martinez Canillas wrote:
>> [...]
>>
>>>
>>> Sudeep, so we may need something like $subject after all from Doug's
>>> explanations since the combiner chip state is lost during a S2R. I know
>>> that it adds more duplicated code (others irqchip drivers do the same)
>>> and it may not scale well if a chip has many registers but is the best
>>> solution I could came with.
>>>
>>
>> OK
>>
>>> If you have a suggestion for a better alternative, I can give a try and
>>> write the patch. But I think $subject could also land to fix this issue
>>> since is a very non intrusive change and later can be changed once the
>>> irqchip core supports this use case.
>>>
>>
>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>> also and then you can restore iff it's non-zero as irq core will take
>> care of most of the non-wakeup sources. Because I am planning to push
>
> I've looking at this and a problem I found is that IIUC the set_irq_wake
> is not propagated from the the Exynos pinctrl / GPIO driver which is the
> combiner's external interrupt source so the callback is never called.
> Which means that right now only the state of the wakeup source IRQs can't
> be saved since that information is not present.
>
> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
> the combiner interrupts but its .irq_set_wake handler only updates the
> wakeup source mask for the external interrupts but does not call the
> combiner .set_irq_wake so that should be changed as well.
>

As far as I'm aware of, wake-up events from pin controllers don't go
through GIC, but rather directly to PMU, which is a dedicated unit
responsible for power management and not a standalone interrupt
controller (well actually I saw a series making it a cascaded
controller some time ago, but I'm not sure if that went in). Based on
this, I don't think we have to call set_irq_wake on GIC. Correct me if
I'm wrong, though.

Best regards,
Tomasz

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Tomasz Figa]

2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas
<javier.martinez@collabora.co.uk>:
> Hello Sudeep,
>
> On 06/15/2015 11:01 AM, Sudeep Holla wrote:
>>
>>
>> On 15/06/15 08:46, Javier Martinez Canillas wrote:
>> [...]
>>
>>>
>>> Sudeep, so we may need something like $subject after all from Doug's
>>> explanations since the combiner chip state is lost during a S2R. I know
>>> that it adds more duplicated code (others irqchip drivers do the same)
>>> and it may not scale well if a chip has many registers but is the best
>>> solution I could came with.
>>>
>>
>> OK
>>
>>> If you have a suggestion for a better alternative, I can give a try and
>>> write the patch. But I think $subject could also land to fix this issue
>>> since is a very non intrusive change and later can be changed once the
>>> irqchip core supports this use case.
>>>
>>
>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and set_irq_wake
>> also and then you can restore iff it's non-zero as irq core will take
>> care of most of the non-wakeup sources. Because I am planning to push
>
> I've looking at this and a problem I found is that IIUC the set_irq_wake
> is not propagated from the the Exynos pinctrl / GPIO driver which is the
> combiner's external interrupt source so the callback is never called.
> Which means that right now only the state of the wakeup source IRQs can't
> be saved since that information is not present.
>
> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and disables
> the combiner interrupts but its .irq_set_wake handler only updates the
> wakeup source mask for the external interrupts but does not call the
> combiner .set_irq_wake so that should be changed as well.
>

As far as I'm aware of, wake-up events from pin controllers don't go
through GIC, but rather directly to PMU, which is a dedicated unit
responsible for power management and not a standalone interrupt
controller (well actually I saw a series making it a cascaded
controller some time ago, but I'm not sure if that went in). Based on
this, I don't think we have to call set_irq_wake on GIC. Correct me if
I'm wrong, though.

Best regards,
Tomasz

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 16/06/15 13:32, Tomasz Figa wrote:
> 2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas <javier.martinez@collabora.co.uk>:
>> On 06/15/2015 11:01 AM, Sudeep Holla wrote:

[...]

>>>
>>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and
>>> set_irq_wake also and then you can restore iff it's non-zero as
>>> irq core will take care of most of the non-wakeup sources.
>>> Because I am planning to push
>>
>> I've looking at this and a problem I found is that IIUC the
>> set_irq_wake is not propagated from the the Exynos pinctrl / GPIO
>> driver which is the combiner's external interrupt source so the
>> callback is never called. Which means that right now only the
>> state of the wakeup source IRQs can't be saved since that
>> information is not present.
>>
>> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and
>> disables the combiner interrupts but its .irq_set_wake handler
>> only updates the wakeup source mask for the external interrupts but
>> does not call the combiner .set_irq_wake so that should be changed
>> as well.
>>
>
> As far as I'm aware of, wake-up events from pin controllers don't go
>  through GIC, but rather directly to PMU, which is a dedicated unit
> responsible for power management and not a standalone interrupt
> controller (well actually I saw a series making it a cascaded
> controller some time ago, but I'm not sure if that went in). Based on
> this, I don't think we have to call set_irq_wake on GIC. Correct me
> if I'm wrong, though.

Thanks for the details, this was my assumption when Doug confirmed that
the combiner is also powered down, either there must be some bypass or a
dedicated logic to wakeup. But I was not sure though so insisted
set_irq_wake but based on what you say it's not required.

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 16/06/15 13:32, Tomasz Figa wrote:
> 2015-06-16 0:00 GMT+09:00 Javier Martinez Canillas <javier.martinez@collabora.co.uk>:
>> On 06/15/2015 11:01 AM, Sudeep Holla wrote:

[...]

>>>
>>> Agreed. But I would suggest also to add MASK_ON_SUSPEND and
>>> set_irq_wake also and then you can restore iff it's non-zero as
>>> irq core will take care of most of the non-wakeup sources.
>>> Because I am planning to push
>>
>> I've looking at this and a problem I found is that IIUC the
>> set_irq_wake is not propagated from the the Exynos pinctrl / GPIO
>> driver which is the combiner's external interrupt source so the
>> callback is never called. Which means that right now only the
>> state of the wakeup source IRQs can't be saved since that
>> information is not present.
>>
>> The drivers/pinctrl/samsung/pinctrl-exynos.c driver enables and
>> disables the combiner interrupts but its .irq_set_wake handler
>> only updates the wakeup source mask for the external interrupts but
>> does not call the combiner .set_irq_wake so that should be changed
>> as well.
>>
>
> As far as I'm aware of, wake-up events from pin controllers don't go
>  through GIC, but rather directly to PMU, which is a dedicated unit
> responsible for power management and not a standalone interrupt
> controller (well actually I saw a series making it a cascaded
> controller some time ago, but I'm not sure if that went in). Based on
> this, I don't think we have to call set_irq_wake on GIC. Correct me
> if I'm wrong, though.

Thanks for the details, this was my assumption when Doug confirmed that
the combiner is also powered down, either there must be some bypass or a
dedicated logic to wakeup. But I was not sure though so insisted
set_irq_wake but based on what you say it's not required.

Regards,
Sudeep

Re: [PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 21:17, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>>>> registers are lost assuming the combiner was powered down, even the
>>>> status register will be lost and you will not know exactly the wakeup
>>>> reason right ?
>>>>
>>>
>>> Good question, I didn't find in the documentation I've access to that
>>> this happen but just found through experimentation that the IRQ enable
>>> set register values are lost after a resume and that saving/restoring
>>> the values makes the interrupts to be triggered again.
>>>
>>
>> I'll share here too the findings I mentioned over IRC. As you suggested I
>> add some printouts and noticed that the ISTRn (Interrupt Status) registers
>> values are indeed preserved on resume so I can know for example if the
>> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
>> values of the registers against the Exynos manual and they corresponds to
>> the interrupt sources in each case so the values are correct.
>>
>> So as you said, it seems that is not that the IP block loses its state on
>> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
>> registers.
>
> I'll postulate an alternate theory here.  You can tell me if you buy
> it or if you think I've been out in the sun too long.
>
> Let's say that the interrupt combiner's status registers show the raw
> status as asserted by whatever is hooked up to the combiner.  This
> means that even if the combiner got reset we could still read the
> status register and get the status of the source.  Imagine that the
> combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
> all kinds of config (input vs. output, edge interrupt status, maybe
> pulls, etc).  ...but you can still read the state asserted by an
> external source, right?
>

Right, this makes sense. I assumed status was latched output and might
be lost. But that's wrong assumption I believe.

> In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
> pretty sure that the controller managine 'EINT 11' _doesn't_ lose
> power across suspend/resume...
>
>
> I'll further bolster my theory by saying that _almost nothing_ in the
> exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
> controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
> might be nice, but you're out of luck.  ...so it would make me
> terribly surprised to see the combiner keep power.
>

Interesting even GIC loses power ? I would be interested in knowing more
details as who will wake up the system then. Is the wakeup source
offloaded to some external power controller ?

>
>> To reduce the possible s/w components that could be doing this, I booted a
>> signed FIT image directly using the RO U-Boot instead of chain loading a
>> mainline nv-uboot. In this configuration I've the same issue so it seems
>> that if something is zeroing those registers on S2R, this can't be changed
>> without void the warranty of these machines.
>>
>> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
>> they have a very similar solution than my patch, the IESRn are also saved
>> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
>> or registering a syscore ops but the idea is basically the same.
>
> Yup, you can see where kliegs added it in
> <https://chromium-review.googlesource.com/#/c/27964/>.  As per the
> comments in that CL, this was probably broken in:
>
> 063bd6f ARM: EXYNOS: Remove GIC save & restore function
>
>
>> I have to take a look to the U-boot that is shipped on the device, I think
>> the correct branch is [1] but I'm not sure if that is the correct one.
>
> It is the right one.  If U-Boot were touching this (which would
> greatly surprise me) it should be here:
>
> arch/arm/include/asm/arch-exynos/cpu.h
>
> ...and it's not.  Doing a grep for '10440000' (the combiner base
> address) doesn't find anything in U-Boot either, which makes it less
> likely.  ...and it's even less likely since the amount of code that is
> in U-Boot that runs at resume time is a very small subset and I'm
> fairly familiar with it and I would have remembered it touching the
> combiner.
>

Yes, I did search and find nothing, so definitely not U-Boot if I am
looking at the right source.

> It's POSSIBLE that the internal ROM in exynos is clobbering this, but
> as per above it seems crazy unlikely and I think it's just losing
> power.
>

Agreed, not because I believe ROM code are not crazy but because of the
theory you have provided above :)

Regards,
Sudeep

[PATCH v2 1/1] irqchip: exynos-combiner: Save IRQ enable set on suspend

[Sudeep Holla]

On 12/06/15 21:17, Doug Anderson wrote:
> Hi,
>
> On Fri, Jun 12, 2015 at 12:36 PM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>>>> registers are lost assuming the combiner was powered down, even the
>>>> status register will be lost and you will not know exactly the wakeup
>>>> reason right ?
>>>>
>>>
>>> Good question, I didn't find in the documentation I've access to that
>>> this happen but just found through experimentation that the IRQ enable
>>> set register values are lost after a resume and that saving/restoring
>>> the values makes the interrupts to be triggered again.
>>>
>>
>> I'll share here too the findings I mentioned over IRC. As you suggested I
>> add some printouts and noticed that the ISTRn (Interrupt Status) registers
>> values are indeed preserved on resume so I can know for example if the
>> wakeup source was the power gpio-key or cros_ec keyboard. I've checked the
>> values of the registers against the Exynos manual and they corresponds to
>> the interrupt sources in each case so the values are correct.
>>
>> So as you said, it seems that is not that the IP block loses its state on
>> S2R but that something is blindly writing the IESRn (Interrupt Enable Set)
>> registers.
>
> I'll postulate an alternate theory here.  You can tell me if you buy
> it or if you think I've been out in the sun too long.
>
> Let's say that the interrupt combiner's status registers show the raw
> status as asserted by whatever is hooked up to the combiner.  This
> means that even if the combiner got reset we could still read the
> status register and get the status of the source.  Imagine that the
> combiner is like a GPIO bank.  If you reset the GPIO bank, you'll lose
> all kinds of config (input vs. output, edge interrupt status, maybe
> pulls, etc).  ...but you can still read the state asserted by an
> external source, right?
>

Right, this makes sense. I assumed status was latched output and might
be lost. But that's wrong assumption I believe.

> In this case the combiner's interrupt source is 'EINT 11'.  ...and I'm
> pretty sure that the controller managine 'EINT 11' _doesn't_ lose
> power across suspend/resume...
>
>
> I'll further bolster my theory by saying that _almost nothing_ in the
> exynos keeps power across suspend/resume.  The UART?  Nope.  The GPIO
> controllers?  Nuh-uh.  The GIC?  Sorry, but no.  The clock tree?  It
> might be nice, but you're out of luck.  ...so it would make me
> terribly surprised to see the combiner keep power.
>

Interesting even GIC loses power ? I would be interested in knowing more
details as who will wake up the system then. Is the wakeup source
offloaded to some external power controller ?

>
>> To reduce the possible s/w components that could be doing this, I booted a
>> signed FIT image directly using the RO U-Boot instead of chain loading a
>> mainline nv-uboot. In this configuration I've the same issue so it seems
>> that if something is zeroing those registers on S2R, this can't be changed
>> without void the warranty of these machines.
>>
>> I also looked at the downstream ChromiumOS v3.8 tree [0] and I see that
>> they have a very similar solution than my patch, the IESRn are also saved
>> but using a notifier for the CPU_PM_ENTER and CPU_PM_EXIT events instead
>> or registering a syscore ops but the idea is basically the same.
>
> Yup, you can see where kliegs added it in
> <https://chromium-review.googlesource.com/#/c/27964/>.  As per the
> comments in that CL, this was probably broken in:
>
> 063bd6f ARM: EXYNOS: Remove GIC save & restore function
>
>
>> I have to take a look to the U-boot that is shipped on the device, I think
>> the correct branch is [1] but I'm not sure if that is the correct one.
>
> It is the right one.  If U-Boot were touching this (which would
> greatly surprise me) it should be here:
>
> arch/arm/include/asm/arch-exynos/cpu.h
>
> ...and it's not.  Doing a grep for '10440000' (the combiner base
> address) doesn't find anything in U-Boot either, which makes it less
> likely.  ...and it's even less likely since the amount of code that is
> in U-Boot that runs at resume time is a very small subset and I'm
> fairly familiar with it and I would have remembered it touching the
> combiner.
>

Yes, I did search and find nothing, so definitely not U-Boot if I am
looking at the right source.

> It's POSSIBLE that the internal ROM in exynos is clobbering this, but
> as per above it seems crazy unlikely and I think it's just losing
> power.
>

Agreed, not because I believe ROM code are not crazy but because of the
theory you have provided above :)

Regards,
Sudeep

[tip:irq/core] irqchip: exynos-combiner: Save IRQ enable set on suspend

[tip-bot for Javier Martinez Canillas]

Commit-ID:  6fd4899a54a522ccd6a24fea2318d3b515b95945
Gitweb:     http://git.kernel.org/tip/6fd4899a54a522ccd6a24fea2318d3b515b95945
Author:     Javier Martinez Canillas <javier.martinez@collabora.co.uk>
AuthorDate: Fri, 12 Jun 2015 07:43:15 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 16 Jun 2015 11:34:41 +0200

irqchip: exynos-combiner: Save IRQ enable set on suspend

The Exynos interrupt combiner IP loses its state when the SoC enters
into a low power state during a Suspend-to-RAM. This means that if a
IRQ is used as a source, the interrupts for the devices are disabled
when the system is resumed from a sleep state so are not triggered.

Save the interrupt enable set register for each combiner group and
restore it after resume to make sure that the interrupts are enabled.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Doug Anderson <dianders@chromium.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Peter Chubb <peter.chubb@nicta.com.au>
Cc: Shuah Khan <shuahkhan@gmail.com>
Cc: Chanho Park <parkch98@gmail.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Link: http://lkml.kernel.org/r/1434087795-13990-1-git-send-email-javier.martinez@collabora.co.uk
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/exynos-combiner.c | 64 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/exynos-combiner.c b/drivers/irqchip/exynos-combiner.c
index a57a3a1..5c82e3b 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/interrupt.h>
@@ -34,9 +35,14 @@ struct combiner_chip_data {
 	unsigned int irq_mask;
 	void __iomem *base;
 	unsigned int parent_irq;
+#ifdef CONFIG_PM
+	u32 pm_save;
+#endif
 };
 
+static struct combiner_chip_data *combiner_data;
 static struct irq_domain *combiner_irq_domain;
+static unsigned int max_nr = 20;
 
 static inline void __iomem *combiner_base(struct irq_data *data)
 {
@@ -170,12 +176,10 @@ static const struct irq_domain_ops combiner_irq_domain_ops = {
 };
 
 static void __init combiner_init(void __iomem *combiner_base,
-				 struct device_node *np,
-				 unsigned int max_nr)
+				 struct device_node *np)
 {
 	int i, irq;
 	unsigned int nr_irq;
-	struct combiner_chip_data *combiner_data;
 
 	nr_irq = max_nr * IRQ_IN_COMBINER;
 
@@ -201,11 +205,59 @@ static void __init combiner_init(void __iomem *combiner_base,
 	}
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * combiner_suspend - save interrupt combiner state before suspend
+ *
+ * Save the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state.
+ *
+ */
+static int combiner_suspend(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++)
+		combiner_data[i].pm_save =
+			__raw_readl(combiner_data[i].base + COMBINER_ENABLE_SET);
+
+	return 0;
+}
+
+/**
+ * combiner_resume - restore interrupt combiner state after resume
+ *
+ * Restore the interrupt enable set register for all combiner groups since
+ * the state is lost when the system enters into a sleep state on suspend.
+ *
+ */
+static void combiner_resume(void)
+{
+	int i;
+
+	for (i = 0; i < max_nr; i++) {
+		__raw_writel(combiner_data[i].irq_mask,
+			     combiner_data[i].base + COMBINER_ENABLE_CLEAR);
+		__raw_writel(combiner_data[i].pm_save,
+			     combiner_data[i].base + COMBINER_ENABLE_SET);
+	}
+}
+
+#else
+#define combiner_suspend	NULL
+#define combiner_resume		NULL
+#endif
+
+static struct syscore_ops combiner_syscore_ops = {
+	.suspend	= combiner_suspend,
+	.resume		= combiner_resume,
+};
+
 static int __init combiner_of_init(struct device_node *np,
 				   struct device_node *parent)
 {
 	void __iomem *combiner_base;
-	unsigned int max_nr = 20;
 
 	combiner_base = of_iomap(np, 0);
 	if (!combiner_base) {
@@ -219,7 +271,9 @@ static int __init combiner_of_init(struct device_node *np,
 			__func__, max_nr);
 	}
 
-	combiner_init(combiner_base, np, max_nr);
+	combiner_init(combiner_base, np);
+
+	register_syscore_ops(&combiner_syscore_ops);
 
 	return 0;
 }