diff mbox

[v3,02/17] ARM64 / ACPI: Get RSDP and ACPI boot-time tables

Message ID 1409583475-6978-3-git-send-email-hanjun.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo Sept. 1, 2014, 2:57 p.m. UTC
From: Al Stone <al.stone@linaro.org>

As we want to get ACPI tables to parse and then use the information
for system initialization, we should get the RSDP (Root System
Description Pointer) first, it then locates Extended Root Description
Table (XSDT) which contains all the 64-bit physical address that
pointer to other boot-time tables.

Introduce acpi.c and its related head file in this patch to provide
fundamental needs of extern variables and functions for ACPI core,
and then get boot-time tables as needed.
  - asm/acenv.h for arch specific ACPICA environments and
    implementation;
  - asm/acpi.h for arch specific variables and functions needed by
    ACPI driver core;
  - acpi.c for ARM64 related ACPI implementation for ACPI driver
    core;

acpi_boot_table_init() is introduced to get RSDP and boot-time tables,
it will be called in setup_arch() before paging_init(), so we should
use eary_memremap() mechanism here to get the RSDP and all the table
pointers.

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/acenv.h |   18 +++++++++++
 arch/arm64/include/asm/acpi.h  |   45 ++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile     |    1 +
 arch/arm64/kernel/acpi.c       |   69 ++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c      |    4 +++
 5 files changed, 137 insertions(+)
 create mode 100644 arch/arm64/include/asm/acenv.h
 create mode 100644 arch/arm64/include/asm/acpi.h
 create mode 100644 arch/arm64/kernel/acpi.c

Comments

Catalin Marinas Sept. 9, 2014, 4:26 p.m. UTC | #1
On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> new file mode 100644
> index 0000000..3899ee6
> --- /dev/null
> +++ b/arch/arm64/include/asm/acenv.h
> @@ -0,0 +1,18 @@
> +/*
> + * ARM64 specific ACPICA environments and implementation
> + *
> + * Copyright (C) 2014, Linaro Ltd.
> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _ASM_ACENV_H
> +#define _ASM_ACENV_H
> +
> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")

Does this mean that it will be supported at some point? Looking at the
places where this function is called, I don't really see how this would
ever work on ARM. Which means that we add such macro just to be able to
compile code that would never be used on arm64. I would rather see the
relevant ACPI files only compiled on x86/IA-64 rather than arm64.

> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> new file mode 100644
> index 0000000..9252f72
> --- /dev/null
> +++ b/arch/arm64/kernel/acpi.c

[...]

> +/*
> + * acpi_boot_table_init() called from setup_arch(), always.
> + *	1. find RSDP and get its address, and then find XSDT
> + *	2. extract all tables and checksums them all
> + *
> + * We can parse ACPI boot-time tables such as MADT after
> + * this function is called.
> + */
> +void __init acpi_boot_table_init(void)
> +{
> +	/* If acpi_disabled, bail out */
> +	if (acpi_disabled)
> +		return;

So at this point, there wouldn't be anything yet to set acpi_disabled to
1, unless !CONFIG_ACPI.

> +
> +	/* Initialize the ACPI boot-time table parser. */
> +	if (acpi_table_init())
> +		disable_acpi();

Do you need anything more to add to this function later? Otherwise, just
call it in setup_arch() directly (maybe dependent on
IS_ENABLED(CONFIG_ACPI) or just create an equivalent dummy
acpi_table_init() when not configured).

> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index c96172a..fb7cc0e 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -43,6 +43,7 @@
>  #include <linux/of_fdt.h>
>  #include <linux/of_platform.h>
>  #include <linux/efi.h>
> +#include <linux/acpi.h>
>  
>  #include <asm/fixmap.h>
>  #include <asm/cpu.h>
> @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p)
>  	efi_init();
>  	arm64_memblock_init();
>  
> +	/* Parse the ACPI tables for possible boot-time configuration */
> +	acpi_boot_table_init();

Is there any dummy acpi_boot_table_init() when !CONFIG_ACPI? I couldn't
see one in this patch, so I don't see how this would compile when ACPI
is disabled.
Jon Masters Sept. 9, 2014, 4:41 p.m. UTC | #2
On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>> new file mode 100644
>> index 0000000..3899ee6
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/acenv.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * ARM64 specific ACPICA environments and implementation
>> + *
>> + * Copyright (C) 2014, Linaro Ltd.
>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _ASM_ACENV_H
>> +#define _ASM_ACENV_H
>> +
>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> 
> Does this mean that it will be supported at some point? Looking at the
> places where this function is called, I don't really see how this would
> ever work on ARM. Which means that we add such macro just to be able to
> compile code that would never be used on arm64. I would rather see the
> relevant ACPI files only compiled on x86/IA-64 rather than arm64.

That specific cache behavior is a part of e.g. ACPI C3 state support
(e.g. ACPI5.1 8.1.4 Processor Power State C3). As you note, it's not
going to work on 64-bit ARM as it does on x86, but it's optional to
implement C3 and early 64-bit ARM systems should not report Wbindv flags
in the FADT anyway. They can also set FADT.P_LVL3_LAT > 1000, which has
the effect of disabling C3 support, while also allowing for use of _CST
objects to define more flexible C-States later on.

