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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Aug 02, 2024 at 12:13:31PM +0200, Lukas Wunner wrote:
> On Fri, Aug 02, 2024 at 11:25:03AM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > 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.
> [...]
> > +		if (state == PCI_D3hot && dev_of_node(&bridge->dev))
> > +			return true;
> 
> For such a simple change which several parties are interested in,
> I think it would be better to move it to the front of the series.
> 
> Patch [1/4] looks like an optimization (using a cached value)
> which this patch doesn't depend on.  Patch [2/4] looks like a
> change of bikeshed color which isn't strictly necessary for
> this fourth patch either.  If you want to propose those changes,
> fine, but by making this fourth patch depend on them, you risk
> delaying its acceptance.  As an upstreaming strategy it's usually
> smarter to move potentially controversial or unnecessary material
> to the back of the series, or submit it separately if it can be
> applied standalone.
> 

Agree with you! Even after doing upstreaming for this much time, I tend to
ignore this...

> 
> > 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.
> 
> 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). But D3Cold is a power
off state, and the platform may choose to use it for the case of system suspend.

So I still feel that decoupling D3Hot and D3Cold is necessary.

- Mani

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