[PATCH 0/2] Enable D3 support for Qualcomm bridges

[PATCH 0/2] Enable D3 support for Qualcomm bridges

[Manivannan Sadhasivam]

Hello,

This series enables D3 support for PCI bridges found in Qcom SoCs. Currently,
PCI core will enable D3 support for PCI bridges only when 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 Qcom SoCs, option 3 will make the life
harder for distro maintainers. Due to this, runtime PM is also not getting
enabled for the bridges.

Ideally, D3 support should be enabled by default for the recent PCI bridges,
but we do not have a sane way to detect them. So this series adds a new flag
"bridge_d3_capable" to "struct pci_dev" which could be set by the bridge drivers
for capable devices. This will allow the PCI core to enable D3 support for the
bridges during enumeration.

In the Qcom controller driver, this flag is only set for recent bridges with PID
(0x0110).

Testing
=======

This series is tested on SM8450 based development board.

- Mani

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

---
Manivannan Sadhasivam (2):
      PCI: Add a flag to enable D3 support for PCI bridges
      PCI: qcom: Enable D3 support for the recent PCI bridges

 drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++++
 drivers/pci/pci.c                      | 3 +++
 include/linux/pci.h                    | 1 +
 3 files changed, 10 insertions(+)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240131-pcie-qcom-bridge-b6802a9770a3

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

[PATCH 1/2] PCI: Add a flag to enable D3 support for PCI bridges

[Manivannan Sadhasivam]

Currently, PCI core will enable D3 support for PCI bridges only when 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 Qcom SoCs, option 3 will make the
life harder for distro maintainers. Due to this, runtime PM is also not
getting enabled for the bridges.

Ideally, D3 support should be enabled by default for the recent PCI
bridges, but we do not have a sane way to detect them. So let's adds a new
flag, "bridge_d3_capable" to "struct pci_dev" which could be set by the
bridge drivers for capable devices. This will allow the PCI core to enable
D3 support for the bridges during enumeration.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci.c   | 3 +++
 include/linux/pci.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d8f11a078924..8226a65d8ca1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3127,6 +3127,9 @@ bool pci_bridge_d3_possible(struct pci_dev *bridge)
 		 */
 		if (dmi_get_bios_year() >= 2015)
 			return true;