Jon.
Jon Masters Sept. 9, 2014, 4:44 p.m. UTC | #3
One of the ARM guys pointed out the cache issue before I had meant to send this note then. Sorry.
Mark Rutland Sept. 9, 2014, 4:54 p.m. UTC | #4
On Tue, Sep 09, 2014 at 05:26:49PM +0100, Catalin Marinas wrote:
> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> > diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> > new file mode 100644
> > index 0000000..3899ee6
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/acenv.h
> > @@ -0,0 +1,18 @@
> > +/*
> > + * ARM64 specific ACPICA environments and implementation
> > + *
> > + * Copyright (C) 2014, Linaro Ltd.
> > + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> > + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef _ASM_ACENV_H
> > +#define _ASM_ACENV_H
> > +
> > +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> 
> Does this mean that it will be supported at some point?

From what I recall it's impossible to support as we have no equivalent
of WBINVD -- the possibility migration of dirty cache lines renders it
impossible to guarantee that the caches remain empty (or that data even
left the local caches in the first place).

> Looking at the places where this function is called, I don't really
> see how this would ever work on ARM. Which means that we add such
> macro just to be able to compile code that would never be used on
> arm64. I would rather see the relevant ACPI files only compiled on
> x86/IA-64 rather than arm64.

Agreed.

It looks like include/acpi/platform/acenv.h defines an empty
ACPI_FLUSH_CPU_CACHE() stub, so something else should probably be done
to turn the use of ACPI_FLUSH_CPU_CACHE() into a build-time bug on
arm64.

Mark.
Mark Rutland Sept. 9, 2014, 5:15 p.m. UTC | #5
On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> > On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> >> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> >> new file mode 100644
> >> index 0000000..3899ee6
> >> --- /dev/null
> >> +++ b/arch/arm64/include/asm/acenv.h
> >> @@ -0,0 +1,18 @@
> >> +/*
> >> + * ARM64 specific ACPICA environments and implementation
> >> + *
> >> + * Copyright (C) 2014, Linaro Ltd.
> >> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> >> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#ifndef _ASM_ACENV_H
> >> +#define _ASM_ACENV_H
> >> +
> >> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> > 
> > Does this mean that it will be supported at some point? Looking at the
> > places where this function is called, I don't really see how this would
> > ever work on ARM. Which means that we add such macro just to be able to
> > compile code that would never be used on arm64. I would rather see the
> > relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> 
> That specific cache behavior is a part of e.g. ACPI C3 state support
> (e.g. ACPI5.1 8.1.4 Processor Power State C3).

Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
we don't get S1, S2, or S3 states either.

> As you note, it's not going to work on 64-bit ARM as it does on x86,
> but it's optional to implement C3 and early 64-bit ARM systems should
> not report Wbindv flags in the FADT anyway.

Unless the arm cache architecture changes, I wouldn't expect any 64-bit
ARM system to set either of the WBINVD flags.

> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> disabling C3 support, while also allowing for use of _CST objects to
> define more flexible C-States later on.

It sounds like we should be sanity checking these in the arm64 ACPI code
for the moment. I don't want us to discover that current platforms
report the wrong thing only when new platforms come out that might
actually report things correctly.

Mark.
Jon Masters Sept. 9, 2014, 5:33 p.m. UTC | #6
On 09/09/2014 01:15 PM, Mark Rutland wrote:
> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>>>> new file mode 100644
>>>> index 0000000..3899ee6
>>>> --- /dev/null
>>>> +++ b/arch/arm64/include/asm/acenv.h
>>>> @@ -0,0 +1,18 @@
>>>> +/*
>>>> + * ARM64 specific ACPICA environments and implementation
>>>> + *
>>>> + * Copyright (C) 2014, Linaro Ltd.
>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +#ifndef _ASM_ACENV_H
>>>> +#define _ASM_ACENV_H
>>>> +
>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
>>>
>>> Does this mean that it will be supported at some point? Looking at the
>>> places where this function is called, I don't really see how this would
>>> ever work on ARM. Which means that we add such macro just to be able to
>>> compile code that would never be used on arm64. I would rather see the
>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
>>
>> That specific cache behavior is a part of e.g. ACPI C3 state support
>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> 
> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> we don't get S1, S2, or S3 states either.

That's not quite what it says. You could still define an \_S1 state on
such a system. That table simply makes an assumption that if the stride
parameters are not defined and neither is wbindv then there's no way to
reliably flush caches, which of course isn't a valid conclusion, it's
just language that needs to be updated to reflect reality. So it's not
"don't get", it's "machine cannot support", which is an assumption.

>> As you note, it's not going to work on 64-bit ARM as it does on x86,
>> but it's optional to implement C3 and early 64-bit ARM systems should
>> not report Wbindv flags in the FADT anyway.
> 
> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> ARM system to set either of the WBINVD flags.

Indeed.

>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
>> disabling C3 support, while also allowing for use of _CST objects to
>> define more flexible C-States later on.
> 
> It sounds like we should be sanity checking these in the arm64 ACPI code
> for the moment. I don't want us to discover that current platforms
> report the wrong thing only when new platforms come out that might
> actually report things correctly.

It's reasonable to do this sooner rather than later, sure.

Jon.
Lorenzo Pieralisi Sept. 9, 2014, 5:50 p.m. UTC | #7
On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> > On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> > > On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> > >> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> > >> new file mode 100644
> > >> index 0000000..3899ee6
> > >> --- /dev/null
> > >> +++ b/arch/arm64/include/asm/acenv.h
> > >> @@ -0,0 +1,18 @@
> > >> +/*
> > >> + * ARM64 specific ACPICA environments and implementation
> > >> + *
> > >> + * Copyright (C) 2014, Linaro Ltd.
> > >> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> > >> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> > >> + *
> > >> + * This program is free software; you can redistribute it and/or modify
> > >> + * it under the terms of the GNU General Public License version 2 as
> > >> + * published by the Free Software Foundation.
> > >> + */
> > >> +
> > >> +#ifndef _ASM_ACENV_H
> > >> +#define _ASM_ACENV_H
> > >> +
> > >> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> > > 
> > > Does this mean that it will be supported at some point? Looking at the
> > > places where this function is called, I don't really see how this would
> > > ever work on ARM. Which means that we add such macro just to be able to
> > > compile code that would never be used on arm64. I would rather see the
> > > relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> > 
> > That specific cache behavior is a part of e.g. ACPI C3 state support
> > (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> 
> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> we don't get S1, S2, or S3 states either.
> 
> > As you note, it's not going to work on 64-bit ARM as it does on x86,
> > but it's optional to implement C3 and early 64-bit ARM systems should
> > not report Wbindv flags in the FADT anyway.
> 
> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> ARM system to set either of the WBINVD flags.
> 
> > They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> > disabling C3 support, while also allowing for use of _CST objects to
> > define more flexible C-States later on.
> 
> It sounds like we should be sanity checking these in the arm64 ACPI code
> for the moment. I don't want us to discover that current platforms
> report the wrong thing only when new platforms come out that might
> actually report things correctly.

I think that the kernel must ignore most of the stuff mentioned above
in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
is not even there. The problem is trying to compile code that basically
has no defined behaviour - ie it is unspecified - on ARM64, that's what
Catalin pointed out.

I understand it is compiled in by default on x86, but that's not a reason
why we should add empty hooks all over the place to compile code that
does not stand a chance to be doing anything sensible apart from
returning an error code, in the best case scenario.

Lorenzo
Sudeep Holla Sept. 9, 2014, 6:05 p.m. UTC | #8
On 09/09/14 18:50, Lorenzo Pieralisi wrote:
> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>>>>> new file mode 100644
>>>>> index 0000000..3899ee6
>>>>> --- /dev/null
>>>>> +++ b/arch/arm64/include/asm/acenv.h
>>>>> @@ -0,0 +1,18 @@
>>>>> +/*
>>>>> + * ARM64 specific ACPICA environments and implementation
>>>>> + *
>>>>> + * Copyright (C) 2014, Linaro Ltd.
>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + */
>>>>> +
>>>>> +#ifndef _ASM_ACENV_H
>>>>> +#define _ASM_ACENV_H
>>>>> +
>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
>>>>
>>>> Does this mean that it will be supported at some point? Looking at the
>>>> places where this function is called, I don't really see how this would
>>>> ever work on ARM. Which means that we add such macro just to be able to
>>>> compile code that would never be used on arm64. I would rather see the
>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
>>>
>>> That specific cache behavior is a part of e.g. ACPI C3 state support
>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
>>
>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
>> we don't get S1, S2, or S3 states either.
>>
>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
>>> but it's optional to implement C3 and early 64-bit ARM systems should
>>> not report Wbindv flags in the FADT anyway.
>>
>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
>> ARM system to set either of the WBINVD flags.
>>
>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
>>> disabling C3 support, while also allowing for use of _CST objects to
>>> define more flexible C-States later on.
>>
>> It sounds like we should be sanity checking these in the arm64 ACPI code
>> for the moment. I don't want us to discover that current platforms
>> report the wrong thing only when new platforms come out that might
>> actually report things correctly.
>
> I think that the kernel must ignore most of the stuff mentioned above
> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
> is not even there. The problem is trying to compile code that basically
> has no defined behaviour - ie it is unspecified - on ARM64, that's what
> Catalin pointed out.
>
> I understand it is compiled in by default on x86, but that's not a reason
> why we should add empty hooks all over the place to compile code that
> does not stand a chance to be doing anything sensible apart from
> returning an error code, in the best case scenario.
>

I had pointed out this earlier, even if we make it compile there's
every possibility that it can blow up if some vendor adds S- states
to their ACPI tables. One clear reason why it could blow up is:

"
      /* This violates the spec but is required for bug compatibility. */
      acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
"

I don't think this can ever work on ARM platforms. So better to fix it
properly.

Regards,
Sudeep
Jon Masters Sept. 9, 2014, 7:06 p.m. UTC | #9
On 09/09/2014 02:05 PM, Sudeep Holla wrote:
> 
> 
> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>>>>>> new file mode 100644
>>>>>> index 0000000..3899ee6
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm64/include/asm/acenv.h
>>>>>> @@ -0,0 +1,18 @@
>>>>>> +/*
>>>>>> + * ARM64 specific ACPICA environments and implementation
>>>>>> + *
>>>>>> + * Copyright (C) 2014, Linaro Ltd.
>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>>> + * published by the Free Software Foundation.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _ASM_ACENV_H
>>>>>> +#define _ASM_ACENV_H
>>>>>> +
>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
>>>>>
>>>>> Does this mean that it will be supported at some point? Looking at the
>>>>> places where this function is called, I don't really see how this would
>>>>> ever work on ARM. Which means that we add such macro just to be able to
>>>>> compile code that would never be used on arm64. I would rather see the
>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
>>>>
>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
>>>
>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
>>> we don't get S1, S2, or S3 states either.
>>>
>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
>>>> but it's optional to implement C3 and early 64-bit ARM systems should
>>>> not report Wbindv flags in the FADT anyway.
>>>
>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
>>> ARM system to set either of the WBINVD flags.
>>>
>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
>>>> disabling C3 support, while also allowing for use of _CST objects to
>>>> define more flexible C-States later on.
>>>
>>> It sounds like we should be sanity checking these in the arm64 ACPI code
>>> for the moment. I don't want us to discover that current platforms
>>> report the wrong thing only when new platforms come out that might
>>> actually report things correctly.
>>
>> I think that the kernel must ignore most of the stuff mentioned above
>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
>> is not even there. The problem is trying to compile code that basically
>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
>> Catalin pointed out.
>>
>> I understand it is compiled in by default on x86, but that's not a reason
>> why we should add empty hooks all over the place to compile code that
>> does not stand a chance to be doing anything sensible apart from
>> returning an error code, in the best case scenario.
>>
> 
> I had pointed out this earlier, even if we make it compile there's
> every possibility that it can blow up if some vendor adds S- states
> to their ACPI tables. One clear reason why it could blow up is:
> 
> "
>       /* This violates the spec but is required for bug compatibility. */
>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> "
> 
> I don't think this can ever work on ARM platforms. So better to fix it
> properly.

Hanjun,

How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
for the cache behavior, because an embedded x86 box would still probably
define those, but removing the hooks on ARM may make sense.

Jon.
Hanjun Guo Sept. 10, 2014, 7:30 a.m. UTC | #10
On 2014/9/10 0:26, Catalin Marinas wrote:
> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>> new file mode 100644
>> index 0000000..3899ee6
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/acenv.h
>> @@ -0,0 +1,18 @@
>> +/*
>> + * ARM64 specific ACPICA environments and implementation
>> + *
>> + * Copyright (C) 2014, Linaro Ltd.
>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _ASM_ACENV_H
>> +#define _ASM_ACENV_H
>> +
>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> 
> Does this mean that it will be supported at some point? Looking at the
> places where this function is called, I don't really see how this would
> ever work on ARM. Which means that we add such macro just to be able to
> compile code that would never be used on arm64. I would rather see the
> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> 
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> new file mode 100644
>> index 0000000..9252f72
>> --- /dev/null
>> +++ b/arch/arm64/kernel/acpi.c
> 
> [...]
> 
>> +/*
>> + * acpi_boot_table_init() called from setup_arch(), always.
>> + *	1. find RSDP and get its address, and then find XSDT
>> + *	2. extract all tables and checksums them all
>> + *
>> + * We can parse ACPI boot-time tables such as MADT after
>> + * this function is called.
>> + */
>> +void __init acpi_boot_table_init(void)
>> +{
>> +	/* If acpi_disabled, bail out */
>> +	if (acpi_disabled)
>> +		return;
> 
> So at this point, there wouldn't be anything yet to set acpi_disabled to
> 1, unless !CONFIG_ACPI.
> 
>> +
>> +	/* Initialize the ACPI boot-time table parser. */
>> +	if (acpi_table_init())
>> +		disable_acpi();
> 
> Do you need anything more to add to this function later? Otherwise, just
> call it in setup_arch() directly (maybe dependent on
> IS_ENABLED(CONFIG_ACPI) or just create an equivalent dummy
> acpi_table_init() when not configured).

We add FADT parse function to this function later.

> 
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index c96172a..fb7cc0e 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -43,6 +43,7 @@
>>  #include <linux/of_fdt.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/efi.h>
>> +#include <linux/acpi.h>
>>  
>>  #include <asm/fixmap.h>
>>  #include <asm/cpu.h>
>> @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p)
>>  	efi_init();
>>  	arm64_memblock_init();
>>  
>> +	/* Parse the ACPI tables for possible boot-time configuration */
>> +	acpi_boot_table_init();
> 
> Is there any dummy acpi_boot_table_init() when !CONFIG_ACPI? I couldn't
> see one in this patch, so I don't see how this would compile when ACPI
> is disabled.

There is a stub function in linux/acpi.h when ACPI is disabled.

Thanks
Hanjun
Hanjun Guo Sept. 10, 2014, 11:13 a.m. UTC | #11
On 2014/9/10 3:06, Jon Masters wrote:
> On 09/09/2014 02:05 PM, Sudeep Holla wrote:
>>
>>
>> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
>>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
>>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
>>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
>>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
>>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3899ee6
>>>>>>> --- /dev/null
>>>>>>> +++ b/arch/arm64/include/asm/acenv.h
>>>>>>> @@ -0,0 +1,18 @@
>>>>>>> +/*
>>>>>>> + * ARM64 specific ACPICA environments and implementation
>>>>>>> + *
>>>>>>> + * Copyright (C) 2014, Linaro Ltd.
>>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
>>>>>>> + *
>>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>>>> + * published by the Free Software Foundation.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef _ASM_ACENV_H
>>>>>>> +#define _ASM_ACENV_H
>>>>>>> +
>>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
>>>>>>
>>>>>> Does this mean that it will be supported at some point? Looking at the
>>>>>> places where this function is called, I don't really see how this would
>>>>>> ever work on ARM. Which means that we add such macro just to be able to
>>>>>> compile code that would never be used on arm64. I would rather see the
>>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
>>>>>
>>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
>>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
>>>>
>>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
>>>> we don't get S1, S2, or S3 states either.
>>>>
>>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
>>>>> but it's optional to implement C3 and early 64-bit ARM systems should
>>>>> not report Wbindv flags in the FADT anyway.
>>>>
>>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
>>>> ARM system to set either of the WBINVD flags.
>>>>
>>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
>>>>> disabling C3 support, while also allowing for use of _CST objects to
>>>>> define more flexible C-States later on.
>>>>
>>>> It sounds like we should be sanity checking these in the arm64 ACPI code
>>>> for the moment. I don't want us to discover that current platforms
>>>> report the wrong thing only when new platforms come out that might
>>>> actually report things correctly.
>>>
>>> I think that the kernel must ignore most of the stuff mentioned above
>>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
>>> is not even there. The problem is trying to compile code that basically
>>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
>>> Catalin pointed out.
>>>
>>> I understand it is compiled in by default on x86, but that's not a reason
>>> why we should add empty hooks all over the place to compile code that
>>> does not stand a chance to be doing anything sensible apart from
>>> returning an error code, in the best case scenario.
>>>
>>
>> I had pointed out this earlier, even if we make it compile there's
>> every possibility that it can blow up if some vendor adds S- states
>> to their ACPI tables. One clear reason why it could blow up is:
>>
>> "
>>       /* This violates the spec but is required for bug compatibility. */
>>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
>> "
>>
>> I don't think this can ever work on ARM platforms. So better to fix it
>> properly.
> 
> Hanjun,
> 
> How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
> for the cache behavior, because an embedded x86 box would still probably
> define those, but removing the hooks on ARM may make sense.

As Graeme said in the reply, for sleep we are doing the same thing as
ia64 in stubbing out the functions, and before that we are trying to remove
the hooks on ARM by introducing more stubs and making things more complicated.

I agree that we should rework the ACPI core to make sleep/cache related
stuff compatible with ARM, but I think we may not do this in one go, it will
need incremental changes over the next couple of years as real hardware
starts to appear and we finalise the standards to support this.

Now, as we list in the arm-acpi.txt doc, power management of sleep states
and CPU power/frequency control are not well defined in ACPI spec for ARM,
we need some time to finalize the standard and then we know how to implement
that in a good shape.

ACPI 5.1 already fixed lots missing features for ARM64 and provide the
fundamental needs to bring up the basic system for ARM server, power
management is not critical at now, so why not fix/implement it later?

Thanks
Hanjun
Catalin Marinas Sept. 10, 2014, 12:33 p.m. UTC | #12
On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
> On 2014/9/10 3:06, Jon Masters wrote:
> > On 09/09/2014 02:05 PM, Sudeep Holla wrote:
> >> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
> >>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
> >>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> >>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> >>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> >>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..3899ee6
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/arch/arm64/include/asm/acenv.h
> >>>>>>> @@ -0,0 +1,18 @@
> >>>>>>> +/*
> >>>>>>> + * ARM64 specific ACPICA environments and implementation
> >>>>>>> + *
> >>>>>>> + * Copyright (C) 2014, Linaro Ltd.
> >>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> >>>>>>> + *
> >>>>>>> + * This program is free software; you can redistribute it and/or modify
> >>>>>>> + * it under the terms of the GNU General Public License version 2 as
> >>>>>>> + * published by the Free Software Foundation.
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +#ifndef _ASM_ACENV_H
> >>>>>>> +#define _ASM_ACENV_H
> >>>>>>> +
> >>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> >>>>>>
> >>>>>> Does this mean that it will be supported at some point? Looking at the
> >>>>>> places where this function is called, I don't really see how this would
> >>>>>> ever work on ARM. Which means that we add such macro just to be able to
> >>>>>> compile code that would never be used on arm64. I would rather see the
> >>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> >>>>>
> >>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
> >>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> >>>>
> >>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> >>>> we don't get S1, S2, or S3 states either.
> >>>>
> >>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
> >>>>> but it's optional to implement C3 and early 64-bit ARM systems should
> >>>>> not report Wbindv flags in the FADT anyway.
> >>>>
> >>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> >>>> ARM system to set either of the WBINVD flags.
> >>>>
> >>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> >>>>> disabling C3 support, while also allowing for use of _CST objects to
> >>>>> define more flexible C-States later on.
> >>>>
> >>>> It sounds like we should be sanity checking these in the arm64 ACPI code
> >>>> for the moment. I don't want us to discover that current platforms
> >>>> report the wrong thing only when new platforms come out that might
> >>>> actually report things correctly.
> >>>
> >>> I think that the kernel must ignore most of the stuff mentioned above
> >>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
> >>> is not even there. The problem is trying to compile code that basically
> >>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
> >>> Catalin pointed out.
> >>>
> >>> I understand it is compiled in by default on x86, but that's not a reason
> >>> why we should add empty hooks all over the place to compile code that
> >>> does not stand a chance to be doing anything sensible apart from
> >>> returning an error code, in the best case scenario.
> >>>
> >>
> >> I had pointed out this earlier, even if we make it compile there's
> >> every possibility that it can blow up if some vendor adds S- states
> >> to their ACPI tables. One clear reason why it could blow up is:
> >>
> >> "
> >>       /* This violates the spec but is required for bug compatibility. */
> >>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> >> "
> >>
> >> I don't think this can ever work on ARM platforms. So better to fix it
> >> properly.

Agree.

> > How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
> > for the cache behavior, because an embedded x86 box would still probably
> > define those, but removing the hooks on ARM may make sense.
> 
> As Graeme said in the reply, for sleep we are doing the same thing as
> ia64 in stubbing out the functions,

Sorry, that's not really an argument.

> and before that we are trying to remove
> the hooks on ARM by introducing more stubs and making things more complicated.

I still think you can make things simpler by not compiling the code
in question because we'll never implement such hooks on arm64, they
don't make sense from an architecture perspective (whole cache flushing
with MMU enabled cannot be done).

