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.1On 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
>
--
மணிவண்ணன் சதாசிவம்
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);
>
>
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 -- மணிவண்ணன் சதாசிவம்
在 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);
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
在 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
>
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
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);
>
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
© 2016 - 2026 Red Hat, Inc.