[PATCH 0/4] irqchip: imgpdc: Fix various issues

[PATCH 0/4] irqchip: imgpdc: Fix various issues

[Ed Blake]

This patch set contains fixes for various issues in the irq-imgpdc
driver.

Ed Blake (4):
  irqchip: imgpdc: Avoid unbalanced irq wake disable
  irqchip: imgpdc: Avoid immediate wake event during set_wake
  irqchip: imgpdc: Set sys wake polarities to active high
  irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

 drivers/irqchip/irq-imgpdc.c | 107 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH 1/4] irqchip: imgpdc: Avoid unbalanced irq wake disable

[Ed Blake]

To avoid 'unbalanced irq wake disable' kernel warnings, only pass on
wake disables to the parent irq controller if the previous wake enable
succeeded.

Some platforms utilise the wake capability of the parent irq
controller, so we must attempt to pass on wake enables to the parent.
But in other platforms the wake capability of the PDC is used, when
the parent isn't required to support wake and we need to avoid
unbalanced wake disables in this case.

Signed-off-by: Ed Blake <ed.blake@sondrel.com>
---
 drivers/irqchip/irq-imgpdc.c | 62 ++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
index c02d29c..2ac3210 100644
--- a/drivers/irqchip/irq-imgpdc.c
+++ b/drivers/irqchip/irq-imgpdc.c
@@ -63,21 +63,25 @@
 
 /**
  * struct pdc_intc_priv - private pdc interrupt data.
- * @nr_perips:		Number of peripheral interrupt signals.
- * @nr_syswakes:	Number of syswake signals.
- * @perip_irqs:		List of peripheral IRQ numbers handled.
- * @syswake_irq:	Shared PDC syswake IRQ number.
- * @domain:		IRQ domain for PDC peripheral and syswake IRQs.
- * @pdc_base:		Base of PDC registers.
- * @irq_route:		Cached version of PDC_IRQ_ROUTE register.
- * @lock:		Lock to protect the PDC syswake registers and the cached
- *			values of those registers in this struct.
+ * @nr_perips:			Number of peripheral interrupt signals.
+ * @nr_syswakes:		Number of syswake signals.
+ * @perip_irqs:			List of peripheral IRQ numbers handled.
+ * @perip_irq_wake_depths:	List of peripheral IRQs wake depths.
+ * @syswake_irq:		Shared PDC syswake IRQ number.
+ * @syswake_irq_wake_depth:	Shared PDC syswake IRQ wake depth.
+ * @domain:			IRQ domain for PDC peripheral and syswake IRQs.
+ * @pdc_base:			Base of PDC registers.
+ * @irq_route:			Cached version of PDC_IRQ_ROUTE register.
+ * @lock:			Lock to protect the PDC syswake regs and the
+ *				cached values of those regs in this struct.
  */
 struct pdc_intc_priv {
 	unsigned int		nr_perips;
 	unsigned int		nr_syswakes;
 	unsigned int		*perip_irqs;
+	unsigned int		*perip_irq_wake_depths;
 	unsigned int		syswake_irq;
+	unsigned int		syswake_irq_wake_depth;
 	struct irq_domain	*domain;
 	void __iomem		*pdc_base;
 
@@ -199,6 +203,17 @@ static int pdc_irq_set_wake(struct irq_data *data, unsigned int on)
 	irq_hw_number_t hw = data->hwirq;
 	unsigned int mask = (1 << 16) << hw;
 	unsigned int dst_irq;
+	unsigned int *dst_irq_wake_depth;
+	int ret;
+
+	/* control the destination IRQ wakeup too for standby mode */
+	if (hwirq_is_syswake(hw)) {
+		dst_irq = priv->syswake_irq;
+		dst_irq_wake_depth = &priv->syswake_irq_wake_depth;
+	} else {
+		dst_irq = priv->perip_irqs[hw];
+		dst_irq_wake_depth = &priv->perip_irq_wake_depths[hw];
+	}
 
 	raw_spin_lock(&priv->lock);
 	if (on)
@@ -206,14 +221,22 @@ static int pdc_irq_set_wake(struct irq_data *data, unsigned int on)
 	else
 		priv->irq_route &= ~mask;
 	pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
-	raw_spin_unlock(&priv->lock);
 
-	/* control the destination IRQ wakeup too for standby mode */
-	if (hwirq_is_syswake(hw))
-		dst_irq = priv->syswake_irq;
-	else
-		dst_irq = priv->perip_irqs[hw];
-	irq_set_irq_wake(dst_irq, on);
+	/*
+	 * Pass on the set_wake request to the parent interrupt controller.
+	 * The parent isn't required to support wake, but we must balance
+	 * successful irq_set_irq_wake calls.
+	 */
+	if (on || *dst_irq_wake_depth) {
+		ret = irq_set_irq_wake(dst_irq, on);
+		if (!ret) {
+			if (on)
+				(*dst_irq_wake_depth)++;
+			else
+				(*dst_irq_wake_depth)--;
+		}
+	}
+	raw_spin_unlock(&priv->lock);
 
 	return 0;
 }
