[PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core init

[PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core init

[Vidya Sagar]

This series attempts to fix the issue with core register (Ex:- DBI) accesses
causing system hang issues in platforms where there is a dependency on the
availability of PCIe Reference clock from the host for their core
initialization.
This series is verified on Tegra194 & Tegra234 platforms.

Manivannan, could you please verify on qcom platforms?

V4:
* Addressed review comments from Bjorn and Manivannan
* Added .ep_init_late() ops
* Added patches to refactor code in qcom and tegra platforms

Vidya Sagar (3):
  PCI: designware-ep: Fix DBI access before core init
  PCI: qcom-ep: Refactor EP initialization completion
  PCI: tegra194: Refactor EP initialization completion

 .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
 drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
 drivers/pci/controller/dwc/pcie-qcom-ep.c     |  27 +++--
 drivers/pci/controller/dwc/pcie-tegra194.c    |   4 +-
 4 files changed, 85 insertions(+), 68 deletions(-)

-- 
2.17.1

[PATCH V4 1/3] PCI: designware-ep: Fix DBI access before core init

[Vidya Sagar]

Platforms that cannot support their core initialization without the
reference clock from the host, implement the feature 'core_init_notifier'
to indicate the DesignWare sub-system about when their core is getting
initialized. Any accesses to the core (Ex:- DBI) would the core being
ready result in system hang in such systems (Ex:- tegra194).
This patch moves any access to the core to dw_pcie_ep_init_complete() API
which is effectively called only after the core initialization.
It also introduces .ep_init_late() ops hook to be used for any post init
work that platform drivers may have to do.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* Addressed review comments from Bjorn and Manivannan
* Moved dw_pcie_ep_init_complete() inside dw_pcie_ep_init_notify()
* Added .ep_init_late() ops to perform late init tasks

 .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
 drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
 2 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 83ddb190292e..095fb0291ec9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -23,14 +23,6 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
 
-void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
-{
-	struct pci_epc *epc = ep->epc;
-
-	pci_epc_init_notify(epc);
-}
-EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
-
 struct dw_pcie_ep_func *
 dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no)
 {
@@ -640,12 +632,17 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
 	return 0;
 }
 
-int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
+static int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct dw_pcie_ep_func *ep_func;
+	struct device *dev = pci->dev;
+	struct pci_epc *epc = ep->epc;
 	unsigned int offset;
 	unsigned int nbars;
 	u8 hdr_type;
+	u8 func_no;
+	void *addr;
 	u32 reg;
 	int i;
 
@@ -658,6 +655,40 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 		return -EIO;
 	}
 
+	dw_pcie_version_detect(pci);
+
+	dw_pcie_iatu_detect(pci);
+
+	ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
+					       GFP_KERNEL);
+	if (!ep->ib_window_map)
+		return -ENOMEM;
+
+	ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
+					       GFP_KERNEL);
+	if (!ep->ob_window_map)
+		return -ENOMEM;
+
+	addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
+			    GFP_KERNEL);
+	if (!addr)
+		return -ENOMEM;
+	ep->outbound_addr = addr;
+
+	for (func_no = 0; func_no < epc->max_functions; func_no++) {
+		ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
+		if (!ep_func)
+			return -ENOMEM;
+
+		ep_func->func_no = func_no;
+		ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
+							      PCI_CAP_ID_MSI);
+		ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
+							       PCI_CAP_ID_MSIX);
+
+		list_add_tail(&ep_func->list, &ep->func_list);
+	}
+
 	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
 
 	dw_pcie_dbi_ro_wr_en(pci);
@@ -676,13 +707,28 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
+
+int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
+{
+	struct pci_epc *epc = ep->epc;
+	int ret;
+
+	ret = dw_pcie_ep_init_complete(ep);
+	if (ret)
+		return ret;
+
+	if (ep->ops->ep_init_late)
+		ep->ops->ep_init_late(ep);
+
+	pci_epc_init_notify(epc);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
 
 int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 {
 	int ret;
-	void *addr;
-	u8 func_no;
 	struct resource *res;
 	struct pci_epc *epc;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
@@ -690,7 +736,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
 	const struct pci_epc_features *epc_features;
-	struct dw_pcie_ep_func *ep_func;
 
 	INIT_LIST_HEAD(&ep->func_list);
 
@@ -719,26 +764,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	ep->phys_base = res->start;
 	ep->addr_size = resource_size(res);
 
-	dw_pcie_version_detect(pci);
-
-	dw_pcie_iatu_detect(pci);
-
-	ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
-					       GFP_KERNEL);
-	if (!ep->ib_window_map)
-		return -ENOMEM;
-
-	ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
-					       GFP_KERNEL);
-	if (!ep->ob_window_map)
-		return -ENOMEM;
-
-	addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
-			    GFP_KERNEL);
-	if (!addr)
-		return -ENOMEM;
-	ep->outbound_addr = addr;
-
 	if (pci->link_gen < 1)
 		pci->link_gen = of_pci_get_max_link_speed(np);
 
