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@xxxxxxxxxxxxxx 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