Message-ID: <20250530104412.cc73ewso5ydjhmsk@lcpd911>
Date: Fri, 30 May 2025 16:14:12 +0530
From: Dhruva Gole <d-gole@...com>
To: Cristian Marussi <cristian.marussi@....com>,
        Sudeep Holla
	<sudeep.holla@....com>
CC: Ulf Hansson <ulf.hansson@...aro.org>, <arm-scmi@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-pm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Vignesh Raghavendra
	<vigneshr@...com>, <kamlesh@...com>
Subject: Re: [RFC PATCH] pmdomain: arm: scmi_pm_domain: Do lazy init as part
 of probe

On May 30, 2025 at 16:05:27 +0530, Dhruva Gole wrote:
> Optimize the SCMI power domain driver to only initialize domains that are
> actually referenced in the device tree. Previously, the driver would
> initialize all possible domains up to the maximum ID, which could lead to
> unnecessary firmware calls and longer probe times.
> 
> Key changes:
> - Scan device tree to identify which power domains are actually referenced
> - Use bitmap to track needed domains instead of initializing all
> - Only perform state queries and initialization for referenced domains
> - Maintain proper array sizing for power domain framework compatibility
> - Keep full provider structure to support late binding
> 
> This optimization reduces probe time and unnecessary firmware interactions
> by only initializing power domains that are actually used in the system.
> For example, in a system with 100 possible domains but only 3 referenced
> in the device tree, we now only initialize those 3 domains instead of
> all 100.
> 
> Signed-off-by: Dhruva Gole <d-gole@...com>
> ---


Adding Sudeep and a few others who I missed adding initially.

