[PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Brian Norris]

Some (admittedly odd) irqchips perform functions that are not directly
related to any of their child IRQ lines, and therefore need to perform
some tasks during suspend/resume regardless of whether there are
any "installed" interrupts for the irqchip. However, the current
generic-chip framework does not call the chip's irq_{suspend,resume}
when there are no interrupts installed (this makes sense, because there
are no irq_data objects for such a call to be made).

More specifically, irq-bcm7120-l2 configures both a forwarding mask
(which affects other top-level GIC IRQs) and a second-level interrupt
mask (for managing its own child interrupts). The former must be
saved/restored on suspend/resume, even when there's nothing to do for
the latter.

This patch adds a second set of suspend/resume hooks to irq_chip, this
time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
These callbacks will always be called for an irqchip and are based on
the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
struct.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/irq.h       | 10 ++++++++++
 kernel/irq/generic-chip.c |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index de3213d271ff..2a672d2434a5 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -293,6 +293,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
+struct irq_chip_generic;
+
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
@@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_suspend:	function called from core code on suspend once per chip
  * @irq_resume:		function called from core code on resume once per chip
  * @irq_pm_shutdown:	function called from core code on shutdown once per chip
+ * @chip_suspend:	function called from core code on suspend once per
+ *			chip; for handling chip details even when no interrupts
+ *			are in use
+ * @chip_resume:	function called from core code on resume once per chip;
+ *			for handling chip details even when no interrupts are
+ *			in use
  * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
  * @irq_print_chip:	optional to print special chip info in show_interrupts
  * @irq_request_resources:	optional to request resources before calling
@@ -357,6 +365,8 @@ struct irq_chip {
 	void		(*irq_suspend)(struct irq_data *data);
 	void		(*irq_resume)(struct irq_data *data);
 	void		(*irq_pm_shutdown)(struct irq_data *data);
+	void		(*chip_suspend)(struct irq_chip_generic *gc);
+	void		(*chip_resume)(struct irq_chip_generic *gc);
 
 	void		(*irq_calc_mask)(struct irq_data *data);
 
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 15b370daf234..fe25e72d5d13 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -553,6 +553,9 @@ static int irq_gc_suspend(void)
 			if (data)
 				ct->chip.irq_suspend(data);
 		}
+
+		if (ct->chip.chip_suspend)
+			ct->chip.chip_suspend(gc);
 	}
 	return 0;
 }
@@ -564,6 +567,9 @@ static void irq_gc_resume(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
+		if (ct->chip.chip_resume)
+			ct->chip.chip_resume(gc);
+
 		if (ct->chip.irq_resume) {
 			struct irq_data *data = irq_gc_get_irq_data(gc);
 
-- 
1.9.1

[PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Brian Norris]

Some (admittedly odd) irqchips perform functions that are not directly
related to any of their child IRQ lines, and therefore need to perform
some tasks during suspend/resume regardless of whether there are
any "installed" interrupts for the irqchip. However, the current
generic-chip framework does not call the chip's irq_{suspend,resume}
when there are no interrupts installed (this makes sense, because there
are no irq_data objects for such a call to be made).

More specifically, irq-bcm7120-l2 configures both a forwarding mask
(which affects other top-level GIC IRQs) and a second-level interrupt
mask (for managing its own child interrupts). The former must be
saved/restored on suspend/resume, even when there's nothing to do for
the latter.

This patch adds a second set of suspend/resume hooks to irq_chip, this
time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
These callbacks will always be called for an irqchip and are based on
the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
struct.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/irq.h       | 10 ++++++++++
 kernel/irq/generic-chip.c |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index de3213d271ff..2a672d2434a5 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -293,6 +293,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 	return d->hwirq;
 }
 
+struct irq_chip_generic;
+
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
@@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_suspend:	function called from core code on suspend once per chip
  * @irq_resume:		function called from core code on resume once per chip
  * @irq_pm_shutdown:	function called from core code on shutdown once per chip
+ * @chip_suspend:	function called from core code on suspend once per
+ *			chip; for handling chip details even when no interrupts
+ *			are in use
+ * @chip_resume:	function called from core code on resume once per chip;
+ *			for handling chip details even when no interrupts are
+ *			in use
  * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
  * @irq_print_chip:	optional to print special chip info in show_interrupts
  * @irq_request_resources:	optional to request resources before calling
@@ -357,6 +365,8 @@ struct irq_chip {
 	void		(*irq_suspend)(struct irq_data *data);
 	void		(*irq_resume)(struct irq_data *data);
 	void		(*irq_pm_shutdown)(struct irq_data *data);
+	void		(*chip_suspend)(struct irq_chip_generic *gc);
+	void		(*chip_resume)(struct irq_chip_generic *gc);
 
 	void		(*irq_calc_mask)(struct irq_data *data);
 
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 15b370daf234..fe25e72d5d13 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -553,6 +553,9 @@ static int irq_gc_suspend(void)
 			if (data)
 				ct->chip.irq_suspend(data);
 		}
+
+		if (ct->chip.chip_suspend)
+			ct->chip.chip_suspend(gc);
 	}
 	return 0;
 }
