| Message ID | 20200814123436.61851-1-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpuidle: psci: Allow PM domain to be initialized even if no OSI mode | expand |
On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > topology with the genpd providers isn't initialized. This is perfectly fine > from cpuidle-psci point of view. > Indeed. > However, since the PM domain topology in the DTS files is a description of > the HW, no matter of whether the PSCI OSI mode is supported or not, other > consumers besides the CPUs may rely on it. > And why are they even registered as part of cpuidle-psci-domain ? If they have to be, can be decouple it completely from cpuidle then ? > Therefore, let's always allow the initialization of the PM domain topology > to succeed, independently of whether the PSCI OSI mode is supported. > Consequentially we need to track if we succeed to enable the OSI mode, as > to know when a domain idlestate can be selected. > I thought we had discussed this in past, why are we back to the same discussion ? I may need to read those again to get the context. > Note that, CPU devices are still not being attached to the PM domain > topology, unless the PSCI OSI mode is supported. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > 1 file changed, 24 insertions(+), 25 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > index b6e9649ab0da..55653c110e3a 100644 > --- a/drivers/cpuidle/cpuidle-psci-domain.c > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > static LIST_HEAD(psci_pd_providers); > static bool psci_pd_allow_domain_state; > +static bool psci_osi_mode_enabled; > > static int psci_pd_power_off(struct generic_pm_domain *pd) > { > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > if (!state->data) > return 0; > > - if (!psci_pd_allow_domain_state) > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) I really don't like this check. Why do we have to keep checking psci_osi_mode_enabled every single time and that is the reason IIRC I was against this and just don't add the domains. -- Regards, Sudeep
On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > topology with the genpd providers isn't initialized. This is perfectly fine > > from cpuidle-psci point of view. > > > > Indeed. > > > However, since the PM domain topology in the DTS files is a description of > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > consumers besides the CPUs may rely on it. > > > > And why are they even registered as part of cpuidle-psci-domain ? > If they have to be, can be decouple it completely from cpuidle then ? These devices can't be decoupled as they are a part of the CPU cluster PM domain. This is for example the case RPMH (rsc) device for Qcom platforms, but there are other platforms that need this too. > > > Therefore, let's always allow the initialization of the PM domain topology > > to succeed, independently of whether the PSCI OSI mode is supported. > > Consequentially we need to track if we succeed to enable the OSI mode, as > > to know when a domain idlestate can be selected. > > > > I thought we had discussed this in past, why are we back to the same > discussion ? I may need to read those again to get the context. That discussion was according to my understanding about whether we should allow CPU devices to be managed by runtime PM and the CPU PM domains, if OSI was *not* supported. We concluded that we didn't want to allow that, which makes sense - and I am not changing that in $subject patch. > > > Note that, CPU devices are still not being attached to the PM domain > > topology, unless the PSCI OSI mode is supported. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > index b6e9649ab0da..55653c110e3a 100644 > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > static LIST_HEAD(psci_pd_providers); > > static bool psci_pd_allow_domain_state; > > +static bool psci_osi_mode_enabled; > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > { > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > if (!state->data) > > return 0; > > > > - if (!psci_pd_allow_domain_state) > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > I really don't like this check. Why do we have to keep checking > psci_osi_mode_enabled every single time and that is the reason IIRC > I was against this and just don't add the domains. You have a point about the check, it's not very nice - but from an execution point of view, I don't think it's the end of the world. Note that, when not using OSI, then the ->power_off() callback will not be invoked in the cpuidle path. Anyway, if you like, I can try to rework the code, so that the ->power_off() callback doesn't get assigned, if we are not using OSI. Although, I am not sure the trouble is worth it, as I probably need to try to enable OSI before initializing the genpd data structures. Then, if failing with genpd initializations, I need to revert back to PC mode. What do you think? Kind regards Uffe
On Wed, 19 Aug 2020 at 10:20, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > from cpuidle-psci point of view. > > > > > > > Indeed. > > > > > However, since the PM domain topology in the DTS files is a description of > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > consumers besides the CPUs may rely on it. > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > If they have to be, can be decouple it completely from cpuidle then ? > > These devices can't be decoupled as they are a part of the CPU cluster > PM domain. > > This is for example the case RPMH (rsc) device for Qcom platforms, but > there are other platforms that need this too. > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > to know when a domain idlestate can be selected. > > > > > > > I thought we had discussed this in past, why are we back to the same > > discussion ? I may need to read those again to get the context. > > That discussion was according to my understanding about whether we > should allow CPU devices to be managed by runtime PM and the CPU PM > domains, if OSI was *not* supported. > > We concluded that we didn't want to allow that, which makes sense - > and I am not changing that in $subject patch. > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > topology, unless the PSCI OSI mode is supported. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > index b6e9649ab0da..55653c110e3a 100644 > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > static LIST_HEAD(psci_pd_providers); > > > static bool psci_pd_allow_domain_state; > > > +static bool psci_osi_mode_enabled; > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > { > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > if (!state->data) > > > return 0; > > > > > > - if (!psci_pd_allow_domain_state) > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > I really don't like this check. Why do we have to keep checking > > psci_osi_mode_enabled every single time and that is the reason IIRC > > I was against this and just don't add the domains. > > You have a point about the check, it's not very nice - but from an > execution point of view, I don't think it's the end of the world. > > Note that, when not using OSI, then the ->power_off() callback will > not be invoked in the cpuidle path. > > Anyway, if you like, I can try to rework the code, so that the > ->power_off() callback doesn't get assigned, if we are not using OSI. > Although, I am not sure the trouble is worth it, as I probably need to > try to enable OSI before initializing the genpd data structures. Then, > if failing with genpd initializations, I need to revert back to PC > mode. > > What do you think? Sudeep, do any further comments on this? Or you want to give it your blessings so Rafael can pick it up? Kind regards Uffe
On Wed, Aug 19, 2020 at 10:20:52AM +0200, Ulf Hansson wrote: > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > from cpuidle-psci point of view. > > > > > > > Indeed. > > > > > However, since the PM domain topology in the DTS files is a description of > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > consumers besides the CPUs may rely on it. > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > If they have to be, can be decouple it completely from cpuidle then ? > > These devices can't be decoupled as they are a part of the CPU cluster > PM domain. > > This is for example the case RPMH (rsc) device for Qcom platforms, but > there are other platforms that need this too. > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > to know when a domain idlestate can be selected. > > > > > > > I thought we had discussed this in past, why are we back to the same > > discussion ? I may need to read those again to get the context. > > That discussion was according to my understanding about whether we > should allow CPU devices to be managed by runtime PM and the CPU PM > domains, if OSI was *not* supported. > > We concluded that we didn't want to allow that, which makes sense - > and I am not changing that in $subject patch. > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > topology, unless the PSCI OSI mode is supported. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > index b6e9649ab0da..55653c110e3a 100644 > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > static LIST_HEAD(psci_pd_providers); > > > static bool psci_pd_allow_domain_state; > > > +static bool psci_osi_mode_enabled; > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > { > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > if (!state->data) > > > return 0; > > > > > > - if (!psci_pd_allow_domain_state) > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > I really don't like this check. Why do we have to keep checking > > psci_osi_mode_enabled every single time and that is the reason IIRC > > I was against this and just don't add the domains. > > You have a point about the check, it's not very nice - but from an > execution point of view, I don't think it's the end of the world. > > Note that, when not using OSI, then the ->power_off() callback will > not be invoked in the cpuidle path. > Then drop the check. I am confused as why we need that runtime check if we get the setup right. > Anyway, if you like, I can try to rework the code, so that the > ->power_off() callback doesn't get assigned, if we are not using OSI. +1 for sure. > Although, I am not sure the trouble is worth it, as I probably need to > try to enable OSI before initializing the genpd data structures. Then, > if failing with genpd initializations, I need to revert back to PC > mode. > Just to clarify, you can use psci_osi_mode_enabled anytime during initialisation and get the setup right so that we can drop unnecessary runtime check every single poweroff call. I prefer to remove that. -- Regards, Sudeep
On Tue, Sep 01, 2020 at 09:04:56AM +0200, Ulf Hansson wrote: > On Wed, 19 Aug 2020 at 10:20, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > > from cpuidle-psci point of view. > > > > > > > > > > Indeed. > > > > > > > However, since the PM domain topology in the DTS files is a description of > > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > > consumers besides the CPUs may rely on it. > > > > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > > If they have to be, can be decouple it completely from cpuidle then ? > > > > These devices can't be decoupled as they are a part of the CPU cluster > > PM domain. > > > > This is for example the case RPMH (rsc) device for Qcom platforms, but > > there are other platforms that need this too. > > > > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > > to know when a domain idlestate can be selected. > > > > > > > > > > I thought we had discussed this in past, why are we back to the same > > > discussion ? I may need to read those again to get the context. > > > > That discussion was according to my understanding about whether we > > should allow CPU devices to be managed by runtime PM and the CPU PM > > domains, if OSI was *not* supported. > > > > We concluded that we didn't want to allow that, which makes sense - > > and I am not changing that in $subject patch. > > > > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > > topology, unless the PSCI OSI mode is supported. > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > --- > > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > index b6e9649ab0da..55653c110e3a 100644 > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > > > static LIST_HEAD(psci_pd_providers); > > > > static bool psci_pd_allow_domain_state; > > > > +static bool psci_osi_mode_enabled; > > > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > { > > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > if (!state->data) > > > > return 0; > > > > > > > > - if (!psci_pd_allow_domain_state) > > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > > > I really don't like this check. Why do we have to keep checking > > > psci_osi_mode_enabled every single time and that is the reason IIRC > > > I was against this and just don't add the domains. > > > > You have a point about the check, it's not very nice - but from an > > execution point of view, I don't think it's the end of the world. > > > > Note that, when not using OSI, then the ->power_off() callback will > > not be invoked in the cpuidle path. > > > > Anyway, if you like, I can try to rework the code, so that the > > ->power_off() callback doesn't get assigned, if we are not using OSI. > > Although, I am not sure the trouble is worth it, as I probably need to > > try to enable OSI before initializing the genpd data structures. Then, > > if failing with genpd initializations, I need to revert back to PC > > mode. > > > > What do you think? > > Sudeep, do any further comments on this? Or you want to give it your > blessings so Rafael can pick it up? > Sorry I got confused as you posted some generic changes to PM domains and I didn't get time to look at the yet, so was postponing to reply to this. -- Regards, Sudeep
On Tue, 1 Sep 2020 at 11:01, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Aug 19, 2020 at 10:20:52AM +0200, Ulf Hansson wrote: > > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > > from cpuidle-psci point of view. > > > > > > > > > > Indeed. > > > > > > > However, since the PM domain topology in the DTS files is a description of > > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > > consumers besides the CPUs may rely on it. > > > > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > > If they have to be, can be decouple it completely from cpuidle then ? > > > > These devices can't be decoupled as they are a part of the CPU cluster > > PM domain. > > > > This is for example the case RPMH (rsc) device for Qcom platforms, but > > there are other platforms that need this too. > > > > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > > to know when a domain idlestate can be selected. > > > > > > > > > > I thought we had discussed this in past, why are we back to the same > > > discussion ? I may need to read those again to get the context. > > > > That discussion was according to my understanding about whether we > > should allow CPU devices to be managed by runtime PM and the CPU PM > > domains, if OSI was *not* supported. > > > > We concluded that we didn't want to allow that, which makes sense - > > and I am not changing that in $subject patch. > > > > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > > topology, unless the PSCI OSI mode is supported. > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > --- > > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > index b6e9649ab0da..55653c110e3a 100644 > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > > > static LIST_HEAD(psci_pd_providers); > > > > static bool psci_pd_allow_domain_state; > > > > +static bool psci_osi_mode_enabled; > > > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > { > > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > if (!state->data) > > > > return 0; > > > > > > > > - if (!psci_pd_allow_domain_state) > > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > > > I really don't like this check. Why do we have to keep checking > > > psci_osi_mode_enabled every single time and that is the reason IIRC > > > I was against this and just don't add the domains. > > > > You have a point about the check, it's not very nice - but from an > > execution point of view, I don't think it's the end of the world. > > > > Note that, when not using OSI, then the ->power_off() callback will > > not be invoked in the cpuidle path. > > > > Then drop the check. I am confused as why we need that runtime check if > we get the setup right. > > > Anyway, if you like, I can try to rework the code, so that the > > ->power_off() callback doesn't get assigned, if we are not using OSI. > > +1 for sure. > > > Although, I am not sure the trouble is worth it, as I probably need to > > try to enable OSI before initializing the genpd data structures. Then, > > if failing with genpd initializations, I need to revert back to PC > > mode. > > > > Just to clarify, you can use psci_osi_mode_enabled anytime during > initialisation and get the setup right so that we can drop unnecessary > runtime check every single poweroff call. I prefer to remove that. Okay, I will respin the series then. Although, just to be clear, this means that I will have to invoke psci_set_osi_mode() prior to initializing the PM domains. Then if it turns out that initializing the PM domain fails, I will have to switch back to PC mode. To do that, I will probably need to extend psci_set_osi_mode() to take a bool as a parameter (to indicate enable or disable OSI mode). You are okay with this, right? Kind regards Uffe
On Tue, Sep 01, 2020 at 11:43:34AM +0200, Ulf Hansson wrote: > On Tue, 1 Sep 2020 at 11:01, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Aug 19, 2020 at 10:20:52AM +0200, Ulf Hansson wrote: > > > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > > > from cpuidle-psci point of view. > > > > > > > > > > > > > Indeed. > > > > > > > > > However, since the PM domain topology in the DTS files is a description of > > > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > > > consumers besides the CPUs may rely on it. > > > > > > > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > > > If they have to be, can be decouple it completely from cpuidle then ? > > > > > > These devices can't be decoupled as they are a part of the CPU cluster > > > PM domain. > > > > > > This is for example the case RPMH (rsc) device for Qcom platforms, but > > > there are other platforms that need this too. > > > > > > > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > > > to know when a domain idlestate can be selected. > > > > > > > > > > > > > I thought we had discussed this in past, why are we back to the same > > > > discussion ? I may need to read those again to get the context. > > > > > > That discussion was according to my understanding about whether we > > > should allow CPU devices to be managed by runtime PM and the CPU PM > > > domains, if OSI was *not* supported. > > > > > > We concluded that we didn't want to allow that, which makes sense - > > > and I am not changing that in $subject patch. > > > > > > > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > > > topology, unless the PSCI OSI mode is supported. > > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > --- > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > index b6e9649ab0da..55653c110e3a 100644 > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > > > > > static LIST_HEAD(psci_pd_providers); > > > > > static bool psci_pd_allow_domain_state; > > > > > +static bool psci_osi_mode_enabled; > > > > > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > { > > > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > if (!state->data) > > > > > return 0; > > > > > > > > > > - if (!psci_pd_allow_domain_state) > > > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > > > > > I really don't like this check. Why do we have to keep checking > > > > psci_osi_mode_enabled every single time and that is the reason IIRC > > > > I was against this and just don't add the domains. > > > > > > You have a point about the check, it's not very nice - but from an > > > execution point of view, I don't think it's the end of the world. > > > > > > Note that, when not using OSI, then the ->power_off() callback will > > > not be invoked in the cpuidle path. > > > > > > > Then drop the check. I am confused as why we need that runtime check if > > we get the setup right. > > > > > Anyway, if you like, I can try to rework the code, so that the > > > ->power_off() callback doesn't get assigned, if we are not using OSI. > > > > +1 for sure. > > > > > Although, I am not sure the trouble is worth it, as I probably need to > > > try to enable OSI before initializing the genpd data structures. Then, > > > if failing with genpd initializations, I need to revert back to PC > > > mode. > > > > > > > Just to clarify, you can use psci_osi_mode_enabled anytime during > > initialisation and get the setup right so that we can drop unnecessary > > runtime check every single poweroff call. I prefer to remove that. > > Okay, I will respin the series then. > > Although, just to be clear, this means that I will have to invoke > psci_set_osi_mode() prior to initializing the PM domains. Then if it > turns out that initializing the PM domain fails, I will have to switch > back to PC mode. > > To do that, I will probably need to extend psci_set_osi_mode() to take > a bool as a parameter (to indicate enable or disable OSI mode). You > are okay with this, right? > I am fine with that. I assume by disable OSI mode, you are referring to switch back to PC mode. -- Regards, Sudeep
On Tue, 1 Sep 2020 at 12:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Tue, Sep 01, 2020 at 11:43:34AM +0200, Ulf Hansson wrote: > > On Tue, 1 Sep 2020 at 11:01, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Aug 19, 2020 at 10:20:52AM +0200, Ulf Hansson wrote: > > > > On Tue, 18 Aug 2020 at 14:35, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > > > On Fri, Aug 14, 2020 at 02:34:36PM +0200, Ulf Hansson wrote: > > > > > > If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain > > > > > > topology with the genpd providers isn't initialized. This is perfectly fine > > > > > > from cpuidle-psci point of view. > > > > > > > > > > > > > > > > Indeed. > > > > > > > > > > > However, since the PM domain topology in the DTS files is a description of > > > > > > the HW, no matter of whether the PSCI OSI mode is supported or not, other > > > > > > consumers besides the CPUs may rely on it. > > > > > > > > > > > > > > > > And why are they even registered as part of cpuidle-psci-domain ? > > > > > If they have to be, can be decouple it completely from cpuidle then ? > > > > > > > > These devices can't be decoupled as they are a part of the CPU cluster > > > > PM domain. > > > > > > > > This is for example the case RPMH (rsc) device for Qcom platforms, but > > > > there are other platforms that need this too. > > > > > > > > > > > > > > > Therefore, let's always allow the initialization of the PM domain topology > > > > > > to succeed, independently of whether the PSCI OSI mode is supported. > > > > > > Consequentially we need to track if we succeed to enable the OSI mode, as > > > > > > to know when a domain idlestate can be selected. > > > > > > > > > > > > > > > > I thought we had discussed this in past, why are we back to the same > > > > > discussion ? I may need to read those again to get the context. > > > > > > > > That discussion was according to my understanding about whether we > > > > should allow CPU devices to be managed by runtime PM and the CPU PM > > > > domains, if OSI was *not* supported. > > > > > > > > We concluded that we didn't want to allow that, which makes sense - > > > > and I am not changing that in $subject patch. > > > > > > > > > > > > > > > Note that, CPU devices are still not being attached to the PM domain > > > > > > topology, unless the PSCI OSI mode is supported. > > > > > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > --- > > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- > > > > > > 1 file changed, 24 insertions(+), 25 deletions(-) > > > > > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > index b6e9649ab0da..55653c110e3a 100644 > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > @@ -28,6 +28,7 @@ struct psci_pd_provider { > > > > > > > > > > > > static LIST_HEAD(psci_pd_providers); > > > > > > static bool psci_pd_allow_domain_state; > > > > > > +static bool psci_osi_mode_enabled; > > > > > > > > > > > > static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > > { > > > > > > @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > > if (!state->data) > > > > > > return 0; > > > > > > > > > > > > - if (!psci_pd_allow_domain_state) > > > > > > + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) > > > > > > > > > > I really don't like this check. Why do we have to keep checking > > > > > psci_osi_mode_enabled every single time and that is the reason IIRC > > > > > I was against this and just don't add the domains. > > > > > > > > You have a point about the check, it's not very nice - but from an > > > > execution point of view, I don't think it's the end of the world. > > > > > > > > Note that, when not using OSI, then the ->power_off() callback will > > > > not be invoked in the cpuidle path. > > > > > > > > > > Then drop the check. I am confused as why we need that runtime check if > > > we get the setup right. > > > > > > > Anyway, if you like, I can try to rework the code, so that the > > > > ->power_off() callback doesn't get assigned, if we are not using OSI. > > > > > > +1 for sure. > > > > > > > Although, I am not sure the trouble is worth it, as I probably need to > > > > try to enable OSI before initializing the genpd data structures. Then, > > > > if failing with genpd initializations, I need to revert back to PC > > > > mode. > > > > > > > > > > Just to clarify, you can use psci_osi_mode_enabled anytime during > > > initialisation and get the setup right so that we can drop unnecessary > > > runtime check every single poweroff call. I prefer to remove that. > > > > Okay, I will respin the series then. > > > > Although, just to be clear, this means that I will have to invoke > > psci_set_osi_mode() prior to initializing the PM domains. Then if it > > turns out that initializing the PM domain fails, I will have to switch > > back to PC mode. > > > > To do that, I will probably need to extend psci_set_osi_mode() to take > > a bool as a parameter (to indicate enable or disable OSI mode). You > > are okay with this, right? > > > > I am fine with that. I assume by disable OSI mode, you are referring to > switch back to PC mode. Correct. New version is about to be posted. Kind regards Uffe
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c index b6e9649ab0da..55653c110e3a 100644 --- a/drivers/cpuidle/cpuidle-psci-domain.c +++ b/drivers/cpuidle/cpuidle-psci-domain.c @@ -28,6 +28,7 @@ struct psci_pd_provider { static LIST_HEAD(psci_pd_providers); static bool psci_pd_allow_domain_state; +static bool psci_osi_mode_enabled; static int psci_pd_power_off(struct generic_pm_domain *pd) { @@ -37,7 +38,7 @@ static int psci_pd_power_off(struct generic_pm_domain *pd) if (!state->data) return 0; - if (!psci_pd_allow_domain_state) + if (!psci_pd_allow_domain_state || !psci_osi_mode_enabled) return -EBUSY; /* OSI mode is enabled, set the corresponding domain state. */ @@ -190,7 +191,7 @@ static void psci_pd_remove(void) } } -static int psci_pd_init_topology(struct device_node *np, bool add) +static int psci_pd_init_topology(struct device_node *np) { struct device_node *node; struct of_phandle_args child, parent; @@ -203,9 +204,7 @@ static int psci_pd_init_topology(struct device_node *np, bool add) child.np = node; child.args_count = 0; - - ret = add ? of_genpd_add_subdomain(&parent, &child) : - of_genpd_remove_subdomain(&parent, &child); + ret = of_genpd_add_subdomain(&parent, &child); of_node_put(parent.np); if (ret) { of_node_put(node); @@ -216,14 +215,21 @@ static int psci_pd_init_topology(struct device_node *np, bool add) return 0; } -static int psci_pd_add_topology(struct device_node *np) +static void psci_pd_try_set_osi_mode(void) { - return psci_pd_init_topology(np, true); -} + int ret; -static void psci_pd_remove_topology(struct device_node *np) -{ - psci_pd_init_topology(np, false); + if (!psci_has_osi_support()) + return; + + /* Try to enable OSI mode. */ + ret = psci_set_osi_mode(); + if (ret) { + pr_warn("failed to enable OSI mode: %d\n", ret); + return; + } + + psci_osi_mode_enabled = true; } static void psci_cpuidle_domain_sync_state(struct device *dev) @@ -249,10 +255,6 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) if (!np) return -ENODEV; - /* Currently limit the hierarchical topology to be used in OSI mode. */ - if (!psci_has_osi_support()) - return 0; - /* * Parse child nodes for the "#power-domain-cells" property and * initialize a genpd/genpd-of-provider pair when it's found. @@ -273,17 +275,15 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) return 0; /* Link genpd masters/subdomains to model the CPU topology. */ - ret = psci_pd_add_topology(np); + ret = psci_pd_init_topology(np); if (ret) goto remove_pd; - /* Try to enable OSI mode. */ - ret = psci_set_osi_mode(); - if (ret) { - pr_warn("failed to enable OSI mode: %d\n", ret); - psci_pd_remove_topology(np); - goto remove_pd; - } + /* + * If OSI mode isn't supported, the topology isn't used by CPUs, but + * it may still be used by other consumers. Leave it initialized. + */ + psci_pd_try_set_osi_mode(); pr_info("Initialized CPU PM domain topology\n"); return 0; @@ -291,8 +291,7 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev) put_node: of_node_put(node); remove_pd: - if (pd_count) - psci_pd_remove(); + psci_pd_remove(); pr_err("failed to create CPU PM domains ret=%d\n", ret); return ret; }
If the PSCI OSI mode isn't supported or fails to be enabled, the PM domain topology with the genpd providers isn't initialized. This is perfectly fine from cpuidle-psci point of view. However, since the PM domain topology in the DTS files is a description of the HW, no matter of whether the PSCI OSI mode is supported or not, other consumers besides the CPUs may rely on it. Therefore, let's always allow the initialization of the PM domain topology to succeed, independently of whether the PSCI OSI mode is supported. Consequentially we need to track if we succeed to enable the OSI mode, as to know when a domain idlestate can be selected. Note that, CPU devices are still not being attached to the PM domain topology, unless the PSCI OSI mode is supported. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/cpuidle/cpuidle-psci-domain.c | 49 +++++++++++++-------------- 1 file changed, 24 insertions(+), 25 deletions(-) -- 2.25.1