Re: [PATCH RFC 12/16] clk: qcom: gdsc: Link required power domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: gdsc: Unbalanced hardware state on GenPD unlink leaves GDSCs powered without required rails
- [Low] clk: qcom: gdsc: Redundant wait_for_off logic for non-votable domains
--

commit a3b8e242fc8ba2eae7b378662d9af778b3739733
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

clk: qcom: gdsc: Link required power domains

This commit models enable-only requirements for GDSCs as GenPD subdomain
links without propagating the GDSC performance state to the rail. It allows
GDSCs to specify only the power-domain endpoints they require while powered.

> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 2d0803e1eb3b1..73054c600c29a 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
[ ... ]
> @@ -379,7 +384,10 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	if (sc->pwrsts == PWRSTS_RET_ON)
>  		return 0;
>  
> -	ret = gdsc_toggle_logic(sc, GDSC_OFF, domain->synced_poweroff);
> +	if (sc->num_required_pds && !(sc->flags & VOTABLE))
> +		wait_for_off = true;
> +
> +	ret = gdsc_toggle_logic(sc, GDSC_OFF, wait_for_off);

[Severity: Low]
This isn't a bug, but is setting wait_for_off here redundant for non-votable
domains?

Looking at gdsc_toggle_logic(), the wait parameter appears to only affect
the execution path when sc->flags & VOTABLE is true:

static int gdsc_toggle_logic(...) {
    ...
    /* If disabling votable gdscs, don't poll on status */
    if ((sc->flags & VOTABLE) && status == GDSC_OFF && !wait) {
        udelay(TIMEOUT_US);
        return 0;
    }
    ...
    ret = gdsc_poll_status(sc, status);
}

Since gdsc_toggle_logic() falls through to gdsc_poll_status() for all
non-votable domains regardless of the wait parameter, does overriding it
here have any effect?

[ ... ]
> @@ -669,13 +779,25 @@ int gdsc_register(struct gdsc_desc *desc,
>  			ret = gdsc_add_subdomain_list(desc->pd_list, &scs[i]->pd);
>  
>  		if (ret)
> -			goto err_pm_subdomain_remove;
> +			goto err_remove_parents;
>  	}
>  
> -	return of_genpd_add_provider_onecell(dev->of_node, data);
> +	ret = of_genpd_add_provider_onecell(dev->of_node, data);
> +	if (!ret)
> +		return 0;
>  
> -err_pm_subdomain_remove:
> +err_remove_parents:
>  	gdsc_pm_subdomain_remove(desc, i);
> +err_remove_domains:
> +	while (linked--) {
> +		if (scs[linked])
> +			gdsc_remove_required_subdomains(desc, scs[linked],
> +							scs[linked]->num_required_pds);
> +	}
> +	while (initialized--) {
> +		if (scs[initialized])
> +			pm_genpd_remove(&scs[initialized]->pd);
> +	}

[Severity: High]
Does this error path leave initialized GDSCs physically powered ON while
their required supply rails are powered OFF?

During initialization, gdsc_get_rail_voters() powers ON the required rails
via pm_runtime_resume_and_get(). Then gdsc_init() turns ON the GDSC
hardware, and gdsc_add_required_subdomains() links the rails as parents,
transferring the power vote to GenPD. Finally, gdsc_put_rail_voters()
drops the explicit pm_runtime vote.

If we take this err_remove_domains path, gdsc_remove_required_subdomains()
removes the GenPD links. This drops the GenPD power vote, causing the
required rails to physically power OFF.

However, the loop then calls pm_genpd_remove() without physically powering
OFF the GDSCs. Will this violate hardware power sequencing constraints by
leaving the GDSCs powered ON without their required rails?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=12