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

From: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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] PCI: dwc: Enable runtime pm of the host bridge
Date: Tue, 27 Feb 2024 13:08:05 +0530	[thread overview]
Message-ID: <3b4d553c-401e-da56-4554-47c4cf040e8c@quicinc.com> (raw)
In-Reply-To: <20240226155305.GJ8422@thinkpad>


On 2/26/2024 9:23 PM, Manivannan Sadhasivam wrote:
> On Mon, Feb 19, 2024 at 06:51:10PM +0530, Krishna chaitanya chundru wrote:
>> Currently controller driver goes to runtime suspend irrespective
>> of the child(pci-pci bridge & endpoint driver) runtime state.
>> This is because the runtime pm is not being enabled for the host
>> bridge dev which maintains parent child relationship.
>>
> 
> You should describe the parent-child topology first. Maybe a simple flow like
> below will help:
> 
> 	PCIe Controller
> 	      |
> 	PCIe Host bridge
> 	      |
> 	PCI-PCI bridge
> 	      |
> 	PCIe endpoint
> 
> Also explain the fact that 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. And this causes the topology breakage and also
> possible PM issues.
> 
> - Mani
>
ACK.

- Krishna Chaitanya.
>> So enable pm runtime for the host bridge, so that controller driver
>> goes to suspend only when all child devices goes to runtime suspend.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index d5fc31f8345f..57756a73df30 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/of_pci.h>
>>   #include <linux/pci_regs.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>   
>>   #include "../../pci.h"
>>   #include "pcie-designware.h"
>> @@ -505,6 +506,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>   	if (pp->ops->post_init)
>>   		pp->ops->post_init(pp);
>>   
>> +	pm_runtime_set_active(&bridge->dev);
>> +	pm_runtime_enable(&bridge->dev);
>> +
>>   	return 0;
>>   
>>   err_stop_link:
>>
>> ---
>> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
>> change-id: 20240219-runtime_pm_enable-bdc17914bd50
>>
>> Best regards,
>> -- 
>> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>
>