ACK: [PATCH] pcie: aspm: add more information for L0s and L1 warnings.

[Colin Ian King]

Correction on on subject, I'll ACK it once the typos are fixed.

On 26/09/12 09:30, Colin Ian King wrote:
> On 26/09/12 03:59, Alex Hung wrote:
>> Signed-off-by: Alex Hung <alex.hung at canonical.com>
>> ---
>>   src/pci/aspm/aspm.c |   23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/src/pci/aspm/aspm.c b/src/pci/aspm/aspm.c
>> index 90452c2..f0e6d72 100644
>> --- a/src/pci/aspm/aspm.c
>> +++ b/src/pci/aspm/aspm.c
>> @@ -122,6 +122,7 @@ static int
>> pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>>       uint8_t rp_aspm_cntrl, device_aspm_cntrl;
>>       uint8_t next_cap;
>>       int ret = FWTS_OK;
>> +    bool l0_disabled = false, l1_disabled = false;
>>
>>       next_cap = rp->config[FWTS_PCI_CAPABILITIES_POINTER];
>>       rp_cap = (struct pcie_capability *) &rp->config[next_cap];
>> @@ -146,24 +147,46 @@ static int
>> pcie_compare_rp_dev_aspm_registers(fwts_framework *fw,
>>           (rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
>>           fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L0s not enabled.",
>>                rp->bus, rp->dev, rp->func);
>> +        l0_disabled = true;
>>       }
>>
>>       if (((rp_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >> 10) !=
>>           (rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
>>           fwts_warning(fw, "RP %02Xh:%02Xh.%02Xh L1 not enabled.",
>>               rp->bus, rp->dev, rp->func);
>> +        l1_disabled = true;
>>       }
>>
>>       if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L0_FIELD) >>
>> 10) !=
>>           (device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L0_FIELD)) {
>>           fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L0s not enabled.",
>>               dev->bus, dev->dev, dev->func);
>> +        l0_disabled = true;
>>       }
>>
>>       if (((device_cap->link_cap & FWTS_PCIE_ASPM_SUPPORT_L1_FIELD) >>
>> 10) !=
>>           (device_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_L1_FIELD)) {
>>           fwts_warning(fw, "Device %02Xh:%02Xh.%02Xh L1 not enabled.",
>>               dev->bus, dev->dev, dev->func);
>> +        l1_disabled = true;
>> +    }
>> +
>> +    if (l0_disabled) {
>> +        fwts_advice(fw,
>> +            "The ASPM L0s low power Link state is optimized for "
>> +            "short entry and exit latencies, while providing "
>> +            "substantial power savings. Disabling L0s of a pcie "
>
> Perhaps pcie should be PCIe?
>
>> +            "device may increases power consumption, and will  "
>> +            "impact the battery life of a moble system.");
>
> typo: moble --> mobile
>
>> +    }
>> +
>> +    if (l1_disabled) {
>> +        fwts_advice(fw,
>> +            "The ASPM L1 low power Link state is optimized for "
>> +            "maximum power savings with longer entry and exit "
>> +            "latencies. Disabling L1 of a pcie device may "
>
> Perhaps pcie should be PCIe?
>
>> +            "increases power consumption, and will impact the "
>> +            "battery life of a mobile system significantly.");
>>       }
>>
>>       rp_aspm_cntrl = rp_cap->link_contrl & FWTS_PCIE_ASPM_CONTROL_FIELD;
>>
>
> Apart from the above, messages very are helpful.  Thanks
>
> Colin
>