| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdWdJ9jN9-cko2zSoqSS0acbwYB77aBWvenJHMrFTXhdWg@mail.gmail.com>
Date: Fri, 16 Feb 2024 15:08:59 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: mturquette@...libre.com, sboyd@...nel.org, robh@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, magnus.damm@...il.com,
paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 07/17] clk: renesas: rzg2l: Extend power domain support
Hi Claudiu,
On Thu, Feb 8, 2024 at 1:44 PM Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...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@...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@...ux-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
Powered by blists - more mailing lists