Re: [PATCH v3 12/18] pmdomain: st: ux500: Control DB8500 EPODs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pmdomain: ux500: missing RAM retention flags in power domains
- [Medium] pmdomain: ux500: power leak from initializing domains as OFF
--

commit 9e8ecf92bb791471023c621b0664e285a95b3ad6
Author: Linus Walleij <linusw@kernel.org>

pmdomain: st: ux500: Control DB8500 EPODs

This commit moves the DB8500 EPOD state handling into the Ux500 power-domain
driver. It also keeps the old regulator driver mutually exclusive with the
pmdomain driver.

> diff --git a/drivers/pmdomain/st/ste-ux500-pm-domain.c b/drivers/pmdomain/st/ste-ux500-pm-domain.c
> index d90a563cd6f37..b31a36a4acd17 100644
> --- a/drivers/pmdomain/st/ste-ux500-pm-domain.c
> +++ b/drivers/pmdomain/st/ste-ux500-pm-domain.c
[ ... ]
> +static int enable_epod(u16 epod_id, bool ramret)
> +{
> +	int ret;
> +
> +	if (ramret) {
> +		if (!epod_on[epod_id]) {
> +			ret = db8500_prcmu_set_epod(epod_id, EPOD_STATE_RAMRET);

[Severity: High]
Are the RAM retention branches in enable_epod() and disable_epod() dead code?

The dbx500_powerdomain_info struct defines an is_ramret boolean, and these
functions check it to use EPOD_STATE_RAMRET. However, none of the 16 domains
initialized in ux500_pm_domains set .is_ramret = true.

[ ... ]
> -static struct generic_pm_domain ux500_pm_domain_sva_mmdsp = {
> +static struct dbx500_powerdomain_info ux500_pm_domain_sva_mmdsp = {
>  	/* Vape_SVA_MMDSP_pwr */
> -	.name = "SVA_MMDSP",
> -	.power_off = pd_power_off,
> -	.power_on = pd_power_on,
> +	.genpd = {
> +		.name = "SVA_MMDSP",
> +		.power_off = pd_power_off,
> +		.power_on = pd_power_on,
> +	},
> +	.epod_id = EPOD_ID_SVAMMDSP,
>  };

[Severity: High]
Will the omission of .is_ramret = true here cause SRAM data loss or prevent
the domain from entering retention during suspend?

In the old regulator driver, domains such as SVAMMDSP, SIAMMDSP, ESRAM12,
and ESRAM34 explicitly had this flag set. Because the flag is never set in
the new PM domain driver, it defaults to false. This means normal runtime PM
transitions will fully power off the domains (EPOD_STATE_OFF) instead of
retaining their state.

[ ... ]
> @@ -186,11 +304,12 @@ static int ux500_pm_domains_probe(struct platform_device *pdev)
[ ... ]
>  	genpd_data->num_domains = ARRAY_SIZE(ux500_pm_domains);
>  
>  	for (i = 0; i < ARRAY_SIZE(ux500_pm_domains); ++i)
> -		pm_genpd_init(ux500_pm_domains[i], NULL, false);
> +		pm_genpd_init(ux500_pm_domains[i], NULL, true);

[Severity: Medium]
Could initializing these domains as software-OFF (is_off = true) lead to a
power leak if the bootloader leaves them enabled?

When initialized as OFF (true), the genpd framework does not set stay_on
and considers the domain to be already powered off in software. Because of
this, genpd_power_off_unused() sees them as already OFF and skips calling
pd_power_off().

Consequently, any domain left ON by the bootloader without an active kernel
consumer is never powered off, failing to replicate the late init cleanup
behavior of the old regulator driver.

> +
> +	ret = of_genpd_add_provider_onecell(np, genpd_data);
> +	if (ret)
> +		goto remove_genpds;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-ux500-power-domains-v7-1-v3-0-9352be6b86b1@kernel.org?part=12