| 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: <CAPDyKFpTgAmLBq2ZExPoxWM0wL756zH96vW7M6wHSA1MTTG1wA@mail.gmail.com>
Date: Thu, 19 Jun 2025 12:04:21 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Kevin Hilman <khilman@...libre.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, devicetree@...r.kernel.org,
linux-pm@...r.kernel.org, arm-scmi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v3 2/2] pmdomain: core: add support for subdomains
using power-domain-map
[...]
> I've done an implementation with struct device_node *. This works
> better (IMO) than struct of_phandle_args * because the caller (in my
> case scmi_pm_domain.c) already has device nodes, but not phandle args.
>
> The result will be that the pmdomain helper will call
> pm_genpd_add_subdomain() instead of of_genpd_add_subdomain().
>
> Below[1] is the current working version, which includes adding the
> helper to the PM domain core and showing the usage by the SCMI provider.
>
> How does this look?
It's a lot better in my opinion. Although, I have a few comments below.
>
> Note that doing this at provider creation time instead of
> <genpd>->attach_dev() time will require some changes to
> of_parse_phandle_with_args_map() because that function expects to be
> called for a device that has a `power-domains = <provider>` property,
> not for the provider itself. But I have it working with some local
> changes to make that helper work if called for the provider directly.
> If you're OK with the PM domains approach, I'll post another rev of this
> series which includes the OF changes for review by DT maintainers.
>
> Kevin
>
> [1]
> ---
> drivers/pmdomain/arm/scmi_pm_domain.c | 12 ++++++++--
> drivers/pmdomain/core.c | 34 +++++++++++++++++++++++++++
> include/linux/pm_domain.h | 11 ++++++++-
> 3 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index a7784a8bb5db..8197447e9d17 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -54,7 +54,7 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
>
> static int scmi_pm_domain_probe(struct scmi_device *sdev)
> {
> - int num_domains, i;
> + int num_domains, i, ret;
> struct device *dev = &sdev->dev;
> struct device_node *np = dev->of_node;
> struct scmi_pm_domain *scmi_pd;
> @@ -115,7 +115,15 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>
> dev_set_drvdata(dev, scmi_pd_data);
>
> - return of_genpd_add_provider_onecell(np, scmi_pd_data);
> + ret = of_genpd_add_provider_onecell(np, scmi_pd_data);
> + if (ret)
> + return ret;
> +
> + /* check for (optional) subdomain mapping with power-domain-map */
> + for (i = 0; i < num_domains; i++, scmi_pd++)
> + of_genpd_add_subdomain_map(np, domains[i], i);
> +
> + return ret;
> }
>
> static void scmi_pm_domain_remove(struct scmi_device *sdev)
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 88819659df83..3ede4baa4bee 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -3220,6 +3220,40 @@ int of_genpd_parse_idle_states(struct device_node *dn,
> }
> EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +int of_genpd_add_subdomain_map(struct device_node *np,
> + struct generic_pm_domain *domain,
> + int index)
Providing the struct generic_pm_domain *domain as an in-parameter for
the child-domain seems unnecessary and limiting to me.
Instead I think we should parse the power-domain-map DT property at
'index', to find the corresponding child-domain's specifier/index and
its corresponding parent-domain.
In other words, we don't need the struct generic_pm_domain *domain as
an in-parameter, right?
> +{
> + struct of_phandle_args parent_args;
> + struct generic_pm_domain *parent_pd;
> + struct device *dev = &domain->dev;
> + int ret;
> +
> + if (!domain)
> + return -ENODEV;
> +
> + /*
> + * Check for power-domain-map, which implies the primary
> + * power-doamin is a subdomain of the parent found in the map.
> + */
> + ret = of_parse_phandle_with_args_map(np, NULL, "power-domain",
> + index, &parent_args);
> + if (!ret && parent_args.np) {
> + parent_pd = genpd_get_from_provider(&parent_args);
> + of_node_put(parent_args.np);
> +
> + if (IS_ERR(parent_pd))
> + return -EINVAL;
> +
> + ret = pm_genpd_add_subdomain(parent_pd, domain);
> + if (!ret)
> + dev_dbg(dev, "adding PM domain %s as subdomain of %s\n",
> + domain->name, parent_pd->name);
> + }
> +
> + return ret;
> +}
> +
> static int __init genpd_bus_init(void)
> {
> return bus_register(&genpd_bus_type);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index cf4b11be3709..65d459d703bb 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -402,9 +402,11 @@ int of_genpd_add_subdomain(const struct of_phandle_args *parent_spec,
> int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
> const struct of_phandle_args *subdomain_spec);
> struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
> +int of_genpd_add_subdomain_map(struct device_node *np,
> + struct generic_pm_domain *genpd,
> + int index);
> int of_genpd_parse_idle_states(struct device_node *dn,
> struct genpd_power_state **states, int *n);
> -
> int genpd_dev_pm_attach(struct device *dev);
> struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> unsigned int index);
> @@ -443,6 +445,13 @@ static inline int of_genpd_remove_subdomain(const struct of_phandle_args *parent
> return -ENODEV;
> }
>
> +static int of_genpd_add_subdomain_map(struct device_node *np,
> + struct generic_pm_domain *genpd,
> + int index)
> +{
> + return -ENODEV;
> +}
> +
> static inline int of_genpd_parse_idle_states(struct device_node *dn,
> struct genpd_power_state **states, int *n)
> {
> --
> 2.49.0
>
>
Kind regards
Uffe
Powered by blists - more mailing lists