@@ -755,20 +780,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	if (ret < 0)
 		epc->max_functions = 1;
 
-	for (func_no = 0; func_no < epc->max_functions; func_no++) {
-		ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
-		if (!ep_func)
-			return -ENOMEM;
-
-		ep_func->func_no = func_no;
-		ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
-							      PCI_CAP_ID_MSI);
-		ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
-							       PCI_CAP_ID_MSIX);
-
-		list_add_tail(&ep_func->list, &ep->func_list);
-	}
-
 	if (ep->ops->ep_init)
 		ep->ops->ep_init(ep);
 
@@ -793,6 +804,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 			return 0;
 	}
 
+	/*
+	 * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this
+	 * step as platforms that implement 'core_init_notifier' feature may
+	 * not have the hardware ready (i.e. core initialized) for access
+	 * (Ex: tegra194). Any hardware access on such platforms result
+	 * in system hard hang.
+	 */
 	ret = dw_pcie_ep_init_complete(ep);
 	if (ret)
 		goto err_free_epc_mem;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 09b887093a84..7222717afa33 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -254,6 +254,7 @@ struct dw_pcie_rp {
 
 struct dw_pcie_ep_ops {
 	void	(*ep_init)(struct dw_pcie_ep *ep);
+	void	(*ep_init_late)(struct dw_pcie_ep *ep);
 	int	(*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
 			     enum pci_epc_irq_type type, u16 interrupt_num);
 	const struct pci_epc_features* (*get_features)(struct dw_pcie_ep *ep);
@@ -467,8 +468,7 @@ static inline void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus,
 #ifdef CONFIG_PCIE_DW_EP
 void dw_pcie_ep_linkup(struct dw_pcie_ep *ep);
 int dw_pcie_ep_init(struct dw_pcie_ep *ep);
-int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep);
-void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
+int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep);
 void dw_pcie_ep_exit(struct dw_pcie_ep *ep);
 int dw_pcie_ep_raise_legacy_irq(struct dw_pcie_ep *ep, u8 func_no);
 int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
@@ -490,15 +490,11 @@ static inline int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	return 0;
 }
 
-static inline int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
+static inline int dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
 {
 	return 0;
 }
 
-static inline void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
-{
-}
-
 static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 {
 }
-- 
2.17.1

[PATCH V4 2/3] PCI: qcom-ep: Refactor EP initialization completion

[Vidya Sagar]

Move the post initialization code to .ep_init_late() call back and call
only dw_pcie_ep_init_notify() which internally takes care of calling
dw_pcie_ep_init_complete().

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* New patch in this series

 drivers/pci/controller/dwc/pcie-qcom-ep.c | 27 ++++++++++++++---------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index ec99116ad05c..cc7ce470640e 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -361,22 +361,12 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 	      PARF_INT_ALL_LINK_UP;
 	writel_relaxed(val, pcie_ep->parf + PARF_INT_ALL_MASK);
 
-	ret = dw_pcie_ep_init_complete(&pcie_ep->pci.ep);
+	ret = dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
 	if (ret) {
 		dev_err(dev, "Failed to complete initialization: %d\n", ret);
 		goto err_disable_resources;
 	}
 
-	/*
-	 * The physical address of the MMIO region which is exposed as the BAR
-	 * should be written to MHI BASE registers.
-	 */
-	writel_relaxed(pcie_ep->mmio_res->start,
-		       pcie_ep->parf + PARF_MHI_BASE_ADDR_LOWER);
-	writel_relaxed(0, pcie_ep->parf + PARF_MHI_BASE_ADDR_UPPER);
-
-	dw_pcie_ep_init_notify(&pcie_ep->pci.ep);
-
 	/* Enable LTSSM */
 	val = readl_relaxed(pcie_ep->parf + PARF_LTSSM);
 	val |= BIT(8);
