* [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:29 ` Rafael J. Wysocki
2013-11-22 6:14 ` Zheng, Lv
2013-11-10 1:36 ` [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode al.stone
` (11 subsequent siblings)
12 siblings, 2 replies; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Hanjun Guo, Al Stone
From: Al Stone <ahs3@redhat.com>
To enable the hardware reduced mode of ACPI on some platforms (such as
ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
to TRUE in the ACPICA source.
This can be done more resonably by introducing a kernel config item
to enable/disable ACPI_REDUCED_HARDWARE. We can then change the kernel
config instead of having to modify the kernel source directly to enable
the reduced hardware mode of ACPI.
Lv Zheng suggested that this configuration item does not belong in ACPICA,
the upstream source for much of the ACPI internals, but rather to the
Linux kernel itself. Hence, we introduce this flag so that we can make
ACPI_REDUCED_HARDWARE configurable. For the details of the discussion,
please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/Kconfig | 8 ++++++++
include/acpi/platform/aclinux.h | 4 ++++
2 files changed, 12 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 589da05..7bbd3b0 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -354,6 +354,14 @@ config ACPI_BGRT
data from the firmware boot splash. It will appear under
/sys/firmware/acpi/bgrt/ .
+config ACPI_REDUCED_HARDWARE
+ bool "Hardware-reduced ACPI support"
+ depends on !(IA64 || X86)
+ help
+ This config adds support for Hardware-reduced ACPI. When this option
+ is selected, will generate a specialized version of ACPICA that ONLY
+ supports the ACPI "reduced hardware".
+
source "drivers/acpi/apei/Kconfig"
endif # ACPI
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 28f4f4d..ae93a91 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -67,6 +67,10 @@
/* Host-dependent types and defines for in-kernel ACPICA */
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE
+#define ACPI_REDUCED_HARDWARE TRUE
+#endif
+
#define ACPI_MACHINE_WIDTH BITS_PER_LONG
#define ACPI_EXPORT_SYMBOL(symbol) EXPORT_SYMBOL(symbol);
#define strtoul simple_strtoul
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-10 1:36 ` [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode al.stone
@ 2013-11-17 22:29 ` Rafael J. Wysocki
[not found] ` <CAGHbJ3ArVr+4g8UHyxFSL9Bu2ehsUAqsapGuxYLgfoR4NfT02w@mail.gmail.com>
2013-11-22 6:14 ` Zheng, Lv
1 sibling, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:29 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone, Hanjun Guo
On Saturday, November 09, 2013 06:36:11 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> To enable the hardware reduced mode of ACPI on some platforms (such as
> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> to TRUE in the ACPICA source.
>
> This can be done more resonably by introducing a kernel config item
> to enable/disable ACPI_REDUCED_HARDWARE. We can then change the kernel
> config instead of having to modify the kernel source directly to enable
> the reduced hardware mode of ACPI.
>
> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> the upstream source for much of the ACPI internals, but rather to the
> Linux kernel itself. Hence, we introduce this flag so that we can make
> ACPI_REDUCED_HARDWARE configurable. For the details of the discussion,
> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/Kconfig | 8 ++++++++
> include/acpi/platform/aclinux.h | 4 ++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 589da05..7bbd3b0 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -354,6 +354,14 @@ config ACPI_BGRT
> data from the firmware boot splash. It will appear under
> /sys/firmware/acpi/bgrt/ .
>
> +config ACPI_REDUCED_HARDWARE
> + bool "Hardware-reduced ACPI support"
> + depends on !(IA64 || X86)
Why don't you use
depends on (ARM || ARM64)
here instead?
> + help
> + This config adds support for Hardware-reduced ACPI. When this option
> + is selected, will generate a specialized version of ACPICA that ONLY
> + supports the ACPI "reduced hardware".
> +
> source "drivers/acpi/apei/Kconfig"
>
> endif # ACPI
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 28f4f4d..ae93a91 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -67,6 +67,10 @@
>
> /* Host-dependent types and defines for in-kernel ACPICA */
>
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +#define ACPI_REDUCED_HARDWARE TRUE
> +#endif
> +
> #define ACPI_MACHINE_WIDTH BITS_PER_LONG
> #define ACPI_EXPORT_SYMBOL(symbol) EXPORT_SYMBOL(symbol);
> #define strtoul simple_strtoul
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* RE: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-10 1:36 ` [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode al.stone
2013-11-17 22:29 ` Rafael J. Wysocki
@ 2013-11-22 6:14 ` Zheng, Lv
2013-11-22 9:56 ` Hanjun Guo
1 sibling, 1 reply; 57+ messages in thread
From: Zheng, Lv @ 2013-11-22 6:14 UTC (permalink / raw)
To: al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone, Hanjun Guo
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of al.stone@linaro.org
> Sent: Sunday, November 10, 2013 9:36 AM
>
> From: Al Stone <ahs3@redhat.com>
>
> To enable the hardware reduced mode of ACPI on some platforms (such as
> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> to TRUE in the ACPICA source.
>
> This can be done more resonably by introducing a kernel config item
> to enable/disable ACPI_REDUCED_HARDWARE. We can then change the kernel
> config instead of having to modify the kernel source directly to enable
> the reduced hardware mode of ACPI.
>
> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> the upstream source for much of the ACPI internals, but rather to the
> Linux kernel itself. Hence, we introduce this flag so that we can make
> ACPI_REDUCED_HARDWARE configurable. For the details of the discussion,
> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/Kconfig | 8 ++++++++
> include/acpi/platform/aclinux.h | 4 ++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 589da05..7bbd3b0 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -354,6 +354,14 @@ config ACPI_BGRT
> data from the firmware boot splash. It will appear under
> /sys/firmware/acpi/bgrt/ .
>
> +config ACPI_REDUCED_HARDWARE
> + bool "Hardware-reduced ACPI support"
> + depends on !(IA64 || X86)
> + help
> + This config adds support for Hardware-reduced ACPI. When this option
> + is selected, will generate a specialized version of ACPICA that ONLY
> + supports the ACPI "reduced hardware".
> +
> source "drivers/acpi/apei/Kconfig"
>
> endif # ACPI
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 28f4f4d..ae93a91 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -67,6 +67,10 @@
>
> /* Host-dependent types and defines for in-kernel ACPICA */
>
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +#define ACPI_REDUCED_HARDWARE TRUE
> +#endif
> +
Maybe you put this here because of my previous wrong comment.
For ACPICA environments that work like Kconfigs for Linux, it is good to define them before including any ACPICA files.
While putting things here cannot cover <asm/acpi.h>.
Normally, I will do:
...
#ifdef __KERNEL__
/* some comment */
(one empty line as ACPICA enforces 1 empty line after 1 line comment and no empty lines after a block of comments)
#ifdef CONFIG_ACPI_REDUCED_HARDWARE
#define ACPI_REDUCED_HARDWARE(spaces not tabs here according to ACPICA's coding style)TRUE
#endif
#include <linux/string.h>
The coding style can help ACPICA release process to generate correct Linuxized patches.
It would be good to Linux developers to follow this currently for ACPICA internal code or we may see a small useless divergences commit generated from a back ported Linux commit :-( .
I'm sorry for the inconvenience.
Thanks
-Lv
> #define ACPI_MACHINE_WIDTH BITS_PER_LONG
> #define ACPI_EXPORT_SYMBOL(symbol) EXPORT_SYMBOL(symbol);
> #define strtoul simple_strtoul
> --
> 1.8.3.1
>
> --
> 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-22 6:14 ` Zheng, Lv
@ 2013-11-22 9:56 ` Hanjun Guo
2013-11-25 7:43 ` Zheng, Lv
0 siblings, 1 reply; 57+ messages in thread
From: Hanjun Guo @ 2013-11-22 9:56 UTC (permalink / raw)
To: Zheng, Lv, al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
On 2013-11-22 14:14, Zheng, Lv wrote:
[...]
>> endif # ACPI
>> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
>> index 28f4f4d..ae93a91 100644
>> --- a/include/acpi/platform/aclinux.h
>> +++ b/include/acpi/platform/aclinux.h
>> @@ -67,6 +67,10 @@
>>
>> /* Host-dependent types and defines for in-kernel ACPICA */
>>
>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
>> +#define ACPI_REDUCED_HARDWARE TRUE
>> +#endif
>> +
>
> Maybe you put this here because of my previous wrong comment.
>
> For ACPICA environments that work like Kconfigs for Linux, it is good to define them before including any ACPICA files.
> While putting things here cannot cover <asm/acpi.h>.
Good catch! thanks for the reminding.
>
> Normally, I will do:
>
> ...
>
> #ifdef __KERNEL__
>
> /* some comment */
> (one empty line as ACPICA enforces 1 empty line after 1 line comment and no empty lines after a block of comments)
> #ifdef CONFIG_ACPI_REDUCED_HARDWARE
> #define ACPI_REDUCED_HARDWARE(spaces not tabs here according to ACPICA's coding style)TRUE
> #endif
>
> #include <linux/string.h>
There is a problem when I try yours suggestion, it is a compiling warning:
warning: "TRUE" is not defined
And I find that "TRUE" is defined in include/acpi/actypes.
So, is this ok to you?
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -63,6 +63,13 @@
#ifdef EXPORT_ACPI_INTERFACES
#include <linux/export.h>
#endif
+
+#define TRUE (1 == 1)
+
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE
+#define ACPI_REDUCED_HARDWARE TRUE
+#endif
+
#include <asm/acpi.h>
>
> The coding style can help ACPICA release process to generate correct Linuxized patches.
> It would be good to Linux developers to follow this currently for ACPICA internal code or we may see a small useless divergences commit generated from a back ported Linux commit :-( .
> I'm sorry for the inconvenience.
ok, will update in next version.
>
> Thanks
> -Lv
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-22 9:56 ` Hanjun Guo
@ 2013-11-25 7:43 ` Zheng, Lv
2013-11-25 8:14 ` Zheng, Lv
0 siblings, 1 reply; 57+ messages in thread
From: Zheng, Lv @ 2013-11-25 7:43 UTC (permalink / raw)
To: Hanjun Guo, al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
HI,
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hanjun Guo
> Sent: Friday, November 22, 2013 5:57 PM
>
> On 2013-11-22 14:14, Zheng, Lv wrote:
> [...]
> >> endif # ACPI
> >> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> >> index 28f4f4d..ae93a91 100644
> >> --- a/include/acpi/platform/aclinux.h
> >> +++ b/include/acpi/platform/aclinux.h
> >> @@ -67,6 +67,10 @@
> >>
> >> /* Host-dependent types and defines for in-kernel ACPICA */
> >>
> >> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> >> +#define ACPI_REDUCED_HARDWARE TRUE
> >> +#endif
> >> +
> >
> > Maybe you put this here because of my previous wrong comment.
> >
> > For ACPICA environments that work like Kconfigs for Linux, it is good to define them before including any ACPICA files.
> > While putting things here cannot cover <asm/acpi.h>.
>
> Good catch! thanks for the reminding.
>
> >
> > Normally, I will do:
> >
> > ...
> >
> > #ifdef __KERNEL__
> >
> > /* some comment */
> > (one empty line as ACPICA enforces 1 empty line after 1 line comment and no empty lines after a block of comments)
> > #ifdef CONFIG_ACPI_REDUCED_HARDWARE
> > #define ACPI_REDUCED_HARDWARE(spaces not tabs here according to ACPICA's coding style)TRUE
> > #endif
> >
> > #include <linux/string.h>
>
> There is a problem when I try yours suggestion, it is a compiling warning:
>
> warning: "TRUE" is not defined
>
> And I find that "TRUE" is defined in include/acpi/actypes.
>
> So, is this ok to you?
... How ugly the TRUE is.
OK, you can keep your original code as this is really not a real issue, just a tricky point.
I'll try to offer a cleanup after another ACPICA cleanup that tries to modify all "#if" condition related TRUE into "1" in ACPICA.
Thanks and best regards
-Lv
>
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -63,6 +63,13 @@
> #ifdef EXPORT_ACPI_INTERFACES
> #include <linux/export.h>
> #endif
> +
> +#define TRUE (1 == 1)
> +
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +#define ACPI_REDUCED_HARDWARE TRUE
> +#endif
> +
> #include <asm/acpi.h>
>
> >
> > The coding style can help ACPICA release process to generate correct Linuxized patches.
> > It would be good to Linux developers to follow this currently for ACPICA internal code or we may see a small useless divergences
> commit generated from a back ported Linux commit :-( .
> > I'm sorry for the inconvenience.
>
> ok, will update in next version.
>
> >
> > Thanks
> > -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
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-25 7:43 ` Zheng, Lv
@ 2013-11-25 8:14 ` Zheng, Lv
2013-11-27 9:02 ` Hanjun Guo
0 siblings, 1 reply; 57+ messages in thread
From: Zheng, Lv @ 2013-11-25 8:14 UTC (permalink / raw)
To: Zheng, Lv, Hanjun Guo, al.stone@linaro.org,
linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Monday, November 25, 2013 3:44 PM
>
> HI,
>
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hanjun Guo
> > Sent: Friday, November 22, 2013 5:57 PM
> >
> > On 2013-11-22 14:14, Zheng, Lv wrote:
> > [...]
> > >> endif # ACPI
> > >> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> > >> index 28f4f4d..ae93a91 100644
> > >> --- a/include/acpi/platform/aclinux.h
> > >> +++ b/include/acpi/platform/aclinux.h
> > >> @@ -67,6 +67,10 @@
> > >>
> > >> /* Host-dependent types and defines for in-kernel ACPICA */
> > >>
> > >> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> > >> +#define ACPI_REDUCED_HARDWARE TRUE
> > >> +#endif
> > >> +
> > >
> > > Maybe you put this here because of my previous wrong comment.
> > >
> > > For ACPICA environments that work like Kconfigs for Linux, it is good to define them before including any ACPICA files.
> > > While putting things here cannot cover <asm/acpi.h>.
> >
> > Good catch! thanks for the reminding.
> >
> > >
> > > Normally, I will do:
> > >
> > > ...
> > >
> > > #ifdef __KERNEL__
> > >
> > > /* some comment */
> > > (one empty line as ACPICA enforces 1 empty line after 1 line comment and no empty lines after a block of comments)
> > > #ifdef CONFIG_ACPI_REDUCED_HARDWARE
> > > #define ACPI_REDUCED_HARDWARE(spaces not tabs here according to ACPICA's coding style)TRUE
> > > #endif
> > >
> > > #include <linux/string.h>
> >
> > There is a problem when I try yours suggestion, it is a compiling warning:
> >
> > warning: "TRUE" is not defined
> >
> > And I find that "TRUE" is defined in include/acpi/actypes.
> >
> > So, is this ok to you?
>
> ... How ugly the TRUE is.
> OK, you can keep your original code as this is really not a real issue, just a tricky point.
> I'll try to offer a cleanup after another ACPICA cleanup that tries to modify all "#if" condition related TRUE into "1" in ACPICA.
I checked ACPICA code base and found there is really nothing need to be cleaned up.
Why not:
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE
+#define ACPI_REDUCED_HARDWARE 1
+#endif
If you take a look at generated/autoconf.h, such environmental conditions should be 1 rather than TRUE as they are meant to be included prior than any source code.
Thanks and best regards
-Lv
>
> Thanks and best regards
> -Lv
>
> >
> > --- a/include/acpi/platform/aclinux.h
> > +++ b/include/acpi/platform/aclinux.h
> > @@ -63,6 +63,13 @@
> > #ifdef EXPORT_ACPI_INTERFACES
> > #include <linux/export.h>
> > #endif
> > +
> > +#define TRUE (1 == 1)
> > +
> > +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> > +#define ACPI_REDUCED_HARDWARE TRUE
> > +#endif
> > +
> > #include <asm/acpi.h>
> >
> > >
> > > The coding style can help ACPICA release process to generate correct Linuxized patches.
> > > It would be good to Linux developers to follow this currently for ACPICA internal code or we may see a small useless divergences
> > commit generated from a back ported Linux commit :-( .
> > > I'm sorry for the inconvenience.
> >
> > ok, will update in next version.
> >
> > >
> > > Thanks
> > > -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
> --
> 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
2013-11-25 8:14 ` Zheng, Lv
@ 2013-11-27 9:02 ` Hanjun Guo
0 siblings, 0 replies; 57+ messages in thread
From: Hanjun Guo @ 2013-11-27 9:02 UTC (permalink / raw)
To: Zheng, Lv, al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
Hi Lv,
Sorry for the late reply, I have some comments below.
On 2013-11-25 16:14, Zheng, Lv wrote:
[...]
>>>>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
>>>>> +#define ACPI_REDUCED_HARDWARE TRUE
>>>>> +#endif
>>>>> +
>>>>
>>>> Maybe you put this here because of my previous wrong comment.
>>>>
>>>> For ACPICA environments that work like Kconfigs for Linux, it is good to define them before including any ACPICA files.
>>>> While putting things here cannot cover <asm/acpi.h>.
>>>
>>> Good catch! thanks for the reminding.
>>>
>>>>
>>>> Normally, I will do:
>>>>
>>>> ...
>>>>
>>>> #ifdef __KERNEL__
>>>>
>>>> /* some comment */
>>>> (one empty line as ACPICA enforces 1 empty line after 1 line comment and no empty lines after a block of comments)
>>>> #ifdef CONFIG_ACPI_REDUCED_HARDWARE
>>>> #define ACPI_REDUCED_HARDWARE(spaces not tabs here according to ACPICA's coding style)TRUE
>>>> #endif
>>>>
>>>> #include <linux/string.h>
>>>
>>> There is a problem when I try yours suggestion, it is a compiling warning:
>>>
>>> warning: "TRUE" is not defined
>>>
>>> And I find that "TRUE" is defined in include/acpi/actypes.
>>>
>>> So, is this ok to you?
>>
>> ... How ugly the TRUE is.
>> OK, you can keep your original code as this is really not a real issue, just a tricky point.
>> I'll try to offer a cleanup after another ACPICA cleanup that tries to modify all "#if" condition related TRUE into "1" in ACPICA.
>
> I checked ACPICA code base and found there is really nothing need to be cleaned up.
>
> Why not:
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +#define ACPI_REDUCED_HARDWARE 1
> +#endif
In include/acpi/acconfig.h:
#ifndef ACPI_REDUCED_HARDWARE
#define ACPI_REDUCED_HARDWARE FALSE
#endif
In order to make the code consistent, I used "TRUE" here.
But it is ok to me to use "1" here, will update in next version.
Thanks
Hanjun
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
2013-11-10 1:36 ` [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 21:56 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field al.stone
` (10 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Remove the saving and restoring of bus master reload registers in
suspend/resume when in reduces HW mode; according to the spec, no
such registers should exist
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/processor_idle.c | 8 +++++++-
include/acpi/acpixf.h | 6 ++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 35c8f2b..28079a6 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -210,6 +210,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
#endif
#ifdef CONFIG_PM_SLEEP
+#if (!ACPI_REDUCED_HARDWARE)
static u32 saved_bm_rld;
static int acpi_processor_suspend(void)
@@ -228,6 +229,11 @@ static void acpi_processor_resume(void)
acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
}
+#else
+/* Bus master reload is not supported in reduced HW mode. */
+static int acpi_processor_suspend(void) { return 0; }
+static void acpi_processor_resume(void) { return; }
+#endif /* (!ACPI_REDUCED_HARDWARE) */
static struct syscore_ops acpi_processor_syscore_ops = {
.suspend = acpi_processor_suspend,
@@ -605,7 +611,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
case ACPI_STATE_C2:
if (!cx->address)
break;
- cx->valid = 1;
+ cx->valid = 1;
break;
case ACPI_STATE_C3:
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index d8f9457..27ef69b 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -99,6 +99,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
#define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
prototype;
+#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
+ prototype;
+
#else
#define ACPI_HW_DEPENDENT_RETURN_STATUS(prototype) \
static ACPI_INLINE prototype {return(AE_NOT_CONFIGURED);}
@@ -109,6 +112,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
#define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
static ACPI_INLINE prototype {return;}
+#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
+ static ACPI_INLINE prototype {return 0;}
+
#endif /* !ACPI_REDUCED_HARDWARE */
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode
2013-11-10 1:36 ` [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode al.stone
@ 2013-11-17 21:56 ` Rafael J. Wysocki
2013-11-20 21:11 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 21:56 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:12 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> Remove the saving and restoring of bus master reload registers in
> suspend/resume when in reduces HW mode; according to the spec, no
> such registers should exist
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/processor_idle.c | 8 +++++++-
> include/acpi/acpixf.h | 6 ++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 35c8f2b..28079a6 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -210,6 +210,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> #endif
>
> #ifdef CONFIG_PM_SLEEP
> +#if (!ACPI_REDUCED_HARDWARE)
Why don't you use #ifndef CONFIG_ACPI_REDUCED_HARDWARE here?
And I believe we don't need acpi_processor_syscore_ops in that case at all?
So why don't you put the whole section under
#if (IS_ENABLED(CONFIG_PM_SLEEP) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)) ?
> static u32 saved_bm_rld;
>
> static int acpi_processor_suspend(void)
> @@ -228,6 +229,11 @@ static void acpi_processor_resume(void)
>
> acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
> }
> +#else
> +/* Bus master reload is not supported in reduced HW mode. */
> +static int acpi_processor_suspend(void) { return 0; }
> +static void acpi_processor_resume(void) { return; }
> +#endif /* (!ACPI_REDUCED_HARDWARE) */
>
> static struct syscore_ops acpi_processor_syscore_ops = {
> .suspend = acpi_processor_suspend,
I'm not sure why you think that the changes below belong to this patch?
> @@ -605,7 +611,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
> case ACPI_STATE_C2:
> if (!cx->address)
> break;
> - cx->valid = 1;
> + cx->valid = 1;
> break;
>
> case ACPI_STATE_C3:
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index d8f9457..27ef69b 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -99,6 +99,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
> #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
> prototype;
>
> +#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
> + prototype;
> +
> #else
> #define ACPI_HW_DEPENDENT_RETURN_STATUS(prototype) \
> static ACPI_INLINE prototype {return(AE_NOT_CONFIGURED);}
> @@ -109,6 +112,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
> #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
> static ACPI_INLINE prototype {return;}
>
> +#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
> + static ACPI_INLINE prototype {return 0;}
> +
> #endif /* !ACPI_REDUCED_HARDWARE */
>
> /*
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode
2013-11-17 21:56 ` Rafael J. Wysocki
@ 2013-11-20 21:11 ` Al Stone
2013-11-21 0:31 ` Rafael J. Wysocki
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:11 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 02:56 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:12 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>>
>> Remove the saving and restoring of bus master reload registers in
>> suspend/resume when in reduces HW mode; according to the spec, no
>> such registers should exist
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/processor_idle.c | 8 +++++++-
>> include/acpi/acpixf.h | 6 ++++++
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 35c8f2b..28079a6 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -210,6 +210,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>> #endif
>>
>> #ifdef CONFIG_PM_SLEEP
>> +#if (!ACPI_REDUCED_HARDWARE)
>
> Why don't you use #ifndef CONFIG_ACPI_REDUCED_HARDWARE here?
>
> And I believe we don't need acpi_processor_syscore_ops in that case at all?
>
> So why don't you put the whole section under
>
> #if (IS_ENABLED(CONFIG_PM_SLEEP) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)) ?
My original thinking was that ACPI_REDUCED_HARDWARE is the flag
being used in the ACPICA code. On re-thinking it (and I'll use
your suggestion, if you don't mind), is it fair to say that there
is essentially a policy of keeping the Linux acpi driver and the
ACPICA code as separate as possible? If so -- and I would think
it makes sense -- then the ACPI_REDUCED_HARDWARE flag should not
be used here, as suggested.
Thanks.
>> static u32 saved_bm_rld;
>>
>> static int acpi_processor_suspend(void)
>> @@ -228,6 +229,11 @@ static void acpi_processor_resume(void)
>>
>> acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
>> }
>> +#else
>> +/* Bus master reload is not supported in reduced HW mode. */
>> +static int acpi_processor_suspend(void) { return 0; }
>> +static void acpi_processor_resume(void) { return; }
>> +#endif /* (!ACPI_REDUCED_HARDWARE) */
>>
>> static struct syscore_ops acpi_processor_syscore_ops = {
>> .suspend = acpi_processor_suspend,
>
> I'm not sure why you think that the changes below belong to this patch?
It doesn't. My bad. I'll remove this.
>> @@ -605,7 +611,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>> case ACPI_STATE_C2:
>> if (!cx->address)
>> break;
>> - cx->valid = 1;
>> + cx->valid = 1;
>> break;
>>
>> case ACPI_STATE_C3:
>> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
>> index d8f9457..27ef69b 100644
>> --- a/include/acpi/acpixf.h
>> +++ b/include/acpi/acpixf.h
>> @@ -99,6 +99,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
>> #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
>> prototype;
>>
>> +#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
>> + prototype;
>> +
>> #else
>> #define ACPI_HW_DEPENDENT_RETURN_STATUS(prototype) \
>> static ACPI_INLINE prototype {return(AE_NOT_CONFIGURED);}
>> @@ -109,6 +112,9 @@ extern u8 acpi_gbl_disable_ssdt_table_load;
>> #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
>> static ACPI_INLINE prototype {return;}
>>
>> +#define ACPI_HW_DEPENDENT_RETURN_INT(prototype) \
>> + static ACPI_INLINE prototype {return 0;}
>> +
>> #endif /* !ACPI_REDUCED_HARDWARE */
>>
>> /*
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode
2013-11-20 21:11 ` Al Stone
@ 2013-11-21 0:31 ` Rafael J. Wysocki
0 siblings, 0 replies; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 0:31 UTC (permalink / raw)
To: Al Stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Wednesday, November 20, 2013 02:11:34 PM Al Stone wrote:
> On 11/17/2013 02:56 PM, Rafael J. Wysocki wrote:
> > On Saturday, November 09, 2013 06:36:12 PM al.stone@linaro.org wrote:
> >> From: Al Stone <ahs3@redhat.com>
> >>
> >> Remove the saving and restoring of bus master reload registers in
> >> suspend/resume when in reduces HW mode; according to the spec, no
> >> such registers should exist
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >> drivers/acpi/processor_idle.c | 8 +++++++-
> >> include/acpi/acpixf.h | 6 ++++++
> >> 2 files changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index 35c8f2b..28079a6 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -210,6 +210,7 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
> >> #endif
> >>
> >> #ifdef CONFIG_PM_SLEEP
> >> +#if (!ACPI_REDUCED_HARDWARE)
> >
> > Why don't you use #ifndef CONFIG_ACPI_REDUCED_HARDWARE here?
> >
> > And I believe we don't need acpi_processor_syscore_ops in that case at all?
> >
> > So why don't you put the whole section under
> >
> > #if (IS_ENABLED(CONFIG_PM_SLEEP) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)) ?
>
> My original thinking was that ACPI_REDUCED_HARDWARE is the flag
> being used in the ACPICA code. On re-thinking it (and I'll use
> your suggestion, if you don't mind), is it fair to say that there
> is essentially a policy of keeping the Linux acpi driver and the
> ACPICA code as separate as possible?
We generally avoid mixing the code bases so that ACPICA upstream can make
changes without worrying (too much) about breaking Linux inadvertently.
In any case it is better to use Linux' symbols rather than ACPICA's symbols
wherever possible.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
2013-11-10 1:36 ` [PATCH 01/12] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode al.stone
2013-11-10 1:36 ` [PATCH 02/12] ACPI: bus master reload not supported in reduced HW mode al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 21:58 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field al.stone
` (9 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/sleep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 14df305..721e949 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -525,7 +525,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
* generate wakeup events.
*/
if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3)) {
- acpi_event_status pwr_btn_status;
+ acpi_event_status pwr_btn_status = ACPI_EVENT_FLAG_DISABLED;
acpi_get_event_status(ACPI_EVENT_POWER_BUTTON, &pwr_btn_status);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field
2013-11-10 1:36 ` [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field al.stone
@ 2013-11-17 21:58 ` Rafael J. Wysocki
2013-11-20 21:13 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 21:58 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:13 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
Any changelog? Some people use git logs to check what patches do and that
is not particularly clear from the subject.
Besides, this looks like a genuine fix not related directly to HW reduced,
in which case why don't you submit it separately?
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/sleep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 14df305..721e949 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -525,7 +525,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
> * generate wakeup events.
> */
> if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3)) {
> - acpi_event_status pwr_btn_status;
> + acpi_event_status pwr_btn_status = ACPI_EVENT_FLAG_DISABLED;
>
> acpi_get_event_status(ACPI_EVENT_POWER_BUTTON, &pwr_btn_status);
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field
2013-11-17 21:58 ` Rafael J. Wysocki
@ 2013-11-20 21:13 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:13 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 02:58 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:13 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>
> Any changelog? Some people use git logs to check what patches do and that
> is not particularly clear from the subject.
Argh. Yup. Stupid mistake on my part. I'll fix it.
> Besides, this looks like a genuine fix not related directly to HW reduced,
> in which case why don't you submit it separately?
Fair enough. I'll send this independently.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/sleep.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index 14df305..721e949 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -525,7 +525,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>> * generate wakeup events.
>> */
>> if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3)) {
>> - acpi_event_status pwr_btn_status;
>> + acpi_event_status pwr_btn_status = ACPI_EVENT_FLAG_DISABLED;
>>
>> acpi_get_event_status(ACPI_EVENT_POWER_BUTTON, &pwr_btn_status);
>>
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (2 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 03/12] ACPI: clean up compiler warning about uninitialized field al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:06 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86 al.stone
` (8 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/bus.c | 3 ++-
drivers/acpi/osl.c | 10 ++++++----
drivers/acpi/pci_link.c | 14 ++++++++------
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b587ec8..6a54dd5 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -540,7 +540,8 @@ void __init acpi_early_init(void)
goto error0;
}
-#ifdef CONFIG_X86
+#if (!CONFIG_ACPI_REDUCED_HARDWARE)
+ /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
if (!acpi_ioapic) {
/* compatible (0) means level (3) */
if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 54a20ff..017b85c 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
+static u32 acpi_irq_number;
static struct workqueue_struct *kacpid_wq;
static struct workqueue_struct *kacpi_notify_wq;
static struct workqueue_struct *kacpi_hotplug_wq;
@@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
/*
* ACPI interrupts different from the SCI in our copy of the FADT are
- * not supported.
+ * not supported, except in HW reduced mode.
*/
- if (gsi != acpi_gbl_FADT.sci_interrupt)
+ if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
return AE_BAD_PARAMETER;
if (acpi_irq_handler)
@@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
acpi_irq_handler = NULL;
return AE_NOT_ACQUIRED;
}
+ acpi_irq_number = irq;
return AE_OK;
}
acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
{
- if (irq != acpi_gbl_FADT.sci_interrupt)
+ if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
return AE_BAD_PARAMETER;
free_irq(irq, acpi_irq);
@@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void)
acpi_status acpi_os_terminate(void)
{
if (acpi_irq_handler) {
- acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
+ acpi_os_remove_interrupt_handler(acpi_irq_number,
acpi_irq_handler);
}
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 2652a61..c0ab28a 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -23,7 +23,7 @@
*
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
*
- * TBD:
+ * TBD:
* 1. Support more than one IRQ resource entry per link device (index).
* 2. Implement start/stop mechanism and use ACPI Bus Driver facilities
* for IRQ management (e.g. start()->_SRS).
@@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link)
}
}
- /*
- * Query and parse _CRS to get the current IRQ assignment.
+ /*
+ * Query and parse _CRS to get the current IRQ assignment.
*/
status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
@@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
/*
* "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt
* Link Devices to move the PIRQs around to minimize sharing.
- *
+ *
* "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
* that the BIOS has already set to active. This is necessary because
* ACPI has no automatic means of knowing what ISA IRQs are used. Note that
@@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
*
* Note that PCI IRQ routers have a list of possible IRQs,
* which may not include the IRQs this table says are available.
- *
+ *
* Since this heuristic can't tell the difference between a link
* that no device will attach to, vs. a link which may be shared
* by multiple active devices -- it is not optimal.
@@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void)
}
}
/* Add a penalty for the SCI */
- acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
+ if (!acpi_gbl_reduced_hardware)
+ acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
+ PIRQ_PENALTY_PCI_USING;
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-10 1:36 ` [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field al.stone
@ 2013-11-17 22:06 ` Rafael J. Wysocki
2013-11-20 21:24 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:06 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
-ENOCHANGELOG
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/bus.c | 3 ++-
> drivers/acpi/osl.c | 10 ++++++----
> drivers/acpi/pci_link.c | 14 ++++++++------
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index b587ec8..6a54dd5 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
> goto error0;
> }
>
> -#ifdef CONFIG_X86
> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
Why don't you use #ifndef here?
> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
used for elsewhere.
> if (!acpi_ioapic) {
> /* compatible (0) means level (3) */
> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 54a20ff..017b85c 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>
> static acpi_osd_handler acpi_irq_handler;
> static void *acpi_irq_context;
> +static u32 acpi_irq_number;
> static struct workqueue_struct *kacpid_wq;
> static struct workqueue_struct *kacpi_notify_wq;
> static struct workqueue_struct *kacpi_hotplug_wq;
> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>
> /*
> * ACPI interrupts different from the SCI in our copy of the FADT are
> - * not supported.
> + * not supported, except in HW reduced mode.
> */
> - if (gsi != acpi_gbl_FADT.sci_interrupt)
> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
The inner parens are not necessary.
Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
generically, because there may be GPE device objects with interrupts different
from the SCI.
> return AE_BAD_PARAMETER;
>
> if (acpi_irq_handler)
> @@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> acpi_irq_handler = NULL;
> return AE_NOT_ACQUIRED;
> }
> + acpi_irq_number = irq;
>
> return AE_OK;
> }
>
> acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> {
> - if (irq != acpi_gbl_FADT.sci_interrupt)
> + if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
The inner parens are not necessary.
> return AE_BAD_PARAMETER;
>
> free_irq(irq, acpi_irq);
> @@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void)
> acpi_status acpi_os_terminate(void)
> {
> if (acpi_irq_handler) {
> - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> + acpi_os_remove_interrupt_handler(acpi_irq_number,
> acpi_irq_handler);
It looks like this could be one line now?
> }
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 2652a61..c0ab28a 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
I'm not sure how the comment changes below belong to this patch.
> @@ -23,7 +23,7 @@
> *
> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> *
> - * TBD:
> + * TBD:
> * 1. Support more than one IRQ resource entry per link device (index).
> * 2. Implement start/stop mechanism and use ACPI Bus Driver facilities
> * for IRQ management (e.g. start()->_SRS).
> @@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link)
> }
> }
>
> - /*
> - * Query and parse _CRS to get the current IRQ assignment.
> + /*
> + * Query and parse _CRS to get the current IRQ assignment.
> */
>
> status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
> @@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> /*
> * "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt
> * Link Devices to move the PIRQs around to minimize sharing.
> - *
> + *
> * "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
> * that the BIOS has already set to active. This is necessary because
> * ACPI has no automatic means of knowing what ISA IRQs are used. Note that
> @@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> *
> * Note that PCI IRQ routers have a list of possible IRQs,
> * which may not include the IRQs this table says are available.
> - *
> + *
> * Since this heuristic can't tell the difference between a link
> * that no device will attach to, vs. a link which may be shared
> * by multiple active devices -- it is not optimal.
> @@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void)
> }
> }
Why don't you simply put
if (acpi_gbl_reduced_hardware)
return 0;
here?
> /* Add a penalty for the SCI */
> - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> + if (!acpi_gbl_reduced_hardware)
> + acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
> + PIRQ_PENALTY_PCI_USING;
> return 0;
> }
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-17 22:06 ` Rafael J. Wysocki
@ 2013-11-20 21:24 ` Al Stone
2013-11-21 0:27 ` Rafael J. Wysocki
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:24 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>
> -ENOCHANGELOG
Yup. Will be added.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/bus.c | 3 ++-
>> drivers/acpi/osl.c | 10 ++++++----
>> drivers/acpi/pci_link.c | 14 ++++++++------
>> 3 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index b587ec8..6a54dd5 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
>> goto error0;
>> }
>>
>> -#ifdef CONFIG_X86
>> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
>
> Why don't you use #ifndef here?
No particular reason; I'll change it.
>> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
>
> I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
> used for elsewhere.
Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a
personal habit when writing. Yes, it should be "NOTE:".
>> if (!acpi_ioapic) {
>> /* compatible (0) means level (3) */
>> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 54a20ff..017b85c 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>>
>> static acpi_osd_handler acpi_irq_handler;
>> static void *acpi_irq_context;
>> +static u32 acpi_irq_number;
>> static struct workqueue_struct *kacpid_wq;
>> static struct workqueue_struct *kacpi_notify_wq;
>> static struct workqueue_struct *kacpi_hotplug_wq;
>> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>
>> /*
>> * ACPI interrupts different from the SCI in our copy of the FADT are
>> - * not supported.
>> + * not supported, except in HW reduced mode.
>> */
>> - if (gsi != acpi_gbl_FADT.sci_interrupt)
>> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
>
> The inner parens are not necessary.
Ack.
> Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
> generically, because there may be GPE device objects with interrupts different
> from the SCI.
In reduced HW mode, there are no GPE blocks defined; all
interrupts of that nature are required to use GPIO interrupts
instead, afaict. The spec unfortunately has this info scattered
through out -- the earlier parts of the spec discussing the
reduced HW mode and the discussion around the FADT go into
some of the details.
>> return AE_BAD_PARAMETER;
>>
>> if (acpi_irq_handler)
>> @@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>> acpi_irq_handler = NULL;
>> return AE_NOT_ACQUIRED;
>> }
>> + acpi_irq_number = irq;
>>
>> return AE_OK;
>> }
>>
>> acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>> {
>> - if (irq != acpi_gbl_FADT.sci_interrupt)
>> + if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
>
> The inner parens are not necessary.
Ack.
>> return AE_BAD_PARAMETER;
>>
>> free_irq(irq, acpi_irq);
>> @@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void)
>> acpi_status acpi_os_terminate(void)
>> {
>> if (acpi_irq_handler) {
>> - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
>> + acpi_os_remove_interrupt_handler(acpi_irq_number,
>> acpi_irq_handler);
>
> It looks like this could be one line now?
Yup. Will fix.
>> }
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 2652a61..c0ab28a 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>
> I'm not sure how the comment changes below belong to this patch.
Sigh. They don't. Will omit.
>> @@ -23,7 +23,7 @@
>> *
>> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> *
>> - * TBD:
>> + * TBD:
>> * 1. Support more than one IRQ resource entry per link device (index).
>> * 2. Implement start/stop mechanism and use ACPI Bus Driver facilities
>> * for IRQ management (e.g. start()->_SRS).
>> @@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link)
>> }
>> }
>>
>> - /*
>> - * Query and parse _CRS to get the current IRQ assignment.
>> + /*
>> + * Query and parse _CRS to get the current IRQ assignment.
>> */
>>
>> status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
>> @@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>> /*
>> * "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt
>> * Link Devices to move the PIRQs around to minimize sharing.
>> - *
>> + *
>> * "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
>> * that the BIOS has already set to active. This is necessary because
>> * ACPI has no automatic means of knowing what ISA IRQs are used. Note that
>> @@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>> *
>> * Note that PCI IRQ routers have a list of possible IRQs,
>> * which may not include the IRQs this table says are available.
>> - *
>> + *
>> * Since this heuristic can't tell the difference between a link
>> * that no device will attach to, vs. a link which may be shared
>> * by multiple active devices -- it is not optimal.
>> @@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void)
>> }
>> }
>
> Why don't you simply put
>
> if (acpi_gbl_reduced_hardware)
> return 0;
>
> here?
Aha. Much more obvious. Thanks. Will fix.
>> /* Add a penalty for the SCI */
>> - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>> + if (!acpi_gbl_reduced_hardware)
>> + acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
>> + PIRQ_PENALTY_PCI_USING;
>> return 0;
>> }
>>
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-20 21:24 ` Al Stone
@ 2013-11-21 0:27 ` Rafael J. Wysocki
2013-11-21 19:36 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 0:27 UTC (permalink / raw)
To: Al Stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Wednesday, November 20, 2013 02:24:29 PM Al Stone wrote:
> On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
> > On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
> >> From: Al Stone <ahs3@redhat.com>
> >
> > -ENOCHANGELOG
>
> Yup. Will be added.
>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >> drivers/acpi/bus.c | 3 ++-
> >> drivers/acpi/osl.c | 10 ++++++----
> >> drivers/acpi/pci_link.c | 14 ++++++++------
> >> 3 files changed, 16 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index b587ec8..6a54dd5 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
> >> goto error0;
> >> }
> >>
> >> -#ifdef CONFIG_X86
> >> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
> >
> > Why don't you use #ifndef here?
>
> No particular reason; I'll change it.
>
> >> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
> >
> > I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
> > used for elsewhere.
>
> Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a
> personal habit when writing. Yes, it should be "NOTE:".
>
> >> if (!acpi_ioapic) {
> >> /* compatible (0) means level (3) */
> >> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >> index 54a20ff..017b85c 100644
> >> --- a/drivers/acpi/osl.c
> >> +++ b/drivers/acpi/osl.c
> >> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
> >>
> >> static acpi_osd_handler acpi_irq_handler;
> >> static void *acpi_irq_context;
> >> +static u32 acpi_irq_number;
> >> static struct workqueue_struct *kacpid_wq;
> >> static struct workqueue_struct *kacpi_notify_wq;
> >> static struct workqueue_struct *kacpi_hotplug_wq;
> >> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>
> >> /*
> >> * ACPI interrupts different from the SCI in our copy of the FADT are
> >> - * not supported.
> >> + * not supported, except in HW reduced mode.
> >> */
> >> - if (gsi != acpi_gbl_FADT.sci_interrupt)
> >> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
> >
> > The inner parens are not necessary.
>
> Ack.
>
> > Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
> > generically, because there may be GPE device objects with interrupts different
> > from the SCI.
>
> In reduced HW mode, there are no GPE blocks defined; all
> interrupts of that nature are required to use GPIO interrupts
> instead, afaict.
Well, I'm not sure about that. The GPE0/1 blocks in FADT are not supposed to
be present, but does that apply to GPE block devices (Section 9.10 of ACPI 5.0)?
> The spec unfortunately has this info scattered
> through out -- the earlier parts of the spec discussing the
> reduced HW mode and the discussion around the FADT go into
> some of the details.
Any more precise pointers?
Anyway, the point was that we may need to support interrupts different from
acpi_gbl_FADT.sci_interrupt even if the HW reduced mode is *not* used, so
making that depend on acpi_gbl_reduced_hardware doesn't seem quite right.
> >> return AE_BAD_PARAMETER;
> >>
> >> if (acpi_irq_handler)
> >> @@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >> acpi_irq_handler = NULL;
> >> return AE_NOT_ACQUIRED;
> >> }
> >> + acpi_irq_number = irq;
> >>
> >> return AE_OK;
> >> }
> >>
> >> acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> >> {
> >> - if (irq != acpi_gbl_FADT.sci_interrupt)
> >> + if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
> >
> > The inner parens are not necessary.
>
> Ack.
>
> >> return AE_BAD_PARAMETER;
> >>
> >> free_irq(irq, acpi_irq);
> >> @@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void)
> >> acpi_status acpi_os_terminate(void)
> >> {
> >> if (acpi_irq_handler) {
> >> - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> >> + acpi_os_remove_interrupt_handler(acpi_irq_number,
> >> acpi_irq_handler);
> >
> > It looks like this could be one line now?
>
> Yup. Will fix.
>
> >> }
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index 2652a61..c0ab28a 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c
> >
> > I'm not sure how the comment changes below belong to this patch.
>
> Sigh. They don't. Will omit.
>
> >> @@ -23,7 +23,7 @@
> >> *
> >> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> *
> >> - * TBD:
> >> + * TBD:
> >> * 1. Support more than one IRQ resource entry per link device (index).
> >> * 2. Implement start/stop mechanism and use ACPI Bus Driver facilities
> >> * for IRQ management (e.g. start()->_SRS).
> >> @@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link)
> >> }
> >> }
> >>
> >> - /*
> >> - * Query and parse _CRS to get the current IRQ assignment.
> >> + /*
> >> + * Query and parse _CRS to get the current IRQ assignment.
> >> */
> >>
> >> status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
> >> @@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >> /*
> >> * "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt
> >> * Link Devices to move the PIRQs around to minimize sharing.
> >> - *
> >> + *
> >> * "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
> >> * that the BIOS has already set to active. This is necessary because
> >> * ACPI has no automatic means of knowing what ISA IRQs are used. Note that
> >> @@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >> *
> >> * Note that PCI IRQ routers have a list of possible IRQs,
> >> * which may not include the IRQs this table says are available.
> >> - *
> >> + *
> >> * Since this heuristic can't tell the difference between a link
> >> * that no device will attach to, vs. a link which may be shared
> >> * by multiple active devices -- it is not optimal.
> >> @@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void)
> >> }
> >> }
> >
> > Why don't you simply put
> >
> > if (acpi_gbl_reduced_hardware)
> > return 0;
> >
> > here?
>
> Aha. Much more obvious. Thanks. Will fix.
>
> >> /* Add a penalty for the SCI */
> >> - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> >> + if (!acpi_gbl_reduced_hardware)
> >> + acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
> >> + PIRQ_PENALTY_PCI_USING;
> >> return 0;
> >> }
> >>
> >>
>
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-21 0:27 ` Rafael J. Wysocki
@ 2013-11-21 19:36 ` Al Stone
2013-11-21 21:36 ` Rafael J. Wysocki
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-21 19:36 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/20/2013 05:27 PM, Rafael J. Wysocki wrote:
> On Wednesday, November 20, 2013 02:24:29 PM Al Stone wrote:
>> On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
>>> On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
>>>> From: Al Stone <ahs3@redhat.com>
>>>
>>> -ENOCHANGELOG
>>
>> Yup. Will be added.
>>
>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>>> ---
>>>> drivers/acpi/bus.c | 3 ++-
>>>> drivers/acpi/osl.c | 10 ++++++----
>>>> drivers/acpi/pci_link.c | 14 ++++++++------
>>>> 3 files changed, 16 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>>> index b587ec8..6a54dd5 100644
>>>> --- a/drivers/acpi/bus.c
>>>> +++ b/drivers/acpi/bus.c
>>>> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
>>>> goto error0;
>>>> }
>>>>
>>>> -#ifdef CONFIG_X86
>>>> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
>>>
>>> Why don't you use #ifndef here?
>>
>> No particular reason; I'll change it.
>>
>>>> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
>>>
>>> I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
>>> used for elsewhere.
>>
>> Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a
>> personal habit when writing. Yes, it should be "NOTE:".
>>
>>>> if (!acpi_ioapic) {
>>>> /* compatible (0) means level (3) */
>>>> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>>>> index 54a20ff..017b85c 100644
>>>> --- a/drivers/acpi/osl.c
>>>> +++ b/drivers/acpi/osl.c
>>>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>>>>
>>>> static acpi_osd_handler acpi_irq_handler;
>>>> static void *acpi_irq_context;
>>>> +static u32 acpi_irq_number;
>>>> static struct workqueue_struct *kacpid_wq;
>>>> static struct workqueue_struct *kacpi_notify_wq;
>>>> static struct workqueue_struct *kacpi_hotplug_wq;
>>>> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>>>
>>>> /*
>>>> * ACPI interrupts different from the SCI in our copy of the FADT are
>>>> - * not supported.
>>>> + * not supported, except in HW reduced mode.
>>>> */
>>>> - if (gsi != acpi_gbl_FADT.sci_interrupt)
>>>> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
>>>
>>> The inner parens are not necessary.
>>
>> Ack.
>>
>>> Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
>>> generically, because there may be GPE device objects with interrupts different
>>> from the SCI.
>>
>> In reduced HW mode, there are no GPE blocks defined; all
>> interrupts of that nature are required to use GPIO interrupts
>> instead, afaict.
>
> Well, I'm not sure about that. The GPE0/1 blocks in FADT are not supposed to
> be present, but does that apply to GPE block devices (Section 9.10 of ACPI 5.0)?
>
>> The spec unfortunately has this info scattered
>> through out -- the earlier parts of the spec discussing the
>> reduced HW mode and the discussion around the FADT go into
>> some of the details.
>
> Any more precise pointers?
>
> Anyway, the point was that we may need to support interrupts different from
> acpi_gbl_FADT.sci_interrupt even if the HW reduced mode is *not* used, so
> making that depend on acpi_gbl_reduced_hardware doesn't seem quite right.
Yeah, sorry, I should have included the pointers earlier. I'm basing
my thinking on my understanding of these sections:
-- Section 4.1 which defines HW reduced ACPI, and specifically
on 4.1.1, Hardware-Reduced Events.
-- Section 5.2.9 defining the FADT and the HW reduced restrictions
-- Section 5.6.5, GPIO-signaled ACPI events
-- Section 9.10, GPE block device
The way I read 9.10 in particular is why I'm thinking that any use of
acpi_gbl_FADT.sci_interrupt needs to go away in HW reduced mode. The
first sentence says:
The GPE Block device is an optional device that allows a system
designer to describe GPE blocks beyond the two that are described
in the FADT.
The way I am interpreting that is that a GPE block device only makes
sense as an extension of the GPE0/1 blocks, not as an independent
device. Since using the GPE0/1 blocks is not allowed in reduced HW
mode (see 5.2.9), we cannot extend them with a GPE block device.
That being said, I agree we should be able to install interrupt
handlers other than acpi_gbl_FADT.sci_interrupt regardless of whether
we are in legacy or reduced HW mode. So, I'm thinking that this
block:
if (gsi != acpi_gbl_FADT.sci_interrupt)
return AE_BAD_PARAMETER;
should just be removed from acpi_os_install_interrupt_handler().
Does that make sense?
>>>> return AE_BAD_PARAMETER;
>>>>
>>>> if (acpi_irq_handler)
>>>> @@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>>> acpi_irq_handler = NULL;
>>>> return AE_NOT_ACQUIRED;
>>>> }
>>>> + acpi_irq_number = irq;
>>>>
>>>> return AE_OK;
>>>> }
>>>>
>>>> acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>>>> {
>>>> - if (irq != acpi_gbl_FADT.sci_interrupt)
>>>> + if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
>>>
>>> The inner parens are not necessary.
>>
>> Ack.
>>
>>>> return AE_BAD_PARAMETER;
>>>>
>>>> free_irq(irq, acpi_irq);
>>>> @@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void)
>>>> acpi_status acpi_os_terminate(void)
>>>> {
>>>> if (acpi_irq_handler) {
>>>> - acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
>>>> + acpi_os_remove_interrupt_handler(acpi_irq_number,
>>>> acpi_irq_handler);
>>>
>>> It looks like this could be one line now?
>>
>> Yup. Will fix.
>>
>>>> }
>>>>
>>>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>>>> index 2652a61..c0ab28a 100644
>>>> --- a/drivers/acpi/pci_link.c
>>>> +++ b/drivers/acpi/pci_link.c
>>>
>>> I'm not sure how the comment changes below belong to this patch.
>>
>> Sigh. They don't. Will omit.
>>
>>>> @@ -23,7 +23,7 @@
>>>> *
>>>> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> *
>>>> - * TBD:
>>>> + * TBD:
>>>> * 1. Support more than one IRQ resource entry per link device (index).
>>>> * 2. Implement start/stop mechanism and use ACPI Bus Driver facilities
>>>> * for IRQ management (e.g. start()->_SRS).
>>>> @@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link)
>>>> }
>>>> }
>>>>
>>>> - /*
>>>> - * Query and parse _CRS to get the current IRQ assignment.
>>>> + /*
>>>> + * Query and parse _CRS to get the current IRQ assignment.
>>>> */
>>>>
>>>> status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
>>>> @@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>>>> /*
>>>> * "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt
>>>> * Link Devices to move the PIRQs around to minimize sharing.
>>>> - *
>>>> + *
>>>> * "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
>>>> * that the BIOS has already set to active. This is necessary because
>>>> * ACPI has no automatic means of knowing what ISA IRQs are used. Note that
>>>> @@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>>>> *
>>>> * Note that PCI IRQ routers have a list of possible IRQs,
>>>> * which may not include the IRQs this table says are available.
>>>> - *
>>>> + *
>>>> * Since this heuristic can't tell the difference between a link
>>>> * that no device will attach to, vs. a link which may be shared
>>>> * by multiple active devices -- it is not optimal.
>>>> @@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void)
>>>> }
>>>> }
>>>
>>> Why don't you simply put
>>>
>>> if (acpi_gbl_reduced_hardware)
>>> return 0;
>>>
>>> here?
>>
>> Aha. Much more obvious. Thanks. Will fix.
>>
>>>> /* Add a penalty for the SCI */
>>>> - acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>>>> + if (!acpi_gbl_reduced_hardware)
>>>> + acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=
>>>> + PIRQ_PENALTY_PCI_USING;
>>>> return 0;
>>>> }
>>>>
>>>>
>>
>>
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-21 19:36 ` Al Stone
@ 2013-11-21 21:36 ` Rafael J. Wysocki
2013-11-21 22:19 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 21:36 UTC (permalink / raw)
To: Al Stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Thursday, November 21, 2013 12:36:35 PM Al Stone wrote:
> On 11/20/2013 05:27 PM, Rafael J. Wysocki wrote:
> > On Wednesday, November 20, 2013 02:24:29 PM Al Stone wrote:
> >> On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
> >>> On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
> >>>> From: Al Stone <ahs3@redhat.com>
> >>>
> >>> -ENOCHANGELOG
> >>
> >> Yup. Will be added.
> >>
> >>>> Signed-off-by: Al Stone <al.stone@linaro.org>
> >>>> ---
> >>>> drivers/acpi/bus.c | 3 ++-
> >>>> drivers/acpi/osl.c | 10 ++++++----
> >>>> drivers/acpi/pci_link.c | 14 ++++++++------
> >>>> 3 files changed, 16 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >>>> index b587ec8..6a54dd5 100644
> >>>> --- a/drivers/acpi/bus.c
> >>>> +++ b/drivers/acpi/bus.c
> >>>> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
> >>>> goto error0;
> >>>> }
> >>>>
> >>>> -#ifdef CONFIG_X86
> >>>> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
> >>>
> >>> Why don't you use #ifndef here?
> >>
> >> No particular reason; I'll change it.
> >>
> >>>> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
> >>>
> >>> I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
> >>> used for elsewhere.
> >>
> >> Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a
> >> personal habit when writing. Yes, it should be "NOTE:".
> >>
> >>>> if (!acpi_ioapic) {
> >>>> /* compatible (0) means level (3) */
> >>>> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >>>> index 54a20ff..017b85c 100644
> >>>> --- a/drivers/acpi/osl.c
> >>>> +++ b/drivers/acpi/osl.c
> >>>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
> >>>>
> >>>> static acpi_osd_handler acpi_irq_handler;
> >>>> static void *acpi_irq_context;
> >>>> +static u32 acpi_irq_number;
> >>>> static struct workqueue_struct *kacpid_wq;
> >>>> static struct workqueue_struct *kacpi_notify_wq;
> >>>> static struct workqueue_struct *kacpi_hotplug_wq;
> >>>> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>>>
> >>>> /*
> >>>> * ACPI interrupts different from the SCI in our copy of the FADT are
> >>>> - * not supported.
> >>>> + * not supported, except in HW reduced mode.
> >>>> */
> >>>> - if (gsi != acpi_gbl_FADT.sci_interrupt)
> >>>> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
> >>>
> >>> The inner parens are not necessary.
> >>
> >> Ack.
> >>
> >>> Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
> >>> generically, because there may be GPE device objects with interrupts different
> >>> from the SCI.
> >>
> >> In reduced HW mode, there are no GPE blocks defined; all
> >> interrupts of that nature are required to use GPIO interrupts
> >> instead, afaict.
> >
> > Well, I'm not sure about that. The GPE0/1 blocks in FADT are not supposed to
> > be present, but does that apply to GPE block devices (Section 9.10 of ACPI 5.0)?
> >
> >> The spec unfortunately has this info scattered
> >> through out -- the earlier parts of the spec discussing the
> >> reduced HW mode and the discussion around the FADT go into
> >> some of the details.
> >
> > Any more precise pointers?
> >
> > Anyway, the point was that we may need to support interrupts different from
> > acpi_gbl_FADT.sci_interrupt even if the HW reduced mode is *not* used, so
> > making that depend on acpi_gbl_reduced_hardware doesn't seem quite right.
>
> Yeah, sorry, I should have included the pointers earlier. I'm basing
> my thinking on my understanding of these sections:
>
> -- Section 4.1 which defines HW reduced ACPI, and specifically
> on 4.1.1, Hardware-Reduced Events.
>
> -- Section 5.2.9 defining the FADT and the HW reduced restrictions
>
> -- Section 5.6.5, GPIO-signaled ACPI events
>
> -- Section 9.10, GPE block device
>
> The way I read 9.10 in particular is why I'm thinking that any use of
> acpi_gbl_FADT.sci_interrupt needs to go away in HW reduced mode. The
> first sentence says:
>
> The GPE Block device is an optional device that allows a system
> designer to describe GPE blocks beyond the two that are described
> in the FADT.
Well, precisely. It doesn't say anything like "GPE block devices may
only be used if the GPE blocks described in the FADT are present."
> The way I am interpreting that is that a GPE block device only makes
> sense as an extension of the GPE0/1 blocks, not as an independent
> device.
It is an independent device and it may use a *different* interrupt (see
the example in Section 9.10). [The paragraph right before that example
even says: "To represent the GPE block associated with the FADT ..."
and describes that in detail.]
So to my eyes the spec doesn't actually explicitly say anywhere that
using GPE block devices in the HW reduced mode is invalid.
> Since using the GPE0/1 blocks is not allowed in reduced HW
> mode (see 5.2.9), we cannot extend them with a GPE block device.
>
> That being said, I agree we should be able to install interrupt
> handlers other than acpi_gbl_FADT.sci_interrupt regardless of whether
> we are in legacy or reduced HW mode. So, I'm thinking that this
> block:
>
> if (gsi != acpi_gbl_FADT.sci_interrupt)
> return AE_BAD_PARAMETER;
>
> should just be removed from acpi_os_install_interrupt_handler().
>
> Does that make sense?
I think so. At least I don't recall any specific situation in which it will
lead to problems.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field
2013-11-21 21:36 ` Rafael J. Wysocki
@ 2013-11-21 22:19 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-21 22:19 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/21/2013 02:36 PM, Rafael J. Wysocki wrote:
> On Thursday, November 21, 2013 12:36:35 PM Al Stone wrote:
>> On 11/20/2013 05:27 PM, Rafael J. Wysocki wrote:
>>> On Wednesday, November 20, 2013 02:24:29 PM Al Stone wrote:
>>>> On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
>>>>> On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
>>>>>> From: Al Stone <ahs3@redhat.com>
>>>>>
>>>>> -ENOCHANGELOG
>>>>
>>>> Yup. Will be added.
>>>>
>>>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>>>>> ---
>>>>>> drivers/acpi/bus.c | 3 ++-
>>>>>> drivers/acpi/osl.c | 10 ++++++----
>>>>>> drivers/acpi/pci_link.c | 14 ++++++++------
>>>>>> 3 files changed, 16 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>>>>> index b587ec8..6a54dd5 100644
>>>>>> --- a/drivers/acpi/bus.c
>>>>>> +++ b/drivers/acpi/bus.c
>>>>>> @@ -540,7 +540,8 @@ void __init acpi_early_init(void)
>>>>>> goto error0;
>>>>>> }
>>>>>>
>>>>>> -#ifdef CONFIG_X86
>>>>>> +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
>>>>>
>>>>> Why don't you use #ifndef here?
>>>>
>>>> No particular reason; I'll change it.
>>>>
>>>>>> + /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
>>>>>
>>>>> I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is
>>>>> used for elsewhere.
>>>>
>>>> Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a
>>>> personal habit when writing. Yes, it should be "NOTE:".
>>>>
>>>>>> if (!acpi_ioapic) {
>>>>>> /* compatible (0) means level (3) */
>>>>>> if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>>>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>>>>>> index 54a20ff..017b85c 100644
>>>>>> --- a/drivers/acpi/osl.c
>>>>>> +++ b/drivers/acpi/osl.c
>>>>>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>>>>>>
>>>>>> static acpi_osd_handler acpi_irq_handler;
>>>>>> static void *acpi_irq_context;
>>>>>> +static u32 acpi_irq_number;
>>>>>> static struct workqueue_struct *kacpid_wq;
>>>>>> static struct workqueue_struct *kacpi_notify_wq;
>>>>>> static struct workqueue_struct *kacpi_hotplug_wq;
>>>>>> @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>>>>>
>>>>>> /*
>>>>>> * ACPI interrupts different from the SCI in our copy of the FADT are
>>>>>> - * not supported.
>>>>>> + * not supported, except in HW reduced mode.
>>>>>> */
>>>>>> - if (gsi != acpi_gbl_FADT.sci_interrupt)
>>>>>> + if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
>>>>>
>>>>> The inner parens are not necessary.
>>>>
>>>> Ack.
>>>>
>>>>> Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt
>>>>> generically, because there may be GPE device objects with interrupts different
>>>>> from the SCI.
>>>>
>>>> In reduced HW mode, there are no GPE blocks defined; all
>>>> interrupts of that nature are required to use GPIO interrupts
>>>> instead, afaict.
>>>
>>> Well, I'm not sure about that. The GPE0/1 blocks in FADT are not supposed to
>>> be present, but does that apply to GPE block devices (Section 9.10 of ACPI 5.0)?
>>>
>>>> The spec unfortunately has this info scattered
>>>> through out -- the earlier parts of the spec discussing the
>>>> reduced HW mode and the discussion around the FADT go into
>>>> some of the details.
>>>
>>> Any more precise pointers?
>>>
>>> Anyway, the point was that we may need to support interrupts different from
>>> acpi_gbl_FADT.sci_interrupt even if the HW reduced mode is *not* used, so
>>> making that depend on acpi_gbl_reduced_hardware doesn't seem quite right.
>>
>> Yeah, sorry, I should have included the pointers earlier. I'm basing
>> my thinking on my understanding of these sections:
>>
>> -- Section 4.1 which defines HW reduced ACPI, and specifically
>> on 4.1.1, Hardware-Reduced Events.
>>
>> -- Section 5.2.9 defining the FADT and the HW reduced restrictions
>>
>> -- Section 5.6.5, GPIO-signaled ACPI events
>>
>> -- Section 9.10, GPE block device
>>
>> The way I read 9.10 in particular is why I'm thinking that any use of
>> acpi_gbl_FADT.sci_interrupt needs to go away in HW reduced mode. The
>> first sentence says:
>>
>> The GPE Block device is an optional device that allows a system
>> designer to describe GPE blocks beyond the two that are described
>> in the FADT.
>
> Well, precisely. It doesn't say anything like "GPE block devices may
> only be used if the GPE blocks described in the FADT are present."
>
>> The way I am interpreting that is that a GPE block device only makes
>> sense as an extension of the GPE0/1 blocks, not as an independent
>> device.
>
> It is an independent device and it may use a *different* interrupt (see
> the example in Section 9.10). [The paragraph right before that example
> even says: "To represent the GPE block associated with the FADT ..."
> and describes that in detail.]
>
> So to my eyes the spec doesn't actually explicitly say anywhere that
> using GPE block devices in the HW reduced mode is invalid.
Valid point. I am making the interpretation based on implied
statements, versus an explicit statement. Now that the ACPI
spec is part of the UEFI forum, perhaps I can get some clarifying
language inserted one way or the other. I'll start poking at
that as a side project.
>> Since using the GPE0/1 blocks is not allowed in reduced HW
>> mode (see 5.2.9), we cannot extend them with a GPE block device.
>>
>> That being said, I agree we should be able to install interrupt
>> handlers other than acpi_gbl_FADT.sci_interrupt regardless of whether
>> we are in legacy or reduced HW mode. So, I'm thinking that this
>> block:
>>
>> if (gsi != acpi_gbl_FADT.sci_interrupt)
>> return AE_BAD_PARAMETER;
>>
>> should just be removed from acpi_os_install_interrupt_handler().
>>
>> Does that make sense?
>
> I think so. At least I don't recall any specific situation in which it will
> lead to problems.
I couldn't recall any either. I'll change the patch to remove
this if test, then.
Thanks for the feedback.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (3 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:08 ` Rafael J. Wysocki
2013-11-22 6:19 ` Zheng, Lv
2013-11-10 1:36 ` [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode al.stone
` (7 subsequent siblings)
12 siblings, 2 replies; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Corrected #ifdef so that DMI is not used on ARM platforms which are
currently implementing reduced HW mode.
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/bus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 6a54dd5..f41949a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -513,11 +513,15 @@ void __init acpi_early_init(void)
acpi_gbl_permanent_mmap = 1;
+#if !(CONFIG_ARM || CONFIG_ARM64)
/*
+ * NB: ARM does not use DMI at present.
+ *
* If the machine falls into the DMI check table,
* DSDT will be copied to memory
*/
dmi_check_system(dsdt_dmi_table);
+#endif
status = acpi_reallocate_root_table();
if (ACPI_FAILURE(status)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86
2013-11-10 1:36 ` [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86 al.stone
@ 2013-11-17 22:08 ` Rafael J. Wysocki
2013-11-20 21:25 ` Al Stone
2013-11-22 6:19 ` Zheng, Lv
1 sibling, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:08 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:15 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> Corrected #ifdef so that DMI is not used on ARM platforms which are
> currently implementing reduced HW mode.
Please define an empty dmi_check_system() stub for ARM/ARM64 then.
Generally, please avoid adding compiler directives into function bodies if
possible.
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/bus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 6a54dd5..f41949a 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -513,11 +513,15 @@ void __init acpi_early_init(void)
>
> acpi_gbl_permanent_mmap = 1;
>
> +#if !(CONFIG_ARM || CONFIG_ARM64)
> /*
> + * NB: ARM does not use DMI at present.
> + *
> * If the machine falls into the DMI check table,
> * DSDT will be copied to memory
> */
> dmi_check_system(dsdt_dmi_table);
> +#endif
>
> status = acpi_reallocate_root_table();
> if (ACPI_FAILURE(status)) {
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86
2013-11-17 22:08 ` Rafael J. Wysocki
@ 2013-11-20 21:25 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:25 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:08 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:15 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>>
>> Corrected #ifdef so that DMI is not used on ARM platforms which are
>> currently implementing reduced HW mode.
>
> Please define an empty dmi_check_system() stub for ARM/ARM64 then.
>
> Generally, please avoid adding compiler directives into function bodies if
> possible.
Will do.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/bus.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 6a54dd5..f41949a 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -513,11 +513,15 @@ void __init acpi_early_init(void)
>>
>> acpi_gbl_permanent_mmap = 1;
>>
>> +#if !(CONFIG_ARM || CONFIG_ARM64)
>> /*
>> + * NB: ARM does not use DMI at present.
>> + *
>> * If the machine falls into the DMI check table,
>> * DSDT will be copied to memory
>> */
>> dmi_check_system(dsdt_dmi_table);
>> +#endif
>>
>> status = acpi_reallocate_root_table();
>> if (ACPI_FAILURE(status)) {
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* RE: [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86
2013-11-10 1:36 ` [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86 al.stone
2013-11-17 22:08 ` Rafael J. Wysocki
@ 2013-11-22 6:19 ` Zheng, Lv
1 sibling, 0 replies; 57+ messages in thread
From: Zheng, Lv @ 2013-11-22 6:19 UTC (permalink / raw)
To: al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of al.stone@linaro.org
> Sent: Sunday, November 10, 2013 9:36 AM
>
> From: Al Stone <ahs3@redhat.com>
>
> Corrected #ifdef so that DMI is not used on ARM platforms which are
> currently implementing reduced HW mode.
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/bus.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 6a54dd5..f41949a 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -513,11 +513,15 @@ void __init acpi_early_init(void)
>
> acpi_gbl_permanent_mmap = 1;
>
> +#if !(CONFIG_ARM || CONFIG_ARM64)
> /*
> + * NB: ARM does not use DMI at present.
> + *
> * If the machine falls into the DMI check table,
> * DSDT will be copied to memory
> */
> dmi_check_system(dsdt_dmi_table);
> +#endif
I can see dsdt_dmi_table is well protected in this file. So is that possible to introduce dmi_check_system() stub for non DMI systems?
Thanks
-Lv
>
> status = acpi_reallocate_root_table();
> if (ACPI_FAILURE(status)) {
> --
> 1.8.3.1
>
> --
> 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (4 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 05/12] ACPI: ARM: exclude calls on ARM platforms, not include them on x86 al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-22 6:05 ` Zheng, Lv
2013-11-10 1:36 ` [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries " al.stone
` (6 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
In acpi_os_terminate(), it was assumed that several FADT entries were
mapped into memory. For HW reduced mode, that will not be the case.
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/osl.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 017b85c..075545e 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1812,10 +1812,12 @@ acpi_status acpi_os_terminate(void)
acpi_irq_handler);
}
- acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
- acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
- acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
- acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+ if (!acpi_gbl_reduced_hardware) {
+ acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
+ acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
+ acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+ acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+ }
destroy_workqueue(kacpid_wq);
destroy_workqueue(kacpi_notify_wq);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* RE: [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode
2013-11-10 1:36 ` [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode al.stone
@ 2013-11-22 6:05 ` Zheng, Lv
2013-11-22 6:26 ` Zheng, Lv
0 siblings, 1 reply; 57+ messages in thread
From: Zheng, Lv @ 2013-11-22 6:05 UTC (permalink / raw)
To: al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of al.stone@linaro.org
> Sent: Sunday, November 10, 2013 9:36 AM
>
> From: Al Stone <ahs3@redhat.com>
>
> In acpi_os_terminate(), it was assumed that several FADT entries were
> mapped into memory. For HW reduced mode, that will not be the case.
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/osl.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 017b85c..075545e 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1812,10 +1812,12 @@ acpi_status acpi_os_terminate(void)
> acpi_irq_handler);
> }
>
> - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> + if (!acpi_gbl_reduced_hardware) {
> + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> + }
>
> destroy_workqueue(kacpid_wq);
> destroy_workqueue(kacpi_notify_wq);
Is that possible to make stubs for acpi_os_map_generic_address and acpi_os_unmap_generic_address when CONFIG_ACPI_REDUCED_HARDWARE is enabled?
So that this patch and some logics in the PATCH 07 can be removed.
Thanks
-Lv
> --
> 1.8.3.1
>
> --
> 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
^ permalink raw reply [flat|nested] 57+ messages in thread* RE: [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode
2013-11-22 6:05 ` Zheng, Lv
@ 2013-11-22 6:26 ` Zheng, Lv
0 siblings, 0 replies; 57+ messages in thread
From: Zheng, Lv @ 2013-11-22 6:26 UTC (permalink / raw)
To: Zheng, Lv, al.stone@linaro.org, linux-acpi@vger.kernel.org
Cc: linaro-acpi@lists.linaro.org, Al Stone
Sorry, this is wrong.
There are still generic addresses in HW_REDUCED mode, please ignore this.
Thanks
-Lv
> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Friday, November 22, 2013 2:06 PM
> To: al.stone@linaro.org; linux-acpi@vger.kernel.org
> Cc: linaro-acpi@lists.linaro.org; Al Stone
> Subject: RE: [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode
>
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of al.stone@linaro.org
> > Sent: Sunday, November 10, 2013 9:36 AM
> >
> > From: Al Stone <ahs3@redhat.com>
> >
> > In acpi_os_terminate(), it was assumed that several FADT entries were
> > mapped into memory. For HW reduced mode, that will not be the case.
> >
> > Signed-off-by: Al Stone <al.stone@linaro.org>
> > ---
> > drivers/acpi/osl.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 017b85c..075545e 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -1812,10 +1812,12 @@ acpi_status acpi_os_terminate(void)
> > acpi_irq_handler);
> > }
> >
> > - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> > - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> > - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> > - acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> > + if (!acpi_gbl_reduced_hardware) {
> > + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> > + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> > + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> > + acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> > + }
> >
> > destroy_workqueue(kacpid_wq);
> > destroy_workqueue(kacpi_notify_wq);
>
> Is that possible to make stubs for acpi_os_map_generic_address and acpi_os_unmap_generic_address when
> CONFIG_ACPI_REDUCED_HARDWARE is enabled?
> So that this patch and some logics in the PATCH 07 can be removed.
>
> Thanks
> -Lv
>
> > --
> > 1.8.3.1
> >
> > --
> > 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
> --
> 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries in HW reduced mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (5 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 06/12] ACPI: ensure several FADT fields are only used in HW reduced mode al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:15 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed al.stone
` (5 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Since some of the FADT fields reserved are not to be used by the OSPM,
do not map in the memory areas that the FADT fields reference.
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/osl.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 075545e..2750cb5 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -160,6 +160,9 @@ static u32 acpi_osi_handler(acpi_string interface, u32 supported)
return supported;
}
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE
+static int __init acpi_reserve_resources(void) { return 0; }
+#else
static void __init acpi_request_region (struct acpi_generic_address *gas,
unsigned int length, char *desc)
{
@@ -209,6 +212,7 @@ static int __init acpi_reserve_resources(void)
return 0;
}
+#endif
device_initcall(acpi_reserve_resources);
void acpi_os_printf(const char *fmt, ...)
@@ -1782,6 +1786,9 @@ static int __init acpi_no_auto_ssdt_setup(char *s)
__setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE
+acpi_status __init acpi_os_initialize(void) { return AE_OK; }
+#else
acpi_status __init acpi_os_initialize(void)
{
acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
@@ -1791,6 +1798,7 @@ acpi_status __init acpi_os_initialize(void)
return AE_OK;
}
+#endif
acpi_status __init acpi_os_initialize1(void)
{
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries in HW reduced mode
2013-11-10 1:36 ` [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries " al.stone
@ 2013-11-17 22:15 ` Rafael J. Wysocki
2013-11-20 21:27 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:15 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:17 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> Since some of the FADT fields reserved are not to be used by the OSPM,
"are not to be used by the OSPM in the HW reduced mode" would be clearer.
> do not map in the memory areas that the FADT fields reference.
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/osl.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 075545e..2750cb5 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -160,6 +160,9 @@ static u32 acpi_osi_handler(acpi_string interface, u32 supported)
> return supported;
> }
>
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +static int __init acpi_reserve_resources(void) { return 0; }
> +#else
> static void __init acpi_request_region (struct acpi_generic_address *gas,
> unsigned int length, char *desc)
> {
> @@ -209,6 +212,7 @@ static int __init acpi_reserve_resources(void)
>
> return 0;
> }
> +#endif
> device_initcall(acpi_reserve_resources);
Do we need the device_initcall() above at all if CONFIG_ACPI_REDUCED_HARDWARE
is unset?
> void acpi_os_printf(const char *fmt, ...)
> @@ -1782,6 +1786,9 @@ static int __init acpi_no_auto_ssdt_setup(char *s)
>
> __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
> +acpi_status __init acpi_os_initialize(void) { return AE_OK; }
> +#else
> acpi_status __init acpi_os_initialize(void)
> {
> acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> @@ -1791,6 +1798,7 @@ acpi_status __init acpi_os_initialize(void)
>
> return AE_OK;
> }
> +#endif
>
> acpi_status __init acpi_os_initialize1(void)
> {
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries in HW reduced mode
2013-11-17 22:15 ` Rafael J. Wysocki
@ 2013-11-20 21:27 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:27 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:15 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:17 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>>
>> Since some of the FADT fields reserved are not to be used by the OSPM,
>
> "are not to be used by the OSPM in the HW reduced mode" would be clearer.
Good suggestion. I'll add it in.
>> do not map in the memory areas that the FADT fields reference.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/osl.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 075545e..2750cb5 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -160,6 +160,9 @@ static u32 acpi_osi_handler(acpi_string interface, u32 supported)
>> return supported;
>> }
>>
>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
>> +static int __init acpi_reserve_resources(void) { return 0; }
>> +#else
>> static void __init acpi_request_region (struct acpi_generic_address *gas,
>> unsigned int length, char *desc)
>> {
>> @@ -209,6 +212,7 @@ static int __init acpi_reserve_resources(void)
>>
>> return 0;
>> }
>> +#endif
>> device_initcall(acpi_reserve_resources);
>
> Do we need the device_initcall() above at all if CONFIG_ACPI_REDUCED_HARDWARE
> is unset?
Sigh -- off-by-one error on the #endif. No, it's not really
needed; I'll fix that.
>> void acpi_os_printf(const char *fmt, ...)
>> @@ -1782,6 +1786,9 @@ static int __init acpi_no_auto_ssdt_setup(char *s)
>>
>> __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>>
>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE
>> +acpi_status __init acpi_os_initialize(void) { return AE_OK; }
>> +#else
>> acpi_status __init acpi_os_initialize(void)
>> {
>> acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> @@ -1791,6 +1798,7 @@ acpi_status __init acpi_os_initialize(void)
>>
>> return AE_OK;
>> }
>> +#endif
>>
>> acpi_status __init acpi_os_initialize1(void)
>> {
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (6 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 07/12] ACPI: do not reserve memory regions for some FADT entries " al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:17 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode al.stone
` (4 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Make sure we are not in HW reduced mode when we rely on the the
P_LVL2_LAT or P_LVL3_LAT (c2_latency, c3_latency) values from the
FADT.
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/processor_idle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 28079a6..e2bd0bf 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -644,7 +644,7 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
memset(pr->power.states, 0, sizeof(pr->power.states));
result = acpi_processor_get_power_info_cst(pr);
- if (result == -ENODEV)
+ if (!acpi_gbl_reduced_hardware && (result == -ENODEV))
result = acpi_processor_get_power_info_fadt(pr);
if (result)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed
2013-11-10 1:36 ` [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed al.stone
@ 2013-11-17 22:17 ` Rafael J. Wysocki
2013-11-20 21:48 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:17 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:18 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> Make sure we are not in HW reduced mode when we rely on the the
> P_LVL2_LAT or P_LVL3_LAT (c2_latency, c3_latency) values from the
> FADT.
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/processor_idle.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 28079a6..e2bd0bf 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -644,7 +644,7 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
> memset(pr->power.states, 0, sizeof(pr->power.states));
>
> result = acpi_processor_get_power_info_cst(pr);
> - if (result == -ENODEV)
> + if (!acpi_gbl_reduced_hardware && (result == -ENODEV))
> result = acpi_processor_get_power_info_fadt(pr);
Wouldn't it be better to make acpi_processor_get_power_info_fadt() check
acpi_gbl_reduced_hardware ?
>
> if (result)
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed
2013-11-17 22:17 ` Rafael J. Wysocki
@ 2013-11-20 21:48 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:48 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:17 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:18 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>>
>> Make sure we are not in HW reduced mode when we rely on the the
>> P_LVL2_LAT or P_LVL3_LAT (c2_latency, c3_latency) values from the
>> FADT.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/processor_idle.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 28079a6..e2bd0bf 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -644,7 +644,7 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>> memset(pr->power.states, 0, sizeof(pr->power.states));
>>
>> result = acpi_processor_get_power_info_cst(pr);
>> - if (result == -ENODEV)
>> + if (!acpi_gbl_reduced_hardware && (result == -ENODEV))
>> result = acpi_processor_get_power_info_fadt(pr);
>
> Wouldn't it be better to make acpi_processor_get_power_info_fadt() check
> acpi_gbl_reduced_hardware ?
Hrm. Yes, it would. I'll change that.
>>
>> if (result)
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (7 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 08/12] ACPI: in HW reduced mode, getting power latencies from FADT is not allowed al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:20 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored " al.stone
` (3 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/processor_throttling.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
index e7dd2c1..200738e 100644
--- a/drivers/acpi/processor_throttling.c
+++ b/drivers/acpi/processor_throttling.c
@@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
return -EINVAL;
}
+ /*
+ * NB: in HW reduced mode, duty_width is always zero
+ * so this count may not be what is wanted.
+ */
pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
/*
@@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
/* Used to clear all duty_value bits */
duty_mask = pr->throttling.state_count - 1;
+ /*
+ * NB: in HW reduced mode, duty_offset is always zero
+ * so this mask may not be what is wanted.
+ */
duty_mask <<= acpi_gbl_FADT.duty_offset;
duty_mask = ~duty_mask;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode
2013-11-10 1:36 ` [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode al.stone
@ 2013-11-17 22:20 ` Rafael J. Wysocki
2013-11-20 21:54 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:20 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:19 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/processor_throttling.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
> index e7dd2c1..200738e 100644
> --- a/drivers/acpi/processor_throttling.c
> +++ b/drivers/acpi/processor_throttling.c
> @@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
> return -EINVAL;
> }
>
> + /*
> + * NB: in HW reduced mode, duty_width is always zero
> + * so this count may not be what is wanted.
> + */
> pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
>
> /*
> @@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
> /* Used to clear all duty_value bits */
> duty_mask = pr->throttling.state_count - 1;
>
> + /*
> + * NB: in HW reduced mode, duty_offset is always zero
> + * so this mask may not be what is wanted.
> + */
> duty_mask <<= acpi_gbl_FADT.duty_offset;
> duty_mask = ~duty_mask;
> }
I'm not sure how these comments help to be honest. It looks like
pr->throttling.state_count should be 0 in HW reduced mode, shouldn't it?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode
2013-11-17 22:20 ` Rafael J. Wysocki
@ 2013-11-20 21:54 ` Al Stone
2013-11-21 0:14 ` Rafael J. Wysocki
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:54 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:20 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:19 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/processor_throttling.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
>> index e7dd2c1..200738e 100644
>> --- a/drivers/acpi/processor_throttling.c
>> +++ b/drivers/acpi/processor_throttling.c
>> @@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
>> return -EINVAL;
>> }
>>
>> + /*
>> + * NB: in HW reduced mode, duty_width is always zero
>> + * so this count may not be what is wanted.
>> + */
>> pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
>>
>> /*
>> @@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
>> /* Used to clear all duty_value bits */
>> duty_mask = pr->throttling.state_count - 1;
>>
>> + /*
>> + * NB: in HW reduced mode, duty_offset is always zero
>> + * so this mask may not be what is wanted.
>> + */
>> duty_mask <<= acpi_gbl_FADT.duty_offset;
>> duty_mask = ~duty_mask;
>> }
>
> I'm not sure how these comments help to be honest. It looks like
> pr->throttling.state_count should be 0 in HW reduced mode, shouldn't it?
>
It should. The comments clarified things for me but perhaps
they should just note that these values are always zero in
reduced HW mode. The other option would be to not add any
comments, of course. Hopefully someone working with reduced
HW mode would be aware of these changes to the FADT values.
I can go either way; what's the preference?
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode
2013-11-20 21:54 ` Al Stone
@ 2013-11-21 0:14 ` Rafael J. Wysocki
2013-11-21 23:11 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-21 0:14 UTC (permalink / raw)
To: Al Stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Wednesday, November 20, 2013 02:54:32 PM Al Stone wrote:
> On 11/17/2013 03:20 PM, Rafael J. Wysocki wrote:
> > On Saturday, November 09, 2013 06:36:19 PM al.stone@linaro.org wrote:
> >> From: Al Stone <ahs3@redhat.com>
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >> drivers/acpi/processor_throttling.c | 8 ++++++++
> >> 1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
> >> index e7dd2c1..200738e 100644
> >> --- a/drivers/acpi/processor_throttling.c
> >> +++ b/drivers/acpi/processor_throttling.c
> >> @@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
> >> return -EINVAL;
> >> }
> >>
> >> + /*
> >> + * NB: in HW reduced mode, duty_width is always zero
> >> + * so this count may not be what is wanted.
> >> + */
> >> pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
> >>
> >> /*
> >> @@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
> >> /* Used to clear all duty_value bits */
> >> duty_mask = pr->throttling.state_count - 1;
> >>
> >> + /*
> >> + * NB: in HW reduced mode, duty_offset is always zero
> >> + * so this mask may not be what is wanted.
> >> + */
> >> duty_mask <<= acpi_gbl_FADT.duty_offset;
> >> duty_mask = ~duty_mask;
> >> }
> >
> > I'm not sure how these comments help to be honest. It looks like
> > pr->throttling.state_count should be 0 in HW reduced mode, shouldn't it?
> >
>
> It should. The comments clarified things for me but perhaps
> they should just note that these values are always zero in
> reduced HW mode. The other option would be to not add any
> comments, of course. Hopefully someone working with reduced
> HW mode would be aware of these changes to the FADT values.
>
> I can go either way; what's the preference?
I would just avoid making changes until we figure out what to do ultimately
here. That depends on what pr->throttling.state_count is used for and that
part should be hardened against pr->throttling.state_count == 0. Having
done that, we can simply set pr->throttling.state_count = 0 in the HW reduced
mode without adding any comments at all.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode
2013-11-21 0:14 ` Rafael J. Wysocki
@ 2013-11-21 23:11 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-21 23:11 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/20/2013 05:14 PM, Rafael J. Wysocki wrote:
> On Wednesday, November 20, 2013 02:54:32 PM Al Stone wrote:
>> On 11/17/2013 03:20 PM, Rafael J. Wysocki wrote:
>>> On Saturday, November 09, 2013 06:36:19 PM al.stone@linaro.org wrote:
>>>> From: Al Stone <ahs3@redhat.com>
>>>>
>>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>>> ---
>>>> drivers/acpi/processor_throttling.c | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
>>>> index e7dd2c1..200738e 100644
>>>> --- a/drivers/acpi/processor_throttling.c
>>>> +++ b/drivers/acpi/processor_throttling.c
>>>> @@ -942,6 +942,10 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
>>>> return -EINVAL;
>>>> }
>>>>
>>>> + /*
>>>> + * NB: in HW reduced mode, duty_width is always zero
>>>> + * so this count may not be what is wanted.
>>>> + */
>>>> pr->throttling.state_count = 1 << acpi_gbl_FADT.duty_width;
>>>>
>>>> /*
>>>> @@ -991,6 +995,10 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,
>>>> /* Used to clear all duty_value bits */
>>>> duty_mask = pr->throttling.state_count - 1;
>>>>
>>>> + /*
>>>> + * NB: in HW reduced mode, duty_offset is always zero
>>>> + * so this mask may not be what is wanted.
>>>> + */
>>>> duty_mask <<= acpi_gbl_FADT.duty_offset;
>>>> duty_mask = ~duty_mask;
>>>> }
>>>
>>> I'm not sure how these comments help to be honest. It looks like
>>> pr->throttling.state_count should be 0 in HW reduced mode, shouldn't it?
>>>
>>
>> It should. The comments clarified things for me but perhaps
>> they should just note that these values are always zero in
>> reduced HW mode. The other option would be to not add any
>> comments, of course. Hopefully someone working with reduced
>> HW mode would be aware of these changes to the FADT values.
>>
>> I can go either way; what's the preference?
>
> I would just avoid making changes until we figure out what to do ultimately
> here. That depends on what pr->throttling.state_count is used for and that
> part should be hardened against pr->throttling.state_count == 0. Having
> done that, we can simply set pr->throttling.state_count = 0 in the HW reduced
> mode without adding any comments at all.
>
> Thanks!
>
I'll remove this patch for now. I think what makes sense is a
separate patch to harden the use of pr->throttling.state_count
in processor_throttling.c and processor_thermal.c. I see a
couple of places where the values may not make sense when it
is zero (regardless of reduced HW mode) but I think I'd like
to treat that as a separate issue and think it through some
more.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored in HW reduced mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (8 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 09/12] ACPI: add clarifying comment about processor throttling in HW reduced mode al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:24 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed " al.stone
` (2 subsequent siblings)
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/processor_idle.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e2bd0bf..262b84b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -290,7 +290,8 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
* Check for P_LVL2_UP flag before entering C2 and above on
* an SMP system.
*/
- if ((num_online_cpus() > 1) &&
+ if (!acpi_gbl_reduced_hardware &&
+ (num_online_cpus() > 1) &&
!(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
return -ENODEV;
#endif
@@ -965,7 +966,8 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
continue;
#ifdef CONFIG_HOTPLUG_CPU
- if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
+ if (!acpi_gbl_reduced_hardware &&
+ (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
!pr->flags.has_cst &&
!(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
continue;
@@ -1020,7 +1022,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
continue;
#ifdef CONFIG_HOTPLUG_CPU
- if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
+ if (!acpi_gbl_reduced_hardware &&
+ (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
!pr->flags.has_cst &&
!(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
continue;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored in HW reduced mode
2013-11-10 1:36 ` [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored " al.stone
@ 2013-11-17 22:24 ` Rafael J. Wysocki
2013-11-20 21:55 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:24 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:20 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
-ENOCHANGELOG
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/processor_idle.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e2bd0bf..262b84b 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -290,7 +290,8 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
> * Check for P_LVL2_UP flag before entering C2 and above on
> * an SMP system.
> */
> - if ((num_online_cpus() > 1) &&
> + if (!acpi_gbl_reduced_hardware &&
> + (num_online_cpus() > 1) &&
> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
> return -ENODEV;
> #endif
Patch [8/12] added code to avoid calling this function for acpi_gbl_reduced_hardware
set at all, so isn't the check added here actually useless?
> @@ -965,7 +966,8 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> continue;
>
> #ifdef CONFIG_HOTPLUG_CPU
> - if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
> + if (!acpi_gbl_reduced_hardware &&
> + (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
> !pr->flags.has_cst &&
> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
> continue;
> @@ -1020,7 +1022,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> continue;
>
> #ifdef CONFIG_HOTPLUG_CPU
> - if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
> + if (!acpi_gbl_reduced_hardware &&
> + (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
> !pr->flags.has_cst &&
> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
> continue;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored in HW reduced mode
2013-11-17 22:24 ` Rafael J. Wysocki
@ 2013-11-20 21:55 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 21:55 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:24 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:20 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>
> -ENOCHANGELOG
Ack.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/processor_idle.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index e2bd0bf..262b84b 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -290,7 +290,8 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>> * Check for P_LVL2_UP flag before entering C2 and above on
>> * an SMP system.
>> */
>> - if ((num_online_cpus() > 1) &&
>> + if (!acpi_gbl_reduced_hardware &&
>> + (num_online_cpus() > 1) &&
>> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
>> return -ENODEV;
>> #endif
>
> Patch [8/12] added code to avoid calling this function for acpi_gbl_reduced_hardware
> set at all, so isn't the check added here actually useless?
Yup, it's redundant. I'll sort out 8/12 and this part and simplify it.
>> @@ -965,7 +966,8 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> continue;
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>> - if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
>> + if (!acpi_gbl_reduced_hardware &&
>> + (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
>> !pr->flags.has_cst &&
>> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
>> continue;
>> @@ -1020,7 +1022,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>> continue;
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>> - if ((cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
>> + if (!acpi_gbl_reduced_hardware &&
>> + (cx->type != ACPI_STATE_C1) && (num_online_cpus() > 1) &&
>> !pr->flags.has_cst &&
>> !(acpi_gbl_FADT.flags & ACPI_FADT_C2_MP_SUPPORTED))
>> continue;
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (9 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 10/12] ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored " al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:26 ` Rafael J. Wysocki
2013-11-10 1:36 ` [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works al.stone
2013-11-17 21:47 ` [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI Rafael J. Wysocki
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/acpica/utxface.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index be322c8..fe94b3e 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -187,7 +187,8 @@ acpi_status acpi_get_system_info(struct acpi_buffer * out_buffer)
/* Timer resolution - 24 or 32 bits */
- if (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER) {
+ if (!acpi_gbl_reduced_hardware &&
+ (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER)) {
info_ptr->timer_resolution = 24;
} else {
info_ptr->timer_resolution = 32;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
2013-11-10 1:36 ` [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed " al.stone
@ 2013-11-17 22:26 ` Rafael J. Wysocki
2013-11-20 22:15 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:26 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:21 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
I'm reading the patch as "the timer resolution in HW reduced mode is always
32-bit". Is my reading correct?
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/acpica/utxface.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
> index be322c8..fe94b3e 100644
> --- a/drivers/acpi/acpica/utxface.c
> +++ b/drivers/acpi/acpica/utxface.c
> @@ -187,7 +187,8 @@ acpi_status acpi_get_system_info(struct acpi_buffer * out_buffer)
>
> /* Timer resolution - 24 or 32 bits */
>
> - if (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER) {
> + if (!acpi_gbl_reduced_hardware &&
> + (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER)) {
> info_ptr->timer_resolution = 24;
> } else {
> info_ptr->timer_resolution = 32;
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
2013-11-17 22:26 ` Rafael J. Wysocki
@ 2013-11-20 22:15 ` Al Stone
2013-11-21 23:43 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-20 22:15 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:26 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:21 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>
> I'm reading the patch as "the timer resolution in HW reduced mode is always
> 32-bit". Is my reading correct?
Whups, I see what I did. You are reading the patch correctly.
However, what the code should ultimately do is ignore the
ACPI_FADT_32BIT_TIMER flag completely. Let me look at where the
timer_resolution field is being used also since it is not immediately
clear what value it should have, or if it should even be used, when
in reduced HW mode.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/acpica/utxface.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
>> index be322c8..fe94b3e 100644
>> --- a/drivers/acpi/acpica/utxface.c
>> +++ b/drivers/acpi/acpica/utxface.c
>> @@ -187,7 +187,8 @@ acpi_status acpi_get_system_info(struct acpi_buffer * out_buffer)
>>
>> /* Timer resolution - 24 or 32 bits */
>>
>> - if (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER) {
>> + if (!acpi_gbl_reduced_hardware &&
>> + (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER)) {
>> info_ptr->timer_resolution = 24;
>> } else {
>> info_ptr->timer_resolution = 32;
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
2013-11-20 22:15 ` Al Stone
@ 2013-11-21 23:43 ` Al Stone
2013-11-22 12:08 ` Rafael J. Wysocki
0 siblings, 1 reply; 57+ messages in thread
From: Al Stone @ 2013-11-21 23:43 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/20/2013 03:15 PM, Al Stone wrote:
> On 11/17/2013 03:26 PM, Rafael J. Wysocki wrote:
>> On Saturday, November 09, 2013 06:36:21 PM al.stone@linaro.org wrote:
>>> From: Al Stone <ahs3@redhat.com>
>>
>> I'm reading the patch as "the timer resolution in HW reduced mode is
>> always
>> 32-bit". Is my reading correct?
>
> Whups, I see what I did. You are reading the patch correctly.
>
> However, what the code should ultimately do is ignore the
> ACPI_FADT_32BIT_TIMER flag completely. Let me look at where the
> timer_resolution field is being used also since it is not immediately
> clear what value it should have, or if it should even be used, when
> in reduced HW mode.
On further investigation, I'll take this patch out of this set.
I think this needs better handling in the ACPICA code so I'll work
with them on that.
In the meantime, the spec seems to be completely mum on what the
timer resolution should be in reduced HW mode. What's more, it
looks like acpi_get_system_info() never gets called in the first
place, so this change would be irrelevant. Further, it also looks
like acpi_get_timer_resolution() (which returns the timer_resolution
value) never gets called either, nor is the timer_resolution used
outside of the ACPICA code in any way.
So, whenever the right answer gets figured out in ACPICA, it will
eventually get pulled into Linux ACPI.
>>> Signed-off-by: Al Stone <al.stone@linaro.org>
>>> ---
>>> drivers/acpi/acpica/utxface.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/acpica/utxface.c
>>> b/drivers/acpi/acpica/utxface.c
>>> index be322c8..fe94b3e 100644
>>> --- a/drivers/acpi/acpica/utxface.c
>>> +++ b/drivers/acpi/acpica/utxface.c
>>> @@ -187,7 +187,8 @@ acpi_status acpi_get_system_info(struct
>>> acpi_buffer * out_buffer)
>>>
>>> /* Timer resolution - 24 or 32 bits */
>>>
>>> - if (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER) {
>>> + if (!acpi_gbl_reduced_hardware &&
>>> + (acpi_gbl_FADT.flags & ACPI_FADT_32BIT_TIMER)) {
>>> info_ptr->timer_resolution = 24;
>>> } else {
>>> info_ptr->timer_resolution = 32;
>>>
>
>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
2013-11-21 23:43 ` Al Stone
@ 2013-11-22 12:08 ` Rafael J. Wysocki
0 siblings, 0 replies; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-22 12:08 UTC (permalink / raw)
To: Al Stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Thursday, November 21, 2013 04:43:59 PM Al Stone wrote:
> On 11/20/2013 03:15 PM, Al Stone wrote:
> > On 11/17/2013 03:26 PM, Rafael J. Wysocki wrote:
> >> On Saturday, November 09, 2013 06:36:21 PM al.stone@linaro.org wrote:
> >>> From: Al Stone <ahs3@redhat.com>
> >>
> >> I'm reading the patch as "the timer resolution in HW reduced mode is
> >> always
> >> 32-bit". Is my reading correct?
> >
> > Whups, I see what I did. You are reading the patch correctly.
> >
> > However, what the code should ultimately do is ignore the
> > ACPI_FADT_32BIT_TIMER flag completely. Let me look at where the
> > timer_resolution field is being used also since it is not immediately
> > clear what value it should have, or if it should even be used, when
> > in reduced HW mode.
>
> On further investigation, I'll take this patch out of this set.
> I think this needs better handling in the ACPICA code so I'll work
> with them on that.
>
> In the meantime, the spec seems to be completely mum on what the
> timer resolution should be in reduced HW mode.
If that's about the ACPI PM timer, that's because it's not present
at all in HW reduced mode.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (10 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 11/12] ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed " al.stone
@ 2013-11-10 1:36 ` al.stone
2013-11-17 22:28 ` Rafael J. Wysocki
2013-11-17 21:47 ` [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI Rafael J. Wysocki
12 siblings, 1 reply; 57+ messages in thread
From: al.stone @ 2013-11-10 1:36 UTC (permalink / raw)
To: linux-acpi; +Cc: linaro-acpi, Al Stone, Al Stone
From: Al Stone <ahs3@redhat.com>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
drivers/acpi/bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index f41949a..e868de3 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -513,7 +513,7 @@ void __init acpi_early_init(void)
acpi_gbl_permanent_mmap = 1;
-#if !(CONFIG_ARM || CONFIG_ARM64)
+#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64))
/*
* NB: ARM does not use DMI at present.
*
@@ -544,7 +544,7 @@ void __init acpi_early_init(void)
goto error0;
}
-#if (!CONFIG_ACPI_REDUCED_HARDWARE)
+#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64)) && (!ACPI_REDUCED_HARDWARE)
/* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
if (!acpi_ioapic) {
/* compatible (0) means level (3) */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works
2013-11-10 1:36 ` [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works al.stone
@ 2013-11-17 22:28 ` Rafael J. Wysocki
2013-11-20 22:17 ` Al Stone
0 siblings, 1 reply; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 22:28 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone
On Saturday, November 09, 2013 06:36:22 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
Doesn't this fix problems with one of the previous patches in the series?
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
> drivers/acpi/bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index f41949a..e868de3 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -513,7 +513,7 @@ void __init acpi_early_init(void)
>
> acpi_gbl_permanent_mmap = 1;
>
> -#if !(CONFIG_ARM || CONFIG_ARM64)
> +#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64))
> /*
> * NB: ARM does not use DMI at present.
> *
> @@ -544,7 +544,7 @@ void __init acpi_early_init(void)
> goto error0;
> }
>
> -#if (!CONFIG_ACPI_REDUCED_HARDWARE)
> +#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64)) && (!ACPI_REDUCED_HARDWARE)
> /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
> if (!acpi_ioapic) {
> /* compatible (0) means level (3) */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works
2013-11-17 22:28 ` Rafael J. Wysocki
@ 2013-11-20 22:17 ` Al Stone
0 siblings, 0 replies; 57+ messages in thread
From: Al Stone @ 2013-11-20 22:17 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-acpi, linaro-acpi, Al Stone
On 11/17/2013 03:28 PM, Rafael J. Wysocki wrote:
> On Saturday, November 09, 2013 06:36:22 PM al.stone@linaro.org wrote:
>> From: Al Stone <ahs3@redhat.com>
>
> Doesn't this fix problems with one of the previous patches in the series?
Whups. Yes, it does. I'll merge them.
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>> drivers/acpi/bus.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index f41949a..e868de3 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -513,7 +513,7 @@ void __init acpi_early_init(void)
>>
>> acpi_gbl_permanent_mmap = 1;
>>
>> -#if !(CONFIG_ARM || CONFIG_ARM64)
>> +#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64))
>> /*
>> * NB: ARM does not use DMI at present.
>> *
>> @@ -544,7 +544,7 @@ void __init acpi_early_init(void)
>> goto error0;
>> }
>>
>> -#if (!CONFIG_ACPI_REDUCED_HARDWARE)
>> +#if !(defined(CONFIG_ARM) || defined(CONFIG_ARM64)) && (!ACPI_REDUCED_HARDWARE)
>> /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
>> if (!acpi_ioapic) {
>> /* compatible (0) means level (3) */
>>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI
2013-11-10 1:36 [PATCH 00/12] Hardware Reduced Mode cleanup for ACPI al.stone
` (11 preceding siblings ...)
2013-11-10 1:36 ` [PATCH 12/12] ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE works al.stone
@ 2013-11-17 21:47 ` Rafael J. Wysocki
12 siblings, 0 replies; 57+ messages in thread
From: Rafael J. Wysocki @ 2013-11-17 21:47 UTC (permalink / raw)
To: al.stone; +Cc: linux-acpi, linaro-acpi, Al Stone, Lv Zheng, Mika Westerberg
On Saturday, November 09, 2013 06:36:10 PM al.stone@linaro.org wrote:
> From: Al Stone <ahs3@redhat.com>
>
> This series of patches starts with Hanjun's patch to create a kernel
> config item for CONFIG_ACPI_REDUCED_HARDWARE [0]. Building on that, I
> then reviewed all of the code that touched any of several fields in the
> FADT that the OSPM is supposed to ignore when ACPI is in Hardware Reduced
> mode [1]. Any time there was a use of one of the fields to be ignored,
> I evaluated whether or not the code was implementing Hardware Reduced
> mode correctly. Similarly, for each the flags in the FADT flags field
> that are to be ignored in Hardware Reduced mode, the kernel code was again
> scanned for proper usage. The remainder of the patches are to fix all of
> the situations I could find where the kernel would not behave correctly
> in this ACPI mode.
>
> These seem to work just fine on the RTSM model for ARMv7, both with and
> without ACPI enabled, and with and without ACPI_REDUCED_HARDWARE enabled;
> similarly for the FVP model for ARMv8. The patches for ACPI on ARM
> hardware will be coming later but they presume that reduced HW mode is
> functioning correctly. In the meantime, there's no way I can think of
> to test all possible scenarios so feedback would be greatly appreciated.
>
>
> [0] List at https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/AcpiReducedHw#Section_5:_ACPI_Software_Programming_Model
> [1] Please see the ACPI Specification v5.0 for details on Hardware Reduced
> mode.
>
>
> Al Stone (12):
> ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE to enable this ACPI mode
> ACPI: bus master reload not supported in reduced HW mode
> ACPI: clean up compiler warning about uninitialized field
> ACPI: HW reduced mode does not allow use of the FADT sci_interrupt
> field
> ACPI: ARM: exclude calls on ARM platforms, not include them on x86
> ACPI: ensure several FADT fields are only used in HW reduced mode
> ACPI: do not reserve memory regions for some FADT entries in HW
> reduced mode
> ACPI: in HW reduced mode, getting power latencies from FADT is not
> allowed
> ACPI: add clarifying comment about processor throttling in HW reduced
> mode
> ACPI: ACPI_FADT_C2_MP_SUPPORTED must be ignored in HW reduced mode
> ACPI: use of ACPI_FADT_32BIT_TIMER is not allowed in HW reduced mode
> ACPI: correct #ifdef so compilation without ACPI_REDUCED_HARDWARE
> works
I have some comments that will be sent in replies to the individual patches.
In addition to that my personal experience with the HW reduced mode is
somewhat limited, so to speak, so I'll need to ask people with more experience
in that field for opinions.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 57+ messages in thread