[PATCH v1 0/2] Add runtime PM support to Rockchip DW PCIe driver

[PATCH v1 0/2] Add runtime PM support to Rockchip DW PCIe driver

[Anand Moon]

Introduce runtime power management support in the Rockchip DesignWare PCIe
controller driver.  These changes allow the PCIe controller to suspend and
resume dynamically, improving power efficiency on supported platforms.

Can Patch 1 can be backpoted to stable? It helps clean shutdown of PCIe.

Clarification: the series is based on

git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
branch : controller/dw-rockchip

Thanks
-Anand

Anand Moon (2):
  PCI: dw-rockchip: Add remove callback for resource cleanup
  PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)


base-commit: 7ad31f88429369ada44710176e176256a2812c3f
-- 
2.50.1

[PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Anand Moon]

Introduce a .remove() callback to the Rockchip DesignWare PCIe
controller driver to ensure proper resource deinitialization during
device removal. This includes disabling clocks and deinitializing the
PCIe PHY.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 87dd2dd188b4..b878ae8e2b3e 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void rockchip_pcie_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
+
+	/* Perform other cleanups as necessary */
+	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
+	rockchip_pcie_phy_deinit(rockchip);
+}
+
 static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
 	.mode = DW_PCIE_RC_TYPE,
 };
@@ -754,5 +764,6 @@ static struct platform_driver rockchip_pcie_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe = rockchip_pcie_probe,
+	.remove = rockchip_pcie_remove,
 };
 builtin_platform_driver(rockchip_pcie_driver);
-- 
2.50.1

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Nicolas Frattaroli]

On Monday, 27 October 2025 15:55:29 Central European Standard Time Anand Moon wrote:
> Introduce a .remove() callback to the Rockchip DesignWare PCIe
> controller driver to ensure proper resource deinitialization during
> device removal. This includes disabling clocks and deinitializing the
> PCIe PHY.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 87dd2dd188b4..b878ae8e2b3e 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> +	/* Perform other cleanups as necessary */
> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +	rockchip_pcie_phy_deinit(rockchip);

You may want to add a

    if (rockchip->vpcie3v3)
            regulator_disable(rockchip->vpcie3v3);

here, since it's enabled in the probe function if it's found.

Not doing so means the regulator core will produce a warning
splat when devres removes it I'm fairly sure.

Kind regards,
Nicolas Frattaroli

> +}
> +
>  static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
>  	.mode = DW_PCIE_RC_TYPE,
>  };
> @@ -754,5 +764,6 @@ static struct platform_driver rockchip_pcie_driver = {
>  		.suppress_bind_attrs = true,
>  	},
>  	.probe = rockchip_pcie_probe,
> +	.remove = rockchip_pcie_remove,
>  };
>  builtin_platform_driver(rockchip_pcie_driver);
> 

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Anand Moon]

Hi Nicolas,

Thanks for your review comments.

On Mon, 27 Oct 2025 at 20:42, Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Monday, 27 October 2025 15:55:29 Central European Standard Time Anand Moon wrote:
> > Introduce a .remove() callback to the Rockchip DesignWare PCIe
> > controller driver to ensure proper resource deinitialization during
> > device removal. This includes disabling clocks and deinitializing the
> > PCIe PHY.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 87dd2dd188b4..b878ae8e2b3e 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       return ret;
> >  }
> >
> > +static void rockchip_pcie_remove(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > +
> > +     /* Perform other cleanups as necessary */
> > +     clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> > +     rockchip_pcie_phy_deinit(rockchip);
>
> You may want to add a
>
>     if (rockchip->vpcie3v3)
>             regulator_disable(rockchip->vpcie3v3);
>
> here, since it's enabled in the probe function if it's found.
>
> Not doing so means the regulator core will produce a warning
> splat when devres removes it I'm fairly sure.
>
I've removed the dependency on vpcie3v3 in the following commit:
 c930b10f17c0 ("PCI: dw-rockchip: Simplify regulator setup with
devm_regulator_get_enable_optional()")
Please review this commit and confirm if everything looks good.

> Kind regards,
> Nicolas Frattaroli
>
Thanks
-Anand

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Nicolas Frattaroli]

On Monday, 27 October 2025 17:31:23 Central European Standard Time Anand Moon wrote:
> Hi Nicolas,
> 
> Thanks for your review comments.
> 
> On Mon, 27 Oct 2025 at 20:42, Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > On Monday, 27 October 2025 15:55:29 Central European Standard Time Anand Moon wrote:
> > > Introduce a .remove() callback to the Rockchip DesignWare PCIe
> > > controller driver to ensure proper resource deinitialization during
> > > device removal. This includes disabling clocks and deinitializing the
> > > PCIe PHY.
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > index 87dd2dd188b4..b878ae8e2b3e 100644
> > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > >       return ret;
> > >  }
> > >
> > > +static void rockchip_pcie_remove(struct platform_device *pdev)
> > > +{
> > > +     struct device *dev = &pdev->dev;
> > > +     struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > > +
> > > +     /* Perform other cleanups as necessary */
> > > +     clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> > > +     rockchip_pcie_phy_deinit(rockchip);
> >
> > You may want to add a
> >
> >     if (rockchip->vpcie3v3)
> >             regulator_disable(rockchip->vpcie3v3);
> >
> > here, since it's enabled in the probe function if it's found.
> >
> > Not doing so means the regulator core will produce a warning
> > splat when devres removes it I'm fairly sure.
> >
> I've removed the dependency on vpcie3v3 in the following commit:
>  c930b10f17c0 ("PCI: dw-rockchip: Simplify regulator setup with
> devm_regulator_get_enable_optional()")
> Please review this commit and confirm if everything looks good.

I see. In that case, your code is indeed correct, thank you for
pointing this out.

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Kind regards,
Nicolas Frattaroli

> 
> > Kind regards,
> > Nicolas Frattaroli
> >
> Thanks
> -Anand
> 

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Shawn Lin]

在 2025/10/27 星期一 22:55, Anand Moon 写道:
> Introduce a .remove() callback to the Rockchip DesignWare PCIe
> controller driver to ensure proper resource deinitialization during
> device removal. This includes disabling clocks and deinitializing the
> PCIe PHY.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 87dd2dd188b4..b878ae8e2b3e 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   	return ret;
>   }
>   
> +static void rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> +	/* Perform other cleanups as necessary */
> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +	rockchip_pcie_phy_deinit(rockchip);
> +}

