All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] PCI / ACPI: PCI delay optimization from ACPI
@ 2015-03-09  7:46 Aaron Lu
  2015-03-09 14:33 ` Rafael J. Wysocki
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
  0 siblings, 2 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-09  7:46 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas; +Cc: ACPI Devel Mailing List, Linux PCI

An ECN meant to specify possible delay optimizations is available on
the PCI website:
https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
where it has defined two functions for an UUID specified _DSM:
Function 8: If system firmware assumes the responsibility of post
Conventional Reset delay (and informs the Operating System via this DSM
function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
S4 or S3 states), the Operating System may assume sufficient time has
elapsed since the end of reset, and devices within the PCI subsystem are
ready for Configuration Access.
If the system firmware supports runtime power gating on any of the
device within PCI subsystem covered by this DSM function, the system
firmware is responsible for covering the necessary post power-on reset
delay.

Function 9: Specify various smaller delay values than required by the
SPEC for individual PCI devices like shorter delay values after
conventional reset, D3hot to D0 transition, functional level reset, etc.

This patch adds support for function 8 and part of function 9. For
function 8, the patch will check if the required _DSM function satisfies
the requirement and then set the per PCI device's d3cold_delay variable
to zero. For function 9, the values affecting delays after conventional
reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
and d3_delay are updated if the _DSM's return value is smaller than what
the SPEC requires.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 489063987325..c833b054c00c 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Description:
+ *   This function is used to update the d3_delay and d3cold_delay of a PCI
+ *   device from ACPI _DSM control method of either its own or its parent
+ *   bridge device. The UUID of the _DSM control method, together with other
+ *   information like which delay values can be optimized, etc. is defined in
+ *   the fw_latency_optimization ECN availabe on PCIsig.com.
+ *   Function 9 of the ACPI _DSM control method, if availabe for a specific PCI
+ *   device, provides various possible delay values that are less than what the
+ *   SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ *   can be added later.
+ *   Function 8 of the ACPI _DSM control method, if availabe for a specific PCI
+ *   bridge, means all its children devices do not need the reset delay when
+ *   leaving from D3cold state.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
+{
+	const u8 uuid[] = {
+		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
+		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
+	};
+	int revision = 3, function = 9, value;
+	acpi_handle handle = adev->handle;
+	union acpi_object *obj, *elements;
+
+	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
+	if (obj) {
+		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+			elements = obj->package.elements;
+			if (elements[3].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[3].integer.value / 1000;
+				if (value < PCI_PM_D3_WAIT)
+					pdev->d3_delay = value;
+			}
+			if (elements[0].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[0].integer.value / 1000;
+				if (value < PCI_PM_D3COLD_WAIT)
+					pdev->d3cold_delay = value;
+			}
+		}
+		kfree(obj);
+	}
+
+	function = 8;
+	handle = ACPI_HANDLE(pdev->bus->bridge);
+	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
+		pdev->d3cold_delay = 0;
+	kfree(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	if (pci_dev->pm_cap)
+		pci_acpi_delay_optimize(pci_dev, adev);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-09  7:46 [PATCH] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
@ 2015-03-09 14:33 ` Rafael J. Wysocki
  2015-03-10  6:47   ` Aaron Lu
  2015-03-10  6:48   ` [PATCH update] " Aaron Lu
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
  1 sibling, 2 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2015-03-09 14:33 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On Monday, March 09, 2015 03:46:01 PM Aaron Lu wrote:
> An ECN meant to specify possible delay optimizations is available on
> the PCI website:
> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> where it has defined two functions for an UUID specified _DSM:
> Function 8: If system firmware assumes the responsibility of post
> Conventional Reset delay (and informs the Operating System via this DSM
> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> S4 or S3 states), the Operating System may assume sufficient time has
> elapsed since the end of reset, and devices within the PCI subsystem are
> ready for Configuration Access.
> If the system firmware supports runtime power gating on any of the
> device within PCI subsystem covered by this DSM function, the system
> firmware is responsible for covering the necessary post power-on reset
> delay.
> 
> Function 9: Specify various smaller delay values than required by the
> SPEC for individual PCI devices like shorter delay values after
> conventional reset, D3hot to D0 transition, functional level reset, etc.
> 
> This patch adds support for function 8 and part of function 9. For
> function 8, the patch will check if the required _DSM function satisfies
> the requirement and then set the per PCI device's d3cold_delay variable
> to zero. For function 9, the values affecting delays after conventional
> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
> and d3_delay are updated if the _DSM's return value is smaller than what
> the SPEC requires.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 489063987325..c833b054c00c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Description:

The word "Description" above is not necessary.

> + *   This function is used to update the d3_delay and d3cold_delay of a PCI

I'd simply say

"Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM control
method of either its own or its parent bridge."

I'd add an empty line here and then the rest of the below starting with "The UUID ...".

> + *   device from ACPI _DSM control method of either its own or its parent
> + *   bridge device. The UUID of the _DSM control method, together with other
> + *   information like which delay values can be optimized, etc. is defined in
> + *   the fw_latency_optimization ECN availabe on PCIsig.com.

The ECN has a number or title I suppose?