@@ -639,8 +629,23 @@ static void qcom_pcie_ep_init(struct dw_pcie_ep *ep)
 		dw_pcie_ep_reset_bar(pci, bar);
 }
 
+static void qcom_pcie_ep_init_late(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci);
+
+	/*
+	 * The physical address of the MMIO region which is exposed as the BAR
+	 * should be written to MHI BASE registers.
+	 */
+	writel_relaxed(pcie_ep->mmio_res->start,
+		       pcie_ep->parf + PARF_MHI_BASE_ADDR_LOWER);
+	writel_relaxed(0, pcie_ep->parf + PARF_MHI_BASE_ADDR_UPPER);
+}
+
 static const struct dw_pcie_ep_ops pci_ep_ops = {
 	.ep_init = qcom_pcie_ep_init,
+	.ep_init_late = qcom_pcie_ep_init_late,
 	.raise_irq = qcom_pcie_ep_raise_irq,
 	.get_features = qcom_pcie_epc_get_features,
 };
-- 
2.17.1

[PATCH V4 3/3] PCI: tegra194: Refactor EP initialization completion

[Vidya Sagar]

Call only dw_pcie_ep_init_notify() which internally takes care of calling
dw_pcie_ep_init_complete() to notify about the EP initialization
completion to the DWC EP framework.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* New patch in this series

 drivers/pci/controller/dwc/pcie-tegra194.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 1b6b437823d2..2600304522eb 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1885,14 +1885,12 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
 	val = (upper_32_bits(ep->msi_mem_phys) & MSIX_ADDR_MATCH_HIGH_OFF_MASK);
 	dw_pcie_writel_dbi(pci, MSIX_ADDR_MATCH_HIGH_OFF, val);
 
-	ret = dw_pcie_ep_init_complete(ep);
+	ret = dw_pcie_ep_init_notify(ep);
 	if (ret) {
 		dev_err(dev, "Failed to complete initialization: %d\n", ret);
 		goto fail_init_complete;
 	}
 
-	dw_pcie_ep_init_notify(ep);
-
 	/* Program the private control to allow sending LTR upstream */
 	if (pcie->of_data->has_ltr_req_fix) {
 		val = appl_readl(pcie, APPL_LTR_MSG_2);
-- 
2.17.1

Re: [PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core init

[Bjorn Helgaas]

On Tue, Sep 20, 2022 at 12:03:39AM +0530, Vidya Sagar wrote:
> This series attempts to fix the issue with core register (Ex:- DBI) accesses
> causing system hang issues in platforms where there is a dependency on the
> availability of PCIe Reference clock from the host for their core
> initialization.
> This series is verified on Tegra194 & Tegra234 platforms.

I think this design is just kind of weird, specifically, the fact that
setting .core_init_notifier makes dw_pcie_ep_init() bail out early.
The usual pattern is more like "if the specific driver sets this
function pointer, the generic code calls it."

The name "dw_pcie_ep_init_complete()" is not as helpful as it could
be: it tells us something about what has happened before this point,
but it doesn't tell us anything about what dw_pcie_ep_init_complete()
*does*.

Same thing with dw_pcie_ep_init_notify() -- it doesn't tell us
anything about what the function *does*.  I see that it calls
pci_epc_init_notify(), which calls a notifier call chain (currently
always empty except for a test case).  I think pci_epc_linkup() is a
better name because it says something about what's happening: the link
is now up and we're telling somebody about it.  "pci_epc_init_notify()"
doesn't convey that.  "pci_epc_core_initialized()" might.

It looks like both qcom and tegra wait for an interrupt before calling
dw_pcie_ep_init_notify(), but I'm a little concerned because I can't
figure out what specifically they do to start the process that
ultimately generates the interrupt.  Presumably they request the IRQ
*before* starting the process, but there's not much between the
devm_request_threaded_irq() and the interrupt handler, which makes me
wonder if both are racy.

> Manivannan, could you please verify on qcom platforms?
> 
> V4:
> * Addressed review comments from Bjorn and Manivannan
> * Added .ep_init_late() ops
> * Added patches to refactor code in qcom and tegra platforms
> 
> Vidya Sagar (3):
>   PCI: designware-ep: Fix DBI access before core init
>   PCI: qcom-ep: Refactor EP initialization completion
>   PCI: tegra194: Refactor EP initialization completion
> 
>  .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
>  drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
>  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  27 +++--
>  drivers/pci/controller/dwc/pcie-tegra194.c    |   4 +-
>  4 files changed, 85 insertions(+), 68 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core init

[Vidya Sagar]

On 9/20/2022 4:10 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Sep 20, 2022 at 12:03:39AM +0530, Vidya Sagar wrote:
>> This series attempts to fix the issue with core register (Ex:- DBI) accesses
>> causing system hang issues in platforms where there is a dependency on the
>> availability of PCIe Reference clock from the host for their core
>> initialization.
>> This series is verified on Tegra194 & Tegra234 platforms.
> 
> I think this design is just kind of weird, specifically, the fact that
> setting .core_init_notifier makes dw_pcie_ep_init() bail out early.
> The usual pattern is more like "if the specific driver sets this
> function pointer, the generic code calls it."

Thanks for the review Bjorn.

Typically the PCIe endpoints run using the reference clock from the 
hosts that they are connected to. Our hardware designers followed the 
same approach here as well, but the main difference here being that the 
controllers operating in the endpoint mode are not standalone 
controllers but part of a bigger Tegra (/Qcom) systems.
So, the complete controller initialization sequence just can't happen 
during the boot stage itself, hence the boot initialization sequence 
needs to be split into two parts viz a) early initialization - that just 
parses DT, does the programming that doesn't depend on the reference 
clock from host and b) does the programming that can only be performed 
after reference clock is available from the host
We are working with our hardware designers to avoid this dependency on 
the reference clock from the host so that all the programming can happen 
during boot itself and hardware is smart enough to switch to using the 
reference clock from the host when it is available. But, this is for 
future designs and Tegra194 & Tegra234 continue to have this limitation.