+
+		if (bridge->bridge_d3_capable)
+			return true;
 		break;
 	}
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index add9368e6314..161c0acf2b3e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,6 +376,7 @@ struct pci_dev {
 	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_capable:1;	/* D3 capability 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 2/2] PCI: qcom: Enable D3 support for the recent PCI bridges

[Manivannan Sadhasivam]

Make use of the "bridge_d3_capable" flag to specify the D3 capability to
the PCI core so that the D3 support (in turn runtime PM) will be enabled
for the PCI bridges.

Currently, only for the recent bridges with PID "0x0110", this flag is set
as a fixup. Because, there is no guarantee that the older bridges will
support D3.

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

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 10f2d0bb86be..a6ae78d2ce92 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1649,6 +1649,11 @@ static void qcom_fixup_class(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_PCI_NORMAL;
 }
+
+static void qcom_fixup_bridge_d3_capability(struct pci_dev *dev)
+{
+	dev->bridge_d3_capable = true;
+}
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0101, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0104, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0106, qcom_fixup_class);
@@ -1656,6 +1661,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0107, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0302, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1000, qcom_fixup_class);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x0110, qcom_fixup_bridge_d3_capability);
 
 static const struct dev_pm_ops qcom_pcie_pm_ops = {
 	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_suspend_noirq, qcom_pcie_resume_noirq)

-- 
2.25.1

Re: [PATCH 0/2] Enable D3 support for Qualcomm bridges

[Lukas Wunner]

On Fri, Feb 02, 2024 at 12:24:16PM +0530, Manivannan Sadhasivam wrote:
> This series enables D3 support for PCI bridges found in Qcom SoCs. Currently,
> PCI core will enable D3 support for PCI bridges only when 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 Qcom SoCs, option 3 will make the life
> harder for distro maintainers. Due to this, runtime PM is also not getting
> enabled for the bridges.
> 
> Ideally, D3 support should be enabled by default for the recent PCI bridges,
> but we do not have a sane way to detect them. So this series adds a new flag
> "bridge_d3_capable" to "struct pci_dev" which could be set by the bridge
> drivers for capable devices. This will allow the PCI core to enable D3
> support for the bridges during enumeration.

I think the right way to do this is to use the existing call to
platform_pci_bridge_d3() in pci_bridge_d3_possible().

Please amend platform_pci_bridge_d3() to call a new of_pci_bridge_d3()
function which determines whether D3 is supported by the platform.

E.g. of_pci_bridge_d3() could contain a whitelist of supported VID/DID
tuples.  Or it could be defined as a __weak function which always
returns false but can be overridden at link time by a function
defined somewhere in arch/arm/, arch/arm64/ or in some driver
whose Kconfig option is enabled in Qualcomm platforms.

Adding a bit to struct pci_dev essentially duplicates the existing
platform_pci_bridge_d3() functionality, which seems inelegant.
It increases the size of struct pci_dev even on platforms which
don't need it (e.g. ACPI).

Thanks,

Lukas

Re: [PATCH 0/2] Enable D3 support for Qualcomm bridges

[Manivannan Sadhasivam]

On Fri, Feb 02, 2024 at 10:00:33AM +0100, Lukas Wunner wrote:
> On Fri, Feb 02, 2024 at 12:24:16PM +0530, Manivannan Sadhasivam wrote:
> > This series enables D3 support for PCI bridges found in Qcom SoCs. Currently,
> > PCI core will enable D3 support for PCI bridges only when 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 Qcom SoCs, option 3 will make the life
> > harder for distro maintainers. Due to this, runtime PM is also not getting
> > enabled for the bridges.
> > 
> > Ideally, D3 support should be enabled by default for the recent PCI bridges,
> > but we do not have a sane way to detect them. So this series adds a new flag
> > "bridge_d3_capable" to "struct pci_dev" which could be set by the bridge
> > drivers for capable devices. This will allow the PCI core to enable D3
> > support for the bridges during enumeration.
> 
> I think the right way to do this is to use the existing call to
> platform_pci_bridge_d3() in pci_bridge_d3_possible().
> 
> Please amend platform_pci_bridge_d3() to call a new of_pci_bridge_d3()
> function which determines whether D3 is supported by the platform.
> 
> E.g. of_pci_bridge_d3() could contain a whitelist of supported VID/DID
> tuples.  Or it could be defined as a __weak function which always
> returns false but can be overridden at link time by a function
> defined somewhere in arch/arm/, arch/arm64/ or in some driver
> whose Kconfig option is enabled in Qualcomm platforms.
> 

Hmm. If we go with a DT based solution, then introducing a new property like
"d3-support" in the PCI bridge node would be the right approach. But then, it
also requires defining the PCI bridge node in all the DTs. But that should be
fine since it will help us to support WAKE# (per bridge) in the future.

Thanks for the review.

- Mani

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

Re: [PATCH 0/2] Enable D3 support for Qualcomm bridges

[Lukas Wunner]

On Fri, Feb 02, 2024 at 03:30:41PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Feb 02, 2024 at 10:00:33AM +0100, Lukas Wunner wrote:
> > Please amend platform_pci_bridge_d3() to call a new of_pci_bridge_d3()
> > function which determines whether D3 is supported by the platform.
> > 
> > E.g. of_pci_bridge_d3() could contain a whitelist of supported VID/DID
> > tuples.  Or it could be defined as a __weak function which always
> > returns false but can be overridden at link time by a function
> > defined somewhere in arch/arm/, arch/arm64/ or in some driver
> > whose Kconfig option is enabled in Qualcomm platforms.
> 
> Hmm. If we go with a DT based solution, then introducing a new property like
> "d3-support" in the PCI bridge node would be the right approach. But then, it
> also requires defining the PCI bridge node in all the DTs. But that should be
> fine since it will help us to support WAKE# (per bridge) in the future.

I'm not sure whether a "d3-support" property would be acceptable.
My understanding is that capabilities which can be auto-sensed by
the driver (or the PCI core in this case), e.g. by looking at the
PCI IDs or compatible string, should not be described in the DT.

My point was really that this should be determined by
platform_pci_bridge_d3(), that's what the function is for,
instead of inventing a new mechanism.  Exactly how the capability
is detected by of_pci_bridge_d3() is up to DT schema maintainers.

A DT property does have the advantage of better maintainability,
unlike a whitelist which may need to constantly be extended.

Thanks,

Lukas

Re: [PATCH 0/2] Enable D3 support for Qualcomm bridges

[Manivannan Sadhasivam]

On Fri, Feb 02, 2024 at 08:33:26PM +0100, Lukas Wunner wrote:
> On Fri, Feb 02, 2024 at 03:30:41PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Feb 02, 2024 at 10:00:33AM +0100, Lukas Wunner wrote:
> > > Please amend platform_pci_bridge_d3() to call a new of_pci_bridge_d3()
> > > function which determines whether D3 is supported by the platform.
> > > 
> > > E.g. of_pci_bridge_d3() could contain a whitelist of supported VID/DID
> > > tuples.  Or it could be defined as a __weak function which always
> > > returns false but can be overridden at link time by a function
> > > defined somewhere in arch/arm/, arch/arm64/ or in some driver
> > > whose Kconfig option is enabled in Qualcomm platforms.
> > 
> > Hmm. If we go with a DT based solution, then introducing a new property like
> > "d3-support" in the PCI bridge node would be the right approach. But then, it
> > also requires defining the PCI bridge node in all the DTs. But that should be
> > fine since it will help us to support WAKE# (per bridge) in the future.
> 
> I'm not sure whether a "d3-support" property would be acceptable.
> My understanding is that capabilities which can be auto-sensed by
> the driver (or the PCI core in this case), e.g. by looking at the
> PCI IDs or compatible string, should not be described in the DT.
> 

We cannot whitelist platforms in DT. DT should describe the hardware and its
capabilities. In this case, the "supports-d3" property as I proposed [1] tells
the OS that this bridge is capable of supporting D3.

Blacklisting/whitelisting belongs to the OS. We can however, whitelist the
bridges in PCI core. But that has the downside of not being useful to other OSes
supporting DT. Hence, a DT property that describes the hardware capability
makes sense to me.

- Mani

[1] https://github.com/devicetree-org/dt-schema/pull/127

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