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@xxxxxxxxx> >> --- >> 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