> I agree that we should rework the ACPI core to make sleep/cache related
> stuff compatible with ARM, but I think we may not do this in one go, it will
> need incremental changes over the next couple of years as real hardware
> starts to appear and we finalise the standards to support this.
> 
> Now, as we list in the arm-acpi.txt doc, power management of sleep states
> and CPU power/frequency control are not well defined in ACPI spec for ARM,
> we need some time to finalize the standard and then we know how to implement
> that in a good shape.
> 
> ACPI 5.1 already fixed lots missing features for ARM64 and provide the
> fundamental needs to bring up the basic system for ARM server, power
> management is not critical at now, so why not fix/implement it later?

I don't have a problem with implementing (rather than fixing) power
management later, once the ACPI spec covers it. But the point a few of
us were trying to make is that even when ACPI spec is updated, the
current power management code and hooks still won't make sense on ARM.
The best is to avoid compiling it for ARM now and look at refactoring it
later to accommodate ARM ACPI.
Grant Likely Sept. 10, 2014, 9:37 p.m. UTC | #13
On Tue, 9 Sep 2014 17:26:49 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > index c96172a..fb7cc0e 100644
> > --- a/arch/arm64/kernel/setup.c
> > +++ b/arch/arm64/kernel/setup.c
> > @@ -43,6 +43,7 @@
> >  #include <linux/of_fdt.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/efi.h>
> > +#include <linux/acpi.h>
> >  
> >  #include <asm/fixmap.h>
> >  #include <asm/cpu.h>
> > @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p)
> >  	efi_init();
> >  	arm64_memblock_init();
> >  
> > +	/* Parse the ACPI tables for possible boot-time configuration */
> > +	acpi_boot_table_init();
> 
> Is there any dummy acpi_boot_table_init() when !CONFIG_ACPI? I couldn't
> see one in this patch, so I don't see how this would compile when ACPI
> is disabled.

