diff mbox series

[5/7] irqchip/renesas-rzg2l: cache registers on suspend/resume

Message ID 20231023102223.1309614-6-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Not Applicable, archived
Headers show
Series irqchip/renesas-rzg2l: add support for RZ/G3S SoC | expand

Commit Message

Claudiu Oct. 23, 2023, 10:22 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Cache registers content when going to suspend and restore them in resume
as these may be lost when switching to deep sleep states. With this
driver data has been marked as static to be able to play with it
in struct syscon_ops::{suspend, resume}. Because IA55 input is from
pin controller and IA55 resumes before pin controller we don't restore
interrupt enable bits here but let the pinctrl to do it on IA55 behalf
after pins are in proper state to avoid invalid interrupts.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 67 +++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 13 deletions(-)

Comments

Thomas Gleixner Oct. 27, 2023, 5:57 p.m. UTC | #1 
On Mon, Oct 23 2023 at 13:22, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Cache registers content when going to suspend and restore them in resume
> as these may be lost when switching to deep sleep states. With this
> driver data has been marked as static to be able to play with it
> in struct syscon_ops::{suspend, resume}.

I have no idea what you are trying to tell me here. Why do the
suspend/resume callbacks need a static data structure and cannot operate
on a pointer which wastes less builtin memory when the driver is not
used?

Also "play with it" is definitely not a technical term. See
Documentation/process/* which has lots of explanations how to write
proper change logs.

> Because IA55 input is from pin controller and IA55 resumes before pin
> controller we don't restore interrupt enable bits here but let the
> pinctrl to do it on IA55 behalf after pins are in proper state to
> avoid invalid interrupts.
> +
> +static struct rzg2l_irqc_priv {
>  	void __iomem *base;
>  	struct irq_fwspec fwspec[IRQC_NUM_IRQ];
>  	raw_spinlock_t lock;
> -};
> +	struct rzg2l_irqc_reg_cache cache;
> +} priv;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

>  
>  static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
>  {
> @@ -238,6 +251,37 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
>  	return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
>  }
>  
> +static int rzg2l_irqc_irq_suspend(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +
> +	cache->iitsr = readl_relaxed(priv.base + IITSR);
> +	for (u8 i = 0; i < 2; i++)
> +		cache->titsr[i] = readl_relaxed(priv.base + TITSR(i));
> +
> +	return 0;
> +}
> +
> +static void rzg2l_irqc_irq_resume(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +	u8 i;
> +
> +	/*
> +	 * Restore only interrupt type. TSSRx will be restored at the
> +	 * request of pin controller to avoid spurious interrupts due
> +	 * to invalid PIN states.
> +	 */
> +	for (i = 0; i < 2; i++)
> +		writel_relaxed(cache->titsr[i], priv.base + TITSR(i));
> +	writel_relaxed(cache->iitsr, priv.base + IITSR);
> +}
> +
> +static struct syscore_ops rzg2l_irqc_syscore_ops = {
> +	.suspend = rzg2l_irqc_irq_suspend,
> +	.resume = rzg2l_irqc_irq_resume,
> +};

Ditto.

