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@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()?
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
>