It's in include/linux/acpi.h in the !CONFIG_ACPI section.

g.
Grant Likely Sept. 10, 2014, 9:41 p.m. UTC | #14
On Wed, 10 Sep 2014 19:13:51 +0800, Hanjun Guo <guohanjun@huawei.com> wrote:
> On 2014/9/10 3:06, Jon Masters wrote:
> > On 09/09/2014 02:05 PM, Sudeep Holla wrote:
> >>
> >>
> >> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
> >>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
> >>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> >>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> >>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> >>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..3899ee6
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/arch/arm64/include/asm/acenv.h
> >>>>>>> @@ -0,0 +1,18 @@
> >>>>>>> +/*
> >>>>>>> + * ARM64 specific ACPICA environments and implementation
> >>>>>>> + *
> >>>>>>> + * Copyright (C) 2014, Linaro Ltd.
> >>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> >>>>>>> + *
> >>>>>>> + * This program is free software; you can redistribute it and/or modify
> >>>>>>> + * it under the terms of the GNU General Public License version 2 as
> >>>>>>> + * published by the Free Software Foundation.
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +#ifndef _ASM_ACENV_H
> >>>>>>> +#define _ASM_ACENV_H
> >>>>>>> +
> >>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> >>>>>>
> >>>>>> Does this mean that it will be supported at some point? Looking at the
> >>>>>> places where this function is called, I don't really see how this would
> >>>>>> ever work on ARM. Which means that we add such macro just to be able to
> >>>>>> compile code that would never be used on arm64. I would rather see the
> >>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> >>>>>
> >>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
> >>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> >>>>
> >>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> >>>> we don't get S1, S2, or S3 states either.
> >>>>
> >>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
> >>>>> but it's optional to implement C3 and early 64-bit ARM systems should
> >>>>> not report Wbindv flags in the FADT anyway.
> >>>>
> >>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> >>>> ARM system to set either of the WBINVD flags.
> >>>>
> >>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> >>>>> disabling C3 support, while also allowing for use of _CST objects to
> >>>>> define more flexible C-States later on.
> >>>>
> >>>> It sounds like we should be sanity checking these in the arm64 ACPI code
> >>>> for the moment. I don't want us to discover that current platforms
> >>>> report the wrong thing only when new platforms come out that might
> >>>> actually report things correctly.
> >>>
> >>> I think that the kernel must ignore most of the stuff mentioned above
> >>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
> >>> is not even there. The problem is trying to compile code that basically
> >>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
> >>> Catalin pointed out.
> >>>
> >>> I understand it is compiled in by default on x86, but that's not a reason
> >>> why we should add empty hooks all over the place to compile code that
> >>> does not stand a chance to be doing anything sensible apart from
> >>> returning an error code, in the best case scenario.
> >>>
> >>
> >> I had pointed out this earlier, even if we make it compile there's
> >> every possibility that it can blow up if some vendor adds S- states
> >> to their ACPI tables. One clear reason why it could blow up is:
> >>
> >> "
> >>       /* This violates the spec but is required for bug compatibility. */
> >>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> >> "
> >>
> >> I don't think this can ever work on ARM platforms. So better to fix it
> >> properly.
> > 
> > Hanjun,
> > 
> > How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
> > for the cache behavior, because an embedded x86 box would still probably
> > define those, but removing the hooks on ARM may make sense.
> 
> As Graeme said in the reply, for sleep we are doing the same thing as
> ia64 in stubbing out the functions, and before that we are trying to remove
> the hooks on ARM by introducing more stubs and making things more complicated.
> 
> I agree that we should rework the ACPI core to make sleep/cache related
> stuff compatible with ARM, but I think we may not do this in one go, it will
> need incremental changes over the next couple of years as real hardware
> starts to appear and we finalise the standards to support this.

Yes, reworking acpica is out of scope for this patch set. I don't think
it makes sense to try and do that refactoring here.