@@ -564,6 +567,9 @@ static void irq_gc_resume(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
+		if (ct->chip.chip_resume)
+			ct->chip.chip_resume(gc);
+
 		if (ct->chip.irq_resume) {
 			struct irq_data *data = irq_gc_get_irq_data(gc);
 
-- 
1.9.1

[PATCH 2/2] IRQCHIP: bcm7120-l2: perform suspend/resume even without installed child IRQs

[Brian Norris]

Make use of the new irq_chip chip_{suspend,resume} callbacks.

This is required because if there are no installed child IRQs for
this chip, the irq_{suspend,resume} functions will not be called.
However, we still need to save/restore the forwarding mask, to enable
the top-level GIC interrupt; otherwise, we lose UART output after S3
resume.

In addition to refactoring the callbacks, we have to self-initialize the
mask cache, since the genirq core also doesn't initialize this until the
first child IRQ is installed.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index 299d4de2deb5..98f0129fe843 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -81,10 +81,9 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static void bcm7120_l2_intc_suspend(struct irq_data *d)
+static void bcm7120_l2_intc_suspend(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 	struct bcm7120_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
@@ -94,10 +93,9 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 	irq_gc_unlock(gc);
 }
 
-static void bcm7120_l2_intc_resume(struct irq_data *d)
+static void bcm7120_l2_intc_resume(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
@@ -281,8 +279,15 @@ int __init bcm7120_l2_intc_probe(struct device_node *dn,
 		ct->chip.irq_mask = irq_gc_mask_clr_bit;
 		ct->chip.irq_unmask = irq_gc_mask_set_bit;
 		ct->chip.irq_ack = irq_gc_noop;
-		ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
-		ct->chip.irq_resume = bcm7120_l2_intc_resume;
+		ct->chip.chip_suspend = bcm7120_l2_intc_suspend;
+		ct->chip.chip_resume = bcm7120_l2_intc_resume;
+
+		/*
+		 * Initialize mask-cache, in case we need it for
+		 * saving/restoring fwd mask even w/o any child interrupts
+		 * installed
+		 */
+		gc->mask_cache = irq_reg_readl(gc, ct->regs.mask);
 
 		if (data->can_wake) {
 			/* This IRQ chip can wake the system, set all
-- 
1.9.1

[PATCH 2/2] IRQCHIP: bcm7120-l2: perform suspend/resume even without installed child IRQs

[Brian Norris]

Make use of the new irq_chip chip_{suspend,resume} callbacks.

This is required because if there are no installed child IRQs for
this chip, the irq_{suspend,resume} functions will not be called.
However, we still need to save/restore the forwarding mask, to enable
the top-level GIC interrupt; otherwise, we lose UART output after S3
resume.

In addition to refactoring the callbacks, we have to self-initialize the
mask cache, since the genirq core also doesn't initialize this until the
first child IRQ is installed.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index 299d4de2deb5..98f0129fe843 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -81,10 +81,9 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static void bcm7120_l2_intc_suspend(struct irq_data *d)
+static void bcm7120_l2_intc_suspend(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 	struct bcm7120_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
@@ -94,10 +93,9 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 	irq_gc_unlock(gc);
 }
 
-static void bcm7120_l2_intc_resume(struct irq_data *d)
+static void bcm7120_l2_intc_resume(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
@@ -281,8 +279,15 @@ int __init bcm7120_l2_intc_probe(struct device_node *dn,
 		ct->chip.irq_mask = irq_gc_mask_clr_bit;
 		ct->chip.irq_unmask = irq_gc_mask_set_bit;
 		ct->chip.irq_ack = irq_gc_noop;
-		ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
-		ct->chip.irq_resume = bcm7120_l2_intc_resume;
+		ct->chip.chip_suspend = bcm7120_l2_intc_suspend;
+		ct->chip.chip_resume = bcm7120_l2_intc_resume;
+
+		/*
+		 * Initialize mask-cache, in case we need it for
+		 * saving/restoring fwd mask even w/o any child interrupts
+		 * installed
+		 */
+		gc->mask_cache = irq_reg_readl(gc, ct->regs.mask);
 
 		if (data->can_wake) {
 			/* This IRQ chip can wake the system, set all
-- 
1.9.1

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Florian Fainelli]

On 19/06/15 16:26, Brian Norris wrote:
> Some (admittedly odd) irqchips perform functions that are not directly
> related to any of their child IRQ lines, and therefore need to perform
> some tasks during suspend/resume regardless of whether there are
> any "installed" interrupts for the irqchip. However, the current
> generic-chip framework does not call the chip's irq_{suspend,resume}
> when there are no interrupts installed (this makes sense, because there
> are no irq_data objects for such a call to be made).
> 
> More specifically, irq-bcm7120-l2 configures both a forwarding mask
> (which affects other top-level GIC IRQs) and a second-level interrupt
> mask (for managing its own child interrupts). The former must be
> saved/restored on suspend/resume, even when there's nothing to do for
> the latter.
> 
> This patch adds a second set of suspend/resume hooks to irq_chip, this
> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
> These callbacks will always be called for an irqchip and are based on
> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
> struct.
> 
> The original problem report is described in extra detail here:
> http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  include/linux/irq.h       | 10 ++++++++++
>  kernel/irq/generic-chip.c |  6 ++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index de3213d271ff..2a672d2434a5 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -293,6 +293,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  	return d->hwirq;
>  }
>  
> +struct irq_chip_generic;
> +
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @irq_suspend:	function called from core code on suspend once per chip
>   * @irq_resume:		function called from core code on resume once per chip
>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
> + * @chip_suspend:	function called from core code on suspend once per
> + *			chip; for handling chip details even when no interrupts
> + *			are in use
> + * @chip_resume:	function called from core code on resume once per chip;
> + *			for handling chip details even when no interrupts are
> + *			in use
>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
>   * @irq_print_chip:	optional to print special chip info in show_interrupts
>   * @irq_request_resources:	optional to request resources before calling
> @@ -357,6 +365,8 @@ struct irq_chip {
>  	void		(*irq_suspend)(struct irq_data *data);
>  	void		(*irq_resume)(struct irq_data *data);
>  	void		(*irq_pm_shutdown)(struct irq_data *data);
> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
> +	void		(*chip_resume)(struct irq_chip_generic *gc);
>  
>  	void		(*irq_calc_mask)(struct irq_data *data);
>  
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index 15b370daf234..fe25e72d5d13 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -553,6 +553,9 @@ static int irq_gc_suspend(void)
>  			if (data)
>  				ct->chip.irq_suspend(data);
>  		}
> +
> +		if (ct->chip.chip_suspend)
> +			ct->chip.chip_suspend(gc);
>  	}
>  	return 0;
>  }
> @@ -564,6 +567,9 @@ static void irq_gc_resume(void)
>  	list_for_each_entry(gc, &gc_list, list) {
>  		struct irq_chip_type *ct = gc->chip_types;
>  
> +		if (ct->chip.chip_resume)
> +			ct->chip.chip_resume(gc);
> +
>  		if (ct->chip.irq_resume) {
>  			struct irq_data *data = irq_gc_get_irq_data(gc);
>  
> 


-- 
Florian

Re: [PATCH 2/2] IRQCHIP: bcm7120-l2: perform suspend/resume even without installed child IRQs

[Florian Fainelli]

On 19/06/15 16:26, Brian Norris wrote:
> Make use of the new irq_chip chip_{suspend,resume} callbacks.
> 
> This is required because if there are no installed child IRQs for
> this chip, the irq_{suspend,resume} functions will not be called.
> However, we still need to save/restore the forwarding mask, to enable
> the top-level GIC interrupt; otherwise, we lose UART output after S3
> resume.
> 
> In addition to refactoring the callbacks, we have to self-initialize the
> mask cache, since the genirq core also doesn't initialize this until the
> first child IRQ is installed.
> 
> The original problem report is described in extra detail here:
> http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  drivers/irqchip/irq-bcm7120-l2.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
> index 299d4de2deb5..98f0129fe843 100644
> --- a/drivers/irqchip/irq-bcm7120-l2.c
> +++ b/drivers/irqchip/irq-bcm7120-l2.c
> @@ -81,10 +81,9 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -static void bcm7120_l2_intc_suspend(struct irq_data *d)
> +static void bcm7120_l2_intc_suspend(struct irq_chip_generic *gc)
>  {
> -	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	struct irq_chip_type *ct = gc->chip_types;
>  	struct bcm7120_l2_intc_data *b = gc->private;
>  
>  	irq_gc_lock(gc);
> @@ -94,10 +93,9 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
>  	irq_gc_unlock(gc);
>  }
>  
> -static void bcm7120_l2_intc_resume(struct irq_data *d)
> +static void bcm7120_l2_intc_resume(struct irq_chip_generic *gc)
>  {
> -	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> -	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	struct irq_chip_type *ct = gc->chip_types;
>  
>  	/* Restore the saved mask */
>  	irq_gc_lock(gc);
> @@ -281,8 +279,15 @@ int __init bcm7120_l2_intc_probe(struct device_node *dn,
>  		ct->chip.irq_mask = irq_gc_mask_clr_bit;
>  		ct->chip.irq_unmask = irq_gc_mask_set_bit;
>  		ct->chip.irq_ack = irq_gc_noop;
> -		ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
> -		ct->chip.irq_resume = bcm7120_l2_intc_resume;
> +		ct->chip.chip_suspend = bcm7120_l2_intc_suspend;
> +		ct->chip.chip_resume = bcm7120_l2_intc_resume;
> +
> +		/*
> +		 * Initialize mask-cache, in case we need it for
> +		 * saving/restoring fwd mask even w/o any child interrupts
> +		 * installed
> +		 */

Your commit message is a tad clearer than this comment, but this is
still good enough to understand what is being done here.

> +		gc->mask_cache = irq_reg_readl(gc, ct->regs.mask);
>  
>  		if (data->can_wake) {
>  			/* This IRQ chip can wake the system, set all
> 


-- 
Florian

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Thomas Gleixner]

On Fri, 19 Jun 2015, Brian Norris wrote:
> This patch adds a second set of suspend/resume hooks to irq_chip, this
> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
> These callbacks will always be called for an irqchip and are based on
> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
> struct.

There is no per-chip irq_chip_generic struct. It's only there if the
irq chip has been instantiated as a generic chip.
 
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @irq_suspend:	function called from core code on suspend once per chip
>   * @irq_resume:		function called from core code on resume once per chip
>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
> + * @chip_suspend:	function called from core code on suspend once per
> + *			chip; for handling chip details even when no interrupts
> + *			are in use
> + * @chip_resume:	function called from core code on resume once per chip;
> + *			for handling chip details even when no interrupts are
> + *			in use
>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
>   * @irq_print_chip:	optional to print special chip info in show_interrupts
>   * @irq_request_resources:	optional to request resources before calling
> @@ -357,6 +365,8 @@ struct irq_chip {
>  	void		(*irq_suspend)(struct irq_data *data);
>  	void		(*irq_resume)(struct irq_data *data);
>  	void		(*irq_pm_shutdown)(struct irq_data *data);
> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
> +	void		(*chip_resume)(struct irq_chip_generic *gc);

I really don't want to set a precedent for random (*foo)(*bar)
callbacks.
 
> +
> +		if (ct->chip.chip_suspend)
> +			ct->chip.chip_suspend(gc);

So wouldn't it be the more intuitive solution to make this a callback
in the struct gc itself?

Thanks,

	tglx

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Florian Fainelli]

On 20/06/15 07:11, Thomas Gleixner wrote:
> On Fri, 19 Jun 2015, Brian Norris wrote:
>> This patch adds a second set of suspend/resume hooks to irq_chip, this
>> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
>> These callbacks will always be called for an irqchip and are based on
>> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
>> struct.
> 
> There is no per-chip irq_chip_generic struct. It's only there if the
> irq chip has been instantiated as a generic chip.
>  
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @irq_suspend:	function called from core code on suspend once per chip
>>   * @irq_resume:		function called from core code on resume once per chip
>>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
>> + * @chip_suspend:	function called from core code on suspend once per
>> + *			chip; for handling chip details even when no interrupts
>> + *			are in use
>> + * @chip_resume:	function called from core code on resume once per chip;
>> + *			for handling chip details even when no interrupts are
>> + *			in use
>>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
>>   * @irq_print_chip:	optional to print special chip info in show_interrupts
>>   * @irq_request_resources:	optional to request resources before calling
>> @@ -357,6 +365,8 @@ struct irq_chip {
>>  	void		(*irq_suspend)(struct irq_data *data);
>>  	void		(*irq_resume)(struct irq_data *data);
>>  	void		(*irq_pm_shutdown)(struct irq_data *data);
>> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
>> +	void		(*chip_resume)(struct irq_chip_generic *gc);
> 
> I really don't want to set a precedent for random (*foo)(*bar)
> callbacks.
>  
>> +
>> +		if (ct->chip.chip_suspend)
>> +			ct->chip.chip_suspend(gc);
> 
> So wouldn't it be the more intuitive solution to make this a callback
> in the struct gc itself?

Brian can correct me, but his approach is more generic, if there is
another irqchip driver needing a similar infrastructure, this would be
already there, and directly usable. Maybe all we need to is to change
the chip_suspend/resume arguments to pass a reference to irq_chip instead?

I can go ahead and rewrite that part of the patch to make this is
exclusively located to the irq_chip_generic structure instead.
--
Florian

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Thomas Gleixner]

On Tue, 21 Jul 2015, Florian Fainelli wrote:
> On 20/06/15 07:11, Thomas Gleixner wrote:
> > On Fri, 19 Jun 2015, Brian Norris wrote:
> >> This patch adds a second set of suspend/resume hooks to irq_chip, this
> >> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
> >> These callbacks will always be called for an irqchip and are based on
> >> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
> >> struct.
> > 
> > There is no per-chip irq_chip_generic struct. It's only there if the
> > irq chip has been instantiated as a generic chip.
> >  
> >>  /**
> >>   * struct irq_chip - hardware interrupt chip descriptor
> >>   *
> >> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
> >>   * @irq_suspend:	function called from core code on suspend once per chip
> >>   * @irq_resume:		function called from core code on resume once per chip
> >>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
> >> + * @chip_suspend:	function called from core code on suspend once per
> >> + *			chip; for handling chip details even when no interrupts
> >> + *			are in use
> >> + * @chip_resume:	function called from core code on resume once per chip;
> >> + *			for handling chip details even when no interrupts are
> >> + *			in use
> >>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
> >>   * @irq_print_chip:	optional to print special chip info in show_interrupts
> >>   * @irq_request_resources:	optional to request resources before calling
> >> @@ -357,6 +365,8 @@ struct irq_chip {
> >>  	void		(*irq_suspend)(struct irq_data *data);
> >>  	void		(*irq_resume)(struct irq_data *data);
> >>  	void		(*irq_pm_shutdown)(struct irq_data *data);
> >> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
> >> +	void		(*chip_resume)(struct irq_chip_generic *gc);
> > 
> > I really don't want to set a precedent for random (*foo)(*bar)
> > callbacks.
> >  
> >> +
> >> +		if (ct->chip.chip_suspend)
> >> +			ct->chip.chip_suspend(gc);
> > 
> > So wouldn't it be the more intuitive solution to make this a callback
> > in the struct gc itself?
> 
> Brian can correct me, but his approach is more generic, if there is
> another irqchip driver needing a similar infrastructure, this would be
> already there, and directly usable.

No it's not directly usable. It's only usable with irq_chip_generic
incarnations.

> Maybe all we need to is to change the chip_suspend/resume arguments
> to pass a reference to irq_chip instead?

I just read back on the problem report which was mentioned in the
changelog:

"It's not a problem with patch 7, exactly, it's a problem with the
 irqchip driver which handles the UART interrupt mask (irq-bcm7120-l2.c).
 The problem is that with a trimmed down device tree (such as the one
 found at arch/arm/boot/dts/bcm7445-bcm97445svmb.dtb), none of the child
 interrupts of the 'irq0_intc' node are described -- we don't have device
 tree nodes for them yet -- but we still require saving and restoring the
 forwarding mask (see 'brcm,int-fwd-mask') in order for the UART
 interrupts to continue operating."

So you are trying to work around a flaw in the device tree by adding
random callbacks to the core kernel?

Thanks,

	tglx

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Florian Fainelli]

On 21/07/15 14:23, Thomas Gleixner wrote:
> On Tue, 21 Jul 2015, Florian Fainelli wrote:
>> On 20/06/15 07:11, Thomas Gleixner wrote:
>>> On Fri, 19 Jun 2015, Brian Norris wrote:
>>>> This patch adds a second set of suspend/resume hooks to irq_chip, this
>>>> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
>>>> These callbacks will always be called for an irqchip and are based on
>>>> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
>>>> struct.
>>>
>>> There is no per-chip irq_chip_generic struct. It's only there if the
>>> irq chip has been instantiated as a generic chip.
>>>  
>>>>  /**
>>>>   * struct irq_chip - hardware interrupt chip descriptor
>>>>   *
>>>> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>>   * @irq_suspend:	function called from core code on suspend once per chip
>>>>   * @irq_resume:		function called from core code on resume once per chip
>>>>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
>>>> + * @chip_suspend:	function called from core code on suspend once per
>>>> + *			chip; for handling chip details even when no interrupts
>>>> + *			are in use
>>>> + * @chip_resume:	function called from core code on resume once per chip;
>>>> + *			for handling chip details even when no interrupts are
>>>> + *			in use
>>>>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
>>>>   * @irq_print_chip:	optional to print special chip info in show_interrupts
>>>>   * @irq_request_resources:	optional to request resources before calling
>>>> @@ -357,6 +365,8 @@ struct irq_chip {
>>>>  	void		(*irq_suspend)(struct irq_data *data);
>>>>  	void		(*irq_resume)(struct irq_data *data);
>>>>  	void		(*irq_pm_shutdown)(struct irq_data *data);
>>>> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
>>>> +	void		(*chip_resume)(struct irq_chip_generic *gc);
>>>
>>> I really don't want to set a precedent for random (*foo)(*bar)
>>> callbacks.
>>>  
>>>> +
>>>> +		if (ct->chip.chip_suspend)
>>>> +			ct->chip.chip_suspend(gc);
>>>
>>> So wouldn't it be the more intuitive solution to make this a callback
>>> in the struct gc itself?
>>
>> Brian can correct me, but his approach is more generic, if there is
>> another irqchip driver needing a similar infrastructure, this would be
>> already there, and directly usable.
> 
> No it's not directly usable. It's only usable with irq_chip_generic
> incarnations.
> 
>> Maybe all we need to is to change the chip_suspend/resume arguments
>> to pass a reference to irq_chip instead?
> 
> I just read back on the problem report which was mentioned in the
> changelog:
> 
> "It's not a problem with patch 7, exactly, it's a problem with the
>  irqchip driver which handles the UART interrupt mask (irq-bcm7120-l2.c).
>  The problem is that with a trimmed down device tree (such as the one
>  found at arch/arm/boot/dts/bcm7445-bcm97445svmb.dtb), none of the child
>  interrupts of the 'irq0_intc' node are described -- we don't have device
>  tree nodes for them yet -- but we still require saving and restoring the
>  forwarding mask (see 'brcm,int-fwd-mask') in order for the UART
>  interrupts to continue operating."
> 
> So you are trying to work around a flaw in the device tree by adding
> random callbacks to the core kernel?

Not quite, you could have your interrupt controller node declared in
Device Tree, but have no "interrupts" property referencing it because:

- the hardware is just not there, but you inherit a common Device Tree
skleten (*.dtsi)
- you could have Device Tree overlays which may or may not be loaded as
a result of finding expansion boards etc...

It just appeared that Brian was specifically testing with something that
exposed the problem.
-- 
Florian

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Florian Fainelli]

On 21/07/15 14:23, Thomas Gleixner wrote:
> On Tue, 21 Jul 2015, Florian Fainelli wrote:
>> On 20/06/15 07:11, Thomas Gleixner wrote:
>>> On Fri, 19 Jun 2015, Brian Norris wrote:
>>>> This patch adds a second set of suspend/resume hooks to irq_chip, this
>>>> time to represent *chip* suspend/resume, rather than IRQ suspend/resume.
>>>> These callbacks will always be called for an irqchip and are based on
>>>> the per-chip irq_chip_generic struct, rather than the per-IRQ irq_data
>>>> struct.
>>>
>>> There is no per-chip irq_chip_generic struct. It's only there if the
>>> irq chip has been instantiated as a generic chip.
>>>  
>>>>  /**
>>>>   * struct irq_chip - hardware interrupt chip descriptor
>>>>   *
>>>> @@ -317,6 +319,12 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>>   * @irq_suspend:	function called from core code on suspend once per chip
>>>>   * @irq_resume:		function called from core code on resume once per chip
>>>>   * @irq_pm_shutdown:	function called from core code on shutdown once per chip
>>>> + * @chip_suspend:	function called from core code on suspend once per
>>>> + *			chip; for handling chip details even when no interrupts
>>>> + *			are in use
>>>> + * @chip_resume:	function called from core code on resume once per chip;
>>>> + *			for handling chip details even when no interrupts are
>>>> + *			in use
>>>>   * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
>>>>   * @irq_print_chip:	optional to print special chip info in show_interrupts
>>>>   * @irq_request_resources:	optional to request resources before calling
>>>> @@ -357,6 +365,8 @@ struct irq_chip {
>>>>  	void		(*irq_suspend)(struct irq_data *data);
>>>>  	void		(*irq_resume)(struct irq_data *data);
>>>>  	void		(*irq_pm_shutdown)(struct irq_data *data);
>>>> +	void		(*chip_suspend)(struct irq_chip_generic *gc);
>>>> +	void		(*chip_resume)(struct irq_chip_generic *gc);
>>>
>>> I really don't want to set a precedent for random (*foo)(*bar)
>>> callbacks.
>>>  
>>>> +
>>>> +		if (ct->chip.chip_suspend)
>>>> +			ct->chip.chip_suspend(gc);
>>>
>>> So wouldn't it be the more intuitive solution to make this a callback
>>> in the struct gc itself?
>>
>> Brian can correct me, but his approach is more generic, if there is
>> another irqchip driver needing a similar infrastructure, this would be
>> already there, and directly usable.
> 
> No it's not directly usable. It's only usable with irq_chip_generic
> incarnations.
> 
>> Maybe all we need to is to change the chip_suspend/resume arguments
>> to pass a reference to irq_chip instead?
> 
> I just read back on the problem report which was mentioned in the
> changelog:
> 
> "It's not a problem with patch 7, exactly, it's a problem with the
>  irqchip driver which handles the UART interrupt mask (irq-bcm7120-l2.c).
>  The problem is that with a trimmed down device tree (such as the one
>  found at arch/arm/boot/dts/bcm7445-bcm97445svmb.dtb), none of the child
>  interrupts of the 'irq0_intc' node are described -- we don't have device
>  tree nodes for them yet -- but we still require saving and restoring the
>  forwarding mask (see 'brcm,int-fwd-mask') in order for the UART
>  interrupts to continue operating."
> 
> So you are trying to work around a flaw in the device tree by adding
> random callbacks to the core kernel?

Not quite, you could have your interrupt controller node declared in
Device Tree, but have no "interrupts" property referencing it because:

- the hardware is just not there, but you inherit a common Device Tree
skleten (*.dtsi)
- you could have Device Tree overlays which may or may not be loaded as
a result of finding expansion boards etc...

It just appeared that Brian was specifically testing with something that
exposed the problem.
-- 
Florian

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Brian Norris]

Hi Florian, Thomas,

On Tue, Jul 21, 2015 at 11:24:29AM -0700, Florian Fainelli wrote:
> On 20/06/15 07:11, Thomas Gleixner wrote:
> > On Fri, 19 Jun 2015, Brian Norris wrote:
...
> > I really don't want to set a precedent for random (*foo)(*bar)
> > callbacks.
> >  
> >> +
> >> +		if (ct->chip.chip_suspend)
> >> +			ct->chip.chip_suspend(gc);
> > 
> > So wouldn't it be the more intuitive solution to make this a callback
> > in the struct gc itself?
> 
> Brian can correct me, but his approach is more generic, if there is
> another irqchip driver needing a similar infrastructure, this would be
> already there, and directly usable. Maybe all we need to is to change
> the chip_suspend/resume arguments to pass a reference to irq_chip instead?

I believe Thomas is right. We could just make these into
irq_chip_generic callbacks, which would probably be the right
abstraction level. Wouldn't be much code change from this submission,
AFAICT.

(Sorry for dropping the ball on this one.)

Brian

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Thomas Gleixner]

On Tue, 21 Jul 2015, Florian Fainelli wrote:
> On 21/07/15 14:23, Thomas Gleixner wrote:
> > I just read back on the problem report which was mentioned in the
> > changelog:
> > 
> > "It's not a problem with patch 7, exactly, it's a problem with the
> >  irqchip driver which handles the UART interrupt mask (irq-bcm7120-l2.c).
> >  The problem is that with a trimmed down device tree (such as the one
> >  found at arch/arm/boot/dts/bcm7445-bcm97445svmb.dtb), none of the child
> >  interrupts of the 'irq0_intc' node are described -- we don't have device
> >  tree nodes for them yet -- but we still require saving and restoring the
> >  forwarding mask (see 'brcm,int-fwd-mask') in order for the UART
> >  interrupts to continue operating."
> > 
> > So you are trying to work around a flaw in the device tree by adding
> > random callbacks to the core kernel?
> 
> Not quite, you could have your interrupt controller node declared in
> Device Tree, but have no "interrupts" property referencing it because:
> 
> - the hardware is just not there, but you inherit a common Device Tree
> skleten (*.dtsi)
> - you could have Device Tree overlays which may or may not be loaded as
> a result of finding expansion boards etc...

So if no hardware is there which uses any of those interrupts, then
WHY is it a problem at all?

If it's a requirement that these registers must be restored (once, not
per irq), then I can see that it'd be nice to do that from the core.

Though that core suspend/resume function is generic chip specific. So
it does not make any sense to force it into struct irq_chip because we
have no core infrastructure to deal with it.

Thanks,

	tglx

[PATCH v2 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Brian Norris]

Some (admittedly odd) irqchips perform functions that are not directly
related to any of their child IRQ lines, and therefore need to perform
some tasks during suspend/resume regardless of whether there are
any "installed" interrupts for the irqchip. However, the current
generic-chip framework does not call the chip's irq_{suspend,resume}
when there are no interrupts installed (this makes sense, because there
are no irq_data objects for such a call to be made).

More specifically, irq-bcm7120-l2 configures both a forwarding mask
(which affects other top-level GIC IRQs) and a second-level interrupt
mask (for managing its own child interrupts). The former must be
saved/restored on suspend/resume, even when there's nothing to do for
the latter.

This patch adds a new set of suspend/resume hooks to irq_chip_generic,
to help represent *chip* suspend/resume, rather than IRQ suspend/resume.
These callbacks will always be called for an IRQ chip (regardless of the
installed interrupts) and are based on the per-chip irq_chip_generic
struct, rather than the per-IRQ irq_data struct.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1: https://lkml.org/lkml/2015/6/19/760

v1 -> v2:
  * clarify the comments on irq_chip::irq_{suspend,resume}
  * add new suspend/resume hooks to the irq_chip_generic instead of irq_chip,
    since that is the right level of abstraction (see v1 discussion)

 include/linux/irq.h       | 14 ++++++++++++--
 kernel/irq/generic-chip.c |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 92188b0225bb..9fd346e605ff 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -324,8 +324,10 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
  * @irq_cpu_online:	configure an interrupt source for a secondary CPU
  * @irq_cpu_offline:	un-configure an interrupt source for a secondary CPU
- * @irq_suspend:	function called from core code on suspend once per chip
- * @irq_resume:		function called from core code on resume once per chip
+ * @irq_suspend:	function called from core code on suspend once per
+ *			chip, when one or more interrupts are installed
+ * @irq_resume:		function called from core code on resume once per chip,
+ *			when one ore more interrupts are installed
  * @irq_pm_shutdown:	function called from core code on shutdown once per chip
  * @irq_calc_mask:	Optional function to set irq_data.mask for special cases
  * @irq_print_chip:	optional to print special chip info in show_interrupts
@@ -761,6 +763,12 @@ struct irq_chip_type {
  * @reg_base:		Register base address (virtual)
  * @reg_readl:		Alternate I/O accessor (defaults to readl if NULL)
  * @reg_writel:		Alternate I/O accessor (defaults to writel if NULL)
+ * @suspend:		Function called from core code on suspend once per
+ *			chip; can be useful instead of irq_chip::suspend to
+ *			handle chip details even when no interrupts are in use
+ * @resume:		Function called from core code on resume once per chip;
+ *			can be useful instead of irq_chip::suspend to handle
+ *			chip details even when no interrupts are in use
  * @irq_base:		Interrupt base nr for this chip
  * @irq_cnt:		Number of interrupts handled by this chip
  * @mask_cache:		Cached mask register shared between all chip types
@@ -787,6 +795,8 @@ struct irq_chip_generic {
 	void __iomem		*reg_base;
 	u32			(*reg_readl)(void __iomem *addr);
 	void			(*reg_writel)(u32 val, void __iomem *addr);
+	void			(*suspend)(struct irq_chip_generic *gc);
+	void			(*resume)(struct irq_chip_generic *gc);
 	unsigned int		irq_base;
 	unsigned int		irq_cnt;
 	u32			mask_cache;
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 15b370daf234..abd286afbd27 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -553,6 +553,9 @@ static int irq_gc_suspend(void)
 			if (data)
 				ct->chip.irq_suspend(data);
 		}
+
+		if (gc->suspend)
+			gc->suspend(gc);
 	}
 	return 0;
 }
@@ -564,6 +567,9 @@ static void irq_gc_resume(void)
 	list_for_each_entry(gc, &gc_list, list) {
 		struct irq_chip_type *ct = gc->chip_types;
 
+		if (gc->resume)
+			gc->resume(gc);
+
 		if (ct->chip.irq_resume) {
 			struct irq_data *data = irq_gc_get_irq_data(gc);
 
-- 
2.4.3.573.g4eafbef

[PATCH v2 2/2] IRQCHIP: bcm7120-l2: perform suspend/resume even without installed child IRQs

[Brian Norris]

Make use of the new irq_chip_generic suspend/resume callbacks.

This is required because if there are no installed child IRQs for this
chip, the irq_chip::irq_{suspend,resume} functions will not be called.
However, we still need to save/restore the forwarding mask, to enable
the top-level GIC interrupt; otherwise, we lose UART output after S3
resume.

In addition to refactoring the callbacks, we have to self-initialize the
mask cache, since the genirq core also doesn't initialize this until the
first child IRQ is installed.

The original problem report is described in extra detail here:
http://lkml.kernel.org/g/20150619224123.GL4917@ld-irv-0074

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1: https://lkml.org/lkml/2015/6/19/761

v1 -> v2: adapt to changes in patch 1

 drivers/irqchip/irq-bcm7120-l2.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index 3ba5cc780fcb..e5e51bd9fa48 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -81,10 +81,9 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static void bcm7120_l2_intc_suspend(struct irq_data *d)
+static void bcm7120_l2_intc_suspend(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 	struct bcm7120_l2_intc_data *b = gc->private;
 
 	irq_gc_lock(gc);
@@ -94,10 +93,9 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 	irq_gc_unlock(gc);
 }
 
-static void bcm7120_l2_intc_resume(struct irq_data *d)
+static void bcm7120_l2_intc_resume(struct irq_chip_generic *gc)
 {
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct irq_chip_type *ct = gc->chip_types;
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
@@ -280,8 +278,15 @@ int __init bcm7120_l2_intc_probe(struct device_node *dn,
 		ct->chip.irq_mask = irq_gc_mask_clr_bit;
 		ct->chip.irq_unmask = irq_gc_mask_set_bit;
 		ct->chip.irq_ack = irq_gc_noop;
-		ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
-		ct->chip.irq_resume = bcm7120_l2_intc_resume;
+		gc->suspend = bcm7120_l2_intc_suspend;
+		gc->resume = bcm7120_l2_intc_resume;
+
+		/*
+		 * Initialize mask-cache, in case we need it for
+		 * saving/restoring fwd mask even w/o any child interrupts
+		 * installed
+		 */
+		gc->mask_cache = irq_reg_readl(gc, ct->regs.mask);
 
 		if (data->can_wake) {
 			/* This IRQ chip can wake the system, set all
-- 
2.4.3.573.g4eafbef

Re: [PATCH 1/2] genirq: add chip_{suspend,resume} PM support to irq_chip

[Brian Norris]

On Tue, Jul 21, 2015 at 11:58:01PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Jul 2015, Florian Fainelli wrote:
> > On 21/07/15 14:23, Thomas Gleixner wrote:
> > > I just read back on the problem report which was mentioned in the
> > > changelog:
> > > 
> > > "It's not a problem with patch 7, exactly, it's a problem with the
> > >  irqchip driver which handles the UART interrupt mask (irq-bcm7120-l2.c).
> > >  The problem is that with a trimmed down device tree (such as the one
> > >  found at arch/arm/boot/dts/bcm7445-bcm97445svmb.dtb), none of the child
> > >  interrupts of the 'irq0_intc' node are described -- we don't have device
> > >  tree nodes for them yet -- but we still require saving and restoring the
> > >  forwarding mask (see 'brcm,int-fwd-mask') in order for the UART
> > >  interrupts to continue operating."
> > > 
> > > So you are trying to work around a flaw in the device tree by adding
> > > random callbacks to the core kernel?
> > 
> > Not quite, you could have your interrupt controller node declared in
> > Device Tree, but have no "interrupts" property referencing it because:
> > 
> > - the hardware is just not there, but you inherit a common Device Tree
> > skleten (*.dtsi)
> > - you could have Device Tree overlays which may or may not be loaded as
> > a result of finding expansion boards etc...
> 
> So if no hardware is there which uses any of those interrupts, then
> WHY is it a problem at all?

This particular badly-designed L2 interrupt controller not only
configures its own constituent interrupts, but it controls whether some
interrupts are seen at level 1 (e.g., GIC), rather than L2. So some
interrupts are affected, but not owned, by this hardware (and driver).

> If it's a requirement that these registers must be restored (once, not
> per irq), then I can see that it'd be nice to do that from the core.

Right, they must be restored for the whole chip.

> Though that core suspend/resume function is generic chip specific. So
> it does not make any sense to force it into struct irq_chip because we
> have no core infrastructure to deal with it.

Right, and that's what v2 does.

Thanks for the comments.

Brian