From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
Bjorn Helgaas <helgaas@kernel.org>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: "Jingoo Han" <jingoohan1@gmail.com>,
"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Rob Herring" <robh@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
quic_vbadigan@quicinc.com, quic_ramkri@quicinc.com,
quic_nitegupt@quicinc.com, quic_skananth@quicinc.com,
quic_parass@quicinc.com
Subject: Re: [PATCH v2] PCI: dwc: Enable runtime pm of the host bridge
Date: Fri, 12 Apr 2024 16:02:48 +0530 [thread overview]
Message-ID: <5b7ab46f-3448-f212-fe26-fc7019ce63d3@quicinc.com> (raw)
In-Reply-To: <20240325110959.GC2938@thinkpad>
On 3/25/2024 4:39 PM, Manivannan Sadhasivam wrote:
> 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@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.
>>
>> 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.
>
Hi Rafeal,
can you please comment on this.
> - 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
>
prev parent reply other threads:[~2024-04-12 10:33 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-05 9:49 [PATCH v2] PCI: dwc: Enable runtime pm of the host bridge Krishna chaitanya chundru
2024-03-05 19:57 ` Bjorn Helgaas
2024-03-07 1:58 ` Krishna Chaitanya Chundru
2024-03-07 21:55 ` Bjorn Helgaas
2024-03-08 3:08 ` Krishna Chaitanya Chundru
2024-03-08 17:12 ` Bjorn Helgaas
2024-03-19 11:11 ` Manivannan Sadhasivam
2024-03-20 15:27 ` Krishna Chaitanya Chundru
2024-03-22 22:04 ` Bjorn Helgaas
2024-03-25 11:09 ` Manivannan Sadhasivam
2024-04-12 10:32 ` Krishna Chaitanya Chundru [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5b7ab46f-3448-f212-fe26-fc7019ce63d3@quicinc.com \
--to=quic_krichai@quicinc.com \
--cc=bhelgaas@google.com \
--cc=gustavo.pimentel@synopsys.com \
--cc=helgaas@kernel.org \
--cc=jingoohan1@gmail.com \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=quic_nitegupt@quicinc.com \
--cc=quic_parass@quicinc.com \
--cc=quic_ramkri@quicinc.com \
--cc=quic_skananth@quicinc.com \
--cc=quic_vbadigan@quicinc.com \
--cc=rjw@rjwysocki.net \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).