g.
Grant Likely Sept. 10, 2014, 9:51 p.m. UTC | #15
On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
> > On 2014/9/10 3:06, Jon Masters wrote:
> > > On 09/09/2014 02:05 PM, Sudeep Holla wrote:
> > >> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
> > >>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
> > >>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> > >>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> > >>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> > >>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> > >>>>>>> new file mode 100644
> > >>>>>>> index 0000000..3899ee6
> > >>>>>>> --- /dev/null
> > >>>>>>> +++ b/arch/arm64/include/asm/acenv.h
> > >>>>>>> @@ -0,0 +1,18 @@
> > >>>>>>> +/*
> > >>>>>>> + * ARM64 specific ACPICA environments and implementation
> > >>>>>>> + *
> > >>>>>>> + * Copyright (C) 2014, Linaro Ltd.
> > >>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> > >>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> > >>>>>>> + *
> > >>>>>>> + * This program is free software; you can redistribute it and/or modify
> > >>>>>>> + * it under the terms of the GNU General Public License version 2 as
> > >>>>>>> + * published by the Free Software Foundation.
> > >>>>>>> + */
> > >>>>>>> +
> > >>>>>>> +#ifndef _ASM_ACENV_H
> > >>>>>>> +#define _ASM_ACENV_H
> > >>>>>>> +
> > >>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> > >>>>>>
> > >>>>>> Does this mean that it will be supported at some point? Looking at the
> > >>>>>> places where this function is called, I don't really see how this would
> > >>>>>> ever work on ARM. Which means that we add such macro just to be able to
> > >>>>>> compile code that would never be used on arm64. I would rather see the
> > >>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> > >>>>>
> > >>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
> > >>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> > >>>>
> > >>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> > >>>> we don't get S1, S2, or S3 states either.
> > >>>>
> > >>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
> > >>>>> but it's optional to implement C3 and early 64-bit ARM systems should
> > >>>>> not report Wbindv flags in the FADT anyway.
> > >>>>
> > >>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> > >>>> ARM system to set either of the WBINVD flags.
> > >>>>
> > >>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> > >>>>> disabling C3 support, while also allowing for use of _CST objects to
> > >>>>> define more flexible C-States later on.
> > >>>>
> > >>>> It sounds like we should be sanity checking these in the arm64 ACPI code
> > >>>> for the moment. I don't want us to discover that current platforms
> > >>>> report the wrong thing only when new platforms come out that might
> > >>>> actually report things correctly.
> > >>>
> > >>> I think that the kernel must ignore most of the stuff mentioned above
> > >>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
> > >>> is not even there. The problem is trying to compile code that basically
> > >>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
> > >>> Catalin pointed out.
> > >>>
> > >>> I understand it is compiled in by default on x86, but that's not a reason
> > >>> why we should add empty hooks all over the place to compile code that
> > >>> does not stand a chance to be doing anything sensible apart from
> > >>> returning an error code, in the best case scenario.
> > >>>
> > >>
> > >> I had pointed out this earlier, even if we make it compile there's
> > >> every possibility that it can blow up if some vendor adds S- states
> > >> to their ACPI tables. One clear reason why it could blow up is:
> > >>
> > >> "
> > >>       /* This violates the spec but is required for bug compatibility. */
> > >>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> > >> "
> > >>
> > >> I don't think this can ever work on ARM platforms. So better to fix it
> > >> properly.
> 
> Agree.
> 
> > > How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
> > > for the cache behavior, because an embedded x86 box would still probably
> > > define those, but removing the hooks on ARM may make sense.
> > 
> > As Graeme said in the reply, for sleep we are doing the same thing as
> > ia64 in stubbing out the functions,
> 
> Sorry, that's not really an argument.
> 
> > and before that we are trying to remove
> > the hooks on ARM by introducing more stubs and making things more complicated.
> 
> I still think you can make things simpler by not compiling the code
> in question because we'll never implement such hooks on arm64, they
> don't make sense from an architecture perspective (whole cache flushing
> with MMU enabled cannot be done).
> 
> > I agree that we should rework the ACPI core to make sleep/cache related
> > stuff compatible with ARM, but I think we may not do this in one go, it will
> > need incremental changes over the next couple of years as real hardware
> > starts to appear and we finalise the standards to support this.
> > 
> > Now, as we list in the arm-acpi.txt doc, power management of sleep states
> > and CPU power/frequency control are not well defined in ACPI spec for ARM,
> > we need some time to finalize the standard and then we know how to implement
> > that in a good shape.
> > 
> > ACPI 5.1 already fixed lots missing features for ARM64 and provide the
> > fundamental needs to bring up the basic system for ARM server, power
> > management is not critical at now, so why not fix/implement it later?
> 
> I don't have a problem with implementing (rather than fixing) power
> management later, once the ACPI spec covers it. But the point a few of
> us were trying to make is that even when ACPI spec is updated, the
> current power management code and hooks still won't make sense on ARM.
> The best is to avoid compiling it for ARM now and look at refactoring it
> later to accommodate ARM ACPI.

I disagree strongly here. We're talking about a library of code that is
working on x86 and ia64, but hasn't been tuned for ARM. Trying to
refactor it first, and then get it compiling for ARM is entirely the
wrong way around. The best way to get that code refactored for
ARM is to get it compiling first, and then refactor it to remove the
unnecessary stubs. That makes it a whole lot easier to make the
arguements about why the changes are needed. Otherwise it just shows
churn on the ACPICA side without any evidence of why the change is
good. Trying to refactor first also has the risk of breaking things
that work now in the name of "refactoring" and not being able to easily
bisect it for ARM. That's just madness.

Aside from a slightly larger kernel, there is no downside to using
stubs for now.

Getting things working is the first step. Then we've got a good base to
launch the refactoring from.

g.
Catalin Marinas Sept. 11, 2014, 11:01 a.m. UTC | #16
On Wed, Sep 10, 2014 at 10:51:30PM +0100, Grant Likely wrote:
> On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
> > > I agree that we should rework the ACPI core to make sleep/cache related
> > > stuff compatible with ARM, but I think we may not do this in one go, it will
> > > need incremental changes over the next couple of years as real hardware
> > > starts to appear and we finalise the standards to support this.
> > > 
> > > Now, as we list in the arm-acpi.txt doc, power management of sleep states
> > > and CPU power/frequency control are not well defined in ACPI spec for ARM,
> > > we need some time to finalize the standard and then we know how to implement
> > > that in a good shape.
> > > 
> > > ACPI 5.1 already fixed lots missing features for ARM64 and provide the
> > > fundamental needs to bring up the basic system for ARM server, power
> > > management is not critical at now, so why not fix/implement it later?
> > 
> > I don't have a problem with implementing (rather than fixing) power
> > management later, once the ACPI spec covers it. But the point a few of
> > us were trying to make is that even when ACPI spec is updated, the
> > current power management code and hooks still won't make sense on ARM.
> > The best is to avoid compiling it for ARM now and look at refactoring it
> > later to accommodate ARM ACPI.
> 
> I disagree strongly here. We're talking about a library of code that is
> working on x86 and ia64, but hasn't been tuned for ARM.

I think where we disagree is the "tuning" part. If it's just that, it
would be fine, but there are fundamental differences in the
architectures that the above would not even make sense on ARM (so it's
more like rewriting than tuning).

> Trying to
> refactor it first, and then get it compiling for ARM is entirely the
> wrong way around.

Note that I explicitly stated "refactoring it later". I have not asked
for the existing code to be refactored _now_ as it's not even clear how
it would look like on ARM (what's clear though is that its behaviour is
_undefined_).

> The best way to get that code refactored for
> ARM is to get it compiling first, and then refactor it to remove the
> unnecessary stubs. That makes it a whole lot easier to make the
> arguements about why the changes are needed. Otherwise it just shows
> churn on the ACPICA side without any evidence of why the change is
> good. Trying to refactor first also has the risk of breaking things
> that work now in the name of "refactoring" and not being able to easily
> bisect it for ARM. That's just madness.

I agree with not starting the refactoring now. But is it difficult to
make it compilable based on something like CONFIG_ACPI_SLEEP and the
latter depend on !ARM64 until the spec is clear?

> Aside from a slightly larger kernel, there is no downside to using
> stubs for now.

