[PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Mayank Rana]

PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
bridge device. To enable runtime PM of PCIe host bridge device (child
device), it is must to enable parent device's runtime PM to avoid seeing
the below warning from PM core:

pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
with active children

Fix this issue by enabling starfive pcie controller device's runtime PM
before calling pci_host_probe() in plda_pcie_host_init().

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
v2->v3:
- Update commit description based on Mani's feedback
- Updated Reviewed-by tag
Link to v2: https://patchwork.kernel.org/project/linux-pci/patch/20241011235530.3919347-1-quic_mrana@quicinc.com/

v1->v2: Updated commit description based on Bjorn's feedback
Link to v1: https://patchwork.kernel.org/project/linux-pci/patch/20241010202950.3263899-1-quic_mrana@quicinc.com/
 
 drivers/pci/controller/plda/pcie-starfive.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index 0567ec373a3e..e73c1b7bc8ef 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -404,6 +404,9 @@ static int starfive_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
 	plda->host_ops = &sf_host_ops;
 	plda->num_events = PLDA_MAX_EVENT_NUM;
 	/* mask doorbell event */
@@ -413,11 +416,12 @@ static int starfive_pcie_probe(struct platform_device *pdev)
 	plda->events_bitmap <<= PLDA_NUM_DMA_EVENTS;
 	ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops,
 				  &stf_pcie_event);
-	if (ret)
+	if (ret) {
+		pm_runtime_put_sync(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
 		return ret;
+	}
 
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
 	platform_set_drvdata(pdev, pcie);
 
 	return 0;
-- 
2.25.1

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Bjorn Helgaas]

On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
> PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
> bridge device. To enable runtime PM of PCIe host bridge device (child
> device), it is must to enable parent device's runtime PM to avoid seeing
> the below warning from PM core:
> 
> pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
> with active children
> 
> Fix this issue by enabling starfive pcie controller device's runtime PM
> before calling pci_host_probe() in plda_pcie_host_init().
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

I want this in the same series as Krishna's patch to turn on runtime
PM of host bridges.  That's how I know they need to be applied in
order.  If they're not in the same series, they're likely to be
applied out of order.

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Mayank Rana]

Hi Bjorn

On 10/14/2024 10:23 AM, Bjorn Helgaas wrote:
> On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
>> PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
>> bridge device. To enable runtime PM of PCIe host bridge device (child
>> device), it is must to enable parent device's runtime PM to avoid seeing
>> the below warning from PM core:
>>
>> pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
>> with active children
>>
>> Fix this issue by enabling starfive pcie controller device's runtime PM
>> before calling pci_host_probe() in plda_pcie_host_init().
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> I want this in the same series as Krishna's patch to turn on runtime
> PM of host bridges.  That's how I know they need to be applied in
> order.  If they're not in the same series, they're likely to be
> applied out of order.Thank you for quick response and suggestion.


Hi Krishna

Please add this proposed change as part of your changes to enable 
runtime PM of host bridges.

Regards,
Mayank

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Manivannan Sadhasivam]

On Mon, Oct 14, 2024 at 12:23:21PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
> > PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
> > bridge device. To enable runtime PM of PCIe host bridge device (child
> > device), it is must to enable parent device's runtime PM to avoid seeing
> > the below warning from PM core:
> > 
> > pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
> > with active children
> > 
> > Fix this issue by enabling starfive pcie controller device's runtime PM
> > before calling pci_host_probe() in plda_pcie_host_init().
> > 
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> I want this in the same series as Krishna's patch to turn on runtime
> PM of host bridges.  That's how I know they need to be applied in
> order.  If they're not in the same series, they're likely to be
> applied out of order.

There is no harm in applying this patch on its own. It fixes a legit issue of
enabling the parent runtime PM before the child as required by the PM core. Rest
of the controller drivers follow the same pattern.

I fail to understand why you want this to be combined with Krishna's patch. That
patch is only a trigger, but even without that patch the issue still exists (not
user visible ofc).

- Mani

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

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Bjorn Helgaas]

On Mon, Oct 14, 2024 at 11:18:17PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 14, 2024 at 12:23:21PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
> > > PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
> > > bridge device. To enable runtime PM of PCIe host bridge device (child
> > > device), it is must to enable parent device's runtime PM to avoid seeing
> > > the below warning from PM core:
> > > 
> > > pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
> > > with active children
> > > 
> > > Fix this issue by enabling starfive pcie controller device's runtime PM
> > > before calling pci_host_probe() in plda_pcie_host_init().
> > > 
> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > I want this in the same series as Krishna's patch to turn on runtime
> > PM of host bridges.  That's how I know they need to be applied in
> > order.  If they're not in the same series, they're likely to be
> > applied out of order.
> 
> There is no harm in applying this patch on its own. It fixes a legit
> issue of enabling the parent runtime PM before the child as required
> by the PM core. Rest of the controller drivers follow the same
> pattern.
> 
> I fail to understand why you want this to be combined with Krishna's
> patch. That patch is only a trigger, but even without that patch the
> issue still exists (not user visible ofc).

