| Message ID | 1580736940-6985-1-git-send-email-mkshah@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add RSC power domain support | expand |
On 2/4/2020 8:51 PM, Sudeep Holla wrote: > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote: >> On 2/3/2020 10:38 PM, Sudeep Holla wrote: >>> On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote: >>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>> >>>> If the hierarchical CPU topology is used, but the OS initiated mode isn't >>>> supported, we need to rely solely on the regular cpuidle framework to >>>> manage the idle state selection, rather than using genpd and its >>>> governor. >>>> >>>> For this reason, introduce a new PSCI DT helper function, >>>> psci_dt_pm_domains_parse_states(), which parses and converts the >>>> hierarchically described domain idle states from DT, into regular flattened >>>> cpuidle states. The converted states are added to the existing cpuidle >>>> driver's array of idle states, which make them available for cpuidle. >>>> >>> And what's the main motivation for this if OSI is not supported in the >>> firmware ? >> Hi Sudeep, >> >> Main motivation is to do last-man activities before the CPU cluster can >> enter a deep idle state. >> > Details on those last-man activities will help the discussion. Basically > I am wondering what they are and why they need to done in OSPM ? Hi Sudeep, there are cases like, Last cpu going to deepest idle mode need to lower various resoruce requirements (for eg DDR freq). This is done by calling rpmh_flush which send SLEEP values for various shared resources. >>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>> [applied to new path, resolved conflicts] >>>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >>>> --- >>>> drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++----- >>>> drivers/cpuidle/cpuidle-psci.c | 41 +++++----- >>>> drivers/cpuidle/cpuidle-psci.h | 11 +++ >>>> 3 files changed, 153 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c >>>> index 423f03b..3c417f7 100644 >>>> --- a/drivers/cpuidle/cpuidle-psci-domain.c >>>> +++ b/drivers/cpuidle/cpuidle-psci-domain.c >>>> @@ -26,13 +26,17 @@ struct psci_pd_provider { >>>> }; >>>> >>>> static LIST_HEAD(psci_pd_providers); >>>> -static bool osi_mode_enabled __initdata; >>>> +static bool osi_mode_enabled; >>>> >>>> static int psci_pd_power_off(struct generic_pm_domain *pd) >>>> { >>>> struct genpd_power_state *state = &pd->states[pd->state_idx]; >>>> u32 *pd_state; >>>> >>>> + /* If we have failed to enable OSI mode, then abort power off. */ >>>> + if ((psci_has_osi_support()) && !osi_mode_enabled) >>>> + return -EBUSY; >>>> + >>> Why is this needed ? IIUC we don't create genpd domains if OSI is not >>> enabled. >> we do create genpd domains, for cpu domains, we just abort power off here >> since idle states are converted into regular flattened mode. >> > OK, IIRC the OSI patches from Ulf didn't add the genpd or rather removed > them in case of any failure to enable OSI. Has that been changed ? If so, > why ? > >> however genpd poweroff will be used by parent domain (rsc in this case) >> which is kept in hireachy in DTSI with cluster domain to do last man >> activities. >> > I am bit confused here. Either we do OSI or PC and what you are describing > sounds like a mix-n-match to me and I am totally against it. we still do PC based on sc7180. there is no OSI. can you please check v4 series, i have cleaned this change by remove converter part. Thanks, Maulik > > -- > Regards, > Sudeep
On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote: > > > > On 2/4/2020 8:51 PM, Sudeep Holla wrote: > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote: > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote: > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote: > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > > > > > If the hierarchical CPU topology is used, but the OS initiated mode isn't > > > > > > supported, we need to rely solely on the regular cpuidle framework to > > > > > > manage the idle state selection, rather than using genpd and its > > > > > > governor. > > > > > > > > > > > > For this reason, introduce a new PSCI DT helper function, > > > > > > psci_dt_pm_domains_parse_states(), which parses and converts the > > > > > > hierarchically described domain idle states from DT, into regular flattened > > > > > > cpuidle states. The converted states are added to the existing cpuidle > > > > > > driver's array of idle states, which make them available for cpuidle. > > > > > > > > > > > And what's the main motivation for this if OSI is not supported in the > > > > > firmware ? > > > > Hi Sudeep, > > > > > > > > Main motivation is to do last-man activities before the CPU cluster can > > > > enter a deep idle state. > > > > > > > Details on those last-man activities will help the discussion. Basically > > > I am wondering what they are and why they need to done in OSPM ? > > > > Hi Sudeep, > > > > there are cases like, > > > > Last cpu going to deepest idle mode need to lower various resoruce > > requirements (for eg DDR freq). > > > > In PC mode, only PSCI implementation knows the last man and there shouldn't > be any notion of it in OS. If you need it, you may need OSI. You are still > mixing up the things. NACK for any such approach, sorry. Sudeep, I don't quite agree with your NACK to this. At least not yet. :-) I do agree that the best suited solution seems to be OSI, as to support this kind of SoC requirements. However, if for some reason the PC mode is being used, we could still allow Linux to control "last-man activities" as it knows what each CPU has voted for when going idle. Yes, the PSCI FW decides in the end, but that doesn't really matter. Or is there another technical reason to why you object? As a matter of fact, if we allow support for PC mode with "last-man-activities", it would allow us to make a fair performance/energy comparison between the two PSCI CPU suspend modes, for the same SoC. I would be thrilled about looking into doing such tests, I bet you are as well!? Kind regards Uffe
On Thu, Feb 06 2020 at 01:46 -0700, Ulf Hansson wrote: >On Wed, 5 Feb 2020 at 17:18, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> On Wed, Feb 05, 2020 at 04:55:17PM +0100, Ulf Hansson wrote: >> > On Wed, 5 Feb 2020 at 15:06, Sudeep Holla <sudeep.holla@arm.com> wrote: >> > > >> > > On Wed, Feb 05, 2020 at 05:53:00PM +0530, Maulik Shah wrote: >> > > > >> > > > On 2/4/2020 8:51 PM, Sudeep Holla wrote: >> > > > > On Tue, Feb 04, 2020 at 10:22:42AM +0530, Maulik Shah wrote: >> > > > > > On 2/3/2020 10:38 PM, Sudeep Holla wrote: >> > > > > > > On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote: >> > > > > > > > From: Ulf Hansson <ulf.hansson@linaro.org> >> > > > > > > > >> I was, but not anymore, especially if we want such changes in the kernel >> to do so. >> >> Just use OSI as that was the point of adding all these after years of >> discussion claiming it's more optimal compared to PC. Now telling that >> you need more changes to compare it with PC just doesn't make any sense >> at all to me. > >Fair enough. > >I was just pondering over if there are other reasons to why we may want this. > >One other thing that could be problematic to support, is when are >other resources, I/O controllers for example, sharing the same power >rail as a cluster. When such controller is in use, idle states of the >cluster must be prevented. Without using genpd to model the CPU >topology, it may be difficult to deal with this. > >Of course, using PC mode when trying to deal with this >platform/board-requirement would also be suboptimal. In other words, >your argument about when using OSI vs PC mode, still stands. > I understand the arguments for using PC vs OSI and agree with it. But what in PSCI is against Linux knowing when the last core is powering down when the PSCI is configured to do only Platform Cordinated. There should not be any objection to drivers knowing when all the cores are powered down, be it reference counting CPU PM notifications or using a cleaner approach like this where GendPD framwork does everything cleanly and gives a nice callback. ARM architecture allows for different aspects of CPU access be handled at different levels. I see this as an extension of that approach. -- Lina
Changes in v3: - Address Rob's comment on dt property value - Address Stephen's comments on rpmh-rsc driver change - Include sc7180 cpuidle low power mode changes from [1] - Include hierarchical domain idle states converter change from [2] Changes in v2: - Add Stephen's Reviewed-By to the first three patches - Addressed Stephen's comments on fourth patch - Include changes to connect rpmh domain to cpuidle and genpds Resource State Coordinator (RSC) is responsible for powering off/lowering the requirements from CPU subsystem for the associated hardware like buses, clocks, and regulators when all CPUs and cluster is powered down. RSC power domain uses last-man activities provided by genpd framework based on Ulf Hansoon's patch series[3], when the cluster of CPUs enter deepest idle states. As a part of domain poweroff, RSC can lower resource state requirements by flushing the cached sleep and wake state votes for resources. [1] https://patchwork.kernel.org/patch/11218965 [2] https://patchwork.kernel.org/patch/10941671 [3] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=222355 Maulik Shah (6): drivers: qcom: rpmh: fix macro to accept NULL argument drivers: qcom: rpmh: remove rpmh_flush export dt-bindings: soc: qcom: Add RSC power domain specifier drivers: qcom: rpmh-rsc: Add RSC power domain support arm64: dts: qcom: sc7180: Add cpuidle low power states arm64: dts: qcom: sc7180: Convert to the hierarchical CPU topology layout Ulf Hansson (1): drivers: firmware: psci: Add hierarchical domain idle states converter .../devicetree/bindings/soc/qcom/rpmh-rsc.txt | 9 ++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 133 ++++++++++++++++++++ drivers/cpuidle/cpuidle-psci-domain.c | 137 ++++++++++++++++++--- drivers/cpuidle/cpuidle-psci.c | 41 +++--- drivers/cpuidle/cpuidle-psci.h | 11 ++ drivers/soc/qcom/rpmh-internal.h | 3 + drivers/soc/qcom/rpmh-rsc.c | 81 ++++++++++++ drivers/soc/qcom/rpmh.c | 22 ++-- include/soc/qcom/rpmh.h | 5 - 9 files changed, 389 insertions(+), 53 deletions(-)