There is a serious downside - code with _undefined_ behaviour on arm64
that may get executed depending on the content of the ACPI tables. I'm
sure it's a lot of fun debugging this.
Grant Likely Sept. 14, 2014, 3:40 p.m. UTC | #17
On Thu, 11 Sep 2014 12:01:46 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Sep 10, 2014 at 10:51:30PM +0100, Grant Likely wrote:
> > On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
> > > > I agree that we should rework the ACPI core to make sleep/cache related
> > > > stuff compatible with ARM, but I think we may not do this in one go, it will
> > > > need incremental changes over the next couple of years as real hardware
> > > > starts to appear and we finalise the standards to support this.
> > > > 
> > > > Now, as we list in the arm-acpi.txt doc, power management of sleep states
> > > > and CPU power/frequency control are not well defined in ACPI spec for ARM,
> > > > we need some time to finalize the standard and then we know how to implement
> > > > that in a good shape.
> > > > 
> > > > ACPI 5.1 already fixed lots missing features for ARM64 and provide the
> > > > fundamental needs to bring up the basic system for ARM server, power
> > > > management is not critical at now, so why not fix/implement it later?
> > > 
> > > I don't have a problem with implementing (rather than fixing) power
> > > management later, once the ACPI spec covers it. But the point a few of
> > > us were trying to make is that even when ACPI spec is updated, the
> > > current power management code and hooks still won't make sense on ARM.
> > > The best is to avoid compiling it for ARM now and look at refactoring it
> > > later to accommodate ARM ACPI.
> > 
> > I disagree strongly here. We're talking about a library of code that is
> > working on x86 and ia64, but hasn't been tuned for ARM.
> 
> I think where we disagree is the "tuning" part. If it's just that, it
> would be fine, but there are fundamental differences in the
> architectures that the above would not even make sense on ARM (so it's
> more like rewriting than tuning).
> 
> > Trying to
> > refactor it first, and then get it compiling for ARM is entirely the
> > wrong way around.
> 
> Note that I explicitly stated "refactoring it later". I have not asked
> for the existing code to be refactored _now_ as it's not even clear how
> it would look like on ARM (what's clear though is that its behaviour is
> _undefined_).
> 
> > The best way to get that code refactored for
> > ARM is to get it compiling first, and then refactor it to remove the
> > unnecessary stubs. That makes it a whole lot easier to make the
> > arguements about why the changes are needed. Otherwise it just shows
> > churn on the ACPICA side without any evidence of why the change is
> > good. Trying to refactor first also has the risk of breaking things
> > that work now in the name of "refactoring" and not being able to easily
> > bisect it for ARM. That's just madness.
> 
> I agree with not starting the refactoring now. But is it difficult to
> make it compilable based on something like CONFIG_ACPI_SLEEP and the
> latter depend on !ARM64 until the spec is clear?

This particular issue is pretty much moot now. The latest series was
able to compile out the sleep functions. If they hadn't however, we
weren't at any risk. Empty hooks have no impact, and even if the
behaviour is defined in a future revision of the spec, they will remain
noops for ACPI 5.1 systems.
> 
> > Aside from a slightly larger kernel, there is no downside to using
> > stubs for now.
> 
> There is a serious downside - code with _undefined_ behaviour on arm64
> that may get executed depending on the content of the ACPI tables. I'm
> sure it's a lot of fun debugging this.

It's really easy for us to deal with this. For ACPI 5.1, we don't do
anything, and we should never try to do anything with those states.  If
and when a future revision of the ACPI spec appears that does define
those states, then we will use them, but only if the reported ACPI
version is high enough.

g.
Catalin Marinas Sept. 14, 2014, 9:59 p.m. UTC | #18
On 14 Sep 2014, at 16:40, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, 11 Sep 2014 12:01:46 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> On Wed, Sep 10, 2014 at 10:51:30PM +0100, Grant Likely wrote:
>>> On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>>> On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
>>>>> I agree that we should rework the ACPI core to make sleep/cache related
>>>>> stuff compatible with ARM, but I think we may not do this in one go, it will
>>>>> need incremental changes over the next couple of years as real hardware
>>>>> starts to appear and we finalise the standards to support this.
>>>>> 
>>>>> Now, as we list in the arm-acpi.txt doc, power management of sleep states
>>>>> and CPU power/frequency control are not well defined in ACPI spec for ARM,
>>>>> we need some time to finalize the standard and then we know how to implement
>>>>> that in a good shape.
>>>>> 
>>>>> ACPI 5.1 already fixed lots missing features for ARM64 and provide the
>>>>> fundamental needs to bring up the basic system for ARM server, power
>>>>> management is not critical at now, so why not fix/implement it later?
>>>> 
>>>> I don't have a problem with implementing (rather than fixing) power
>>>> management later, once the ACPI spec covers it. But the point a few of
>>>> us were trying to make is that even when ACPI spec is updated, the
>>>> current power management code and hooks still won't make sense on ARM.
>>>> The best is to avoid compiling it for ARM now and look at refactoring it
>>>> later to accommodate ARM ACPI.
>>> 
>>> I disagree strongly here. We're talking about a library of code that is
>>> working on x86 and ia64, but hasn't been tuned for ARM.
>> 
>> I think where we disagree is the "tuning" part. If it's just that, it
>> would be fine, but there are fundamental differences in the
>> architectures that the above would not even make sense on ARM (so it's
>> more like rewriting than tuning).
>> 
>>> Trying to
>>> refactor it first, and then get it compiling for ARM is entirely the
>>> wrong way around.
>> 
>> Note that I explicitly stated "refactoring it later". I have not asked
>> for the existing code to be refactored _now_ as it's not even clear how
>> it would look like on ARM (what's clear though is that its behaviour is
>> _undefined_).
>> 
>>> The best way to get that code refactored for
>>> ARM is to get it compiling first, and then refactor it to remove the
>>> unnecessary stubs. That makes it a whole lot easier to make the
>>> arguements about why the changes are needed. Otherwise it just shows
>>> churn on the ACPICA side without any evidence of why the change is
>>> good. Trying to refactor first also has the risk of breaking things
>>> that work now in the name of "refactoring" and not being able to easily
>>> bisect it for ARM. That's just madness.
>> 
>> I agree with not starting the refactoring now. But is it difficult to
>> make it compilable based on something like CONFIG_ACPI_SLEEP and the
>> latter depend on !ARM64 until the spec is clear?
> 
> This particular issue is pretty much moot now. The latest series was
> able to compile out the sleep functions.

That’s what I was looking for.

> If they hadn't however, we
> weren't at any risk. Empty hooks have no impact, and even if the
> behaviour is defined in a future revision of the spec, they will remain
> noops for ACPI 5.1 systems.

It’s not the hooks I’m worried about but the code that calls those
hooks. As Sudeep pointed out, the kernel will probably panic before even
reaching the hooks. IOW, would you have been OK with simply defining
these hooks as BUG()?

In principle, I’m against compiling in code paths that you can never
test, it just goes against the idea of a “robust" system.

>>> Aside from a slightly larger kernel, there is no downside to using
>>> stubs for now.
>> 
>> There is a serious downside - code with _undefined_ behaviour on arm64
>> that may get executed depending on the content of the ACPI tables. I'm
>> sure it's a lot of fun debugging this.
> 
> It's really easy for us to deal with this. For ACPI 5.1, we don't do
> anything, and we should never try to do anything with those states.  If
> and when a future revision of the ACPI spec appears that does define
> those states, then we will use them, but only if the reported ACPI
> version is high enough.

It’s the other way around: the reported ACPI version (by firmware) is
high enough but the kernel code is not updated beyond 5.1 and may run
the current ARM-undefined ACPI sleep code.