Thanks for the patch.

Dou you need to call dw_pcie_host_deinit()?
And I think you should also try to mask PCIE_CLIENT_INTR_MASK_MISC and
remove the irq domain as well.

if (rockchip->irq_domain) {
	int virq, j;
	for (j = 0; j < PCI_NUM_INTX; j++) {
		virq = irq_find_mapping(rockchip->irq_domain, j);
		if (virq > 0)
			irq_dispose_mapping(virq);
         }
	irq_set_chained_handler_and_data(rockchip->irq, NULL, NULL);
	irq_domain_remove(rockchip->irq_domain);
}

Another thin I noticed is should we call pm_runtime_* here for hope that
genpd could be powered donw once removed?


> +
>   static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
>   	.mode = DW_PCIE_RC_TYPE,
>   };
> @@ -754,5 +764,6 @@ static struct platform_driver rockchip_pcie_driver = {
>   		.suppress_bind_attrs = true,
>   	},
>   	.probe = rockchip_pcie_probe,
> +	.remove = rockchip_pcie_remove,
>   };
>   builtin_platform_driver(rockchip_pcie_driver);

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Anand Moon]

Hi Shawn,

Thanks for your review comments.

On Tue, 28 Oct 2025 at 05:56, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> 在 2025/10/27 星期一 22:55, Anand Moon 写道:
> > Introduce a .remove() callback to the Rockchip DesignWare PCIe
> > controller driver to ensure proper resource deinitialization during
> > device removal. This includes disabling clocks and deinitializing the
> > PCIe PHY.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 87dd2dd188b4..b878ae8e2b3e 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       return ret;
> >   }
> >
> > +static void rockchip_pcie_remove(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > +
> > +     /* Perform other cleanups as necessary */
> > +     clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> > +     rockchip_pcie_phy_deinit(rockchip);
> > +}
>
> Thanks for the patch.
>
> Dou you need to call dw_pcie_host_deinit()?
I feel the rockchip_pcie_phy_deinit will power off the phy
> And I think you should also try to mask PCIE_CLIENT_INTR_MASK_MISC and
> remove the irq domain as well.
>
> if (rockchip->irq_domain) {
>         int virq, j;
>         for (j = 0; j < PCI_NUM_INTX; j++) {
>                 virq = irq_find_mapping(rockchip->irq_domain, j);
>                 if (virq > 0)
>                         irq_dispose_mapping(virq);
>          }
>         irq_set_chained_handler_and_data(rockchip->irq, NULL, NULL);
>         irq_domain_remove(rockchip->irq_domain);
> }
>
I have implemented resource cleanup in rockchip_pcie_remove,
which is invoked when the system is shutting down.
Your feedback on the updated code is welcome.

static void rockchip_pcie_remove(struct platform_device *pdev)
{
        struct device *dev = &pdev->dev;
        struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
        int irq;

        irq = of_irq_get_byname(dev->of_node, "legacy");
        if (irq < 0)
                return;

        /* Perform other cleanups as necessary */
        /* clear up INTR staatus register */
        rockchip_pcie_writel_apb(rockchip, 0xffffffff,
                                 PCIE_CLIENT_INTR_STATUS_MISC);
        if (rockchip->irq_domain) {
                int virq, j;
                for (j = 0; j < PCI_NUM_INTX; j++) {
                        virq = irq_find_mapping(rockchip->irq_domain, j);
                        if (virq > 0)
                                irq_dispose_mapping(virq);
                }
                irq_set_chained_handler_and_data(irq, NULL, NULL);
                irq_domain_remove(rockchip->irq_domain);
        }

        clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
        /* poweroff the phy */
        rockchip_pcie_phy_deinit(rockchip);
        /* release the reset */
        reset_control_assert(rockchip->rst);
        pm_runtime_put_sync(dev);
        pm_runtime_disable(dev);
        pm_runtime_no_callbacks(dev);
}

> Another thin I noticed is should we call pm_runtime_* here for hope that
> genpd could be powered donw once removed?
>
I could not find 'genpd' (power domain) used in the PCIe driver
If we have an example to use genpd I will update this.

I am also looking into adding NOIRQ_SYSTEM_SLEEP_PM_OPS

Thanks
-Anand

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Shawn Lin]