@@ -359,6 +382,13 @@ static int pdc_intc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "cannot allocate perip IRQ list\n");
 		return -ENOMEM;
 	}
+	priv->perip_irq_wake_depths = devm_kzalloc(&pdev->dev,
+						   4 * priv->nr_perips,
+						   GFP_KERNEL);
+	if (!priv->perip_irq_wake_depths) {
+		dev_err(&pdev->dev, "cannot allocate perip IRQ wake depth list\n");
+		return -ENOMEM;
+	}
 	for (i = 0; i < priv->nr_perips; ++i) {
 		irq = platform_get_irq(pdev, 1 + i);
 		if (irq < 0) {
-- 
1.9.1

[PATCH 2/4] irqchip: imgpdc: Avoid immediate wake event during set_wake

[Ed Blake]

When enabling sys wakes, an immediate power transition may occur if the
wake input has been active in the past. Work around this by doing the
following:

    1) Set SYS_WAKE_CONFIG to the current value of SOC_POWER so that an
       immediate power transition has no effect
    2) Enable and disable the sys wake to flush the stale wake event
    3) Set SYS_WAKE_CONFIG back to the original value

Signed-off-by: Ed Blake <ed.blake@sondrel.com>
---
 drivers/irqchip/irq-imgpdc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
index 2ac3210..22d8f8a 100644
--- a/drivers/irqchip/irq-imgpdc.c
+++ b/drivers/irqchip/irq-imgpdc.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
 #include <linux/io.h>
@@ -18,6 +19,7 @@
 
 /* PDC interrupt register numbers */
 
+#define PDC_SOC_POWER			0x300
 #define PDC_IRQ_STATUS			0x310
 #define PDC_IRQ_ENABLE			0x314
 #define PDC_IRQ_CLEAR			0x318
@@ -204,6 +206,8 @@ static int pdc_irq_set_wake(struct irq_data *data, unsigned int on)
 	unsigned int mask = (1 << 16) << hw;
 	unsigned int dst_irq;
 	unsigned int *dst_irq_wake_depth;
+	unsigned int syswake, soc_sys_wake_cfg_regoff;
+	u32 soc_sys_wake_cfg, soc_power;
 	int ret;
 
 	/* control the destination IRQ wakeup too for standby mode */
@@ -216,6 +220,27 @@ static int pdc_irq_set_wake(struct irq_data *data, unsigned int on)
 	}
 
 	raw_spin_lock(&priv->lock);
