On 24/01/2023 11:33, Ulf Hansson wrote: > On Thu, 19 Jan 2023 at 19:42, Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> The runtime Power Management of CPU topology is not compatible with >> PREEMPT_RT: >> 1. Core cpuidle path disables IRQs. >> 2. Core cpuidle calls cpuidle-psci. >> 3. cpuidle-psci in __psci_enter_domain_idle_state() calls >> pm_runtime_put_sync_suspend() and pm_runtime_get_sync() which use >> spinlocks (which are sleeping on PREEMPT_RT). >> >> Deep sleep modes are not a priority of Realtime kernels because the >> latencies might become unpredictable. On the other hand the PSCI CPU >> idle power domain is a parent of other devices and power domain >> controllers, thus it cannot be simply skipped (e.g. on Qualcomm SM8250). >> >> Disable the runtime PM calls from cpuidle-psci, which effectively stops >> suspending the cpuidle PSCI domain. This is a trade-off between making >> PREEMPT_RT working and still having a proper power domain hierarchy in >> the system. > > I think this sounds like a reasonable compromise, at least at this point. > >> >> Cc: Adrien Thierry <athierry@xxxxxxxxxx> >> Cc: Brian Masney <bmasney@xxxxxxxxxx> >> Cc: linux-rt-users@xxxxxxxxxxxxxxx >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> >> >> --- >> >> Changes since v1: >> 1. Re-work commit msg. >> 2. Add note to Kconfig. >> >> Several other patches were dropped, as this is the only one actually >> needed. It effectively stops PSCI cpuidle power domains from suspending >> thus solving all other issues I experienced. > > I like this approach better, thanks! > >> --- >> drivers/cpuidle/Kconfig.arm | 3 +++ >> drivers/cpuidle/cpuidle-psci.c | 4 ++-- >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm >> index 747aa537389b..24429b5bfd1c 100644 >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm >> @@ -24,6 +24,9 @@ config ARM_PSCI_CPUIDLE >> It provides an idle driver that is capable of detecting and >> managing idle states through the PSCI firmware interface. >> >> + The driver is not yet compatible with PREEMPT_RT: no idle states will >> + be entered by CPUs on such kernel. > > This isn't entirely correct. In principle your suggested change ends > up providing the below updated behaviour for PREEMPT_RT. > > *) If the idle states are described with the non-hierarchical layout, > all idle states are still available. > **) If the idle states are described with the hierarchical layout, > only the idle states defined per CPU are available, but not the ones > being shared among a group of CPUs (aka cluster idle states). > > Perhaps there is an easier way to summarize what I stated above? Yes, I'll correct the message. > >> + >> config ARM_PSCI_CPUIDLE_DOMAIN >> bool "PSCI CPU idle Domain" >> depends on ARM_PSCI_CPUIDLE >> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c >> index 312a34ef28dc..c25592718984 100644 >> --- a/drivers/cpuidle/cpuidle-psci.c >> +++ b/drivers/cpuidle/cpuidle-psci.c >> @@ -66,7 +66,7 @@ static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev, >> /* Do runtime PM to manage a hierarchical CPU toplogy. */ >> if (s2idle) >> dev_pm_genpd_suspend(pd_dev); >> - else >> + else if (!IS_ENABLED(CONFIG_PREEMPT_RT)) > > Rather than doing this (and the below) in > __psci_enter_domain_idle_state(), I suggest replacing this with a > bailout point in psci_dt_cpu_init_topology(). That would prevent the > __psci_enter_domain_idle_state() from being called altogether, which > is really what we need. Ack > > Moreover, I think it would make sense to set the GENPD_FLAG_ALWAYS_ON > for the corresponding genpd, when CONFIG_PREEMPT_RT is set. See > psci_pd_init(). Makes sense. Best regards, Krzysztof