> + *   Function 9 of the ACPI _DSM control method, if availabe for a specific PCI
> + *   device, provides various possible delay values that are less than what the
> + *   SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + *   can be added later.
> + *   Function 8 of the ACPI _DSM control method, if availabe for a specific PCI
> + *   bridge, means all its children devices do not need the reset delay when
> + *   leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
> +{
> +	const u8 uuid[] = {
> +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> +	};
> +	int revision = 3, function = 9, value;
> +	acpi_handle handle = adev->handle;
> +	union acpi_object *obj, *elements;
> +
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +			elements = obj->package.elements;
> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[3].integer.value / 1000;
> +				if (value < PCI_PM_D3_WAIT)
> +					pdev->d3_delay = value;
> +			}
> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[0].integer.value / 1000;
> +				if (value < PCI_PM_D3COLD_WAIT)
> +					pdev->d3cold_delay = value;
> +			}
> +		}
> +		kfree(obj);
> +	}
> +
> +	function = 8;
> +	handle = ACPI_HANDLE(pdev->bus->bridge);
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> +		pdev->d3cold_delay = 0;
> +	kfree(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	if (pci_dev->pm_cap)
> +		pci_acpi_delay_optimize(pci_dev, adev);
> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-09 14:33 ` Rafael J. Wysocki
@ 2015-03-10  6:47   ` Aaron Lu
  2015-03-10  6:48   ` [PATCH update] " Aaron Lu
  1 sibling, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-10  6:47 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On 03/09/2015 10:33 PM, Rafael J. Wysocki wrote:
> On Monday, March 09, 2015 03:46:01 PM Aaron Lu wrote:
>> An ECN meant to specify possible delay optimizations is available on
>> the PCI website:
>> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
>> where it has defined two functions for an UUID specified _DSM:
>> Function 8: If system firmware assumes the responsibility of post
>> Conventional Reset delay (and informs the Operating System via this DSM
>> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
>> S4 or S3 states), the Operating System may assume sufficient time has
>> elapsed since the end of reset, and devices within the PCI subsystem are
>> ready for Configuration Access.
>> If the system firmware supports runtime power gating on any of the
>> device within PCI subsystem covered by this DSM function, the system
>> firmware is responsible for covering the necessary post power-on reset
>> delay.
>>
>> Function 9: Specify various smaller delay values than required by the
>> SPEC for individual PCI devices like shorter delay values after
>> conventional reset, D3hot to D0 transition, functional level reset, etc.
>>
>> This patch adds support for function 8 and part of function 9. For
>> function 8, the patch will check if the required _DSM function satisfies
>> the requirement and then set the per PCI device's d3cold_delay variable
>> to zero. For function 9, the values affecting delays after conventional
>> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
>> and d3_delay are updated if the _DSM's return value is smaller than what
>> the SPEC requires.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 489063987325..c833b054c00c 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>>  				      check_children);
>>  }
>>  
>> +/**
>> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
>> + * @pdev: the PCI device whose delay is to be updated
>> + * @adev: the companion ACPI device of this PCI device
>> + *
>> + * Description:
> 
> The word "Description" above is not necessary.
> 
>> + *   This function is used to update the d3_delay and d3cold_delay of a PCI
> 
> I'd simply say
> 
> "Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM control
> method of either its own or its parent bridge."
> 
> I'd add an empty line here and then the rest of the below starting with "The UUID ...".

OK.

> 
>> + *   device from ACPI _DSM control method of either its own or its parent
>> + *   bridge device. The UUID of the _DSM control method, together with other
>> + *   information like which delay values can be optimized, etc. is defined in
>> + *   the fw_latency_optimization ECN availabe on PCIsig.com.
> 
> The ECN has a number or title I suppose?

Yes.

Thanks for the review and suggestion, I'll send an updated one.

Regards,
Aaron

> 
>> + *   Function 9 of the ACPI _DSM control method, if availabe for a specific PCI
>> + *   device, provides various possible delay values that are less than what the
>> + *   SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
>> + *   can be added later.
>> + *   Function 8 of the ACPI _DSM control method, if availabe for a specific PCI
>> + *   bridge, means all its children devices do not need the reset delay when
>> + *   leaving from D3cold state.
>> + */
>> +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
>> +{
>> +	const u8 uuid[] = {
>> +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
>> +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>> +	};
>> +	int revision = 3, function = 9, value;
>> +	acpi_handle handle = adev->handle;
>> +	union acpi_object *obj, *elements;
>> +
>> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
>> +	if (obj) {
>> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
>> +			elements = obj->package.elements;
>> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
>> +				value = (int)elements[3].integer.value / 1000;
>> +				if (value < PCI_PM_D3_WAIT)
>> +					pdev->d3_delay = value;
>> +			}
>> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
>> +				value = (int)elements[0].integer.value / 1000;
>> +				if (value < PCI_PM_D3COLD_WAIT)
>> +					pdev->d3cold_delay = value;
>> +			}
>> +		}
>> +		kfree(obj);
>> +	}
>> +
>> +	function = 8;
>> +	handle = ACPI_HANDLE(pdev->bus->bridge);
>> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
>> +	if (!obj)
>> +		return;
>> +
>> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
>> +		pdev->d3cold_delay = 0;
>> +	kfree(obj);
>> +}
>> +
>>  static void pci_acpi_setup(struct device *dev)
>>  {
>>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>> @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
>>  	if (!adev)
>>  		return;
>>  
>> +	if (pci_dev->pm_cap)
>> +		pci_acpi_delay_optimize(pci_dev, adev);
>> +
>>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>>  	if (!adev->wakeup.flags.valid)
>>  		return;
>>
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-09 14:33 ` Rafael J. Wysocki
  2015-03-10  6:47   ` Aaron Lu
@ 2015-03-10  6:48   ` Aaron Lu
  2015-03-20  6:14     ` Aaron Lu
  2015-03-20 21:03     ` Bjorn Helgaas
  1 sibling, 2 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-10  6:48 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

An ECN meant to specify possible delay optimizations is available on
the PCI website:
https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
where it has defined two functions for an UUID specified _DSM:
Function 8: If system firmware assumes the responsibility of post
Conventional Reset delay (and informs the Operating System via this DSM
function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
S4 or S3 states), the Operating System may assume sufficient time has
elapsed since the end of reset, and devices within the PCI subsystem are
ready for Configuration Access.
If the system firmware supports runtime power gating on any of the
device within PCI subsystem covered by this DSM function, the system
firmware is responsible for covering the necessary post power-on reset
delay.

Function 9: Specify various smaller delay values than required by the
SPEC for individual PCI devices like shorter delay values after
conventional reset, D3hot to D0 transition, functional level reset, etc.

This patche adds support for function 8 and part of function 9. For
function 8, the patch will check if the required _DSM function satisfies
the requirement and then set the per PCI device's d3cold_delay variable
to zero. For function 9, the values affecting delays after conventional
reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
and d3_delay are updated if the _DSM's return value is smaller than what
the SPEC requires.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 489063987325..468c0733838e 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
+ * control method of either its own or its parent bridge.
+ *
+ * The UUID of the _DSM control method, together with other information like
+ * which delay values can be optimized, etc. is defined in a ECN available on
+ * PCIsig.com titled as: ACPI additions for FW latency optimizations.
+ * Function 9 of the ACPI _DSM control method, if available for a specific PCI
+ * device, provides various possible delay values that are less than what the
+ * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ * can be added later.
+ * Function 8 of the ACPI _DSM control method, if available for a specific PCI
+ * bridge, means all its children devices do not need the reset delay when
+ * leaving from D3cold state.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
+{
+	const u8 uuid[] = {
+		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
+		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
+	};
+	int revision = 3, function = 9, value;
+	acpi_handle handle = adev->handle;
+	union acpi_object *obj, *elements;
+
+	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
+	if (obj) {
+		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+			elements = obj->package.elements;
+			if (elements[3].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[3].integer.value / 1000;
+				if (value < PCI_PM_D3_WAIT)
+					pdev->d3_delay = value;
+			}
+			if (elements[0].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[0].integer.value / 1000;
+				if (value < PCI_PM_D3COLD_WAIT)
+					pdev->d3cold_delay = value;
+			}
+		}
+		kfree(obj);
+	}
+
+	function = 8;
+	handle = ACPI_HANDLE(pdev->bus->bridge);
+	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
+		pdev->d3cold_delay = 0;
+	kfree(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	if (pci_dev->pm_cap)
+		pci_acpi_delay_optimize(pci_dev, adev);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-10  6:48   ` [PATCH update] " Aaron Lu
@ 2015-03-20  6:14     ` Aaron Lu
  2015-03-20 12:39       ` Rafael J. Wysocki
  2015-03-20 21:03     ` Bjorn Helgaas
  1 sibling, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-20  6:14 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

I wonder if there is any more comments about this patch?

Thanks,
Aaron

On 03/10/2015 02:48 PM, Aaron Lu wrote:
> An ECN meant to specify possible delay optimizations is available on
> the PCI website:
> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> where it has defined two functions for an UUID specified _DSM:
> Function 8: If system firmware assumes the responsibility of post
> Conventional Reset delay (and informs the Operating System via this DSM
> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> S4 or S3 states), the Operating System may assume sufficient time has
> elapsed since the end of reset, and devices within the PCI subsystem are
> ready for Configuration Access.
> If the system firmware supports runtime power gating on any of the
> device within PCI subsystem covered by this DSM function, the system
> firmware is responsible for covering the necessary post power-on reset
> delay.
> 
> Function 9: Specify various smaller delay values than required by the
> SPEC for individual PCI devices like shorter delay values after
> conventional reset, D3hot to D0 transition, functional level reset, etc.
> 
> This patche adds support for function 8 and part of function 9. For
> function 8, the patch will check if the required _DSM function satisfies
> the requirement and then set the per PCI device's d3cold_delay variable
> to zero. For function 9, the values affecting delays after conventional
> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
> and d3_delay are updated if the _DSM's return value is smaller than what
> the SPEC requires.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 489063987325..468c0733838e 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * control method of either its own or its parent bridge.
> + *
> + * The UUID of the _DSM control method, together with other information like
> + * which delay values can be optimized, etc. is defined in a ECN available on
> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> + * device, provides various possible delay values that are less than what the
> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + * can be added later.
> + * Function 8 of the ACPI _DSM control method, if available for a specific PCI
> + * bridge, means all its children devices do not need the reset delay when
> + * leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
> +{
> +	const u8 uuid[] = {
> +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> +	};
> +	int revision = 3, function = 9, value;
> +	acpi_handle handle = adev->handle;
> +	union acpi_object *obj, *elements;
> +
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +			elements = obj->package.elements;
> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[3].integer.value / 1000;
> +				if (value < PCI_PM_D3_WAIT)
> +					pdev->d3_delay = value;
> +			}
> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[0].integer.value / 1000;
> +				if (value < PCI_PM_D3COLD_WAIT)
> +					pdev->d3cold_delay = value;
> +			}
> +		}
> +		kfree(obj);
> +	}
> +
> +	function = 8;
> +	handle = ACPI_HANDLE(pdev->bus->bridge);
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> +		pdev->d3cold_delay = 0;
> +	kfree(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	if (pci_dev->pm_cap)
> +		pci_acpi_delay_optimize(pci_dev, adev);
> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-20  6:14     ` Aaron Lu
@ 2015-03-20 12:39       ` Rafael J. Wysocki
  0 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2015-03-20 12:39 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On Friday, March 20, 2015 02:14:38 PM Aaron Lu wrote:
> I wonder if there is any more comments about this patch?

I don't have any, but I'd like to know the Bjorn's opinion too.


> On 03/10/2015 02:48 PM, Aaron Lu wrote:
> > An ECN meant to specify possible delay optimizations is available on
> > the PCI website:
> > https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> > where it has defined two functions for an UUID specified _DSM:
> > Function 8: If system firmware assumes the responsibility of post
> > Conventional Reset delay (and informs the Operating System via this DSM
> > function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> > S4 or S3 states), the Operating System may assume sufficient time has
> > elapsed since the end of reset, and devices within the PCI subsystem are
> > ready for Configuration Access.
> > If the system firmware supports runtime power gating on any of the
> > device within PCI subsystem covered by this DSM function, the system
> > firmware is responsible for covering the necessary post power-on reset
> > delay.
> > 
> > Function 9: Specify various smaller delay values than required by the
> > SPEC for individual PCI devices like shorter delay values after
> > conventional reset, D3hot to D0 transition, functional level reset, etc.
> > 
> > This patche adds support for function 8 and part of function 9. For
> > function 8, the patch will check if the required _DSM function satisfies
> > the requirement and then set the per PCI device's d3cold_delay variable
> > to zero. For function 9, the values affecting delays after conventional
> > reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
> > and d3_delay are updated if the _DSM's return value is smaller than what
> > the SPEC requires.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 489063987325..468c0733838e 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
> >  				      check_children);
> >  }
> >  
> > +/**
> > + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> > + * @pdev: the PCI device whose delay is to be updated
> > + * @adev: the companion ACPI device of this PCI device
> > + *
> > + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> > + * control method of either its own or its parent bridge.
> > + *
> > + * The UUID of the _DSM control method, together with other information like
> > + * which delay values can be optimized, etc. is defined in a ECN available on
> > + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> > + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> > + * device, provides various possible delay values that are less than what the
> > + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> > + * can be added later.
> > + * Function 8 of the ACPI _DSM control method, if available for a specific PCI
> > + * bridge, means all its children devices do not need the reset delay when
> > + * leaving from D3cold state.
> > + */
> > +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
> > +{
> > +	const u8 uuid[] = {
> > +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> > +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> > +	};
> > +	int revision = 3, function = 9, value;
> > +	acpi_handle handle = adev->handle;
> > +	union acpi_object *obj, *elements;
> > +
> > +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> > +	if (obj) {
> > +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> > +			elements = obj->package.elements;
> > +			if (elements[3].type == ACPI_TYPE_INTEGER) {
> > +				value = (int)elements[3].integer.value / 1000;
> > +				if (value < PCI_PM_D3_WAIT)
> > +					pdev->d3_delay = value;
> > +			}
> > +			if (elements[0].type == ACPI_TYPE_INTEGER) {
> > +				value = (int)elements[0].integer.value / 1000;
> > +				if (value < PCI_PM_D3COLD_WAIT)
> > +					pdev->d3cold_delay = value;
> > +			}
> > +		}
> > +		kfree(obj);
> > +	}
> > +
> > +	function = 8;
> > +	handle = ACPI_HANDLE(pdev->bus->bridge);
> > +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> > +	if (!obj)
> > +		return;
> > +
> > +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> > +		pdev->d3cold_delay = 0;
> > +	kfree(obj);
> > +}
> > +
> >  static void pci_acpi_setup(struct device *dev)
> >  {
> >  	struct pci_dev *pci_dev = to_pci_dev(dev);
> > @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
> >  	if (!adev)
> >  		return;
> >  
> > +	if (pci_dev->pm_cap)
> > +		pci_acpi_delay_optimize(pci_dev, adev);
> > +
> >  	pci_acpi_add_pm_notifier(adev, pci_dev);
> >  	if (!adev->wakeup.flags.valid)
> >  		return;
> > 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-10  6:48   ` [PATCH update] " Aaron Lu
  2015-03-20  6:14     ` Aaron Lu
@ 2015-03-20 21:03     ` Bjorn Helgaas
  2015-03-23  5:35       ` Aaron Lu
                         ` (2 more replies)
  1 sibling, 3 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2015-03-20 21:03 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Tue, Mar 10, 2015 at 02:48:04PM +0800, Aaron Lu wrote:
> An ECN meant to specify possible delay optimizations is available on
> the PCI website:
> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> where it has defined two functions for an UUID specified _DSM:
> Function 8: If system firmware assumes the responsibility of post
> Conventional Reset delay (and informs the Operating System via this DSM
> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> S4 or S3 states), the Operating System may assume sufficient time has
> elapsed since the end of reset, and devices within the PCI subsystem are
> ready for Configuration Access.
> If the system firmware supports runtime power gating on any of the
> device within PCI subsystem covered by this DSM function, the system
> firmware is responsible for covering the necessary post power-on reset
> delay.
> 
> Function 9: Specify various smaller delay values than required by the
> SPEC for individual PCI devices like shorter delay values after
> conventional reset, D3hot to D0 transition, functional level reset, etc.
> 
> This patche adds support for function 8 and part of function 9. For
> function 8, the patch will check if the required _DSM function satisfies
> the requirement and then set the per PCI device's d3cold_delay variable
> to zero. For function 9, the values affecting delays after conventional
> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
> and d3_delay are updated if the _DSM's return value is smaller than what
> the SPEC requires.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 489063987325..468c0733838e 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * control method of either its own or its parent bridge.
> + *
> + * The UUID of the _DSM control method, together with other information like
> + * which delay values can be optimized, etc. is defined in a ECN available on
> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> + * device, provides various possible delay values that are less than what the
> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + * can be added later.
> + * Function 8 of the ACPI _DSM control method, if available for a specific PCI
> + * bridge, means all its children devices do not need the reset delay when
> + * leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
> +{
> +	const u8 uuid[] = {
> +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> +	};

This is a duplicate of device_label_dsm_uuid[] from
drivers/pci/pci-label.c.  I don't really want two copies.

That UUID is not specific to device labels, so device_label_dsm_uuid[] is
mis-named anyway.  It's just the UUID for the single _DSM for PCI (see PCI
Firmware Specification, r3.0, sec 4.6), and all these different things
(device label, reset delay, slot info, etc.) use the same UUID with
different function indices.

We should also make #defines for the function indices instead of using
hard-coded numbers here.

> +	int revision = 3, function = 9, value;
> +	acpi_handle handle = adev->handle;
> +	union acpi_object *obj, *elements;
> +
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +			elements = obj->package.elements;
> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[3].integer.value / 1000;
> +				if (value < PCI_PM_D3_WAIT)
> +					pdev->d3_delay = value;
> +			}
> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[0].integer.value / 1000;
> +				if (value < PCI_PM_D3COLD_WAIT)
> +					pdev->d3cold_delay = value;
> +			}

Unless there's a reason to do this in "element[3], element[0]" order,
please do it in the natural "0, 3" order.

> +		}
> +		kfree(obj);
> +	}
> +
> +	function = 8;
> +	handle = ACPI_HANDLE(pdev->bus->bridge);
> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);

Hmm.  I think the ECN is poorly worded here.  Sec 4.6.8 says "This object
[_DSM] can only be placed within the scope of a PCI host bus." I think it
means "this _DSM *function* can only be implemented ..." (since any device
can have a _DSM), and I think it means "host *bridge*" (not bus, since I
don't think there's an ACPI object for a PCI bus).

It probably should say "This function can be implemented only by a _DSM
method within the scope of a PCI host bridge."

Anyway, I think this patch looks for a _DSM in PCI-PCI bridge devices as
well as PCI host bridge devices, and the ECN says any values returned by
function 8 of a non-host bridge _DSM method should be ignored.  At least,
that's how I read it.

I think you need something in pci_root.c that evaluates _DSM function 8 for
the host bridge, and then some mechanism for all the devices under that
bridge to inherit the result.

> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> +		pdev->d3cold_delay = 0;
> +	kfree(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	if (pci_dev->pm_cap)
> +		pci_acpi_delay_optimize(pci_dev, adev);
> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-20 21:03     ` Bjorn Helgaas
@ 2015-03-23  5:35       ` Aaron Lu
  2015-03-23  9:15         ` Aaron Lu
  2015-03-23  9:16       ` [PATCH 1/2] PCI: rename dsm uuid for PCI Aaron Lu
  2015-03-23  9:17       ` [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
  2 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-23  5:35 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On 03/21/2015 05:03 AM, Bjorn Helgaas wrote:
> On Tue, Mar 10, 2015 at 02:48:04PM +0800, Aaron Lu wrote:
>> An ECN meant to specify possible delay optimizations is available on
>> the PCI website:
>> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
>> where it has defined two functions for an UUID specified _DSM:
>> Function 8: If system firmware assumes the responsibility of post
>> Conventional Reset delay (and informs the Operating System via this DSM
>> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
>> S4 or S3 states), the Operating System may assume sufficient time has
>> elapsed since the end of reset, and devices within the PCI subsystem are
>> ready for Configuration Access.
>> If the system firmware supports runtime power gating on any of the
>> device within PCI subsystem covered by this DSM function, the system
>> firmware is responsible for covering the necessary post power-on reset
>> delay.
>>
>> Function 9: Specify various smaller delay values than required by the
>> SPEC for individual PCI devices like shorter delay values after
>> conventional reset, D3hot to D0 transition, functional level reset, etc.
>>
>> This patche adds support for function 8 and part of function 9. For
>> function 8, the patch will check if the required _DSM function satisfies
>> the requirement and then set the per PCI device's d3cold_delay variable
>> to zero. For function 9, the values affecting delays after conventional
>> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
>> and d3_delay are updated if the _DSM's return value is smaller than what
>> the SPEC requires.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/pci/pci-acpi.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 489063987325..468c0733838e 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -558,6 +558,64 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>>  				      check_children);
>>  }
>>  
>> +/**
>> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
>> + * @pdev: the PCI device whose delay is to be updated
>> + * @adev: the companion ACPI device of this PCI device
>> + *
>> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
>> + * control method of either its own or its parent bridge.
>> + *
>> + * The UUID of the _DSM control method, together with other information like
>> + * which delay values can be optimized, etc. is defined in a ECN available on
>> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
>> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
>> + * device, provides various possible delay values that are less than what the
>> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
>> + * can be added later.
>> + * Function 8 of the ACPI _DSM control method, if available for a specific PCI
>> + * bridge, means all its children devices do not need the reset delay when
>> + * leaving from D3cold state.
>> + */
>> +static void pci_acpi_delay_optimize(struct pci_dev *pdev, struct acpi_device *adev)
>> +{
>> +	const u8 uuid[] = {
>> +		0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
>> +		0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>> +	};
> 
> This is a duplicate of device_label_dsm_uuid[] from
> drivers/pci/pci-label.c.  I don't really want two copies.
> 
> That UUID is not specific to device labels, so device_label_dsm_uuid[] is
> mis-named anyway.  It's just the UUID for the single _DSM for PCI (see PCI
> Firmware Specification, r3.0, sec 4.6), and all these different things
> (device label, reset delay, slot info, etc.) use the same UUID with
> different function indices.
> 
> We should also make #defines for the function indices instead of using
> hard-coded numbers here.

