[07/17] clk: renesas: rzg2l: Extend power domain support

Message ID 20240208124300.2740313-8-claudiu.beznea.uj@bp.renesas.com
State New
Headers
Series clk: renesas: rzg2l: Add support for power domains |

Commit Message

claudiu beznea Feb. 8, 2024, 12:42 p.m. UTC
  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

RZ/{G2L, V2L, G3S}-based CPG versions have support for saving extra
power when clocks are disabled by activating module standby. This is done
through MSTOP-specific registers that are part of CPG. Each individual
module has one or more bits associated with one MSTOP register (see table
"Registers for Module Standby Mode" from HW manuals). Hardware manual
associates modules' clocks with one or more MSTOP bits. There are 3 mappings
available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):

case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, ..., Y})

Case 3 has been currently identified on RZ/V2L for the VCPL4 module.

To cover all three cases, the individual platform drivers will provide to
clock driver MSTOP register offset and associated bits in this register
as a bitmask and the clock driver will apply this bitmask to proper
MSTOP register.

Apart from MSTOP support, RZ/G3S can save more power by powering down the
individual IPs (after MSTOP has been set) if proper bits in
CPG_PWRDN_IP{1,2} registers are set.

The MSTOP and IP power down support were implemented through power
domains. Platform-specific clock drivers will register an array of
type struct rzg2l_cpg_pm_domain_init_data, which will be used to
instantiate properly the power domains.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/clk/renesas/rzg2l-cpg.c | 227 ++++++++++++++++++++++++++++++--
 drivers/clk/renesas/rzg2l-cpg.h |  68 ++++++++++
 2 files changed, 281 insertions(+), 14 deletions(-)
  

Comments

Geert Uytterhoeven Feb. 16, 2024, 2:08 p.m. UTC | #1
Hi Claudiu,

On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> RZ/{G2L, V2L, G3S}-based CPG versions have support for saving extra
> power when clocks are disabled by activating module standby. This is done
> through MSTOP-specific registers that are part of CPG. Each individual
> module has one or more bits associated with one MSTOP register (see table
> "Registers for Module Standby Mode" from HW manuals). Hardware manual
> associates modules' clocks with one or more MSTOP bits. There are 3 mappings
> available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):
>
> case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
> case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
> case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, .., Y})
>
> Case 3 has been currently identified on RZ/V2L for the VCPL4 module.
>
> To cover all three cases, the individual platform drivers will provide to
> clock driver MSTOP register offset and associated bits in this register
> as a bitmask and the clock driver will apply this bitmask to proper
> MSTOP register.
>
> Apart from MSTOP support, RZ/G3S can save more power by powering down the
> individual IPs (after MSTOP has been set) if proper bits in
> CPG_PWRDN_IP{1,2} registers are set.
>
> The MSTOP and IP power down support were implemented through power
> domains. Platform-specific clock drivers will register an array of
> type struct rzg2l_cpg_pm_domain_init_data, which will be used to
> instantiate properly the power domains.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -1559,9 +1556,34 @@ static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
>         return true;
>  }
>
> +/**
> + * struct rzg2l_cpg_pm_domain - RZ/G2L PM domains data structure
> + * @domains: generic PM domains
> + * @onecell_data: cell data
> + */
> +struct rzg2l_cpg_pm_domain {

rzg2l_cpg_pm_domains (plural)?

> +       struct generic_pm_domain **domains;
> +       struct genpd_onecell_data onecell_data;
> +};

Using a flexible array like

    struct rzg2l_cpg_pm_domains {
            struct genpd_onecell_data onecell_data;
            struct generic_pm_domain *domains[];
    };

would let you allocate the structure and the array in a single step,
using devm_kzalloc(..., struct_size(...), ...).

> +
> +/**
> + * struct rzg2l_cpg_pd - RZ/G2L power domain data structure
> + * @priv: pointer to CPG private data structure
> + * @genpd: generic PM domain
> + * @conf: CPG PM domain configuration info
> + * @id: RZ/G2L power domain ID
> + */
> +struct rzg2l_cpg_pd {
> +       struct rzg2l_cpg_priv *priv;
> +       struct generic_pm_domain genpd;

Please make genpd the first member, for simpler conversion between
rzg2l_cpg_pd and generic_pm_domain pointers.

> +       struct rzg2l_cpg_pm_domain_conf conf;
> +       u16 id;
> +};

