| Message ID | 20240224-pci-dbi-rework-v8-0-64c7fd0cfe64@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host | expand |
On Sat, Feb 24, 2024 at 12:24:15PM +0530, Manivannan Sadhasivam wrote: > Now that the API is available, let's make use of it. It also handles the > reinitialization of DWC non-sticky registers in addition to sending the > notification to EPF drivers. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > index 2fb8c15e7a91..4e45bc4bca45 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > @@ -640,7 +640,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) > if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) { > dev_dbg(dev, "Received Linkdown event\n"); > pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; > - pci_epc_linkdown(pci->ep.epc); > + dw_pcie_ep_linkdown(&pci->ep); Suppose pci_epc_linkdown() will call dw_pcie_ep_linkdown() ? why need direct call dw_pcie_ep_linkdown() here? Frank > } else if (FIELD_GET(PARF_INT_ALL_BME, status)) { > dev_dbg(dev, "Received BME event. Link is enabled!\n"); > pcie_ep->link_status = QCOM_PCIE_EP_LINK_ENABLED; > > -- > 2.25.1 >
On Mon, Feb 26, 2024 at 12:20:41PM -0500, Frank Li wrote: > On Sat, Feb 24, 2024 at 12:24:15PM +0530, Manivannan Sadhasivam wrote: > > Now that the API is available, let's make use of it. It also handles the > > reinitialization of DWC non-sticky registers in addition to sending the > > notification to EPF drivers. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > index 2fb8c15e7a91..4e45bc4bca45 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > @@ -640,7 +640,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) > > if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) { > > dev_dbg(dev, "Received Linkdown event\n"); > > pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; > > - pci_epc_linkdown(pci->ep.epc); > > + dw_pcie_ep_linkdown(&pci->ep); > > Suppose pci_epc_linkdown() will call dw_pcie_ep_linkdown() ? > why need direct call dw_pcie_ep_linkdown() here? > I've already justified this in the commit message. Here is the excerpt: "It also handles the reinitialization of DWC non-sticky registers in addition to sending the notification to EPF drivers." - Mani
On Tue, Feb 27, 2024 at 06:02:30PM +0530, Manivannan Sadhasivam wrote: > On Mon, Feb 26, 2024 at 12:20:41PM -0500, Frank Li wrote: > > On Sat, Feb 24, 2024 at 12:24:15PM +0530, Manivannan Sadhasivam wrote: > > > Now that the API is available, let's make use of it. It also handles the > > > reinitialization of DWC non-sticky registers in addition to sending the > > > notification to EPF drivers. > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > index 2fb8c15e7a91..4e45bc4bca45 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > @@ -640,7 +640,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) > > > if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) { > > > dev_dbg(dev, "Received Linkdown event\n"); > > > pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; > > > - pci_epc_linkdown(pci->ep.epc); > > > + dw_pcie_ep_linkdown(&pci->ep); > > > > Suppose pci_epc_linkdown() will call dw_pcie_ep_linkdown() ? > > why need direct call dw_pcie_ep_linkdown() here? > > > > I've already justified this in the commit message. Here is the excerpt: > > "It also handles the reinitialization of DWC non-sticky registers in addition > to sending the notification to EPF drivers." API function name is too similar. It is hard to know difference from API naming. It'd better to know what function do from function name. Frank > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 12:34:15PM -0500, Frank Li wrote: > On Tue, Feb 27, 2024 at 06:02:30PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Feb 26, 2024 at 12:20:41PM -0500, Frank Li wrote: > > > On Sat, Feb 24, 2024 at 12:24:15PM +0530, Manivannan Sadhasivam wrote: > > > > Now that the API is available, let's make use of it. It also handles the > > > > reinitialization of DWC non-sticky registers in addition to sending the > > > > notification to EPF drivers. > > > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > > index 2fb8c15e7a91..4e45bc4bca45 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > > @@ -640,7 +640,7 @@ static irqreturn_t qcom_pcie_ep_global_irq_thread(int irq, void *data) > > > > if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) { > > > > dev_dbg(dev, "Received Linkdown event\n"); > > > > pcie_ep->link_status = QCOM_PCIE_EP_LINK_DOWN; > > > > - pci_epc_linkdown(pci->ep.epc); > > > > + dw_pcie_ep_linkdown(&pci->ep); > > > > > > Suppose pci_epc_linkdown() will call dw_pcie_ep_linkdown() ? > > > why need direct call dw_pcie_ep_linkdown() here? > > > > > > > I've already justified this in the commit message. Here is the excerpt: > > > > "It also handles the reinitialization of DWC non-sticky registers in addition > > to sending the notification to EPF drivers." > > API function name is too similar. It is hard to know difference from API > naming. It'd better to know what function do from function name. > In reality we cannot name a function based on everything it does. The naming is mostly based on what is the primary motive of the API and here it is handling Link down event. Maybe dw_pcie_ep_handle_linkdown() would be an apt one, but that's out of scope of this series (since changing that would also require changes to other similar APIs). - Mani > Frank > > > > - Mani > > > > -- > > மணிவண்ணன் சதாசிவம்
Hello, This series is the continuation of previous work by Vidya Sagar [1] to fix the issues related to accessing DBI register space before completing the core initialization in some EP platforms like Tegra194/234 and Qcom EP. Since Vidya is busy, I took over the series based on his consent (off-list discussion). NOTE ==== Based on the comments received in v7 [2], I've heavily modified the series to fix several other issues reported by Bjorn and Niklas. One noticeable change is getting rid of the 'core_init_notifer' flag added to differentiate between glue drivers requiring refclk from host and drivers getting refclk locally. By getting rid of this flag, now both the DWC EP driver and the EPF drivers can use a single flow and need not distinguish between the glue drivers. We can also get rid of the 'link_up_notifier' flag in the future by following the same convention. Testing ======= I've tested the series on Qcom SM8450 based dev board that depends on refclk from host with EPF_MHI driver. It'd be good to test this series on platforms that generate refclk locally and also with EPF_TEST driver. - Mani [1] https://lore.kernel.org/linux-pci/20221013175712.7539-1-vidyas@nvidia.com/ [2] https://lore.kernel.org/linux-pci/20231120084014.108274-1-manivannan.sadhasivam@linaro.org/ Changes in v8: - Rebased on top of v6.8-rc1 - Removed the deinit callback from struct dw_pcie_ep_ops - Renamed dw_pcie_ep_exit() to dw_pcie_ep_deinit() - Introduced dw_pcie_ep_cleanup() API for drivers supporting PERST# - Renamed dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers() - Called dw_pcie_ep_init_registers() API directly from all glue drivers - Removed "core_init_notifier" flag - Added a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event and used it in qcom driver - Added Kernel-doc comments for DWC EP APIs Changes in v7: - Rebased on top of v6.7-rc1 - Kept the current dw_pcie_ep_init_complete() API instead of renaming it to dw_pcie_ep_init_late(), since changing the name causes a slight ambiguity. - Splitted the change that moves pci_epc_init_notify() inside dw_pcie_ep_init_notify() to help bisecting and also to avoid build issue. - Added a new patch that moves pci_epc_init_notify() inside dw_pcie_ep_init_notify(). - Took over the authorship and dropped the previous Ack as the patches are heavily modified. Changes in v6: - Rebased on top of pci/next (6e2fca71e187) - removed ep_init_late() callback as it is no longer necessary For previous changelog, please refer [1]. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Manivannan Sadhasivam (10): PCI: dwc: ep: Remove deinit() callback from struct dw_pcie_ep_ops PCI: dwc: ep: Rename dw_pcie_ep_exit() to dw_pcie_ep_deinit() PCI: dwc: ep: Introduce dw_pcie_ep_cleanup() API for drivers supporting PERST# PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host PCI: dwc: ep: Rename dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers() PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers PCI: dwc: ep: Remove "core_init_notifier" flag PCI: dwc: ep: Add a generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event PCI: qcom-ep: Use the generic dw_pcie_ep_linkdown() API to handle LINK_DOWN event PCI: dwc: ep: Add Kernel-doc comments for APIs drivers/pci/controller/dwc/pci-dra7xx.c | 9 + drivers/pci/controller/dwc/pci-imx6.c | 10 + drivers/pci/controller/dwc/pci-keystone.c | 11 + drivers/pci/controller/dwc/pci-layerscape-ep.c | 9 + drivers/pci/controller/dwc/pcie-designware-ep.c | 307 +++++++++++++++------- drivers/pci/controller/dwc/pcie-designware-plat.c | 11 + drivers/pci/controller/dwc/pcie-designware.h | 19 +- drivers/pci/controller/dwc/pcie-qcom-ep.c | 6 +- drivers/pci/controller/dwc/pcie-rcar-gen4.c | 28 +- drivers/pci/controller/dwc/pcie-tegra194.c | 5 +- drivers/pci/controller/dwc/pcie-uniphier-ep.c | 15 +- drivers/pci/endpoint/functions/pci-epf-test.c | 18 +- include/linux/pci-epc.h | 3 - 13 files changed, 321 insertions(+), 130 deletions(-) --- base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d change-id: 20240224-pci-dbi-rework-b2e99a62930c Best regards,