+	/*
+	 * When enabling sys wakes, an immediate power transition may occur if
+	 * the wake input has been active in the past. Work around this:
+	 * 1) Set SYS_WAKE_CONFIG to the current value of SOC_POWER so that an
+	 *    immediate power transition has no effect
+	 * 2) Enable and disable the sys wake to flush the stale wake event
+	 * 3) Set SYS_WAKE_CONFIG back to the original value
+	 */
+	if (hwirq_is_syswake(hw) && on) {
+		syswake = hwirq_to_syswake(hw);
+		soc_sys_wake_cfg_regoff = PDC_SYS_WAKE_CONFIG_BASE +
+					  syswake * PDC_SYS_WAKE_CONFIG_STRIDE;
+		soc_sys_wake_cfg = pdc_read(priv, soc_sys_wake_cfg_regoff);
+		soc_power = pdc_read(priv, PDC_SOC_POWER);
+		pdc_write(priv, soc_sys_wake_cfg_regoff, soc_power);
+		pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route | mask);
+		udelay(31);
+		pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
+		pdc_write(priv, soc_sys_wake_cfg_regoff, soc_sys_wake_cfg);
+	}
+
 	if (on)
 		priv->irq_route |= mask;
 	else
-- 
1.9.1

[PATCH 3/4] irqchip: imgpdc: Set sys wake polarities to active high

[Ed Blake]

Set all sys wake polarities to active high during initial setup.  The
default is active low, which currently causes the 'flow_type' passed
into the set_type function to be effectively inverted.

Signed-off-by: Ed Blake <ed.blake@sondrel.com>
---
 drivers/irqchip/irq-imgpdc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
index 22d8f8a..d1bcfef 100644
--- a/drivers/irqchip/irq-imgpdc.c
+++ b/drivers/irqchip/irq-imgpdc.c
@@ -49,6 +49,8 @@
 #define PDC_IRQ_ROUTE_EXT_EN_WD		0x00000004
 #define PDC_IRQ_ROUTE_EXT_EN_IR		0x00000002
 #define PDC_IRQ_ROUTE_EXT_EN_RTC	0x00000001
+#define PDC_SYS_WAKE_POL		0x00000020
+#define PDC_SYS_WAKE_POL_SHIFT		5
 #define PDC_SYS_WAKE_RESET		0x00000010
 #define PDC_SYS_WAKE_INT_MODE		0x0000000e
 #define PDC_SYS_WAKE_INT_MODE_SHIFT	1
@@ -63,6 +65,9 @@
 #define PDC_SYS_WAKE_INT_CHANGE		0x6
 #define PDC_SYS_WAKE_INT_NONE		0x4
 
+#define PDC_SYS_WAKE_ACTIVE_LOW		0x0
+#define PDC_SYS_WAKE_ACTIVE_HIGH	0x1
+
 /**
  * struct pdc_intc_priv - private pdc interrupt data.
  * @nr_perips:			Number of peripheral interrupt signals.
@@ -335,8 +340,9 @@ static void pdc_intc_setup(struct pdc_intc_priv *priv)
 	for (i = 0; i < priv->nr_syswakes; ++i) {
 		/* set the IRQ mode to none */
 		soc_sys_wake_regoff = PDC_SYS_WAKE_BASE + i*PDC_SYS_WAKE_STRIDE;
-		soc_sys_wake = PDC_SYS_WAKE_INT_NONE
-				<< PDC_SYS_WAKE_INT_MODE_SHIFT;
+		soc_sys_wake =
+			PDC_SYS_WAKE_ACTIVE_HIGH << PDC_SYS_WAKE_POL_SHIFT |
+			PDC_SYS_WAKE_INT_NONE << PDC_SYS_WAKE_INT_MODE_SHIFT;
 		pdc_write(priv, soc_sys_wake_regoff, soc_sys_wake);
 	}
 }
-- 
1.9.1

[PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[Ed Blake]

Pass on peripheral (RTC/IR/WD) irq masks and unmasks to the parent
interrupt controller, as well as setting / clearing the relevant bits
in the IRQ_ROUTE register.

Clearing bits in the IRQ_ROUTE register will prevent future interrupts
from being passed on to the parent, but won't mask an existing
interrupt which has already made it to the parent.  This is currently
causing peipheral interrupts to fire continuously when the system wakes
from a suspended state when one of the peripherals is used to wake the
system (e.g. RTC, IR).  The interrupt occurs early in the wake process
(still in the noirq phase) and because the peripheral interrupt is
disabled at that point, the core marks it as pending and masks it out.
This mask must be passed to the parent controller to be effective.

Signed-off-by: Ed Blake <ed.blake@sondrel.com>
---
 drivers/irqchip/irq-imgpdc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
index d1bcfef..05c48dd 100644
--- a/drivers/irqchip/irq-imgpdc.c
+++ b/drivers/irqchip/irq-imgpdc.c
@@ -141,21 +141,31 @@ static struct pdc_intc_priv *irqd_to_priv(struct irq_data *data)
 static void perip_irq_mask(struct irq_data *data)
 {
 	struct pdc_intc_priv *priv = irqd_to_priv(data);
+	unsigned int parent_irq = priv->perip_irqs[data->hwirq];
+	struct irq_data *parent_irq_data = irq_get_irq_data(parent_irq);
 
 	raw_spin_lock(&priv->lock);
 	priv->irq_route &= ~data->mask;
 	pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
 	raw_spin_unlock(&priv->lock);
+
+	/* Pass on the mask to the parent */
+	parent_irq_data->chip->irq_mask(parent_irq_data);
 }
 
 static void perip_irq_unmask(struct irq_data *data)
 {
 	struct pdc_intc_priv *priv = irqd_to_priv(data);
+	unsigned int parent_irq = priv->perip_irqs[data->hwirq];
+	struct irq_data *parent_irq_data = irq_get_irq_data(parent_irq);
 
 	raw_spin_lock(&priv->lock);
 	priv->irq_route |= data->mask;
 	pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
 	raw_spin_unlock(&priv->lock);
+
+	/* Pass on the unmask to the parent */
+	parent_irq_data->chip->irq_unmask(parent_irq_data);
 }
 
 static int syswake_irq_set_type(struct irq_data *data, unsigned int flow_type)
-- 
1.9.1

Re: [PATCH 0/4] irqchip: imgpdc: Fix various issues

[Thomas Gleixner]

On Mon, 2 Oct 2017, Ed Blake wrote:

Cc'ing James Hogan, the author of that driver ...

> This patch set contains fixes for various issues in the irq-imgpdc
> driver.
> 
> Ed Blake (4):
>   irqchip: imgpdc: Avoid unbalanced irq wake disable
>   irqchip: imgpdc: Avoid immediate wake event during set_wake
>   irqchip: imgpdc: Set sys wake polarities to active high
>   irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent
> 
>  drivers/irqchip/irq-imgpdc.c | 107 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 89 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.1
> 
> 

Re: [PATCH 3/4] irqchip: imgpdc: Set sys wake polarities to active high

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

Hi Ed,

On Mon, Oct 02, 2017 at 10:55:58AM +0100, Ed Blake wrote:
> 
> Set all sys wake polarities to active high during initial setup.  The
> default is active low, which currently causes the 'flow_type' passed
> into the set_type function to be effectively inverted.
> 
> Signed-off-by: Ed Blake <ed.blake@sondrel.com>
> ---
>  drivers/irqchip/irq-imgpdc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
> index 22d8f8a..d1bcfef 100644
> --- a/drivers/irqchip/irq-imgpdc.c
> +++ b/drivers/irqchip/irq-imgpdc.c
> @@ -49,6 +49,8 @@
>  #define PDC_IRQ_ROUTE_EXT_EN_WD		0x00000004
>  #define PDC_IRQ_ROUTE_EXT_EN_IR		0x00000002
>  #define PDC_IRQ_ROUTE_EXT_EN_RTC	0x00000001
> +#define PDC_SYS_WAKE_POL		0x00000020
> +#define PDC_SYS_WAKE_POL_SHIFT		5

I don't see this bit in the documentation for the PDC in TZ1090, so it
must be new in later versions of the IP. Maybe that can be mentioned in
the commit message or a code comment.

Does it need backporting to any stable branches (presumably v4.1+ for
Pistachio's sake)?

>  #define PDC_SYS_WAKE_RESET		0x00000010
>  #define PDC_SYS_WAKE_INT_MODE		0x0000000e
>  #define PDC_SYS_WAKE_INT_MODE_SHIFT	1
> @@ -63,6 +65,9 @@
>  #define PDC_SYS_WAKE_INT_CHANGE		0x6
>  #define PDC_SYS_WAKE_INT_NONE		0x4
>  
> +#define PDC_SYS_WAKE_ACTIVE_LOW		0x0
> +#define PDC_SYS_WAKE_ACTIVE_HIGH	0x1

Its a pity this HW isn't compatible with the older version which I
presume treated the INT_MODEs as active high but would have reported 0
in this bit. Never mind.

> +
>  /**
>   * struct pdc_intc_priv - private pdc interrupt data.
>   * @nr_perips:			Number of peripheral interrupt signals.
> @@ -335,8 +340,9 @@ static void pdc_intc_setup(struct pdc_intc_priv *priv)
>  	for (i = 0; i < priv->nr_syswakes; ++i) {
>  		/* set the IRQ mode to none */
>  		soc_sys_wake_regoff = PDC_SYS_WAKE_BASE + i*PDC_SYS_WAKE_STRIDE;
> -		soc_sys_wake = PDC_SYS_WAKE_INT_NONE
> -				<< PDC_SYS_WAKE_INT_MODE_SHIFT;
> +		soc_sys_wake =
> +			PDC_SYS_WAKE_ACTIVE_HIGH << PDC_SYS_WAKE_POL_SHIFT |
> +			PDC_SYS_WAKE_INT_NONE << PDC_SYS_WAKE_INT_MODE_SHIFT;

Looks reasonable. With a tweaked commit message or comment as mentioned
above:

Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James

>  		pdc_write(priv, soc_sys_wake_regoff, soc_sys_wake);
>  	}
>  }
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2923 bytes --]

