Re: [PATCH v2] PCI: dwc: Enable runtime pm of the host bridge

[Manivannan Sadhasivam]

On Fri, Mar 22, 2024 at 05:04:56PM -0500, Bjorn Helgaas wrote:
> On Tue, Mar 19, 2024 at 04:41:48PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Mar 08, 2024 at 11:12:48AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Mar 08, 2024 at 08:38:52AM +0530, Krishna Chaitanya Chundru wrote:
> > > > On 3/8/2024 3:25 AM, Bjorn Helgaas wrote:
> > > > > [+to Rafael, sorry, another runtime PM question, beginning of thread:
> > > > > https://lore.kernel.org/r/20240305-runtime_pm_enable-v2-1-a849b74091d1@xxxxxxxxxxx]
> > > > > 
> > > > > On Thu, Mar 07, 2024 at 07:28:54AM +0530, Krishna Chaitanya Chundru wrote:
> > > > > > On 3/6/2024 1:27 AM, Bjorn Helgaas wrote:
> > > > > > > On Tue, Mar 05, 2024 at 03:19:01PM +0530, Krishna chaitanya chundru wrote:
> > > > > > > > The Controller driver is the parent device of the PCIe host bridge,
> > > > > > > > PCI-PCI bridge and PCIe endpoint as shown below.
> > > > > > > > 
> > > > > > > > 	PCIe controller(Top level parent & parent of host bridge)
> > > > > > > > 			|
> > > > > > > > 			v
> > > > > > > > 	PCIe Host bridge(Parent of PCI-PCI bridge)
> > > > > > > > 			|
> > > > > > > > 			v
> > > > > > > > 	PCI-PCI bridge(Parent of endpoint driver)
> > > > > > > > 			|
> > > > > > > > 			v
> > > > > > > > 		PCIe endpoint driver
> > > > > > > > 
> > > > > > > > Since runtime PM is disabled for host bridge, the state of the child
> > > > > > > > devices under the host bridge is not taken into account by PM framework
> > > > > > > > for the top level parent, PCIe controller. So PM framework, allows
> > > > > > > > the controller driver to enter runtime PM irrespective of the state
> > > > > > > > of the devices under the host bridge.
> > > > > > > 
> > > > > > > IIUC this says that we runtime suspend the controller even though
> > > > > > > runtime PM is disabled for the host bridge?  I have a hard time
> > > > > > > parsing this; can you cite a function that does this or some relevant
> > > > > > > documentation about how this part of runtime PM works?
> > > > > > > 
> > > > > > Generally controller should go to runtime suspend when endpoint client
> > > > > > drivers and pci-pci host bridge drivers goes to runtime suspend as the
> > > > > > controller driver is the parent, but we are observing controller driver
> > > > > > goes to runtime suspend even when client drivers and PCI-PCI bridge are
> > > > > > in active state.
> > > > > 
> > > > > It surprises me that a device could be suspended while children are
> > > > > active.  A PCI-PCI bridge must be in D0 for any devices below it to be
> > > > > active.  The controller is a platform device, not a PCI device, but I
> > > > > am similarly surprised that we would suspend it when children are
> > > > > active, which makes me think we didn't set the hierarchy up correctly.
> > > > > 
> > > > > It doesn't seem like we should need to enable runtime PM for a parent
> > > > > to keep it from being suspended when children are active.
> > > >
> > > > Here we are not enabling runtime PM of the controller device, we are
> > > > enabling runtime PM for the bridge device which is maintained by the
> > > > PCIe framework. The bridge device is the parent of the PCI-PCI
> > > > bridge and child of the controller device. As the bridge device's
> > > > runtime PM is not enabled the PM framework is ignoring the child's
> > > > runtime status.
> > > 
> > > OK, it's the host bridge, not the controller.
> > > 
> > > I'm still surprised that the PM framework will runtime suspend a
> > > device when child devices are active.
> > 
> > There is a catch here. Even though the child devices are funtionally
> > active, PM framework will only consider their runtime_pm state,
> > which is initially set to 'disabled' for all devices. It is upto the
> > device drivers to enable it when required.
> > 
> > Here is the initial runtime PM status of each device post boot:
> > 
> > Controller device -> disabled initially but enabled by pcie-qcom.c
> > Host bridge -> disabled initially
> > PCIe bridge -> disabled initially but conditionally enabled by portdrv.c
> > PCIe devices -> disabled initially but enabled by respective drivers like WLAN
> > 
> > Now, when the controller device goes to runtime suspend, PM
> > framework will check the runtime PM state of the child device (host
> > bridge) and will find it to be disabled. So it will allow the parent
> > (controller device) to go to runtime suspend. Only if the child
> > device's state was 'active' it will prevent the parent to get
> > suspended.
> > 
> > But you may wonder if this is ideal? IMO NO. But we cannot blame the
> > PM framework here. The responsibility is within the device drivers
> > to handle the PM state based on the usecase. Ideally, the host
> > bridge driver should've handled runtime PM state during the probe
> > time. Otherwise, PM framework wouldn't know when would be the best
> > time to suspend the devices.
> 
> My expectation is that adding new functionality should only require
> changes in drivers that want to take advantage of it.  For example, if
> we add runtime PM support in the controller driver, the result should
> be functionally correct even if we don't update drivers for downstream
> devices.
> 

Well, IMO PM framework should disable runtime PM for the parent if the child's
runtime PM state is disabled.

It'd be good to get the opinion of Rafael.

- Mani

> If that's not the way it works, I suggest that would be a problem in
> the PM framework.
> 
> The host bridge might be a special case because we don't have a
> separate "host bridge" driver; that code is kind of integrated with
> the controller drivers.  So maybe it's OK to do controller + host
> bridge runtime PM support at the same time, as long as any time we add
> runtime PM to a controller, we sure it's also set up for the host
> bridge.
> 
> Bjorn

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