> +static int __init rzg2l_cpg_add_pm_domains(struct rzg2l_cpg_priv *priv)
> +{
> +       const struct rzg2l_cpg_info *info = priv->info;
> +       struct device *dev = priv->dev;
> +       struct device_node *np = dev->of_node;
> +       struct rzg2l_cpg_pm_domain *domains;
> +       struct generic_pm_domain *parent;

Missing initialization parent = NULL;

> +       u32 ncells;
> +       int ret;
> +
> +       ret = of_property_read_u32(np, "#power-domain-cells", &ncells);
> +       if (ret)
> +               return ret;
> +
> +       /* For backward compatibility. */
> +       if (!ncells)
> +               return rzg2l_cpg_add_clk_domain(priv);
> +
> +       domains = devm_kzalloc(priv->dev, sizeof(*domains), GFP_KERNEL);
> +       if (!domains)
> +               return -ENOMEM;
> +
> +       domains->domains = devm_kcalloc(priv->dev, info->num_pm_domains,
> +                                       sizeof(struct generic_pm_domain *), GFP_KERNEL);
> +       if (!domains->domains)
> +               return -ENOMEM;
> +
> +       domains->onecell_data.domains = domains->domains;
> +       domains->onecell_data.num_domains = info->num_pm_domains;
> +       domains->onecell_data.xlate = rzg2l_cpg_pm_domain_xlate;
> +
> +       ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove, &domains->onecell_data);
> +       if (ret)
> +               return ret;
> +
> +       for (unsigned int i = 0; i < info->num_pm_domains; i++) {
> +               bool always_on = !!(info->pm_domains[i].flags & RZG2L_PD_F_ALWAYS_ON);
> +               struct rzg2l_cpg_pd *pd;
> +
> +               pd = devm_kzalloc(priv->dev, sizeof(*pd), GFP_KERNEL);
> +               if (!pd)
> +                       return -ENOMEM;
> +
> +               pd->genpd.name = info->pm_domains[i].name;
> +               pd->conf = info->pm_domains[i].conf;
> +               pd->id = info->pm_domains[i].id;
> +               pd->priv = priv;
> +
> +               ret = rzg2l_cpg_pd_setup(pd, always_on);
> +               if (ret)
> +                       return ret;
> +
> +               if (always_on) {
> +                       ret = rzg2l_cpg_power_on(&pd->genpd);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               domains->domains[i] = &pd->genpd;
> +               /* Parent should be on the very first entry of info->pm_domains[]. */
> +               if (info->pm_domains[i].flags & RZG2L_PD_F_PARENT) {
> +                       parent = &pd->genpd;
> +                       continue;
> +               }
> +
> +               ret = pm_genpd_add_subdomain(parent, &pd->genpd);
> +               if (ret)
> +                       return ret;

I think you can simplify/generalize the above logic without needing
the RZG2L_PD_F_PARENT flag:

    if (i) {
            ret = pm_genpd_add_subdomain(domains->domains[0], &pd->genpd);
            if (ret)
                    return ret;
    }

> +       }
> +
> +       ret = of_genpd_add_provider_onecell(np, &domains->onecell_data);
> +       if (ret)
> +               return ret;
> +
> +       /* 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);
> +
> +       return 0;
>  }
>
>  static int __init rzg2l_cpg_probe(struct platform_device *pdev)

> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -27,6 +27,16 @@
>  #define CPG_PL6_ETH_SSEL       (0x418)
>  #define CPG_PL5_SDIV           (0x420)
>  #define CPG_RST_MON            (0x680)
> +#define CPG_ACPU_MSTOP         (0xB60)
> +#define CPG_MCPU2_MSTOP                (0xB68)
> +#define CPG_PERI_COM_MSTOP     (0xB6C)
> +#define CPG_PERI_CPU_MSTOP     (0xB70)
> +#define CPG_PERI_DDR_MSTOP     (0xB74)
> +#define CPG_REG1_MSTOP         (0xB80)
> +#define CPG_TZCDDR_MSTOP       (0xB84)
> +#define CPG_PWRDN_IP1          (0xBB0)
> +#define CPG_PWRDN_IP2          (0xBB4)
> +#define CPG_PWRDN_MSTOP                (0xBC0)

Please name these CPG_BUS_*, to match the documentation.

>  #define CPG_OTHERFUNC1_REG     (0xBE8)
>
>  #define CPG_SIPLL5_STBY_RESETB         BIT(0)

> @@ -234,6 +246,54 @@ struct rzg2l_reset {
>  #define DEF_RST(_id, _off, _bit)       \
>         DEF_RST_MON(_id, _off, _bit, -1)
>
> +/**
> + * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
> + * @mstop: MSTOP configuration (MSB = register offset, LSB = bitmask)
> + * @pwrdn: PWRDN configuration (MSB = register offset, LSB = register bit)
> + */
> +struct rzg2l_cpg_pm_domain_conf {
> +       u32 mstop;
> +       u32 pwrdn;

Why not

    u16 mstop_off;
    u16 mstop_mask;
    u16 pwrdn_off;
    u16 pwrdn_mask;

so you can drop the MSTOP*() and PWRDN*() macros below?

> +};
> +
> +/**
> + * struct rzg2l_cpg_pm_domain_init_data - PM domain init data
> + * @name: PM domain name
> + * @conf: PM domain configuration
> + * @flags: RZG2L PM domain flags (see RZG2L_PD_F_*)
> + * @id: PM domain ID (similar to the ones defined in
> + *      include/dt-bindings/clock/<soc-id>-cpg.h)
> + */
> +struct rzg2l_cpg_pm_domain_init_data {
> +       const char * const name;
> +       struct rzg2l_cpg_pm_domain_conf conf;
> +       u32 flags;

With a single flag left, this can become "bool always_on"
(and be moved after "id" to improve packing).

> +       u16 id;
> +};
> +
> +#define DEF_PD(_name, _id, _mstop_conf, _pwrdn_conf, _flags) \
> +       { \
> +               .name = (_name), \
> +               .id = (_id), \
> +               .conf = { \
> +                       .mstop = (_mstop_conf), \
> +                       .pwrdn = (_pwrdn_conf), \
> +               }, \
> +               .flags = (_flags), \
> +       }
> +
> +#define MSTOP(name, bitmask)   ((CPG_##name##_MSTOP) << 16 | (bitmask))
> +#define MSTOP_OFF(conf)                ((conf) >> 16)
> +#define MSTOP_MASK(conf)       ((conf) & GENMASK(15, 0))
> +
> +#define PWRDN(name, bit)       ((CPG_PWRDN_##name) << 16 | BIT(bit))
> +#define PWRDN_OFF(conf)                ((conf) >> 16)
> +#define PWRDN_MASK(conf)       ((conf) & GENMASK(15, 0))
> +
> +/* Power domain flags. */
> +#define RZG2L_PD_F_PARENT      BIT(0)
> +#define RZG2L_PD_F_ALWAYS_ON   BIT(1)
> +
>  /**
>   * struct rzg2l_cpg_info - SoC-specific CPG Description
>   *

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
  
claudiu beznea Feb. 19, 2024, 8:24 a.m. UTC | #2
On 16.02.2024 16:08, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> RZ/{G2L, V2L, G3S}-based CPG versions have support for saving extra
>> power when clocks are disabled by activating module standby. This is done
>> through MSTOP-specific registers that are part of CPG. Each individual
>> module has one or more bits associated with one MSTOP register (see table
>> "Registers for Module Standby Mode" from HW manuals). Hardware manual
>> associates modules' clocks with one or more MSTOP bits. There are 3 mappings
>> available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):
>>
>> case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
>> case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
>> case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, ..., Y})
>>
>> Case 3 has been currently identified on RZ/V2L for the VCPL4 module.
>>
>> To cover all three cases, the individual platform drivers will provide to
>> clock driver MSTOP register offset and associated bits in this register
>> as a bitmask and the clock driver will apply this bitmask to proper
>> MSTOP register.
>>
>> Apart from MSTOP support, RZ/G3S can save more power by powering down the
>> individual IPs (after MSTOP has been set) if proper bits in
>> CPG_PWRDN_IP{1,2} registers are set.
>>
>> The MSTOP and IP power down support were implemented through power
>> domains. Platform-specific clock drivers will register an array of
>> type struct rzg2l_cpg_pm_domain_init_data, which will be used to
>> instantiate properly the power domains.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>> @@ -1559,9 +1556,34 @@ static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
>>         return true;
>>  }
[ ... ]

> 
>> @@ -234,6 +246,54 @@ struct rzg2l_reset {
>>  #define DEF_RST(_id, _off, _bit)       \
>>         DEF_RST_MON(_id, _off, _bit, -1)
>>
>> +/**
>> + * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
>> + * @mstop: MSTOP configuration (MSB = register offset, LSB = bitmask)
>> + * @pwrdn: PWRDN configuration (MSB = register offset, LSB = register bit)
>> + */
>> +struct rzg2l_cpg_pm_domain_conf {
>> +       u32 mstop;
>> +       u32 pwrdn;
> 
> Why not
> 
>     u16 mstop_off;
>     u16 mstop_mask;
>     u16 pwrdn_off;
>     u16 pwrdn_mask;
> 
> so you can drop the MSTOP*() and PWRDN*() macros below?

I did it like this to align with the already existing approach for this
kind of things available in this driver. I can do it as you proposed.

For the rest of your comments on this patch: I agree and will adjust the
patch in the next version.

Thank you,
Claudiu Beznea
  
Geert Uytterhoeven Feb. 19, 2024, 8:48 a.m. UTC | #3
Hi Claudiu,

On Mon, Feb 19, 2024 at 9:24 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
> On 16.02.2024 16:08, Geert Uytterhoeven wrote:
> > On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> RZ/{G2L, V2L, G3S}-based CPG versions have support for saving extra
> >> power when clocks are disabled by activating module standby. This is done
> >> through MSTOP-specific registers that are part of CPG. Each individual
> >> module has one or more bits associated with one MSTOP register (see table
> >> "Registers for Module Standby Mode" from HW manuals). Hardware manual
> >> associates modules' clocks with one or more MSTOP bits. There are 3 mappings
> >> available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):
> >>
> >> case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
> >> case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
> >> case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, ..., Y})
> >>
> >> Case 3 has been currently identified on RZ/V2L for the VCPL4 module.
> >>
> >> To cover all three cases, the individual platform drivers will provide to
> >> clock driver MSTOP register offset and associated bits in this register
> >> as a bitmask and the clock driver will apply this bitmask to proper
> >> MSTOP register.
> >>
> >> Apart from MSTOP support, RZ/G3S can save more power by powering down the
> >> individual IPs (after MSTOP has been set) if proper bits in
> >> CPG_PWRDN_IP{1,2} registers are set.
> >>
> >> The MSTOP and IP power down support were implemented through power
> >> domains. Platform-specific clock drivers will register an array of
> >> type struct rzg2l_cpg_pm_domain_init_data, which will be used to
> >> instantiate properly the power domains.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >> @@ -1559,9 +1556,34 @@ static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
> >>         return true;
> >>  }
> [ ... ]
>
> >
> >> @@ -234,6 +246,54 @@ struct rzg2l_reset {
> >>  #define DEF_RST(_id, _off, _bit)       \
> >>         DEF_RST_MON(_id, _off, _bit, -1)
> >>
> >> +/**
> >> + * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
> >> + * @mstop: MSTOP configuration (MSB = register offset, LSB = bitmask)
> >> + * @pwrdn: PWRDN configuration (MSB = register offset, LSB = register bit)
> >> + */
> >> +struct rzg2l_cpg_pm_domain_conf {
> >> +       u32 mstop;
> >> +       u32 pwrdn;
> >
> > Why not
> >
> >     u16 mstop_off;
> >     u16 mstop_mask;
> >     u16 pwrdn_off;
> >     u16 pwrdn_mask;
> >
> > so you can drop the MSTOP*() and PWRDN*() macros below?
>
> I did it like this to align with the already existing approach for this
> kind of things available in this driver. I can do it as you proposed.

The other fields do not align nicely with byte or word boundaries.

I can see the value of the MSTOP(name, bitmask) and
PWRDN(name, bitmask) macros, but I'd rather get rid of the *_MASK()
and *_OFF() variants.

> For the rest of your comments on this patch: I agree and will adjust the
> patch in the next version.

Thanks!

Gr{oetje,eeting}s,

                        Geert
  
claudiu beznea Feb. 19, 2024, 9:04 a.m. UTC | #4
Hi, Geert,

On 19.02.2024 10:48, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Mon, Feb 19, 2024 at 9:24 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 16.02.2024 16:08, Geert Uytterhoeven wrote:
>>> On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> RZ/{G2L, V2L, G3S}-based CPG versions have support for saving extra
>>>> power when clocks are disabled by activating module standby. This is done
>>>> through MSTOP-specific registers that are part of CPG. Each individual
>>>> module has one or more bits associated with one MSTOP register (see table
>>>> "Registers for Module Standby Mode" from HW manuals). Hardware manual
>>>> associates modules' clocks with one or more MSTOP bits. There are 3 mappings
>>>> available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):
>>>>
>>>> case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
>>>> case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
>>>> case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, ..., Y})
>>>>
>>>> Case 3 has been currently identified on RZ/V2L for the VCPL4 module.
>>>>
>>>> To cover all three cases, the individual platform drivers will provide to
>>>> clock driver MSTOP register offset and associated bits in this register
>>>> as a bitmask and the clock driver will apply this bitmask to proper
>>>> MSTOP register.
>>>>
>>>> Apart from MSTOP support, RZ/G3S can save more power by powering down the
>>>> individual IPs (after MSTOP has been set) if proper bits in
>>>> CPG_PWRDN_IP{1,2} registers are set.
>>>>
>>>> The MSTOP and IP power down support were implemented through power
>>>> domains. Platform-specific clock drivers will register an array of
>>>> type struct rzg2l_cpg_pm_domain_init_data, which will be used to
>>>> instantiate properly the power domains.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>>>> @@ -1559,9 +1556,34 @@ static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
>>>>         return true;
>>>>  }
>> [ ... ]
>>
>>>
>>>> @@ -234,6 +246,54 @@ struct rzg2l_reset {
>>>>  #define DEF_RST(_id, _off, _bit)       \
>>>>         DEF_RST_MON(_id, _off, _bit, -1)
>>>>
>>>> +/**
>>>> + * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
>>>> + * @mstop: MSTOP configuration (MSB = register offset, LSB = bitmask)
>>>> + * @pwrdn: PWRDN configuration (MSB = register offset, LSB = register bit)
>>>> + */
>>>> +struct rzg2l_cpg_pm_domain_conf {
>>>> +       u32 mstop;
>>>> +       u32 pwrdn;
>>>
>>> Why not
>>>
>>>     u16 mstop_off;
>>>     u16 mstop_mask;
>>>     u16 pwrdn_off;
>>>     u16 pwrdn_mask;
>>>
>>> so you can drop the MSTOP*() and PWRDN*() macros below?
>>
>> I did it like this to align with the already existing approach for this
>> kind of things available in this driver. I can do it as you proposed.
> 
> The other fields do not align nicely with byte or word boundaries.
> 
> I can see the value of the MSTOP(name, bitmask) and
> PWRDN(name, bitmask) macros, but I'd rather get rid of the *_MASK()
> and *_OFF() variants.

Sure, I'll do proper adjustments in the next version.

Thank you,
Claudiu Beznea

> 
>> For the rest of your comments on this patch: I agree and will adjust the
>> patch in the next version.
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
  
Geert Uytterhoeven Feb. 20, 2024, 7:32 p.m. UTC | #5
Hi Claudiu,

On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c

> +static struct generic_pm_domain *
> +rzg2l_cpg_pm_domain_xlate(struct of_phandle_args *spec, void *data)

As of commit 4d0824608a636b64 ("pmdomain: core: constify of_phandle_args
in xlate") in next-20240215 and later the first parameter needs to
be const.

Gr{oetje,eeting}s,

                        Geert
  
claudiu beznea Feb. 21, 2024, 6:14 a.m. UTC | #6
Hi, Geert,

On 20.02.2024 21:32, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
>> +static struct generic_pm_domain *
>> +rzg2l_cpg_pm_domain_xlate(struct of_phandle_args *spec, void *data)
> 
> As of commit 4d0824608a636b64 ("pmdomain: core: constify of_phandle_args
> in xlate") in next-20240215 and later the first parameter needs to
> be const.

Indeed, I noticed that. Thank you for pointing it.


> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
  

Patch

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 3d2daa4ba2a4..3a7168c314c2 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -139,7 +139,6 @@  struct rzg2l_pll5_mux_dsi_div_param {
  * @num_resets: Number of Module Resets in info->resets[]
  * @last_dt_core_clk: ID of the last Core Clock exported to DT
  * @info: Pointer to platform data
- * @genpd: PM domain
  * @mux_dsi_div_params: pll5 mux and dsi div parameters
  */
 struct rzg2l_cpg_priv {
@@ -156,8 +155,6 @@  struct rzg2l_cpg_priv {
 
 	const struct rzg2l_cpg_info *info;
 
-	struct generic_pm_domain genpd;
-
 	struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
 };
 
@@ -1559,9 +1556,34 @@  static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
 	return true;
 }
 
+/**
+ * struct rzg2l_cpg_pm_domain - RZ/G2L PM domains data structure
+ * @domains: generic PM domains
+ * @onecell_data: cell data
+ */
+struct rzg2l_cpg_pm_domain {
+	struct generic_pm_domain **domains;
+	struct genpd_onecell_data onecell_data;
+};
+
+/**
+ * struct rzg2l_cpg_pd - RZ/G2L power domain data structure
+ * @priv: pointer to CPG private data structure
+ * @genpd: generic PM domain
+ * @conf: CPG PM domain configuration info
+ * @id: RZ/G2L power domain ID
+ */
+struct rzg2l_cpg_pd {
+	struct rzg2l_cpg_priv *priv;
+	struct generic_pm_domain genpd;
+	struct rzg2l_cpg_pm_domain_conf conf;
+	u16 id;
+};
+
 static int rzg2l_cpg_attach_dev(struct generic_pm_domain *domain, struct device *dev)
 {
-	struct rzg2l_cpg_priv *priv = container_of(domain, struct rzg2l_cpg_priv, genpd);
+	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
+	struct rzg2l_cpg_priv *priv = pd->priv;
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args clkspec;
 	bool once = true;
@@ -1617,31 +1639,208 @@  static void rzg2l_cpg_detach_dev(struct generic_pm_domain *unused, struct device
 }
 
 static void rzg2l_cpg_genpd_remove(void *data)
+{
+	struct genpd_onecell_data *celldata = data;
+
+	for (unsigned int i = 0; i < celldata->num_domains; i++)
+		pm_genpd_remove(celldata->domains[i]);
+}
+
+static void rzg2l_cpg_genpd_remove_simple(void *data)
 {
 	pm_genpd_remove(data);
 }
 
+static int rzg2l_cpg_power_on(struct generic_pm_domain *domain)
+{
+	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
+	struct rzg2l_cpg_priv *priv = pd->priv;
+	u32 off, mask;
+
+	/* Set PWRDN. */
+	if (pd->conf.pwrdn) {
+		off = PWRDN_OFF(pd->conf.pwrdn);
+		mask = PWRDN_MASK(pd->conf.pwrdn) << 16;
+		writel(mask, priv->base + off);
+	}
+
+	/* Set MSTOP. */
+	if (pd->conf.mstop) {
+		off = MSTOP_OFF(pd->conf.mstop);
+		mask = MSTOP_MASK(pd->conf.mstop) << 16;
+		writel(mask, priv->base + off);
+	}
+
+	return 0;
+}
+
+static int rzg2l_cpg_power_off(struct generic_pm_domain *domain)
+{
+	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
+	struct rzg2l_cpg_priv *priv = pd->priv;
+	u32 off, mask;
+
+	/* Set MSTOP. */
+	if (pd->conf.mstop) {
+		off = MSTOP_OFF(pd->conf.mstop);
+		mask = MSTOP_MASK(pd->conf.mstop);
+		writel(mask | (mask << 16), priv->base + off);
+	}
+
+	/* Set PWRDN. */
+	if (pd->conf.pwrdn) {
+		off = PWRDN_OFF(pd->conf.pwrdn);
+		mask = PWRDN_MASK(pd->conf.pwrdn);
+		writel(mask | (mask << 16), priv->base + off);
+	}
+
+	return 0;
+}
+
+static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd, bool always_on)
+{
+	struct dev_power_governor *governor;
+
+	pd->genpd.flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
+	pd->genpd.attach_dev = rzg2l_cpg_attach_dev;
+	pd->genpd.detach_dev = rzg2l_cpg_detach_dev;
+	if (always_on) {
+		pd->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
+		governor = &pm_domain_always_on_gov;
+	} else {
+		pd->genpd.power_on = rzg2l_cpg_power_on;
+		pd->genpd.power_off = rzg2l_cpg_power_off;
+		governor = &simple_qos_governor;
+	}
+
+	return pm_genpd_init(&pd->genpd, governor, false);
+}
+
 static int __init rzg2l_cpg_add_clk_domain(struct rzg2l_cpg_priv *priv)
 {
 	struct device *dev = priv->dev;
 	struct device_node *np = dev->of_node;
-	struct generic_pm_domain *genpd = &priv->genpd;
+	struct rzg2l_cpg_pd *pd;
 	int ret;
 
-	genpd->name = np->name;
-	genpd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ALWAYS_ON |
-		       GENPD_FLAG_ACTIVE_WAKEUP;
-	genpd->attach_dev = rzg2l_cpg_attach_dev;
-	genpd->detach_dev = rzg2l_cpg_detach_dev;
-	ret = pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
+	pd = devm_kzalloc(priv->dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pd->genpd.name = np->name;
+	pd->priv = priv;
+	ret = rzg2l_cpg_pd_setup(pd, true);
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove, genpd);
+	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove_simple, &pd->genpd);
 	if (ret)
 		return ret;
 
-	return of_genpd_add_provider_simple(np, genpd);
+	return of_genpd_add_provider_simple(np, &pd->genpd);
+}
+
+static struct generic_pm_domain *
+rzg2l_cpg_pm_domain_xlate(struct of_phandle_args *spec, void *data)
+{
+	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+	struct genpd_onecell_data *genpd = data;
+
+	if (spec->args_count != 1)
+		return ERR_PTR(-EINVAL);
+
+	for (unsigned int i = 0; i < genpd->num_domains; i++) {
+		struct rzg2l_cpg_pd *pd = container_of(genpd->domains[i], struct rzg2l_cpg_pd,
+						       genpd);
+
+		if (pd->id == spec->args[0]) {
+			domain = &pd->genpd;
+			break;
+		}
+	}
+
+	return domain;
+}
+
+static int __init rzg2l_cpg_add_pm_domains(struct rzg2l_cpg_priv *priv)
+{
+	const struct rzg2l_cpg_info *info = priv->info;
+	struct device *dev = priv->dev;
+	struct device_node *np = dev->of_node;
+	struct rzg2l_cpg_pm_domain *domains;
+	struct generic_pm_domain *parent;
+	u32 ncells;
+	int ret;
+
+	ret = of_property_read_u32(np, "#power-domain-cells", &ncells);
+	if (ret)
+		return ret;
+
+	/* For backward compatibility. */
+	if (!ncells)
+		return rzg2l_cpg_add_clk_domain(priv);
+
+	domains = devm_kzalloc(priv->dev, sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	domains->domains = devm_kcalloc(priv->dev, info->num_pm_domains,
+					sizeof(struct generic_pm_domain *), GFP_KERNEL);
+	if (!domains->domains)
+		return -ENOMEM;
+
+	domains->onecell_data.domains = domains->domains;
+	domains->onecell_data.num_domains = info->num_pm_domains;
+	domains->onecell_data.xlate = rzg2l_cpg_pm_domain_xlate;
+
+	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove, &domains->onecell_data);
+	if (ret)
+		return ret;
+
+	for (unsigned int i = 0; i < info->num_pm_domains; i++) {
+		bool always_on = !!(info->pm_domains[i].flags & RZG2L_PD_F_ALWAYS_ON);
+		struct rzg2l_cpg_pd *pd;
+
+		pd = devm_kzalloc(priv->dev, sizeof(*pd), GFP_KERNEL);
+		if (!pd)
+			return -ENOMEM;
+
+		pd->genpd.name = info->pm_domains[i].name;
+		pd->conf = info->pm_domains[i].conf;
+		pd->id = info->pm_domains[i].id;
+		pd->priv = priv;
+
+		ret = rzg2l_cpg_pd_setup(pd, always_on);
+		if (ret)
+			return ret;
+
+		if (always_on) {
+			ret = rzg2l_cpg_power_on(&pd->genpd);
+			if (ret)
+				return ret;
+		}
+
+		domains->domains[i] = &pd->genpd;
+		/* Parent should be on the very first entry of info->pm_domains[]. */
+		if (info->pm_domains[i].flags & RZG2L_PD_F_PARENT) {
+			parent = &pd->genpd;
+			continue;
+		}
+
+		ret = pm_genpd_add_subdomain(parent, &pd->genpd);
+		if (ret)
+			return ret;
+	}
+
+	ret = of_genpd_add_provider_onecell(np, &domains->onecell_data);
+	if (ret)
+		return ret;
+
+	/* 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);
+
+	return 0;
 }
 
 static int __init rzg2l_cpg_probe(struct platform_device *pdev)
@@ -1697,7 +1896,7 @@  static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
-	error = rzg2l_cpg_add_clk_domain(priv);
+	error = rzg2l_cpg_add_pm_domains(priv);
 	if (error)
 		return error;
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 6e38c8fc888c..00d12b04ba2f 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -27,6 +27,16 @@ 
 #define CPG_PL6_ETH_SSEL	(0x418)
 #define CPG_PL5_SDIV		(0x420)
 #define CPG_RST_MON		(0x680)
+#define CPG_ACPU_MSTOP		(0xB60)
+#define CPG_MCPU2_MSTOP		(0xB68)
+#define CPG_PERI_COM_MSTOP	(0xB6C)
+#define CPG_PERI_CPU_MSTOP	(0xB70)
+#define CPG_PERI_DDR_MSTOP	(0xB74)
+#define CPG_REG1_MSTOP		(0xB80)
+#define CPG_TZCDDR_MSTOP	(0xB84)
+#define CPG_PWRDN_IP1		(0xBB0)
+#define CPG_PWRDN_IP2		(0xBB4)
+#define CPG_PWRDN_MSTOP		(0xBC0)
 #define CPG_OTHERFUNC1_REG	(0xBE8)
 
 #define CPG_SIPLL5_STBY_RESETB		BIT(0)
@@ -70,6 +80,8 @@ 
 
 #define EXTAL_FREQ_IN_MEGA_HZ	(24)
 
+#define CPG_PWRDN_MSTOP_ENABLE		(BIT(16) | BIT(0))
+
 /**
  * Definitions of CPG Core Clocks
  *
@@ -234,6 +246,54 @@  struct rzg2l_reset {
 #define DEF_RST(_id, _off, _bit)	\
 	DEF_RST_MON(_id, _off, _bit, -1)
 
+/**
+ * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
+ * @mstop: MSTOP configuration (MSB = register offset, LSB = bitmask)
+ * @pwrdn: PWRDN configuration (MSB = register offset, LSB = register bit)
+ */
+struct rzg2l_cpg_pm_domain_conf {
+	u32 mstop;
+	u32 pwrdn;
+};
+
+/**
+ * struct rzg2l_cpg_pm_domain_init_data - PM domain init data
+ * @name: PM domain name
+ * @conf: PM domain configuration
+ * @flags: RZG2L PM domain flags (see RZG2L_PD_F_*)
+ * @id: PM domain ID (similar to the ones defined in
+ *      include/dt-bindings/clock/<soc-id>-cpg.h)
+ */
+struct rzg2l_cpg_pm_domain_init_data {
+	const char * const name;
+	struct rzg2l_cpg_pm_domain_conf conf;
+	u32 flags;
+	u16 id;
+};
+
+#define DEF_PD(_name, _id, _mstop_conf, _pwrdn_conf, _flags) \
+	{ \
+		.name = (_name), \
+		.id = (_id), \
+		.conf = { \
+			.mstop = (_mstop_conf), \
+			.pwrdn = (_pwrdn_conf), \
+		}, \
+		.flags = (_flags), \
+	}
+
+#define MSTOP(name, bitmask)	((CPG_##name##_MSTOP) << 16 | (bitmask))
+#define MSTOP_OFF(conf)		((conf) >> 16)
+#define MSTOP_MASK(conf)	((conf) & GENMASK(15, 0))
+
+#define PWRDN(name, bit)	((CPG_PWRDN_##name) << 16 | BIT(bit))
+#define PWRDN_OFF(conf)		((conf) >> 16)
+#define PWRDN_MASK(conf)	((conf) & GENMASK(15, 0))
+
+/* Power domain flags. */
+#define RZG2L_PD_F_PARENT	BIT(0)
+#define RZG2L_PD_F_ALWAYS_ON	BIT(1)
+
 /**
  * struct rzg2l_cpg_info - SoC-specific CPG Description
  *
@@ -252,6 +312,9 @@  struct rzg2l_reset {
  * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
  *                 should not be disabled without a knowledgeable driver
  * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
+ * @pm_domains: PM domains init data array
+ * @num_pm_domains: Number of PM domains
+ * @pm_domain_pwrdn_mstop: Specifies if PWRDN MSTOP is supported
  * @has_clk_mon_regs: Flag indicating whether the SoC has CLK_MON registers
  */
 struct rzg2l_cpg_info {
@@ -278,6 +341,11 @@  struct rzg2l_cpg_info {
 	const unsigned int *crit_mod_clks;
 	unsigned int num_crit_mod_clks;
 
+	/* Power domain. */
+	const struct rzg2l_cpg_pm_domain_init_data *pm_domains;
+	unsigned int num_pm_domains;
+	bool pm_domain_pwrdn_mstop;
+
 	bool has_clk_mon_regs;
 };