diff mbox series

[v2] PCI: Add D3 support for PCI bridges in DT based platforms

Message ID 20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org (mailing list archive)
State Superseded
Headers show
Series [v2] PCI: Add D3 support for PCI bridges in DT based platforms | expand

Commit Message

Manivannan Sadhasivam Feb. 14, 2024, 8:48 a.m. UTC 
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 most of the DT based platforms,
option 3 will make the life harder for distro maintainers. Due to this,
runtime PM is also not getting enabled for the bridges.

To fix this, let's make use of the "supports-d3" property [1] in the bridge
DT nodes to enable D3 support for the capable bridges. This will also allow
the capable bridges to support runtime PM, thereby conserving power.

Ideally, D3 support should be enabled by default for the more recent PCI
bridges, but we do not have a sane way to detect them.

[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-pci-bridge.yaml#L31

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
This patch is tested on Qcom SM8450 based development board with an out-of-tree
DT patch.

NOTE: I will submit the DT patches adding this property for applicable bridges
in Qcom SoCs separately.

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
---
 drivers/pci/of.c  | 12 ++++++++++++
 drivers/pci/pci.c |  3 +++
 drivers/pci/pci.h |  6 ++++++
 3 files changed, 21 insertions(+)


---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240131-pcie-qcom-bridge-b6802a9770a3

Best regards,

Comments

Lukas Wunner Feb. 14, 2024, 10:28 a.m. UTC | #1 
On Wed, Feb 14, 2024 at 02:18:31PM +0530, Manivannan Sadhasivam wrote:
> +/**
> + * of_pci_bridge_d3 - Check if the bridge is supporting D3 states or not
> + *
> + * @node: device tree node of the bridge
> + *
> + * Return: True if the bridge is supporting D3 states, False otherwise.

A lot of kernel-doc uses %true and %false.


> +bool of_pci_bridge_d3(struct device_node *node)
> +{
> +	return of_property_read_bool(node, "supports-d3");
> +}

What's the difference between of_property_read_bool() and
of_property_present()?  When should one use which?
The former has 691 occurrences in the tree, the latter 120.
The latter would seem more "literary" / readable here,
but maybe that's just me.


> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1142,6 +1142,9 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>  	if (pci_use_mid_pm())
>  		return false;
>  
> +	if (dev->dev.of_node)
> +		return of_pci_bridge_d3(dev->dev.of_node);
> +
>  	return acpi_pci_bridge_d3(dev);
>  }

This will result in an unnecessary test on non-DT platforms (e.g. ACPI)
whether dev->dev.of_node is set.

Please use dev_of_node() instead of "dev->dev.of_node" so that the
code added here can be optimized away by the compiler on non-DT
platforms (due to the IS_ENABLED(CONFIG_OF)).

Thanks,

Lukas
Manivannan Sadhasivam Feb. 14, 2024, 11:34 a.m. UTC | #2 
On Wed, Feb 14, 2024 at 11:28:37AM +0100, Lukas Wunner wrote:
> On Wed, Feb 14, 2024 at 02:18:31PM +0530, Manivannan Sadhasivam wrote:
> > +/**
> > + * of_pci_bridge_d3 - Check if the bridge is supporting D3 states or not
> > + *
> > + * @node: device tree node of the bridge
> > + *
> > + * Return: True if the bridge is supporting D3 states, False otherwise.
> 
> A lot of kernel-doc uses %true and %false.
> 

Ack.

> 
> > +bool of_pci_bridge_d3(struct device_node *node)
> > +{
> > +	return of_property_read_bool(node, "supports-d3");
> > +}
> 
> What's the difference between of_property_read_bool() and
> of_property_present()?  When should one use which?
> The former has 691 occurrences in the tree, the latter 120.
> The latter would seem more "literary" / readable here,
> but maybe that's just me.
> 

of_property_present() just calls of_property_read_bool() and it is fairly new.
But yeah, the API name itself indicates that it is better suited for the
purpose. Will change it.

> 
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1142,6 +1142,9 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> >  	if (pci_use_mid_pm())
> >  		return false;
> >  
> > +	if (dev->dev.of_node)
> > +		return of_pci_bridge_d3(dev->dev.of_node);
> > +
> >  	return acpi_pci_bridge_d3(dev);
> >  }
> 
> This will result in an unnecessary test on non-DT platforms (e.g. ACPI)
> whether dev->dev.of_node is set.
> 
> Please use dev_of_node() instead of "dev->dev.of_node" so that the
> code added here can be optimized away by the compiler on non-DT
> platforms (due to the IS_ENABLED(CONFIG_OF)).
> 

Sounds good.

- Mani
diff mbox series

Patch

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..77dc14a3c91d 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -786,3 +786,15 @@  u32 of_pci_get_slot_power_limit(struct device_node *node,
 	return slot_power_limit_mw;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
+
+/**
+ * of_pci_bridge_d3 - Check if the bridge is supporting D3 states or not
+ *
+ * @node: device tree node of the bridge
+ *
+ * Return: True if the bridge is supporting D3 states, False otherwise.
+ */
+bool of_pci_bridge_d3(struct device_node *node)
+{
+	return of_property_read_bool(node, "supports-d3");
+}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d8f11a078924..3309c45b656c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1142,6 +1142,9 @@  static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
 	if (pci_use_mid_pm())
 		return false;
 
+	if (dev->dev.of_node)
+		return of_pci_bridge_d3(dev->dev.of_node);
+
 	return acpi_pci_bridge_d3(dev);
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2336a8d1edab..10387461b1fe 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -635,6 +635,7 @@  int of_pci_get_max_link_speed(struct device_node *node);
 u32 of_pci_get_slot_power_limit(struct device_node *node,
 				u8 *slot_power_limit_value,
 				u8 *slot_power_limit_scale);
+bool of_pci_bridge_d3(struct device_node *node);
 int pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -673,6 +674,11 @@  of_pci_get_slot_power_limit(struct device_node *node,
 	return 0;
 }
 
+static inline bool of_pci_bridge_d3(struct device_node *node)
+{
+	return false;
+}
+
 static inline int pci_set_of_node(struct pci_dev *dev) { return 0; }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }