Re: [RFC PATCH v4 9/9] pmdomain: rockchip: Add a regulator to the RK3568 NPU power domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled error from `rockchip_pd_power()` during probe leaves the power domain in an inconsistent state, causing `genpd` to wrongly assume the domain is ON.
--

commit 08758fdfbdda5f83d100c68fb3b2bbe8fd467ea7
Author: Midgy BALON <midgy971@gmail.com>

pmdomain: rockchip: Add a regulator to the RK3568 NPU power domain

This patch gives DOMAIN_RK3568 a regulator parameter, allowing the NPU
domain to set need_regulator. This enables genpd to manage the NPU rail
wired up as the domain's domain-supply instead of marking it always-on.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index 490bbb1d1d8e8..19db307e3811d 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c
[ ... ]
> @@ -1274,15 +1288,15 @@ static const struct rockchip_domain_info rk3562_pm_domains[] = {
>  };
>  
>  static const struct rockchip_domain_info rk3568_pm_domains[] = {
> -	[RK3568_PD_NPU]		= DOMAIN_RK3568("npu",  BIT(1), BIT(2),  false),
> -	[RK3568_PD_GPU]		= DOMAIN_RK3568("gpu",  BIT(0), BIT(1),  false),
> -	[RK3568_PD_VI]		= DOMAIN_RK3568("vi",   BIT(6), BIT(3),  false),
> -	[RK3568_PD_VO]		= DOMAIN_RK3568("vo",   BIT(7), BIT(4),  false),
> -	[RK3568_PD_RGA]		= DOMAIN_RK3568("rga",  BIT(5), BIT(5),  false),
> -	[RK3568_PD_VPU]		= DOMAIN_RK3568("vpu",  BIT(2), BIT(6),  false),
> -	[RK3568_PD_RKVDEC]	= DOMAIN_RK3568("vdec", BIT(4), BIT(8),  false),
> -	[RK3568_PD_RKVENC]	= DOMAIN_RK3568("venc", BIT(3), BIT(7),  false),
> -	[RK3568_PD_PIPE]	= DOMAIN_RK3568("pipe", BIT(8), BIT(11), false),
> +	[RK3568_PD_NPU]		= DOMAIN_RK3568("npu",  BIT(1), BIT(2),  false, true),

[Severity: High]
This is a pre-existing issue, but by setting need_regulator to true here,
does this expose the NPU domain to a potential probe failure and system
hang?

Looking at rockchip_pm_add_one_domain(), the return value of
rockchip_pd_power(pd, false) is currently ignored:

drivers/pmdomain/rockchip/pm-domains.c:rockchip_pm_add_one_domain() {
	...
	if (pd->info->need_regulator)
		rockchip_pd_power(pd, false);
	...
}

If the bootloader leaves the PMU MTCMOS register ON but the external
regulator is OFF, rockchip_pd_power() attempts to send an idle request.
Because the IP is physically unpowered, it cannot assert the ACK signal
and the request times out:

drivers/pmdomain/rockchip/pm-domains.c:rockchip_pd_power() {
	...
	ret = rockchip_pmu_set_idle_request(pd, true);
	if (ret < 0)
		goto out;
	...
}

Since the timeout error is ignored in rockchip_pm_add_one_domain(), the PMU
MTCMOS register remains ON, which causes pm_genpd_init() to incorrectly
initialize the domain as ON. When the consumer driver later requests power,
genpd assumes the domain is already active and skips enabling the regulator,
causing the driver to access an unpowered IP block.

Should the error handling in rockchip_pm_add_one_domain() be fixed to
properly synchronize the domain state before enabling this feature for the
NPU?

> +	[RK3568_PD_GPU]		= DOMAIN_RK3568("gpu",  BIT(0), BIT(1),  false, false),
> +	[RK3568_PD_VI]		= DOMAIN_RK3568("vi",   BIT(6), BIT(3),  false, false),
> +	[RK3568_PD_VO]		= DOMAIN_RK3568("vo",   BIT(7), BIT(4),  false, false),
> +	[RK3568_PD_RGA]		= DOMAIN_RK3568("rga",  BIT(5), BIT(5),  false, false),
> +	[RK3568_PD_VPU]		= DOMAIN_RK3568("vpu",  BIT(2), BIT(6),  false, false),
> +	[RK3568_PD_RKVDEC]	= DOMAIN_RK3568("vdec", BIT(4), BIT(8),  false, false),
> +	[RK3568_PD_RKVENC]	= DOMAIN_RK3568("venc", BIT(3), BIT(7),  false, false),
> +	[RK3568_PD_PIPE]	= DOMAIN_RK3568("pipe", BIT(8), BIT(11), false, false),
>  };
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613070116.438906-1-midgy971@gmail.com?part=9