| Message ID | 1521046715-30683-21-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14 March 2018 at 18:38, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Mar 14, 2018 at 05:58:30PM +0100, Ulf Hansson wrote: >> In case the hierarchical layout is used in DT, we want to initialize the >> corresponding PM domain topology for the CPUs, by using the generic PM >> domain (aka genpd) infrastructure. >> >> At first glance, it may seem feasible to hook into the existing >> psci_dt_init() function, although because it's called quite early in the >> boot sequence, allocating the dynamic data structure for a genpd doesn't >> work. >> >> Therefore, let's export a new init function for PSCI, >> psci_dt_topology_init(), which the ARM machine code should call from a >> suitable initcall. >> >> Succeeding to initialize the PM domain topology, which means at least one >> instance of a genpd becomes created, allows us to continue to enable the >> PSCI OS initiated mode for the platform. If everything turns out fine, >> let's print a message in log to inform the user about the changed mode. >> >> In case of any failures, we stick to the default PSCI Platform Coordinated >> mode. > > For kexec/kdump we'll need to explicitly set the suspend mode to > platform coordinated if for whatever reason we choose not to use OSI. Could you please elaborate on this? I am not really understanding what you are suggesting me to do and why. > >> >> Cc: Lina Iyer <ilina@codeaurora.org> >> Co-developed-by: Lina Iyer <lina.iyer@linaro.org> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/firmware/psci.c | 31 +++++++++++++++++++++++++++++++ >> include/linux/psci.h | 2 ++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c >> index 463f78c..45d55fc 100644 >> --- a/drivers/firmware/psci.c >> +++ b/drivers/firmware/psci.c >> @@ -723,6 +723,37 @@ int __init psci_dt_init(void) >> return ret; >> } >> >> +int __init psci_dt_topology_init(void) >> +{ >> + struct device_node *np; >> + int ret; >> + >> + if (!psci_has_osi_support()) >> + return 0; >> + >> + np = of_find_matching_node_and_match(NULL, psci_of_match, NULL); >> + if (!np) >> + return -ENODEV; >> + >> + /* Initialize the CPU PM domains based on topology described in DT. */ >> + ret = psci_dt_init_pm_domains(np); >> + if (ret <= 0) >> + goto out; >> + >> + /* Enable OSI mode. */ >> + ret = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, >> + PSCI_1_0_SUSPEND_MODE_OSI, 0, 0); > > IIUC, this patch will need to be moved after the subsequent two patches > to ensure that this series bisects cleanly. Actually not. $subject patch adds a new init function, which purpose is to initialize the PSCI PM domains. At this point none is calling it. The following changes, builds on top, making it possible to add CPU devices to the PSCI PM domains (if the PM domains has been created and OSI mode successfully enabled) and also to properly deal CPU hotplug when OSI mode is enabled. The purpose doing it this way is to keep changes small and review-able, but in principle I can squash 20, 21 and 22 - if you prefer. Again, thanks for reviewing! Kind regards Uffe
On Tue, Apr 10, 2018 at 09:19:26AM +0200, Ulf Hansson wrote: > On 14 March 2018 at 18:38, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Mar 14, 2018 at 05:58:30PM +0100, Ulf Hansson wrote: > >> In case the hierarchical layout is used in DT, we want to initialize the > >> corresponding PM domain topology for the CPUs, by using the generic PM > >> domain (aka genpd) infrastructure. > >> > >> At first glance, it may seem feasible to hook into the existing > >> psci_dt_init() function, although because it's called quite early in the > >> boot sequence, allocating the dynamic data structure for a genpd doesn't > >> work. > >> > >> Therefore, let's export a new init function for PSCI, > >> psci_dt_topology_init(), which the ARM machine code should call from a > >> suitable initcall. > >> > >> Succeeding to initialize the PM domain topology, which means at least one > >> instance of a genpd becomes created, allows us to continue to enable the > >> PSCI OS initiated mode for the platform. If everything turns out fine, > >> let's print a message in log to inform the user about the changed mode. > >> > >> In case of any failures, we stick to the default PSCI Platform Coordinated > >> mode. > > > > For kexec/kdump we'll need to explicitly set the suspend mode to > > platform coordinated if for whatever reason we choose not to use OSI. > > Could you please elaborate on this? I am not really understanding what > you are suggesting me to do and why. Sorry for not being clear. What I'd like to see is that we always call SET_SUSPEND_MODE before invoking CPU_SUSPEND. Either deciding early if we can use OSI, or always setting it to platform co-ordinated early on in boot and later switching it over. That way we can be sure of the suspend mode, even if we've kexec'd from a kernel that had fiddled with it. > >> Cc: Lina Iyer <ilina@codeaurora.org> > >> Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >> --- > >> drivers/firmware/psci.c | 31 +++++++++++++++++++++++++++++++ > >> include/linux/psci.h | 2 ++ > >> 2 files changed, 33 insertions(+) > >> > >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > >> index 463f78c..45d55fc 100644 > >> --- a/drivers/firmware/psci.c > >> +++ b/drivers/firmware/psci.c > >> @@ -723,6 +723,37 @@ int __init psci_dt_init(void) > >> return ret; > >> } > >> > >> +int __init psci_dt_topology_init(void) > >> +{ > >> + struct device_node *np; > >> + int ret; > >> + > >> + if (!psci_has_osi_support()) > >> + return 0; > >> + > >> + np = of_find_matching_node_and_match(NULL, psci_of_match, NULL); > >> + if (!np) > >> + return -ENODEV; > >> + > >> + /* Initialize the CPU PM domains based on topology described in DT. */ > >> + ret = psci_dt_init_pm_domains(np); > >> + if (ret <= 0) > >> + goto out; > >> + > >> + /* Enable OSI mode. */ > >> + ret = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, > >> + PSCI_1_0_SUSPEND_MODE_OSI, 0, 0); > > > > IIUC, this patch will need to be moved after the subsequent two patches > > to ensure that this series bisects cleanly. > > Actually not. > > $subject patch adds a new init function, which purpose is to > initialize the PSCI PM domains. At this point none is calling it. Ah. I missed the fact there were no callers. Sorry for the noise! Thanks, Mark.
On 10 April 2018 at 13:10, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Apr 10, 2018 at 09:19:26AM +0200, Ulf Hansson wrote: >> On 14 March 2018 at 18:38, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Wed, Mar 14, 2018 at 05:58:30PM +0100, Ulf Hansson wrote: >> >> In case the hierarchical layout is used in DT, we want to initialize the >> >> corresponding PM domain topology for the CPUs, by using the generic PM >> >> domain (aka genpd) infrastructure. >> >> >> >> At first glance, it may seem feasible to hook into the existing >> >> psci_dt_init() function, although because it's called quite early in the >> >> boot sequence, allocating the dynamic data structure for a genpd doesn't >> >> work. >> >> >> >> Therefore, let's export a new init function for PSCI, >> >> psci_dt_topology_init(), which the ARM machine code should call from a >> >> suitable initcall. >> >> >> >> Succeeding to initialize the PM domain topology, which means at least one >> >> instance of a genpd becomes created, allows us to continue to enable the >> >> PSCI OS initiated mode for the platform. If everything turns out fine, >> >> let's print a message in log to inform the user about the changed mode. >> >> >> >> In case of any failures, we stick to the default PSCI Platform Coordinated >> >> mode. >> > >> > For kexec/kdump we'll need to explicitly set the suspend mode to >> > platform coordinated if for whatever reason we choose not to use OSI. >> >> Could you please elaborate on this? I am not really understanding what >> you are suggesting me to do and why. > > Sorry for not being clear. > > What I'd like to see is that we always call SET_SUSPEND_MODE before > invoking CPU_SUSPEND. Either deciding early if we can use OSI, or always > setting it to platform co-ordinated early on in boot and later switching > it over. > > That way we can be sure of the suspend mode, even if we've kexec'd from > a kernel that had fiddled with it. Right, good point. Let's me update the patch to deal with that. One thing though, kexec from a "new" kernel having OSI mode enabled into an "old" kernel that doesn't even have the new OSI mode support in psci driver, is going to be difficult to deal with. On the other hand I guess that isn't a very important usecase we need to cover, or is it? [...] Kind regards Uffe
On Tue, Apr 10, 2018 at 02:25:52PM +0200, Ulf Hansson wrote: > On 10 April 2018 at 13:10, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Apr 10, 2018 at 09:19:26AM +0200, Ulf Hansson wrote: > >> On 14 March 2018 at 18:38, Mark Rutland <mark.rutland@arm.com> wrote: > >> > On Wed, Mar 14, 2018 at 05:58:30PM +0100, Ulf Hansson wrote: > >> >> In case the hierarchical layout is used in DT, we want to initialize the > >> >> corresponding PM domain topology for the CPUs, by using the generic PM > >> >> domain (aka genpd) infrastructure. > >> >> > >> >> At first glance, it may seem feasible to hook into the existing > >> >> psci_dt_init() function, although because it's called quite early in the > >> >> boot sequence, allocating the dynamic data structure for a genpd doesn't > >> >> work. > >> >> > >> >> Therefore, let's export a new init function for PSCI, > >> >> psci_dt_topology_init(), which the ARM machine code should call from a > >> >> suitable initcall. > >> >> > >> >> Succeeding to initialize the PM domain topology, which means at least one > >> >> instance of a genpd becomes created, allows us to continue to enable the > >> >> PSCI OS initiated mode for the platform. If everything turns out fine, > >> >> let's print a message in log to inform the user about the changed mode. > >> >> > >> >> In case of any failures, we stick to the default PSCI Platform Coordinated > >> >> mode. > >> > > >> > For kexec/kdump we'll need to explicitly set the suspend mode to > >> > platform coordinated if for whatever reason we choose not to use OSI. > >> > >> Could you please elaborate on this? I am not really understanding what > >> you are suggesting me to do and why. > > > > Sorry for not being clear. > > > > What I'd like to see is that we always call SET_SUSPEND_MODE before > > invoking CPU_SUSPEND. Either deciding early if we can use OSI, or always > > setting it to platform co-ordinated early on in boot and later switching > > it over. > > > > That way we can be sure of the suspend mode, even if we've kexec'd from > > a kernel that had fiddled with it. > > Right, good point. Let's me update the patch to deal with that. > > One thing though, kexec from a "new" kernel having OSI mode enabled > into an "old" kernel that doesn't even have the new OSI mode support > in psci driver, is going to be difficult to deal with. > On the other hand I guess that isn't a very important usecase we need > to cover, or is it? I think we can also transition back to platform-coordinated in a reboot notifier, which should cater for kexec to an old kernel. I agree it shouldn't be a big deal, though. Thanks, Mark.
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index 463f78c..45d55fc 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -723,6 +723,37 @@ int __init psci_dt_init(void) return ret; } +int __init psci_dt_topology_init(void) +{ + struct device_node *np; + int ret; + + if (!psci_has_osi_support()) + return 0; + + np = of_find_matching_node_and_match(NULL, psci_of_match, NULL); + if (!np) + return -ENODEV; + + /* Initialize the CPU PM domains based on topology described in DT. */ + ret = psci_dt_init_pm_domains(np); + if (ret <= 0) + goto out; + + /* Enable OSI mode. */ + ret = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, + PSCI_1_0_SUSPEND_MODE_OSI, 0, 0); + if (ret) { + pr_info("failed to enable OSI mode: %d\n", ret); + goto out; + } + + pr_info("OSI mode enabled.\n"); +out: + of_node_put(np); + return ret; +} + #ifdef CONFIG_ACPI /* * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's diff --git a/include/linux/psci.h b/include/linux/psci.h index 8b1b3b5..298a044 100644 --- a/include/linux/psci.h +++ b/include/linux/psci.h @@ -53,8 +53,10 @@ extern struct psci_operations psci_ops; #if defined(CONFIG_ARM_PSCI_FW) int __init psci_dt_init(void); +int __init psci_dt_topology_init(void); #else static inline int psci_dt_init(void) { return 0; } +static inline int psci_dt_topology_init(void) { return 0; } #endif #if defined(CONFIG_ARM_PSCI_FW) && defined(CONFIG_ACPI)
In case the hierarchical layout is used in DT, we want to initialize the corresponding PM domain topology for the CPUs, by using the generic PM domain (aka genpd) infrastructure. At first glance, it may seem feasible to hook into the existing psci_dt_init() function, although because it's called quite early in the boot sequence, allocating the dynamic data structure for a genpd doesn't work. Therefore, let's export a new init function for PSCI, psci_dt_topology_init(), which the ARM machine code should call from a suitable initcall. Succeeding to initialize the PM domain topology, which means at least one instance of a genpd becomes created, allows us to continue to enable the PSCI OS initiated mode for the platform. If everything turns out fine, let's print a message in log to inform the user about the changed mode. In case of any failures, we stick to the default PSCI Platform Coordinated mode. Cc: Lina Iyer <ilina@codeaurora.org> Co-developed-by: Lina Iyer <lina.iyer@linaro.org> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/firmware/psci.c | 31 +++++++++++++++++++++++++++++++ include/linux/psci.h | 2 ++ 2 files changed, 33 insertions(+)