Catalin
Grant Likely Sept. 15, 2014, 3:53 a.m. UTC | #19
On Sun, Sep 14, 2014 at 2:59 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On 14 Sep 2014, at 16:40, Grant Likely <grant.likely@linaro.org> wrote:
>> On Thu, 11 Sep 2014 12:01:46 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>> On Wed, Sep 10, 2014 at 10:51:30PM +0100, Grant Likely wrote:
>>>> On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>>>> On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
>>>>>> I agree that we should rework the ACPI core to make sleep/cache related
>>>>>> stuff compatible with ARM, but I think we may not do this in one go, it will
>>>>>> need incremental changes over the next couple of years as real hardware
>>>>>> starts to appear and we finalise the standards to support this.
>>>>>>
>>>>>> Now, as we list in the arm-acpi.txt doc, power management of sleep states
>>>>>> and CPU power/frequency control are not well defined in ACPI spec for ARM,
>>>>>> we need some time to finalize the standard and then we know how to implement
>>>>>> that in a good shape.
>>>>>>
>>>>>> ACPI 5.1 already fixed lots missing features for ARM64 and provide the
>>>>>> fundamental needs to bring up the basic system for ARM server, power
>>>>>> management is not critical at now, so why not fix/implement it later?
>>>>>
>>>>> I don't have a problem with implementing (rather than fixing) power
>>>>> management later, once the ACPI spec covers it. But the point a few of
>>>>> us were trying to make is that even when ACPI spec is updated, the
>>>>> current power management code and hooks still won't make sense on ARM.
>>>>> The best is to avoid compiling it for ARM now and look at refactoring it
>>>>> later to accommodate ARM ACPI.
>>>>
>>>> I disagree strongly here. We're talking about a library of code that is
>>>> working on x86 and ia64, but hasn't been tuned for ARM.
>>>
>>> I think where we disagree is the "tuning" part. If it's just that, it
>>> would be fine, but there are fundamental differences in the
>>> architectures that the above would not even make sense on ARM (so it's
>>> more like rewriting than tuning).
>>>
>>>> Trying to
>>>> refactor it first, and then get it compiling for ARM is entirely the
>>>> wrong way around.
>>>
>>> Note that I explicitly stated "refactoring it later". I have not asked
>>> for the existing code to be refactored _now_ as it's not even clear how
>>> it would look like on ARM (what's clear though is that its behaviour is
>>> _undefined_).
>>>
>>>> The best way to get that code refactored for
>>>> ARM is to get it compiling first, and then refactor it to remove the
>>>> unnecessary stubs. That makes it a whole lot easier to make the
>>>> arguements about why the changes are needed. Otherwise it just shows
>>>> churn on the ACPICA side without any evidence of why the change is
>>>> good. Trying to refactor first also has the risk of breaking things
>>>> that work now in the name of "refactoring" and not being able to easily
>>>> bisect it for ARM. That's just madness.
>>>
>>> I agree with not starting the refactoring now. But is it difficult to
>>> make it compilable based on something like CONFIG_ACPI_SLEEP and the
>>> latter depend on !ARM64 until the spec is clear?
>>
>> This particular issue is pretty much moot now. The latest series was
>> able to compile out the sleep functions.
>
> That's what I was looking for.
>
>> If they hadn't however, we
>> weren't at any risk. Empty hooks have no impact, and even if the
>> behaviour is defined in a future revision of the spec, they will remain
>> noops for ACPI 5.1 systems.
>
> It's not the hooks I'm worried about but the code that calls those
> hooks. As Sudeep pointed out, the kernel will probably panic before even
> reaching the hooks. IOW, would you have been OK with simply defining
> these hooks as BUG()?

Yes.

> In principle, I'm against compiling in code paths that you can never
> test, it just goes against the idea of a "robust" system.

I don't dispute the code paths should be removed, but rather that
removing them shouldn't be a prerequisite for merging it into
mainline.

I went through exactly this same exercise when getting DT to be cross
platform. There was all kinds of stuff that got compiled into
Microblaze, and later ARM that was only applicable for PowerPC. The
refactoring came later and incrementally. It would have been a much
more difficult job if I had to maintain the patches out of tree until
the common code was near perfect. It would have taken a lot longer
too.

>>>> Aside from a slightly larger kernel, there is no downside to using
>>>> stubs for now.
>>>
>>> There is a serious downside - code with _undefined_ behaviour on arm64
>>> that may get executed depending on the content of the ACPI tables. I'm
>>> sure it's a lot of fun debugging this.
>>
>> It's really easy for us to deal with this. For ACPI 5.1, we don't do
>> anything, and we should never try to do anything with those states.  If
>> and when a future revision of the ACPI spec appears that does define
>> those states, then we will use them, but only if the reported ACPI
>> version is high enough.
>
> It's the other way around: the reported ACPI version (by firmware) is
> high enough but the kernel code is not updated beyond 5.1 and may run
> the current ARM-undefined ACPI sleep code.

That is fine too. Future ACPI platforms are also to be backwards
compatible. Platforms are not allowed to use new features if the OS
only supports an old rev of the spec.

g.
Lv Zheng Sept. 16, 2014, 5:29 a.m. UTC | #20
Hi,

> From: Grant Likely [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
> Sent: Thursday, September 11, 2014 5:52 AM
> 
> On Wed, 10 Sep 2014 13:33:52 +0100, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Sep 10, 2014 at 12:13:51PM +0100, Hanjun Guo wrote:
> > > On 2014/9/10 3:06, Jon Masters wrote:
> > > > On 09/09/2014 02:05 PM, Sudeep Holla wrote:
> > > >> On 09/09/14 18:50, Lorenzo Pieralisi wrote:
> > > >>> On Tue, Sep 09, 2014 at 06:15:41PM +0100, Mark Rutland wrote:
> > > >>>> On Tue, Sep 09, 2014 at 05:41:51PM +0100, Jon Masters wrote:
> > > >>>>> On 09/09/2014 12:26 PM, Catalin Marinas wrote:
> > > >>>>>> On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:
> > > >>>>>>> diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
> > > >>>>>>> new file mode 100644
> > > >>>>>>> index 0000000..3899ee6
> > > >>>>>>> --- /dev/null
> > > >>>>>>> +++ b/arch/arm64/include/asm/acenv.h
> > > >>>>>>> @@ -0,0 +1,18 @@
> > > >>>>>>> +/*
> > > >>>>>>> + * ARM64 specific ACPICA environments and implementation
> > > >>>>>>> + *
> > > >>>>>>> + * Copyright (C) 2014, Linaro Ltd.
> > > >>>>>>> + *   Author: Hanjun Guo <hanjun.guo@linaro.org>
> > > >>>>>>> + *   Author: Graeme Gregory <graeme.gregory@linaro.org>
> > > >>>>>>> + *
> > > >>>>>>> + * This program is free software; you can redistribute it and/or modify
> > > >>>>>>> + * it under the terms of the GNU General Public License version 2 as
> > > >>>>>>> + * published by the Free Software Foundation.
> > > >>>>>>> + */
> > > >>>>>>> +
> > > >>>>>>> +#ifndef _ASM_ACENV_H
> > > >>>>>>> +#define _ASM_ACENV_H
> > > >>>>>>> +
> > > >>>>>>> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> > > >>>>>>
> > > >>>>>> Does this mean that it will be supported at some point? Looking at the
> > > >>>>>> places where this function is called, I don't really see how this would
> > > >>>>>> ever work on ARM. Which means that we add such macro just to be able to
> > > >>>>>> compile code that would never be used on arm64. I would rather see the
> > > >>>>>> relevant ACPI files only compiled on x86/IA-64 rather than arm64.
> > > >>>>>
> > > >>>>> That specific cache behavior is a part of e.g. ACPI C3 state support
> > > >>>>> (e.g. ACPI5.1 8.1.4 Processor Power State C3).
> > > >>>>
> > > >>>> Per table 5-35, if neither WBINVD or WBINVD_FLUSH are set in the FADT,
> > > >>>> we don't get S1, S2, or S3 states either.
> > > >>>>
> > > >>>>> As you note, it's not going to work on 64-bit ARM as it does on x86,
> > > >>>>> but it's optional to implement C3 and early 64-bit ARM systems should
> > > >>>>> not report Wbindv flags in the FADT anyway.
> > > >>>>
> > > >>>> Unless the arm cache architecture changes, I wouldn't expect any 64-bit
> > > >>>> ARM system to set either of the WBINVD flags.
> > > >>>>
> > > >>>>> They can also set FADT.P_LVL3_LAT > 1000, which has the effect of
> > > >>>>> disabling C3 support, while also allowing for use of _CST objects to
> > > >>>>> define more flexible C-States later on.
> > > >>>>
> > > >>>> It sounds like we should be sanity checking these in the arm64 ACPI code
> > > >>>> for the moment. I don't want us to discover that current platforms
> > > >>>> report the wrong thing only when new platforms come out that might
> > > >>>> actually report things correctly.
> > > >>>
> > > >>> I think that the kernel must ignore most of the stuff mentioned above
> > > >>> in HW_REDUCED_ACPI mode. And to be frank I still think that the problem
> > > >>> is not even there. The problem is trying to compile code that basically
> > > >>> has no defined behaviour - ie it is unspecified - on ARM64, that's what
> > > >>> Catalin pointed out.
> > > >>>
> > > >>> I understand it is compiled in by default on x86, but that's not a reason
> > > >>> why we should add empty hooks all over the place to compile code that
> > > >>> does not stand a chance to be doing anything sensible apart from
> > > >>> returning an error code, in the best case scenario.
> > > >>>
> > > >>
> > > >> I had pointed out this earlier, even if we make it compile there's
> > > >> every possibility that it can blow up if some vendor adds S- states
> > > >> to their ACPI tables. One clear reason why it could blow up is:
> > > >>
> > > >> "
> > > >>       /* This violates the spec but is required for bug compatibility. */
> > > >>       acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> > > >> "
> > > >>
> > > >> I don't think this can ever work on ARM platforms. So better to fix it
> > > >> properly.
> >
> > Agree.
> >
> > > > How do you want to proceed? I'm not sure it should be !HW_REDUCED_MODE
> > > > for the cache behavior, because an embedded x86 box would still probably
> > > > define those, but removing the hooks on ARM may make sense.
> > >
> > > As Graeme said in the reply, for sleep we are doing the same thing as
> > > ia64 in stubbing out the functions,
> >
> > Sorry, that's not really an argument.
> >
> > > and before that we are trying to remove
> > > the hooks on ARM by introducing more stubs and making things more complicated.
> >
> > I still think you can make things simpler by not compiling the code
> > in question because we'll never implement such hooks on arm64, they
> > don't make sense from an architecture perspective (whole cache flushing
> > with MMU enabled cannot be done).
> >
> > > I agree that we should rework the ACPI core to make sleep/cache related
> > > stuff compatible with ARM, but I think we may not do this in one go, it will
> > > need incremental changes over the next couple of years as real hardware
> > > starts to appear and we finalise the standards to support this.
> > >
> > > Now, as we list in the arm-acpi.txt doc, power management of sleep states
> > > and CPU power/frequency control are not well defined in ACPI spec for ARM,
> > > we need some time to finalize the standard and then we know how to implement
> > > that in a good shape.
> > >
> > > ACPI 5.1 already fixed lots missing features for ARM64 and provide the
> > > fundamental needs to bring up the basic system for ARM server, power
> > > management is not critical at now, so why not fix/implement it later?
> >
> > I don't have a problem with implementing (rather than fixing) power
> > management later, once the ACPI spec covers it. But the point a few of
> > us were trying to make is that even when ACPI spec is updated, the
> > current power management code and hooks still won't make sense on ARM.
> > The best is to avoid compiling it for ARM now and look at refactoring it
> > later to accommodate ARM ACPI.
> 
> I disagree strongly here. We're talking about a library of code that is
> working on x86 and ia64, but hasn't been tuned for ARM. Trying to
> refactor it first, and then get it compiling for ARM is entirely the
> wrong way around. The best way to get that code refactored for
> ARM is to get it compiling first, and then refactor it to remove the
> unnecessary stubs. That makes it a whole lot easier to make the
> arguements about why the changes are needed. Otherwise it just shows
> churn on the ACPICA side without any evidence of why the change is
> good. Trying to refactor first also has the risk of breaking things
> that work now in the name of "refactoring" and not being able to easily
> bisect it for ARM. That's just madness.
> 
> Aside from a slightly larger kernel, there is no downside to using
> stubs for now.
> 
> Getting things working is the first step. Then we've got a good base to
> launch the refactoring from.

