Re: [PATCH 10/27] pmdomain: rockchip: Add RK3572 boot peripheral domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pmdomain: rockchip: incomplete power-on retry tracking in rockchip_pd_power()
--

commit c324244eacf06dcfc61ec2e5830623efbacb5d15
Author: Cunhao Lu <1579567540@qq.com>

pmdomain: rockchip: Add RK3572 boot peripheral domains

This commit adds the rk3572 boot peripheral power domains (nvm0, nvm1, php,
and vop), waits for the grf firewall before restoring consumers, and tracks
pending power-on transitions to retry failures without power-cycling the
domain.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index e193f978d5f35..86a6ffd43254b 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -644,14 +657,29 @@ static int rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>  	return 0;
>  }
>  
> +static int rockchip_pmu_wait_firewall(struct rockchip_pm_domain *pd)
> +{
> +	struct rockchip_pmu *pmu = pd->pmu;
> +	u32 mask = pd->info->firewall_mask;
> +	u32 val;
> +
> +	if (!mask)
> +		return 0;
> +
> +	return regmap_read_poll_timeout(pmu->grf, pmu->info->firewall_status_offset,
> +				       val, (val & mask) == mask, 0, 10000);
> +}
> +
>  static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  {
>  	struct rockchip_pmu *pmu = pd->pmu;
> +	bool is_on;
>  	int ret;
>  
>  	guard(mutex)(&pmu->mutex);
>  
> -	if (rockchip_pmu_domain_is_on(pd) == power_on)
> +	is_on = rockchip_pmu_domain_is_on(pd);
> +	if (is_on == power_on && !pd->power_on_pending)
>  		return 0;
>  
>  	ret = clk_bulk_enable(pd->num_clks, pd->clks);
> @@ -663,7 +691,8 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  	rockchip_pmu_ungate_clk(pd, true);
>  
>  	if (!power_on) {
> -		rockchip_pmu_save_qos(pd);
> +		if (!pd->power_on_pending)
> +			rockchip_pmu_save_qos(pd);
>  
>  		/* if powering down, idle request to NIU first */
>  		ret = rockchip_pmu_set_idle_request(pd, true);
> @@ -671,18 +700,29 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  			goto out;
>  	}
>  
> -	ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> -	if (ret < 0)
> -		goto out;
> +	/* A failed firewall wait must not restart powered memory on retry. */
> +	if (is_on != power_on) {
> +		ret = rockchip_do_pmu_set_power_domain(pd, power_on);
> +		if (ret < 0)
> +			goto out;
> +	}
>  
>  	if (power_on) {
> +		if (pd->info->firewall_mask)
> +			pd->power_on_pending = true;
> +
>  		/* if powering up, leave idle mode */
>  		ret = rockchip_pmu_set_idle_request(pd, false);
>  		if (ret < 0)
>  			goto out;

[Severity: High]
In drivers/pmdomain/rockchip/pm-domains.c, does setting pd->power_on_pending
only when pd->info->firewall_mask is non-zero and only after
rockchip_do_pmu_set_power_domain() succeeds leave partial power-on states
untracked in rockchip_pd_power()?

When genpd_power_on() -> rockchip_pd_power_on() -> rockchip_pd_power(pd, true)
is called for a domain that is initially off (is_on == false),
rockchip_do_pmu_set_power_domain(pd, true) enables the hardware power switch
in pmu->info->pwr_offset + pd_pwr_offset, making rockchip_pmu_domain_is_on(pd)
return true.

If rockchip_pmu_set_idle_request(pd, false) subsequently fails on a domain
where pd->info->firewall_mask is 0 (such as the "nvm0" and "nvm1" domains on
rk3572, or domains on other SoCs), or if rockchip_do_pmu_set_power_domain()
fails in rockchip_pmu_domain_mem_reset() after enabling the power switch,
pd->power_on_pending remains false when jumping to out.

If a later power-on retry calls rockchip_pd_power(pd, true),
rockchip_pmu_domain_is_on(pd) will return true while pd->power_on_pending is
false:

	is_on = rockchip_pmu_domain_is_on(pd);
	if (is_on == power_on && !pd->power_on_pending)
		return 0;

Wouldn't rockchip_pd_power(pd, true) then return 0 immediately without
deasserting the bus idle request, resetting memory, waiting for the firewall,
or restoring QoS registers via rockchip_pmu_restore_qos()? Furthermore, if
rockchip_pd_power(pd, false) is called in that state, wouldn't the
!pd->power_on_pending check allow rockchip_pmu_save_qos(pd) to overwrite
pd->qos_save_regs with unrestored values while the domain is still idle?

Should pd->power_on_pending be set unconditionally before
rockchip_do_pmu_set_power_domain(pd, true) when powering on a domain?

>  
> +		ret = rockchip_pmu_wait_firewall(pd);
> +		if (ret)
> +			goto out;
> +
>  		rockchip_pmu_restore_qos(pd);
>  	}
> +	pd->power_on_pending = false;
>  
>  out:
>  	rockchip_pmu_ungate_clk(pd, false);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add-board-ok3572-c-v1-0-0b0fd85a31ee@qq.com?part=10