[PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Manivannan Sadhasivam]

Hi,

This series allows D3Hot for PCI bridges in Devicetree based platforms.
Even though most of the bridges in Devicetree platforms support D3Hot, PCI
core will allow D3Hot only when one of the following conditions are met:

1. Platform is ACPI based
2. Thunderbolt controller is used
3. pcie_port_pm=force passed in cmdline

While options 1 and 2 do not apply to most of the DT based platforms,
option 3 will make the life harder for distro maintainers.

Initially, I tried to fix this issue by using a Devicetree property [1], but
that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
default for all the Devicetree based platforms.

During the review of v3 series, Bjorn noted several shortcomings of the
pci_bridge_d3_possible() API [2] and I tried to address them in this series as
well.

But please note that the patches 2 and 3 needs closer review from ACPI and x86
folks since I've splitted the D3Hot and D3Cold handling based on my little
understanding of the code.

Testing
=======

This series is tested on SM8450 based development board on top of [3].

- Mani

[1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
[2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
[3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/

Changes in v4:
- Added pci_bridge_d3_possible() rework based on comments from Bjorn
- Got rid of the DT property and allowed D3Hot by default on all DT platforms

Changes in v3:
- Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
- Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org

Changes in v2:
- Switched to DT based approach as suggested by Lukas.
- Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Manivannan Sadhasivam (4):
      PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
      PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
      PCI: Decouple D3Hot and D3Cold handling for bridges
      PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

 drivers/pci/bus.c          |  2 +-
 drivers/pci/pci-acpi.c     |  9 ++---
 drivers/pci/pci-sysfs.c    |  2 +-
 drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
 drivers/pci/pci.h          | 12 ++++---
 drivers/pci/pcie/portdrv.c | 16 ++++-----
 drivers/pci/remove.c       |  2 +-
 include/linux/pci.h        |  3 +-
 8 files changed, 89 insertions(+), 47 deletions(-)
---
base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
change-id: 20240320-pci-bridge-d3-092e2beac438

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v4 1/4] PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility

[Manivannan Sadhasivam]

PCI core is already caching the value of pci_bridge_d3_possible() in
pci_dev::bridge_d3 during pci_pm_init(). Since the value is not going to
change, let's make use of the cached value.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pcie/portdrv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..1f02e5d7b2e9 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (pci_bridge_d3_possible(dev)) {
+	if (dev->bridge_d3) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);

-- 
2.25.1

[PATCH v4 2/4] PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()

[Manivannan Sadhasivam]

As per the 'PCI Bus Power Management Interface Specification' v1.2, all
devices should support D3 states (both D3Hot and D3Cold). So the term
'possible' doesn't make sense for the bridge devices, since D3 states
should be possible as per the design. But due to various circumstances,
D3 states might not be allowed for the bridges.

In those cases, the API should be called 'pci_bridge_d3_allowed()' to
reflect the actual behavior. So let's rename it.

This also warrants renaming the variable 'bridge_d3' in 'struct pci_dev'
to 'bridge_d3_allowed' for the reason cited above.

No functional change.

Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Closes: https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci-acpi.c     |  8 ++++----
 drivers/pci/pci.c          | 18 +++++++++---------
 drivers/pci/pci.h          |  4 ++--
 drivers/pci/pcie/portdrv.c | 14 +++++++-------
 include/linux/pci.h        |  2 +-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 004575091596..0f260cdc4592 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1429,12 +1429,12 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
 
 	device_set_wakeup_capable(dev, true);
 	/*
-	 * For bridges that can do D3 we enable wake automatically (as
-	 * we do for the power management itself in that case). The
+	 * For bridges that are allowed to do D3, we enable wake automatically
+	 * (as we do for the power management itself in that case). The
 	 * reason is that the bridge may have additional methods such as
 	 * _DSW that need to be called.
 	 */
-	if (pci_dev->bridge_d3)
+	if (pci_dev->bridge_d3_allowed)
 		device_wakeup_enable(dev);
 
 	acpi_pci_wakeup(pci_dev, false);
@@ -1452,7 +1452,7 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
 	pci_acpi_remove_pm_notifier(adev);
 	if (adev->wakeup.flags.valid) {
 		acpi_device_power_remove_dependent(adev, dev);
-		if (pci_dev->bridge_d3)
+		if (pci_dev->bridge_d3_allowed)
 			device_wakeup_disable(dev);
 
 		device_set_wakeup_capable(dev, false);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5f243dd4288..0edc4e448c2d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2967,13 +2967,13 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
 };
 
 /**
- * pci_bridge_d3_possible - Is it possible to put the bridge into D3
+ * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
  * @bridge: Bridge to check
  *
- * This function checks if it is possible to move the bridge to D3.
+ * This function checks if the bridge is allowed to move to D3.
  * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
  */
-bool pci_bridge_d3_possible(struct pci_dev *bridge)
+bool pci_bridge_d3_allowed(struct pci_dev *bridge)
 {
 	if (!pci_is_pcie(bridge))
 		return false;
@@ -3060,14 +3060,14 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_possible(bridge))
+	if (!bridge || !pci_bridge_d3_allowed(bridge))
 		return;
 
 	/*
 	 * If D3 is currently allowed for the bridge, removing one of its
 	 * children won't change that.
 	 */
-	if (remove && bridge->bridge_d3)
+	if (remove && bridge->bridge_d3_allowed)
 		return;
 
 	/*
@@ -3087,12 +3087,12 @@ void pci_bridge_d3_update(struct pci_dev *dev)
 	 * so we need to go through all children to find out if one of them
 	 * continues to block D3.
 	 */
-	if (d3cold_ok && !bridge->bridge_d3)
+	if (d3cold_ok && !bridge->bridge_d3_allowed)
 		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
 			     &d3cold_ok);
 