在 2025/10/28 星期二 17:34, Anand Moon 写道:
> Hi Shawn,
> 
> Thanks for your review comments.
> 
> On Tue, 28 Oct 2025 at 05:56, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>
>> 在 2025/10/27 星期一 22:55, Anand Moon 写道:
>>> Introduce a .remove() callback to the Rockchip DesignWare PCIe
>>> controller driver to ensure proper resource deinitialization during
>>> device removal. This includes disabling clocks and deinitializing the
>>> PCIe PHY.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> index 87dd2dd188b4..b878ae8e2b3e 100644
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>>        return ret;
>>>    }
>>>
>>> +static void rockchip_pcie_remove(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>> +
>>> +     /* Perform other cleanups as necessary */
>>> +     clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>>> +     rockchip_pcie_phy_deinit(rockchip);
>>> +}
>>
>> Thanks for the patch.
>>
>> Dou you need to call dw_pcie_host_deinit()?
> I feel the rockchip_pcie_phy_deinit will power off the phy
>> And I think you should also try to mask PCIE_CLIENT_INTR_MASK_MISC and
>> remove the irq domain as well.
>>
>> if (rockchip->irq_domain) {
>>          int virq, j;
>>          for (j = 0; j < PCI_NUM_INTX; j++) {
>>                  virq = irq_find_mapping(rockchip->irq_domain, j);
>>                  if (virq > 0)
>>                          irq_dispose_mapping(virq);
>>           }
>>          irq_set_chained_handler_and_data(rockchip->irq, NULL, NULL);
>>          irq_domain_remove(rockchip->irq_domain);
>> }
>>
> I have implemented resource cleanup in rockchip_pcie_remove,
> which is invoked when the system is shutting down.
> Your feedback on the updated code is welcome.
> 
> static void rockchip_pcie_remove(struct platform_device *pdev)
> {
>          struct device *dev = &pdev->dev;
>          struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>          int irq;
> 
>          irq = of_irq_get_byname(dev->of_node, "legacy");
>          if (irq < 0)
>                  return;
> 
>          /* Perform other cleanups as necessary */
>          /* clear up INTR staatus register */
>          rockchip_pcie_writel_apb(rockchip, 0xffffffff,
>                                   PCIE_CLIENT_INTR_STATUS_MISC);
>          if (rockchip->irq_domain) {
>                  int virq, j;
>                  for (j = 0; j < PCI_NUM_INTX; j++) {
>                          virq = irq_find_mapping(rockchip->irq_domain, j);
>                          if (virq > 0)
>                                  irq_dispose_mapping(virq);
>                  }
>                  irq_set_chained_handler_and_data(irq, NULL, NULL);
>                  irq_domain_remove(rockchip->irq_domain);
>          }
> 
>          clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>          /* poweroff the phy */
>          rockchip_pcie_phy_deinit(rockchip);
>          /* release the reset */

release? Or "reset the controller"?

>          reset_control_assert(rockchip->rst);
>          pm_runtime_put_sync(dev);
>          pm_runtime_disable(dev);
>          pm_runtime_no_callbacks(dev);
> }
> 
>> Another thin I noticed is should we call pm_runtime_* here for hope that
>> genpd could be powered donw once removed?
>>
> I could not find 'genpd' (power domain) used in the PCIe driver
> If we have an example to use genpd I will update this.
 > > I am also looking into adding NOIRQ_SYSTEM_SLEEP_PM_OPS

That sounds good, you can pick up my patch[1] if you'd like to continue
addressing the comments that I haven't had time to think more.

[1] https://www.spinics.net/lists/linux-pci/msg171846.html

> 
> Thanks
> -Anand
> 

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Anand Moon]

Hi Shawn,

On Wed, 29 Oct 2025 at 05:58, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
>
> 在 2025/10/28 星期二 17:34, Anand Moon 写道:
> > Hi Shawn,
> >
> > Thanks for your review comments.
> >
> > On Tue, 28 Oct 2025 at 05:56, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> >>
> >> 在 2025/10/27 星期一 22:55, Anand Moon 写道:
> >>> Introduce a .remove() callback to the Rockchip DesignWare PCIe
> >>> controller driver to ensure proper resource deinitialization during
> >>> device removal. This includes disabling clocks and deinitializing the
> >>> PCIe PHY.
> >>>
> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>> ---
> >>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
> >>>    1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> >>> index 87dd2dd188b4..b878ae8e2b3e 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> >>> @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >>>        return ret;
> >>>    }
> >>>
> >>> +static void rockchip_pcie_remove(struct platform_device *pdev)
> >>> +{
> >>> +     struct device *dev = &pdev->dev;
> >>> +     struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> >>> +
> >>> +     /* Perform other cleanups as necessary */
> >>> +     clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> >>> +     rockchip_pcie_phy_deinit(rockchip);
> >>> +}
> >>
> >> Thanks for the patch.
> >>
> >> Dou you need to call dw_pcie_host_deinit()?
> > I feel the rockchip_pcie_phy_deinit will power off the phy
> >> And I think you should also try to mask PCIE_CLIENT_INTR_MASK_MISC and
> >> remove the irq domain as well.
> >>
> >> if (rockchip->irq_domain) {
> >>          int virq, j;
> >>          for (j = 0; j < PCI_NUM_INTX; j++) {
> >>                  virq = irq_find_mapping(rockchip->irq_domain, j);
> >>                  if (virq > 0)
> >>                          irq_dispose_mapping(virq);
> >>           }
> >>          irq_set_chained_handler_and_data(rockchip->irq, NULL, NULL);
> >>          irq_domain_remove(rockchip->irq_domain);
> >> }
> >>
> > I have implemented resource cleanup in rockchip_pcie_remove,
> > which is invoked when the system is shutting down.
> > Your feedback on the updated code is welcome.
> >
> > static void rockchip_pcie_remove(struct platform_device *pdev)
> > {
> >          struct device *dev = &pdev->dev;
> >          struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> >          int irq;
> >
> >          irq = of_irq_get_byname(dev->of_node, "legacy");
> >          if (irq < 0)
> >                  return;
> >
> >          /* Perform other cleanups as necessary */
> >          /* clear up INTR staatus register */
> >          rockchip_pcie_writel_apb(rockchip, 0xffffffff,
> >                                   PCIE_CLIENT_INTR_STATUS_MISC);
> >          if (rockchip->irq_domain) {
> >                  int virq, j;
> >                  for (j = 0; j < PCI_NUM_INTX; j++) {
> >                          virq = irq_find_mapping(rockchip->irq_domain, j);
> >                          if (virq > 0)
> >                                  irq_dispose_mapping(virq);
> >                  }
> >                  irq_set_chained_handler_and_data(irq, NULL, NULL);
> >                  irq_domain_remove(rockchip->irq_domain);
> >          }
> >
> >          clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> >          /* poweroff the phy */
> >          rockchip_pcie_phy_deinit(rockchip);
> >          /* release the reset */
>
> release? Or "reset the controller"?
>
Ok, I will fix this.
> >          reset_control_assert(rockchip->rst);
> >          pm_runtime_put_sync(dev);
> >          pm_runtime_disable(dev);
> >          pm_runtime_no_callbacks(dev);
> > }
> >
> >> Another thin I noticed is should we call pm_runtime_* here for hope that
> >> genpd could be powered donw once removed?
> >>
> > I could not find 'genpd' (power domain) used in the PCIe driver
> > If we have an example to use genpd I will update this.
>  > > I am also looking into adding NOIRQ_SYSTEM_SLEEP_PM_OPS
>
> That sounds good, you can pick up my patch[1] if you'd like to continue
> addressing the comments that I haven't had time to think more.
>
> [1] https://www.spinics.net/lists/linux-pci/msg171846.html
>
Ok, thanks. I will carefully study and address the comments.

