[PATCH] pcie: aspm: add aspm option and detect if the "PCIe ASPM Controls" bit is set in FADT.
Colin Ian King
colin.king at canonical.com
Tue Dec 6 10:52:30 UTC 2011
On 06/12/11 10:45, Alex Hung wrote:
> 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)
OK - that makes sense.
>
> 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.
Good plan. Let's run with that.
>
> * We can discuss the details of _OSC implementation.
Yep, if you have a look at what the kernel is currently doing and also
what linux-next is doing (as this has Matthew Garrett's ASPM fix) and
then we can discuss this further.
>
> What do you think?
>
> P.S. Do you need the PCI firmware document?
Yes please do.
>
> 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