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:... [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
drivers/acpi/Kconfig | 8 ++++++++ drivers/acpi/acpica/utxface.c | 3 ++- drivers/acpi/bus.c | 7 ++++++- drivers/acpi/osl.c | 28 ++++++++++++++++++++-------- drivers/acpi/pci_link.c | 14 ++++++++------ drivers/acpi/processor_idle.c | 19 ++++++++++++++----- drivers/acpi/processor_throttling.c | 8 ++++++++ drivers/acpi/sleep.c | 2 +- include/acpi/acpixf.h | 6 ++++++ include/acpi/platform/aclinux.h | 4 ++++ 10 files changed, 77 insertions(+), 22 deletions(-)
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
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
On 18 November 2013 06:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
Thanks
Hanjun
On Monday, November 18, 2013 08:48:05 PM Hanjun Guo wrote:
On 18 November 2013 06:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
On 18 November 2013 21:24, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Monday, November 18, 2013 08:48:05 PM Hanjun Guo wrote:
On 18 November 2013 06:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
To telling the truth, I didn't see any other platform using ACPI except IA64, X86 and ARM/ARM64, I just used depends on !(IA64 || x86) for future purpose.
Thanks Hanjun
On Monday, November 18, 2013 09:21:30 PM Hanjun Guo wrote:
On 18 November 2013 21:24, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Monday, November 18, 2013 08:48:05 PM Hanjun Guo wrote:
On 18 November 2013 06:29, Rafael J. Wysocki rjw@rjwysocki.net wrote:
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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
To telling the truth, I didn't see any other platform using ACPI except IA64, X86 and ARM/ARM64, I just used depends on !(IA64 || x86) for future purpose.
However, if you used "depends on ARM || ARM64" (the parens are not needed BTW), the subsequent patches wouldn't need to check CONFIG_ARM/CONFIG_ARM64 in addition to CONFIG_ACPI_REDUCED_HARDWARE. That would simplify stuff somewhat.
Thanks!
+CC Robert Moore
On 2013-11-18 21:37, Rafael J. Wysocki wrote:
On Monday, November 18, 2013 09:21:30 PM Hanjun Guo wrote:
[...]
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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
To telling the truth, I didn't see any other platform using ACPI except IA64, X86 and ARM/ARM64, I just used depends on !(IA64 || x86) for future purpose.
However, if you used "depends on ARM || ARM64" (the parens are not needed BTW), the subsequent patches wouldn't need to check CONFIG_ARM/CONFIG_ARM64 in addition to CONFIG_ACPI_REDUCED_HARDWARE. That would simplify stuff somewhat.
Yes, you are right :) It is ok for me to use "depends on ARM || ARM64", but Robert Moore also suggested that hardware-reduced is not restricted to ARM platforms.
Robert, any comments on this?
Thanks Hanjun
On Tuesday, November 19, 2013 03:32:57 PM Hanjun Guo wrote:
+CC Robert Moore
On 2013-11-18 21:37, Rafael J. Wysocki wrote:
On Monday, November 18, 2013 09:21:30 PM Hanjun Guo wrote:
[...]
> 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?
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
To telling the truth, I didn't see any other platform using ACPI except IA64, X86 and ARM/ARM64, I just used depends on !(IA64 || x86) for future purpose.
However, if you used "depends on ARM || ARM64" (the parens are not needed BTW), the subsequent patches wouldn't need to check CONFIG_ARM/CONFIG_ARM64 in addition to CONFIG_ACPI_REDUCED_HARDWARE. That would simplify stuff somewhat.
Yes, you are right :) It is ok for me to use "depends on ARM || ARM64", but Robert Moore also suggested that hardware-reduced is not restricted to ARM platforms.
From the ACPICA's point of view, it isn't.
In the kernel, however, x86 and ia64 are the only users of ACPI in addition to ARM.
Thanks!
On 2013-11-19 21:10, Rafael J. Wysocki wrote:
On Tuesday, November 19, 2013 03:32:57 PM Hanjun Guo wrote:
+CC Robert Moore
On 2013-11-18 21:37, Rafael J. Wysocki wrote:
On Monday, November 18, 2013 09:21:30 PM Hanjun Guo wrote:
[...]
>> 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? >
hardware-reduced is not restricted to ARM platforms, that's why
I used depends on !(IA64 || X86) here.
So what exactly are the other platforms using ACPI in the Linux kernel?
To telling the truth, I didn't see any other platform using ACPI except IA64, X86 and ARM/ARM64, I just used depends on !(IA64 || x86) for future purpose.
However, if you used "depends on ARM || ARM64" (the parens are not needed BTW), the subsequent patches wouldn't need to check CONFIG_ARM/CONFIG_ARM64 in addition to CONFIG_ACPI_REDUCED_HARDWARE. That would simplify stuff somewhat.
Yes, you are right :) It is ok for me to use "depends on ARM || ARM64", but Robert Moore also suggested that hardware-reduced is not restricted to ARM platforms.
From the ACPICA's point of view, it isn't.
In the kernel, however, x86 and ia64 are the only users of ACPI in addition to ARM.
Ok, will update it in next version, thanks for the guidance.
Hanjun
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
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
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
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
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
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 */
/*
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 */ /*
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 */
/*
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!
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);
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);
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);
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; }
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).
- 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;}
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).
- 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;
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).
- 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;
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).
- 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;
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 deviceThe 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!
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 deviceThe 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.
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)) {
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.**/ dmi_check_system(dsdt_dmi_table);
- If the machine falls into the DMI check table,
- DSDT will be copied to memory
+#endif status = acpi_reallocate_root_table(); if (ACPI_FAILURE(status)) {
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.**/ dmi_check_system(dsdt_dmi_table);
- If the machine falls into the DMI check table,
- DSDT will be copied to memory
+#endif
status = acpi_reallocate_root_table(); if (ACPI_FAILURE(status)) {
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.**/ dmi_check_system(dsdt_dmi_table);
- If the machine falls into the DMI check table,
- DSDT will be copied to memory
+#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
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);
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
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
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) {
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) {
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) {
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)
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)
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)
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; }
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.*//* @@ -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.*/
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?
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.*/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?
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.*/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!
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.
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;
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))#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) &&@@ -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) &&
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))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) &&@@ -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) &&
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;
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)) {
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)) {
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;
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!
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) */
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) */
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) */
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:... [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!
-
Al Stone -
al.stone＠linaro.org
-
Hanjun Guo -
Rafael J. Wysocki -
Zheng, Lv