Thanks
-Anand

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 08:25:29PM +0530, Anand Moon wrote:
> Introduce a .remove() callback to the Rockchip DesignWare PCIe
> controller driver to ensure proper resource deinitialization during
> device removal. This includes disabling clocks and deinitializing the
> PCIe PHY.
> 

How can you remove a driver that is only built-in? You are just sending some
pointless patches that were not tested and does not make sense at all.

Please stop wasting others time.

- Mani

> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 87dd2dd188b4..b878ae8e2b3e 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void rockchip_pcie_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> +
> +	/* Perform other cleanups as necessary */
> +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> +	rockchip_pcie_phy_deinit(rockchip);
> +}
> +
>  static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
>  	.mode = DW_PCIE_RC_TYPE,
>  };
> @@ -754,5 +764,6 @@ static struct platform_driver rockchip_pcie_driver = {
>  		.suppress_bind_attrs = true,
>  	},
>  	.probe = rockchip_pcie_probe,
> +	.remove = rockchip_pcie_remove,
>  };
>  builtin_platform_driver(rockchip_pcie_driver);
> -- 
> 2.50.1
> 

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

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Nicolas Frattaroli]

On Friday, 31 October 2025 09:36:05 Central European Standard Time Manivannan Sadhasivam wrote:
> On Mon, Oct 27, 2025 at 08:25:29PM +0530, Anand Moon wrote:
> > Introduce a .remove() callback to the Rockchip DesignWare PCIe
> > controller driver to ensure proper resource deinitialization during
> > device removal. This includes disabling clocks and deinitializing the
> > PCIe PHY.
> > 
> 
> How can you remove a driver that is only built-in? You are just sending some
> pointless patches that were not tested and does not make sense at all.

The better question would be: why does Kconfig make PCIE_ROCKCHIP_DW
a bool rather than a tristate? I see other PCIE_DW drivers using
tristate, so this doesn't seem like a technical limitation with the
IP.

> 
> Please stop wasting others time.
> 
> - Mani
> 
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index 87dd2dd188b4..b878ae8e2b3e 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -717,6 +717,16 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >  	return ret;
> >  }
> >  
> > +static void rockchip_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > +
> > +	/* Perform other cleanups as necessary */
> > +	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
> > +	rockchip_pcie_phy_deinit(rockchip);
> > +}
> > +
> >  static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
> >  	.mode = DW_PCIE_RC_TYPE,
> >  };
> > @@ -754,5 +764,6 @@ static struct platform_driver rockchip_pcie_driver = {
> >  		.suppress_bind_attrs = true,
> >  	},
> >  	.probe = rockchip_pcie_probe,
> > +	.remove = rockchip_pcie_remove,
> >  };
> >  builtin_platform_driver(rockchip_pcie_driver);
> 
> 

Re: [PATCH v1 1/2] PCI: dw-rockchip: Add remove callback for resource cleanup

[Manivannan Sadhasivam]

On Fri, Oct 31, 2025 at 01:29:25PM +0100, Nicolas Frattaroli wrote:
> On Friday, 31 October 2025 09:36:05 Central European Standard Time Manivannan Sadhasivam wrote:
> > On Mon, Oct 27, 2025 at 08:25:29PM +0530, Anand Moon wrote:
> > > Introduce a .remove() callback to the Rockchip DesignWare PCIe
> > > controller driver to ensure proper resource deinitialization during
> > > device removal. This includes disabling clocks and deinitializing the
> > > PCIe PHY.
> > > 
> > 
> > How can you remove a driver that is only built-in? You are just sending some
> > pointless patches that were not tested and does not make sense at all.
> 
> The better question would be: why does Kconfig make PCIE_ROCKCHIP_DW
> a bool rather than a tristate? I see other PCIE_DW drivers using
> tristate, so this doesn't seem like a technical limitation with the
> IP.
> 

The limitation is due to irqchip maintainers not allowing (or recommending) to
remove a controller driver which implements an irqchip. So if any controller
driver that does so, we make them built-in. Recently, I suggested some
controller drivers to be tristate, but without the remove() callback. This will
allow the controller drivers to be built as a module, but not get removed
during runtime.

The reason why an irqchip controller should not be removed during runtime is
that the concerns around disposing the IRQs consumed by the client drivers.
Current irqchip framework doesn't guarantee that all IRQs would be disposed when
the controller is removed.

But irrespective of the above explanation, my statement was correct that this
patch is pointless.

- Mani

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

[PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Anand Moon]

Add runtime power management support to the Rockchip DesignWare PCIe
controller driver by enabling devm_pm_runtime() in the probe function.
These changes allow the PCIe controller to suspend and resume dynamically,
improving power efficiency on supported platforms.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index b878ae8e2b3e..5026598d09f8 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
@@ -690,6 +691,20 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto deinit_phy;
 
+	ret = pm_runtime_set_suspended(dev);
+	if (ret)
+		goto disable_pm_runtime;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret) {
+		ret = dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+		goto deinit_clk;
+	}
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		goto disable_pm_runtime;
+
 	switch (data->mode) {
 	case DW_PCIE_RC_TYPE:
 		ret = rockchip_pcie_configure_rc(pdev, rockchip);
@@ -709,7 +724,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
+disable_pm_runtime:
+	pm_runtime_disable(dev);
 deinit_clk:
+	pm_runtime_no_callbacks(dev);
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
@@ -725,6 +743,9 @@ static void rockchip_pcie_remove(struct platform_device *pdev)
 	/* Perform other cleanups as necessary */
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 	rockchip_pcie_phy_deinit(rockchip);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	pm_runtime_no_callbacks(dev);
 }
 
 static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
-- 
2.50.1

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Shawn Lin]