> 
> Hi all,
> The approach of doing a lazy init was briefly proposed in this [1] 2024
> Embedded Open Source talk. It was also brought up in the monthly ARM
> SCMI meetings that take place and it didn't recieve too much opposition.
> 
> This greatly helps to improve the boot time, and I have some data to
> back this up as well. This[2] experiment was done on a TI AM62L SoC
> (which is yet to make it upstream) to measure the time taken in the scmi
> pm domain probe function when it does a full 0..N scmi pd init vs just
> the ones being used in the device tree.
> 
> If you have any feedback on this, please let me know. If not, I will go
> ahead and post a "non-RFC" patch assuming everyone is mostly on board
> with this. Also request other SCMI consumers to test this out as much as
> possible to see if it breaks in any situations.
> 
> [1] https://static.sched.com/hosted_files/eoss24/2f/ARM_SCMI_Primer_Dhruva_kamlesh.pdf
> [2] https://gist.github.com/DhruvaG2000/75d6a4c31e817f56587537b022761c45
> 
> Applies cleanly on top of next-20250530, built and tested on top of the
> same
> 
> ---
> 
>  drivers/pmdomain/arm/scmi_pm_domain.c | 88 ++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index 2a213c218126..8acbfe249ed2 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -10,6 +10,8 @@
>  #include <linux/module.h>
>  #include <linux/pm_domain.h>
>  #include <linux/scmi_protocol.h>
> +#include <linux/of.h>
> +
>  
>  static const struct scmi_power_proto_ops *power_ops;
>  
> @@ -47,14 +49,20 @@ 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;
>  	struct device *dev = &sdev->dev;
> -	struct device_node *np = dev->of_node;
> +	struct device_node *np;
>  	struct scmi_pm_domain *scmi_pd;
> -	struct genpd_onecell_data *scmi_pd_data;
> -	struct generic_pm_domain **domains;
> +	struct of_phandle_args args;
>  	const struct scmi_handle *handle = sdev->handle;
>  	struct scmi_protocol_handle *ph;
> +	struct genpd_onecell_data *scmi_pd_data;
> +	struct generic_pm_domain **domains;
> +	int max_id = -1;
> +	int index, num_domains;
> +	ktime_t start_time = ktime_get();
> +	unsigned long *domain_ids;
> +
> +	dev_err(dev, "Starting optimized SCMI power domain probe\n");
>  
>  	if (!handle)
>  		return -ENODEV;
> @@ -69,54 +77,90 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>  		return num_domains;
>  	}
>  
> -	scmi_pd = devm_kcalloc(dev, num_domains, sizeof(*scmi_pd), GFP_KERNEL);
> -	if (!scmi_pd)
> +	domain_ids = devm_bitmap_zalloc(dev, num_domains, GFP_KERNEL);
> +	if (!domain_ids)
>  		return -ENOMEM;
>  
> +	/* Find referenced domain IDs and mark them in bitmap */
> +	for_each_node_with_property(np, "power-domains") {
> +		index = 0;
> +		while (!of_parse_phandle_with_args(np, "power-domains",
> +					"#power-domain-cells",
> +					index, &args)) {
> +			if (args.args_count >= 1 && args.np == dev->of_node) {
> +				int id = args.args[0];
> +				if (id < num_domains) {
> +					set_bit(id, domain_ids);
> +					max_id = max(max_id, id);
> +					dev_dbg(dev, "Found power domain reference %d from node %pOF\n",
> +							id, np);
> +				}
> +			}
> +			of_node_put(args.np);
> +			index++;
> +		}
> +	}
> +
> +	if (max_id < 0) {
> +		dev_warn(dev, "No power domains referenced in device tree\n");
> +		/* Create provider anyway as domains might be referenced later */
> +		max_id = 0;
> +	}
> +
> +	dev_warn(dev, "Highest referenced domain ID: %d\n", max_id);
> +
>  	scmi_pd_data = devm_kzalloc(dev, sizeof(*scmi_pd_data), GFP_KERNEL);
>  	if (!scmi_pd_data)
>  		return -ENOMEM;
>  
> -	domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL);
> +	domains = devm_kcalloc(dev, max_id + 1, sizeof(*domains), GFP_KERNEL);
>  	if (!domains)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < num_domains; i++, scmi_pd++) {
> +	scmi_pd = devm_kcalloc(dev, max_id + 1, sizeof(*scmi_pd), GFP_KERNEL);
> +	if (!scmi_pd)
> +		return -ENOMEM;
> +
> +	/* Initialize only referenced domains */
> +	for_each_set_bit(index, domain_ids, num_domains) {
>  		u32 state;
>  
> -		if (power_ops->state_get(ph, i, &state)) {
> -			dev_warn(dev, "failed to get state for domain %d\n", i);
> +		if (power_ops->state_get(ph, index, &state)) {
> +			dev_err(dev, "Domain %d not available\n", index);
>  			continue;
>  		}
>  
> -		/*
> -		 * Register the explicit power on request to the firmware so
> -		 * that it is tracked as used by OSPM agent and not
> -		 * accidentally turned off with OSPM's knowledge
> -		 */
> -		if (state == SCMI_POWER_STATE_GENERIC_ON)
> -			power_ops->state_set(ph, i, state);
> +		dev_warn(dev, "Initializing referenced domain %d\n", index);
>  
> -		scmi_pd->domain = i;
> +		scmi_pd->domain = index;
>  		scmi_pd->ph = ph;
> -		scmi_pd->name = power_ops->name_get(ph, i);
> +		scmi_pd->name = power_ops->name_get(ph, index);
>  		scmi_pd->genpd.name = scmi_pd->name;
>  		scmi_pd->genpd.power_off = scmi_pd_power_off;
>  		scmi_pd->genpd.power_on = scmi_pd_power_on;
>  		scmi_pd->genpd.flags = GENPD_FLAG_ACTIVE_WAKEUP;
>  
> +		if (state == SCMI_POWER_STATE_GENERIC_ON) {
> +			dev_warn(dev, "Domain %d is ON, registering state\n", index);
> +			power_ops->state_set(ph, index, state);
> +		}
> +
>  		pm_genpd_init(&scmi_pd->genpd, NULL,
>  			      state == SCMI_POWER_STATE_GENERIC_OFF);
>  
> -		domains[i] = &scmi_pd->genpd;
> +		domains[index] = &scmi_pd->genpd;
> +		scmi_pd++;
>  	}
>  
>  	scmi_pd_data->domains = domains;
> -	scmi_pd_data->num_domains = num_domains;
> +	scmi_pd_data->num_domains = max_id + 1;
>  
>  	dev_set_drvdata(dev, scmi_pd_data);
>  
> -	return of_genpd_add_provider_onecell(np, scmi_pd_data);
> +	dev_err(dev, "SCMI power domains probe completed in %lld us\n",
> +			ktime_us_delta(ktime_get(), start_time));
> +
> +	return of_genpd_add_provider_onecell(dev->of_node, scmi_pd_data);
>  }
>  
>  static void scmi_pm_domain_remove(struct scmi_device *sdev)
> -- 
> 2.34.1
> 

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

Powered by blists - more mailing lists