thr3ads.net - Nouveau - [Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk [Feb 2022]

If this information is useful, please help other people find it:
Share via:

Mario Limonciello

2022-Feb-11 19:32 UTC

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

`pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
controller to indicate that D3 is possible.  As this is used solely
for older Apple systems, move it into a quirk that enumerates across
all Intel TBT controllers.

Suggested-by: Mika Westerberg <mika.westerberg at linux.intel.com>
Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
---
 drivers/pci/pci.c    | 12 +++++-----
 drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..5002e214c9a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct pci_dev
*dev)
 	if (pci_use_mid_pm())
 		return false;
 
-	return acpi_pci_bridge_d3(dev);
+	if (acpi_pci_bridge_d3(dev))
+		return true;
+
+	if (device_property_read_bool(&dev->dev,
"HotPlugSupportInD3"))
+		return true;
+
+	return false;
 }
 
 /**
@@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		if (pci_bridge_d3_force)
 			return true;
 
-		/* Even the oldest 2010 Thunderbolt controller supports D3. */
-		if (bridge->is_thunderbolt)
-			return true;
-
 		/* Platform might know better if the bridge supports D3 */
 		if (platform_pci_bridge_d3(bridge))
 			return true;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6d3c88edde00..aaf098ca7d54 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
 			       quirk_apple_poweroff_thunderbolt);
 #endif
 
+/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers, but
don't specify
+ * it in the ACPI tables
+ */
+static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
+{
+	struct property_entry properties[] = {
+		PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
+		{},
+	};
+
+	if (!x86_apple_machine)
+		return;
+
+	if (device_create_managed_software_node(&dev->dev, properties, NULL))
+		pci_warn(dev, "could not add HotPlugSupportInD3 property");
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI,
+			quirk_apple_d3_thunderbolt);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE,
+			quirk_apple_d3_thunderbolt);
+
 /*
  * Following are device-specific reset methods which can be used to
  * reset a single function if other methods (e.g. FLR, PM D0->D3) are
-- 
2.34.1

Bjorn Helgaas

2022-Feb-11 21:35 UTC

head link

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello
wrote:> `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
> controller to indicate that D3 is possible.  As this is used solely
> for older Apple systems, move it into a quirk that enumerates across
> all Intel TBT controllers.
> 
> Suggested-by: Mika Westerberg <mika.westerberg at linux.intel.com>
> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> ---
>  drivers/pci/pci.c    | 12 +++++-----
>  drivers/pci/quirks.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..5002e214c9a6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1064,7 +1064,13 @@ static inline bool platform_pci_bridge_d3(struct
pci_dev *dev)
>  	if (pci_use_mid_pm())
>  		return false;
>  
> -	return acpi_pci_bridge_d3(dev);
> +	if (acpi_pci_bridge_d3(dev))
> +		return true;
> +
> +	if (device_property_read_bool(&dev->dev,
"HotPlugSupportInD3"))
> +		return true;
Why do we need this?  acpi_pci_bridge_d3() already looks for
"HotPlugSupportInD3".
> +	return false;
>  }
>  
>  /**
> @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  		if (pci_bridge_d3_force)
>  			return true;
>  
> -		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> -		if (bridge->is_thunderbolt)
> -			return true;
> -
>  		/* Platform might know better if the bridge supports D3 */
>  		if (platform_pci_bridge_d3(bridge))
>  			return true;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 6d3c88edde00..aaf098ca7d54 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3756,6 +3756,59 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
>  			       quirk_apple_poweroff_thunderbolt);
>  #endif
>  
> +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers,
but don't specify
> + * it in the ACPI tables
Wrap to fit in 80 columns like the rest of the file.  Also use the:

  /*
   * comment ...
   */

style if it's more than one line.

I don't think "as old as 2010" is helpful here -- I assume 2010 is
there because there *were* no Thunderbolt controllers before 2010, but
the code doesn't check any dates, so we basically assume all Apple
machines of any age with the listed controllers can do this.
> + */
> +static void quirk_apple_d3_thunderbolt(struct pci_dev *dev)
> +{
> +	struct property_entry properties[] = {
> +		PROPERTY_ENTRY_BOOL("HotPlugSupportInD3"),
> +		{},
> +	};
> +
> +	if (!x86_apple_machine)
> +		return;
The current code doesn't check x86_apple_machine, so this needs some
justification.  How do I know this works the same as before?
> +
> +	if (device_create_managed_software_node(&dev->dev, properties,
NULL))
> +		pci_warn(dev, "could not add HotPlugSupportInD3 property");
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_LIGHT_RIDGE,
> +			quirk_apple_d3_thunderbolt);
The current code assumes *all* Thunderbolt controllers support D3, so
it would assume a controller released next year would support D3, but
this code would assume the opposite.  Are we supposed to add
everything to this list, or do newer machines supply
HotPlugSupportInD3, or ...?

How did you derive this list?  (Question for the commit log and/or
comments here.)
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_EAGLE_RIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_LIGHT_PEAK,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI,
> +			quirk_apple_d3_thunderbolt);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE,
> +			quirk_apple_d3_thunderbolt);
> +
>  /*
>   * Following are device-specific reset methods which can be used to
>   * reset a single function if other methods (e.g. FLR, PM D0->D3) are
> -- 
> 2.34.1
>

Lukas Wunner

2022-Feb-13 09:19 UTC

head link

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

On Fri, Feb 11, 2022 at 01:32:41PM -0600, Mario Limonciello
wrote:> `pci_bridge_d3_possible` currently checks explicitly for a Thunderbolt
> controller to indicate that D3 is possible.  As this is used solely
> for older Apple systems, move it into a quirk that enumerates across
> all Intel TBT controllers.
I'm not so sure if it is only needed on Apple systems.

> @@ -2954,10 +2960,6 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
>  		if (pci_bridge_d3_force)
>  			return true;
>  
> -		/* Even the oldest 2010 Thunderbolt controller supports D3. */
> -		if (bridge->is_thunderbolt)
> -			return true;
> -
>  		/* Platform might know better if the bridge supports D3 */
>  		if (platform_pci_bridge_d3(bridge))
>  			return true;
The fact that Thunderbolt PCIe ports support D3 is a property of those
devices.  It's not a property of the platform or a quirk of a particular
vendor.

Hence in my view the current location of the check (pci_bridge_d3_possible())
makes sense wheras the location you're moving it to does not.

> +/* Apple machines as old as 2010 can do D3 with Thunderbolt controllers,
but don't specify
> + * it in the ACPI tables
> + */
Apple started shipping Thunderbolt in 2011.
Intel brought the first chips to market in 2010.

The date is meaningful at the code's current location in
pci_bridge_d3_possible() because a few lines further down
there's a 2015 BIOS cut-off date.

Microsoft came up with an ACPI property that BIOS vendors may set
so that Windows knows it may put a Thunderbolt controller into D3cold.
I'm not even sure if that property was ever officially adopted by the
ACPI spec or if it's just a Microsoft-defined "standard".

Apple had been using its own scheme to put Thunderbolt controllers
into D3cold when nothing is plugged in, about a decade before Microsoft
defined the ACPI property.

I'm not sure if other vendors came up with their own schemes to
power-manage Thunderbolt.  We may regress those with the present
patch.

Thanks,

Lukas

Nouveau - Feb 2022 - [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk

[Nouveau] [PATCH v3 03/12] PCI: Move check for old Apple Thunderbolt controllers into a quirk