Hi Ed,

On Mon, Oct 02, 2017 at 10:55:59AM +0100, Ed Blake wrote:
> 
> Pass on peripheral (RTC/IR/WD) irq masks and unmasks to the parent
> interrupt controller, as well as setting / clearing the relevant bits
> in the IRQ_ROUTE register.
> 
> Clearing bits in the IRQ_ROUTE register will prevent future interrupts
> from being passed on to the parent, but won't mask an existing
> interrupt which has already made it to the parent.

Is it an edge or level sensitive interrupt from the PDC?

I'm a little rusty on the IRQ subsystem TBH, but if edge sensitive I
would have expected the parent interrupt to be acked/cleared by the
parent handler.

And if level sensitive I would have expected the deasserted parent
interrupt to be masked by the parent handler, and immediately cleared
upon rerouting.

Maybe you can clarify whats going on here.

Cheers
James

> This is currently
> causing peipheral interrupts to fire continuously when the system wakes
> from a suspended state when one of the peripherals is used to wake the
> system (e.g. RTC, IR).  The interrupt occurs early in the wake process
> (still in the noirq phase) and because the peripheral interrupt is
> disabled at that point, the core marks it as pending and masks it out.
> This mask must be passed to the parent controller to be effective.
> 
> Signed-off-by: Ed Blake <ed.blake@sondrel.com>
> ---
>  drivers/irqchip/irq-imgpdc.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
> index d1bcfef..05c48dd 100644
> --- a/drivers/irqchip/irq-imgpdc.c
> +++ b/drivers/irqchip/irq-imgpdc.c
> @@ -141,21 +141,31 @@ static struct pdc_intc_priv *irqd_to_priv(struct irq_data *data)
>  static void perip_irq_mask(struct irq_data *data)
>  {
>  	struct pdc_intc_priv *priv = irqd_to_priv(data);
> +	unsigned int parent_irq = priv->perip_irqs[data->hwirq];
> +	struct irq_data *parent_irq_data = irq_get_irq_data(parent_irq);
>  
>  	raw_spin_lock(&priv->lock);
>  	priv->irq_route &= ~data->mask;
>  	pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
>  	raw_spin_unlock(&priv->lock);
> +
> +	/* Pass on the mask to the parent */
> +	parent_irq_data->chip->irq_mask(parent_irq_data);
>  }
>  
>  static void perip_irq_unmask(struct irq_data *data)
>  {
>  	struct pdc_intc_priv *priv = irqd_to_priv(data);
> +	unsigned int parent_irq = priv->perip_irqs[data->hwirq];
> +	struct irq_data *parent_irq_data = irq_get_irq_data(parent_irq);
>  
>  	raw_spin_lock(&priv->lock);
>  	priv->irq_route |= data->mask;
>  	pdc_write(priv, PDC_IRQ_ROUTE, priv->irq_route);
>  	raw_spin_unlock(&priv->lock);
> +
> +	/* Pass on the unmask to the parent */
> +	parent_irq_data->chip->irq_unmask(parent_irq_data);
>  }
>  
>  static int syswake_irq_set_type(struct irq_data *data, unsigned int flow_type)
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] irqchip: imgpdc: Set sys wake polarities to active high

