| Message ID | 20231115142749.853106-9-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 |
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@bp.renesas.com> > > 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@bp.renesas.com> > --- > > 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
Hi, Biju, On 15.11.2023 16:45, Biju Das wrote: > 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@bp.renesas.com> >> >> 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@bp.renesas.com> >> --- >> >> 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()? I can do that. I think I wrongly understood previous review comment on this. I'll update and resend. Thank you, Claudiu Beznea > > 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 >
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; 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: