Re: [PATCH 3/3] pmdomain: arm_scmi: add support for domain hierarchies

[Dhruva Gole]

On Mar 10, 2026 at 17:19:25 -0700, Kevin Hilman (TI) wrote:
> After primary SCMI pmdomain is created, use new of_genpd helper which
> checks for child domain mappings defined in power-domains-child-ids.
> 
> Also remove any child domain mappings when SCMI domain is removed.
> 
> Signed-off-by: Kevin Hilman (TI) <khilman@xxxxxxxxxxxx>
> ---

Again, since it worked fine on my AM62L,
Tested-by: Dhruva Gole <d-gole@xxxxxx>

But I had some thoughts further down...

>  drivers/pmdomain/arm/scmi_pm_domain.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pmdomain/arm/scmi_pm_domain.c b/drivers/pmdomain/arm/scmi_pm_domain.c
> index b5e2ffd5ea64..9d8faef44aa9 100644
> --- a/drivers/pmdomain/arm/scmi_pm_domain.c
> +++ b/drivers/pmdomain/arm/scmi_pm_domain.c
> @@ -114,6 +114,14 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>  
>  	dev_set_drvdata(dev, scmi_pd_data);
>  
> +	/*
> +	 * Parse (optional) power-domains-child-ids property to
> +	 * establish parent-child relationships
> +	 */
> +	ret = of_genpd_add_child_ids(np, scmi_pd_data);
> +	if (ret < 0 && ret != -ENOENT)
> +		pr_err("Failed to parse power-domains-child-ids for %pOF: %d\n", np, ret);

Nit: I think the style of this driver is to use dev_err than pr_err

Also, maybe a dev_warn makes more sense since we're not even returning
the error or doing anything different if we get certain error path.

I am wondering if it makes sense to just abort the whole idea of
creating power-domain child ids if anything goes wrong?

Basically just of_genpd_remove_child_ids if we face a condition where we
have different number of parents/ children or id > num etc...

All are error cases where the system behaviour can go on to become very
unpredictable if we end up making a false/ incomplete parent-child ID
map.

Thoughts?

PS.
If we go on to do a of_genpd_remove_child_ids here incase of failure then it makes sense to
scream a dev_err here.


> +
>  	return 0;
>  err_rm_genpds:
>  	for (i = num_domains - 1; i >= 0; i--)
> @@ -129,9 +137,13 @@ static void scmi_pm_domain_remove(struct scmi_device *sdev)
>  	struct device *dev = &sdev->dev;
>  	struct device_node *np = dev->of_node;
>  
> +	scmi_pd_data = dev_get_drvdata(dev);
> +
> +	/* Remove any parent-child relationships established at probe time */
> +	of_genpd_remove_child_ids(np, scmi_pd_data);
> +
>  	of_genpd_del_provider(np);
>  
> -	scmi_pd_data = dev_get_drvdata(dev);
>  	for (i = 0; i < scmi_pd_data->num_domains; i++) {
>  		if (!scmi_pd_data->domains[i])
>  			continue;
> 
> -- 
> 2.51.0
> 
> 

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated