[PATCH 06/19] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Grant Likely]

On Thu, 31 Jul 2014 18:23:18 +0800, Hanjun Guo <hanjun.guo at linaro.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 at linaro.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 at linaro.org>
> >> Signed-off-by: Graeme Gregory <graeme.gregory at linaro.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.