| Message ID | 20250307103128.3287497-4-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] PCI: cadence: Add support to build pcie-cadence library as a kernel module | expand |
On Fri, Mar 07, 2025 at 04:01:27PM +0530, Siddharth Vadapalli wrote: > Introduce the helper function cdns_pcie_ep_disable() which will undo the > configuration performed by cdns_pcie_ep_setup(). Also, export it for use > by the existing callers of cdns_pcie_ep_setup(), thereby allowing them > to cleanup on their exit path. > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++ > drivers/pci/controller/cadence/pcie-cadence.h | 5 +++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > index eeb2afdd223e..85bec57fa5d9 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { > .get_features = cdns_pcie_ep_get_features, > }; > > +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) > +{ > + struct device *dev = ep->pcie.dev; > + struct pci_epc *epc = to_pci_epc(dev); > + pci_epc_deinit_notify() - Mani
On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote: Hello Mani, > On Fri, Mar 07, 2025 at 04:01:27PM +0530, Siddharth Vadapalli wrote: > > Introduce the helper function cdns_pcie_ep_disable() which will undo the > > configuration performed by cdns_pcie_ep_setup(). Also, export it for use > > by the existing callers of cdns_pcie_ep_setup(), thereby allowing them > > to cleanup on their exit path. > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > --- > > drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++ > > drivers/pci/controller/cadence/pcie-cadence.h | 5 +++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > index eeb2afdd223e..85bec57fa5d9 100644 > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { > > .get_features = cdns_pcie_ep_get_features, > > }; > > > > +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) > > +{ > > + struct device *dev = ep->pcie.dev; > > + struct pci_epc *epc = to_pci_epc(dev); > > + > > pci_epc_deinit_notify() I had initially planned to add this, but I noticed that it is not invoked by dw_pcie_ep_deinit() within drivers/pci/controller/dwc/pcie-designware-ep.c Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I decided to drop it. Current callers of pci_epc_deinit_notify() are: drivers/pci/controller/dwc/pcie-qcom-ep.c drivers/pci/controller/dwc/pcie-tegra194.c while there are many more users of dw_pcie_ep_deinit() that don't invoke pci_epc_deinit_notify(). While I don't intend to justify dropping pci_epc_deinit_notify() in the cleanup path, I wanted to check if this should be added to dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit() is different from cdns_pcie_ep_disable()? Please let me know. Regards, Siddharth.
On Tue, Mar 18, 2025 at 01:42:39PM +0530, Siddharth Vadapalli wrote: > On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote: > > Hello Mani, > > > On Fri, Mar 07, 2025 at 04:01:27PM +0530, Siddharth Vadapalli wrote: > > > Introduce the helper function cdns_pcie_ep_disable() which will undo the > > > configuration performed by cdns_pcie_ep_setup(). Also, export it for use > > > by the existing callers of cdns_pcie_ep_setup(), thereby allowing them > > > to cleanup on their exit path. > > > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > --- > > > drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++ > > > drivers/pci/controller/cadence/pcie-cadence.h | 5 +++++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > index eeb2afdd223e..85bec57fa5d9 100644 > > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { > > > .get_features = cdns_pcie_ep_get_features, > > > }; > > > > > > +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) > > > +{ > > > + struct device *dev = ep->pcie.dev; > > > + struct pci_epc *epc = to_pci_epc(dev); > > > + > > > > pci_epc_deinit_notify() > > I had initially planned to add this, but I noticed that it is not > invoked by dw_pcie_ep_deinit() within > drivers/pci/controller/dwc/pcie-designware-ep.c > Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I > decided to drop it. Current callers of pci_epc_deinit_notify() are: > drivers/pci/controller/dwc/pcie-qcom-ep.c > drivers/pci/controller/dwc/pcie-tegra194.c > while there are many more users of dw_pcie_ep_deinit() that don't invoke > pci_epc_deinit_notify(). > > While I don't intend to justify dropping pci_epc_deinit_notify() in the > cleanup path, I wanted to check if this should be added to > dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit() > is different from cdns_pcie_ep_disable()? Please let me know. > Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is supposed to be called while performing the resource cleanup with active refclk. Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is called when there is no refclk, it will crash the endpoint SoC. But since cadence endpoint platforms seem to generate their own refclk, you can call deinit_notify() during deinit phase. That said, I noticed some issues in the DWC cleanup path while checking the code now. Will submit fixes for them. - Mani
On Wed, Mar 19, 2025 at 04:02:17PM +0530, Manivannan Sadhasivam wrote: > On Tue, Mar 18, 2025 at 01:42:39PM +0530, Siddharth Vadapalli wrote: > > On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote: > > > > Hello Mani, > > > > > On Fri, Mar 07, 2025 at 04:01:27PM +0530, Siddharth Vadapalli wrote: > > > > Introduce the helper function cdns_pcie_ep_disable() which will undo the > > > > configuration performed by cdns_pcie_ep_setup(). Also, export it for use > > > > by the existing callers of cdns_pcie_ep_setup(), thereby allowing them > > > > to cleanup on their exit path. > > > > > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > --- > > > > drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++ > > > > drivers/pci/controller/cadence/pcie-cadence.h | 5 +++++ > > > > 2 files changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > > index eeb2afdd223e..85bec57fa5d9 100644 > > > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c > > > > @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { > > > > .get_features = cdns_pcie_ep_get_features, > > > > }; > > > > > > > > +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) > > > > +{ > > > > + struct device *dev = ep->pcie.dev; > > > > + struct pci_epc *epc = to_pci_epc(dev); > > > > + > > > > > > pci_epc_deinit_notify() > > > > I had initially planned to add this, but I noticed that it is not > > invoked by dw_pcie_ep_deinit() within > > drivers/pci/controller/dwc/pcie-designware-ep.c > > Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I > > decided to drop it. Current callers of pci_epc_deinit_notify() are: > > drivers/pci/controller/dwc/pcie-qcom-ep.c > > drivers/pci/controller/dwc/pcie-tegra194.c > > while there are many more users of dw_pcie_ep_deinit() that don't invoke > > pci_epc_deinit_notify(). > > > > While I don't intend to justify dropping pci_epc_deinit_notify() in the > > cleanup path, I wanted to check if this should be added to > > dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit() > > is different from cdns_pcie_ep_disable()? Please let me know. > > > > Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is > supposed to be called while performing the resource cleanup with active refclk. > > Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is > called when there is no refclk, it will crash the endpoint SoC. But since > cadence endpoint platforms seem to generate their own refclk, you can call > deinit_notify() during deinit phase. Thank you for the clarification. I will add pci_epc_deinit_notify() to cdns_pcie_ep_disable() in the v2 series. Regards, Siddharth.
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c index eeb2afdd223e..85bec57fa5d9 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = { .get_features = cdns_pcie_ep_get_features, }; +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) +{ + struct device *dev = ep->pcie.dev; + struct pci_epc *epc = to_pci_epc(dev); + + pci_epc_mem_free_addr(epc, ep->irq_phys_addr, ep->irq_cpu_addr, + SZ_128K); + pci_epc_mem_exit(epc); +} +EXPORT_SYMBOL_GPL(cdns_pcie_ep_disable); int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep) { diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h index 9068f140596a..c2b9dbb7e1db 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.h +++ b/drivers/pci/controller/cadence/pcie-cadence.h @@ -555,11 +555,16 @@ static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int d #if IS_ENABLED(CONFIG_PCIE_CADENCE_EP) int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep); +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep); #else static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep) { return 0; } + +static inline void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep) +{ +} #endif void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
Introduce the helper function cdns_pcie_ep_disable() which will undo the configuration performed by cdns_pcie_ep_setup(). Also, export it for use by the existing callers of cdns_pcie_ep_setup(), thereby allowing them to cleanup on their exit path. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++ drivers/pci/controller/cadence/pcie-cadence.h | 5 +++++ 2 files changed, 15 insertions(+)