Date:	Wed, 20 Aug 2014 10:02:54 -0500
From:	Grant Likely <grant.likely@...aro.org>
To:	Hanjun Guo <hanjun.guo@...aro.org>, Olof Johansson <olof@...om.net>
Cc:	Catalin Marinas <catalin.marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Mark Rutland <mark.rutland@....com>,
	Graeme Gregory <graeme.gregory@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Sudeep Holla <Sudeep.Holla@....com>,
	Will Deacon <will.deacon@....com>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Mark Brown <broonie@...aro.org>,
	Robert Richter <rric@...nel.org>,
	Lv Zheng <lv.zheng@...el.com>,
	Robert Moore <robert.moore@...el.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Charles Garcia-Tobin <Charles.Garcia-Tobin@....com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linaro-acpi-private@...aro.org
Subject: Re: [PATCH 06/19] ARM64 / ACPI: Parse FADT table to get PSCI flags
 for PSCI init

On Thu, 31 Jul 2014 18:23:18 +0800, Hanjun Guo <hanjun.guo@...aro.org> wrote:
> On 2014-7-31 12:22, Olof Johansson wrote:
> > Hi,
> 
> Hi Olof,
> 
> > 
> > On Thu, Jul 24, 2014 at 6:00 AM, Hanjun Guo <hanjun.guo@...aro.org> wrote:
> >> There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set,
> >> the former signals to the OS that the hardware is PSCI compliant.
> >> The latter selects the appropriate conduit for PSCI calls by
> >> toggling between Hypervisor Calls (HVC) and Secure Monitor Calls
> >> (SMC).
> >>
> >> FADT table contains such information, parse FADT to get the flags
> >> for PSCI init. Since ACPI 5.1 doesn't support self defined PSCI
> >> function IDs, which means that only PSCI 0.2+ is supported in ACPI.
> >>
> >> At the same time, only ACPI 5.1 or higher verison supports PSCI,
> >> and FADT Major.Minor version was introduced in ACPI 5.1, so we
> >> will check the version and only parse FADT table with version >= 5.1.
> >>
> >> If firmware provides ACPI tables with ACPI version less than 5.1,
> >> OS will be messed up with those information and have no way to
> >> bring up secondery CPUs, so disable ACPI if we get an FADT table
> >> with version less that 5.1.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
> >> Signed-off-by: Graeme Gregory <graeme.gregory@...aro.org>
> >> ---
> >>  arch/arm64/include/asm/acpi.h |    2 +
> >>  arch/arm64/kernel/acpi.c      |   50 ++++++++++++++++++++++
> >>  arch/arm64/kernel/psci.c      |   95 +++++++++++++++++++++++++++++------------
> >>  arch/arm64/kernel/setup.c     |    2 +
> >>  4 files changed, 121 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index 44b617f..67dac90 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -18,6 +18,8 @@ extern int acpi_disabled;
> >>  extern int acpi_noirq;
> >>  extern int acpi_pci_disabled;
> >>  extern int acpi_strict;
> >> +extern int acpi_psci_present;
> >> +extern int acpi_psci_use_hvc;
> >>
> >>  static inline void disable_acpi(void)
> >>  {
> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >> index f5a10b5..374926f 100644
> >> --- a/arch/arm64/kernel/acpi.c
> >> +++ b/arch/arm64/kernel/acpi.c
> >> @@ -11,6 +11,8 @@
> >>   *  published by the Free Software Foundation.
> >>   */
> >>
> >> +#define pr_fmt(fmt) "ACPI: " fmt
> >> +
> >>  #include <linux/init.h>
> >>  #include <linux/acpi.h>
> >>  #include <linux/cpumask.h>
> >> @@ -34,6 +36,12 @@ EXPORT_SYMBOL(acpi_disabled);
> >>  int acpi_pci_disabled;         /* skip ACPI PCI scan and IRQ initialization */
> >>  EXPORT_SYMBOL(acpi_pci_disabled);
> >>
> >> +/* 1 to indicate PSCI is implemented */
> >> +int acpi_psci_present;
> >> +
> >> +/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */
> >> +int acpi_psci_use_hvc;
> > 
> > Here's a prime example of where it would just make more sense to
> > populate DT based on what's in the ACPI info.
> > 
> > Have a acpi_parse_fadt() that, if needed, creates a /psci node in the
> > system-wide DT and populates it with the few properties needed.
> > 
> > That way, the rest of the code path in the kernel setup is identical,
> > instead of dealing with separate functions for setup, two exported
> > variables just to communicate the state, and so on. It's just extra
> > complexity for no good reason. The ACPi side code isn't even adding
> > significant complexity compared to this. We'll need to add an
> > of_add_node() property though.
> 
> Yes, this will make the code path in the kernel setup is identical, but
> I think mixture of ACPI and DT (converting ACPI into DT at run-time) is
> not a good solution, and this had been discussed last year:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/211662.html

Agreed. I don't want to get into populating DT structures with ACPI
data. I think doing so ends up increasing the conceptual complexity.

> >> -       np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> >> +       if (acpi_disabled) {
> >> +               np = of_find_matching_node_and_match(NULL,
> >> +                               psci_of_match, &matched_np);
> > 
> > Ideally this code should go away by changing the rest of it, but for
> > future cases: It'd be a lot cleaner to do this as:
> > 
> >         if (!acpi_disabled)
> >                 return psci_0_2_init_acpi();
> > 
> > ... then fall through to the current implementation instead.
> 
> I prefer this one :)

Good! :-)

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists