Hi Claudiu,
Thanks for the patch.
> Subject: [PATCH v2 8/9] irqchip/renesas-rzg2l: Add support for suspend to
> RAM
>
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> irqchip-renesas-rzg2l driver is used on RZ/G3S SoC. RZ/G3S could go to
> deep sleep states where power to different SoC's parts are cut off and RAM
> is switched to self-refresh. The resume from these states is done with the
> help of bootloader.
>
> IA55 IRQ controller needs to be reconfigured when resuming from deep sleep
> state. For this the IA55 registers are cached in suspend and restored in
> resume.
>
> The IA55 IRQ controller is connected to GPIO controller and GIC as
> follows:
>
> ┌──────────┐ ┌──────────┐
> │ │ SPIX │ │
> │ ├─────────►│ │
> │ │ │ │
> │ │ │ │
> ┌────────┐IRQ0-7 │ IA55 │ │ GIC │
> Pin0 ───────►│ ├─────────────►│ │ │ │
> │ │ │ │ PPIY │ │
> ... │ GPIO │ │ ├─────────►│ │
> │ │GPIOINT0-127 │ │ │ │
> PinN ───────►│ ├─────────────►│ │ │ │
> └────────┘ └──────────┘ └──────────┘
>
> where:
> - Pin0 is the first GPIO controller pin
> - PinN is the last GPIO controller pin
> - SPIX is the SPI interrupt with identifier X
> - PPIY is the PPI interrupt with identifier Y
>
> Suspend/resume functionality was implemented with syscore_ops to be able
> to cache/restore the registers after/before GPIO controller suspend/resume
> was called. As suspend/resume function members of syscore_ops doesn't take
> any argument, to be able to access the cache data structure and
> controller's base address from within suspend/resume functions, the driver
> private data structure was declared as static in file, named
> rzg2l_irqc_data and driver has been adjusted accordingly for this.
>
> Because IA55 IRQC is resumed before GPIO controller and different GPIO
> pins could be in unwanted state for IA55 IRQC (e.g. HiZ) when IA55
> reconfiguration is done on resume path, to avoid spurious interrupts the
> IA55 resume configures only interrupt type on resume. The interrupt enable
> operation will be done at the end of GPIO controller resume.
> The interrupt type reconfiguration was kept in IA55 driver to minimize the
> number of subsystems interactions on suspend/resume b/w GPIO and
> IA55 drivers (as the IRQ reconfiguration from GPIO driver is done with IRQ
> specific APIs).
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - improved commit description
> - use uppercase letter after ":" in patch title
> - implemented review comments: used tabs to align initialized structures
> members, use proper naming for driver's private data structure
> - use local variable for controller's base address in suspend/resume
> functions
>
> drivers/irqchip/irq-renesas-rzg2l.c | 68 +++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-
> renesas-rzg2l.c
> index 45b696db220f..bd0dd9fcd68a 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
> @@ -55,17 +56,29 @@
> #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_reg_cache - registers cache (necessary for
> +suspend/resume)
> + * @iitsr: IITSR register
> + * @titsr: TITSR registers
> + */
> +struct rzg2l_irqc_reg_cache {
> + u32 iitsr;
> + u32 titsr[2];
> +};
> +
> /**
> * struct rzg2l_irqc_priv - IRQ controller private data structure
> * @base: controller's base address
> * @fwspec: IRQ firmware specific data
> * @lock: lock to protect concurrent access to hardware registers
> + * @cache: registers cache (necessary for suspend/resume)
> */
> -struct rzg2l_irqc_priv {
> +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;
> +} rzg2l_irqc_data;
Why can't you use a static pointer here and fill it in probe()
and use this pointer in suspend()/resume()?
Cheers,
Biju
>
> static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
> { @@ -246,6 +259,38 @@ 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 = &rzg2l_irqc_data.cache;
> + void __iomem *base = rzg2l_irqc_data.base;
> +
> + cache->iitsr = readl_relaxed(base + IITSR);
> + for (u8 i = 0; i < 2; i++)
> + cache->titsr[i] = readl_relaxed(base + TITSR(i));
> +
> + return 0;
> +}
> +
> +static void rzg2l_irqc_irq_resume(void) {
> + struct rzg2l_irqc_reg_cache *cache = &rzg2l_irqc_data.cache;
> + void __iomem *base = rzg2l_irqc_data.base;
> +
> + /*
> + * Restore only interrupt type. TSSRx will be restored at the
> + * request of pin controller to avoid spurious interrupts due
> + * to invalid PIN states.
> + */
> + for (u8 i = 0; i < 2; i++)
> + writel_relaxed(cache->titsr[i], base + TITSR(i));
> + writel_relaxed(cache->iitsr, 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,
> @@ -331,7 +376,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);
> @@ -344,15 +388,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;
> + rzg2l_irqc_data.base = devm_of_iomap(&pdev->dev, pdev->dev.of_node,
> 0, NULL);
> + if (IS_ERR(rzg2l_irqc_data.base))
> + return PTR_ERR(rzg2l_irqc_data.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(&rzg2l_irqc_data, node);
> if (ret) {
> dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret);
> return ret;
> @@ -375,17 +415,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(&rzg2l_irqc_data.lock);
>
> irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
> IRQC_NUM_IRQ,
> node, &rzg2l_irqc_domain_ops,
> - priv);
> + &rzg2l_irqc_data);
> 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:
> --
> 2.39.2
