[PATCH] pcie: aspm: add aspm option and detect if the "PCIe ASPM Controls" bit is set in FADT.
Alex Hung
alex.hung at canonical.com
Tue Dec 6 10:45:05 UTC 2011
Hi Colin,
Thanks for the quick feedback. I haven't finished reading the patch but
just provide quick information.
Two thoughts pop up in my mind when I decide to add aspm option.
1. Smaller changes:
* I intend to add incremental changes, and I think a simple check on
FADT is a good starting point - BIOS tells kernel which Linux should
control ASPM.
* Next thing is the sanity check on PCI(E) hardware registers. This is
the actual ASPM setting, whether it is set by BIOS or by kernel.
* Next thing is _OSC evaluation (more below)
2. _OSC evaluation:
I thought about _OSC when I was adding the feature, and I want to
limited the --aspm to BIOS-to-OS which includes two things - FADT and
the PCI(E) registers, at least to now.
According to Section 4.5.1 in PCI Firmware Specification Rev 3.0, _OSC
is an OS-to-BIOS call
==============================================
On input argument:
Active State Power Management supported
The operating system sets this bit to 1 if it natively supports
configuration of Active
State Power Management registers in PCI Express devices. Otherwise, the
operating system sets this bit to 0
On return values, there is nothing associated to ASPM, which means that
BIOS does not tell OSPM whether OS should have the control (reasonable,
since FADT reports it).
==============================================
As it is not defined very clearly, I think OSPM (i.e. Linux, Windows,
etc...) sets this bit and tells BIOS that it controls ASPM, and I think
the action is based on FADT table as there is no other places that tells
whether OSPM should control ASPM.
Summary of above:
* I intend to implement the checks on FADT and PCI(E) registers first.
* We can discuss the details of _OSC implementation.
What do you think?
P.S. Do you need the PCI firmware document?
Best Regards,
Alex Hung
On 12/06/2011 06:04 PM, Colin Ian King wrote:
> On 06/12/11 06:48, Alex Hung wrote:
>> Signed-off-by: Alex Hung<alex.hung at canonical.com>
>> ---
>> src/lib/include/fwts.h | 1 +
>> src/lib/include/fwts_aspm.h | 42 +++++++++++++++++++++
>> src/lib/src/Makefile.am | 3 +-
>> src/lib/src/fwts_aspm.c | 84
>> ++++++++++++++++++++++++++++++++++++++++++
>> src/lib/src/fwts_framework.c | 4 ++
>> 5 files changed, 133 insertions(+), 1 deletions(-)
>> create mode 100644 src/lib/include/fwts_aspm.h
>> create mode 100644 src/lib/src/fwts_aspm.c
>>
>> diff --git a/src/lib/include/fwts.h b/src/lib/include/fwts.h
>> index f5602ea..abea55a 100644
>> --- a/src/lib/include/fwts.h
>> +++ b/src/lib/include/fwts.h
>> @@ -75,5 +75,6 @@
>> #include "fwts_ac_adapter.h"
>> #include "fwts_battery.h"
>> #include "fwts_button.h"
>> +#include "fwts_aspm.h"
>>
>> #endif
>> diff --git a/src/lib/include/fwts_aspm.h b/src/lib/include/fwts_aspm.h
>> new file mode 100644
>> index 0000000..9695fe9
>> --- /dev/null
>> +++ b/src/lib/include/fwts_aspm.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright (C) 2010-2011 Canonical
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> 02110-1301, USA.
>> + *
>> + */
>> +
>> +#ifndef __FWTS_ASPM_H__
>> +#define __FWTS_ASPM_H__
>> +
>> +#define FWTS_SYS_FIRMWARE_ACPI_TABLE_FACP
>> "/sys/firmware/acpi/tables/FACP"
>> +
>> +#define FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET 109
>> +#define FACP_IAPC_BOOT_ARCH_HIGH_BYTE_OFFSET 110
>> +#define FACP_TABLE_MAX 1024
>> +
>> +#define BIT0 0x01
>> +#define BIT1 0x02
>> +#define BIT2 0x04
>> +#define BIT3 0x08
>> +#define BIT4 0x10
>> +#define BIT5 0x20
>> +#define BIT6 0x40
>> +#define BIT7 0x80
>> +
>> +
>> +
>> +int fwts_aspm_check_configuration(fwts_framework *fw);
>> +
>> +#endif
>> diff --git a/src/lib/src/Makefile.am b/src/lib/src/Makefile.am
>> index 2aac13e..d76d4cd 100644
>> --- a/src/lib/src/Makefile.am
>> +++ b/src/lib/src/Makefile.am
>> @@ -56,4 +56,5 @@ libfwts_la_SOURCES = \
>> fwts_wakealarm.c \
>> fwts_ac_adapter.c \
>> fwts_battery.c \
>> - fwts_button.c
>> + fwts_button.c \
>> + fwts_aspm.c
>> diff --git a/src/lib/src/fwts_aspm.c b/src/lib/src/fwts_aspm.c
>> new file mode 100644
>> index 0000000..397dfc8
>> --- /dev/null
>> +++ b/src/lib/src/fwts_aspm.c
>> @@ -0,0 +1,84 @@
>> +/*
>> + * Copyright (C) 2010-2011 Canonical
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> 02110-1301, USA.
>> + *
>> + */
>> +#include<stdlib.h>
>> +#include<stdio.h>
>> +#include<errno.h>
>> +#include<sys/types.h>
>> +#include<sys/stat.h>
>> +#include<sys/wait.h>
>> +#include<unistd.h>
>> +#include<string.h>
>> +#include<fcntl.h>
>> +#include<limits.h>
>> +#include<linux/pci.h>
>> +
>> +#include "fwts.h"
>> +
>> +int fwts_facp_get_aspm_control(fwts_framework *fw, int *aspm)
>> +{
>> + FILE *fp;
>> + char path[PATH_MAX], facp[FACP_TABLE_MAX];
>> + char c;
>> + int i = 0;
>> + uint16_t iapc_boot_arch_low_byte;
>> +
>> + snprintf(path, sizeof(path), "%s",
>> FWTS_SYS_FIRMWARE_ACPI_TABLE_FACP);
>> + if ((fp = fopen(path, "rb")) == NULL) {
>> + fwts_log_info(fw, "FACP is not present in %s.", path);
>> + return FWTS_ERROR;
>> + }
>> +
>> + c = fgetc(fp);
>> + do {
>> + facp[i] = c;
>> + c = fgetc(fp);
>> + i++;
>> + } while(c != EOF);
>> + fclose(fp);
>> +
>> + iapc_boot_arch_low_byte =
>> facp[FACP_IAPC_BOOT_ARCH_LOW_BYTE_OFFSET];
>> + if ((iapc_boot_arch_low_byte& BIT4) == 0)
>> + {
>> + *aspm = 1;
>> + fwts_log_info(fw, "PCIE ASPM is controlled by Linux kernel.");
>> + } else
>> + {
>> + *aspm = 0;
>> + fwts_log_info(fw, "PCIE ASPM is not controlled by Linux
>> kernel.");
>> + }
>> +
>> + return FWTS_OK;
>> +}
>> +
>> +int fwts_aspm_check_configuration(fwts_framework *fw)
>> +{
>> + int ret;
>> + int aspm_facp;
>> +
>> + ret = fwts_facp_get_aspm_control(fw,&aspm_facp);
>> + if (ret == FWTS_ERROR)
>> + {
>> + fwts_log_info(fw, "No valid FACP information present: cannot
>> test aspm.");
>> + return FWTS_ERROR;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>> diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c
>> index 9898537..7064c44 100644
>> --- a/src/lib/src/fwts_framework.c
>> +++ b/src/lib/src/fwts_framework.c
>> @@ -76,6 +76,7 @@ static fwts_option fwts_framework_options[] = {
>> { "json-data-path", "j:", 1, "Specify path to fwts json
>> data files - default is /usr/share/fwts." },
>> { "lp-tags-log", "", 0, "Output LaunchPad bug tags in
>> results log." },
>> { "disassemble-aml", "", 0, "Disassemble AML from DSDT
>> and SSDT tables." },
>> + { "aspm", "", 0, "Test ASPM configuration." },
>> { NULL, NULL, 0, NULL }
>> };
>>
>> @@ -967,6 +968,9 @@ int fwts_framework_options_handler(fwts_framework
>> *fw, int argc, char * const ar
>> case 31: /* --disassemble-aml */
>> fwts_iasl_disassemble_all_to_file(fw);
>> return FWTS_COMPLETE;
>> + case 32: /* --aspm */
>> + fwts_aspm_check_configuration(fw);
>> + return FWTS_COMPLETE;
>> }
>> break;
>> case 'a': /* --all */
> Come to think of it a little deeper, I believe we need to add some
> more logic for the up and coming Precise kernel as I believe this also
> consults _OSC as well as the boot arch flags before it enables PCIe
> ASPM. I just got Matthew Garrett's PCIe ASPM patch ACK'd and included
> into the next Precise kernel, so I suspect you need to consult this
> patch too to double check exactly what the kernel is doing. The patch
> in question is: http://lwn.net/Articles/467654/
>
> ..it's just some more food for thought.
>
> Colin
>
>
More information about the fwts-devel
mailing list