Hi, Geert,
Sorry for replying that late to this one.
On 18.03.2024 18:48, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Thu, Mar 7, 2024 at 3:07 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> RZ/G3S supports deep sleep states that it can reach with the help of the
>> TF-A.
>>
>> RZ/G3S has a few power domains (e.g. GIC) that need to be always-on while
>> Linux is running. These domains are initialized (and powered on) when
>> clock driver is probed.
>>
>> As the TF-A takes control at the very last(suspend)/first(resume)
>> phase of configuring the deep sleep state, it can do it's own settings on
>> power domains.
>>
>> Thus, to restore the proper Linux state, add rzg2l_cpg_resume() which
>> powers on the always-on domains and rzg2l_cpg_complete() which activates
>> the power down mode for the IPs selected through CPG_PWRDN_IP{1, 2}.
>>
>> Along with it, added the suspend_check member to the RZ/G2L power domain
>> data structure whose purpose is to checks if a domain can be powered off
>> while the system is going to suspend. This is necessary for the serial
>> console domain which needs to be powered on if no_console_suspend is
>> available in bootargs.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>
> Thanks for your patch!
>
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>> @@ -1700,6 +1719,8 @@ static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd, bool always_on)
>> } else {
>> pd->genpd.power_on = rzg2l_cpg_power_on;
>> pd->genpd.power_off = rzg2l_cpg_power_off;
>> + if (flags & RZG2L_PD_F_CONSOLE)
>
> I think this should be replaced by some dynamic check, cfr. my comments
> on PATCH 9/10.
I agree.
>
>> + pd->suspend_check = rzg2l_pd_suspend_check_console;
>> governor = &simple_qos_governor;
>> }
>>
>
>> @@ -1890,9 +1911,43 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
>> if (error)
>> return error;
>>
>> + dev_set_drvdata(dev, priv);
>> +
>> return 0;
>> }
>>
>> +static int rzg2l_cpg_resume(struct device *dev)
>> +{
>> + struct rzg2l_cpg_priv *priv = dev_get_drvdata(dev);
>> + const struct rzg2l_cpg_info *info = priv->info;
>> +
>> + /* Power on always ON domains. */
>> + for (unsigned int i = 0; i < info->num_pm_domains; i++) {
>> + if (info->pm_domains[i].flags & RZG2L_PD_F_ALWAYS_ON) {
>
> If you would check "priv-domains[i].flags & GENPD_FLAG_ALWAYS_ON"
> instead, I think you can make r9a08g045_pm_domains[] __initconst.
> You may need to make a copy of the name for pd->genpd.name, though.
I wanted to avoid this copy.
>
>> + int ret = rzg2l_cpg_power_on(priv->domains[i]);
>
> I assume you are sure none of these domains are enabled by TF/A after
> system resume, or by the pmdomain core code?
Out of TF-A the MSTOP and PWRDN bits for these ones are set and setting
CPG_PWRDN_MSTOP though rzg2l_cpg_complete() leads to system being blocked.
It is the same as in booting case exlained in cover letter.
"the DDR, TZCDDR, OTFDE_DDR were also added, to avoid system being blocked
due to the following lines of code from patch 6/10.
+ /* Prepare for power down the BUSes in power down mode. */
+ if (info->pm_domain_pwrdn_mstop)
+ writel(CPG_PWRDN_MSTOP_ENABLE, priv->base + CPG_PWRDN_MSTOP);
Domain IDs were added to all SoC specific bindings.
"
The PM domain core code doesn't touch these domains while resuming as of my
checkings.
Thank you,
Claudiu Beznea
>
> Gr{oetje,eeting}s,
>
> Geert
>
