Re: [PATCH v14 09/15] pmdomain: rockchip: add optional per-domain power-on settle delay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pmdomain: rockchip: forced NPU power-off bypasses child QoS save and state hierarchy
--

commit e61a0d5aec29d1b9ed2c36b6307c8069719a9a05
Author: Jiaxing Hu <gahing@gahingwoo.com>

pmdomain: rockchip: add optional per-domain power-on settle delay

The RK3576 NPU domains need a short settle time after the idle request is
released before the registers behind the domain answer. This patch gives
rockchip_domain_info an optional delay_us and waits for it between releasing
idle and restoring QoS. It also passes true for the regulator argument for
the NPU domain, so it powers off at probe and the delay runs on the first
power-on.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index ba66ae7194289..39988efd86aaa 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -681,6 +688,10 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>  		if (ret < 0)
>  			goto out;
>  
> +		/* Some domains need to settle before the QoS registers answer. */
> +		if (pd->info->delay_us)
> +			udelay(pd->info->delay_us);
> +
>  		rockchip_pmu_restore_qos(pd);

[Severity: High]
Does restoring the QoS registers here lead to zero-overwrites if the bootloader
leaves the NPU domains active?

When rockchip_pm_add_one_domain() forcefully powers down the parent domain at
probe, it physically cuts power to its subdomains before their QoS state can be
saved. This leaves pd->qos_save_regs zero-initialized.

When the subdomains are finally powered on, this routine restores those
bypassed, zero-initialized arrays into the hardware registers, which could
cause a hardware malfunction.

>  	}

[ ... ]

> @@ -1300,25 +1311,26 @@ static const struct rockchip_domain_info rk3568_pm_domains[] = {
>  };
>  
>  static const struct rockchip_domain_info rk3576_pm_domains[] = {
> -	[RK3576_PD_NPU]		= DOMAIN_RK3576("npu",    0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       false),
> -	[RK3576_PD_NVM]		= DOMAIN_RK3576("nvm",    0x0, BIT(6),  0,      BIT(6),  0x4, BIT(2),  BIT(18), BIT(2),  false),
[ ... ]
> +	/*                                            name    p_offset pwr      status  r_status r_offset req      idle     g_mask   delay wakeup */
> +	[RK3576_PD_NPU]		= DOMAIN_RK3576_R("npu",  0x0, BIT(0),  BIT(0), 0,       0x0, 0,       0,       0,       0,    false),

[Severity: High]
Can setting need_regulator = true (via DOMAIN_RK3576_R) for the parent NPU
domain cause a state mismatch and fatal probe failure?

This intentionally forces the parent NPU domain to power off at probe time.
However, its subdomains do not enforce the OFF state at probe and retain their
status register states.

For example, subdomains like RK3576_PD_NPUTOP remain configured without
need_regulator set:

> +	[RK3576_PD_NVM]		= DOMAIN_RK3576("nvm",    0x0, BIT(6),  0,      BIT(6),  0x4, BIT(2),  BIT(18), BIT(2),  0,    false),
[ ... ]
> +	[RK3576_PD_NPUTOP]	= DOMAIN_RK3576("nputop", 0x4, BIT(6),  0,      BIT(22), 0x0, 0x18,    0x18,    0x18,    15,   false),

If the child PMU status registers still read as active after the parent power
is abruptly cut, wouldn't pm_genpd_add_subdomain() encounter a
parent-OFF/child-ON state mismatch and fail with -EINVAL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924102135.92217-1-gahing@gahingwoo.com?part=9