[Ed Blake]

Hi James,


Thanks for the review.


On 04/10/17 14:14, James Hogan wrote:
> Hi Ed,
>
> On Mon, Oct 02, 2017 at 10:55:58AM +0100, Ed Blake wrote:
>> Set all sys wake polarities to active high during initial setup.  The
>> default is active low, which currently causes the 'flow_type' passed
>> into the set_type function to be effectively inverted.
>>
>> Signed-off-by: Ed Blake <ed.blake@sondrel.com>
>> ---
>>  drivers/irqchip/irq-imgpdc.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-imgpdc.c b/drivers/irqchip/irq-imgpdc.c
>> index 22d8f8a..d1bcfef 100644
>> --- a/drivers/irqchip/irq-imgpdc.c
>> +++ b/drivers/irqchip/irq-imgpdc.c
>> @@ -49,6 +49,8 @@
>>  #define PDC_IRQ_ROUTE_EXT_EN_WD		0x00000004
>>  #define PDC_IRQ_ROUTE_EXT_EN_IR		0x00000002
>>  #define PDC_IRQ_ROUTE_EXT_EN_RTC	0x00000001
>> +#define PDC_SYS_WAKE_POL		0x00000020
>> +#define PDC_SYS_WAKE_POL_SHIFT		5
> I don't see this bit in the documentation for the PDC in TZ1090, so it
> must be new in later versions of the IP. Maybe that can be mentioned in
> the commit message or a code comment.

OK, I'll add this in v2.


