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

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

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

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

On Mon, Aug 05, 2024 at 07:05:55PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 02, 2024 at 12:13:31PM +0200, Lukas Wunner wrote:
> > The PCI core cannot put devices into D3cold without help from the
> > platform.  Checking whether D3cold is possible (or allowed or
> > whatever) thus requires asking platform support code via
> > platform_pci_power_manageable(), platform_pci_choose_state() etc.
> > 
> > I think patch [3/4] is a little confusing because it creates
> > infrastructure to decide whether D3cold is supported (allowed?)
> > but we already have that in the platform_pci_*() functions.
> > So I'm not sure if patch [3/4] adds value.  I think generally
> > speaking if D3hot isn't possible (allowed?), D3cold is assumed
> > to not be possible either.
> 
> Why? D3Hot is useful for runtime PM and if the platform doesn't want to do
> runtime PM, it can always skip D3Hot (not ideal though).

AFAICS we always program the device to go to D3hot and the platform
then cuts power, thereby putting it into D3cold.  So D3hot is never
skipped.  See __pci_set_power_state():

	if (state == PCI_D3cold) {
		/*
		 * To put the device in D3cold, put it into D3hot in the native
		 * way, then put it into D3cold using platform ops.
		 */
		error = pci_set_low_power_state(dev, PCI_D3hot, locked);

		if (pci_platform_power_transition(dev, PCI_D3cold))
			return error;

Thanks,

Lukas

On Tue, Aug 06, 2024 at 06:11:07PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> > AFAICS we always program the device to go to D3hot and the platform
> > then cuts power, thereby putting it into D3cold.  So D3hot is never
> > skipped.  See __pci_set_power_state():
> > 
> > 	if (state == PCI_D3cold) {
> > 		/*
> > 		 * To put the device in D3cold, put it into D3hot in the native
> > 		 * way, then put it into D3cold using platform ops.
> > 		 */
> > 		error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > 
> > 		if (pci_platform_power_transition(dev, PCI_D3cold))
> > 			return error;
> > 
> 
> This is applicable only to pci_set_power_state(), but AFAIK PCIe spec
> doesn't mandate switching to D3Hot for entering D3Cold.

Per PCI Bus Power Management Interface Specification r1.2 sec 5.5 fig 5-1,
the only supported state transition to D3cold is from D3hot.

Per PCIe r6.2 sec 5.2, "PM is compatible with the PCI Bus Power Management
Interface Specification".

Granted, PCI-PM is an ancient spec, so I think anyone can be forgiven
for not knowing its intricacies off-the-cuff. :)


> So the PCIe host controller drivers (especically non-ACPI platforms)
> may just send PME_Turn_Off followed by removing the slot power
> (which again is not controlled by pci_set_power_state())
> as there are no non-ACPI related hooks as of now.

Ideally, devicetree-based platforms should be brought into the
platform_pci_*() fold to align them with ACPI and get common
behavior across all platforms.

Thanks,

Lukas

On Tue, Aug 06, 2024 at 08:09:18PM +0530, Manivannan Sadhasivam wrote:
> Regarding your comment on patch 3/4, we already have the sysfs attribute
> to control whether the device can be put into D3Cold or not and that is
> directly coming from userspace. So there is no guarantee to assume that
> D3Hot support is considered.

If a device is not allowed to be suspended to D3cold, it will only be
suspended to D3hot.

If a port is put into anything deeper than D0, its secondary bus is
no longer in B0 (PCI-PM r1.2 table 6-1) and children are inaccessible,
so they're "effectively" in D3cold.  Hence if a device cannot be
suspended to D3cold, it will block all parent bridges from being
suspended.  That's what the logic in pci_bridge_d3_update() and
pci_dev_check_d3cold() is doing.

The d3cold_allowed attribute in sysfs is just one of several reasons
why a device may not go to D3cold (see pci_dev_check_d3cold() for
details).

The d3cold_allowed attribute was originally intended to disable D3cold
on devices where it was known to not work.  Nowadays this should all
be handled automatically, which is why we've discussed moving the
attribute to debugfs:

https://lore.kernel.org/all/20230918132424.GA11357@wunner.de/
https://lore.kernel.org/all/20231002181025.82746-1-mario.limonciello@amd.com/

Thanks,

Lukas

On Tue, Aug 6, 2024 at 7:39 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Aug 06, 2024 at 03:02:39PM +0200, Lukas Wunner wrote:
> > On Tue, Aug 06, 2024 at 06:11:07PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Aug 06, 2024 at 08:53:01AM +0200, Lukas Wunner wrote:
> > > > AFAICS we always program the device to go to D3hot and the platform
> > > > then cuts power, thereby putting it into D3cold.  So D3hot is never
> > > > skipped.  See __pci_set_power_state():
> > > >
> > > >   if (state == PCI_D3cold) {
> > > >           /*
> > > >            * To put the device in D3cold, put it into D3hot in the native
> > > >            * way, then put it into D3cold using platform ops.
> > > >            */
> > > >           error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > > >
> > > >           if (pci_platform_power_transition(dev, PCI_D3cold))
> > > >                   return error;
> > > >
> > >
> > > This is applicable only to pci_set_power_state(), but AFAIK PCIe spec
> > > doesn't mandate switching to D3Hot for entering D3Cold.
> >
> > Per PCI Bus Power Management Interface Specification r1.2 sec 5.5 fig 5-1,
> > the only supported state transition to D3cold is from D3hot.
> >
> > Per PCIe r6.2 sec 5.2, "PM is compatible with the PCI Bus Power Management
> > Interface Specification".
> >
> > Granted, PCI-PM is an ancient spec, so I think anyone can be forgiven
> > for not knowing its intricacies off-the-cuff. :)
> >
>
> Ah, the grand old PCI-PM... I don't remember the last time I looked into it :)
>
> >
> > > So the PCIe host controller drivers (especically non-ACPI platforms)
> > > may just send PME_Turn_Off followed by removing the slot power
> > > (which again is not controlled by pci_set_power_state())
> > > as there are no non-ACPI related hooks as of now.
> >
> > Ideally, devicetree-based platforms should be brought into the
> > platform_pci_*() fold to align them with ACPI and get common
> > behavior across all platforms.
> >
>
> Yeah, that would be the ideal case. Unfortunately, there is no ideal ground for
> DT :/ We do not even have the supplies populated properly. But with the advent
> of power sequencing framework, I think this can be fixed.
>

Looking in acpi_pci_bridge_d3(), it has several checkings about
whether d3 is supported, including reading power_manageable flag
(acpi_device_power_manageable) and reading the root port property.
For DT, does it make sense to have a chosen property about this?

> Regarding your comment on patch 3/4, we already have the sysfs attribute to
> control whether the device can be put into D3Cold or not and that is directly
> coming from userspace. So there is no guarantee to assume that D3Hot support is
> considered.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

On Tue, Aug 06, 2024 at 10:20:39PM +0200, Lukas Wunner wrote:
> On Tue, Aug 06, 2024 at 08:09:18PM +0530, Manivannan Sadhasivam wrote:
> > Regarding your comment on patch 3/4, we already have the sysfs attribute
> > to control whether the device can be put into D3Cold or not and that is
> > directly coming from userspace. So there is no guarantee to assume that
> > D3Hot support is considered.
> 
> If a device is not allowed to be suspended to D3cold, it will only be
> suspended to D3hot.
> 
> If a port is put into anything deeper than D0, its secondary bus is
> no longer in B0 (PCI-PM r1.2 table 6-1) and children are inaccessible,
> so they're "effectively" in D3cold.  Hence if a device cannot be
> suspended to D3cold, it will block all parent bridges from being
> suspended.  That's what the logic in pci_bridge_d3_update() and
> pci_dev_check_d3cold() is doing.
> 

Agree.

But patch 3/4 is mostly based on the suggestion from Bjorn [1] for earlier
revision. He specifically mentioned that the platform_pci_bridge_d3() function
doesn't differentiate between D3Hot and D3Cold and that's why I splitted them:

"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 agree with your point that if D3Hot is not possible, then D3Cold is also not
possible as per the PCI PM reference you quoted. But here, D3Hot is possible,
but D3Cold is not. And platform_pci_power_manageable(),
platform_pci_choose_state() are already returning false for DT platforms.

So if 'true' is returned from platform_pci_bridge_d3(), then it implies that
D3Cold is also supported, but it doesn't on DT platforms. So a split seems to be
necessary IMO.

- Mani

[1] https://lore.kernel.org/linux-pci/20240221182000.GA1533634@bhelgaas/

> The d3cold_allowed attribute in sysfs is just one of several reasons
> why a device may not go to D3cold (see pci_dev_check_d3cold() for
> details).
> 
> The d3cold_allowed attribute was originally intended to disable D3cold
> on devices where it was known to not work.  Nowadays this should all
> be handled automatically, which is why we've discussed moving the
> attribute to debugfs:
> 
> https://lore.kernel.org/all/20230918132424.GA11357@wunner.de/
> https://lore.kernel.org/all/20231002181025.82746-1-mario.limonciello@amd.com/
> 
> Thanks,
> 
> Lukas

Hi Manivannan,

Dredging up an old one, but it seems like there was almost consensus on
this patch, and yet it stalled because the series does too much. I'm
interested in reviving it, but I also have some thoughts on the
usability.

On Fri, Aug 02, 2024 at 11:25:03AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Unlike ACPI based platforms, there are no known issues with D3Hot for the
> PCI bridges in the Devicetree based platforms. So let's allow the PCI
> bridges to go to D3Hot during runtime. It should be noted that the bridges
> need to be defined in Devicetree for this to work.

IMO, it's not an amazing idea to key off the presence of a bridge DT
node for this. AFAIK, that's not really required for most other things
(especially if we're not mapping legacy INTx support), and many
platforms I work with do not define a bridge node. But they do use DT,
and I'd like to be able to suspend their bridges.

Personally, I'd choose to match the same requirements as used by
devm_pci_alloc_host_bridge() -> devm_of_pci_bridge_init() -- that the
parent device under which the host bridge is created has an of_node.
Code sample below.

> Currently, D3Cold is not allowed since Vcc supply which is required for
> transitioning the device to D3Cold is not exposed on all Devicetree based
> platforms.
> 
> Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/pci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c7a4f961ec28..bc1e1ca673f1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2992,6 +2992,18 @@ static bool pci_bridge_d3_allowed(struct pci_dev *bridge, pci_power_t state)
>  		if (pci_bridge_d3_force)
>  			return true;
>  
> +		/*
> +		 * Allow D3Hot for all Devicetree based platforms having a
> +		 * separate node for the bridge. We don't allow D3Cold for now
> +		 * since not all platforms are exposing the Vcc supply in
> +		 * Devicetree which is required for transitioning the bridge to
> +		 * D3Cold.
> +		 *
> +		 * NOTE: The bridge is expected to be defined in Devicetree.
> +		 */
> +		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
> +			return true;
> +

For me, a way to lighten the bridge-node restriction is:

	struct pci_host_bridge *host_bridge;

	...
		/*
		 * Allow D3 for all Device Tree based systems. We check
		 * if our host bridge's parent has a Device Tree node.
		 * None of the D3 restrictions that applied to old BIOS
		 * systems are known to apply to DT systems.
		 */
		host_bridge = pci_find_host_bridge(bridge->bus);
		if (dev_of_node(host_bridge->dev.parent))
			return true;

Brian

>  		/* Even the oldest 2010 Thunderbolt controller supports D3. */
>  		if (bridge->is_thunderbolt)
>  			return true;
> @@ -3042,7 +3054,7 @@ bool pci_bridge_d3cold_allowed(struct pci_dev *bridge)
>   *
>   * This function checks if the bridge is allowed to move to D3Hot.
>   * Currently we only allow D3Hot for recent enough PCIe ports on ACPI based
> - * platforms and Thunderbolt.
> + * platforms, Thunderbolt and Devicetree based platforms.
>   */
>  bool pci_bridge_d3hot_allowed(struct pci_dev *bridge)
>  {
> 
> -- 
> 2.25.1
> 
>