From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751601AbcFXWn3 (ORCPT ); Fri, 24 Jun 2016 18:43:29 -0400 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:43968 "HELO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751209AbcFXWnU (ORCPT ); Fri, 24 Jun 2016 18:43:20 -0400 From: "Rafael J. Wysocki" To: Daniel Lezcano Cc: Lorenzo Pieralisi , Sudeep Holla , linux-acpi@vger.kernel.org, Vikas Sajjan , Sunil , Prashanth Prakash , Al Stone , Ashwin Chaugule , linux-kernel@vger.kernel.org, Mark Rutland , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v6 4/5] arm64: add support for ACPI Low Power Idle(LPI) Date: Sat, 25 Jun 2016 00:47:41 +0200 Message-ID: <15462807.mkANpcI9rV@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.5.0-rc1+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <576DA047.7080808@linaro.org> References: <1465915719-8409-1-git-send-email-sudeep.holla@arm.com> <20160622141700.GB2733@red-moon> <576DA047.7080808@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, June 24, 2016 11:04:07 PM Daniel Lezcano wrote: > On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote: > > Hi Sudeep, > > > > On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote: > >> This patch adds appropriate callbacks to support ACPI Low Power Idle > >> (LPI) on ARM64. > >> > >> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64} > >> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can > >> unify it and move to cpuidle-arm.h header. > >> > >> Cc: Lorenzo Pieralisi > >> Cc: Mark Rutland > >> Cc: Daniel Lezcano > >> Cc: "Rafael J. Wysocki" > >> Cc: linux-arm-kernel@lists.infradead.org > >> Signed-off-by: Sudeep Holla > >> --- > >> arch/arm64/kernel/cpuidle.c | 17 +++++++++++++ > >> drivers/cpuidle/cpuidle-arm.c | 23 ++---------------- > >> drivers/firmware/psci.c | 56 +++++++++++++++++++++++++++++++++++++++++++ > >> include/linux/cpuidle-arm.h | 30 +++++++++++++++++++++++ > >> 4 files changed, 105 insertions(+), 21 deletions(-) > >> create mode 100644 include/linux/cpuidle-arm.h > > > > This patch seems fine by me, it would be good if Daniel can have > > a look too. > > > > Some minor comments below. > > > > [...] > > > >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c > >> index 03e04582791c..c6caa863d156 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; > >> + if (count <= 0) > >> + 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; > > Why is needed to mask 'address' ? > > >> + 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; > > > > Most of the code in this function is FW independent, it would be nice > > to factor it out and add the respective ACPI/DT helper functions to > > retrieve idle states count and parameters, we can update it later > > anyway it is fine by me to leave it as it is. > > > >> +} > >> +#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); > > Is it possible the case where there is information in both the DT and in > ACPI ? No, it isn't. It is either-or, never both at the same time. Thanks, Rafael