> Does it need backporting to any stable branches (presumably v4.1+ for
> Pistachio's sake)?

Pistachio didn't have a PDC, so I don't think this needs backporting.

Ed.

Re: [PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[Ed Blake]

On 04/10/17 15:03, James Hogan wrote:
> Hi Ed,
>
> On Mon, Oct 02, 2017 at 10:55:59AM +0100, Ed Blake wrote:
>> Pass on peripheral (RTC/IR/WD) irq masks and unmasks to the parent
>> interrupt controller, as well as setting / clearing the relevant bits
>> in the IRQ_ROUTE register.
>>
>> Clearing bits in the IRQ_ROUTE register will prevent future interrupts
>> from being passed on to the parent, but won't mask an existing
>> interrupt which has already made it to the parent.
> Is it an edge or level sensitive interrupt from the PDC?

It's level triggered.

> I'm a little rusty on the IRQ subsystem TBH, but if edge sensitive I
> would have expected the parent interrupt to be acked/cleared by the
> parent handler.
>
> And if level sensitive I would have expected the deasserted parent
> interrupt to be masked by the parent handler, and immediately cleared
> upon rerouting.
>
> Maybe you can clarify whats going on here.

I'm not sure how this is supposed to work, but the issue seems to be
that without this patch the parent irq isn't being masked.  This is
causing the parent handler (MIPS GIC in this case) to be called
continuously.  This leads to the PDC irq being masked each time, but not
the parent irq.  This is the callstack:

    "irq-imgpdc.c"::perip_irq_mask
    mask_ack_irq
    handle_level_irq
    generic_handle_irq_desc
    generic_handle_irq
    generic_handle_irq_desc
    generic_handle_irq
    gic_handle_shared_int
    gic_handle_local_int
    "irq-mips-gic.c"::gic_irq_dispatch
    generic_handle_irq_desc
    generic_handle_irq
    do_IRQ
    plat_irq_dispatch()

Ed.

Re: [PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

On Thu, Oct 05, 2017 at 03:48:53PM +0100, Ed Blake wrote:
> On 04/10/17 15:03, James Hogan wrote:
> > Hi Ed,
> >
> > On Mon, Oct 02, 2017 at 10:55:59AM +0100, Ed Blake wrote:
> >> Pass on peripheral (RTC/IR/WD) irq masks and unmasks to the parent
> >> interrupt controller, as well as setting / clearing the relevant bits
> >> in the IRQ_ROUTE register.
> >>
> >> Clearing bits in the IRQ_ROUTE register will prevent future interrupts
> >> from being passed on to the parent, but won't mask an existing
> >> interrupt which has already made it to the parent.
> > Is it an edge or level sensitive interrupt from the PDC?
> 
> It's level triggered.
> 
> > I'm a little rusty on the IRQ subsystem TBH, but if edge sensitive I
> > would have expected the parent interrupt to be acked/cleared by the
> > parent handler.
> >
> > And if level sensitive I would have expected the deasserted parent
> > interrupt to be masked by the parent handler, and immediately cleared
> > upon rerouting.
> >
> > Maybe you can clarify whats going on here.
> 
> I'm not sure how this is supposed to work, but the issue seems to be
> that without this patch the parent irq isn't being masked.  This is
> causing the parent handler (MIPS GIC in this case) to be called
> continuously.  This leads to the PDC irq being masked each time, but not
> the parent irq.  This is the callstack:
> 
>     "irq-imgpdc.c"::perip_irq_mask
>     mask_ack_irq
>     handle_level_irq
>     generic_handle_irq_desc
>     generic_handle_irq
>     generic_handle_irq_desc
>     generic_handle_irq
>     gic_handle_shared_int
>     gic_handle_local_int
>     "irq-mips-gic.c"::gic_irq_dispatch
>     generic_handle_irq_desc
>     generic_handle_irq
>     do_IRQ
>     plat_irq_dispatch()

Right, yeh it shouldn't technically be masked by the parent (contrary to
what I said above) because its a chained handler, i.e. as far as the
kernel knows there could be other IRQs coming through that GIC pin that
would also get masked.

(though IIRC the perip IRQs can wake, but then they go straight out to
separate dedicated IRQ pins into the main IRQ chip, i.e. the GIC in this
case).

I think its worth understanding the root cause here though. Disabling
routing of an IRQ fundamentally should deassert it. Is it an actual
hardware bug that has reached silicon?

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[Ed Blake]

On 05/10/17 16:26, James Hogan wrote:
> On Thu, Oct 05, 2017 at 03:48:53PM +0100, Ed Blake wrote:
>>
>> I'm not sure how this is supposed to work, but the issue seems to be
>> that without this patch the parent irq isn't being masked.  This is
>> causing the parent handler (MIPS GIC in this case) to be called
>> continuously.  This leads to the PDC irq being masked each time, but not
>> the parent irq.  This is the callstack:
>>
>>     "irq-imgpdc.c"::perip_irq_mask
>>     mask_ack_irq
>>     handle_level_irq
>>     generic_handle_irq_desc
>>     generic_handle_irq
>>     generic_handle_irq_desc
>>     generic_handle_irq
>>     gic_handle_shared_int
>>     gic_handle_local_int
>>     "irq-mips-gic.c"::gic_irq_dispatch
>>     generic_handle_irq_desc
>>     generic_handle_irq
>>     do_IRQ
>>     plat_irq_dispatch()
> Right, yeh it shouldn't technically be masked by the parent (contrary to
> what I said above) because its a chained handler, i.e. as far as the
> kernel knows there could be other IRQs coming through that GIC pin that
> would also get masked.
>
> (though IIRC the perip IRQs can wake, but then they go straight out to
> separate dedicated IRQ pins into the main IRQ chip, i.e. the GIC in this
> case).

That's right, each of the PDC peripherals (RTC, WD, IR) has a dedicated
IRQ to the parent, and the sys wakes are muxed onto a single IRQ.
> I think its worth understanding the root cause here though. Disabling
> routing of an IRQ fundamentally should deassert it. Is it an actual
> hardware bug that has reached silicon?

So you think the PDC->parent IRQ must not be being de-asserted when
IRQ_ROUTE is cleared?  I hadn't considered this and thought it was some
persistence in the GIC due to not being masked / ack'd there.  Is that
possible?  I'll discuss the possible IRQ_ROUTE issue with the hardware team.

Ed.

Re: [PATCH 4/4] irqchip: imgpdc: Pass on peripheral mask/unmasks to the parent

[Ed Blake]

On 05/10/17 16:43, Ed Blake wrote:
> On 05/10/17 16:26, James Hogan wrote:
>> On Thu, Oct 05, 2017 at 03:48:53PM +0100, Ed Blake wrote:
>>> I'm not sure how this is supposed to work, but the issue seems to be
>>> that without this patch the parent irq isn't being masked.  This is
>>> causing the parent handler (MIPS GIC in this case) to be called
>>> continuously.  This leads to the PDC irq being masked each time, but not
>>> the parent irq.  This is the callstack:
>>>
>>>     "irq-imgpdc.c"::perip_irq_mask
>>>     mask_ack_irq
>>>     handle_level_irq
>>>     generic_handle_irq_desc
>>>     generic_handle_irq
>>>     generic_handle_irq_desc
>>>     generic_handle_irq
>>>     gic_handle_shared_int
>>>     gic_handle_local_int
>>>     "irq-mips-gic.c"::gic_irq_dispatch
>>>     generic_handle_irq_desc
>>>     generic_handle_irq
>>>     do_IRQ
>>>     plat_irq_dispatch()
>> Right, yeh it shouldn't technically be masked by the parent (contrary to
>> what I said above) because its a chained handler, i.e. as far as the
>> kernel knows there could be other IRQs coming through that GIC pin that
>> would also get masked.
>>
>> (though IIRC the perip IRQs can wake, but then they go straight out to
>> separate dedicated IRQ pins into the main IRQ chip, i.e. the GIC in this
>> case).
> That's right, each of the PDC peripherals (RTC, WD, IR) has a dedicated
> IRQ to the parent, and the sys wakes are muxed onto a single IRQ.
>> I think its worth understanding the root cause here though. Disabling
>> routing of an IRQ fundamentally should deassert it. Is it an actual
>> hardware bug that has reached silicon?
> So you think the PDC->parent IRQ must not be being de-asserted when
> IRQ_ROUTE is cleared?  I hadn't considered this and thought it was some
> persistence in the GIC due to not being masked / ack'd there.  Is that
> possible?  I'll discuss the possible IRQ_ROUTE issue with the hardware team.

OK, I've looked at the RTL and discussed it with the hardware team, and
yes it's a bug.  Clearing IRQ_ROUTE will not clear a PDC->GIC IRQ once
asserted, the only way is to clear the RTC->PDC IRQ, which we can't do
at this point as interrupts are disabled.  So I think this method of
propagating the mask to the parent is a reasonable workaround.  Is it ok
if I just modify the commit message and add comments to make it clear
it's a workaround for a h/w bug?

Ed.