在 2025/10/27 星期一 22:55, Anand Moon 写道:
> Add runtime power management support to the Rockchip DesignWare PCIe
> controller driver by enabling devm_pm_runtime() in the probe function.
> These changes allow the PCIe controller to suspend and resume dynamically,
> improving power efficiency on supported platforms.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index b878ae8e2b3e..5026598d09f8 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -20,6 +20,7 @@
>   #include <linux/of_irq.h>
>   #include <linux/phy/phy.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/regmap.h>
>   #include <linux/reset.h>
>   
> @@ -690,6 +691,20 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto deinit_phy;
>   
> +	ret = pm_runtime_set_suspended(dev);
> +	if (ret)
> +		goto disable_pm_runtime;
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +		goto deinit_clk;
> +	}
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto disable_pm_runtime;
> +
>   	switch (data->mode) {
>   	case DW_PCIE_RC_TYPE:
>   		ret = rockchip_pcie_configure_rc(pdev, rockchip);
> @@ -709,7 +724,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>   
>   	return 0;
>   
> +disable_pm_runtime:

We need to call reset_control_assert(rockchip->rst) before releasing the
the pm refcount. The problem we faced on vendor kernel is there might be 
still on-going transaction from IP to the AXI which blocks genpd to be
powered down.

> +	pm_runtime_disable(dev);
>   deinit_clk:
> +	pm_runtime_no_callbacks(dev);
>   	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>   deinit_phy:
>   	rockchip_pcie_phy_deinit(rockchip);
> @@ -725,6 +743,9 @@ static void rockchip_pcie_remove(struct platform_device *pdev)
>   	/* Perform other cleanups as necessary */
>   	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>   	rockchip_pcie_phy_deinit(rockchip);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_no_callbacks(dev);
>   }
>   
>   static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Anand Moon]

Hi Shawn,

Thanks for your review comments.

On Tue, 28 Oct 2025 at 06:14, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> 在 2025/10/27 星期一 22:55, Anand Moon 写道:
> > Add runtime power management support to the Rockchip DesignWare PCIe
> > controller driver by enabling devm_pm_runtime() in the probe function.
> > These changes allow the PCIe controller to suspend and resume dynamically,
> > improving power efficiency on supported platforms.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > index b878ae8e2b3e..5026598d09f8 100644
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/of_irq.h>
> >   #include <linux/phy/phy.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >   #include <linux/regmap.h>
> >   #include <linux/reset.h>
> >
> > @@ -690,6 +691,20 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto deinit_phy;
> >
> > +     ret = pm_runtime_set_suspended(dev);
> > +     if (ret)
> > +             goto disable_pm_runtime;
> > +
> > +     ret = devm_pm_runtime_enable(dev);
> > +     if (ret) {
> > +             ret = dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> > +             goto deinit_clk;
> > +     }
> > +
> > +     ret = pm_runtime_resume_and_get(dev);
> > +     if (ret)
> > +             goto disable_pm_runtime;
> > +
> >       switch (data->mode) {
> >       case DW_PCIE_RC_TYPE:
> >               ret = rockchip_pcie_configure_rc(pdev, rockchip);
> > @@ -709,7 +724,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> >
> >       return 0;
> >
> > +disable_pm_runtime:
>
> We need to call reset_control_assert(rockchip->rst) before releasing the
> the pm refcount. The problem we faced on vendor kernel is there might be
> still on-going transaction from IP to the AXI which blocks genpd to be
> powered down.

Thanks, I will try to fix this in the next version.

Thanks
-Anand

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Manivannan Sadhasivam]

On Mon, Oct 27, 2025 at 08:25:30PM +0530, Anand Moon wrote:
> Add runtime power management support to the Rockchip DesignWare PCIe
> controller driver by enabling devm_pm_runtime() in the probe function.
> These changes allow the PCIe controller to suspend and resume dynamically,
> improving power efficiency on supported platforms.
> 

Seriously? How can this patch improve the power efficiency if it is not doing
any PM operation on its own?

Again a pointless patch.

- Mani

> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index b878ae8e2b3e..5026598d09f8 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -20,6 +20,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  
> @@ -690,6 +691,20 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto deinit_phy;
>  
> +	ret = pm_runtime_set_suspended(dev);
> +	if (ret)
> +		goto disable_pm_runtime;
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +		goto deinit_clk;
> +	}
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto disable_pm_runtime;
> +
>  	switch (data->mode) {
>  	case DW_PCIE_RC_TYPE:
>  		ret = rockchip_pcie_configure_rc(pdev, rockchip);
> @@ -709,7 +724,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +disable_pm_runtime:
> +	pm_runtime_disable(dev);
>  deinit_clk:
> +	pm_runtime_no_callbacks(dev);
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>  deinit_phy:
>  	rockchip_pcie_phy_deinit(rockchip);
> @@ -725,6 +743,9 @@ static void rockchip_pcie_remove(struct platform_device *pdev)
>  	/* Perform other cleanups as necessary */
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>  	rockchip_pcie_phy_deinit(rockchip);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_no_callbacks(dev);
>  }
>  
>  static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
> -- 
> 2.50.1
> 

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

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Anand Moon]

Hi Manivannan

Thanks for your review comment.

On Fri, 31 Oct 2025 at 14:09, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Mon, Oct 27, 2025 at 08:25:30PM +0530, Anand Moon wrote:
> > Add runtime power management support to the Rockchip DesignWare PCIe
> > controller driver by enabling devm_pm_runtime() in the probe function.
> > These changes allow the PCIe controller to suspend and resume dynamically,
> > improving power efficiency on supported platforms.
> >
>
> Seriously? How can this patch improve the power efficiency if it is not doing
> any PM operation on its own?
>
I could verify that runtime power management is active

[root@rockpi-5b alarm]# cat
/sys/devices/platform/a41000000.pcie/power/runtime_status
active
[root@rockpi-5b alarm]#  find /sys -name runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/pci_bus/0004:41/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-3::lan/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-2::lan/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-1::lan/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-0::lan/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/power/runtime_status
/sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/hwmon/hwmon11/power/runtime_status

Well, the powertop shows that the runtime power management is enabled
on Radxa Rock 5b,

PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
stats   Device Freq stats   Tunables   WakeUp
>> Good          Wireless Power Saving for interface wlan0
   Good          VM writeback timeout
   Good          Bluetooth device interface status
   Good          NMI watchdog should be turned off
   Good          Autosuspend for unknown USB device 2-1.3 (8087:0a2b)
   Good          Autosuspend for USB device USB 2.0 Hub [2-1]
   Good          Autosuspend for USB device Generic Platform OHCI
controller [usb1]
   Good          Autosuspend for USB device xHCI Host Controller [usb8]
   Good          Autosuspend for USB device Generic Platform OHCI
controller [usb4]
   Good          Autosuspend for USB device EHCI Host Controller [usb2]
   Good          Autosuspend for USB device xHCI Host Controller [usb6]
   Good          Autosuspend for USB device EHCI Host Controller [usb3]
   Good          Autosuspend for USB device xHCI Host Controller [usb5]
   Good          Autosuspend for USB device xHCI Host Controller [usb7]
   Good          Runtime PM for PCI Device Intel Corporation Wireless
8265 / 8275
   Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
   Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
   Good          Runtime PM for PCI Device Realtek Semiconductor Co.,
Ltd. RTL8125 2.5GbE Controller
   Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
   Good          Runtime PM for PCI Device Samsung Electronics Co Ltd
NVMe SSD Controller SM981/PM981/PM983

PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
stats   Device Freq stats   Tunables   WakeUp
              Usage     Device name
              1.1%        CPU use
            100.0%        Radio device: rfkill_gpio
            100.0%        runtime-rockchip-gate-link-clk.712
            100.0%        PCI Device: Realtek Semiconductor Co., Ltd.
RTL8125 2.5GbE Controller
            100.0%        runtime-rockchip-gate-link-clk.717
            100.0%        runtime-rockchip-gate-link-clk.714
            100.0%        runtime-rockchip-gate-link-clk.489
            100.0%        runtime-a40000000.pcie
            100.0%        runtime-a40800000.pcie
            100.0%        runtime-rockchip-gate-link-clk.718
            100.0%        runtime-rockchip-gate-link-clk.706
            100.0%        runtime-rockchip-gate-link-clk.708
            100.0%        PCI Device: Intel Corporation Wireless 8265 / 8275
            100.0%        Radio device: btusb
            100.0%        runtime-fcd00000.usb
            100.0%        PCI Device: Samsung Electronics Co Ltd NVMe
SSD Controller SM981/PM981/PM983
            100.0%        Radio device: rfkill_gpio
            100.0%        runtime-fc000000.usb
            100.0%        Radio device: iwlwifi
            100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
            100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
            100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
            100.0%        runtime-rockchip-gate-link-clk.711
            100.0%        runtime-fc400000.usb
            100.0%        runtime-rockchip-gate-link-clk.704
            100.0%        runtime-rockchip-gate-link-clk.701
            100.0%        runtime-rockchip-gate-link-clk.716
            100.0%        runtime-rockchip-gate-link-clk.707
            100.0%        runtime-rockchip-gate-link-clk.709
            100.0%        runtime-rockchip-gate-link-clk.719
            100.0%        runtime-xhci-hcd.1.auto
            100.0%        runtime-feb50000.serial
            100.0%        runtime-rockchip-gate-link-clk.715
            100.0%        runtime-rockchip-gate-link-clk.710

> Again, a pointless patch.
I implemented a .remove patch to ensure proper resource cleanup,
which is a necessary step for successfully enabling and managing
runtime power for the device.
>
> - Mani
Thanks
-Anand

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Manivannan Sadhasivam]

On Fri, Oct 31, 2025 at 07:33:23PM +0530, Anand Moon wrote:
> Hi Manivannan
> 
> Thanks for your review comment.
> 
> On Fri, 31 Oct 2025 at 14:09, Manivannan Sadhasivam <mani@kernel.org> wrote:
> >
> > On Mon, Oct 27, 2025 at 08:25:30PM +0530, Anand Moon wrote:
> > > Add runtime power management support to the Rockchip DesignWare PCIe
> > > controller driver by enabling devm_pm_runtime() in the probe function.
> > > These changes allow the PCIe controller to suspend and resume dynamically,
> > > improving power efficiency on supported platforms.
> > >
> >
> > Seriously? How can this patch improve the power efficiency if it is not doing
> > any PM operation on its own?
> >
> I could verify that runtime power management is active
> 

This is runtime status being active, which is a different thing as it only
allows the runtime PM hierarchy to be maintained. But the way you described the
commit message sounds like the patch is enabling runtime PM of the controller
and that improves efficiency (as if the controller driver is actively doing
runtime PM operations).

