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 <[email protected]> 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/[email protected]?part=9
