diff mbox

pcie: aspm: add more information for L0s and L1 warnings.

Message ID 1348628398-13789-1-git-send-email-alex.hung@canonical.com
State Rejected
Headers show

Commit Message

Alex Hung Sept. 26, 2012, 2:59 a.m. UTC 
Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/pci/aspm/aspm.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Colin Ian King Sept. 26, 2012, 8:30 a.m. UTC | #1 
On 26/09/12 03:59, Alex Hung wrote:
> Signed-off-by: Alex Hung <alex.hung@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
Colin Ian King Sept. 26, 2012, 8:33 a.m. UTC | #2 
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@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
>
Keng-Yu Lin Sept. 26, 2012, 9:29 a.m. UTC | #3 
On Wed, Sep 26, 2012 at 10:59 AM, Alex Hung <alex.hung@canonical.com> wrote:
> Signed-off-by: Alex Hung <alex.hung@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;

I hope I am not too paranoid but it puzzled me a bit.
Shall we use "l0s_disabled" instead as it stands for the "L0 Standby"
mode actually.

>
>         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 "
> +                       "device may increases power consumption, and will  "
> +                       "impact the battery life of a moble system.");
> +       }
> +
> +       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 "
> +                       "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;
> --
> 1.7.9.5
>
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
Alex Hung Sept. 26, 2012, 10:08 a.m. UTC | #4 
Hi Keng-yu and Colin, 

Thanks for the feedbacks. I will make changes accordingly.

Sent from my iPad

On 2012/9/26, at 下午5:29, Keng-Yu Lin <kengyu@canonical.com> wrote:

> On Wed, Sep 26, 2012 at 10:59 AM, Alex Hung <alex.hung@canonical.com> wrote:
>> Signed-off-by: Alex Hung <alex.hung@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;
> 
> I hope I am not too paranoid but it puzzled me a bit.
> Shall we use "l0s_disabled" instead as it stands for the "L0 Standby"
> mode actually.
> 
>> 
>>        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 "
>> +                       "device may increases power consumption, and will  "
>> +                       "impact the battery life of a moble system.");
>> +       }
>> +
>> +       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 "
>> +                       "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;
>> --
>> 1.7.9.5
>> 
>> 
>> --
>> fwts-devel mailing list
>> fwts-devel@lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
>
diff mbox

Patch

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 "
+			"device may increases power consumption, and will  "
+			"impact the battery life of a moble system.");
+	}
+
+	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 "
+			"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;