From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753311AbcFJMuD (ORCPT ); Fri, 10 Jun 2016 08:50:03 -0400 Received: from foss.arm.com ([217.140.101.70]:38130 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933118AbcFJMuA (ORCPT ); Fri, 10 Jun 2016 08:50:00 -0400 Date: Fri, 10 Jun 2016 13:50:28 +0100 From: Lorenzo Pieralisi To: Sudeep Holla Cc: linux-acpi@vger.kernel.org, "Rafael J. Wysocki" , linux-kernel@vger.kernel.org, Vikas Sajjan , Sunil , Prashanth Prakash , Ashwin Chaugule , Al Stone , Mark Rutland , linux-arm-kernel@lists.infradead.org, daniel.lezcano@linaro.org, khilman@baylibre.com Subject: Re: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI) Message-ID: <20160610125028.GA24002@red-moon> References: <1462981062-24909-1-git-send-email-sudeep.holla@arm.com> <1462981062-24909-5-git-send-email-sudeep.holla@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462981062-24909-5-git-send-email-sudeep.holla@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [+ Daniel, Kevin] On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote: > This patch adds appropriate callbacks to support ACPI Low Power Idle > (LPI) on ARM64. > > Cc: Lorenzo Pieralisi > Cc: Mark Rutland > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Sudeep Holla > --- > arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++ > drivers/firmware/psci.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 104 insertions(+) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c I think we can add this to arm64/kernel/cpuidle.c so that we have all arch code managing idle in one place. > index d1ce8e2f98b9..bf82ce5c8fce 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -25,6 +26,9 @@ > #include > #include > > +#include > + > +#include > #include > #include > #include > @@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void) > } > } > > +int acpi_processor_ffh_lpi_probe(unsigned int cpu) > +{ > + return arm_cpuidle_init(cpu); > +} > + > +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT BIT(0) > +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT BIT(1) > +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT BIT(2) > +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT BIT(3) > +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT \ > + (ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT) > + > +struct acpi_lpi_state *lpi; > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx) > +{ > + int ret = 0; > + bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT; I am not really that keen on this, as you know. Those flags are there to say "save these components registers". I see the CPU PM notifiers as a way to save/restore CPU peripheral state, but they should *not* carry out any action that affects the power state itself, that's down to the suspend finisher (eg PSCI), because that's where the specific idle states are managed. I agree we have no clue whatsoever on what we *really* need to save/restore, but that's orthogonal to what you are solving here. See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ? No. We should not handle the same problem differently. On top of that, we have no way to solve this problem for DT, all I am saying is that it is ill-defined and given that LPI is new I'd rather we got it right from the beginning. I am open to suggestions here. > + > + if (!idx) { > + cpu_do_idle(); > + return idx; > + } > + > + /* TODO cpu_pm_{enter,exit} can be done in generic code ? */ > + if (save_ctx) > + ret = cpu_pm_enter(); > + if (!ret) { > + /* > + * Pass idle state index to cpu_suspend which in turn will > + * call the CPU ops suspend protocol with idle index as a > + * parameter. > + */ > + ret = arm_cpuidle_suspend(idx); > + > + if (save_ctx) > + cpu_pm_exit(); > + } > + > + return ret ? -1 : idx; The body of this function (if we remove save_ctx) is identical to arm_enter_idle_state(), it would be nice if we found a way where to put this code and share it with the ARM CPUidle driver, but I am not too fussed about that either. > +} > + > #ifdef CONFIG_ACPI_APEI > pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr) > { > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > index fa4ea22ca12e..e06bfee68e1d 100644 > --- a/drivers/firmware/psci.c > +++ b/drivers/firmware/psci.c > @@ -13,6 +13,7 @@ > > #define pr_fmt(fmt) "psci: " fmt > > +#include > #include > #include > #include > @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu) > return ret; > } > > +#ifdef CONFIG_ACPI > +#include > + > +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) > +{ > + int i, count; > + u32 *psci_states; > + struct acpi_processor *pr; > + struct acpi_lpi_state *lpi; > + > + pr = per_cpu(processors, cpu); > + if (unlikely(!pr || !pr->flags.has_lpi)) > + return -EINVAL; > + > + /* > + * If the PSCI cpu_suspend function hook has not been initialized > + * idle states must not be enabled, so bail out > + */ > + if (!psci_ops.cpu_suspend) > + return -EOPNOTSUPP; > + > + count = pr->power.count - 1; Nit: I am not sure this can happen, but you really do not want count to become == -1. > + if (!count) > + return -ENODEV; > + > + psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL); > + if (!psci_states) > + return -ENOMEM; > + > + for (i = 0; i < count; i++) { > + u32 state; > + > + lpi = &pr->power.lpi_states[i + 1]; > + state = lpi->address & 0xFFFFFFFF; > + if (!psci_power_state_is_valid(state)) { > + pr_warn("Invalid PSCI power state %#x\n", state); > + kfree(psci_states); > + return -EINVAL; > + } > + psci_states[i] = state; > + } > + /* Idle states parsed correctly, initialize per-cpu pointer */ > + per_cpu(psci_power_state, cpu) = psci_states; > + return 0; > +} > +#else > +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) > +{ > + return -EINVAL; > +} > +#endif > + > int psci_cpu_init_idle(unsigned int cpu) > { > struct device_node *cpu_node; > int ret; > > + if (!acpi_disabled) > + return psci_acpi_cpu_init_idle(cpu); > + > cpu_node = of_get_cpu_node(cpu, NULL); > if (!cpu_node) > return -ENODEV; save_ctx notwithstanding the patch is fine, let's define what to do with that, remainder of the code is ok. Thanks, Lorenzo From mboxrd@z Thu Jan 1 00:00:00 1970 From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) Date: Fri, 10 Jun 2016 13:50:28 +0100 Subject: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI) In-Reply-To: <1462981062-24909-5-git-send-email-sudeep.holla@arm.com> References: <1462981062-24909-1-git-send-email-sudeep.holla@arm.com> <1462981062-24909-5-git-send-email-sudeep.holla@arm.com> Message-ID: <20160610125028.GA24002@red-moon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org [+ Daniel, Kevin] On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote: > This patch adds appropriate callbacks to support ACPI Low Power Idle > (LPI) on ARM64. > > Cc: Lorenzo Pieralisi > Cc: Mark Rutland > Cc: linux-arm-kernel at lists.infradead.org > Signed-off-by: Sudeep Holla > --- > arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++ > drivers/firmware/psci.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 104 insertions(+) > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c I think we can add this to arm64/kernel/cpuidle.c so that we have all arch code managing idle in one place. > index d1ce8e2f98b9..bf82ce5c8fce 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -25,6 +26,9 @@ > #include > #include > > +#include > + > +#include > #include > #include > #include > @@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void) > } > } > > +int acpi_processor_ffh_lpi_probe(unsigned int cpu) > +{ > + return arm_cpuidle_init(cpu); > +} > + > +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT BIT(0) > +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT BIT(1) > +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT BIT(2) > +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT BIT(3) > +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT \ > + (ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT | \ > + ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT) > + > +struct acpi_lpi_state *lpi; > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx) > +{ > + int ret = 0; > + bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT; I am not really that keen on this, as you know. Those flags are there to say "save these components registers". I see the CPU PM notifiers as a way to save/restore CPU peripheral state, but they should *not* carry out any action that affects the power state itself, that's down to the suspend finisher (eg PSCI), because that's where the specific idle states are managed. I agree we have no clue whatsoever on what we *really* need to save/restore, but that's orthogonal to what you are solving here. See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ? No. We should not handle the same problem differently. On top of that, we have no way to solve this problem for DT, all I am saying is that it is ill-defined and given that LPI is new I'd rather we got it right from the beginning. I am open to suggestions here. > + > + if (!idx) { > + cpu_do_idle(); > + return idx; > + } > + > + /* TODO cpu_pm_{enter,exit} can be done in generic code ? */ > + if (save_ctx) > + ret = cpu_pm_enter(); > + if (!ret) { > + /* > + * Pass idle state index to cpu_suspend which in turn will > + * call the CPU ops suspend protocol with idle index as a > + * parameter. > + */ > + ret = arm_cpuidle_suspend(idx); > + > + if (save_ctx) > + cpu_pm_exit(); > + } > + > + return ret ? -1 : idx; The body of this function (if we remove save_ctx) is identical to arm_enter_idle_state(), it would be nice if we found a way where to put this code and share it with the ARM CPUidle driver, but I am not too fussed about that either. > +} > + > #ifdef CONFIG_ACPI_APEI > pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr) > { > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > index fa4ea22ca12e..e06bfee68e1d 100644 > --- a/drivers/firmware/psci.c > +++ b/drivers/firmware/psci.c > @@ -13,6 +13,7 @@ > > #define pr_fmt(fmt) "psci: " fmt > > +#include > #include > #include > #include > @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu) > return ret; > } > > +#ifdef CONFIG_ACPI > +#include > + > +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) > +{ > + int i, count; > + u32 *psci_states; > + struct acpi_processor *pr; > + struct acpi_lpi_state *lpi; > + > + pr = per_cpu(processors, cpu); > + if (unlikely(!pr || !pr->flags.has_lpi)) > + return -EINVAL; > + > + /* > + * If the PSCI cpu_suspend function hook has not been initialized > + * idle states must not be enabled, so bail out > + */ > + if (!psci_ops.cpu_suspend) > + return -EOPNOTSUPP; > + > + count = pr->power.count - 1; Nit: I am not sure this can happen, but you really do not want count to become == -1. > + if (!count) > + return -ENODEV; > + > + psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL); > + if (!psci_states) > + return -ENOMEM; > + > + for (i = 0; i < count; i++) { > + u32 state; > + > + lpi = &pr->power.lpi_states[i + 1]; > + state = lpi->address & 0xFFFFFFFF; > + if (!psci_power_state_is_valid(state)) { > + pr_warn("Invalid PSCI power state %#x\n", state); > + kfree(psci_states); > + return -EINVAL; > + } > + psci_states[i] = state; > + } > + /* Idle states parsed correctly, initialize per-cpu pointer */ > + per_cpu(psci_power_state, cpu) = psci_states; > + return 0; > +} > +#else > +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) > +{ > + return -EINVAL; > +} > +#endif > + > int psci_cpu_init_idle(unsigned int cpu) > { > struct device_node *cpu_node; > int ret; > > + if (!acpi_disabled) > + return psci_acpi_cpu_init_idle(cpu); > + > cpu_node = of_get_cpu_node(cpu, NULL); > if (!cpu_node) > return -ENODEV; save_ctx notwithstanding the patch is fine, let's define what to do with that, remainder of the code is ok. Thanks, Lorenzo