> [root@rockpi-5b alarm]# cat
> /sys/devices/platform/a41000000.pcie/power/runtime_status
> active
> [root@rockpi-5b alarm]#  find /sys -name runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/pci_bus/0004:41/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-3::lan/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-2::lan/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-1::lan/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-0::lan/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/power/runtime_status
> /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/hwmon/hwmon11/power/runtime_status
> 
> Well, the powertop shows that the runtime power management is enabled
> on Radxa Rock 5b,
> 
> PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
> stats   Device Freq stats   Tunables   WakeUp
> >> Good          Wireless Power Saving for interface wlan0
>    Good          VM writeback timeout
>    Good          Bluetooth device interface status
>    Good          NMI watchdog should be turned off
>    Good          Autosuspend for unknown USB device 2-1.3 (8087:0a2b)
>    Good          Autosuspend for USB device USB 2.0 Hub [2-1]
>    Good          Autosuspend for USB device Generic Platform OHCI
> controller [usb1]
>    Good          Autosuspend for USB device xHCI Host Controller [usb8]
>    Good          Autosuspend for USB device Generic Platform OHCI
> controller [usb4]
>    Good          Autosuspend for USB device EHCI Host Controller [usb2]
>    Good          Autosuspend for USB device xHCI Host Controller [usb6]
>    Good          Autosuspend for USB device EHCI Host Controller [usb3]
>    Good          Autosuspend for USB device xHCI Host Controller [usb5]
>    Good          Autosuspend for USB device xHCI Host Controller [usb7]
>    Good          Runtime PM for PCI Device Intel Corporation Wireless
> 8265 / 8275
>    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
>    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
>    Good          Runtime PM for PCI Device Realtek Semiconductor Co.,
> Ltd. RTL8125 2.5GbE Controller
>    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
>    Good          Runtime PM for PCI Device Samsung Electronics Co Ltd
> NVMe SSD Controller SM981/PM981/PM983
> 
> PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
> stats   Device Freq stats   Tunables   WakeUp
>               Usage     Device name
>               1.1%        CPU use
>             100.0%        Radio device: rfkill_gpio
>             100.0%        runtime-rockchip-gate-link-clk.712
>             100.0%        PCI Device: Realtek Semiconductor Co., Ltd.
> RTL8125 2.5GbE Controller
>             100.0%        runtime-rockchip-gate-link-clk.717
>             100.0%        runtime-rockchip-gate-link-clk.714
>             100.0%        runtime-rockchip-gate-link-clk.489
>             100.0%        runtime-a40000000.pcie
>             100.0%        runtime-a40800000.pcie
>             100.0%        runtime-rockchip-gate-link-clk.718
>             100.0%        runtime-rockchip-gate-link-clk.706
>             100.0%        runtime-rockchip-gate-link-clk.708
>             100.0%        PCI Device: Intel Corporation Wireless 8265 / 8275
>             100.0%        Radio device: btusb
>             100.0%        runtime-fcd00000.usb
>             100.0%        PCI Device: Samsung Electronics Co Ltd NVMe
> SSD Controller SM981/PM981/PM983
>             100.0%        Radio device: rfkill_gpio
>             100.0%        runtime-fc000000.usb
>             100.0%        Radio device: iwlwifi
>             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
>             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
>             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
>             100.0%        runtime-rockchip-gate-link-clk.711
>             100.0%        runtime-fc400000.usb
>             100.0%        runtime-rockchip-gate-link-clk.704
>             100.0%        runtime-rockchip-gate-link-clk.701
>             100.0%        runtime-rockchip-gate-link-clk.716
>             100.0%        runtime-rockchip-gate-link-clk.707
>             100.0%        runtime-rockchip-gate-link-clk.709
>             100.0%        runtime-rockchip-gate-link-clk.719
>             100.0%        runtime-xhci-hcd.1.auto
>             100.0%        runtime-feb50000.serial
>             100.0%        runtime-rockchip-gate-link-clk.715
>             100.0%        runtime-rockchip-gate-link-clk.710
> 
> > Again, a pointless patch.

This patch might make sense on its own, to enable runtime PM status of the
controller so that the runtime PM could be applied to the entire PCIe hierarchy.

> I implemented a .remove patch to ensure proper resource cleanup,
> which is a necessary step for successfully enabling and managing
> runtime power for the device.

How a dead code (remove callback for a always built-in driver) becomes necessary
for runtime PM.

- Mani

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

Re: [PATCH v1 2/2] PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver

[Anand Moon]

Hi Manivannan

On Fri, 31 Oct 2025 at 20:23, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Fri, Oct 31, 2025 at 07:33:23PM +0530, Anand Moon wrote:
> > Hi Manivannan
> >
> > Thanks for your review comment.
> >
> > On Fri, 31 Oct 2025 at 14:09, Manivannan Sadhasivam <mani@kernel.org> wrote:
> > >
> > > On Mon, Oct 27, 2025 at 08:25:30PM +0530, Anand Moon wrote:
> > > > Add runtime power management support to the Rockchip DesignWare PCIe
> > > > controller driver by enabling devm_pm_runtime() in the probe function.
> > > > These changes allow the PCIe controller to suspend and resume dynamically,
> > > > improving power efficiency on supported platforms.
> > > >
> > >
> > > Seriously? How can this patch improve the power efficiency if it is not doing
> > > any PM operation on its own?
> > >
> > I could verify that runtime power management is active
> >
>
> This is runtime status being active, which is a different thing as it only
> allows the runtime PM hierarchy to be maintained. But the way you described the
> commit message sounds like the patch is enabling runtime PM of the controller
> and that improves efficiency (as if the controller driver is actively doing
> runtime PM operations).
>
> > [root@rockpi-5b alarm]# cat
> > /sys/devices/platform/a41000000.pcie/power/runtime_status
> > active
> > [root@rockpi-5b alarm]#  find /sys -name runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/pci_bus/0004:41/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-3::lan/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-2::lan/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-1::lan/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/net/enP4p65s0/enP4p65s0-0::lan/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/power/runtime_status
> > /sys/devices/platform/a41000000.pcie/pci0004:40/0004:40:00.0/0004:41:00.0/mdio_bus/r8169-4-4100/r8169-4-4100:00/hwmon/hwmon11/power/runtime_status
> >
> > Well, the powertop shows that the runtime power management is enabled
> > on Radxa Rock 5b,
> >
> > PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
> > stats   Device Freq stats   Tunables   WakeUp
> > >> Good          Wireless Power Saving for interface wlan0
> >    Good          VM writeback timeout
> >    Good          Bluetooth device interface status
> >    Good          NMI watchdog should be turned off
> >    Good          Autosuspend for unknown USB device 2-1.3 (8087:0a2b)
> >    Good          Autosuspend for USB device USB 2.0 Hub [2-1]
> >    Good          Autosuspend for USB device Generic Platform OHCI
> > controller [usb1]
> >    Good          Autosuspend for USB device xHCI Host Controller [usb8]
> >    Good          Autosuspend for USB device Generic Platform OHCI
> > controller [usb4]
> >    Good          Autosuspend for USB device EHCI Host Controller [usb2]
> >    Good          Autosuspend for USB device xHCI Host Controller [usb6]
> >    Good          Autosuspend for USB device EHCI Host Controller [usb3]
> >    Good          Autosuspend for USB device xHCI Host Controller [usb5]
> >    Good          Autosuspend for USB device xHCI Host Controller [usb7]
> >    Good          Runtime PM for PCI Device Intel Corporation Wireless
> > 8265 / 8275
> >    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
> >    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
> >    Good          Runtime PM for PCI Device Realtek Semiconductor Co.,
> > Ltd. RTL8125 2.5GbE Controller
> >    Good          Runtime PM for PCI Device Rockchip Electronics Co., Ltd RK3588
> >    Good          Runtime PM for PCI Device Samsung Electronics Co Ltd
> > NVMe SSD Controller SM981/PM981/PM983
> >
> > PowerTOP 2.15     Overview   Idle stats   Frequency stats   Device
> > stats   Device Freq stats   Tunables   WakeUp
> >               Usage     Device name
> >               1.1%        CPU use
> >             100.0%        Radio device: rfkill_gpio
> >             100.0%        runtime-rockchip-gate-link-clk.712
> >             100.0%        PCI Device: Realtek Semiconductor Co., Ltd.
> > RTL8125 2.5GbE Controller
> >             100.0%        runtime-rockchip-gate-link-clk.717
> >             100.0%        runtime-rockchip-gate-link-clk.714
> >             100.0%        runtime-rockchip-gate-link-clk.489
> >             100.0%        runtime-a40000000.pcie
> >             100.0%        runtime-a40800000.pcie
> >             100.0%        runtime-rockchip-gate-link-clk.718
> >             100.0%        runtime-rockchip-gate-link-clk.706
> >             100.0%        runtime-rockchip-gate-link-clk.708
> >             100.0%        PCI Device: Intel Corporation Wireless 8265 / 8275
> >             100.0%        Radio device: btusb
> >             100.0%        runtime-fcd00000.usb
> >             100.0%        PCI Device: Samsung Electronics Co Ltd NVMe
> > SSD Controller SM981/PM981/PM983
> >             100.0%        Radio device: rfkill_gpio
> >             100.0%        runtime-fc000000.usb
> >             100.0%        Radio device: iwlwifi
> >             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
> >             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
> >             100.0%        PCI Device: Rockchip Electronics Co., Ltd RK3588
> >             100.0%        runtime-rockchip-gate-link-clk.711
> >             100.0%        runtime-fc400000.usb
> >             100.0%        runtime-rockchip-gate-link-clk.704
> >             100.0%        runtime-rockchip-gate-link-clk.701
> >             100.0%        runtime-rockchip-gate-link-clk.716
> >             100.0%        runtime-rockchip-gate-link-clk.707
> >             100.0%        runtime-rockchip-gate-link-clk.709
> >             100.0%        runtime-rockchip-gate-link-clk.719
> >             100.0%        runtime-xhci-hcd.1.auto
> >             100.0%        runtime-feb50000.serial
> >             100.0%        runtime-rockchip-gate-link-clk.715
> >             100.0%        runtime-rockchip-gate-link-clk.710
> >
> > > Again, a pointless patch.
>
> This patch might make sense on its own, to enable runtime PM status of the
> controller so that the runtime PM could be applied to the entire PCIe hierarchy.
>
I will update this in the commit message.
> > I implemented a .remove patch to ensure proper resource cleanup,
> > which is a necessary step for successfully enabling and managing
> > runtime power for the device.
>
> How a dead code (remove callback for a always built-in driver) becomes necessary
> for runtime PM.
>
Ok, understood we don't have platform_driver_unregister in
builtin_platform_driver
I will drop the first patch.