OK.

> 
>> +	int revision = 3, function = 9, value;
>> +	acpi_handle handle = adev->handle;
>> +	union acpi_object *obj, *elements;
>> +
>> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
>> +	if (obj) {
>> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
>> +			elements = obj->package.elements;
>> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
>> +				value = (int)elements[3].integer.value / 1000;
>> +				if (value < PCI_PM_D3_WAIT)
>> +					pdev->d3_delay = value;
>> +			}
>> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
>> +				value = (int)elements[0].integer.value / 1000;
>> +				if (value < PCI_PM_D3COLD_WAIT)
>> +					pdev->d3cold_delay = value;
>> +			}
> 
> Unless there's a reason to do this in "element[3], element[0]" order,
> please do it in the natural "0, 3" order.

OK.

> 
>> +		}
>> +		kfree(obj);
>> +	}
>> +
>> +	function = 8;
>> +	handle = ACPI_HANDLE(pdev->bus->bridge);
>> +	obj = acpi_evaluate_dsm(handle, uuid, revision, function, NULL);
> 
> Hmm.  I think the ECN is poorly worded here.  Sec 4.6.8 says "This object
> [_DSM] can only be placed within the scope of a PCI host bus." I think it
> means "this _DSM *function* can only be implemented ..." (since any device
> can have a _DSM), and I think it means "host *bridge*" (not bus, since I
> don't think there's an ACPI object for a PCI bus).

I'm confused by this too and then I found the firmware I worked with has
this _DSM function 8 implemented for not only the PCI0 firmware node, but
also the RP01, RP02, etc. firmware nodes which corresponds to the 1c.0,
1c.1, etc. PCI bridges, so I wrote the patch this way. Looks like I
should ignore them instead :-)

> 
> It probably should say "This function can be implemented only by a _DSM
> method within the scope of a PCI host bridge."

Agreed.

> 
> Anyway, I think this patch looks for a _DSM in PCI-PCI bridge devices as
> well as PCI host bridge devices, and the ECN says any values returned by
> function 8 of a non-host bridge _DSM method should be ignored.  At least,
> that's how I read it.

OK, I'll rework the patch to only check the host bridge for function 8.

> 
> I think you need something in pci_root.c that evaluates _DSM function 8 for
> the host bridge, and then some mechanism for all the devices under that
> bridge to inherit the result.

Thanks for the suggestion, will try to do this in the next revision.

Regards,
Aaron

> 
>> +	if (!obj)
>> +		return;
>> +
>> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
>> +		pdev->d3cold_delay = 0;
>> +	kfree(obj);
>> +}
>> +
>>  static void pci_acpi_setup(struct device *dev)
>>  {
>>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>> @@ -566,6 +624,9 @@ static void pci_acpi_setup(struct device *dev)
>>  	if (!adev)
>>  		return;
>>  
>> +	if (pci_dev->pm_cap)
>> +		pci_acpi_delay_optimize(pci_dev, adev);
>> +
>>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>>  	if (!adev->wakeup.flags.valid)
>>  		return;
>> -- 
>> 2.1.0
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH update] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-23  5:35       ` Aaron Lu
@ 2015-03-23  9:15         ` Aaron Lu
  0 siblings, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-23  9:15 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On 03/23/2015 01:35 PM, Aaron Lu wrote:
> On 03/21/2015 05:03 AM, Bjorn Helgaas wrote:
>> I think you need something in pci_root.c that evaluates _DSM function 8 for
>> the host bridge, and then some mechanism for all the devices under that
>> bridge to inherit the result.
> 
> Thanks for the suggestion, will try to do this in the next revision.

Turned out I can decide if a PCI bridge is the host bridge or not by
checking: pdev->bus->bridge->parent pointer, so I did something like
this instead:

+       /* Function 8 is only applicable to host bridge */
+       if (pdev->bus->bridge->parent)
+               return;

Looks good?

And I also renamed the UUID and moved the common definitions to pci.h,
so the following two patches do these things:
patch 1: rename the PCI UUID and move the definitions to pci.h;
patch 2: add check for host bridge in pci_acpi_delay_optimize, re-order
         elements[0] and elements[3] check and use macros for _DSM
	 function 8 and 9.

Thanks,
Aaron

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 1/2] PCI: rename dsm uuid for PCI
  2015-03-20 21:03     ` Bjorn Helgaas
  2015-03-23  5:35       ` Aaron Lu
@ 2015-03-23  9:16       ` Aaron Lu
  2015-03-24  0:24         ` Rafael J. Wysocki
  2015-03-23  9:17       ` [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
  2 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-23  9:16 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

According to the PCI firmware spec, there is only one assigned UUID used
for PCI system so rename the device_label_dsm_uuid to something more
common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
that uuid array extern in the pci.h so that other code can also make use
of it.

This patch shouldn't bring any functional change.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c  |  5 +++++
 drivers/pci/pci-label.c | 11 ++---------
 drivers/pci/pci.h       |  6 ++++++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 489063987325..6ef2019073e2 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -18,6 +18,11 @@
 #include <linux/pm_qos.h>
 #include "pci.h"
 
+const u8 pci_acpi_dsm_uuid[] = {
+	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
+	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
+};
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 2ab1b47c7651..024b5c179348 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -31,8 +31,6 @@
 #include <linux/pci-acpi.h>
 #include "pci.h"
 
-#define	DEVICE_LABEL_DSM	0x07
-
 #ifdef CONFIG_DMI
 enum smbios_attr_enum {
 	SMBIOS_ATTR_NONE = 0,
@@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
 #endif
 
 #ifdef CONFIG_ACPI
-static const char device_label_dsm_uuid[] = {
-	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
-	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
-};
-
 enum acpi_attr_enum {
 	ACPI_ATTR_LABEL_SHOW,
 	ACPI_ATTR_INDEX_SHOW,
@@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 	if (!handle)
 		return -1;
 
-	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				DEVICE_LABEL_DSM, NULL);
 	if (!obj)
 		return -1;
@@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
 	if (!handle)
 		return false;
 
-	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
+	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				1 << DEVICE_LABEL_DSM);
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4091f82239cd..c901ab84cf3b 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -321,4 +321,10 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+#ifdef CONFIG_ACPI
+extern const u8 pci_acpi_dsm_uuid[];
+
+#define DEVICE_LABEL_DSM	0x07
+#endif
+
 #endif /* DRIVERS_PCI_H */
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-20 21:03     ` Bjorn Helgaas
  2015-03-23  5:35       ` Aaron Lu
  2015-03-23  9:16       ` [PATCH 1/2] PCI: rename dsm uuid for PCI Aaron Lu
@ 2015-03-23  9:17       ` Aaron Lu
  2015-03-23 15:09         ` Bjorn Helgaas
  2 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-23  9:17 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

An ECN meant to specify possible delay optimizations is available on
the PCI website:
https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
where it has defined two functions for an UUID specified _DSM:
Function 8: If system firmware assumes the responsibility of post
Conventional Reset delay (and informs the Operating System via this DSM
function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
S4 or S3 states), the Operating System may assume sufficient time has
elapsed since the end of reset, and devices within the PCI subsystem are
ready for Configuration Access.
If the system firmware supports runtime power gating on any of the
device within PCI subsystem covered by this DSM function, the system
firmware is responsible for covering the necessary post power-on reset
delay.

Function 9: Specify various smaller delay values than required by the
SPEC for individual PCI devices like shorter delay values after
conventional reset, D3hot to D0 transition, functional level reset, etc.

This patche adds support for function 8 and part of function 9. For
function 8, the patch will check if the required _DSM function satisfies
the requirement and then set the per PCI device's d3cold_delay variable
to zero. For function 9, the values affecting delays after conventional
reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
and d3_delay are updated if the _DSM's return value is smaller than what
the SPEC requires.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h      |  2 ++
 2 files changed, 65 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 6ef2019073e2..2ebd02d814d7 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -563,6 +563,66 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