>  static const struct irq_chip irqc_chip = {
>  	.name			= "rzg2l-irqc",
>  	.irq_eoi		= rzg2l_irqc_eoi,
> @@ -323,7 +367,6 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
>  	struct irq_domain *irq_domain, *parent_domain;
>  	struct platform_device *pdev;
>  	struct reset_control *resetn;
> -	struct rzg2l_irqc_priv *priv;

Make this pointer static at the top level and leave the rest of the code
alone and please give it a proper name. "priv" at the file level is
really non-descriptive.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 1ed9cb7178fa..e5e158bf028d 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -18,6 +18,7 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/spinlock.h>
+#include <linux/syscore_ops.h>
 
 #define IRQC_IRQ_START			1
 #define IRQC_IRQ_COUNT			8
@@ -39,6 +40,7 @@ 
 
 #define TSSR_OFFSET(n)			((n) % 4)
 #define TSSR_INDEX(n)			((n) / 4)
+#define TSSR_MAX_INDEX			8
 
 #define TITSR_TITSEL_EDGE_RISING	0
 #define TITSR_TITSEL_EDGE_FALLING	1
@@ -57,11 +59,22 @@ 
 #define TINT_EXTRACT_HWIRQ(x)           FIELD_GET(GENMASK(15, 0), (x))
 #define TINT_EXTRACT_GPIOINT(x)         FIELD_GET(GENMASK(31, 16), (x))
 
-struct rzg2l_irqc_priv {
+/**
+ * struct rzg2l_irqc_reg_cache - register cache
+ * @iitsr: IITSR register
+ * @titsr: TITSR registers
+ */
+struct rzg2l_irqc_reg_cache {
+	u32 iitsr;
+	u32 titsr[2];
+};
+
+static struct rzg2l_irqc_priv {
 	void __iomem *base;
 	struct irq_fwspec fwspec[IRQC_NUM_IRQ];
 	raw_spinlock_t lock;
-};
+	struct rzg2l_irqc_reg_cache cache;
+} priv;
 
 static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
 {
@@ -238,6 +251,37 @@  static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
 	return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
 }
 
+static int rzg2l_irqc_irq_suspend(void)
+{
+	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
+
+	cache->iitsr = readl_relaxed(priv.base + IITSR);
+	for (u8 i = 0; i < 2; i++)
+		cache->titsr[i] = readl_relaxed(priv.base + TITSR(i));
+
+	return 0;
+}
+
+static void rzg2l_irqc_irq_resume(void)
+{
+	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
+	u8 i;
+
+	/*
+	 * Restore only interrupt type. TSSRx will be restored at the
+	 * request of pin controller to avoid spurious interrupts due
+	 * to invalid PIN states.
+	 */
+	for (i = 0; i < 2; i++)
+		writel_relaxed(cache->titsr[i], priv.base + TITSR(i));
+	writel_relaxed(cache->iitsr, priv.base + IITSR);
+}
+
+static struct syscore_ops rzg2l_irqc_syscore_ops = {
+	.suspend = rzg2l_irqc_irq_suspend,
+	.resume = rzg2l_irqc_irq_resume,
+};
+
 static const struct irq_chip irqc_chip = {
 	.name			= "rzg2l-irqc",
 	.irq_eoi		= rzg2l_irqc_eoi,
@@ -323,7 +367,6 @@  static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
 	struct irq_domain *irq_domain, *parent_domain;
 	struct platform_device *pdev;
 	struct reset_control *resetn;
-	struct rzg2l_irqc_priv *priv;
 	int ret;
 
 	pdev = of_find_device_by_node(node);
@@ -336,15 +379,11 @@  static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
 		return -ENODEV;
 	}
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
+	priv.base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
+	if (IS_ERR(priv.base))
+		return PTR_ERR(priv.base);
 
-	priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
-
-	ret = rzg2l_irqc_parse_interrupts(priv, node);
+	ret = rzg2l_irqc_parse_interrupts(&priv, node);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret);
 		return ret;
@@ -367,17 +406,19 @@  static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
 		goto pm_disable;
 	}
 
-	raw_spin_lock_init(&priv->lock);
+	raw_spin_lock_init(&priv.lock);
 
 	irq_domain = irq_domain_add_hierarchy(parent_domain, 0, IRQC_NUM_IRQ,
 					      node, &rzg2l_irqc_domain_ops,
-					      priv);
+					      &priv);
 	if (!irq_domain) {
 		dev_err(&pdev->dev, "failed to add irq domain\n");
 		ret = -ENOMEM;
 		goto pm_put;
 	}
 
+	register_syscore_ops(&rzg2l_irqc_syscore_ops);
+
 	return 0;
 
 pm_put: