Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] pmdomain: rockchip: parent-only power-off in rockchip_pm_add_one_domain() --
commit b6869eb827130372ac40273ce1417cb61dc66c51 Author: Jiaxing Hu <[email protected]> pmdomain: rockchip: add optional per-domain power-on settle delay This commit adds an optional per-domain delay_us to rockchip_domain_info to wait between releasing the idle request and restoring QoS registers, and sets a 15 us delay on the RK3576 NPU child domains while marking RK3576_PD_NPU with need_regulator = true. > diff --git a/drivers/pmdomain/rockchip/pm-domains.c > b/drivers/pmdomain/rockchip/pm-domains.c > index db3879846db67..e3cc60fa22771 100644 > --- a/drivers/pmdomain/rockchip/pm-domains.c > +++ b/drivers/pmdomain/rockchip/pm-domains.c [ ... ] > @@ -1314,25 +1325,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), > - [RK3576_PD_SDGMAC] = DOMAIN_RK3576("sdgmac", 0x0, BIT(7), 0, > BIT(7), 0x4, BIT(1), BIT(17), 0x6, false), > - [RK3576_PD_AUDIO] = DOMAIN_RK3576("audio", 0x0, BIT(8), 0, > BIT(8), 0x4, BIT(0), BIT(16), BIT(0), false), > - [RK3576_PD_PHP] = DOMAIN_RK3576("php", 0x0, BIT(9), 0, > BIT(9), 0x0, BIT(15), BIT(15), BIT(15), false), > - [RK3576_PD_SUBPHP] = DOMAIN_RK3576("subphp", 0x0, BIT(10), 0, > BIT(10), 0x0, 0, 0, 0, false), > - [RK3576_PD_VOP] = DOMAIN_RK3576("vop", 0x0, BIT(11), 0, > BIT(11), 0x0, 0x6000, 0x6000, 0x6000, false), > - [RK3576_PD_VO1] = DOMAIN_RK3576("vo1", 0x0, BIT(14), 0, > BIT(14), 0x0, BIT(12), BIT(12), 0x7000, false), > - [RK3576_PD_VO0] = DOMAIN_RK3576("vo0", 0x0, BIT(15), 0, > BIT(15), 0, 0x0, BIT(11), BIT(11), 0x6800, false), > - [RK3576_PD_USB] = DOMAIN_RK3576("usb", 0x4, BIT(0), 0, > BIT(16), 0x0, BIT(10), BIT(10), 0x6400, true), > - [RK3576_PD_VI] = DOMAIN_RK3576("vi", 0x4, BIT(1), 0, > BIT(17), 0x0, BIT(9), BIT(9), BIT(9), false), > - [RK3576_PD_VEPU0] = DOMAIN_RK3576("vepu0", 0x4, BIT(2), 0, > BIT(18), 0x0, BIT(7), BIT(7), 0x280, false), > - [RK3576_PD_VEPU1] = DOMAIN_RK3576("vepu1", 0x4, BIT(3), 0, > BIT(19), 0x0, BIT(8), BIT(8), BIT(8), false), > - [RK3576_PD_VDEC] = DOMAIN_RK3576("vdec", 0x4, BIT(4), 0, > BIT(20), 0x0, BIT(6), BIT(6), BIT(6), false), > - [RK3576_PD_VPU] = DOMAIN_RK3576("vpu", 0x4, BIT(5), 0, > BIT(21), 0x0, BIT(5), BIT(5), BIT(5), false), > - [RK3576_PD_NPUTOP] = DOMAIN_RK3576("nputop", 0x4, BIT(6), 0, > BIT(22), 0x0, 0x18, 0x18, 0x18, false), > - [RK3576_PD_NPU0] = DOMAIN_RK3576("npu0", 0x4, BIT(7), 0, > BIT(23), 0x0, BIT(1), BIT(1), 0x1a, false), > - [RK3576_PD_NPU1] = DOMAIN_RK3576("npu1", 0x4, BIT(8), 0, > BIT(24), 0x0, BIT(2), BIT(2), 0x1c, false), > - [RK3576_PD_GPU] = DOMAIN_RK3576("gpu", 0x4, BIT(9), 0, > BIT(25), 0x0, BIT(0), BIT(0), BIT(0), 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), > + [RK3576_PD_NVM] = DOMAIN_RK3576("nvm", 0x0, BIT(6), 0, > BIT(6), 0x4, BIT(2), BIT(18), BIT(2), 0, false), > + [RK3576_PD_SDGMAC] = DOMAIN_RK3576("sdgmac", 0x0, BIT(7), 0, > BIT(7), 0x4, BIT(1), BIT(17), 0x6, 0, false), > + [RK3576_PD_AUDIO] = DOMAIN_RK3576("audio", 0x0, BIT(8), 0, > BIT(8), 0x4, BIT(0), BIT(16), BIT(0), 0, false), > + [RK3576_PD_PHP] = DOMAIN_RK3576("php", 0x0, BIT(9), 0, > BIT(9), 0x0, BIT(15), BIT(15), BIT(15), 0, false), > + [RK3576_PD_SUBPHP] = DOMAIN_RK3576("subphp", 0x0, BIT(10), 0, > BIT(10), 0x0, 0, 0, 0, 0, false), > + [RK3576_PD_VOP] = DOMAIN_RK3576("vop", 0x0, BIT(11), 0, > BIT(11), 0x0, 0x6000, 0x6000, 0x6000, 0, false), > + [RK3576_PD_VO1] = DOMAIN_RK3576("vo1", 0x0, BIT(14), 0, > BIT(14), 0x0, BIT(12), BIT(12), 0x7000, 0, false), > + [RK3576_PD_VO0] = DOMAIN_RK3576("vo0", 0x0, BIT(15), 0, > BIT(15), 0x0, BIT(11), BIT(11), 0x6800, 0, false), > + [RK3576_PD_USB] = DOMAIN_RK3576("usb", 0x4, BIT(0), 0, > BIT(16), 0x0, BIT(10), BIT(10), 0x6400, 0, true), > + [RK3576_PD_VI] = DOMAIN_RK3576("vi", 0x4, BIT(1), 0, > BIT(17), 0x0, BIT(9), BIT(9), BIT(9), 0, false), > + [RK3576_PD_VEPU0] = DOMAIN_RK3576("vepu0", 0x4, BIT(2), 0, > BIT(18), 0x0, BIT(7), BIT(7), 0x280, 0, false), > + [RK3576_PD_VEPU1] = DOMAIN_RK3576("vepu1", 0x4, BIT(3), 0, > BIT(19), 0x0, BIT(8), BIT(8), BIT(8), 0, false), > + [RK3576_PD_VDEC] = DOMAIN_RK3576("vdec", 0x4, BIT(4), 0, > BIT(20), 0x0, BIT(6), BIT(6), BIT(6), 0, false), > + [RK3576_PD_VPU] = DOMAIN_RK3576("vpu", 0x4, BIT(5), 0, > BIT(21), 0x0, BIT(5), BIT(5), BIT(5), 0, false), > + [RK3576_PD_NPUTOP] = DOMAIN_RK3576("nputop", 0x4, BIT(6), 0, > BIT(22), 0x0, 0x18, 0x18, 0x18, 15, false), > + [RK3576_PD_NPU0] = DOMAIN_RK3576("npu0", 0x4, BIT(7), 0, > BIT(23), 0x0, BIT(1), BIT(1), 0x1a, 15, false), > + [RK3576_PD_NPU1] = DOMAIN_RK3576("npu1", 0x4, BIT(8), 0, > BIT(24), 0x0, BIT(2), BIT(2), 0x1c, 15, false), [Severity: High] As reported by Sashiko on v14 [1], what happens in rockchip_pm_domain_probe() when a bootloader leaves the NPU domains powered on? In rk3576_pm_domains[], RK3576_PD_NPU uses DOMAIN_RK3576_R (setting need_regulator = true and delay_us = 0), while its child domain RK3576_PD_NPUTOP and grandchild domains RK3576_PD_NPU0 and RK3576_PD_NPU1 carry the 15 us delay_us, NIU idle masks, and QoS registers, but use DOMAIN_RK3576 with need_regulator = false. During probe, rockchip_pm_domain_probe() registers parent domains before visiting subdomains via rockchip_pm_add_subdomain(): drivers/pmdomain/rockchip/pm-domains.c:rockchip_pm_add_one_domain() { ... if (pd->info->need_regulator) rockchip_pd_power(pd, false); ... pm_genpd_init(&pd->genpd, NULL, !rockchip_pmu_domain_is_on(pd) || (pd->info->mem_status_mask && !rockchip_pmu_domain_is_mem_on(pd))); } Because only RK3576_PD_NPU has need_regulator = true, rockchip_pm_add_one_domain() calls rockchip_pd_power(pd, false) on the parent domain RK3576_PD_NPU while its child domains RK3576_PD_NPUTOP, RK3576_PD_NPU0, and RK3576_PD_NPU1 are still active, and never powers off those child domains at probe. If the child status bits in repair_status_offset and mem_status_offset still report RK3576_PD_NPUTOP as on after RK3576_PD_NPU is powered off, RK3576_PD_NPU is initialized in GENPD_STATE_OFF while RK3576_PD_NPUTOP is initialized in GENPD_STATE_ON. When rockchip_pm_add_subdomain() then links them: rockchip_pm_add_subdomain() -> pm_genpd_add_subdomain() -> genpd_add_subdomain() drivers/pmdomain/core.c:genpd_add_subdomain() { ... if (!genpd_status_on_unlocked(genpd) && genpd_status_on_unlocked(subdomain)) { ret = -EINVAL; goto out; } } won't pm_genpd_add_subdomain() fail with -EINVAL and abort rockchip_pm_domain_probe() for all power domains on the SoC? Conversely, even if cutting RK3576_PD_NPU forced the child domains off in hardware (or if hardware ignored the parent power-off while children are active), wouldn't that either cut power to the child domains without idling their NIU bus interfaces and saving their QoS registers, or leave the child domains in GENPD_STATE_ON so that the 15 us delay_us in rockchip_pd_power() is still skipped on the first power-on? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