+ * control method of either its own or its parent bridge.
+ *
+ * The UUID of the _DSM control method, together with other information like
+ * which delay values can be optimized, etc. is defined in a ECN available on
+ * PCIsig.com titled as: ACPI additions for FW latency optimizations.
+ * Function 9 of the ACPI _DSM control method, if available for a specific PCI
+ * device, provides various possible delay values that are less than what the
+ * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ * can be added later.
+ * Function 8 of the ACPI _DSM control method, if available for the PCI host
+ * bridge, means all its children devices do not need the reset delay when
+ * leaving from D3cold state.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev,
+				    struct acpi_device *adev)
+{
+	acpi_handle handle = adev->handle;
+	int value;
+	union acpi_object *obj, *elements;
+
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
+				FUNCTION_DELAY_DSM, NULL);
+	if (obj) {
+		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+			elements = obj->package.elements;
+			if (elements[0].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[0].integer.value / 1000;
+				if (value < PCI_PM_D3COLD_WAIT)
+					pdev->d3cold_delay = value;
+			}
+			if (elements[3].type == ACPI_TYPE_INTEGER) {
+				value = (int)elements[3].integer.value / 1000;
+				if (value < PCI_PM_D3_WAIT)
+					pdev->d3_delay = value;
+			}
+		}
+		kfree(obj);
+	}
+
+	/* Function 8 is only applicable to host bridge */
+	if (pdev->bus->bridge->parent)
+		return;
+
+	handle = ACPI_HANDLE(pdev->bus->bridge);
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
+				RESET_DELAY_DSM, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
+		pdev->d3cold_delay = 0;
+	kfree(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -571,6 +631,9 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	if (pci_dev->pm_cap)
+		pci_acpi_delay_optimize(pci_dev, adev);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c901ab84cf3b..3eaefac5acc5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -325,6 +325,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 extern const u8 pci_acpi_dsm_uuid[];
 
 #define DEVICE_LABEL_DSM	0x07
+#define RESET_DELAY_DSM		0x08
+#define FUNCTION_DELAY_DSM	0x09
 #endif
 
 #endif /* DRIVERS_PCI_H */
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-23  9:17       ` [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
@ 2015-03-23 15:09         ` Bjorn Helgaas
  2015-03-24  9:04           ` [PATCH v2 " Aaron Lu
  0 siblings, 1 reply; 29+ messages in thread
From: Bjorn Helgaas @ 2015-03-23 15:09 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Mon, Mar 23, 2015 at 05:17:44PM +0800, Aaron Lu wrote:
> An ECN meant to specify possible delay optimizations is available on
> the PCI website:
> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> where it has defined two functions for an UUID specified _DSM:
> Function 8: If system firmware assumes the responsibility of post
> Conventional Reset delay (and informs the Operating System via this DSM
> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> S4 or S3 states), the Operating System may assume sufficient time has
> elapsed since the end of reset, and devices within the PCI subsystem are
> ready for Configuration Access.
> If the system firmware supports runtime power gating on any of the
> device within PCI subsystem covered by this DSM function, the system
> firmware is responsible for covering the necessary post power-on reset
> delay.
> 
> Function 9: Specify various smaller delay values than required by the
> SPEC for individual PCI devices like shorter delay values after
> conventional reset, D3hot to D0 transition, functional level reset, etc.
> 
> This patche adds support for function 8 and part of function 9. For
> function 8, the patch will check if the required _DSM function satisfies
> the requirement and then set the per PCI device's d3cold_delay variable
> to zero. For function 9, the values affecting delays after conventional
> reset and D3hot->D0 are examined and the per PCI device's d3cold_delay
> and d3_delay are updated if the _DSM's return value is smaller than what
> the SPEC requires.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h      |  2 ++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 6ef2019073e2..2ebd02d814d7 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -563,6 +563,66 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * control method of either its own or its parent bridge.
> + *
> + * The UUID of the _DSM control method, together with other information like
> + * which delay values can be optimized, etc. is defined in a ECN available on
> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> + * device, provides various possible delay values that are less than what the
> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + * can be added later.
> + * Function 8 of the ACPI _DSM control method, if available for the PCI host
> + * bridge, means all its children devices do not need the reset delay when
> + * leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev,
> +				    struct acpi_device *adev)
> +{
> +	acpi_handle handle = adev->handle;
> +	int value;
> +	union acpi_object *obj, *elements;
> +
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
> +				FUNCTION_DELAY_DSM, NULL);
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +			elements = obj->package.elements;
> +			if (elements[0].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[0].integer.value / 1000;
> +				if (value < PCI_PM_D3COLD_WAIT)
> +					pdev->d3cold_delay = value;
> +			}
> +			if (elements[3].type == ACPI_TYPE_INTEGER) {
> +				value = (int)elements[3].integer.value / 1000;
> +				if (value < PCI_PM_D3_WAIT)
> +					pdev->d3_delay = value;
> +			}
> +		}
> +		kfree(obj);
> +	}
> +
> +	/* Function 8 is only applicable to host bridge */
> +	if (pdev->bus->bridge->parent)
> +		return;
> +
> +	handle = ACPI_HANDLE(pdev->bus->bridge);
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
> +				RESET_DELAY_DSM, NULL);

The ECN says this function is in a host bridge scope and applies to the
PCI subsystem beneath the bridge.  This code does not map well to that
because:

  1) It evaluates _DSM more times than necessary (we only need to do it
  once per host bridge, and this does it once for every PCI device
  immediately below a host brige).

  2) The settings are only applied to immediate children of the host
  bridge, not to devices deeper in the hierarchy.

  3) A reader of the ECN will expect the corresponding code to be in the
  host bridge driver (pci_root.c) where we deal with other host bridge
  properties, not in per-PCI device code like this.

  4) The ECN is not explicit about this, but if both function 8 (which
  applies to a whole hierarchy) and function 9 (which applies to a single
  PCI device) are implemented for the same PCI device, I would expect
  function 9 to take precedence over function 8.  This patch does the
  reverse, since function 8 will overwrite any d3cold_delay that was set
  above by function 9.

> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> +		pdev->d3cold_delay = 0;
> +	kfree(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -571,6 +631,9 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	if (pci_dev->pm_cap)
> +		pci_acpi_delay_optimize(pci_dev, adev);
> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index c901ab84cf3b..3eaefac5acc5 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -325,6 +325,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  extern const u8 pci_acpi_dsm_uuid[];
>  
>  #define DEVICE_LABEL_DSM	0x07
> +#define RESET_DELAY_DSM		0x08
> +#define FUNCTION_DELAY_DSM	0x09
>  #endif
>  
>  #endif /* DRIVERS_PCI_H */
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] PCI: rename dsm uuid for PCI
  2015-03-23  9:16       ` [PATCH 1/2] PCI: rename dsm uuid for PCI Aaron Lu
@ 2015-03-24  0:24         ` Rafael J. Wysocki
  2015-03-24  0:35           ` Aaron Lu
  0 siblings, 1 reply; 29+ messages in thread
From: Rafael J. Wysocki @ 2015-03-24  0:24 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

So _DSM is an ACPI-defined object and please spell its name like that.
Also please spell UUID in capitals if that's not a problem.

On Monday, March 23, 2015 05:16:15 PM Aaron Lu wrote:
> According to the PCI firmware spec, there is only one assigned UUID used
> for PCI system so rename the device_label_dsm_uuid to something more
> common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
> that uuid array extern in the pci.h so that other code can also make use
> of it.
> 
> This patch shouldn't bring any functional change.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c  |  5 +++++
>  drivers/pci/pci-label.c | 11 ++---------
>  drivers/pci/pci.h       |  6 ++++++
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 489063987325..6ef2019073e2 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -18,6 +18,11 @@
>  #include <linux/pm_qos.h>
>  #include "pci.h"
>  

Please add a comment referring to the document defining the UUID here.

> +const u8 pci_acpi_dsm_uuid[] = {
> +	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> +	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> +};
> +
>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>  {
>  	acpi_status status = AE_NOT_EXIST;
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 2ab1b47c7651..024b5c179348 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -31,8 +31,6 @@
>  #include <linux/pci-acpi.h>
>  #include "pci.h"
>  
> -#define	DEVICE_LABEL_DSM	0x07
> -
>  #ifdef CONFIG_DMI
>  enum smbios_attr_enum {
>  	SMBIOS_ATTR_NONE = 0,
> @@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
>  #endif
>  
>  #ifdef CONFIG_ACPI
> -static const char device_label_dsm_uuid[] = {
> -	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
> -	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
> -};
> -
>  enum acpi_attr_enum {
>  	ACPI_ATTR_LABEL_SHOW,
>  	ACPI_ATTR_INDEX_SHOW,
> @@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>  	if (!handle)
>  		return -1;
>  
> -	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>  				DEVICE_LABEL_DSM, NULL);
>  	if (!obj)
>  		return -1;
> @@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
>  	if (!handle)
>  		return false;
>  
> -	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
> +	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>  				1 << DEVICE_LABEL_DSM);
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4091f82239cd..c901ab84cf3b 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -321,4 +321,10 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>  
> +#ifdef CONFIG_ACPI
> +extern const u8 pci_acpi_dsm_uuid[];
> +
> +#define DEVICE_LABEL_DSM	0x07
> +#endif

We have pci-acpi.h.  Maybe define this stuff in there and make the user(s)
include that?

> +
>  #endif /* DRIVERS_PCI_H */
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] PCI: rename dsm uuid for PCI
  2015-03-24  0:24         ` Rafael J. Wysocki
@ 2015-03-24  0:35           ` Aaron Lu
  2015-03-24  1:03             ` Rafael J. Wysocki
  0 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-24  0:35 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On 03/24/2015 08:24 AM, Rafael J. Wysocki wrote:
> So _DSM is an ACPI-defined object and please spell its name like that.
> Also please spell UUID in capitals if that's not a problem.

Something like acpi_pci_dsm_UUID?

> 
> On Monday, March 23, 2015 05:16:15 PM Aaron Lu wrote:
>> According to the PCI firmware spec, there is only one assigned UUID used
>> for PCI system so rename the device_label_dsm_uuid to something more
>> common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
>> that uuid array extern in the pci.h so that other code can also make use
>> of it.
>>
>> This patch shouldn't bring any functional change.
>>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/pci/pci-acpi.c  |  5 +++++
>>  drivers/pci/pci-label.c | 11 ++---------
>>  drivers/pci/pci.h       |  6 ++++++
>>  3 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 489063987325..6ef2019073e2 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -18,6 +18,11 @@
>>  #include <linux/pm_qos.h>
>>  #include "pci.h"
>>  
> 
> Please add a comment referring to the document defining the UUID here.

OK.

> 
>> +const u8 pci_acpi_dsm_uuid[] = {
>> +	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
>> +	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>> +};
>> +
>>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>>  {
>>  	acpi_status status = AE_NOT_EXIST;
>> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
>> index 2ab1b47c7651..024b5c179348 100644
>> --- a/drivers/pci/pci-label.c
>> +++ b/drivers/pci/pci-label.c
>> @@ -31,8 +31,6 @@
>>  #include <linux/pci-acpi.h>
>>  #include "pci.h"
>>  
>> -#define	DEVICE_LABEL_DSM	0x07
>> -
>>  #ifdef CONFIG_DMI
>>  enum smbios_attr_enum {
>>  	SMBIOS_ATTR_NONE = 0,
>> @@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
>>  #endif
>>  
>>  #ifdef CONFIG_ACPI
>> -static const char device_label_dsm_uuid[] = {
>> -	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
>> -	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
>> -};
>> -
>>  enum acpi_attr_enum {
>>  	ACPI_ATTR_LABEL_SHOW,
>>  	ACPI_ATTR_INDEX_SHOW,
>> @@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>>  	if (!handle)
>>  		return -1;
>>  
>> -	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
>> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>>  				DEVICE_LABEL_DSM, NULL);
>>  	if (!obj)
>>  		return -1;
>> @@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
>>  	if (!handle)
>>  		return false;
>>  
>> -	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
>> +	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>>  				1 << DEVICE_LABEL_DSM);
>>  }
>>  
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 4091f82239cd..c901ab84cf3b 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -321,4 +321,10 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_ACPI
>> +extern const u8 pci_acpi_dsm_uuid[];
>> +
>> +#define DEVICE_LABEL_DSM	0x07
>> +#endif
> 
> We have pci-acpi.h.  Maybe define this stuff in there and make the user(s)
> include that?

No problem.

Thanks,
Aaron

> 
>> +
>>  #endif /* DRIVERS_PCI_H */
>>
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] PCI: rename dsm uuid for PCI
  2015-03-24  0:35           ` Aaron Lu
@ 2015-03-24  1:03             ` Rafael J. Wysocki
  2015-03-24  9:04               ` [PATCH v2 1/2] PCI: rename _DSM UUID array Aaron Lu
  0 siblings, 1 reply; 29+ messages in thread
From: Rafael J. Wysocki @ 2015-03-24  1:03 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On Tuesday, March 24, 2015 08:35:36 AM Aaron Lu wrote:
> On 03/24/2015 08:24 AM, Rafael J. Wysocki wrote:
> > So _DSM is an ACPI-defined object and please spell its name like that.
> > Also please spell UUID in capitals if that's not a problem.
> 
> Something like acpi_pci_dsm_UUID?

No, not in variable names. :-)

In the subject, comments etc (where it makes sense).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 1/2] PCI: rename _DSM UUID array
  2015-03-24  1:03             ` Rafael J. Wysocki
@ 2015-03-24  9:04               ` Aaron Lu
  2015-03-24 21:57                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-24  9:04 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

According to the PCI firmware spec, there is only one assigned UUID used
for PCI system so rename the device_label_dsm_uuid to something more
common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
that UUID array extern in the pci-acpi.h so that other code can also
make use of it.

This patch shouldn't bring any functional change.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c   |  9 +++++++++
 drivers/pci/pci-label.c  | 11 ++---------
 include/linux/pci-acpi.h |  3 +++
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 489063987325..e0afc94aca01 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -18,6 +18,15 @@
 #include <linux/pm_qos.h>
 #include "pci.h"
 
+/*
+ * The UUID is defined in the PCI firmware specification available here:
+ * https://www.pcisig.com/members/downloads/pcifw_r3_1_13Dec10.pdf
+ */
+const u8 pci_acpi_dsm_uuid[] = {
+	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
+	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
+};
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 2ab1b47c7651..024b5c179348 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -31,8 +31,6 @@
 #include <linux/pci-acpi.h>
 #include "pci.h"
 
-#define	DEVICE_LABEL_DSM	0x07
-
 #ifdef CONFIG_DMI
 enum smbios_attr_enum {
 	SMBIOS_ATTR_NONE = 0,
@@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
 #endif
 
 #ifdef CONFIG_ACPI
-static const char device_label_dsm_uuid[] = {
-	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
-	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
-};
-
 enum acpi_attr_enum {
 	ACPI_ATTR_LABEL_SHOW,
 	ACPI_ATTR_INDEX_SHOW,
@@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 	if (!handle)
 		return -1;
 
-	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				DEVICE_LABEL_DSM, NULL);
 	if (!obj)
 		return -1;
@@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
 	if (!handle)
 		return false;
 
-	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
+	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				1 << DEVICE_LABEL_DSM);
 }
 
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 24c7728ca681..3801c704a945 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -77,6 +77,9 @@ static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
 static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 #endif
 
+extern const u8 pci_acpi_dsm_uuid[];
+#define DEVICE_LABEL_DSM	0x07
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-23 15:09         ` Bjorn Helgaas
@ 2015-03-24  9:04           ` Aaron Lu
  2015-03-24 14:08             ` Bjorn Helgaas
  0 siblings, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-24  9:04 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On 03/23/2015 11:09 PM, Bjorn Helgaas wrote:
> The ECN says this function is in a host bridge scope and applies to the
> PCI subsystem beneath the bridge.  This code does not map well to that
> because:
> 
>   1) It evaluates _DSM more times than necessary (we only need to do it
>   once per host bridge, and this does it once for every PCI device
>   immediately below a host brige).
> 
>   2) The settings are only applied to immediate children of the host
>   bridge, not to devices deeper in the hierarchy.
> 
>   3) A reader of the ECN will expect the corresponding code to be in the
>   host bridge driver (pci_root.c) where we deal with other host bridge
>   properties, not in per-PCI device code like this.
> 
>   4) The ECN is not explicit about this, but if both function 8 (which
>   applies to a whole hierarchy) and function 9 (which applies to a single
>   PCI device) are implemented for the same PCI device, I would expect
>   function 9 to take precedence over function 8.  This patch does the
>   reverse, since function 8 will overwrite any d3cold_delay that was set
>   above by function 9.

I tried to do this in drivers/acpi/pci_root.c, but didn't find a proper
way to pass this information down during PCI device scan time. So
instead, I did it in the pci root bus add time: acpi_pci_add_bus, which
is used by both the x86 code and ia64 code. Suggestions are welcome and
appreciated.


>From 05b625d2444d90e37392dd835a97c0b582fd221f Mon Sep 17 00:00:00 2001
From: Aaron Lu <aaron.lu@intel.com>
Date: Fri, 14 Nov 2014 16:56:43 +0800
Subject: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI

An ECN meant to specify possible delay optimizations is available on
the PCI website:
https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
where it has defined two functions for an UUID specified _DSM:
Function 8: If system firmware assumes the responsibility of post
Conventional Reset delay (and informs the Operating System via this DSM
function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
S4 or S3 states), the Operating System may assume sufficient time has
elapsed since the end of reset, and devices within the PCI subsystem are
ready for Configuration Access.
If the system firmware supports runtime power gating on any of the
device within PCI subsystem covered by this DSM function, the system
firmware is responsible for covering the necessary post power-on reset
delay.

Function 9: Specify various smaller delay values than required by the
SPEC for individual PCI devices like shorter delay values after
conventional reset, D3hot to D0 transition, functional level reset, etc.

This patche adds support for function 8 and part of function 9. For
function 8, the patch will check if the required _DSM function satisfies
the requirement and then all the host bus' immediate children PCI device's
d3cold_delay variable will be updated to zero. For function 9, the values
affecting delays after conventional reset and D3hot->D0 are examined and
the per PCI device's d3cold_delay and d3_delay are updated if the _DSM's
return value is smaller than what the SPEC requires. Function 9's value
takes precedence over function 8.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c   | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  2 ++
 include/linux/pci.h      |  1 +
 3 files changed, 68 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index e0afc94aca01..220371c2def4 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -537,11 +537,24 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
 
 void acpi_pci_add_bus(struct pci_bus *bus)
 {
+	union acpi_object *obj;
+
 	if (acpi_pci_disabled || !bus->bridge)
 		return;
 
 	acpi_pci_slot_enumerate(bus);
 	acpiphp_enumerate_slots(bus);
+
+	/*
+	 * For a host bridge, check its _DSM for function 8 and if
+	 * that is available, mark it in the corresponding pci_bus.
+	 */
+	if (bus->bridge->parent)
+		return;
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
+				RESET_DELAY_DSM, NULL);
+	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
+		bus->ignore_reset_delay = 1;
 }
 
 void acpi_pci_remove_bus(struct pci_bus *bus)
@@ -567,6 +580,55 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
+ * control method of either its own or its parent bridge.
+ *
+ * The UUID of the _DSM control method, together with other information like
+ * which delay values can be optimized, etc. is defined in a ECN available on
+ * PCIsig.com titled as: ACPI additions for FW latency optimizations.
+ * Function 9 of the ACPI _DSM control method, if available for a specific PCI
+ * device, provides various possible delay values that are less than what the
+ * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ * can be added later.
+ * Function 8 of the ACPI _DSM control method, if available for the PCI host
+ * bridge(reflected by the bus' ignore_reset_delay filed), means all its
+ * children devices do not need the reset delay when leaving from D3cold state.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev,
+				    acpi_handle handle)
+{
+	int value;
+	union acpi_object *obj, *elements;
+
+	if (pdev->bus->ignore_reset_delay)
+		pdev->d3cold_delay = 0;
+
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
+				FUNCTION_DELAY_DSM, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+		elements = obj->package.elements;
+		if (elements[0].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[0].integer.value / 1000;
+			if (value < PCI_PM_D3COLD_WAIT)
+				pdev->d3cold_delay = value;
+		}
+		if (elements[3].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[3].integer.value / 1000;
+			if (value < PCI_PM_D3_WAIT)
+				pdev->d3_delay = value;
+		}
+	}
+	kfree(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -575,6 +637,9 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	if (pci_dev->pm_cap)
+		pci_acpi_delay_optimize(pci_dev, adev->handle);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 3801c704a945..a965efa52152 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -79,6 +79,8 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 
 extern const u8 pci_acpi_dsm_uuid[];
 #define DEVICE_LABEL_DSM	0x07
+#define RESET_DELAY_DSM		0x08
+#define FUNCTION_DELAY_DSM	0x09
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a379513bddef..1e56c464d058 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -471,6 +471,7 @@ struct pci_bus {
 	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */
 	struct bin_attribute	*legacy_mem; /* legacy mem */
 	unsigned int		is_added:1;
+	unsigned int		ignore_reset_delay:1;
 };
 
 #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-24  9:04           ` [PATCH v2 " Aaron Lu
@ 2015-03-24 14:08             ` Bjorn Helgaas
  2015-03-24 15:16               ` Aaron Lu
  2015-03-24 15:37               ` Aaron Lu
  0 siblings, 2 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2015-03-24 14:08 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Tue, Mar 24, 2015 at 05:04:58PM +0800, Aaron Lu wrote:
> On 03/23/2015 11:09 PM, Bjorn Helgaas wrote:
> > The ECN says this function is in a host bridge scope and applies to the
> > PCI subsystem beneath the bridge.  This code does not map well to that
> > because:
> > 
> >   1) It evaluates _DSM more times than necessary (we only need to do it
> >   once per host bridge, and this does it once for every PCI device
> >   immediately below a host brige).
> > 
> >   2) The settings are only applied to immediate children of the host
> >   bridge, not to devices deeper in the hierarchy.
> > 
> >   3) A reader of the ECN will expect the corresponding code to be in the
> >   host bridge driver (pci_root.c) where we deal with other host bridge
> >   properties, not in per-PCI device code like this.
> > 
> >   4) The ECN is not explicit about this, but if both function 8 (which
> >   applies to a whole hierarchy) and function 9 (which applies to a single
> >   PCI device) are implemented for the same PCI device, I would expect
> >   function 9 to take precedence over function 8.  This patch does the
> >   reverse, since function 8 will overwrite any d3cold_delay that was set
> >   above by function 9.
> 
> I tried to do this in drivers/acpi/pci_root.c, but didn't find a proper
> way to pass this information down during PCI device scan time. So
> instead, I did it in the pci root bus add time: acpi_pci_add_bus, which
> is used by both the x86 code and ia64 code. Suggestions are welcome and
> appreciated.
> 
> 
> From 05b625d2444d90e37392dd835a97c0b582fd221f Mon Sep 17 00:00:00 2001
> From: Aaron Lu <aaron.lu@intel.com>
> Date: Fri, 14 Nov 2014 16:56:43 +0800
> Subject: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
> 
> An ECN meant to specify possible delay optimizations is available on
> the PCI website:
> https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
> where it has defined two functions for an UUID specified _DSM:
> Function 8: If system firmware assumes the responsibility of post
> Conventional Reset delay (and informs the Operating System via this DSM
> function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
> S4 or S3 states), the Operating System may assume sufficient time has
> elapsed since the end of reset, and devices within the PCI subsystem are
> ready for Configuration Access.
> If the system firmware supports runtime power gating on any of the
> device within PCI subsystem covered by this DSM function, the system
> firmware is responsible for covering the necessary post power-on reset
> delay.
> 
> Function 9: Specify various smaller delay values than required by the
> SPEC for individual PCI devices like shorter delay values after
> conventional reset, D3hot to D0 transition, functional level reset, etc.
> 
> This patche adds support for function 8 and part of function 9. For
> function 8, the patch will check if the required _DSM function satisfies
> the requirement and then all the host bus' immediate children PCI device's
> d3cold_delay variable will be updated to zero. For function 9, the values
> affecting delays after conventional reset and D3hot->D0 are examined and
> the per PCI device's d3cold_delay and d3_delay are updated if the _DSM's
> return value is smaller than what the SPEC requires. Function 9's value
> takes precedence over function 8.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/pci/pci-acpi.c   | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |  2 ++
>  include/linux/pci.h      |  1 +
>  3 files changed, 68 insertions(+)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index e0afc94aca01..220371c2def4 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -537,11 +537,24 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
>  
>  void acpi_pci_add_bus(struct pci_bus *bus)
>  {
> +	union acpi_object *obj;
> +
>  	if (acpi_pci_disabled || !bus->bridge)
>  		return;
>  
>  	acpi_pci_slot_enumerate(bus);
>  	acpiphp_enumerate_slots(bus);
> +
> +	/*
> +	 * For a host bridge, check its _DSM for function 8 and if
> +	 * that is available, mark it in the corresponding pci_bus.
> +	 */
> +	if (bus->bridge->parent)
> +		return;

This is not really an obvious way of testing for a host bridge.  I think
pci_is_root_bus() would be a better way, but I'm still hoping for something
in pci_root.c instead.  There is find_pci_host_bridge(), which might be
useful (it's currently static but we might want to rename and export it for
this and other reasons).

> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
> +				RESET_DELAY_DSM, NULL);
> +	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
> +		bus->ignore_reset_delay = 1;

I think you need to free "obj" here.  Other acpi_evaluate_dsm() callers use
ACPI_FREE().

>  }
>  
>  void acpi_pci_remove_bus(struct pci_bus *bus)
> @@ -567,6 +580,55 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * control method of either its own or its parent bridge.
> + *
> + * The UUID of the _DSM control method, together with other information like
> + * which delay values can be optimized, etc. is defined in a ECN available on
> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> + * device, provides various possible delay values that are less than what the
> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + * can be added later.
> + * Function 8 of the ACPI _DSM control method, if available for the PCI host
> + * bridge(reflected by the bus' ignore_reset_delay filed), means all its
> + * children devices do not need the reset delay when leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev,
> +				    acpi_handle handle)
> +{
> +	int value;
> +	union acpi_object *obj, *elements;
> +
> +	if (pdev->bus->ignore_reset_delay)
> +		pdev->d3cold_delay = 0;

I think this only propagates the function 8 result to the immediate
children of the host bridge, i.e., devices on the root bus.  But the ECN
says it affects the entire hierarchy.  Can you put the ignore_reset_delay
bit in the struct pci_host_bridge instead?

> +
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
> +				FUNCTION_DELAY_DSM, NULL);
> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +		elements = obj->package.elements;
> +		if (elements[0].type == ACPI_TYPE_INTEGER) {
> +			value = (int)elements[0].integer.value / 1000;
> +			if (value < PCI_PM_D3COLD_WAIT)
> +				pdev->d3cold_delay = value;
> +		}
> +		if (elements[3].type == ACPI_TYPE_INTEGER) {
> +			value = (int)elements[3].integer.value / 1000;
> +			if (value < PCI_PM_D3_WAIT)
> +				pdev->d3_delay = value;
> +		}
> +	}
> +	kfree(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -575,6 +637,9 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	if (pci_dev->pm_cap)
> +		pci_acpi_delay_optimize(pci_dev, adev->handle);

Is the "pm_cap" test really necessary?  If we do it this way, we then have
to convince ourselves that pdev->d3cold_delay and pdev->d3_delay are only
needed when pdev has a pm_cap.

If we *always* fill in the delay values, it's possible they won't be used,
but we don't have to prove any connection between them and a pm_cap, so
the code is easier to analyze.

> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 3801c704a945..a965efa52152 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -79,6 +79,8 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  
>  extern const u8 pci_acpi_dsm_uuid[];
>  #define DEVICE_LABEL_DSM	0x07
> +#define RESET_DELAY_DSM		0x08
> +#define FUNCTION_DELAY_DSM	0x09
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a379513bddef..1e56c464d058 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -471,6 +471,7 @@ struct pci_bus {
>  	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */
>  	struct bin_attribute	*legacy_mem; /* legacy mem */
>  	unsigned int		is_added:1;
> +	unsigned int		ignore_reset_delay:1;
>  };
>  
>  #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-24 14:08             ` Bjorn Helgaas
@ 2015-03-24 15:16               ` Aaron Lu
  2015-03-24 22:09                 ` Bjorn Helgaas
  2015-03-24 15:37               ` Aaron Lu
  1 sibling, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-24 15:16 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On 03/24/2015 10:08 PM, Bjorn Helgaas wrote:
> On Tue, Mar 24, 2015 at 05:04:58PM +0800, Aaron Lu wrote:
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index e0afc94aca01..220371c2def4 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -537,11 +537,24 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
>>  
>>  void acpi_pci_add_bus(struct pci_bus *bus)
>>  {
>> +	union acpi_object *obj;
>> +
>>  	if (acpi_pci_disabled || !bus->bridge)
>>  		return;
>>  
>>  	acpi_pci_slot_enumerate(bus);
>>  	acpiphp_enumerate_slots(bus);
>> +
>> +	/*
>> +	 * For a host bridge, check its _DSM for function 8 and if
>> +	 * that is available, mark it in the corresponding pci_bus.
>> +	 */
>> +	if (bus->bridge->parent)
>> +		return;
> 
> This is not really an obvious way of testing for a host bridge.  I think
> pci_is_root_bus() would be a better way, but I'm still hoping for something
> in pci_root.c instead.  There is find_pci_host_bridge(), which might be
> useful (it's currently static but we might want to rename and export it for
> this and other reasons).

I agree that pci_root.c is the proper place for a property of the PCI host
bridge, but since it only calls:
	root->bus = pci_acpi_scan_root(root);
and when it returns, eveything is already properly initialized. To pass
the ignore_reset_delay information from there, we probably need to add a
field ignore_reset_delay to struct acpi_pci_root and then pass that
information to pci_root_info and then pass the pointer of pci_root_info
to pci_create_root_bus where we use that information to set the field in
the pci_host_bridge. Intead of doing it this way, hook the
acpi_pci_add_bus to set the ignore_reset_delay bit of the PCI host
bridge seems to be cheap and easier.

> 
>> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
>> +				RESET_DELAY_DSM, NULL);
>> +	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1)
>> +		bus->ignore_reset_delay = 1;
> 
> I think you need to free "obj" here.  Other acpi_evaluate_dsm() callers use
> ACPI_FREE().

Oops yes.

> 
>>  }
>>  
>>  void acpi_pci_remove_bus(struct pci_bus *bus)
>> @@ -567,6 +580,55 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>>  				      check_children);
>>  }
>>  
>> +/**
>> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
>> + * @pdev: the PCI device whose delay is to be updated
>> + * @adev: the companion ACPI device of this PCI device
>> + *
>> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
>> + * control method of either its own or its parent bridge.
>> + *
>> + * The UUID of the _DSM control method, together with other information like
>> + * which delay values can be optimized, etc. is defined in a ECN available on
>> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
>> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
>> + * device, provides various possible delay values that are less than what the
>> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
>> + * can be added later.
>> + * Function 8 of the ACPI _DSM control method, if available for the PCI host
>> + * bridge(reflected by the bus' ignore_reset_delay filed), means all its
>> + * children devices do not need the reset delay when leaving from D3cold state.
>> + */
>> +static void pci_acpi_delay_optimize(struct pci_dev *pdev,
>> +				    acpi_handle handle)
>> +{
>> +	int value;
>> +	union acpi_object *obj, *elements;
>> +
>> +	if (pdev->bus->ignore_reset_delay)
>> +		pdev->d3cold_delay = 0;
> 
> I think this only propagates the function 8 result to the immediate
> children of the host bridge, i.e., devices on the root bus.  But the ECN
> says it affects the entire hierarchy.  Can you put the ignore_reset_delay
> bit in the struct pci_host_bridge instead?

No problem.

> 
>> +
>> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
>> +				FUNCTION_DELAY_DSM, NULL);
>> +	if (!obj)
>> +		return;
>> +
>> +	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
>> +		elements = obj->package.elements;
>> +		if (elements[0].type == ACPI_TYPE_INTEGER) {
>> +			value = (int)elements[0].integer.value / 1000;
>> +			if (value < PCI_PM_D3COLD_WAIT)
>> +				pdev->d3cold_delay = value;
>> +		}
>> +		if (elements[3].type == ACPI_TYPE_INTEGER) {
>> +			value = (int)elements[3].integer.value / 1000;
>> +			if (value < PCI_PM_D3_WAIT)
>> +				pdev->d3_delay = value;
>> +		}
>> +	}
>> +	kfree(obj);
>> +}
>> +
>>  static void pci_acpi_setup(struct device *dev)
>>  {
>>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>> @@ -575,6 +637,9 @@ static void pci_acpi_setup(struct device *dev)
>>  	if (!adev)
>>  		return;
>>  
>> +	if (pci_dev->pm_cap)
>> +		pci_acpi_delay_optimize(pci_dev, adev->handle);
> 
> Is the "pm_cap" test really necessary?  If we do it this way, we then have
> to convince ourselves that pdev->d3cold_delay and pdev->d3_delay are only
> needed when pdev has a pm_cap.
> 
> If we *always* fill in the delay values, it's possible they won't be used,
> but we don't have to prove any connection between them and a pm_cap, so
> the code is easier to analyze.

Brilliant.

Here is an updat, kind of a proof of concept one, to see if it goes the
right way:

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 3e5bbf9e8889..e40512d3f373 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -16,7 +16,7 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
 	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index e0afc94aca01..04d5b5befbe9 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -537,11 +537,32 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
 
 void acpi_pci_add_bus(struct pci_bus *bus)
 {
+	union acpi_object *obj;
+	struct pci_host_bridge *bridge;
+
 	if (acpi_pci_disabled || !bus->bridge)
 		return;
 
 	acpi_pci_slot_enumerate(bus);
 	acpiphp_enumerate_slots(bus);
+
+	/*
+	 * For a host bridge, check its _DSM for function 8 and if
+	 * that is available, mark it in pci_host_bridge.
+	 */
+	if (!pci_is_root_bus(bus))
+		return;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
+				RESET_DELAY_DSM, NULL);
+	if (obj) {
+		if (obj->type == ACPI_TYPE_INTEGER &&
+		    obj->integer.value == 1) {
+			bridge = find_pci_host_bridge(bus);
+			bridge->ignore_reset_delay = true;
+		}
+		ACPI_FREE(obj);
+	}
 }
 
 void acpi_pci_remove_bus(struct pci_bus *bus)
@@ -567,6 +588,56 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
+ * control method of either its own or its parent bridge.
+ *
+ * The UUID of the _DSM control method, together with other information like
+ * which delay values can be optimized, etc. is defined in a ECN available on
+ * PCIsig.com titled as: ACPI additions for FW latency optimizations.
+ * Function 9 of the ACPI _DSM control method, if available for a specific PCI
+ * device, provides various possible delay values that are less than what the
+ * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ * can be added later.
+ * Function 8 of the ACPI _DSM control method, if available for the PCI host
+ * bridge(reflected by the bus' ignore_reset_delay filed), means all its
+ * children devices do not need the reset delay when leaving from D3cold state.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev,
+				    acpi_handle handle)
+{
+	struct pci_host_bridge *bridge = find_pci_host_bridge(pdev->bus);
+	int value;
+	union acpi_object *obj, *elements;
+
+	if (bridge->ignore_reset_delay)
+		pdev->d3cold_delay = 0;
+
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
+				FUNCTION_DELAY_DSM, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+		elements = obj->package.elements;
+		if (elements[0].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[0].integer.value / 1000;
+			if (value < PCI_PM_D3COLD_WAIT)
+				pdev->d3cold_delay = value;
+		}
+		if (elements[3].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[3].integer.value / 1000;
+			if (value < PCI_PM_D3_WAIT)
+				pdev->d3_delay = value;
+		}
+	}
+	ACPI_FREE(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -575,6 +646,8 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	pci_acpi_delay_optimize(pci_dev, adev->handle);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4091f82239cd..802e7c0c7f9f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -321,4 +321,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
+
 #endif /* DRIVERS_PCI_H */
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 3801c704a945..a965efa52152 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -79,6 +79,8 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 
 extern const u8 pci_acpi_dsm_uuid[];
 #define DEVICE_LABEL_DSM	0x07
+#define RESET_DELAY_DSM		0x08
+#define FUNCTION_DELAY_DSM	0x09
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a379513bddef..e587832885e9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -406,6 +406,7 @@ struct pci_host_bridge {
 	struct list_head windows;	/* resource_entry */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
+	bool ignore_reset_delay;
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-24 14:08             ` Bjorn Helgaas
  2015-03-24 15:16               ` Aaron Lu
@ 2015-03-24 15:37               ` Aaron Lu
  2015-03-24 22:10                 ` Bjorn Helgaas
  1 sibling, 1 reply; 29+ messages in thread
From: Aaron Lu @ 2015-03-24 15:37 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On 03/24/2015 10:08 PM, Bjorn Helgaas wrote:
> On Tue, Mar 24, 2015 at 05:04:58PM +0800, Aaron Lu wrote:
>> @@ -575,6 +637,9 @@ static void pci_acpi_setup(struct device *dev)
>>  	if (!adev)
>>  		return;
>>  
>> +	if (pci_dev->pm_cap)
>> +		pci_acpi_delay_optimize(pci_dev, adev->handle);
> 
> Is the "pm_cap" test really necessary?  If we do it this way, we then have
> to convince ourselves that pdev->d3cold_delay and pdev->d3_delay are only
> needed when pdev has a pm_cap.
> 
> If we *always* fill in the delay values, it's possible they won't be used,
> but we don't have to prove any connection between them and a pm_cap, so
> the code is easier to analyze.

I remembered why I did the pm_cap test: the d3cold_delay and d3_delay is
only filled when pm_cap is set in pci_pm_init - if the device doesn't
have PCI_CAP_ID_PM set, its pm_cap will be 0 and d3cold_delay, d3_delay
will not be assigned.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 1/2] PCI: rename _DSM UUID array
  2015-03-24  9:04               ` [PATCH v2 1/2] PCI: rename _DSM UUID array Aaron Lu
@ 2015-03-24 21:57                 ` Rafael J. Wysocki
  0 siblings, 0 replies; 29+ messages in thread
From: Rafael J. Wysocki @ 2015-03-24 21:57 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Bjorn Helgaas, ACPI Devel Mailing List, Linux PCI

On Tuesday, March 24, 2015 05:04:25 PM Aaron Lu wrote:
> According to the PCI firmware spec, there is only one assigned UUID used
> for PCI system so rename the device_label_dsm_uuid to something more
> common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
> that UUID array extern in the pci-acpi.h so that other code can also
> make use of it.
> 
> This patch shouldn't bring any functional change.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci-acpi.c   |  9 +++++++++
>  drivers/pci/pci-label.c  | 11 ++---------
>  include/linux/pci-acpi.h |  3 +++
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 489063987325..e0afc94aca01 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -18,6 +18,15 @@
>  #include <linux/pm_qos.h>
>  #include "pci.h"
>  
> +/*
> + * The UUID is defined in the PCI firmware specification available here:
> + * https://www.pcisig.com/members/downloads/pcifw_r3_1_13Dec10.pdf
> + */
> +const u8 pci_acpi_dsm_uuid[] = {
> +	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
> +	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> +};
> +
>  phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>  {
>  	acpi_status status = AE_NOT_EXIST;
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 2ab1b47c7651..024b5c179348 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -31,8 +31,6 @@
>  #include <linux/pci-acpi.h>
>  #include "pci.h"
>  
> -#define	DEVICE_LABEL_DSM	0x07
> -
>  #ifdef CONFIG_DMI
>  enum smbios_attr_enum {
>  	SMBIOS_ATTR_NONE = 0,
> @@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
>  #endif
>  
>  #ifdef CONFIG_ACPI
> -static const char device_label_dsm_uuid[] = {
> -	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
> -	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
> -};
> -
>  enum acpi_attr_enum {
>  	ACPI_ATTR_LABEL_SHOW,
>  	ACPI_ATTR_INDEX_SHOW,
> @@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
>  	if (!handle)
>  		return -1;
>  
> -	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>  				DEVICE_LABEL_DSM, NULL);
>  	if (!obj)
>  		return -1;
> @@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
>  	if (!handle)
>  		return false;
>  
> -	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
> +	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
>  				1 << DEVICE_LABEL_DSM);
>  }
>  
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 24c7728ca681..3801c704a945 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -77,6 +77,9 @@ static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
>  static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  #endif
>  
> +extern const u8 pci_acpi_dsm_uuid[];
> +#define DEVICE_LABEL_DSM	0x07
> +
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-24 15:16               ` Aaron Lu
@ 2015-03-24 22:09                 ` Bjorn Helgaas
  0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2015-03-24 22:09 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Tue, Mar 24, 2015 at 11:16:38PM +0800, Aaron Lu wrote:
> Here is an updat, kind of a proof of concept one, to see if it goes the
> right way:

I think what you've outlined below makes sense.  I had a few minor
comments.

> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 3e5bbf9e8889..e40512d3f373 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -16,7 +16,7 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>  	return bus;
>  }
>  
> -static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)

I'd change the name to "pci_find_host_bridge()" to be more like other
interfaces.  Do that in a separate patch because it changes some callers
and can be done separately.

>  {
>  	struct pci_bus *root_bus = find_pci_root_bus(bus);
>  
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index e0afc94aca01..04d5b5befbe9 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -537,11 +537,32 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
>  
>  void acpi_pci_add_bus(struct pci_bus *bus)
>  {
> +	union acpi_object *obj;
> +	struct pci_host_bridge *bridge;
> +
>  	if (acpi_pci_disabled || !bus->bridge)
>  		return;
>  
>  	acpi_pci_slot_enumerate(bus);
>  	acpiphp_enumerate_slots(bus);
> +
> +	/*
> +	 * For a host bridge, check its _DSM for function 8 and if
> +	 * that is available, mark it in pci_host_bridge.
> +	 */
> +	if (!pci_is_root_bus(bus))
> +		return;
> +
> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
> +				RESET_DELAY_DSM, NULL);
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_INTEGER &&
> +		    obj->integer.value == 1) {
> +			bridge = find_pci_host_bridge(bus);
> +			bridge->ignore_reset_delay = true;
> +		}
> +		ACPI_FREE(obj);
> +	}

    if (!obj)
	return;

    <mainline code is normal path, as you did below>
    ACPI_FREE(obj);

>  }
>  
>  void acpi_pci_remove_bus(struct pci_bus *bus)
> @@ -567,6 +588,56 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
>  				      check_children);
>  }
>  
> +/**
> + * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
> + * @pdev: the PCI device whose delay is to be updated
> + * @adev: the companion ACPI device of this PCI device
> + *
> + * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
> + * control method of either its own or its parent bridge.
> + *
> + * The UUID of the _DSM control method, together with other information like
> + * which delay values can be optimized, etc. is defined in a ECN available on
> + * PCIsig.com titled as: ACPI additions for FW latency optimizations.
> + * Function 9 of the ACPI _DSM control method, if available for a specific PCI
> + * device, provides various possible delay values that are less than what the
> + * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
> + * can be added later.
> + * Function 8 of the ACPI _DSM control method, if available for the PCI host
> + * bridge(reflected by the bus' ignore_reset_delay filed), means all its
> + * children devices do not need the reset delay when leaving from D3cold state.
> + */
> +static void pci_acpi_delay_optimize(struct pci_dev *pdev,
> +				    acpi_handle handle)
> +{
> +	struct pci_host_bridge *bridge = find_pci_host_bridge(pdev->bus);
> +	int value;
> +	union acpi_object *obj, *elements;
> +
> +	if (bridge->ignore_reset_delay)
> +		pdev->d3cold_delay = 0;
> +
> +	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
> +				FUNCTION_DELAY_DSM, NULL);
> +	if (!obj)
> +		return;
> +
> +	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +		elements = obj->package.elements;
> +		if (elements[0].type == ACPI_TYPE_INTEGER) {
> +			value = (int)elements[0].integer.value / 1000;
> +			if (value < PCI_PM_D3COLD_WAIT)
> +				pdev->d3cold_delay = value;
> +		}
> +		if (elements[3].type == ACPI_TYPE_INTEGER) {
> +			value = (int)elements[3].integer.value / 1000;
> +			if (value < PCI_PM_D3_WAIT)
> +				pdev->d3_delay = value;
> +		}
> +	}
> +	ACPI_FREE(obj);
> +}
> +
>  static void pci_acpi_setup(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -575,6 +646,8 @@ static void pci_acpi_setup(struct device *dev)
>  	if (!adev)
>  		return;
>  
> +	pci_acpi_delay_optimize(pci_dev, adev->handle);
> +
>  	pci_acpi_add_pm_notifier(adev, pci_dev);
>  	if (!adev->wakeup.flags.valid)
>  		return;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4091f82239cd..802e7c0c7f9f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -321,4 +321,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>  
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 3801c704a945..a965efa52152 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -79,6 +79,8 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  
>  extern const u8 pci_acpi_dsm_uuid[];
>  #define DEVICE_LABEL_DSM	0x07
> +#define RESET_DELAY_DSM		0x08
> +#define FUNCTION_DELAY_DSM	0x09
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a379513bddef..e587832885e9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -406,6 +406,7 @@ struct pci_host_bridge {
>  	struct list_head windows;	/* resource_entry */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> +	bool ignore_reset_delay;

I'm not sold on this new-fangled "bool" thing yet.  Please just use
"unsigned int ignore_reset_delay:1" like most other uses in pci.h.

>  };
>  
>  #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
> -- 
> 2.1.0
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2 2/2] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-24 15:37               ` Aaron Lu
@ 2015-03-24 22:10                 ` Bjorn Helgaas
  0 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2015-03-24 22:10 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Tue, Mar 24, 2015 at 11:37:03PM +0800, Aaron Lu wrote:
> On 03/24/2015 10:08 PM, Bjorn Helgaas wrote:
> > On Tue, Mar 24, 2015 at 05:04:58PM +0800, Aaron Lu wrote:
> >> @@ -575,6 +637,9 @@ static void pci_acpi_setup(struct device *dev)
> >>  	if (!adev)
> >>  		return;
> >>  
> >> +	if (pci_dev->pm_cap)
> >> +		pci_acpi_delay_optimize(pci_dev, adev->handle);
> > 
> > Is the "pm_cap" test really necessary?  If we do it this way, we then have
> > to convince ourselves that pdev->d3cold_delay and pdev->d3_delay are only
> > needed when pdev has a pm_cap.
> > 
> > If we *always* fill in the delay values, it's possible they won't be used,
> > but we don't have to prove any connection between them and a pm_cap, so
> > the code is easier to analyze.
> 
> I remembered why I did the pm_cap test: the d3cold_delay and d3_delay is
> only filled when pm_cap is set in pci_pm_init - if the device doesn't
> have PCI_CAP_ID_PM set, its pm_cap will be 0 and d3cold_delay, d3_delay
> will not be assigned.

Yes, that's true, so I can see why you'd test pm_cap here, too.  But I
don't think it's necessary to propagate that connection here, so I'd omit
the test.

Bjorn

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 0/3] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-09  7:46 [PATCH] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
  2015-03-09 14:33 ` Rafael J. Wysocki
@ 2015-03-25  6:30 ` Aaron Lu
  2015-03-25  6:31   ` [PATCH v3 1/3] PCI: rename _DSM UUID array Aaron Lu
                     ` (4 more replies)
  1 sibling, 5 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-25  6:30 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas; +Cc: ACPI Devel Mailing List, Linux PCI

Here is v3, changes from v2 are:
- Patch 1/3: add Rafael's acked-by tag;
- Patch 2/3: newly introduced, to rename find_pci_host_bridge and export it
- Patch 3/3: address Bjorn's comments for v2

Thanks a lot for your suggestions and review!

Regards,
Aaron

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 1/3] PCI: rename _DSM UUID array
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
@ 2015-03-25  6:31   ` Aaron Lu
  2015-03-25  6:32   ` [PATCH v3 2/3] PCI: rename find_pci_host_bridge and export it Aaron Lu
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-25  6:31 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas; +Cc: ACPI Devel Mailing List, Linux PCI

According to the PCI firmware spec, there is only one assigned UUID used
for PCI system so rename the device_label_dsm_uuid to something more
common as: pci_acpi_dsm_uuid and put it in drivers/pci/pci-acpi.c. Make
that UUID array extern in the pci-acpi.h so that other code can also
make use of it.

This patch shouldn't bring any functional change.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-acpi.c   |  9 +++++++++
 drivers/pci/pci-label.c  | 11 ++---------
 include/linux/pci-acpi.h |  3 +++
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 489063987325..e0afc94aca01 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -18,6 +18,15 @@
 #include <linux/pm_qos.h>
 #include "pci.h"
 
+/*
+ * The UUID is defined in the PCI firmware specification available here:
+ * https://www.pcisig.com/members/downloads/pcifw_r3_1_13Dec10.pdf
+ */
+const u8 pci_acpi_dsm_uuid[] = {
+	0xd0, 0x37, 0xc9, 0xe5, 0x53, 0x35, 0x7a, 0x4d,
+	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
+};
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 2ab1b47c7651..024b5c179348 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -31,8 +31,6 @@
 #include <linux/pci-acpi.h>
 #include "pci.h"
 
-#define	DEVICE_LABEL_DSM	0x07
-
 #ifdef CONFIG_DMI
 enum smbios_attr_enum {
 	SMBIOS_ATTR_NONE = 0,
@@ -148,11 +146,6 @@ static inline void pci_remove_smbiosname_file(struct pci_dev *pdev)
 #endif
 
 #ifdef CONFIG_ACPI
-static const char device_label_dsm_uuid[] = {
-	0xD0, 0x37, 0xC9, 0xE5, 0x53, 0x35, 0x7A, 0x4D,
-	0x91, 0x17, 0xEA, 0x4D, 0x19, 0xC3, 0x43, 0x4D
-};
-
 enum acpi_attr_enum {
 	ACPI_ATTR_LABEL_SHOW,
 	ACPI_ATTR_INDEX_SHOW,
@@ -179,7 +172,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 	if (!handle)
 		return -1;
 
-	obj = acpi_evaluate_dsm(handle, device_label_dsm_uuid, 0x2,
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				DEVICE_LABEL_DSM, NULL);
 	if (!obj)
 		return -1;
@@ -219,7 +212,7 @@ static bool device_has_dsm(struct device *dev)
 	if (!handle)
 		return false;
 
-	return !!acpi_check_dsm(handle, device_label_dsm_uuid, 0x2,
+	return !!acpi_check_dsm(handle, pci_acpi_dsm_uuid, 0x2,
 				1 << DEVICE_LABEL_DSM);
 }
 
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 24c7728ca681..3801c704a945 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -77,6 +77,9 @@ static inline void acpiphp_remove_slots(struct pci_bus *bus) { }
 static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 #endif
 
+extern const u8 pci_acpi_dsm_uuid[];
+#define DEVICE_LABEL_DSM	0x07
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v3 2/3] PCI: rename find_pci_host_bridge and export it
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
  2015-03-25  6:31   ` [PATCH v3 1/3] PCI: rename _DSM UUID array Aaron Lu
@ 2015-03-25  6:32   ` Aaron Lu
  2015-03-25  6:37   ` [PATCH v3 3/3] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-25  6:32 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas; +Cc: ACPI Devel Mailing List, Linux PCI

The find_pci_host_bridge function can be useful to other PCI code so
export it. In the meantime, change its name to more like other interfaces
as pci_find_host_bridge.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/host-bridge.c | 6 +++---
 drivers/pci/pci.h         | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 3e5bbf9e8889..5f4a2e04c8d7 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -16,7 +16,7 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
+struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus)
 {
 	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
@@ -48,7 +48,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
 			     struct resource *res)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
+	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
 	struct resource_entry *window;
 	resource_size_t offset = 0;
 
@@ -73,7 +73,7 @@ static bool region_contains(struct pci_bus_region *region1,
 void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 			     struct pci_bus_region *region)
 {
-	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
+	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
 	struct resource_entry *window;
 	resource_size_t offset = 0;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4091f82239cd..d72f849174a4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -321,4 +321,6 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus);
+
 #endif /* DRIVERS_PCI_H */
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v3 3/3] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
  2015-03-25  6:31   ` [PATCH v3 1/3] PCI: rename _DSM UUID array Aaron Lu
  2015-03-25  6:32   ` [PATCH v3 2/3] PCI: rename find_pci_host_bridge and export it Aaron Lu
@ 2015-03-25  6:37   ` Aaron Lu
  2015-03-29 14:17   ` [PATCH v3 0/3] " Aaron Lu
  2015-04-08 21:32   ` Bjorn Helgaas
  4 siblings, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-25  6:37 UTC (permalink / raw)
  To: Rafael J. Wysocki, Bjorn Helgaas; +Cc: ACPI Devel Mailing List, Linux PCI

An ECN meant to specify possible delay optimization is available on
the PCI website:
https://www.pcisig.com/specifications/conventional/pci_firmware/ECN_fw_latency_optimization_final.pdf
where it has defined two functions for an UUID specified _DSM:
Function 8: If system firmware assumes the responsibility of post
Conventional Reset delay (and informs the Operating System via this DSM
function) on Sx Resume (such as boot from ACPI S5, or resume from ACPI
S4 or S3 states), the Operating System may assume sufficient time has
elapsed since the end of reset, and devices within the PCI subsystem are
ready for Configuration Access.
If the system firmware supports runtime power gating on any of the
device within PCI subsystem covered by this DSM function, the system
firmware is responsible for covering the necessary post power-on reset
delay.

Function 9: Specify various smaller delay values than required by the
SPEC for individual PCI devices like shorter delay values after
conventional reset, D3hot to D0 transition, functional level reset, etc.

This patch adds support for function 8 and part of function 9. For
function 8, the patch will check if the required _DSM function satisfies
the requirement and if so, the d3cold_delay of all PCI devices will be
updated to zero. For function 9, the values affecting delays after
conventional reset and D3hot->D0 are examined and the per PCI device's
d3cold_delay and d3_delay are updated if the _DSM's return value is
smaller than what the SPEC requires. Function 9's value takes precedence
over function 8.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/pci/pci-acpi.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |  2 ++
 include/linux/pci.h      |  1 +
 3 files changed, 77 insertions(+)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index e0afc94aca01..0fea8179857d 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -537,11 +537,32 @@ static struct pci_platform_pm_ops acpi_pci_platform_pm = {
 
 void acpi_pci_add_bus(struct pci_bus *bus)
 {
+	union acpi_object *obj;
+	struct pci_host_bridge *bridge;
+
 	if (acpi_pci_disabled || !bus->bridge)
 		return;
 
 	acpi_pci_slot_enumerate(bus);
 	acpiphp_enumerate_slots(bus);
+
+	/*
+	 * For a host bridge, check its _DSM for function 8 and if
+	 * that is available, mark it in pci_host_bridge.
+	 */
+	if (!pci_is_root_bus(bus))
+		return;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 3,
+				RESET_DELAY_DSM, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 1) {
+		bridge = pci_find_host_bridge(bus);
+		bridge->ignore_reset_delay = 1;
+	}
+	ACPI_FREE(obj);
 }
 
 void acpi_pci_remove_bus(struct pci_bus *bus)
@@ -567,6 +588,57 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev)
 				      check_children);
 }
 
+/**
+ * pci_acpi_delay_optimize - optimize PCI D3 and D3cold delay from ACPI
+ * @pdev: the PCI device whose delay is to be updated
+ * @adev: the companion ACPI device of this PCI device
+ *
+ * Update the d3_delay and d3cold_delay of a PCI device from the ACPI _DSM
+ * control method of either its own or PCI host bridge.
+ *
+ * The UUID of the _DSM control method, together with other information like
+ * which delay values can be optimized, etc. is defined in a ECN available on
+ * PCIsig.com titled as: ACPI additions for FW latency optimizations.
+ * Function 9 of the ACPI _DSM control method, if available for a specific PCI
+ * device, provides various possible delay values that are less than what the
+ * SPEC requires. Here, we only deal with d3_delay and d3cold_delay. Others
+ * can be added later.
+ * Function 8 of the ACPI _DSM control method, if available for the PCI host
+ * bridge, means devices in the entire PCI hierachy do not need the reset delay
+ * when leaving from D3cold state. However, if function 9 also specify the reset
+ * delay value, it will take precedence over function 8.
+ */
+static void pci_acpi_delay_optimize(struct pci_dev *pdev,
+				    acpi_handle handle)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
+	int value;
+	union acpi_object *obj, *elements;
+
+	if (bridge->ignore_reset_delay)
+		pdev->d3cold_delay = 0;
+
+	obj = acpi_evaluate_dsm(handle, pci_acpi_dsm_uuid, 3,
+				FUNCTION_DELAY_DSM, NULL);
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+		elements = obj->package.elements;
+		if (elements[0].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[0].integer.value / 1000;
+			if (value < PCI_PM_D3COLD_WAIT)
+				pdev->d3cold_delay = value;
+		}
+		if (elements[3].type == ACPI_TYPE_INTEGER) {
+			value = (int)elements[3].integer.value / 1000;
+			if (value < PCI_PM_D3_WAIT)
+				pdev->d3_delay = value;
+		}
+	}
+	ACPI_FREE(obj);
+}
+
 static void pci_acpi_setup(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -575,6 +647,8 @@ static void pci_acpi_setup(struct device *dev)
 	if (!adev)
 		return;
 
+	pci_acpi_delay_optimize(pci_dev, adev->handle);
+
 	pci_acpi_add_pm_notifier(adev, pci_dev);
 	if (!adev->wakeup.flags.valid)
 		return;
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 3801c704a945..a965efa52152 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -79,6 +79,8 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 
 extern const u8 pci_acpi_dsm_uuid[];
 #define DEVICE_LABEL_DSM	0x07
+#define RESET_DELAY_DSM		0x08
+#define FUNCTION_DELAY_DSM	0x09
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a379513bddef..c87cb563a90d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -406,6 +406,7 @@ struct pci_host_bridge {
 	struct list_head windows;	/* resource_entry */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
+	unsigned int ignore_reset_delay:1;
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v3 0/3] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
                     ` (2 preceding siblings ...)
  2015-03-25  6:37   ` [PATCH v3 3/3] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
@ 2015-03-29 14:17   ` Aaron Lu
  2015-04-08 21:32   ` Bjorn Helgaas
  4 siblings, 0 replies; 29+ messages in thread
From: Aaron Lu @ 2015-03-29 14:17 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

Hi Bjorn,

I suppose this will go through the PCI tree?
May I know if this will be queued for v4.1?
Thanks.

-Aaron

On Wed, Mar 25, 2015 at 02:30:41PM +0800, Aaron Lu wrote:
> Here is v3, changes from v2 are:
> - Patch 1/3: add Rafael's acked-by tag;
> - Patch 2/3: newly introduced, to rename find_pci_host_bridge and export it
> - Patch 3/3: address Bjorn's comments for v2
> 
> Thanks a lot for your suggestions and review!
> 
> Regards,
> Aaron

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v3 0/3] PCI / ACPI: PCI delay optimization from ACPI
  2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
                     ` (3 preceding siblings ...)
  2015-03-29 14:17   ` [PATCH v3 0/3] " Aaron Lu
@ 2015-04-08 21:32   ` Bjorn Helgaas
  4 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2015-04-08 21:32 UTC (permalink / raw)
  To: Aaron Lu; +Cc: Rafael J. Wysocki, ACPI Devel Mailing List, Linux PCI