> 
> The name "dw_pcie_ep_init_complete()" is not as helpful as it could
> be: it tells us something about what has happened before this point,
> but it doesn't tell us anything about what dw_pcie_ep_init_complete()
> *does*.

To be inline with new ops ep_init_late that I added in this series, 
would it be fine to name this as dw_pcie_ep_init_late()?

> 
> Same thing with dw_pcie_ep_init_notify() -- it doesn't tell us
> anything about what the function *does*.

Would it make more sense to rename it as dw_pcie_ep_linkup_notify()?

   I see that it calls
> pci_epc_init_notify(), which calls a notifier call chain (currently
> always empty except for a test case).  I think pci_epc_linkup() is a
> better name because it says something about what's happening: the link
> is now up and we're telling somebody about it.  "pci_epc_init_notify()"
> doesn't convey that.  "pci_epc_core_initialized()" might.

Ok. I'll rename it to pci_epc_core_initialized().

> 
> It looks like both qcom and tegra wait for an interrupt before calling
> dw_pcie_ep_init_notify(), but I'm a little concerned because I can't
> figure out what specifically they do to start the process that
> ultimately generates the interrupt.

As part of 'start'ing the endpoint as mentioned in 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/PCI/endpoint/pci-test-howto.rst#n101
we execute the following
echo 1 > controllers/141a0000.pcie-ep/start
that enables the interrupt generation for toggles on the PERST# line.

   Presumably they request the IRQ
> *before* starting the process, but there's not much between the
> devm_request_threaded_irq() and the interrupt handler, which makes me
> wonder if both are racy.

I don't think there is any race between these two as the 'start' is 
initiated from the user space. Not sure if I'm missing something here 
though.

> 
>> Manivannan, could you please verify on qcom platforms?
>>
>> V4:
>> * Addressed review comments from Bjorn and Manivannan
>> * Added .ep_init_late() ops
>> * Added patches to refactor code in qcom and tegra platforms
>>
>> Vidya Sagar (3):
>>    PCI: designware-ep: Fix DBI access before core init
>>    PCI: qcom-ep: Refactor EP initialization completion
>>    PCI: tegra194: Refactor EP initialization completion
>>
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
>>   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  27 +++--
>>   drivers/pci/controller/dwc/pcie-tegra194.c    |   4 +-
>>   4 files changed, 85 insertions(+), 68 deletions(-)
>>
>> --
>> 2.17.1
>>

Re: [PATCH V4 0/3] PCI: designware-ep: Fix DBI access before core init

[Vidya Sagar]

Hi Bjorn,
Did you find time to take a look at my responses?
If you don't have anything to add further, I'll take care of the review 
comments as mentioned and send the V5 patch for review.
Please let me know.

Thanks,
Vidya Sagar

