| Message ID | 20240214-pcie-qcom-bridge-v3-1-3a713bbc1fd7@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PCI: Add D3 support for PCI bridges in DT based platforms | expand |
On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > 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. [...] > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Reviewed-by: Lukas Wunner <lukas@wunner.de>
On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > Currently, PCI core will enable D3 support for PCI bridges only when the > following conditions are met: Whenever I read "D3", I first have to figure out whether we're talking about D3hot or D3cold. Please save me the effort :) > 1. Platform is ACPI based > 2. Thunderbolt controller is used > 3. pcie_port_pm=force passed in cmdline Are these joined by "AND" or "OR"? I guess probably "OR"? "... all the following conditions are met" or "... one of the following conditions is met" would clarify this. > 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. Looks like "supports-d3" was added by https://github.com/devicetree-org/dt-schema/commit/4548397d7522. The commit log mentions "platform specific ways", which suggests maybe this is D3cold, since D3hot should be supported via PMCSR without any help from the platform. So I *guess* this really means "platform provides some non-architected way to put devices in D3cold and bring them back to D0"? > 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 This link won't remain accurate as lines are added/removed. The kernel.org cgit allows specific commits (https://git.kernel.org/linus/0dd3ee311255) or line references at specific commits or tags (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94) > 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 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 > --- > drivers/pci/of.c | 12 ++++++++++++ > drivers/pci/pci.c | 3 +++ > drivers/pci/pci.h | 6 ++++++ > 3 files changed, 21 insertions(+) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > +} > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > + 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) { } > > --- > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > Best regards, > -- > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >
On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > Currently, PCI core will enable D3 support for PCI bridges only when the > > following conditions are met: > > Whenever I read "D3", I first have to figure out whether we're talking > about D3hot or D3cold. Please save me the effort :) > Both actually, that's why I used "D3" as in the spec. I should've explicitly mentioned that in the commit message. > > 1. Platform is ACPI based > > 2. Thunderbolt controller is used > > 3. pcie_port_pm=force passed in cmdline > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > "... all the following conditions are met" or "... one of the > following conditions is met" would clarify this. > Will use "one of the..." > > 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. > > Looks like "supports-d3" was added by > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > The commit log mentions "platform specific ways", which suggests maybe > this is D3cold, since D3hot should be supported via PMCSR without any > help from the platform. > > So I *guess* this really means "platform provides some non-architected > way to put devices in D3cold and bring them back to D0"? > By reading the comments and git log of the pci_bridge_d3_possible() function in drivers/pci/pci.c, we can understand that some of the old bridges do not support both D3hot and D3cold. And to differentiate such bridges, platforms have to notify the OS using some ways. ACPI has its own implementation [1] and DT uses "supports-d3" property. And yes, in an ideal world PMCSR should be sufficient for D3hot, but you know the PCI vendors more than me ;) > > 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 > > This link won't remain accurate as lines are added/removed. The > kernel.org cgit allows specific commits > (https://git.kernel.org/linus/0dd3ee311255) or line references at > specific commits or tags > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94) > I'm not aware of such references in github. So I'll reference the commit instead. - Mani [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-acpi.c#n976 > > 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 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 > > --- > > drivers/pci/of.c | 12 ++++++++++++ > > drivers/pci/pci.c | 3 +++ > > drivers/pci/pci.h | 6 ++++++ > > 3 files changed, 21 insertions(+) > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > > +} > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > > + 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) { } > > > > --- > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > > > Best regards, > > -- > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > >
On Wed, Feb 21, 2024 at 10:49:58AM +0530, Manivannan Sadhasivam wrote: > On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > > Currently, PCI core will enable D3 support for PCI bridges only when the > > > following conditions are met: > > > > Whenever I read "D3", I first have to figure out whether we're talking > > about D3hot or D3cold. Please save me the effort :) > > Both actually, that's why I used "D3" as in the spec. I should've explicitly > mentioned that in the commit message. > > > > 1. Platform is ACPI based > > > 2. Thunderbolt controller is used > > > 3. pcie_port_pm=force passed in cmdline > > > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > > > "... all the following conditions are met" or "... one of the > > following conditions is met" would clarify this. > > Will use "one of the..." > > > > 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. > > > > Looks like "supports-d3" was added by > > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > > The commit log mentions "platform specific ways", which suggests maybe > > this is D3cold, since D3hot should be supported via PMCSR without any > > help from the platform. > > > > So I *guess* this really means "platform provides some non-architected > > way to put devices in D3cold and bring them back to D0"? > > By reading the comments and git log of the pci_bridge_d3_possible() > function in drivers/pci/pci.c, we can understand that some of the > old bridges do not support both D3hot and D3cold. And to > differentiate such bridges, platforms have to notify the OS using > some ways. > > ACPI has its own implementation [1] and DT uses "supports-d3" > property. > > And yes, in an ideal world PMCSR should be sufficient for D3hot, but > you know the PCI vendors more than me ;) So it sounds like this is supposed to cover two cases: 1) D3hot doesn't work per spec. This sounds like a hardware defect in the device that should be a quirk based on Vendor/Device ID, not something in DT. I don't actually know if this is common, although there are several existing quirks that mention issues with D3. 2) The platform doesn't support putting the bridge in D3cold and back to D0. I don't understand this either because I assumed DT would describe *hardware*, and "supports-d3" might imply the presence of hardware power control, but doesn't tell us how to operate it, and it must be up to a native driver to know how to do it. These are two vastly different scenarios, and I would really like to untangle them so they aren't conflated. I see that you're extending platform_pci_bridge_d3(), which apparently has that conflation baked into it already, but my personal experience is that this is really hard to maintain. > > > 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 > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-acpi.c#n976 > > > > 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 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 > > > --- > > > drivers/pci/of.c | 12 ++++++++++++ > > > drivers/pci/pci.c | 3 +++ > > > drivers/pci/pci.h | 6 ++++++ > > > 3 files changed, 21 insertions(+) > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > > > +} > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > > > + 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) { } > > > > > > --- > > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > > > > > Best regards, > > > -- > > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > -- > மணிவண்ணன் சதாசிவம்
On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > On Wed, Feb 21, 2024 at 10:49:58AM +0530, Manivannan Sadhasivam wrote: > > On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > > > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > > > Currently, PCI core will enable D3 support for PCI bridges only when the > > > > following conditions are met: > > > > > > Whenever I read "D3", I first have to figure out whether we're talking > > > about D3hot or D3cold. Please save me the effort :) > > > > Both actually, that's why I used "D3" as in the spec. I should've explicitly > > mentioned that in the commit message. > > > > > > 1. Platform is ACPI based > > > > 2. Thunderbolt controller is used > > > > 3. pcie_port_pm=force passed in cmdline > > > > > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > > > > > "... all the following conditions are met" or "... one of the > > > following conditions is met" would clarify this. > > > > Will use "one of the..." > > > > > > 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. > > > > > > Looks like "supports-d3" was added by > > > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > > > The commit log mentions "platform specific ways", which suggests maybe > > > this is D3cold, since D3hot should be supported via PMCSR without any > > > help from the platform. > > > > > > So I *guess* this really means "platform provides some non-architected > > > way to put devices in D3cold and bring them back to D0"? > > > > By reading the comments and git log of the pci_bridge_d3_possible() > > function in drivers/pci/pci.c, we can understand that some of the > > old bridges do not support both D3hot and D3cold. And to > > differentiate such bridges, platforms have to notify the OS using > > some ways. > > > > ACPI has its own implementation [1] and DT uses "supports-d3" > > property. > > > > And yes, in an ideal world PMCSR should be sufficient for D3hot, but > > you know the PCI vendors more than me ;) > > So it sounds like this is supposed to cover two cases: > > 1) D3hot doesn't work per spec. This sounds like a hardware > defect in the device that should be a quirk based on > Vendor/Device ID, not something in DT. I don't actually know if > this is common, although there are several existing quirks that > mention issues with D3. > I'd love to use quirks if we started from that. But right now, quirks are not used and there are multiple checks based on various factors [1], including relying on ACPI. So that's the reason I went with DT based approach. If quirks has to be used now, then it has to be used for both ACPI and DT based platforms. For DT it won't be an issue since nobody bothered until now, but for ACPI, we need to add quirks for all the bridges in the wild which is not feasible. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c#n3116 > 2) The platform doesn't support putting the bridge in D3cold and > back to D0. I don't understand this either because I assumed DT > would describe *hardware*, and "supports-d3" might imply the > presence of hardware power control, but doesn't tell us how to > operate it, and it must be up to a native driver to know how to > do it. > "supports-d3" implies that both D3hot and D3cold works as in the spec and the OS can handle it appropriately. If this is absent, then OS should not transition the bridge to any of the D3 states. I don't understand what is the confusion here. This is similar to what we already have for ACPI (whether or not it is correct is another topic). > These are two vastly different scenarios, and I would really like to > untangle them so they aren't conflated. I see that you're extending > platform_pci_bridge_d3(), which apparently has that conflation baked > into it already, but my personal experience is that this is really > hard to maintain. > I do agree that it is not in a good shape, but there is no better solution other than making use of the DT property. If you have any better idea, please suggest. - Mani > > > > 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 > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-acpi.c#n976 > > > > > > 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 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 > > > > --- > > > > drivers/pci/of.c | 12 ++++++++++++ > > > > drivers/pci/pci.c | 3 +++ > > > > drivers/pci/pci.h | 6 ++++++ > > > > 3 files changed, 21 insertions(+) > > > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > > > > +} > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > > > > + 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) { } > > > > > > > > --- > > > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > > > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > > > > > > > Best regards, > > > > -- > > > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > 1) D3hot doesn't work per spec. This sounds like a hardware > defect in the device that should be a quirk based on > Vendor/Device ID, not something in DT. I don't actually know if > this is common, although there are several existing quirks that > mention issues with D3. My recollection is that putting Root Ports into D3hot on older x86 systems would raise MCEs, which is why pci_bridge_d3_possible() only allows D3hot in cases which are known to work (e.g. Thunderbolt controllers, machines with a recent BIOS). It was a conservative policy chosen to avoid regressions. I don't know if similar issues exist on non-ACPI systems. If they don't exist, platform_pci_bridge_d3() could just return true for all devicetree-based systems. Might be worth testing if any systems can be found which exhibit issues with such a policy. That would obviate the need to specify "supports-d3" in the devicetree. Quite the opposite, ports which are known not to work could be blacklisted. Of course if it turns out that's the majority then whitelisting via "supports-d3" is a better option. > 2) The platform doesn't support putting the bridge in D3cold and > back to D0. I don't understand this either because I assumed DT > would describe *hardware*, and "supports-d3" might imply the > presence of hardware power control, but doesn't tell us how to > operate it, and it must be up to a native driver to know how to > do it. I think we're putting devices into D3hot first before cutting power (i.e. putting them into D3cold), so knowing that D3hot is safe is basically a prerequisite for D3cold. Thanks, Lukas
On Thu, Feb 22, 2024 at 10:40:52AM +0100, Lukas Wunner wrote: > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > 1) D3hot doesn't work per spec. This sounds like a hardware > > defect in the device that should be a quirk based on > > Vendor/Device ID, not something in DT. I don't actually know if > > this is common, although there are several existing quirks that > > mention issues with D3. > > My recollection is that putting Root Ports into D3hot on older x86 > systems would raise MCEs, Color me dubious. I don't know why an MCE should happen unless we tried to access MMIO space on the Root Port or we tried to access downstream devices, and the UR or whatever got turned into MCE. Avoiding D3hot seems like papering over something that we don't fully understand. > which is why pci_bridge_d3_possible() only > allows D3hot in cases which are known to work (e.g. Thunderbolt > controllers, machines with a recent BIOS). It was a conservative > policy chosen to avoid regressions. > > I don't know if similar issues exist on non-ACPI systems. If they > don't exist, platform_pci_bridge_d3() could just return true for > all devicetree-based systems. Might be worth testing if any systems > can be found which exhibit issues with such a policy. That would > obviate the need to specify "supports-d3" in the devicetree. > Quite the opposite, ports which are known not to work could be > blacklisted. Of course if it turns out that's the majority then > whitelisting via "supports-d3" is a better option. > > > 2) The platform doesn't support putting the bridge in D3cold and > > back to D0. I don't understand this either because I assumed DT > > would describe *hardware*, and "supports-d3" might imply the > > presence of hardware power control, but doesn't tell us how to > > operate it, and it must be up to a native driver to know how to > > do it. > > I think we're putting devices into D3hot first before cutting power > (i.e. putting them into D3cold), so knowing that D3hot is safe is > basically a prerequisite for D3cold. > > Thanks, > > Lukas
On Thu, Feb 22, 2024 at 09:36:29AM +0530, Manivannan Sadhasivam wrote: > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > On Wed, Feb 21, 2024 at 10:49:58AM +0530, Manivannan Sadhasivam wrote: > > > On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > > > > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > > > > Currently, PCI core will enable D3 support for PCI bridges only when the > > > > > following conditions are met: > > > > > > > > Whenever I read "D3", I first have to figure out whether we're talking > > > > about D3hot or D3cold. Please save me the effort :) > > > > > > Both actually, that's why I used "D3" as in the spec. I should've explicitly > > > mentioned that in the commit message. > > > > > > > > 1. Platform is ACPI based > > > > > 2. Thunderbolt controller is used > > > > > 3. pcie_port_pm=force passed in cmdline > > > > > > > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > > > > > > > "... all the following conditions are met" or "... one of the > > > > following conditions is met" would clarify this. > > > > > > Will use "one of the..." > > > > > > > > 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. > > > > > > > > Looks like "supports-d3" was added by > > > > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > > > > The commit log mentions "platform specific ways", which suggests maybe > > > > this is D3cold, since D3hot should be supported via PMCSR without any > > > > help from the platform. > > > > > > > > So I *guess* this really means "platform provides some non-architected > > > > way to put devices in D3cold and bring them back to D0"? > > > > > > By reading the comments and git log of the pci_bridge_d3_possible() > > > function in drivers/pci/pci.c, we can understand that some of the > > > old bridges do not support both D3hot and D3cold. And to > > > differentiate such bridges, platforms have to notify the OS using > > > some ways. > > > > > > ACPI has its own implementation [1] and DT uses "supports-d3" > > > property. > > > > > > And yes, in an ideal world PMCSR should be sufficient for D3hot, but > > > you know the PCI vendors more than me ;) > > > > So it sounds like this is supposed to cover two cases: > > > > 1) D3hot doesn't work per spec. This sounds like a hardware > > defect in the device that should be a quirk based on > > Vendor/Device ID, not something in DT. I don't actually know if > > this is common, although there are several existing quirks that > > mention issues with D3. > > > > I'd love to use quirks if we started from that. But right now, quirks are not > used and there are multiple checks based on various factors [1], including > relying on ACPI. So that's the reason I went with DT based approach. > > If quirks has to be used now, then it has to be used for both ACPI and DT based > platforms. For DT it won't be an issue since nobody bothered until now, but for > ACPI, we need to add quirks for all the bridges in the wild which is not > feasible. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c#n3116 > > > 2) The platform doesn't support putting the bridge in D3cold and > > back to D0. I don't understand this either because I assumed DT > > would describe *hardware*, and "supports-d3" might imply the > > presence of hardware power control, but doesn't tell us how to > > operate it, and it must be up to a native driver to know how to > > do it. > > "supports-d3" implies that both D3hot and D3cold works as in the > spec and the OS can handle it appropriately. If this is absent, then > OS should not transition the bridge to any of the D3 states. I don't > understand what is the confusion here. This is similar to what we > already have for ACPI (whether or not it is correct is another > topic). What does "the OS can handle it appropriately" mean? Whatever it means, it sounds like a property of the OS, not a property of the device. I don't know what "D3cold works as in the spec" means, either. The spec says how D3cold affects internal device state, but it doesn't say anything about how to put devices in D3cold or back in D0. > > These are two vastly different scenarios, and I would really like to > > untangle them so they aren't conflated. I see that you're extending > > platform_pci_bridge_d3(), which apparently has that conflation baked > > into it already, but my personal experience is that this is really > > hard to maintain. > > I do agree that it is not in a good shape, but there is no better > solution other than making use of the DT property. If you have any > better idea, please suggest. The longer this goes on the worse shape we are in because we're always adding new special cases. The fundamental problem I have is that "supports-d3" doesn't say anything specific other than "current Linux can put the device in D3hot or D3cold and get it back out again". I think DT should tell us characteristics of the device or the platform, e.g., "PMCSR doesn't work to enter/leave D3hot on this device" or "regulator X controls main power to the device to enter/leave D3cold". But right now it sounds like a mixture of "PMCSR works correctly to enter/leave D3hot" and "some unspecified software can control main power to this device". Putting devices in D3cold and back in D0 needs some kind of platform support like ACPI methods or a native power management driver that knows how to control power on a specific platform. That's completely different from D3hot, where the PCI spec tells us all we need to know. > > > > > 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 > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-acpi.c#n976 > > > > > > > > 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 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 > > > > > --- > > > > > drivers/pci/of.c | 12 ++++++++++++ > > > > > drivers/pci/pci.c | 3 +++ > > > > > drivers/pci/pci.h | 6 ++++++ > > > > > 3 files changed, 21 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > > > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > > > > > +} > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > > > > > + 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) { } > > > > > > > > > > --- > > > > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > > > > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > > > > > > > > > Best regards, > > > > > -- > > > > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
On Mon, Feb 26, 2024 at 05:39:30PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 22, 2024 at 09:36:29AM +0530, Manivannan Sadhasivam wrote: > > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > > On Wed, Feb 21, 2024 at 10:49:58AM +0530, Manivannan Sadhasivam wrote: > > > > On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > > > > > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > > > > > Currently, PCI core will enable D3 support for PCI bridges only when the > > > > > > following conditions are met: > > > > > > > > > > Whenever I read "D3", I first have to figure out whether we're talking > > > > > about D3hot or D3cold. Please save me the effort :) > > > > > > > > Both actually, that's why I used "D3" as in the spec. I should've explicitly > > > > mentioned that in the commit message. > > > > > > > > > > 1. Platform is ACPI based > > > > > > 2. Thunderbolt controller is used > > > > > > 3. pcie_port_pm=force passed in cmdline > > > > > > > > > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > > > > > > > > > "... all the following conditions are met" or "... one of the > > > > > following conditions is met" would clarify this. > > > > > > > > Will use "one of the..." > > > > > > > > > > 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. > > > > > > > > > > Looks like "supports-d3" was added by > > > > > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > > > > > The commit log mentions "platform specific ways", which suggests maybe > > > > > this is D3cold, since D3hot should be supported via PMCSR without any > > > > > help from the platform. > > > > > > > > > > So I *guess* this really means "platform provides some non-architected > > > > > way to put devices in D3cold and bring them back to D0"? > > > > > > > > By reading the comments and git log of the pci_bridge_d3_possible() > > > > function in drivers/pci/pci.c, we can understand that some of the > > > > old bridges do not support both D3hot and D3cold. And to > > > > differentiate such bridges, platforms have to notify the OS using > > > > some ways. > > > > > > > > ACPI has its own implementation [1] and DT uses "supports-d3" > > > > property. > > > > > > > > And yes, in an ideal world PMCSR should be sufficient for D3hot, but > > > > you know the PCI vendors more than me ;) > > > > > > So it sounds like this is supposed to cover two cases: > > > > > > 1) D3hot doesn't work per spec. This sounds like a hardware > > > defect in the device that should be a quirk based on > > > Vendor/Device ID, not something in DT. I don't actually know if > > > this is common, although there are several existing quirks that > > > mention issues with D3. > > > > > > > I'd love to use quirks if we started from that. But right now, quirks are not > > used and there are multiple checks based on various factors [1], including > > relying on ACPI. So that's the reason I went with DT based approach. > > > > If quirks has to be used now, then it has to be used for both ACPI and DT based > > platforms. For DT it won't be an issue since nobody bothered until now, but for > > ACPI, we need to add quirks for all the bridges in the wild which is not > > feasible. > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c#n3116 > > > > > 2) The platform doesn't support putting the bridge in D3cold and > > > back to D0. I don't understand this either because I assumed DT > > > would describe *hardware*, and "supports-d3" might imply the > > > presence of hardware power control, but doesn't tell us how to > > > operate it, and it must be up to a native driver to know how to > > > do it. > > > > "supports-d3" implies that both D3hot and D3cold works as in the > > spec and the OS can handle it appropriately. If this is absent, then > > OS should not transition the bridge to any of the D3 states. I don't > > understand what is the confusion here. This is similar to what we > > already have for ACPI (whether or not it is correct is another > > topic). > > What does "the OS can handle it appropriately" mean? Whatever it > means, it sounds like a property of the OS, not a property of the > device. > "appropiately" means as per the PCIe spec. > I don't know what "D3cold works as in the spec" means, either. The > spec says how D3cold affects internal device state, but it doesn't say > anything about how to put devices in D3cold or back in D0. > > > > These are two vastly different scenarios, and I would really like to > > > untangle them so they aren't conflated. I see that you're extending > > > platform_pci_bridge_d3(), which apparently has that conflation baked > > > into it already, but my personal experience is that this is really > > > hard to maintain. > > > > I do agree that it is not in a good shape, but there is no better > > solution other than making use of the DT property. If you have any > > better idea, please suggest. > > The longer this goes on the worse shape we are in because we're always > adding new special cases. > > The fundamental problem I have is that "supports-d3" doesn't say > anything specific other than "current Linux can put the device in > D3hot or D3cold and get it back out again". I think DT should tell us > characteristics of the device or the platform, e.g., "PMCSR doesn't > work to enter/leave D3hot on this device" or "regulator X controls > main power to the device to enter/leave D3cold". > > But right now it sounds like a mixture of "PMCSR works correctly to > enter/leave D3hot" and "some unspecified software can control main > power to this device". > > Putting devices in D3cold and back in D0 needs some kind of platform > support like ACPI methods or a native power management driver that > knows how to control power on a specific platform. That's completely > different from D3hot, where the PCI spec tells us all we need to know. > Ok, I got the issue. TBH, I added the device tree property based on the existing quirks for the ACPI devices. But none of the DT based platforms I'm aware of (even the legacy Qcom MSM8996 chipset released in early 2016) doesn't have any issue with D3hot. But I'm just nervous to assume it is the case for all the DT based platforms in the wild. But to proceed further, what is your preference? Should we ammend the DT property to make it explicit that the propery only focuses on the D3hot capability of the bridge and it works as per the spec (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI platforms? We can add quirks for the bridges later on if we happen to receive any bug report. - Mani > > > > > > 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 > > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-acpi.c#n976 > > > > > > > > > > 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 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 > > > > > > --- > > > > > > drivers/pci/of.c | 12 ++++++++++++ > > > > > > drivers/pci/pci.c | 3 +++ > > > > > > drivers/pci/pci.h | 6 ++++++ > > > > > > 3 files changed, 21 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > > > > index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); > > > > > > +} > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > > index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) > > > > > > + 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) { } > > > > > > > > > > > > --- > > > > > > base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d > > > > > > change-id: 20240131-pcie-qcom-bridge-b6802a9770a3 > > > > > > > > > > > > Best regards, > > > > > > -- > > > > > > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > > > -- > > மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 01:00:57PM +0530, Manivannan Sadhasivam wrote: > On Mon, Feb 26, 2024 at 05:39:30PM -0600, Bjorn Helgaas wrote: > > On Thu, Feb 22, 2024 at 09:36:29AM +0530, Manivannan Sadhasivam wrote: > > > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > > > On Wed, Feb 21, 2024 at 10:49:58AM +0530, Manivannan Sadhasivam wrote: > > > > > On Tue, Feb 20, 2024 at 04:02:40PM -0600, Bjorn Helgaas wrote: > > > > > > On Wed, Feb 14, 2024 at 05:16:09PM +0530, Manivannan Sadhasivam wrote: > > > > > > > Currently, PCI core will enable D3 support for PCI bridges only when the > > > > > > > following conditions are met: > > > > > > > > > > > > Whenever I read "D3", I first have to figure out whether we're talking > > > > > > about D3hot or D3cold. Please save me the effort :) > > > > > > > > > > Both actually, that's why I used "D3" as in the spec. I should've explicitly > > > > > mentioned that in the commit message. > > > > > > > > > > > > 1. Platform is ACPI based > > > > > > > 2. Thunderbolt controller is used > > > > > > > 3. pcie_port_pm=force passed in cmdline > > > > > > > > > > > > Are these joined by "AND" or "OR"? I guess probably "OR"? > > > > > > > > > > > > "... all the following conditions are met" or "... one of the > > > > > > following conditions is met" would clarify this. > > > > > > > > > > Will use "one of the..." > > > > > > > > > > > > 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. > > > > > > > > > > > > Looks like "supports-d3" was added by > > > > > > https://github.com/devicetree-org/dt-schema/commit/4548397d7522. > > > > > > The commit log mentions "platform specific ways", which suggests maybe > > > > > > this is D3cold, since D3hot should be supported via PMCSR without any > > > > > > help from the platform. > > > > > > > > > > > > So I *guess* this really means "platform provides some non-architected > > > > > > way to put devices in D3cold and bring them back to D0"? > > > > > > > > > > By reading the comments and git log of the pci_bridge_d3_possible() > > > > > function in drivers/pci/pci.c, we can understand that some of the > > > > > old bridges do not support both D3hot and D3cold. And to > > > > > differentiate such bridges, platforms have to notify the OS using > > > > > some ways. > > > > > > > > > > ACPI has its own implementation [1] and DT uses "supports-d3" > > > > > property. > > > > > > > > > > And yes, in an ideal world PMCSR should be sufficient for D3hot, but > > > > > you know the PCI vendors more than me ;) > > > > > > > > So it sounds like this is supposed to cover two cases: > > > > > > > > 1) D3hot doesn't work per spec. This sounds like a hardware > > > > defect in the device that should be a quirk based on > > > > Vendor/Device ID, not something in DT. I don't actually know if > > > > this is common, although there are several existing quirks that > > > > mention issues with D3. > > > > > > > > > > I'd love to use quirks if we started from that. But right now, quirks are not > > > used and there are multiple checks based on various factors [1], including > > > relying on ACPI. So that's the reason I went with DT based approach. > > > > > > If quirks has to be used now, then it has to be used for both ACPI and DT based > > > platforms. For DT it won't be an issue since nobody bothered until now, but for > > > ACPI, we need to add quirks for all the bridges in the wild which is not > > > feasible. > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci.c#n3116 > > > > > > > 2) The platform doesn't support putting the bridge in D3cold and > > > > back to D0. I don't understand this either because I assumed DT > > > > would describe *hardware*, and "supports-d3" might imply the > > > > presence of hardware power control, but doesn't tell us how to > > > > operate it, and it must be up to a native driver to know how to > > > > do it. > > > > > > "supports-d3" implies that both D3hot and D3cold works as in the > > > spec and the OS can handle it appropriately. If this is absent, then > > > OS should not transition the bridge to any of the D3 states. I don't > > > understand what is the confusion here. This is similar to what we > > > already have for ACPI (whether or not it is correct is another > > > topic). > > > > What does "the OS can handle it appropriately" mean? Whatever it > > means, it sounds like a property of the OS, not a property of the > > device. > > "appropiately" means as per the PCIe spec. > > > I don't know what "D3cold works as in the spec" means, either. The > > spec says how D3cold affects internal device state, but it doesn't say > > anything about how to put devices in D3cold or back in D0. > > > > > > These are two vastly different scenarios, and I would really like to > > > > untangle them so they aren't conflated. I see that you're extending > > > > platform_pci_bridge_d3(), which apparently has that conflation baked > > > > into it already, but my personal experience is that this is really > > > > hard to maintain. > > > > > > I do agree that it is not in a good shape, but there is no better > > > solution other than making use of the DT property. If you have any > > > better idea, please suggest. > > > > The longer this goes on the worse shape we are in because we're always > > adding new special cases. > > > > The fundamental problem I have is that "supports-d3" doesn't say > > anything specific other than "current Linux can put the device in > > D3hot or D3cold and get it back out again". I think DT should tell us > > characteristics of the device or the platform, e.g., "PMCSR doesn't > > work to enter/leave D3hot on this device" or "regulator X controls > > main power to the device to enter/leave D3cold". > > > > But right now it sounds like a mixture of "PMCSR works correctly to > > enter/leave D3hot" and "some unspecified software can control main > > power to this device". > > > > Putting devices in D3cold and back in D0 needs some kind of platform > > support like ACPI methods or a native power management driver that > > knows how to control power on a specific platform. That's completely > > different from D3hot, where the PCI spec tells us all we need to know. > > Ok, I got the issue. TBH, I added the device tree property based on > the existing quirks for the ACPI devices. But none of the DT based > platforms I'm aware of (even the legacy Qcom MSM8996 chipset > released in early 2016) doesn't have any issue with D3hot. But I'm > just nervous to assume it is the case for all the DT based platforms > in the wild. > > But to proceed further, what is your preference? Should we ammend > the DT property to make it explicit that the propery only focuses on > the D3hot capability of the bridge and it works as per the spec > (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI > platforms? > > We can add quirks for the bridges later on if we happen to receive > any bug report. I would assume all devices support D3hot via PMCSR per spec. We can add quirks if we discover something that doesn't. If we add annotations that "this device works correctly", we're digging a hole for ourselves because it's impossible to remove those annotations and they complicate all future maintenance. Bjorn
On Tue, Feb 27, 2024 at 10:25:35AM -0600, Bjorn Helgaas wrote: [...] > > Ok, I got the issue. TBH, I added the device tree property based on > > the existing quirks for the ACPI devices. But none of the DT based > > platforms I'm aware of (even the legacy Qcom MSM8996 chipset > > released in early 2016) doesn't have any issue with D3hot. But I'm > > just nervous to assume it is the case for all the DT based platforms > > in the wild. > > > > But to proceed further, what is your preference? Should we ammend > > the DT property to make it explicit that the propery only focuses on > > the D3hot capability of the bridge and it works as per the spec > > (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI > > platforms? > > > > We can add quirks for the bridges later on if we happen to receive > > any bug report. > > I would assume all devices support D3hot via PMCSR per spec. We can > add quirks if we discover something that doesn't. > When you say "all devices", are you referring to bridges in DT platforms or the bridges across all platforms? - Mani > If we add annotations that "this device works correctly", we're > digging a hole for ourselves because it's impossible to remove those > annotations and they complicate all future maintenance. > > Bjorn
On Tue, Feb 27, 2024 at 10:38:40PM +0530, Manivannan Sadhasivam wrote: > On Tue, Feb 27, 2024 at 10:25:35AM -0600, Bjorn Helgaas wrote: > > [...] > > > > Ok, I got the issue. TBH, I added the device tree property based on > > > the existing quirks for the ACPI devices. But none of the DT based > > > platforms I'm aware of (even the legacy Qcom MSM8996 chipset > > > released in early 2016) doesn't have any issue with D3hot. But I'm > > > just nervous to assume it is the case for all the DT based platforms > > > in the wild. > > > > > > But to proceed further, what is your preference? Should we ammend > > > the DT property to make it explicit that the propery only focuses on > > > the D3hot capability of the bridge and it works as per the spec > > > (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI > > > platforms? > > > > > > We can add quirks for the bridges later on if we happen to receive > > > any bug report. > > > > I would assume all devices support D3hot via PMCSR per spec. We can > > add quirks if we discover something that doesn't. > > When you say "all devices", are you referring to bridges in DT > platforms or the bridges across all platforms? This patch is only concerned with DT, so that's what I'm commenting on here. I don't know how to untangle the question of ACPI systems. This patch affects platform_pci_bridge_d3(), so just based on the "platform" in the function name, I would expect it to be concerned with the D3cold case and whether the platform supports controlling main power. It looks like this patch says "we can put devices in D3cold if DT has 'supports-d3'". But I don't know how to make sense of that because that requires (a) platform hardware to control main power and (b) software that knows how to use that hardware. Wouldn't this require a little more DT description, like "regulator X controls main power for this bridge"? And then an OS would only be able to actually use D3cold if it knows how to *operate* the regulator, and it doesn't seem like DT could answer that. > > If we add annotations that "this device works correctly", we're > > digging a hole for ourselves because it's impossible to remove those > > annotations and they complicate all future maintenance. > > > > Bjorn > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 11:37:05AM -0600, Bjorn Helgaas wrote: > On Tue, Feb 27, 2024 at 10:38:40PM +0530, Manivannan Sadhasivam wrote: > > On Tue, Feb 27, 2024 at 10:25:35AM -0600, Bjorn Helgaas wrote: > > > > [...] > > > > > > Ok, I got the issue. TBH, I added the device tree property based on > > > > the existing quirks for the ACPI devices. But none of the DT based > > > > platforms I'm aware of (even the legacy Qcom MSM8996 chipset > > > > released in early 2016) doesn't have any issue with D3hot. But I'm > > > > just nervous to assume it is the case for all the DT based platforms > > > > in the wild. > > > > > > > > But to proceed further, what is your preference? Should we ammend > > > > the DT property to make it explicit that the propery only focuses on > > > > the D3hot capability of the bridge and it works as per the spec > > > > (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI > > > > platforms? > > > > > > > > We can add quirks for the bridges later on if we happen to receive > > > > any bug report. > > > > > > I would assume all devices support D3hot via PMCSR per spec. We can > > > add quirks if we discover something that doesn't. > > > > When you say "all devices", are you referring to bridges in DT > > platforms or the bridges across all platforms? > > This patch is only concerned with DT, so that's what I'm commenting on > here. I don't know how to untangle the question of ACPI systems. > Ok, I just wanted to confirm. > This patch affects platform_pci_bridge_d3(), so just based on the > "platform" in the function name, I would expect it to be concerned > with the D3cold case and whether the platform supports controlling > main power. > > It looks like this patch says "we can put devices in D3cold if DT has > 'supports-d3'". But I don't know how to make sense of that because > that requires (a) platform hardware to control main power and (b) > software that knows how to use that hardware. Wouldn't this require a > little more DT description, like "regulator X controls main power for > this bridge"? And then an OS would only be able to actually use > D3cold if it knows how to *operate* the regulator, and it doesn't seem > like DT could answer that. > Fair point. And for most of the DT based platforms, there is no dedicated power supply for the bridge described in DT. So transitioning the bridge to D3cold is not entirely possible in the OS. Since we concluded that enabling D3hot for all bridges in DT platforms is the way to go, I'll drop supporting the DT property in next version. I'll also remove it from the binding. - Mani
On Wed, Feb 28, 2024 at 12:10:15AM +0530, Manivannan Sadhasivam wrote: > On Tue, Feb 27, 2024 at 11:37:05AM -0600, Bjorn Helgaas wrote: > > On Tue, Feb 27, 2024 at 10:38:40PM +0530, Manivannan Sadhasivam wrote: > > > On Tue, Feb 27, 2024 at 10:25:35AM -0600, Bjorn Helgaas wrote: > > > > > > [...] > > > > > > > > Ok, I got the issue. TBH, I added the device tree property based on > > > > > the existing quirks for the ACPI devices. But none of the DT based > > > > > platforms I'm aware of (even the legacy Qcom MSM8996 chipset > > > > > released in early 2016) doesn't have any issue with D3hot. But I'm > > > > > just nervous to assume it is the case for all the DT based platforms > > > > > in the wild. > > > > > > > > > > But to proceed further, what is your preference? Should we ammend > > > > > the DT property to make it explicit that the propery only focuses on > > > > > the D3hot capability of the bridge and it works as per the spec > > > > > (PMCSR) or bite the bullet and enable D3hot for all the non-ACPI > > > > > platforms? > > > > > > > > > > We can add quirks for the bridges later on if we happen to receive > > > > > any bug report. > > > > > > > > I would assume all devices support D3hot via PMCSR per spec. We can > > > > add quirks if we discover something that doesn't. > > > > > > When you say "all devices", are you referring to bridges in DT > > > platforms or the bridges across all platforms? > > > > This patch is only concerned with DT, so that's what I'm commenting on > > here. I don't know how to untangle the question of ACPI systems. > > Ok, I just wanted to confirm. > > > This patch affects platform_pci_bridge_d3(), so just based on the > > "platform" in the function name, I would expect it to be concerned > > with the D3cold case and whether the platform supports controlling > > main power. > > > > It looks like this patch says "we can put devices in D3cold if DT has > > 'supports-d3'". But I don't know how to make sense of that because > > that requires (a) platform hardware to control main power and (b) > > software that knows how to use that hardware. Wouldn't this require a > > little more DT description, like "regulator X controls main power for > > this bridge"? And then an OS would only be able to actually use > > D3cold if it knows how to *operate* the regulator, and it doesn't seem > > like DT could answer that. > > Fair point. And for most of the DT based platforms, there is no > dedicated power supply for the bridge described in DT. So > transitioning the bridge to D3cold is not entirely possible in the > OS. > > Since we concluded that enabling D3hot for all bridges in DT > platforms is the way to go, I'll drop supporting the DT property in > next version. > > I'll also remove it from the binding. Sounds good, thanks!
On Thu, Feb 22, 2024 at 10:40:52AM +0100, Lukas Wunner wrote: > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > 1) D3hot doesn't work per spec. This sounds like a hardware > > defect in the device that should be a quirk based on > > Vendor/Device ID, not something in DT. I don't actually know if > > this is common, although there are several existing quirks that > > mention issues with D3. > > My recollection is that putting Root Ports into D3hot on older x86 > systems would raise MCEs, which is why pci_bridge_d3_possible() only > allows D3hot in cases which are known to work (e.g. Thunderbolt > controllers, machines with a recent BIOS). It was a conservative > policy chosen to avoid regressions. > So pci_bridge_d3_possible() is only checking for D3hot capability? If so, I'd rename it to pci_bridge_d3hot_possible() and also 'bridge_d3' to 'bridge_d3hot' to make it explicit. Since the default value of 'd3cold_allowed' is true, I believe the code expects all devices to support D0 and D3cold. Please correct me if I'm wrong. - Mani > I don't know if similar issues exist on non-ACPI systems. If they > don't exist, platform_pci_bridge_d3() could just return true for > all devicetree-based systems. Might be worth testing if any systems > can be found which exhibit issues with such a policy. That would > obviate the need to specify "supports-d3" in the devicetree. > Quite the opposite, ports which are known not to work could be > blacklisted. Of course if it turns out that's the majority then > whitelisting via "supports-d3" is a better option. > > > > 2) The platform doesn't support putting the bridge in D3cold and > > back to D0. I don't understand this either because I assumed DT > > would describe *hardware*, and "supports-d3" might imply the > > presence of hardware power control, but doesn't tell us how to > > operate it, and it must be up to a native driver to know how to > > do it. > > I think we're putting devices into D3hot first before cutting power > (i.e. putting them into D3cold), so knowing that D3hot is safe is > basically a prerequisite for D3cold. > > Thanks, > > Lukas
On Tue, Mar 05, 2024 at 09:55:37PM +0530, Manivannan Sadhasivam wrote: > On Thu, Feb 22, 2024 at 10:40:52AM +0100, Lukas Wunner wrote: > > On Wed, Feb 21, 2024 at 12:20:00PM -0600, Bjorn Helgaas wrote: > > > 1) D3hot doesn't work per spec. This sounds like a hardware > > > defect in the device that should be a quirk based on > > > Vendor/Device ID, not something in DT. I don't actually know if > > > this is common, although there are several existing quirks that > > > mention issues with D3. > > > > My recollection is that putting Root Ports into D3hot on older x86 > > systems would raise MCEs, which is why pci_bridge_d3_possible() only > > allows D3hot in cases which are known to work (e.g. Thunderbolt > > controllers, machines with a recent BIOS). It was a conservative > > policy chosen to avoid regressions. > > So pci_bridge_d3_possible() is only checking for D3hot capability? > If so, I'd rename it to pci_bridge_d3hot_possible() and also > 'bridge_d3' to 'bridge_d3hot' to make it explicit. Every device is required to support D3hot (and D3cold), so I think "d3_possible" and "d3hot_possible" are not very descriptive since they should always be *possible*. pci_bridge_d3_possible() seems to be more about whether hotplug and power management events work in D3hot and maybe some firmware coordination and validation concerns. > Since the default value of 'd3cold_allowed' is true, I believe the > code expects all devices to support D0 and D3cold. Please correct me > if I'm wrong. D3cold means "no main power", so every device "supports" that situation. The only time 'd3cold_allowed' can be false is when a user has set it to false via sysfs, so I think it only reflects an administrative policy choice. I think the important question for the code is whether software can remove and restore main power and maybe something about what hotplug events or PME can be reported, and I have a really hard time following that decision path. Bjorn
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 51e3dd0ea5ab..24b0107802af 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_present(node, "supports-d3"); +} diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d8f11a078924..8678fba092bb 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_of_node(&dev->dev)) + 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) { }
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 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 --- 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,