Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <58b2905a-afa1-4991-9d67-8952eaf4b9ea@tuxon.dev>
Date:   Wed, 15 Nov 2023 16:49:03 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 8/9] irqchip/renesas-rzg2l: Add support for suspend to
 RAM
Content-Language: en-US
To:     Biju Das <biju.das.jz@bp.renesas.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "conor+dt@kernel.org" <conor+dt@kernel.org>,
        "geert+renesas@glider.be" <geert+renesas@glider.be>,
        "magnus.damm@gmail.com" <magnus.damm@gmail.com>,
        "mturquette@baylibre.com" <mturquette@baylibre.com>,
        "sboyd@kernel.org" <sboyd@kernel.org>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>,
        "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
References: <20231115142749.853106-1-claudiu.beznea.uj@bp.renesas.com>
 <20231115142749.853106-9-claudiu.beznea.uj@bp.renesas.com>
 <TYCPR01MB11269C1937A0086A53D51D90486B1A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
From:   claudiu beznea <claudiu.beznea@tuxon.dev>
In-Reply-To: <TYCPR01MB11269C1937A0086A53D51D90486B1A@TYCPR01MB11269.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
>