On 9/26/2022 8:32 PM, Vidya Sagar wrote:
> 
> 
> On 9/20/2022 4:10 AM, Bjorn Helgaas wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Tue, Sep 20, 2022 at 12:03:39AM +0530, Vidya Sagar wrote:
>>> This series attempts to fix the issue with core register (Ex:- DBI) 
>>> accesses
>>> causing system hang issues in platforms where there is a dependency 
>>> on the
>>> availability of PCIe Reference clock from the host for their core
>>> initialization.
>>> This series is verified on Tegra194 & Tegra234 platforms.
>>
>> I think this design is just kind of weird, specifically, the fact that
>> setting .core_init_notifier makes dw_pcie_ep_init() bail out early.
>> The usual pattern is more like "if the specific driver sets this
>> function pointer, the generic code calls it."
> 
> Thanks for the review Bjorn.
> 
> Typically the PCIe endpoints run using the reference clock from the 
> hosts that they are connected to. Our hardware designers followed the 
> same approach here as well, but the main difference here being that the 
> controllers operating in the endpoint mode are not standalone 
> controllers but part of a bigger Tegra (/Qcom) systems.
> So, the complete controller initialization sequence just can't happen 
> during the boot stage itself, hence the boot initialization sequence 
> needs to be split into two parts viz a) early initialization - that just 
> parses DT, does the programming that doesn't depend on the reference 
> clock from host and b) does the programming that can only be performed 
> after reference clock is available from the host
> We are working with our hardware designers to avoid this dependency on 
> the reference clock from the host so that all the programming can happen 
> during boot itself and hardware is smart enough to switch to using the 
> reference clock from the host when it is available. But, this is for 
> future designs and Tegra194 & Tegra234 continue to have this limitation.
> 
>>
>> The name "dw_pcie_ep_init_complete()" is not as helpful as it could
>> be: it tells us something about what has happened before this point,
>> but it doesn't tell us anything about what dw_pcie_ep_init_complete()
>> *does*.
> 
> To be inline with new ops ep_init_late that I added in this series, 
> would it be fine to name this as dw_pcie_ep_init_late()?
> 
>>
>> Same thing with dw_pcie_ep_init_notify() -- it doesn't tell us
>> anything about what the function *does*.
> 
> Would it make more sense to rename it as dw_pcie_ep_linkup_notify()?
> 
>    I see that it calls
>> pci_epc_init_notify(), which calls a notifier call chain (currently
>> always empty except for a test case).  I think pci_epc_linkup() is a
>> better name because it says something about what's happening: the link
>> is now up and we're telling somebody about it.  "pci_epc_init_notify()"
>> doesn't convey that.  "pci_epc_core_initialized()" might.
> 
> Ok. I'll rename it to pci_epc_core_initialized().
> 
>>
>> It looks like both qcom and tegra wait for an interrupt before calling
>> dw_pcie_ep_init_notify(), but I'm a little concerned because I can't
>> figure out what specifically they do to start the process that
>> ultimately generates the interrupt.
> 
> As part of 'start'ing the endpoint as mentioned in 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/PCI/endpoint/pci-test-howto.rst#n101 
> 
> we execute the following
> echo 1 > controllers/141a0000.pcie-ep/start
> that enables the interrupt generation for toggles on the PERST# line.
> 
>    Presumably they request the IRQ
>> *before* starting the process, but there's not much between the
>> devm_request_threaded_irq() and the interrupt handler, which makes me
>> wonder if both are racy.
> 
> I don't think there is any race between these two as the 'start' is 
> initiated from the user space. Not sure if I'm missing something here 
> though.
> 
>>
>>> Manivannan, could you please verify on qcom platforms?
>>>
>>> V4:
>>> * Addressed review comments from Bjorn and Manivannan
>>> * Added .ep_init_late() ops
>>> * Added patches to refactor code in qcom and tegra platforms
>>>
>>> Vidya Sagar (3):
>>>    PCI: designware-ep: Fix DBI access before core init
>>>    PCI: qcom-ep: Refactor EP initialization completion
>>>    PCI: tegra194: Refactor EP initialization completion
>>>
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 112 ++++++++++--------
>>>   drivers/pci/controller/dwc/pcie-designware.h  |  10 +-
>>>   drivers/pci/controller/dwc/pcie-qcom-ep.c     |  27 +++--
>>>   drivers/pci/controller/dwc/pcie-tegra194.c    |   4 +-
>>>   4 files changed, 85 insertions(+), 68 deletions(-)
>>>
>>> -- 
>>> 2.17.1
>>>