-	if (bridge->bridge_d3 != d3cold_ok) {
-		bridge->bridge_d3 = d3cold_ok;
+	if (bridge->bridge_d3_allowed != d3cold_ok) {
+		bridge->bridge_d3_allowed = d3cold_ok;
 		/* Propagate change to upstream bridges */
 		pci_bridge_d3_update(bridge);
 	}
@@ -3167,7 +3167,7 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
-	dev->bridge_d3 = pci_bridge_d3_possible(dev);
+	dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..53ca75639201 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,7 +92,7 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_msi_init(struct pci_dev *dev);
 void pci_msix_init(struct pci_dev *dev);
-bool pci_bridge_d3_possible(struct pci_dev *dev);
+bool pci_bridge_d3_allowed(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
@@ -113,7 +113,7 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
 	 * Currently we allow normal PCI devices and PCI bridges transition
 	 * into D3 if their bridge_d3 is set.
 	 */
-	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3;
+	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3_allowed;
 }
 
 static inline bool pcie_downstream_port(const struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 1f02e5d7b2e9..8401a0f7b394 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -632,7 +632,7 @@ __setup("pcie_ports=", pcie_port_setup);
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {
-	if (!to_pci_dev(dev)->bridge_d3)
+	if (!to_pci_dev(dev)->bridge_d3_allowed)
 		return -EBUSY;
 
 	return pcie_port_device_runtime_suspend(dev);
@@ -641,11 +641,11 @@ static int pcie_port_runtime_suspend(struct device *dev)
 static int pcie_port_runtime_idle(struct device *dev)
 {
 	/*
-	 * Assume the PCI core has set bridge_d3 whenever it thinks the port
-	 * should be good to go to D3.  Everything else, including moving
+	 * Assume the PCI core has set bridge_d3_allowed whenever it thinks the
+	 * port should be good to go to D3.  Everything else, including moving
 	 * the port to D3, is handled by the PCI core.
 	 */
-	return to_pci_dev(dev)->bridge_d3 ? 0 : -EBUSY;
+	return to_pci_dev(dev)->bridge_d3_allowed ? 0 : -EBUSY;
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (dev->bridge_d3) {
+	if (dev->bridge_d3_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 16493426a04f..2a48c88512e1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -375,7 +375,7 @@ struct pci_dev {
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
-	unsigned int	bridge_d3:1;	/* Allow D3 for bridge */
+	unsigned int	bridge_d3_allowed:1;	/* Allow D3 for bridge */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */

-- 
2.25.1

[PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

Currently, there is no proper distinction between D3Hot and D3Cold while
handling the power management for PCI bridges. For instance,
pci_bridge_d3_allowed() API decides whether it is allowed to put the
bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
is allowed in a scenario. This often leads to confusion and may be prone
to errors.

So let's split the D3Hot and D3Cold handling where possible. The current
pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
and pci_bridge_d3cold_allowed() APIs and used in relevant places.

Also, pci_bridge_d3_update() API is now renamed to
pci_bridge_d3cold_update() since it was only used to check the possibility
of D3Cold.

Note that it is assumed that only D3Hot needs to be checked while
transitioning the bridge during runtime PM and D3Cold in other places. In
the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
allowed for the bridge.

Still, there are places where just 'd3' is used opaquely, but those are
hard to distinguish, hence left for future cleanups.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/bus.c          |  2 +-
 drivers/pci/pci-acpi.c     |  5 +--
 drivers/pci/pci-sysfs.c    |  2 +-
 drivers/pci/pci.c          | 78 ++++++++++++++++++++++++++++++----------------
 drivers/pci/pci.h          | 12 ++++---
 drivers/pci/pcie/portdrv.c | 16 +++++-----
 drivers/pci/remove.c       |  2 +-
 include/linux/pci.h        |  3 +-
 8 files changed, 75 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 826b5016a101..cb1a1aaefa90 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 		of_pci_make_dev_node(dev);
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
-	pci_bridge_d3_update(dev);
+	pci_bridge_d3cold_update(dev);
 
 	dev->match_driver = !dn || of_device_is_available(dn);
 	retval = device_attach(&dev->dev);
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0f260cdc4592..aaf5a68e7984 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
 	 * reason is that the bridge may have additional methods such as
 	 * _DSW that need to be called.
 	 */
-	if (pci_dev->bridge_d3_allowed)
+	if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
 		device_wakeup_enable(dev);
 
 	acpi_pci_wakeup(pci_dev, false);
@@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
 	pci_acpi_remove_pm_notifier(adev);
 	if (adev->wakeup.flags.valid) {
 		acpi_device_power_remove_dependent(adev, dev);
-		if (pci_dev->bridge_d3_allowed)
+		if (pci_dev->bridge_d3cold_allowed &&
+		    pci_dev->bridge_d3hot_allowed)
 			device_wakeup_disable(dev);
 
 		device_set_wakeup_capable(dev, false);
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 40cfa716392f..45628b0dd116 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
 		return -EINVAL;
 
 	pdev->d3cold_allowed = !!val;
-	pci_bridge_d3_update(pdev);
+	pci_bridge_d3cold_update(pdev);
 
 	pm_runtime_resume(dev);
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0edc4e448c2d..48e2ca0cd8a0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
 }
 EXPORT_SYMBOL_GPL(pci_ats_disabled);
 
-/* Disable bridge_d3 for all PCIe ports */
+/* Disable both D3Hot and D3Cold for all PCIe ports */
 static bool pci_bridge_d3_disable;
-/* Force bridge_d3 for all PCIe ports */
+/* Force both D3Hot and D3Cold for all PCIe ports */
 static bool pci_bridge_d3_force;
 
 static int __init pcie_port_pm_setup(char *str)
@@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
 	{ }
 };
 
-/**
- * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
- * @bridge: Bridge to check
- *
- * This function checks if the bridge is allowed to move to D3.
- * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
+/*
+ * Helper function to check whether it is allowed to put the bridge into D3
+ * states (D3Hot and D3Cold).
  */
-bool pci_bridge_d3_allowed(struct pci_dev *bridge)
+static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
 {
 	if (!pci_is_pcie(bridge))
 		return false;
@@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
 	return false;
 }
 
+/**
+ * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
+ * @bridge: Bridge to check
+ *
+ * This function checks if the bridge is allowed to move to D3Cold.
+ * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
+ * platforms and Thunderbolt.
+ */
+bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
+{
+	return pci_bridge_d3_allowed(bridge, PCI_D3cold);
+}
+
+/**
+ * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Hot
+ * @bridge: Bridge to check
+ *
+ * This function checks if the bridge is allowed to move to D3Hot.
+ * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
+ * platforms and Thunderbolt.
+ */
+bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
+{
+	return pci_bridge_d3_allowed(bridge, PCI_D3hot);
+}
+
 static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 {
 	bool *d3cold_ok = data;
@@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
 }
 
 /*
- * pci_bridge_d3_update - Update bridge D3 capabilities
+ * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
  * @dev: PCI device which is changed
  *
  * Update upstream bridge PM capabilities accordingly depending on if the
  * device PM configuration was changed or the device is being removed.  The
  * change is also propagated upstream.
  */
-void pci_bridge_d3_update(struct pci_dev *dev)
+void pci_bridge_d3cold_update(struct pci_dev *dev)
 {
 	bool remove = !device_is_registered(&dev->dev);
 	struct pci_dev *bridge;
 	bool d3cold_ok = true;
 
 	bridge = pci_upstream_bridge(dev);
-	if (!bridge || !pci_bridge_d3_allowed(bridge))
+	if (!bridge || !pci_bridge_d3cold_allowed(bridge))
 		return;
 
 	/*
-	 * If D3 is currently allowed for the bridge, removing one of its
+	 * If D3Cold is currently allowed for the bridge, removing one of its
 	 * children won't change that.
 	 */
-	if (remove && bridge->bridge_d3_allowed)
+	if (remove && bridge->bridge_d3cold_allowed)
 		return;
 
 	/*
-	 * If D3 is currently allowed for the bridge and a child is added or
-	 * changed, disallowance of D3 can only be caused by that child, so
+	 * If D3Cold is currently allowed for the bridge and a child is added or
+	 * changed, disallowance of D3Cold can only be caused by that child, so
 	 * we only need to check that single device, not any of its siblings.
 	 *
-	 * If D3 is currently not allowed for the bridge, checking the device
-	 * first may allow us to skip checking its siblings.
+	 * If D3Cold is currently not allowed for the bridge, checking the
+	 * device first may allow us to skip checking its siblings.
 	 */
 	if (!remove)
 		pci_dev_check_d3cold(dev, &d3cold_ok);
 
 	/*
-	 * If D3 is currently not allowed for the bridge, this may be caused
+	 * If D3Cold is currently not allowed for the bridge, this may be caused
 	 * either by the device being changed/removed or any of its siblings,
 	 * so we need to go through all children to find out if one of them
-	 * continues to block D3.
+	 * continues to block D3Cold.
 	 */
-	if (d3cold_ok && !bridge->bridge_d3_allowed)
+	if (d3cold_ok && !bridge->bridge_d3cold_allowed)
 		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
 			     &d3cold_ok);
 
-	if (bridge->bridge_d3_allowed != d3cold_ok) {
-		bridge->bridge_d3_allowed = d3cold_ok;
+	if (bridge->bridge_d3cold_allowed != d3cold_ok) {
+		bridge->bridge_d3cold_allowed = d3cold_ok;
 		/* Propagate change to upstream bridges */
-		pci_bridge_d3_update(bridge);
+		pci_bridge_d3cold_update(bridge);
 	}
 }
 
@@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
 {
 	if (dev->no_d3cold) {
 		dev->no_d3cold = false;
-		pci_bridge_d3_update(dev);
+		pci_bridge_d3cold_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_enable);
@@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
 {
 	if (!dev->no_d3cold) {
 		dev->no_d3cold = true;
-		pci_bridge_d3_update(dev);
+		pci_bridge_d3cold_update(dev);
 	}
 }
 EXPORT_SYMBOL_GPL(pci_d3cold_disable);
@@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
 	dev->pm_cap = pm;
 	dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
 	dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
-	dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
+	dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
+	dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
 	dev->d3cold_allowed = true;
 
 	dev->d1_support = false;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 53ca75639201..f819eab793fc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
 void pci_msi_init(struct pci_dev *dev);
 void pci_msix_init(struct pci_dev *dev);
-bool pci_bridge_d3_allowed(struct pci_dev *dev);
-void pci_bridge_d3_update(struct pci_dev *dev);
+bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
+bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
+void pci_bridge_d3cold_update(struct pci_dev *dev);
 int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
@@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
 {
 	/*
 	 * Currently we allow normal PCI devices and PCI bridges transition
-	 * into D3 if their bridge_d3 is set.
+	 * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
+	 * are set.
 	 */
-	return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3_allowed;
+	return !pci_has_subordinate(pci_dev) ||
+	       (pci_dev->bridge_d3cold_allowed &&
+		pci_dev->bridge_d3hot_allowed);
 }
 
 static inline bool pcie_downstream_port(const struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 8401a0f7b394..655754b9f06a 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -632,7 +632,7 @@ __setup("pcie_ports=", pcie_port_setup);
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {
-	if (!to_pci_dev(dev)->bridge_d3_allowed)
+	if (!to_pci_dev(dev)->bridge_d3hot_allowed)
 		return -EBUSY;
 
 	return pcie_port_device_runtime_suspend(dev);
@@ -641,11 +641,11 @@ static int pcie_port_runtime_suspend(struct device *dev)
 static int pcie_port_runtime_idle(struct device *dev)
 {
 	/*
-	 * Assume the PCI core has set bridge_d3_allowed whenever it thinks the
-	 * port should be good to go to D3.  Everything else, including moving
-	 * the port to D3, is handled by the PCI core.
+	 * Assume the PCI core has set bridge_d3hot_allowed whenever it thinks
+	 * the port should be good to go to D3Hot.  Everything else, including
+	 * moving the port to D3Hot, is handled by the PCI core.
 	 */
-	return to_pci_dev(dev)->bridge_d3_allowed ? 0 : -EBUSY;
+	return to_pci_dev(dev)->bridge_d3hot_allowed ? 0 : -EBUSY;
 }
 
 static const struct dev_pm_ops pcie_portdrv_pm_ops = {
@@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 	dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
 					   DPM_FLAG_SMART_SUSPEND);
 
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		/*
 		 * Keep the port resumed 100ms to make sure things like
 		 * config space accesses from userspace (lspci) will not
@@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
 
 static void pcie_portdrv_remove(struct pci_dev *dev)
 {
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
@@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
 
 static void pcie_portdrv_shutdown(struct pci_dev *dev)
 {
-	if (dev->bridge_d3_allowed) {
+	if (dev->bridge_d3hot_allowed) {
 		pm_runtime_forbid(&dev->dev);
 		pm_runtime_get_noresume(&dev->dev);
 		pm_runtime_dont_use_autosuspend(&dev->dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index d749ea8250d6..36d8cb50b582 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -41,7 +41,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
 
 	pci_doe_destroy(dev);
 	pcie_aspm_exit_link_state(dev);
-	pci_bridge_d3_update(dev);
+	pci_bridge_d3cold_update(dev);
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2a48c88512e1..d0947f932b9a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -375,7 +375,8 @@ struct pci_dev {
 	unsigned int	d2_support:1;	/* Low power state D2 is supported */
 	unsigned int	no_d1d2:1;	/* D1 and D2 are forbidden */
 	unsigned int	no_d3cold:1;	/* D3cold is forbidden */
-	unsigned int	bridge_d3_allowed:1;	/* Allow D3 for bridge */
+	unsigned int	bridge_d3cold_allowed:1;	/* Allow D3Cold for bridge */
+	unsigned int	bridge_d3hot_allowed:1;		/* Allow D3Hot for bridge */
 	unsigned int	d3cold_allowed:1;	/* D3cold is allowed by user */
 	unsigned int	mmio_always_on:1;	/* Disallow turning off io/mem
 						   decoding during BAR sizing */

-- 
2.25.1

[PATCH v4 4/4] PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms

[Manivannan Sadhasivam]

Unlike ACPI based platforms, there are no known issues with D3Hot for the
PCI bridges in the Devicetree based platforms. So let's allow the PCI
bridges to go to D3Hot during runtime. It should be noted that the bridges
need to be defined in Devicetree for this to work.

Currently, D3Cold is not allowed since Vcc supply which is required for
transitioning the device to D3Cold is not exposed on all Devicetree based
platforms.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 48e2ca0cd8a0..2fe9defa69e3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2992,6 +2992,18 @@ static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
 		if (pci_bridge_d3_force)
 			return true;
 
+		/*
+		 * Allow D3Hot for all Devicetree based platforms having a
+		 * separate node for the bridge. We don't allow D3Cold for now
+		 * since not all platforms are exposing the Vcc supply in
+		 * Devicetree which is required for transitioning the bridge to
+		 * D3Cold.
+		 *
+		 * NOTE: The bridge is expected to be defined in Devicetree.
+		 */
+		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
+			return true;
+
 		/* Even the oldest 2010 Thunderbolt controller supports D3. */
 		if (bridge->is_thunderbolt)
 			return true;
@@ -3042,7 +3054,7 @@ bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
  *
  * This function checks if the bridge is allowed to move to D3Hot.
  * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
- * platforms and Thunderbolt.
+ * platforms, Thunderbolt and Devicetree based platforms.
  */
 bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
 {

-- 
2.25.1

Re: [PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Manivannan Sadhasivam]

On Tue, Mar 26, 2024 at 04:18:16PM +0530, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series allows D3Hot for PCI bridges in Devicetree based platforms.
> Even though most of the bridges in Devicetree platforms support D3Hot, PCI
> core will allow D3Hot only when one of the following conditions are met:
> 
> 1. Platform is ACPI based
> 2. Thunderbolt controller is used
> 3. pcie_port_pm=force passed in cmdline
> 
> While options 1 and 2 do not apply to most of the DT based platforms,
> option 3 will make the life harder for distro maintainers.
> 
> Initially, I tried to fix this issue by using a Devicetree property [1], but
> that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
> default for all the Devicetree based platforms.
> 
> During the review of v3 series, Bjorn noted several shortcomings of the
> pci_bridge_d3_possible() API [2] and I tried to address them in this series as
> well.
> 
> But please note that the patches 2 and 3 needs closer review from ACPI and x86
> folks since I've splitted the D3Hot and D3Cold handling based on my little
> understanding of the code.
> 
> Testing
> =======
> 
> This series is tested on SM8450 based development board on top of [3].
> 

Bjorn, a gently ping on this series.

- Mani

> - Mani
> 
> [1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
> [2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
> [3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/
> 
> Changes in v4:
> - Added pci_bridge_d3_possible() rework based on comments from Bjorn
> - Got rid of the DT property and allowed D3Hot by default on all DT platforms
> 
> Changes in v3:
> - Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
> - Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org
> 
> Changes in v2:
> - Switched to DT based approach as suggested by Lukas.
> - Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> Manivannan Sadhasivam (4):
>       PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
>       PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
>       PCI: Decouple D3Hot and D3Cold handling for bridges
>       PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms
> 
>  drivers/pci/bus.c          |  2 +-
>  drivers/pci/pci-acpi.c     |  9 ++---
>  drivers/pci/pci-sysfs.c    |  2 +-
>  drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
>  drivers/pci/pci.h          | 12 ++++---
>  drivers/pci/pcie/portdrv.c | 16 ++++-----
>  drivers/pci/remove.c       |  2 +-
>  include/linux/pci.h        |  3 +-
>  8 files changed, 89 insertions(+), 47 deletions(-)
> ---
> base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
> change-id: 20240320-pci-bridge-d3-092e2beac438
> 
> Best regards,
> -- 
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Hsin-Yi Wang]

On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Mar 26, 2024 at 04:18:16PM +0530, Manivannan Sadhasivam wrote:
> > Hi,
> >
> > This series allows D3Hot for PCI bridges in Devicetree based platforms.
> > Even though most of the bridges in Devicetree platforms support D3Hot, PCI
> > core will allow D3Hot only when one of the following conditions are met:
> >
> > 1. Platform is ACPI based
> > 2. Thunderbolt controller is used
> > 3. pcie_port_pm=force passed in cmdline
> >
> > While options 1 and 2 do not apply to most of the DT based platforms,
> > option 3 will make the life harder for distro maintainers.
> >
> > Initially, I tried to fix this issue by using a Devicetree property [1], but
> > that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
> > default for all the Devicetree based platforms.
> >
> > During the review of v3 series, Bjorn noted several shortcomings of the
> > pci_bridge_d3_possible() API [2] and I tried to address them in this series as
> > well.
> >
> > But please note that the patches 2 and 3 needs closer review from ACPI and x86
> > folks since I've splitted the D3Hot and D3Cold handling based on my little
> > understanding of the code.
> >
> > Testing
> > =======
> >
> > This series is tested on SM8450 based development board on top of [3].
> >
>
> Bjorn, a gently ping on this series.
>

Hi, I was also working on a similar patch to add bridge_d3 to arm
platforms until I found this series, which is what we need. Also
kindly ping on this series.

Thanks!

> - Mani
>
> > - Mani
> >
> > [1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
> > [2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
> > [3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/
> >
> > Changes in v4:
> > - Added pci_bridge_d3_possible() rework based on comments from Bjorn
> > - Got rid of the DT property and allowed D3Hot by default on all DT platforms
> >
> > Changes in v3:
> > - Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
> > - Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org
> >
> > Changes in v2:
> > - Switched to DT based approach as suggested by Lukas.
> > - Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > Manivannan Sadhasivam (4):
> >       PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
> >       PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
> >       PCI: Decouple D3Hot and D3Cold handling for bridges
> >       PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms
> >
> >  drivers/pci/bus.c          |  2 +-
> >  drivers/pci/pci-acpi.c     |  9 ++---
> >  drivers/pci/pci-sysfs.c    |  2 +-
> >  drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
> >  drivers/pci/pci.h          | 12 ++++---
> >  drivers/pci/pcie/portdrv.c | 16 ++++-----
> >  drivers/pci/remove.c       |  2 +-
> >  include/linux/pci.h        |  3 +-
> >  8 files changed, 89 insertions(+), 47 deletions(-)
> > ---
> > base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
> > change-id: 20240320-pci-bridge-d3-092e2beac438
> >
> > Best regards,
> > --
> > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Hsin-Yi Wang]

On Fri, Jul 26, 2024 at 4:06 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Mar 26, 2024 at 04:18:16PM +0530, Manivannan Sadhasivam wrote:
> > > Hi,
> > >
> > > This series allows D3Hot for PCI bridges in Devicetree based platforms.
> > > Even though most of the bridges in Devicetree platforms support D3Hot, PCI
> > > core will allow D3Hot only when one of the following conditions are met:
> > >
> > > 1. Platform is ACPI based
> > > 2. Thunderbolt controller is used
> > > 3. pcie_port_pm=force passed in cmdline
> > >
> > > While options 1 and 2 do not apply to most of the DT based platforms,
> > > option 3 will make the life harder for distro maintainers.
> > >
> > > Initially, I tried to fix this issue by using a Devicetree property [1], but
> > > that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
> > > default for all the Devicetree based platforms.
> > >
> > > During the review of v3 series, Bjorn noted several shortcomings of the
> > > pci_bridge_d3_possible() API [2] and I tried to address them in this series as
> > > well.
> > >
> > > But please note that the patches 2 and 3 needs closer review from ACPI and x86
> > > folks since I've splitted the D3Hot and D3Cold handling based on my little
> > > understanding of the code.
> > >
> > > Testing
> > > =======
> > >
> > > This series is tested on SM8450 based development board on top of [3].
> > >
> >
> > Bjorn, a gently ping on this series.
> >
>
> Hi, I was also working on a similar patch to add bridge_d3 to arm
> platforms until I found this series, which is what we need. Also
> kindly ping on this series.
>
> Thanks!
>

Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>

> > - Mani
> >
> > > - Mani
> > >
> > > [1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
> > > [2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
> > > [3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/
> > >
> > > Changes in v4:
> > > - Added pci_bridge_d3_possible() rework based on comments from Bjorn
> > > - Got rid of the DT property and allowed D3Hot by default on all DT platforms
> > >
> > > Changes in v3:
> > > - Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
> > > - Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org
> > >
> > > Changes in v2:
> > > - Switched to DT based approach as suggested by Lukas.
> > > - Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org
> > >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > > Manivannan Sadhasivam (4):
> > >       PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
> > >       PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
> > >       PCI: Decouple D3Hot and D3Cold handling for bridges
> > >       PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms
> > >
> > >  drivers/pci/bus.c          |  2 +-
> > >  drivers/pci/pci-acpi.c     |  9 ++---
> > >  drivers/pci/pci-sysfs.c    |  2 +-
> > >  drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
> > >  drivers/pci/pci.h          | 12 ++++---
> > >  drivers/pci/pcie/portdrv.c | 16 ++++-----
> > >  drivers/pci/remove.c       |  2 +-
> > >  include/linux/pci.h        |  3 +-
> > >  8 files changed, 89 insertions(+), 47 deletions(-)
> > > ---
> > > base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
> > > change-id: 20240320-pci-bridge-d3-092e2beac438
> > >
> > > Best regards,
> > > --
> > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 0/4] PCI: Allow D3Hot for PCI bridges in Devicetree based platforms

[Manivannan Sadhasivam]

On Fri, Jul 26, 2024 at 04:06:03PM -0700, Hsin-Yi Wang wrote:
> On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Mar 26, 2024 at 04:18:16PM +0530, Manivannan Sadhasivam wrote:
> > > Hi,
> > >
> > > This series allows D3Hot for PCI bridges in Devicetree based platforms.
> > > Even though most of the bridges in Devicetree platforms support D3Hot, PCI
> > > core will allow D3Hot only when one of the following conditions are met:
> > >
> > > 1. Platform is ACPI based
> > > 2. Thunderbolt controller is used
> > > 3. pcie_port_pm=force passed in cmdline
> > >
> > > While options 1 and 2 do not apply to most of the DT based platforms,
> > > option 3 will make the life harder for distro maintainers.
> > >
> > > Initially, I tried to fix this issue by using a Devicetree property [1], but
> > > that was rejected by Bjorn and it was concluded that D3Hot should be allowed by
> > > default for all the Devicetree based platforms.
> > >
> > > During the review of v3 series, Bjorn noted several shortcomings of the
> > > pci_bridge_d3_possible() API [2] and I tried to address them in this series as
> > > well.
> > >
> > > But please note that the patches 2 and 3 needs closer review from ACPI and x86
> > > folks since I've splitted the D3Hot and D3Cold handling based on my little
> > > understanding of the code.
> > >
> > > Testing
> > > =======
> > >
> > > This series is tested on SM8450 based development board on top of [3].
> > >
> >
> > Bjorn, a gently ping on this series.
> >
> 
> Hi, I was also working on a similar patch to add bridge_d3 to arm
> platforms until I found this series, which is what we need. Also
> kindly ping on this series.
> 

Thanks a lot for testing. I will respin the series once 6.11-rc1 is out.

- Mani

> Thanks!
> 
> > - Mani
> >
> > > - Mani
> > >
> > > [1] https://lore.kernel.org/linux-pci/20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org/
> > > [2] https://lore.kernel.org/linux-pci/20240305175107.GA539676@bhelgaas/
> > > [3] https://lore.kernel.org/linux-arm-msm/20240321-pcie-qcom-bridge-dts-v2-0-1eb790c53e43@linaro.org/
> > >
> > > Changes in v4:
> > > - Added pci_bridge_d3_possible() rework based on comments from Bjorn
> > > - Got rid of the DT property and allowed D3Hot by default on all DT platforms
> > >
> > > Changes in v3:
> > > - Fixed kdoc, used of_property_present() and dev_of_node() (Lukas)
> > > - Link to v2: https://lore.kernel.org/r/20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org
> > >
> > > Changes in v2:
> > > - Switched to DT based approach as suggested by Lukas.
> > > - Link to v1: https://lore.kernel.org/r/20240202-pcie-qcom-bridge-v1-0-46d7789836c0@linaro.org
> > >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > > Manivannan Sadhasivam (4):
> > >       PCI/portdrv: Make use of pci_dev::bridge_d3 for checking the D3 possibility
> > >       PCI: Rename pci_bridge_d3_possible() to pci_bridge_d3_allowed()
> > >       PCI: Decouple D3Hot and D3Cold handling for bridges
> > >       PCI: Allow PCI bridges to go to D3Hot on all Devicetree based platforms
> > >
> > >  drivers/pci/bus.c          |  2 +-
> > >  drivers/pci/pci-acpi.c     |  9 ++---
> > >  drivers/pci/pci-sysfs.c    |  2 +-
> > >  drivers/pci/pci.c          | 90 ++++++++++++++++++++++++++++++++--------------
> > >  drivers/pci/pci.h          | 12 ++++---
> > >  drivers/pci/pcie/portdrv.c | 16 ++++-----
> > >  drivers/pci/remove.c       |  2 +-
> > >  include/linux/pci.h        |  3 +-
> > >  8 files changed, 89 insertions(+), 47 deletions(-)
> > > ---
> > > base-commit: 705c1da8fa4816fb0159b5602fef1df5946a3ee2
> > > change-id: 20240320-pci-bridge-d3-092e2beac438
> > >
> > > Best regards,
> > > --
> > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Hsin-Yi Wang]

On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Currently, there is no proper distinction between D3Hot and D3Cold while
> handling the power management for PCI bridges. For instance,
> pci_bridge_d3_allowed() API decides whether it is allowed to put the
> bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> is allowed in a scenario. This often leads to confusion and may be prone
> to errors.
>
> So let's split the D3Hot and D3Cold handling where possible. The current
> pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> and pci_bridge_d3cold_allowed() APIs and used in relevant places.
>
> Also, pci_bridge_d3_update() API is now renamed to
> pci_bridge_d3cold_update() since it was only used to check the possibility
> of D3Cold.
>
> Note that it is assumed that only D3Hot needs to be checked while
> transitioning the bridge during runtime PM and D3Cold in other places. In
> the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> allowed for the bridge.
>
> Still, there are places where just 'd3' is used opaquely, but those are
> hard to distinguish, hence left for future cleanups.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/bus.c          |  2 +-
>  drivers/pci/pci-acpi.c     |  5 +--
>  drivers/pci/pci-sysfs.c    |  2 +-
>  drivers/pci/pci.c          | 78 ++++++++++++++++++++++++++++++----------------
>  drivers/pci/pci.h          | 12 ++++---
>  drivers/pci/pcie/portdrv.c | 16 +++++-----
>  drivers/pci/remove.c       |  2 +-
>  include/linux/pci.h        |  3 +-
>  8 files changed, 75 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 826b5016a101..cb1a1aaefa90 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
>                 of_pci_make_dev_node(dev);
>         pci_create_sysfs_dev_files(dev);
>         pci_proc_attach_device(dev);
> -       pci_bridge_d3_update(dev);
> +       pci_bridge_d3cold_update(dev);
>
>         dev->match_driver = !dn || of_device_is_available(dn);
>         retval = device_attach(&dev->dev);
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 0f260cdc4592..aaf5a68e7984 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
>          * reason is that the bridge may have additional methods such as
>          * _DSW that need to be called.
>          */
> -       if (pci_dev->bridge_d3_allowed)
> +       if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
>                 device_wakeup_enable(dev);
>
>         acpi_pci_wakeup(pci_dev, false);
> @@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
>         pci_acpi_remove_pm_notifier(adev);
>         if (adev->wakeup.flags.valid) {
>                 acpi_device_power_remove_dependent(adev, dev);
> -               if (pci_dev->bridge_d3_allowed)
> +               if (pci_dev->bridge_d3cold_allowed &&
> +                   pci_dev->bridge_d3hot_allowed)
>                         device_wakeup_disable(dev);
>
>                 device_set_wakeup_capable(dev, false);
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 40cfa716392f..45628b0dd116 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
>                 return -EINVAL;
>
>         pdev->d3cold_allowed = !!val;
> -       pci_bridge_d3_update(pdev);
> +       pci_bridge_d3cold_update(pdev);
>
>         pm_runtime_resume(dev);
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0edc4e448c2d..48e2ca0cd8a0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_disabled);
>
> -/* Disable bridge_d3 for all PCIe ports */
> +/* Disable both D3Hot and D3Cold for all PCIe ports */
>  static bool pci_bridge_d3_disable;
> -/* Force bridge_d3 for all PCIe ports */
> +/* Force both D3Hot and D3Cold for all PCIe ports */
>  static bool pci_bridge_d3_force;
>
>  static int __init pcie_port_pm_setup(char *str)
> @@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
>         { }
>  };
>
> -/**
> - * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
> - * @bridge: Bridge to check
> - *
> - * This function checks if the bridge is allowed to move to D3.
> - * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
> +/*
> + * Helper function to check whether it is allowed to put the bridge into D3
> + * states (D3Hot and D3Cold).
>   */
> -bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> +static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
>  {
>         if (!pci_is_pcie(bridge))
>                 return false;
> @@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
>         return false;
>  }
>
> +/**
> + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
> + * @bridge: Bridge to check
> + *
> + * This function checks if the bridge is allowed to move to D3Cold.
> + * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
> + * platforms and Thunderbolt.
> + */
> +bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
> +{
> +       return pci_bridge_d3_allowed(bridge, PCI_D3cold);
> +}
> +
> +/**
> + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Hot

typo? pci_bridge_d3hot_allowed.

> + * @bridge: Bridge to check
> + *
> + * This function checks if the bridge is allowed to move to D3Hot.
> + * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> + * platforms and Thunderbolt.
> + */
> +bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
> +{
> +       return pci_bridge_d3_allowed(bridge, PCI_D3hot);
> +}
> +
>  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>  {
>         bool *d3cold_ok = data;
> @@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>  }
>
>  /*
> - * pci_bridge_d3_update - Update bridge D3 capabilities
> + * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
>   * @dev: PCI device which is changed
>   *
>   * Update upstream bridge PM capabilities accordingly depending on if the
>   * device PM configuration was changed or the device is being removed.  The
>   * change is also propagated upstream.
>   */
> -void pci_bridge_d3_update(struct pci_dev *dev)
> +void pci_bridge_d3cold_update(struct pci_dev *dev)
>  {
>         bool remove = !device_is_registered(&dev->dev);
>         struct pci_dev *bridge;
>         bool d3cold_ok = true;
>
>         bridge = pci_upstream_bridge(dev);
> -       if (!bridge || !pci_bridge_d3_allowed(bridge))
> +       if (!bridge || !pci_bridge_d3cold_allowed(bridge))
>                 return;
>
>         /*
> -        * If D3 is currently allowed for the bridge, removing one of its
> +        * If D3Cold is currently allowed for the bridge, removing one of its
>          * children won't change that.
>          */
> -       if (remove && bridge->bridge_d3_allowed)
> +       if (remove && bridge->bridge_d3cold_allowed)
>                 return;
>
>         /*
> -        * If D3 is currently allowed for the bridge and a child is added or
> -        * changed, disallowance of D3 can only be caused by that child, so
> +        * If D3Cold is currently allowed for the bridge and a child is added or
> +        * changed, disallowance of D3Cold can only be caused by that child, so
>          * we only need to check that single device, not any of its siblings.
>          *
> -        * If D3 is currently not allowed for the bridge, checking the device
> -        * first may allow us to skip checking its siblings.
> +        * If D3Cold is currently not allowed for the bridge, checking the
> +        * device first may allow us to skip checking its siblings.
>          */
>         if (!remove)
>                 pci_dev_check_d3cold(dev, &d3cold_ok);
>
>         /*
> -        * If D3 is currently not allowed for the bridge, this may be caused
> +        * If D3Cold is currently not allowed for the bridge, this may be caused
>          * either by the device being changed/removed or any of its siblings,
>          * so we need to go through all children to find out if one of them
> -        * continues to block D3.
> +        * continues to block D3Cold.
>          */
> -       if (d3cold_ok && !bridge->bridge_d3_allowed)
> +       if (d3cold_ok && !bridge->bridge_d3cold_allowed)
>                 pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
>                              &d3cold_ok);
>
> -       if (bridge->bridge_d3_allowed != d3cold_ok) {
> -               bridge->bridge_d3_allowed = d3cold_ok;
> +       if (bridge->bridge_d3cold_allowed != d3cold_ok) {
> +               bridge->bridge_d3cold_allowed = d3cold_ok;
>                 /* Propagate change to upstream bridges */
> -               pci_bridge_d3_update(bridge);
> +               pci_bridge_d3cold_update(bridge);
>         }
>  }
>
> @@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
>  {
>         if (dev->no_d3cold) {
>                 dev->no_d3cold = false;
> -               pci_bridge_d3_update(dev);
> +               pci_bridge_d3cold_update(dev);
>         }
>  }
>  EXPORT_SYMBOL_GPL(pci_d3cold_enable);
> @@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
>  {
>         if (!dev->no_d3cold) {
>                 dev->no_d3cold = true;
> -               pci_bridge_d3_update(dev);
> +               pci_bridge_d3cold_update(dev);
>         }
>  }
>  EXPORT_SYMBOL_GPL(pci_d3cold_disable);
> @@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
>         dev->pm_cap = pm;
>         dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
>         dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
> -       dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
> +       dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
> +       dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
>         dev->d3cold_allowed = true;
>
>         dev->d1_support = false;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 53ca75639201..f819eab793fc 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
>  void pci_ea_init(struct pci_dev *dev);
>  void pci_msi_init(struct pci_dev *dev);
>  void pci_msix_init(struct pci_dev *dev);
> -bool pci_bridge_d3_allowed(struct pci_dev *dev);
> -void pci_bridge_d3_update(struct pci_dev *dev);
> +bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
> +bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
> +void pci_bridge_d3cold_update(struct pci_dev *dev);
>  int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
>
>  static inline void pci_wakeup_event(struct pci_dev *dev)
> @@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
>  {
>         /*
>          * Currently we allow normal PCI devices and PCI bridges transition
> -        * into D3 if their bridge_d3 is set.
> +        * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
> +        * are set.
>          */

If pm requires both D3hot and D3cold, can we add a flag for DT to
support D3cold? Otherwise during suspend, pcieport still stays at D0.

[   42.202016] mt7921e 0000:01:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
[   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
[   42.238048] mt7921e 0000:01:00.0: PM:
pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
[   42.247083] pcieport 0000:00:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
[   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
[   42.302500] pcieport 0000:00:00.0: PCI PM: Skipped

Thanks.


> -       return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3_allowed;
> +       return !pci_has_subordinate(pci_dev) ||
> +              (pci_dev->bridge_d3cold_allowed &&
> +               pci_dev->bridge_d3hot_allowed);
>  }
>
>  static inline bool pcie_downstream_port(const struct pci_dev *dev)
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 8401a0f7b394..655754b9f06a 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -632,7 +632,7 @@ __setup("pcie_ports=", pcie_port_setup);
>  #ifdef CONFIG_PM
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
> -       if (!to_pci_dev(dev)->bridge_d3_allowed)
> +       if (!to_pci_dev(dev)->bridge_d3hot_allowed)
>                 return -EBUSY;
>
>         return pcie_port_device_runtime_suspend(dev);
> @@ -641,11 +641,11 @@ static int pcie_port_runtime_suspend(struct device *dev)
>  static int pcie_port_runtime_idle(struct device *dev)
>  {
>         /*
> -        * Assume the PCI core has set bridge_d3_allowed whenever it thinks the
> -        * port should be good to go to D3.  Everything else, including moving
> -        * the port to D3, is handled by the PCI core.
> +        * Assume the PCI core has set bridge_d3hot_allowed whenever it thinks
> +        * the port should be good to go to D3Hot.  Everything else, including
> +        * moving the port to D3Hot, is handled by the PCI core.
>          */
> -       return to_pci_dev(dev)->bridge_d3_allowed ? 0 : -EBUSY;
> +       return to_pci_dev(dev)->bridge_d3hot_allowed ? 0 : -EBUSY;
>  }
>
>  static const struct dev_pm_ops pcie_portdrv_pm_ops = {
> @@ -702,7 +702,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>         dev_pm_set_driver_flags(&dev->dev, DPM_FLAG_NO_DIRECT_COMPLETE |
>                                            DPM_FLAG_SMART_SUSPEND);
>
> -       if (dev->bridge_d3_allowed) {
> +       if (dev->bridge_d3hot_allowed) {
>                 /*
>                  * Keep the port resumed 100ms to make sure things like
>                  * config space accesses from userspace (lspci) will not
> @@ -720,7 +720,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>
>  static void pcie_portdrv_remove(struct pci_dev *dev)
>  {
> -       if (dev->bridge_d3_allowed) {
> +       if (dev->bridge_d3hot_allowed) {
>                 pm_runtime_forbid(&dev->dev);
>                 pm_runtime_get_noresume(&dev->dev);
>                 pm_runtime_dont_use_autosuspend(&dev->dev);
> @@ -733,7 +733,7 @@ static void pcie_portdrv_remove(struct pci_dev *dev)
>
>  static void pcie_portdrv_shutdown(struct pci_dev *dev)
>  {
> -       if (dev->bridge_d3_allowed) {
> +       if (dev->bridge_d3hot_allowed) {
>                 pm_runtime_forbid(&dev->dev);
>                 pm_runtime_get_noresume(&dev->dev);
>                 pm_runtime_dont_use_autosuspend(&dev->dev);
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index d749ea8250d6..36d8cb50b582 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -41,7 +41,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>
>         pci_doe_destroy(dev);
>         pcie_aspm_exit_link_state(dev);
> -       pci_bridge_d3_update(dev);
> +       pci_bridge_d3cold_update(dev);
>         pci_free_resources(dev);
>         put_device(&dev->dev);
>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2a48c88512e1..d0947f932b9a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -375,7 +375,8 @@ struct pci_dev {
>         unsigned int    d2_support:1;   /* Low power state D2 is supported */
>         unsigned int    no_d1d2:1;      /* D1 and D2 are forbidden */
>         unsigned int    no_d3cold:1;    /* D3cold is forbidden */
> -       unsigned int    bridge_d3_allowed:1;    /* Allow D3 for bridge */
> +       unsigned int    bridge_d3cold_allowed:1;        /* Allow D3Cold for bridge */
> +       unsigned int    bridge_d3hot_allowed:1;         /* Allow D3Hot for bridge */
>         unsigned int    d3cold_allowed:1;       /* D3cold is allowed by user */
>         unsigned int    mmio_always_on:1;       /* Disallow turning off io/mem
>                                                    decoding during BAR sizing */
>
> --
> 2.25.1
>

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Thu, Aug 01, 2024 at 02:07:41PM -0700, Hsin-Yi Wang wrote:
> On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Currently, there is no proper distinction between D3Hot and D3Cold while
> > handling the power management for PCI bridges. For instance,
> > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > is allowed in a scenario. This often leads to confusion and may be prone
> > to errors.
> >
> > So let's split the D3Hot and D3Cold handling where possible. The current
> > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> >
> > Also, pci_bridge_d3_update() API is now renamed to
> > pci_bridge_d3cold_update() since it was only used to check the possibility
> > of D3Cold.
> >
> > Note that it is assumed that only D3Hot needs to be checked while
> > transitioning the bridge during runtime PM and D3Cold in other places. In
> > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > allowed for the bridge.
> >
> > Still, there are places where just 'd3' is used opaquely, but those are
> > hard to distinguish, hence left for future cleanups.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/bus.c          |  2 +-
> >  drivers/pci/pci-acpi.c     |  5 +--
> >  drivers/pci/pci-sysfs.c    |  2 +-
> >  drivers/pci/pci.c          | 78 ++++++++++++++++++++++++++++++----------------
> >  drivers/pci/pci.h          | 12 ++++---
> >  drivers/pci/pcie/portdrv.c | 16 +++++-----
> >  drivers/pci/remove.c       |  2 +-
> >  include/linux/pci.h        |  3 +-
> >  8 files changed, 75 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 826b5016a101..cb1a1aaefa90 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> >                 of_pci_make_dev_node(dev);
> >         pci_create_sysfs_dev_files(dev);
> >         pci_proc_attach_device(dev);
> > -       pci_bridge_d3_update(dev);
> > +       pci_bridge_d3cold_update(dev);
> >
> >         dev->match_driver = !dn || of_device_is_available(dn);
> >         retval = device_attach(&dev->dev);
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index 0f260cdc4592..aaf5a68e7984 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> >          * reason is that the bridge may have additional methods such as
> >          * _DSW that need to be called.
> >          */
> > -       if (pci_dev->bridge_d3_allowed)
> > +       if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> >                 device_wakeup_enable(dev);
> >
> >         acpi_pci_wakeup(pci_dev, false);
> > @@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
> >         pci_acpi_remove_pm_notifier(adev);
> >         if (adev->wakeup.flags.valid) {
> >                 acpi_device_power_remove_dependent(adev, dev);
> > -               if (pci_dev->bridge_d3_allowed)
> > +               if (pci_dev->bridge_d3cold_allowed &&
> > +                   pci_dev->bridge_d3hot_allowed)
> >                         device_wakeup_disable(dev);
> >
> >                 device_set_wakeup_capable(dev, false);
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 40cfa716392f..45628b0dd116 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
> >                 return -EINVAL;
> >
> >         pdev->d3cold_allowed = !!val;
> > -       pci_bridge_d3_update(pdev);
> > +       pci_bridge_d3cold_update(pdev);
> >
> >         pm_runtime_resume(dev);
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 0edc4e448c2d..48e2ca0cd8a0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
> >  }
> >  EXPORT_SYMBOL_GPL(pci_ats_disabled);
> >
> > -/* Disable bridge_d3 for all PCIe ports */
> > +/* Disable both D3Hot and D3Cold for all PCIe ports */
> >  static bool pci_bridge_d3_disable;
> > -/* Force bridge_d3 for all PCIe ports */
> > +/* Force both D3Hot and D3Cold for all PCIe ports */
> >  static bool pci_bridge_d3_force;
> >
> >  static int __init pcie_port_pm_setup(char *str)
> > @@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
> >         { }
> >  };
> >
> > -/**
> > - * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
> > - * @bridge: Bridge to check
> > - *
> > - * This function checks if the bridge is allowed to move to D3.
> > - * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
> > +/*
> > + * Helper function to check whether it is allowed to put the bridge into D3
> > + * states (D3Hot and D3Cold).
> >   */
> > -bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> > +static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
> >  {
> >         if (!pci_is_pcie(bridge))
> >                 return false;
> > @@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> >         return false;
> >  }
> >
> > +/**
> > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
> > + * @bridge: Bridge to check
> > + *
> > + * This function checks if the bridge is allowed to move to D3Cold.
> > + * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
> > + * platforms and Thunderbolt.
> > + */
> > +bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
> > +{
> > +       return pci_bridge_d3_allowed(bridge, PCI_D3cold);
> > +}
> > +
> > +/**
> > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Hot
> 
> typo? pci_bridge_d3hot_allowed.
> 

Yep, nice catch!

> > + * @bridge: Bridge to check
> > + *
> > + * This function checks if the bridge is allowed to move to D3Hot.
> > + * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> > + * platforms and Thunderbolt.
> > + */
> > +bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
> > +{
> > +       return pci_bridge_d3_allowed(bridge, PCI_D3hot);
> > +}
> > +
> >  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> >  {
> >         bool *d3cold_ok = data;
> > @@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> >  }
> >
> >  /*
> > - * pci_bridge_d3_update - Update bridge D3 capabilities
> > + * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
> >   * @dev: PCI device which is changed
> >   *
> >   * Update upstream bridge PM capabilities accordingly depending on if the
> >   * device PM configuration was changed or the device is being removed.  The
> >   * change is also propagated upstream.
> >   */
> > -void pci_bridge_d3_update(struct pci_dev *dev)
> > +void pci_bridge_d3cold_update(struct pci_dev *dev)
> >  {
> >         bool remove = !device_is_registered(&dev->dev);
> >         struct pci_dev *bridge;
> >         bool d3cold_ok = true;
> >
> >         bridge = pci_upstream_bridge(dev);
> > -       if (!bridge || !pci_bridge_d3_allowed(bridge))
> > +       if (!bridge || !pci_bridge_d3cold_allowed(bridge))
> >                 return;
> >
> >         /*
> > -        * If D3 is currently allowed for the bridge, removing one of its
> > +        * If D3Cold is currently allowed for the bridge, removing one of its
> >          * children won't change that.
> >          */
> > -       if (remove && bridge->bridge_d3_allowed)
> > +       if (remove && bridge->bridge_d3cold_allowed)
> >                 return;
> >
> >         /*
> > -        * If D3 is currently allowed for the bridge and a child is added or
> > -        * changed, disallowance of D3 can only be caused by that child, so
> > +        * If D3Cold is currently allowed for the bridge and a child is added or
> > +        * changed, disallowance of D3Cold can only be caused by that child, so
> >          * we only need to check that single device, not any of its siblings.
> >          *
> > -        * If D3 is currently not allowed for the bridge, checking the device
> > -        * first may allow us to skip checking its siblings.
> > +        * If D3Cold is currently not allowed for the bridge, checking the
> > +        * device first may allow us to skip checking its siblings.
> >          */
> >         if (!remove)
> >                 pci_dev_check_d3cold(dev, &d3cold_ok);
> >
> >         /*
> > -        * If D3 is currently not allowed for the bridge, this may be caused
> > +        * If D3Cold is currently not allowed for the bridge, this may be caused
> >          * either by the device being changed/removed or any of its siblings,
> >          * so we need to go through all children to find out if one of them
> > -        * continues to block D3.
> > +        * continues to block D3Cold.
> >          */
> > -       if (d3cold_ok && !bridge->bridge_d3_allowed)
> > +       if (d3cold_ok && !bridge->bridge_d3cold_allowed)
> >                 pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> >                              &d3cold_ok);
> >
> > -       if (bridge->bridge_d3_allowed != d3cold_ok) {
> > -               bridge->bridge_d3_allowed = d3cold_ok;
> > +       if (bridge->bridge_d3cold_allowed != d3cold_ok) {
> > +               bridge->bridge_d3cold_allowed = d3cold_ok;
> >                 /* Propagate change to upstream bridges */
> > -               pci_bridge_d3_update(bridge);
> > +               pci_bridge_d3cold_update(bridge);
> >         }
> >  }
> >
> > @@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
> >  {
> >         if (dev->no_d3cold) {
> >                 dev->no_d3cold = false;
> > -               pci_bridge_d3_update(dev);
> > +               pci_bridge_d3cold_update(dev);
> >         }
> >  }
> >  EXPORT_SYMBOL_GPL(pci_d3cold_enable);
> > @@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
> >  {
> >         if (!dev->no_d3cold) {
> >                 dev->no_d3cold = true;
> > -               pci_bridge_d3_update(dev);
> > +               pci_bridge_d3cold_update(dev);
> >         }
> >  }
> >  EXPORT_SYMBOL_GPL(pci_d3cold_disable);
> > @@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
> >         dev->pm_cap = pm;
> >         dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
> >         dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
> > -       dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
> > +       dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
> > +       dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
> >         dev->d3cold_allowed = true;
> >
> >         dev->d1_support = false;
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 53ca75639201..f819eab793fc 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
> >  void pci_ea_init(struct pci_dev *dev);
> >  void pci_msi_init(struct pci_dev *dev);
> >  void pci_msix_init(struct pci_dev *dev);
> > -bool pci_bridge_d3_allowed(struct pci_dev *dev);
> > -void pci_bridge_d3_update(struct pci_dev *dev);
> > +bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
> > +bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
> > +void pci_bridge_d3cold_update(struct pci_dev *dev);
> >  int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
> >
> >  static inline void pci_wakeup_event(struct pci_dev *dev)
> > @@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
> >  {
> >         /*
> >          * Currently we allow normal PCI devices and PCI bridges transition
> > -        * into D3 if their bridge_d3 is set.
> > +        * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
> > +        * are set.
> >          */
> 
> If pm requires both D3hot and D3cold, can we add a flag for DT to
> support D3cold? Otherwise during suspend, pcieport still stays at D0.
> 

You mean D3hot?

> [   42.202016] mt7921e 0000:01:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot

Here I can see that the port entered D3hot

> [   42.238048] mt7921e 0000:01:00.0: PM:
> pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> [   42.247083] pcieport 0000:00:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0

And resuming with D0.

Problem with D3Cold is, it is a power off state. If a driver wants a device/port
to enter D3Cold, then it needs to know the power supply. Otherwise, techinically
the driver cannot put the device into D3Cold. And there is no power supply
exposed in DT for most of the rootports/bridges.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Hsin-Yi Wang]

On Thu, Aug 1, 2024 at 10:33 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Thu, Aug 01, 2024 at 02:07:41PM -0700, Hsin-Yi Wang wrote:
> > On Fri, Jul 26, 2024 at 4:02 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Currently, there is no proper distinction between D3Hot and D3Cold while
> > > handling the power management for PCI bridges. For instance,
> > > pci_bridge_d3_allowed() API decides whether it is allowed to put the
> > > bridge in D3, but it doesn't explicitly specify whether D3Hot or D3Cold
> > > is allowed in a scenario. This often leads to confusion and may be prone
> > > to errors.
> > >
> > > So let's split the D3Hot and D3Cold handling where possible. The current
> > > pci_bridge_d3_allowed() API is now split into pci_bridge_d3hot_allowed()
> > > and pci_bridge_d3cold_allowed() APIs and used in relevant places.
> > >
> > > Also, pci_bridge_d3_update() API is now renamed to
> > > pci_bridge_d3cold_update() since it was only used to check the possibility
> > > of D3Cold.
> > >
> > > Note that it is assumed that only D3Hot needs to be checked while
> > > transitioning the bridge during runtime PM and D3Cold in other places. In
> > > the ACPI case, wakeup is now only enabled if both D3Hot and D3Cold are
> > > allowed for the bridge.
> > >
> > > Still, there are places where just 'd3' is used opaquely, but those are
> > > hard to distinguish, hence left for future cleanups.
> > >
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/bus.c          |  2 +-
> > >  drivers/pci/pci-acpi.c     |  5 +--
> > >  drivers/pci/pci-sysfs.c    |  2 +-
> > >  drivers/pci/pci.c          | 78 ++++++++++++++++++++++++++++++----------------
> > >  drivers/pci/pci.h          | 12 ++++---
> > >  drivers/pci/pcie/portdrv.c | 16 +++++-----
> > >  drivers/pci/remove.c       |  2 +-
> > >  include/linux/pci.h        |  3 +-
> > >  8 files changed, 75 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > > index 826b5016a101..cb1a1aaefa90 100644
> > > --- a/drivers/pci/bus.c
> > > +++ b/drivers/pci/bus.c
> > > @@ -346,7 +346,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> > >                 of_pci_make_dev_node(dev);
> > >         pci_create_sysfs_dev_files(dev);
> > >         pci_proc_attach_device(dev);
> > > -       pci_bridge_d3_update(dev);
> > > +       pci_bridge_d3cold_update(dev);
> > >
> > >         dev->match_driver = !dn || of_device_is_available(dn);
> > >         retval = device_attach(&dev->dev);
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index 0f260cdc4592..aaf5a68e7984 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -1434,7 +1434,7 @@ void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> > >          * reason is that the bridge may have additional methods such as
> > >          * _DSW that need to be called.
> > >          */
> > > -       if (pci_dev->bridge_d3_allowed)
> > > +       if (pci_dev->bridge_d3cold_allowed && pci_dev->bridge_d3hot_allowed)
> > >                 device_wakeup_enable(dev);
> > >
> > >         acpi_pci_wakeup(pci_dev, false);
> > > @@ -1452,7 +1452,8 @@ void pci_acpi_cleanup(struct device *dev, struct acpi_device *adev)
> > >         pci_acpi_remove_pm_notifier(adev);
> > >         if (adev->wakeup.flags.valid) {
> > >                 acpi_device_power_remove_dependent(adev, dev);
> > > -               if (pci_dev->bridge_d3_allowed)
> > > +               if (pci_dev->bridge_d3cold_allowed &&
> > > +                   pci_dev->bridge_d3hot_allowed)
> > >                         device_wakeup_disable(dev);
> > >
> > >                 device_set_wakeup_capable(dev, false);
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 40cfa716392f..45628b0dd116 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -529,7 +529,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
> > >                 return -EINVAL;
> > >
> > >         pdev->d3cold_allowed = !!val;
> > > -       pci_bridge_d3_update(pdev);
> > > +       pci_bridge_d3cold_update(pdev);
> > >
> > >         pm_runtime_resume(dev);
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 0edc4e448c2d..48e2ca0cd8a0 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -166,9 +166,9 @@ bool pci_ats_disabled(void)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_ats_disabled);
> > >
> > > -/* Disable bridge_d3 for all PCIe ports */
> > > +/* Disable both D3Hot and D3Cold for all PCIe ports */
> > >  static bool pci_bridge_d3_disable;
> > > -/* Force bridge_d3 for all PCIe ports */
> > > +/* Force both D3Hot and D3Cold for all PCIe ports */
> > >  static bool pci_bridge_d3_force;
> > >
> > >  static int __init pcie_port_pm_setup(char *str)
> > > @@ -2966,14 +2966,11 @@ static const struct dmi_system_id bridge_d3_blacklist[] = {
> > >         { }
> > >  };
> > >
> > > -/**
> > > - * pci_bridge_d3_allowed - Is it allowed to put the bridge into D3
> > > - * @bridge: Bridge to check
> > > - *
> > > - * This function checks if the bridge is allowed to move to D3.
> > > - * Currently we only allow D3 for recent enough PCIe ports and Thunderbolt.
> > > +/*
> > > + * Helper function to check whether it is allowed to put the bridge into D3
> > > + * states (D3Hot and D3Cold).
> > >   */
> > > -bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> > > +static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
> > >  {
> > >         if (!pci_is_pcie(bridge))
> > >                 return false;
> > > @@ -3026,6 +3023,32 @@ bool pci_bridge_d3_allowed(struct pci_dev *bridge)
> > >         return false;
> > >  }
> > >
> > > +/**
> > > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Cold
> > > + * @bridge: Bridge to check
> > > + *
> > > + * This function checks if the bridge is allowed to move to D3Cold.
> > > + * Currently we only allow D3Cold for recent enough PCIe ports on ACPI based
> > > + * platforms and Thunderbolt.
> > > + */
> > > +bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
> > > +{
> > > +       return pci_bridge_d3_allowed(bridge, PCI_D3cold);
> > > +}
> > > +
> > > +/**
> > > + * pci_bridge_d3cold_allowed - Is it allowed to put the bridge into D3Hot
> >
> > typo? pci_bridge_d3hot_allowed.
> >
>
> Yep, nice catch!
>
> > > + * @bridge: Bridge to check
> > > + *
> > > + * This function checks if the bridge is allowed to move to D3Hot.
> > > + * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> > > + * platforms and Thunderbolt.
> > > + */
> > > +bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
> > > +{
> > > +       return pci_bridge_d3_allowed(bridge, PCI_D3hot);
> > > +}
> > > +
> > >  static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> > >  {
> > >         bool *d3cold_ok = data;
> > > @@ -3046,55 +3069,55 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
> > >  }
> > >
> > >  /*
> > > - * pci_bridge_d3_update - Update bridge D3 capabilities
> > > + * pci_bridge_d3cold_update - Update bridge D3Cold capabilities
> > >   * @dev: PCI device which is changed
> > >   *
> > >   * Update upstream bridge PM capabilities accordingly depending on if the
> > >   * device PM configuration was changed or the device is being removed.  The
> > >   * change is also propagated upstream.
> > >   */
> > > -void pci_bridge_d3_update(struct pci_dev *dev)
> > > +void pci_bridge_d3cold_update(struct pci_dev *dev)
> > >  {
> > >         bool remove = !device_is_registered(&dev->dev);
> > >         struct pci_dev *bridge;
> > >         bool d3cold_ok = true;
> > >
> > >         bridge = pci_upstream_bridge(dev);
> > > -       if (!bridge || !pci_bridge_d3_allowed(bridge))
> > > +       if (!bridge || !pci_bridge_d3cold_allowed(bridge))
> > >                 return;
> > >
> > >         /*
> > > -        * If D3 is currently allowed for the bridge, removing one of its
> > > +        * If D3Cold is currently allowed for the bridge, removing one of its
> > >          * children won't change that.
> > >          */
> > > -       if (remove && bridge->bridge_d3_allowed)
> > > +       if (remove && bridge->bridge_d3cold_allowed)
> > >                 return;
> > >
> > >         /*
> > > -        * If D3 is currently allowed for the bridge and a child is added or
> > > -        * changed, disallowance of D3 can only be caused by that child, so
> > > +        * If D3Cold is currently allowed for the bridge and a child is added or
> > > +        * changed, disallowance of D3Cold can only be caused by that child, so
> > >          * we only need to check that single device, not any of its siblings.
> > >          *
> > > -        * If D3 is currently not allowed for the bridge, checking the device
> > > -        * first may allow us to skip checking its siblings.
> > > +        * If D3Cold is currently not allowed for the bridge, checking the
> > > +        * device first may allow us to skip checking its siblings.
> > >          */
> > >         if (!remove)
> > >                 pci_dev_check_d3cold(dev, &d3cold_ok);
> > >
> > >         /*
> > > -        * If D3 is currently not allowed for the bridge, this may be caused
> > > +        * If D3Cold is currently not allowed for the bridge, this may be caused
> > >          * either by the device being changed/removed or any of its siblings,
> > >          * so we need to go through all children to find out if one of them
> > > -        * continues to block D3.
> > > +        * continues to block D3Cold.
> > >          */
> > > -       if (d3cold_ok && !bridge->bridge_d3_allowed)
> > > +       if (d3cold_ok && !bridge->bridge_d3cold_allowed)
> > >                 pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> > >                              &d3cold_ok);
> > >
> > > -       if (bridge->bridge_d3_allowed != d3cold_ok) {
> > > -               bridge->bridge_d3_allowed = d3cold_ok;
> > > +       if (bridge->bridge_d3cold_allowed != d3cold_ok) {
> > > +               bridge->bridge_d3cold_allowed = d3cold_ok;
> > >                 /* Propagate change to upstream bridges */
> > > -               pci_bridge_d3_update(bridge);
> > > +               pci_bridge_d3cold_update(bridge);
> > >         }
> > >  }
> > >
> > > @@ -3110,7 +3133,7 @@ void pci_d3cold_enable(struct pci_dev *dev)
> > >  {
> > >         if (dev->no_d3cold) {
> > >                 dev->no_d3cold = false;
> > > -               pci_bridge_d3_update(dev);
> > > +               pci_bridge_d3cold_update(dev);
> > >         }
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_d3cold_enable);
> > > @@ -3127,7 +3150,7 @@ void pci_d3cold_disable(struct pci_dev *dev)
> > >  {
> > >         if (!dev->no_d3cold) {
> > >                 dev->no_d3cold = true;
> > > -               pci_bridge_d3_update(dev);
> > > +               pci_bridge_d3cold_update(dev);
> > >         }
> > >  }
> > >  EXPORT_SYMBOL_GPL(pci_d3cold_disable);
> > > @@ -3167,7 +3190,8 @@ void pci_pm_init(struct pci_dev *dev)
> > >         dev->pm_cap = pm;
> > >         dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
> > >         dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
> > > -       dev->bridge_d3_allowed = pci_bridge_d3_allowed(dev);
> > > +       dev->bridge_d3cold_allowed = pci_bridge_d3cold_allowed(dev);
> > > +       dev->bridge_d3hot_allowed = pci_bridge_d3hot_allowed(dev);
> > >         dev->d3cold_allowed = true;
> > >
> > >         dev->d1_support = false;
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 53ca75639201..f819eab793fc 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -92,8 +92,9 @@ void pci_pm_init(struct pci_dev *dev);
> > >  void pci_ea_init(struct pci_dev *dev);
> > >  void pci_msi_init(struct pci_dev *dev);
> > >  void pci_msix_init(struct pci_dev *dev);
> > > -bool pci_bridge_d3_allowed(struct pci_dev *dev);
> > > -void pci_bridge_d3_update(struct pci_dev *dev);
> > > +bool pci_bridge_d3cold_allowed(struct pci_dev *dev);
> > > +bool pci_bridge_d3hot_allowed(struct pci_dev *dev);
> > > +void pci_bridge_d3cold_update(struct pci_dev *dev);
> > >  int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
> > >
> > >  static inline void pci_wakeup_event(struct pci_dev *dev)
> > > @@ -111,9 +112,12 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
> > >  {
> > >         /*
> > >          * Currently we allow normal PCI devices and PCI bridges transition
> > > -        * into D3 if their bridge_d3 is set.
> > > +        * into D3 states if both bridge_d3cold_allowed and bridge_d3hot_allowed
> > > +        * are set.
> > >          */
> >
> > If pm requires both D3hot and D3cold, can we add a flag for DT to
> > support D3cold? Otherwise during suspend, pcieport still stays at D0.
> >
>
> You mean D3hot?
>
> > [   42.202016] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
>
> Here I can see that the port entered D3hot
>
This one is the wifi device connected to the port.

> > [   42.238048] mt7921e 0000:01:00.0: PM:
> > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > [   42.247083] pcieport 0000:00:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
>
This is the port suspended with D0. If we hack power_manageable to
only consider D3hot, the state here for pcieport will become D3hot
(compared in below).

If it's D0 (and s2idle), in resume it won't restore config:
https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
and in resume it would be an issue.

Comparison:
1. pcieport can go to D3:
(suspend)
[   61.645809] mt7921e 0000:01:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
[   61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
[   61.681931] mt7921e 0000:01:00.0: PM:
pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
[   61.690959] pcieport 0000:00:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
[   61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
[   61.761832] pcieport 0000:00:00.0: PM:
pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs

(resume)
[   65.243981] pcieport 0000:00:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
[   65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
[   65.262725] pcieport 0000:00:00.0: PM:
pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
[   65.273159] mtk-pcie-phy 16930000.phy: No calibration info
[   65.281903] mt7921e 0000:01:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
[   65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
returned 0 after 329 usecs


2. pcieport stays at D0 due to power_manageable returns false:
(suspend)
[   52.435375] mt7921e 0000:01:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
[   52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
[   52.471610] mt7921e 0000:01:00.0: PM:
pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
[   52.480674] pcieport 0000:00:00.0: PM: calling
pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
[   52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
                <-- port is still D0
[   52.536056] pcieport 0000:00:00.0: PCI PM: Skipped

(resume)
[   56.026298] pcieport 0000:00:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
[   56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
[   56.044776] pcieport 0000:00:00.0: PM:
pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
[   56.055409] mtk-pcie-phy 16930000.phy: No calibration info
[   56.064098] mt7921e 0000:01:00.0: PM: calling
pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
[   56.078962] mt7921e 0000:01:00.0: Unable to change power state from
D3hot to D0, device inaccessible                    <-- resume failed.
[   56.152363] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
returned 0 after 73406 usecs


> And resuming with D0.
>
> Problem with D3Cold is, it is a power off state. If a driver wants a device/port
> to enter D3Cold, then it needs to know the power supply. Otherwise, techinically
> the driver cannot put the device into D3Cold. And there is no power supply
> exposed in DT for most of the rootports/bridges.
>

I think maybe it can be fixed in the power_manageable checking? Since
suspend flow still considers this state. If this returns false due to
being unable to D3cold, pcieport still stays at D0 and would cause
device resume failure.

> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Fri, Aug 02, 2024 at 12:53:42PM -0700, Hsin-Yi Wang wrote:

[...]

> > > [   42.202016] mt7921e 0000:01:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > > [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> >
> > Here I can see that the port entered D3hot
> >
> This one is the wifi device connected to the port.
> 

Ah, okay. You could've just shared the logs for the bridge/rootport.

> > > [   42.238048] mt7921e 0000:01:00.0: PM:
> > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > > [   42.247083] pcieport 0000:00:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > > [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> >
> This is the port suspended with D0. If we hack power_manageable to
> only consider D3hot, the state here for pcieport will become D3hot
> (compared in below).
> 
> If it's D0 (and s2idle), in resume it won't restore config:
> https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
> and in resume it would be an issue.
> 
> Comparison:
> 1. pcieport can go to D3:
> (suspend)
> [   61.645809] mt7921e 0000:01:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
> [   61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> [   61.681931] mt7921e 0000:01:00.0: PM:
> pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
> [   61.690959] pcieport 0000:00:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
> [   61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
> [   61.761832] pcieport 0000:00:00.0: PM:
> pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs
> 

Why the device state is not saved? Did you skip those logs?

> (resume)
> [   65.243981] pcieport 0000:00:00.0: PM: calling
> pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
> [   65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> [   65.262725] pcieport 0000:00:00.0: PM:
> pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
> [   65.273159] mtk-pcie-phy 16930000.phy: No calibration info
> [   65.281903] mt7921e 0000:01:00.0: PM: calling
> pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
> [   65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
> returned 0 after 329 usecs
> 
> 
> 2. pcieport stays at D0 due to power_manageable returns false:
> (suspend)
> [   52.435375] mt7921e 0000:01:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
> [   52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> [   52.471610] mt7921e 0000:01:00.0: PM:
> pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
> [   52.480674] pcieport 0000:00:00.0: PM: calling
> pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
> [   52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
>                 <-- port is still D0
> [   52.536056] pcieport 0000:00:00.0: PCI PM: Skipped
> 
> (resume)
> [   56.026298] pcieport 0000:00:00.0: PM: calling
> pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
> [   56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> [   56.044776] pcieport 0000:00:00.0: PM:
> pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
> [   56.055409] mtk-pcie-phy 16930000.phy: No calibration info
> [   56.064098] mt7921e 0000:01:00.0: PM: calling
> pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
> [   56.078962] mt7921e 0000:01:00.0: Unable to change power state from
> D3hot to D0, device inaccessible                    <-- resume failed.

This means the port entered D3Cold? This is not expected during s2idle. During
s2idle, devices should be put into low power state and their power should be
preserved.

Who is pulling the plug here?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Hsin-Yi Wang]

On Mon, Aug 5, 2024 at 8:35 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Aug 02, 2024 at 12:53:42PM -0700, Hsin-Yi Wang wrote:
>
> [...]
>
> > > > [   42.202016] mt7921e 0000:01:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > > > [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > >
> > > Here I can see that the port entered D3hot
> > >
> > This one is the wifi device connected to the port.
> >
>
> Ah, okay. You could've just shared the logs for the bridge/rootport.
>
> > > > [   42.238048] mt7921e 0000:01:00.0: PM:
> > > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > > > [   42.247083] pcieport 0000:00:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > > > [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> > >
> > This is the port suspended with D0. If we hack power_manageable to
> > only consider D3hot, the state here for pcieport will become D3hot
> > (compared in below).
> >
> > If it's D0 (and s2idle), in resume it won't restore config:
> > https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
> > and in resume it would be an issue.
> >
> > Comparison:
> > 1. pcieport can go to D3:
> > (suspend)
> > [   61.645809] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
> > [   61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > [   61.681931] mt7921e 0000:01:00.0: PM:
> > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
> > [   61.690959] pcieport 0000:00:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
> > [   61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
> > [   61.761832] pcieport 0000:00:00.0: PM:
> > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs
> >
>
> Why the device state is not saved? Did you skip those logs?
>
Right, I only showed the power state of pcieport and the device here
to show the difference of 1 and 2.

> > (resume)
> > [   65.243981] pcieport 0000:00:00.0: PM: calling
> > pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
> > [   65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > [   65.262725] pcieport 0000:00:00.0: PM:
> > pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
> > [   65.273159] mtk-pcie-phy 16930000.phy: No calibration info
> > [   65.281903] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
> > [   65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
> > returned 0 after 329 usecs
> >
> >
> > 2. pcieport stays at D0 due to power_manageable returns false:
> > (suspend)
> > [   52.435375] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
> > [   52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > [   52.471610] mt7921e 0000:01:00.0: PM:
> > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
> > [   52.480674] pcieport 0000:00:00.0: PM: calling
> > pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
> > [   52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> >                 <-- port is still D0
> > [   52.536056] pcieport 0000:00:00.0: PCI PM: Skipped
> >
> > (resume)
> > [   56.026298] pcieport 0000:00:00.0: PM: calling
> > pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
> > [   56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > [   56.044776] pcieport 0000:00:00.0: PM:
> > pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
> > [   56.055409] mtk-pcie-phy 16930000.phy: No calibration info
> > [   56.064098] mt7921e 0000:01:00.0: PM: calling
> > pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
> > [   56.078962] mt7921e 0000:01:00.0: Unable to change power state from
> > D3hot to D0, device inaccessible                    <-- resume failed.
>
> This means the port entered D3Cold? This is not expected during s2idle. During
> s2idle, devices should be put into low power state and their power should be
> preserved.
>
> Who is pulling the plug here?

In our system's use case, after the kernel enters s2idle then ATF (arm
trusted firmware) will turn off the power (similar to suspend to ram).

The issue can previously be handled by setting pcie_port_pm=force, or
using v3 of the series that sets a flag in DT.

>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Manivannan Sadhasivam]

On Mon, Aug 05, 2024 at 12:17:13PM -0700, Hsin-Yi Wang wrote:
> On Mon, Aug 5, 2024 at 8:35 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Fri, Aug 02, 2024 at 12:53:42PM -0700, Hsin-Yi Wang wrote:
> >
> > [...]
> >
> > > > > [   42.202016] mt7921e 0000:01:00.0: PM: calling
> > > > > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > > > > [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > >
> > > > Here I can see that the port entered D3hot
> > > >
> > > This one is the wifi device connected to the port.
> > >
> >
> > Ah, okay. You could've just shared the logs for the bridge/rootport.
> >
> > > > > [   42.238048] mt7921e 0000:01:00.0: PM:
> > > > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > > > > [   42.247083] pcieport 0000:00:00.0: PM: calling
> > > > > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > > > > [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> > > >
> > > This is the port suspended with D0. If we hack power_manageable to
> > > only consider D3hot, the state here for pcieport will become D3hot
> > > (compared in below).
> > >
> > > If it's D0 (and s2idle), in resume it won't restore config:
> > > https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
> > > and in resume it would be an issue.
> > >
> > > Comparison:
> > > 1. pcieport can go to D3:
> > > (suspend)
> > > [   61.645809] mt7921e 0000:01:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
> > > [   61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > [   61.681931] mt7921e 0000:01:00.0: PM:
> > > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
> > > [   61.690959] pcieport 0000:00:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
> > > [   61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
> > > [   61.761832] pcieport 0000:00:00.0: PM:
> > > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs
> > >
> >
> > Why the device state is not saved? Did you skip those logs?
> >
> Right, I only showed the power state of pcieport and the device here
> to show the difference of 1 and 2.
> 
> > > (resume)
> > > [   65.243981] pcieport 0000:00:00.0: PM: calling
> > > pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
> > > [   65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > > [   65.262725] pcieport 0000:00:00.0: PM:
> > > pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
> > > [   65.273159] mtk-pcie-phy 16930000.phy: No calibration info
> > > [   65.281903] mt7921e 0000:01:00.0: PM: calling
> > > pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
> > > [   65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
> > > returned 0 after 329 usecs
> > >
> > >
> > > 2. pcieport stays at D0 due to power_manageable returns false:
> > > (suspend)
> > > [   52.435375] mt7921e 0000:01:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
> > > [   52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > [   52.471610] mt7921e 0000:01:00.0: PM:
> > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
> > > [   52.480674] pcieport 0000:00:00.0: PM: calling
> > > pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
> > > [   52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> > >                 <-- port is still D0
> > > [   52.536056] pcieport 0000:00:00.0: PCI PM: Skipped
> > >
> > > (resume)
> > > [   56.026298] pcieport 0000:00:00.0: PM: calling
> > > pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
> > > [   56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > > [   56.044776] pcieport 0000:00:00.0: PM:
> > > pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
> > > [   56.055409] mtk-pcie-phy 16930000.phy: No calibration info
> > > [   56.064098] mt7921e 0000:01:00.0: PM: calling
> > > pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
> > > [   56.078962] mt7921e 0000:01:00.0: Unable to change power state from
> > > D3hot to D0, device inaccessible                    <-- resume failed.
> >
> > This means the port entered D3Cold? This is not expected during s2idle. During
> > s2idle, devices should be put into low power state and their power should be
> > preserved.
> >
> > Who is pulling the plug here?
> 
> In our system's use case, after the kernel enters s2idle then ATF (arm
> trusted firmware) will turn off the power (similar to suspend to ram).
> 

This is not acceptable IMO. S2IDLE != S2RAM. Even if you fix the portdrv, rest
of the PCIe client drivers may fail (hint: have you checked the NVMe driver)?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/4] PCI: Decouple D3Hot and D3Cold handling for bridges

[Hsin-Yi Wang]

On Tue, Aug 6, 2024 at 8:03 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, Aug 05, 2024 at 12:17:13PM -0700, Hsin-Yi Wang wrote:
> > On Mon, Aug 5, 2024 at 8:35 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Fri, Aug 02, 2024 at 12:53:42PM -0700, Hsin-Yi Wang wrote:
> > >
> > > [...]
> > >
> > > > > > [   42.202016] mt7921e 0000:01:00.0: PM: calling
> > > > > > pci_pm_suspend_noirq+0x0/0x300 @ 77, parent: 0000:00:00.0
> > > > > > [   42.231681] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > > >
> > > > > Here I can see that the port entered D3hot
> > > > >
> > > > This one is the wifi device connected to the port.
> > > >
> > >
> > > Ah, okay. You could've just shared the logs for the bridge/rootport.
> > >
> > > > > > [   42.238048] mt7921e 0000:01:00.0: PM:
> > > > > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26583 usecs
> > > > > > [   42.247083] pcieport 0000:00:00.0: PM: calling
> > > > > > pci_pm_suspend_noirq+0x0/0x300 @ 3196, parent: pci0000:00
> > > > > > [   42.296325] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> > > > >
> > > > This is the port suspended with D0. If we hack power_manageable to
> > > > only consider D3hot, the state here for pcieport will become D3hot
> > > > (compared in below).
> > > >
> > > > If it's D0 (and s2idle), in resume it won't restore config:
> > > > https://elixir.bootlin.com/linux/v6.10/source/drivers/pci/pci-driver.c#L959,
> > > > and in resume it would be an issue.
> > > >
> > > > Comparison:
> > > > 1. pcieport can go to D3:
> > > > (suspend)
> > > > [   61.645809] mt7921e 0000:01:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x2f8 @ 1139, parent: 0000:00:00.0
> > > > [   61.675562] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > > [   61.681931] mt7921e 0000:01:00.0: PM:
> > > > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 26502 usecs
> > > > [   61.690959] pcieport 0000:00:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x2f8 @ 3248, parent: pci0000:00
> > > > [   61.755359] pcieport 0000:00:00.0: PCI PM: Suspend power state: D3hot
> > > > [   61.761832] pcieport 0000:00:00.0: PM:
> > > > pci_pm_suspend_noirq+0x0/0x2f8 returned 0 after 61345 usecs
> > > >
> > >
> > > Why the device state is not saved? Did you skip those logs?
> > >
> > Right, I only showed the power state of pcieport and the device here
> > to show the difference of 1 and 2.
> >
> > > > (resume)
> > > > [   65.243981] pcieport 0000:00:00.0: PM: calling
> > > > pci_pm_resume_noirq+0x0/0x190 @ 3258, parent: pci0000:00
> > > > [   65.253122] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > > > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > > > [   65.262725] pcieport 0000:00:00.0: PM:
> > > > pci_pm_resume_noirq+0x0/0x190 returned 0 after 175 usecs
> > > > [   65.273159] mtk-pcie-phy 16930000.phy: No calibration info
> > > > [   65.281903] mt7921e 0000:01:00.0: PM: calling
> > > > pci_pm_resume_noirq+0x0/0x190 @ 3259, parent: 0000:00:00.0
> > > > [   65.297108] mt7921e 0000:01:00.0: PM: pci_pm_resume_noirq+0x0/0x190
> > > > returned 0 after 329 usecs
> > > >
> > > >
> > > > 2. pcieport stays at D0 due to power_manageable returns false:
> > > > (suspend)
> > > > [   52.435375] mt7921e 0000:01:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x300 @ 2040, parent: 0000:00:00.0
> > > > [   52.465235] mt7921e 0000:01:00.0: PCI PM: Suspend power state: D3hot
> > > > [   52.471610] mt7921e 0000:01:00.0: PM:
> > > > pci_pm_suspend_noirq+0x0/0x300 returned 0 after 26602 usecs
> > > > [   52.480674] pcieport 0000:00:00.0: PM: calling
> > > > pci_pm_suspend_noirq+0x0/0x300 @ 143, parent: pci0000:00
> > > > [   52.529876] pcieport 0000:00:00.0: PCI PM: Suspend power state: D0
> > > >                 <-- port is still D0
> > > > [   52.536056] pcieport 0000:00:00.0: PCI PM: Skipped
> > > >
> > > > (resume)
> > > > [   56.026298] pcieport 0000:00:00.0: PM: calling
> > > > pci_pm_resume_noirq+0x0/0x190 @ 3243, parent: pci0000:00
> > > > [   56.035379] mtk-pcie-phy 16930000.phy: CKM_38=0x13040500,
> > > > GLB_20=0x0, GLB_30=0x0, GLB_38=0x30453fc, GLB_F4=0x1453b000
> > > > [   56.044776] pcieport 0000:00:00.0: PM:
> > > > pci_pm_resume_noirq+0x0/0x190 returned 0 after 13 usecs
> > > > [   56.055409] mtk-pcie-phy 16930000.phy: No calibration info
> > > > [   56.064098] mt7921e 0000:01:00.0: PM: calling
> > > > pci_pm_resume_noirq+0x0/0x190 @ 3244, parent: 0000:00:00.0
> > > > [   56.078962] mt7921e 0000:01:00.0: Unable to change power state from
> > > > D3hot to D0, device inaccessible                    <-- resume failed.
> > >
> > > This means the port entered D3Cold? This is not expected during s2idle. During
> > > s2idle, devices should be put into low power state and their power should be
> > > preserved.
> > >
> > > Who is pulling the plug here?
> >
> > In our system's use case, after the kernel enters s2idle then ATF (arm
> > trusted firmware) will turn off the power (similar to suspend to ram).
> >
>
> This is not acceptable IMO. S2IDLE != S2RAM. Even if you fix the portdrv, rest
> of the PCIe client drivers may fail (hint: have you checked the NVMe driver)?
>

NVMe and its port stays at D0. We won't power off them.

> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்