[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <83dfe28d-505b-4009-044c-684827a1656d@quicinc.com>
Date: Wed, 20 Mar 2024 20:57:20 +0530
From: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Bjorn Helgaas
<helgaas@...nel.org>
CC: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Jingoo Han
<jingoohan1@...il.com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof WilczyĆski <kw@...ux.com>,
Rob Herring
<robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_vbadigan@...cinc.com>, <quic_ramkri@...cinc.com>,
<quic_nitegupt@...cinc.com>, <quic_skananth@...cinc.com>,
<quic_parass@...cinc.com>
Subject: Re: [PATCH v2] PCI: dwc: Enable runtime pm of the host bridge
On 3/19/2024 4:41 PM, 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@quicinc.com]
>>>>
>>>> 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.
>
>> And further confused about managing the host bridge runtime PM in a
>> controller driver. Which other callers of pci_alloc_host_bridge() or
>> devm_pci_alloc_host_bridge() will need similar changes?
>>
>
> This scenario applies to all host bridges. So I think we should enable it inside
> pci_host_probe().
>
> - Mani
>
I will these runtime enable inside the pci_host_probe().
- Krishna Chaitanya.
Powered by blists - more mailing lists