| Message ID | 1409583475-6978-3-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
One of the ARM guys pointed out the cache issue before I had meant to send this note then. Sorry.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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();