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