I don't want it *combined* with Krishna's patch.

I want this applied *before* Krishna's patch because if we apply
Krishna's patch first, we have some interval where we report the
"Enabling runtime PM for inactive device with active children" error.

Bjorn

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Manivannan Sadhasivam]

On Mon, Oct 14, 2024 at 01:08:41PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 14, 2024 at 11:18:17PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Oct 14, 2024 at 12:23:21PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
> > > > PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
> > > > bridge device. To enable runtime PM of PCIe host bridge device (child
> > > > device), it is must to enable parent device's runtime PM to avoid seeing
> > > > the below warning from PM core:
> > > > 
> > > > pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
> > > > with active children
> > > > 
> > > > Fix this issue by enabling starfive pcie controller device's runtime PM
> > > > before calling pci_host_probe() in plda_pcie_host_init().
> > > > 
> > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > I want this in the same series as Krishna's patch to turn on runtime
> > > PM of host bridges.  That's how I know they need to be applied in
> > > order.  If they're not in the same series, they're likely to be
> > > applied out of order.
> > 
> > There is no harm in applying this patch on its own. It fixes a legit
> > issue of enabling the parent runtime PM before the child as required
> > by the PM core. Rest of the controller drivers follow the same
> > pattern.
> > 
> > I fail to understand why you want this to be combined with Krishna's
> > patch. That patch is only a trigger, but even without that patch the
> > issue still exists (not user visible ofc).
> 
> I don't want it *combined* with Krishna's patch.
> 
> I want this applied *before* Krishna's patch because if we apply
> Krishna's patch first, we have some interval where we report the
> "Enabling runtime PM for inactive device with active children" error.
> 

No, I was asking why can't this be applied *first* and then Kirshna's patch? Why
do you want this to be again resent in a separate series?

Both patches are in a mergeable state already. So they can be applied in order.

- Mani

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

Re: [PATCH v3] PCI: starfive: Enable PCIe controller's runtime PM before probing host bridge

[Bjorn Helgaas]

On Tue, Oct 15, 2024 at 10:09:01AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 14, 2024 at 01:08:41PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 14, 2024 at 11:18:17PM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Oct 14, 2024 at 12:23:21PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Oct 14, 2024 at 09:26:07AM -0700, Mayank Rana wrote:
> > > > > PCIe controller device (i.e. PCIe starfive device) is parent to PCIe host
> > > > > bridge device. To enable runtime PM of PCIe host bridge device (child
> > > > > device), it is must to enable parent device's runtime PM to avoid seeing
> > > > > the below warning from PM core:
> > > > > 
> > > > > pcie-starfive 940000000.pcie: Enabling runtime PM for inactive device
> > > > > with active children
> > > > > 
> > > > > Fix this issue by enabling starfive pcie controller device's runtime PM
> > > > > before calling pci_host_probe() in plda_pcie_host_init().
> > > > > 
> > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > 
> > > > I want this in the same series as Krishna's patch to turn on runtime
> > > > PM of host bridges.  That's how I know they need to be applied in
> > > > order.  If they're not in the same series, they're likely to be
> > > > applied out of order.
> > > 
> > > There is no harm in applying this patch on its own. It fixes a legit
> > > issue of enabling the parent runtime PM before the child as required
> > > by the PM core. Rest of the controller drivers follow the same
> > > pattern.
> > > 
> > > I fail to understand why you want this to be combined with Krishna's
> > > patch. That patch is only a trigger, but even without that patch the
> > > issue still exists (not user visible ofc).
> > 
> > I don't want it *combined* with Krishna's patch.
> > 
> > I want this applied *before* Krishna's patch because if we apply
> > Krishna's patch first, we have some interval where we report the
> > "Enabling runtime PM for inactive device with active children" error.
> 
> No, I was asking why can't this be applied *first* and then
> Kirshna's patch? Why do you want this to be again resent in a
> separate series?

It can certainly be applied first.  It's just that when they're posted
separately, even with a comment in the email discussion to say "please
apply A before B", it's extra work for whoever merges them and it
makes it more likely to make mistakes like applying them in the wrong
order, or one without the other, or putting them on separate branches.

The usual way to say "B depends on A" is to post them together in a
series with A followed by B.

Bjorn