> - Mani
>
Thanks
-Annad
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v1 0/2] Add runtime PM support to Rockchip DW PCIe driver

[Nicolas Frattaroli]

On Monday, 27 October 2025 15:55:28 Central European Standard Time Anand Moon wrote:
> Introduce runtime power management support in the Rockchip DesignWare PCIe
> controller driver.  These changes allow the PCIe controller to suspend and
> resume dynamically, improving power efficiency on supported platforms.
> 
> Can Patch 1 can be backpoted to stable? It helps clean shutdown of PCIe.

You can do this by adding a Fixes tag to your patch. In your case, it
might be fixing whatever introduced the clk_bulk_prepare_enable, i.e.:

Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")

This would be put above your Signed-off-by in the first patch, after
the empty line.

To generate fixes tags like this, I use the following pretty format
in my .git/config:

    [pretty]
        fixes = Fixes: %h (\"%s\")

I can then do `git log --pretty=fixes` to show commits formatted
the right way. To find which commit to pick, `git blame` and
some sleuthing are helpful.

With this tag, stable bots can pick the commit into any release
that the commit it fixes is in.

Kind regards,
Nicolas Frattaroli

> 
> Clarification: the series is based on
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
> branch : controller/dw-rockchip
> 
> Thanks
> -Anand
> 
> Anand Moon (2):
>   PCI: dw-rockchip: Add remove callback for resource cleanup
>   PCI: dw-rockchip: Add runtime PM support to Rockchip PCIe driver
> 
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> 
> base-commit: 7ad31f88429369ada44710176e176256a2812c3f
> 

Re: [PATCH v1 0/2] Add runtime PM support to Rockchip DW PCIe driver

[Anand Moon]

Hi Nicolas,

On Mon, 27 Oct 2025 at 20:37, Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Monday, 27 October 2025 15:55:28 Central European Standard Time Anand Moon wrote:
> > Introduce runtime power management support in the Rockchip DesignWare PCIe
> > controller driver.  These changes allow the PCIe controller to suspend and
> > resume dynamically, improving power efficiency on supported platforms.
> >
> > Can Patch 1 can be backpoted to stable? It helps clean shutdown of PCIe.
>
> You can do this by adding a Fixes tag to your patch. In your case, it
> might be fixing whatever introduced the clk_bulk_prepare_enable, i.e.:
>
> Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
>
> This would be put above your Signed-off-by in the first patch, after
> the empty line.
>
> To generate fixes tags like this, I use the following pretty format
> in my .git/config:
>
>     [pretty]
>         fixes = Fixes: %h (\"%s\")
>
> I can then do `git log --pretty=fixes` to show commits formatted
> the right way. To find which commit to pick, `git blame` and
> some sleuthing are helpful.
>
> With this tag, stable bots can pick the commit into any release
> that the commit it fixes is in.
>
Thanks for your input.
I will leave this to the maintainers to decide on this.
> Kind regards,
> Nicolas Frattaroli
>
Thanks
-Anand