On Wed, Mar 25, 2015 at 02:30:41PM +0800, Aaron Lu wrote:
> Here is v3, changes from v2 are:
> - Patch 1/3: add Rafael's acked-by tag;
> - Patch 2/3: newly introduced, to rename find_pci_host_bridge and export it
> - Patch 3/3: address Bjorn's comments for v2

Applied to pci/misc for v4.1, thanks!

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2015-04-08 21:32 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-09  7:46 [PATCH] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
2015-03-09 14:33 ` Rafael J. Wysocki
2015-03-10  6:47   ` Aaron Lu
2015-03-10  6:48   ` [PATCH update] " Aaron Lu
2015-03-20  6:14     ` Aaron Lu
2015-03-20 12:39       ` Rafael J. Wysocki
2015-03-20 21:03     ` Bjorn Helgaas
2015-03-23  5:35       ` Aaron Lu
2015-03-23  9:15         ` Aaron Lu
2015-03-23  9:16       ` [PATCH 1/2] PCI: rename dsm uuid for PCI Aaron Lu
2015-03-24  0:24         ` Rafael J. Wysocki
2015-03-24  0:35           ` Aaron Lu
2015-03-24  1:03             ` Rafael J. Wysocki
2015-03-24  9:04               ` [PATCH v2 1/2] PCI: rename _DSM UUID array Aaron Lu
2015-03-24 21:57                 ` Rafael J. Wysocki
2015-03-23  9:17       ` [PATCH 2/2] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
2015-03-23 15:09         ` Bjorn Helgaas
2015-03-24  9:04           ` [PATCH v2 " Aaron Lu
2015-03-24 14:08             ` Bjorn Helgaas
2015-03-24 15:16               ` Aaron Lu
2015-03-24 22:09                 ` Bjorn Helgaas
2015-03-24 15:37               ` Aaron Lu
2015-03-24 22:10                 ` Bjorn Helgaas
2015-03-25  6:30 ` [PATCH v3 0/3] " Aaron Lu
2015-03-25  6:31   ` [PATCH v3 1/3] PCI: rename _DSM UUID array Aaron Lu
2015-03-25  6:32   ` [PATCH v3 2/3] PCI: rename find_pci_host_bridge and export it Aaron Lu
2015-03-25  6:37   ` [PATCH v3 3/3] PCI / ACPI: PCI delay optimization from ACPI Aaron Lu
2015-03-29 14:17   ` [PATCH v3 0/3] " Aaron Lu
2015-04-08 21:32   ` Bjorn Helgaas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.