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

[Geert Uytterhoeven]

Hi Claudiu,

On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>

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