IMO, it is not a good timing to talk about ACPICA sleep code refactoring.
The current power management code and hooks also don't make sense to Intel platforms.
The proof is the Xen hooks which is currently implemented in the hackish style.
In order to cleanup such Xen hooks, the sleep code need to be refactored.

IMO, you may have things proceeded without doing any changes to the sleep code.
When the power management requirement for ARM is fixed, you can also introduce hackish code to support it.
And from that point, we may be able to provide a well-shaped refactoring work.

Best regards
-Lv
diff mbox

Patch

diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
new file mode 100644
index 0000000..3899ee6
--- /dev/null
+++ b/arch/arm64/include/asm/acenv.h
@@ -0,0 +1,18 @@ 
+/*
+ * ARM64 specific ACPICA environments and implementation
+ *
+ * Copyright (C) 2014, Linaro Ltd.
+ *   Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *   Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _ASM_ACENV_H
+#define _ASM_ACENV_H
+
+#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
+
+#endif /* _ASM_ACENV_H */
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
new file mode 100644
index 0000000..8b837ab
--- /dev/null
+++ b/arch/arm64/include/asm/acpi.h
@@ -0,0 +1,45 @@ 
+/*
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation;
+ */
+
+#ifndef _ASM_ACPI_H
+#define _ASM_ACPI_H
+
+/* Basic configuration for ACPI */
+#ifdef	CONFIG_ACPI
+#define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
+extern int acpi_disabled;
+extern int acpi_noirq;
+extern int acpi_pci_disabled;
+
+static inline void disable_acpi(void)
+{
+	acpi_disabled = 1;
+	acpi_pci_disabled = 1;
+	acpi_noirq = 1;
+}
+
+/*
+ * It's used from ACPI core in kdump to boot UP system with SMP kernel,
+ * with this check the ACPI core will not override the CPU index
+ * obtained from GICC with 0 and not print some error message as well.
+ * Since MADT must provide at least one GICC structure for GIC
+ * initialization, CPU will be always available in MADT on ARM64.
+ */
+static inline bool acpi_has_cpu_in_madt(void)
+{
+	return true;
+}
+
+static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+
+#endif /* CONFIG_ACPI */
+
+#endif /*_ASM_ACPI_H*/
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index df7ef87..29ea7d6 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -29,6 +29,7 @@  arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
 arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
+arm64-obj-$(CONFIG_ACPI)		+= acpi.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
new file mode 100644
index 0000000..9252f72
--- /dev/null
+++ b/arch/arm64/kernel/acpi.c
@@ -0,0 +1,69 @@ 
+/*
+ *  ARM64 Specific Low-Level ACPI Boot Support
+ *
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *	Author: Tomasz Nowicki <tomasz.nowicki@linaro.org>
+ *	Author: Naresh Bhat <naresh.bhat@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/cpumask.h>
+#include <linux/memblock.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/bootmem.h>
+#include <linux/smp.h>
+
+int acpi_noirq;			/* skip ACPI IRQ initialization */
+int acpi_disabled;
+EXPORT_SYMBOL(acpi_disabled);
+
+int acpi_pci_disabled;		/* skip ACPI PCI scan and IRQ initialization */
+EXPORT_SYMBOL(acpi_pci_disabled);
+
+/*
+ * __acpi_map_table() will be called before page_init(), so early_ioremap()
+ * or early_memremap() should be called here to for ACPI table mapping.
+ */
+char *__init __acpi_map_table(unsigned long phys, unsigned long size)
+{
+	if (!phys || !size)
+		return NULL;
+
+	return early_memremap(phys, size);
+}
+
+void __init __acpi_unmap_table(char *map, unsigned long size)
+{
+	if (!map || !size)
+		return;
+
+	early_memunmap(map, size);
+}
+
+/*
+ * acpi_boot_table_init() called from setup_arch(), always.
+ *	1. find RSDP and get its address, and then find XSDT
+ *	2. extract all tables and checksums them all
+ *
+ * We can parse ACPI boot-time tables such as MADT after
+ * this function is called.
+ */
+void __init acpi_boot_table_init(void)
+{
+	/* If acpi_disabled, bail out */
+	if (acpi_disabled)
+		return;
+
+	/* Initialize the ACPI boot-time table parser. */
+	if (acpi_table_init())
+		disable_acpi();
+}
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index c96172a..fb7cc0e 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -43,6 +43,7 @@ 
 #include <linux/of_fdt.h>
 #include <linux/of_platform.h>
 #include <linux/efi.h>
+#include <linux/acpi.h>
 
 #include <asm/fixmap.h>
 #include <asm/cpu.h>
@@ -385,6 +386,9 @@  void __init setup_arch(char **cmdline_p)
 	efi_init();
 	arm64_memblock_init();
 
+	/* Parse the ACPI tables for possible boot-time configuration */
+	acpi_boot_table_init();
+
 	paging_init();
 	request_standard_resources();