| Message ID | 20190513192300.653-8-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM/ARM64: Support hierarchical CPU arrangement for PSCI | expand |
On Mon, May 13, 2019 at 09:22:49PM +0200, Ulf Hansson wrote: > The per CPU variable psci_power_state, contains an array of fixed values, > which reflects the corresponding arm,psci-suspend-param parsed from DT, for > each of the available CPU idle states. > > This isn't sufficient when using the hierarchical CPU topology in DT in > combination with having PSCI OS initiated (OSI) mode enabled. More > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what > idle state the cluster (a group of CPUs) should enter, while in PSCI > Platform Coordinated (PC) mode, each CPU independently votes for an idle > state of the cluster. > > For this reason, let's introduce an additional per CPU variable called > domain_state and implement two helper functions to read/write its values. > Following patches, which implements PM domain support for PSCI, will use > the domain_state variable and set it to corresponding bits that represents > the selected idle state for the cluster. > > Finally, in psci_cpu_suspend_enter() and psci_suspend_finisher(), let's > take into account the values in the domain_state, as to get the complete > suspend parameter. > I understand it was split to ease review, but this patch also does nothing as domain_state = 0 always. I was trying hard to find where it's set, but I assume it will be done in later patches. Again may be this can be squashed into the first caller of psci_set_domain_state -- Regards, Sudeep
On Fri, 7 Jun 2019 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Mon, May 13, 2019 at 09:22:49PM +0200, Ulf Hansson wrote: > > The per CPU variable psci_power_state, contains an array of fixed values, > > which reflects the corresponding arm,psci-suspend-param parsed from DT, for > > each of the available CPU idle states. > > > > This isn't sufficient when using the hierarchical CPU topology in DT in > > combination with having PSCI OS initiated (OSI) mode enabled. More > > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what > > idle state the cluster (a group of CPUs) should enter, while in PSCI > > Platform Coordinated (PC) mode, each CPU independently votes for an idle > > state of the cluster. > > > > For this reason, let's introduce an additional per CPU variable called > > domain_state and implement two helper functions to read/write its values. > > Following patches, which implements PM domain support for PSCI, will use > > the domain_state variable and set it to corresponding bits that represents > > the selected idle state for the cluster. > > > > Finally, in psci_cpu_suspend_enter() and psci_suspend_finisher(), let's > > take into account the values in the domain_state, as to get the complete > > suspend parameter. > > > > I understand it was split to ease review, but this patch also does > nothing as domain_state = 0 always. I was trying hard to find where it's > set, but I assume it will be done in later patches. Again may be this > can be squashed into the first caller of psci_set_domain_state You have a point, but I am worried that it would look like this series is solely needed to support OSI mode. This is not the case. Let me explain. Having $subject patch separate shows the specific changes needed to support OSI mode. The first caller of psci_set_domain_state() is added in patch9, however, patch9 is useful no matter of OSI or PC mode. Moreover, if I squash $subject patch with patch9, I would have to squash also the subsequent patch (patch8), as it depends on $subject patch. So, to conclude, are you happy with this as is or do you want me to squash the patches? Kind regards Uffe
On Mon, Jun 10, 2019 at 12:21:10PM +0200, Ulf Hansson wrote: > On Fri, 7 Jun 2019 at 17:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Mon, May 13, 2019 at 09:22:49PM +0200, Ulf Hansson wrote: > > > The per CPU variable psci_power_state, contains an array of fixed values, > > > which reflects the corresponding arm,psci-suspend-param parsed from DT, for > > > each of the available CPU idle states. > > > > > > This isn't sufficient when using the hierarchical CPU topology in DT in > > > combination with having PSCI OS initiated (OSI) mode enabled. More > > > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what > > > idle state the cluster (a group of CPUs) should enter, while in PSCI > > > Platform Coordinated (PC) mode, each CPU independently votes for an idle > > > state of the cluster. > > > > > > For this reason, let's introduce an additional per CPU variable called > > > domain_state and implement two helper functions to read/write its values. > > > Following patches, which implements PM domain support for PSCI, will use > > > the domain_state variable and set it to corresponding bits that represents > > > the selected idle state for the cluster. > > > > > > Finally, in psci_cpu_suspend_enter() and psci_suspend_finisher(), let's > > > take into account the values in the domain_state, as to get the complete > > > suspend parameter. > > > > > > > I understand it was split to ease review, but this patch also does > > nothing as domain_state = 0 always. I was trying hard to find where it's > > set, but I assume it will be done in later patches. Again may be this > > can be squashed into the first caller of psci_set_domain_state > > You have a point, but I am worried that it would look like this series > is solely needed to support OSI mode. This is not the case. Let me > explain. > > Having $subject patch separate shows the specific changes needed to > support OSI mode. The first caller of psci_set_domain_state() is added > in patch9, however, patch9 is useful no matter of OSI or PC mode. > > Moreover, if I squash $subject patch with patch9, I would have to > squash also the subsequent patch (patch8), as it depends on $subject > patch. > > So, to conclude, are you happy with this as is or do you want me to > squash the patches? > Yes I am fine either way. As I put the comments in the same flow as I did review, I thought it's worth mentioning if someone else get similar thoughts. I am fine if you prefer to keep it the same way unless someone else raise the same point. -- Regards, Sudeep
On Mon, May 13, 2019 at 09:22:49PM +0200, Ulf Hansson wrote: > The per CPU variable psci_power_state, contains an array of fixed values, > which reflects the corresponding arm,psci-suspend-param parsed from DT, for > each of the available CPU idle states. > > This isn't sufficient when using the hierarchical CPU topology in DT in > combination with having PSCI OS initiated (OSI) mode enabled. More > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what > idle state the cluster (a group of CPUs) should enter, while in PSCI > Platform Coordinated (PC) mode, each CPU independently votes for an idle > state of the cluster. > > For this reason, let's introduce an additional per CPU variable called > domain_state and implement two helper functions to read/write its values. > Following patches, which implements PM domain support for PSCI, will use > the domain_state variable and set it to corresponding bits that represents > the selected idle state for the cluster. > > Finally, in psci_cpu_suspend_enter() and psci_suspend_finisher(), let's > take into account the values in the domain_state, as to get the complete > suspend parameter. > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > > Changes: > - Clarify changelog. > - Drop changes in psci_cpu_on() as it belongs in the patch for hotplug. > - Move some code inside "#ifdef CONFIG_CPU_IDLE". > > --- > drivers/firmware/psci/psci.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c > index b11560f7c4b9..4aec513136e4 100644 > --- a/drivers/firmware/psci/psci.c > +++ b/drivers/firmware/psci/psci.c > @@ -285,6 +285,17 @@ static int __init psci_features(u32 psci_func_id) > > #ifdef CONFIG_CPU_IDLE > static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); > +static DEFINE_PER_CPU(u32, domain_state); > + > +static inline u32 psci_get_domain_state(void) > +{ > + return __this_cpu_read(domain_state); > +} > + > +static inline void psci_set_domain_state(u32 state) > +{ > + __this_cpu_write(domain_state, state); > +} > > static int psci_dt_parse_state_node(struct device_node *np, u32 *state) > { > @@ -420,15 +431,17 @@ int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu) > static int psci_suspend_finisher(unsigned long index) > { > u32 *state = __this_cpu_read(psci_power_state); > + u32 composite_state = state[index - 1] | psci_get_domain_state(); > The more I read this code and PSCI spec, I think it's not simple OR here unless the specification states that. ACPI LPI explicitly stated that as it was generic and PSCI doesn't. It can be made workable for original format, but I think it's not that simple for extended format unless the suspend parameters are carefully designed to achieve that, so we can't just convert existing platforms the way it's shown on HiKey in this series. -- Regards, Sudeep
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c index b11560f7c4b9..4aec513136e4 100644 --- a/drivers/firmware/psci/psci.c +++ b/drivers/firmware/psci/psci.c @@ -285,6 +285,17 @@ static int __init psci_features(u32 psci_func_id) #ifdef CONFIG_CPU_IDLE static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); +static DEFINE_PER_CPU(u32, domain_state); + +static inline u32 psci_get_domain_state(void) +{ + return __this_cpu_read(domain_state); +} + +static inline void psci_set_domain_state(u32 state) +{ + __this_cpu_write(domain_state, state); +} static int psci_dt_parse_state_node(struct device_node *np, u32 *state) { @@ -420,15 +431,17 @@ int psci_cpu_init_idle(struct cpuidle_driver *drv, unsigned int cpu) static int psci_suspend_finisher(unsigned long index) { u32 *state = __this_cpu_read(psci_power_state); + u32 composite_state = state[index - 1] | psci_get_domain_state(); - return psci_ops.cpu_suspend(state[index - 1], - __pa_symbol(cpu_resume)); + return psci_ops.cpu_suspend(composite_state, __pa_symbol(cpu_resume)); } int psci_cpu_suspend_enter(unsigned long index) { int ret; u32 *state = __this_cpu_read(psci_power_state); + u32 composite_state = state[index - 1] | psci_get_domain_state(); + /* * idle state index 0 corresponds to wfi, should never be called * from the cpu_suspend operations @@ -436,11 +449,14 @@ int psci_cpu_suspend_enter(unsigned long index) if (WARN_ON_ONCE(!index)) return -EINVAL; - if (!psci_power_state_loses_context(state[index - 1])) - ret = psci_ops.cpu_suspend(state[index - 1], 0); + if (!psci_power_state_loses_context(composite_state)) + ret = psci_ops.cpu_suspend(composite_state, 0); else ret = cpu_suspend(index, psci_suspend_finisher); + /* Clear the domain state to start fresh when back from idle. */ + psci_set_domain_state(0); + return ret; }