[PATCH 0/4] Add support to defer core initialization

[PATCH 0/4] Add support to defer core initialization

[Vidya Sagar]

EPC/DesignWare core endpoint subsystems assume that the core registers are
available always for SW to initialize. But, that may not be the case always.
For example, Tegra194 hardware has the core running on a clock that is derived
from reference clock that is coming into the endpoint system from host.
Hence core is made available asynchronously based on when host system is going
for enumeration of devices. To accommodate this kind of hardwares, support is
required to defer the core initialization until the respective platform driver
informs the EPC/DWC endpoint sub-systems that the core is indeed available for
initiaization. This patch series is attempting to add precisely that.
This series is based on Kishon's patch that adds notification mechanism
support from EPC to EPF @ http://patchwork.ozlabs.org/patch/1109884/

Vidya Sagar (4):
  PCI: dwc: Add new feature to skip core initialization
  PCI: endpoint: Add notification for core init completion
  PCI: dwc: Add API to notify core initialization completion
  PCI: pci-epf-test: Add support to defer core initialization

 .../pci/controller/dwc/pcie-designware-ep.c   |  79 +++++++-----
 drivers/pci/controller/dwc/pcie-designware.h  |  11 ++
 drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
 drivers/pci/endpoint/pci-epc-core.c           |  19 ++-
 include/linux/pci-epc.h                       |   2 +
 include/linux/pci-epf.h                       |   5 +
 6 files changed, 164 insertions(+), 66 deletions(-)

-- 
2.17.1

[PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Vidya Sagar]

Add a new feature 'skip_core_init' that can be set by platform drivers
of devices that do not have their core registers available until reference
clock from host is available (Ex:- Tegra194) to indicate DesignWare
endpoint mode sub-system to not perform core registers initialization.
Existing dw_pcie_ep_init() is refactored and all the code that touches
registers is extracted to form a new API dw_pcie_ep_init_complete() that
can be called later by platform drivers setting 'skip_core_init' to '1'.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
 drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
 include/linux/pci-epc.h                       |  1 +
 3 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 3dd2e2697294..06f4379be8a3 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -492,19 +492,53 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
 	return 0;
 }
 
-int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 {
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	unsigned int offset;
+	unsigned int nbars;
+	u8 hdr_type;
+	u32 reg;
 	int i;
+
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev,
+			"PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
+	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
+
+	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
+
+	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
+	if (offset) {
+		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
+		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
+			PCI_REBAR_CTRL_NBAR_SHIFT;
+
+		dw_pcie_dbi_ro_wr_en(pci);
+		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
+			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
+		dw_pcie_dbi_ro_wr_dis(pci);
+	}
+
+	dw_pcie_setup(pci);
+
+	return 0;
+}
+
+int dw_pcie_ep_init(struct dw_pcie_ep *ep)
+{
 	int ret;
-	u32 reg;
 	void *addr;
-	u8 hdr_type;
-	unsigned int nbars;
-	unsigned int offset;
 	struct pci_epc *epc;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
+	const struct pci_epc_features *epc_features;
 
 	if (!pci->dbi_base || !pci->dbi_base2) {
 		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
@@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	if (ep->ops->ep_init)
 		ep->ops->ep_init(ep);
 
-	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
-	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
-		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
-			hdr_type);
-		return -EIO;
-	}
-
 	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
 	if (ret < 0)
 		epc->max_functions = 1;
@@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
 		return -ENOMEM;
 	}
-	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
 
-	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
-
-	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
-	if (offset) {
-		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
-		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
-			PCI_REBAR_CTRL_NBAR_SHIFT;
-
-		dw_pcie_dbi_ro_wr_en(pci);
-		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
-			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
-		dw_pcie_dbi_ro_wr_dis(pci);
+	if (ep->ops->get_features) {
+		epc_features = ep->ops->get_features(ep);
+		if (epc_features->skip_core_init)
+			return 0;
 	}
 
-	dw_pcie_setup(pci);
-
-	return 0;
+	return dw_pcie_ep_init_complete(ep);
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 5accdd6bc388..340783e9032e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
 #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_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,
@@ -416,6 +417,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)
+{
+	return 0;
+}
+
 static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 {
 }
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 36644ccd32ac..241e6a6f39fb 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -121,6 +121,7 @@ struct pci_epc_features {
 	u8	bar_fixed_64bit;
 	u64	bar_fixed_size[PCI_STD_NUM_BARS];
 	size_t	align;
+	bool	skip_core_init;
 };
 
 #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
-- 
2.17.1

[PATCH 2/4] PCI: endpoint: Add notification for core init completion

[Vidya Sagar]

Add support to send notifications to EPF from EPC once the core
registers initialization is complete.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/endpoint/pci-epc-core.c | 19 ++++++++++++++++++-
 include/linux/pci-epc.h             |  1 +
 include/linux/pci-epf.h             |  5 +++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
index 2f6436599fcb..fcc3f7fb19c0 100644
--- a/drivers/pci/endpoint/pci-epc-core.c
+++ b/drivers/pci/endpoint/pci-epc-core.c
@@ -542,10 +542,27 @@ void pci_epc_linkup(struct pci_epc *epc)
 	if (!epc || IS_ERR(epc))
 		return;
 
-	atomic_notifier_call_chain(&epc->notifier, 0, NULL);
+	atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL);
 }
 EXPORT_SYMBOL_GPL(pci_epc_linkup);
 
+/**
+ * pci_epc_init_notify() - Notify the EPF device that EPC device's core
+ *			   initialization is completed.
+ * @epc: the EPC device whose core initialization is completeds
+ *
+ * Invoke to Notify the EPF device that the EPC device's initialization
+ * is completed.
+ */
+void pci_epc_init_notify(struct pci_epc *epc)
+{
+	if (!epc || IS_ERR(epc))
+		return;
+
+	atomic_notifier_call_chain(&epc->notifier, CORE_INIT, NULL);
+}
+EXPORT_SYMBOL_GPL(pci_epc_init_notify);
+
 /**
  * pci_epc_destroy() - destroy the EPC device
  * @epc: the EPC device that has to be destroyed
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index 241e6a6f39fb..c670543e42f9 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -160,6 +160,7 @@ void devm_pci_epc_destroy(struct device *dev, struct pci_epc *epc);
 void pci_epc_destroy(struct pci_epc *epc);
 int pci_epc_add_epf(struct pci_epc *epc, struct pci_epf *epf);
 void pci_epc_linkup(struct pci_epc *epc);
+void pci_epc_init_notify(struct pci_epc *epc);
 void pci_epc_remove_epf(struct pci_epc *epc, struct pci_epf *epf);
 int pci_epc_write_header(struct pci_epc *epc, u8 func_no,
 			 struct pci_epf_header *hdr);
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 4993f7f6439b..3cb65ac1648c 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -15,6 +15,11 @@
 
 struct pci_epf;
 
+enum pci_notify_event {
+	CORE_INIT,
+	LINK_UP,
+};
+
 enum pci_barno {
 	BAR_0,
 	BAR_1,
-- 
2.17.1

[PATCH 3/4] PCI: dwc: Add API to notify core initialization completion

[Vidya Sagar]

Add a new API dw_pcie_ep_init_notify() to let platform drivers
call it when the core is available for initialization.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 7 +++++++
 drivers/pci/controller/dwc/pcie-designware.h    | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 06f4379be8a3..1355e1d4d426 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -19,6 +19,13 @@ void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
 	pci_epc_linkup(epc);
 }
 
+void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
+{
+	struct pci_epc *epc = ep->epc;
+
+	pci_epc_init_notify(epc);
+}
+
 static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
 				   int flags)
 {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 340783e9032e..f62c5bae6b2d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -400,6 +400,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
 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);
 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,
@@ -422,6 +423,10 @@ static inline int dw_pcie_ep_init_complete(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 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Vidya Sagar]

Add support to defer core initialization and to receive a notifier
when core is ready to accommodate platforms where core is not for
initialization untile reference clock from host is available.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
 1 file changed, 77 insertions(+), 37 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index bddff15052cc..068024fab544 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 			   msecs_to_jiffies(1));
 }
 
-static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
-				 void *data)
-{
-	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
-	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
-
-	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
-			   msecs_to_jiffies(1));
-
-	return NOTIFY_OK;
-}
-
 static void pci_epf_test_unbind(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 	return 0;
 }
 
+static int pci_epf_test_core_init(struct pci_epf *epf)
+{
+	struct pci_epf_header *header = epf->header;
+	const struct pci_epc_features *epc_features;
+	struct pci_epc *epc = epf->epc;
+	struct device *dev = &epf->dev;
+	bool msix_capable = false;
+	bool msi_capable = true;
+	int ret;
+
+	epc_features = pci_epc_get_features(epc, epf->func_no);
+	if (epc_features) {
+		msix_capable = epc_features->msix_capable;
+		msi_capable = epc_features->msi_capable;
+	}
+
+	ret = pci_epc_write_header(epc, epf->func_no, header);
+	if (ret) {
+		dev_err(dev, "Configuration header write failed\n");
+		return ret;
+	}
+
+	ret = pci_epf_test_set_bar(epf);
+	if (ret)
+		return ret;
+
+	if (msi_capable) {
+		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
+		if (ret) {
+			dev_err(dev, "MSI configuration failed\n");
+			return ret;
+		}
+	}
+
+	if (msix_capable) {
+		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
+		if (ret) {
+			dev_err(dev, "MSI-X configuration failed\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
+				 void *data)
+{
+	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	int ret;
+
+	switch (val) {
+	case CORE_INIT:
+		ret = pci_epf_test_core_init(epf);
+		if (ret)
+			return NOTIFY_BAD;
+		break;
+
+	case LINK_UP:
+		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
+				   msecs_to_jiffies(1));
+		break;
+
+	default:
+		dev_err(&epf->dev, "Invalid EPF test notifier event\n");
+		return NOTIFY_BAD;
+	}
+
+	return NOTIFY_OK;
+}
+
 static int pci_epf_test_alloc_space(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 {
 	int ret;
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
-	struct pci_epf_header *header = epf->header;
 	const struct pci_epc_features *epc_features;
 	enum pci_barno test_reg_bar = BAR_0;
 	struct pci_epc *epc = epf->epc;
-	struct device *dev = &epf->dev;
 	bool linkup_notifier = false;
+	bool skip_core_init = false;
 	bool msix_capable = false;
 	bool msi_capable = true;
 
@@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	epc_features = pci_epc_get_features(epc, epf->func_no);
 	if (epc_features) {
 		linkup_notifier = epc_features->linkup_notifier;
+		skip_core_init = epc_features->skip_core_init;
 		msix_capable = epc_features->msix_capable;
 		msi_capable = epc_features->msi_capable;
 		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
@@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	epf_test->test_reg_bar = test_reg_bar;
 	epf_test->epc_features = epc_features;
 
-	ret = pci_epc_write_header(epc, epf->func_no, header);
-	if (ret) {
-		dev_err(dev, "Configuration header write failed\n");
-		return ret;
-	}
-
 	ret = pci_epf_test_alloc_space(epf);
 	if (ret)
 		return ret;
 
-	ret = pci_epf_test_set_bar(epf);
-	if (ret)
-		return ret;
-
-	if (msi_capable) {
-		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
-		if (ret) {
-			dev_err(dev, "MSI configuration failed\n");
-			return ret;
-		}
-	}
-
-	if (msix_capable) {
-		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
-		if (ret) {
-			dev_err(dev, "MSI-X configuration failed\n");
+	if (!skip_core_init) {
+		ret = pci_epf_test_core_init(epf);
+		if (ret)
 			return ret;
-		}
 	}
 
 	if (linkup_notifier) {
-- 
2.17.1

Re: [PATCH 0/4] Add support to defer core initialization

[Vidya Sagar]

On 11/13/2019 2:38 PM, Vidya Sagar wrote:
> EPC/DesignWare core endpoint subsystems assume that the core registers are
> available always for SW to initialize. But, that may not be the case always.
> For example, Tegra194 hardware has the core running on a clock that is derived
> from reference clock that is coming into the endpoint system from host.
> Hence core is made available asynchronously based on when host system is going
> for enumeration of devices. To accommodate this kind of hardwares, support is
> required to defer the core initialization until the respective platform driver
> informs the EPC/DWC endpoint sub-systems that the core is indeed available for
> initiaization. This patch series is attempting to add precisely that.
> This series is based on Kishon's patch that adds notification mechanism
> support from EPC to EPF @ http://patchwork.ozlabs.org/patch/1109884/
> 
> Vidya Sagar (4):
>    PCI: dwc: Add new feature to skip core initialization
>    PCI: endpoint: Add notification for core init completion
>    PCI: dwc: Add API to notify core initialization completion
>    PCI: pci-epf-test: Add support to defer core initialization
> 
>   .../pci/controller/dwc/pcie-designware-ep.c   |  79 +++++++-----
>   drivers/pci/controller/dwc/pcie-designware.h  |  11 ++
>   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>   drivers/pci/endpoint/pci-epc-core.c           |  19 ++-
>   include/linux/pci-epc.h                       |   2 +
>   include/linux/pci-epf.h                       |   5 +
>   6 files changed, 164 insertions(+), 66 deletions(-)
> 

Hi Kishon / Gustavo / Jingoo,
Could you please take a look at this patch series?

- Vidya Sagar

Re: [PATCH 0/4] Add support to defer core initialization

[Jingoo Han]

On 11/18/19, 1:55 AM, Vidya Sagar wrote:
> 
> On 11/13/2019 2:38 PM, Vidya Sagar wrote:
> > EPC/DesignWare core endpoint subsystems assume that the core registers are
> > available always for SW to initialize. But, that may not be the case always.
> > For example, Tegra194 hardware has the core running on a clock that is derived
> > from reference clock that is coming into the endpoint system from host.
> > Hence core is made available asynchronously based on when host system is going
> > for enumeration of devices. To accommodate this kind of hardwares, support is
> > required to defer the core initialization until the respective platform driver
> > informs the EPC/DWC endpoint sub-systems that the core is indeed available for
> > initiaization. This patch series is attempting to add precisely that.
> > This series is based on Kishon's patch that adds notification mechanism
> > support from EPC to EPF @ http://patchwork.ozlabs.org/patch/1109884/
> > 
> > Vidya Sagar (4):
> >    PCI: dwc: Add new feature to skip core initialization
> >    PCI: endpoint: Add notification for core init completion
> >    PCI: dwc: Add API to notify core initialization completion
> >    PCI: pci-epf-test: Add support to defer core initialization
> > 
> >   .../pci/controller/dwc/pcie-designware-ep.c   |  79 +++++++-----
> >   drivers/pci/controller/dwc/pcie-designware.h  |  11 ++
> >   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
> >   drivers/pci/endpoint/pci-epc-core.c           |  19 ++-
> >   include/linux/pci-epc.h                       |   2 +
> >   include/linux/pci-epf.h                       |   5 +
> >   6 files changed, 164 insertions(+), 66 deletions(-)
> > 
>
> Hi Kishon / Gustavo / Jingoo,
> Could you please take a look at this patch series?

You need a Ack from Kishon, because he made EP code.


> - Vidya Sagar

Re: [PATCH 0/4] Add support to defer core initialization

[Vidya Sagar]

On 11/18/2019 10:13 PM, Jingoo Han wrote:
> 
> 
> On 11/18/19, 1:55 AM, Vidya Sagar wrote:
>>
>> On 11/13/2019 2:38 PM, Vidya Sagar wrote:
>>> EPC/DesignWare core endpoint subsystems assume that the core registers are
>>> available always for SW to initialize. But, that may not be the case always.
>>> For example, Tegra194 hardware has the core running on a clock that is derived
>>> from reference clock that is coming into the endpoint system from host.
>>> Hence core is made available asynchronously based on when host system is going
>>> for enumeration of devices. To accommodate this kind of hardwares, support is
>>> required to defer the core initialization until the respective platform driver
>>> informs the EPC/DWC endpoint sub-systems that the core is indeed available for
>>> initiaization. This patch series is attempting to add precisely that.
>>> This series is based on Kishon's patch that adds notification mechanism
>>> support from EPC to EPF @ http://patchwork.ozlabs.org/patch/1109884/
>>>
>>> Vidya Sagar (4):
>>>     PCI: dwc: Add new feature to skip core initialization
>>>     PCI: endpoint: Add notification for core init completion
>>>     PCI: dwc: Add API to notify core initialization completion
>>>     PCI: pci-epf-test: Add support to defer core initialization
>>>
>>>    .../pci/controller/dwc/pcie-designware-ep.c   |  79 +++++++-----
>>>    drivers/pci/controller/dwc/pcie-designware.h  |  11 ++
>>>    drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>>    drivers/pci/endpoint/pci-epc-core.c           |  19 ++-
>>>    include/linux/pci-epc.h                       |   2 +
>>>    include/linux/pci-epf.h                       |   5 +
>>>    6 files changed, 164 insertions(+), 66 deletions(-)
>>>
>>
>> Hi Kishon / Gustavo / Jingoo,
>> Could you please take a look at this patch series?
> 
> You need a Ack from Kishon, because he made EP code.
Hi Kishon,
Could you please find time to review this series?

- Vidya Sagar
> 
> 
>> - Vidya Sagar

Re: [PATCH 0/4] Add support to defer core initialization

[Kishon Vijay Abraham I]

Hi,

On 25/11/19 10:03 AM, Vidya Sagar wrote:
> On 11/18/2019 10:13 PM, Jingoo Han wrote:
>>
>>
>> On 11/18/19, 1:55 AM, Vidya Sagar wrote:
>>>
>>> On 11/13/2019 2:38 PM, Vidya Sagar wrote:
>>>> EPC/DesignWare core endpoint subsystems assume that the core registers are
>>>> available always for SW to initialize. But, that may not be the case always.
>>>> For example, Tegra194 hardware has the core running on a clock that is derived
>>>> from reference clock that is coming into the endpoint system from host.
>>>> Hence core is made available asynchronously based on when host system is going
>>>> for enumeration of devices. To accommodate this kind of hardwares, support is
>>>> required to defer the core initialization until the respective platform driver
>>>> informs the EPC/DWC endpoint sub-systems that the core is indeed available for
>>>> initiaization. This patch series is attempting to add precisely that.
>>>> This series is based on Kishon's patch that adds notification mechanism
>>>> support from EPC to EPF @ http://patchwork.ozlabs.org/patch/1109884/
>>>>
>>>> Vidya Sagar (4):
>>>>     PCI: dwc: Add new feature to skip core initialization
>>>>     PCI: endpoint: Add notification for core init completion
>>>>     PCI: dwc: Add API to notify core initialization completion
>>>>     PCI: pci-epf-test: Add support to defer core initialization
>>>>
>>>>    .../pci/controller/dwc/pcie-designware-ep.c   |  79 +++++++-----
>>>>    drivers/pci/controller/dwc/pcie-designware.h  |  11 ++
>>>>    drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>>>    drivers/pci/endpoint/pci-epc-core.c           |  19 ++-
>>>>    include/linux/pci-epc.h                       |   2 +
>>>>    include/linux/pci-epf.h                       |   5 +
>>>>    6 files changed, 164 insertions(+), 66 deletions(-)
>>>>
>>>
>>> Hi Kishon / Gustavo / Jingoo,
>>> Could you please take a look at this patch series?
>>
>> You need a Ack from Kishon, because he made EP code.
> Hi Kishon,
> Could you please find time to review this series?

I'll review it this week. Sorry for the delay.

-Kishon

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Kishon Vijay Abraham I]

Hi,

On 13/11/19 2:38 PM, Vidya Sagar wrote:
> Add a new feature 'skip_core_init' that can be set by platform drivers
> of devices that do not have their core registers available until reference
> clock from host is available (Ex:- Tegra194) to indicate DesignWare
> endpoint mode sub-system to not perform core registers initialization.
> Existing dw_pcie_ep_init() is refactored and all the code that touches
> registers is extracted to form a new API dw_pcie_ep_init_complete() that
> can be called later by platform drivers setting 'skip_core_init' to '1'.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
>  drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
>  include/linux/pci-epc.h                       |  1 +
>  3 files changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 3dd2e2697294..06f4379be8a3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -492,19 +492,53 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>  	return 0;
>  }
>  
> -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	unsigned int offset;
> +	unsigned int nbars;
> +	u8 hdr_type;
> +	u32 reg;
>  	int i;
> +
> +	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> +	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> +		dev_err(pci->dev,
> +			"PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
> +			hdr_type);
> +		return -EIO;
> +	}
> +
> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +
> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
> +
> +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> +	if (offset) {
> +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> +			PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +		dw_pcie_dbi_ro_wr_en(pci);
> +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> +		dw_pcie_dbi_ro_wr_dis(pci);
> +	}
> +
> +	dw_pcie_setup(pci);
> +
> +	return 0;
> +}
> +
> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
>  	int ret;
> -	u32 reg;
>  	void *addr;
> -	u8 hdr_type;
> -	unsigned int nbars;
> -	unsigned int offset;
>  	struct pci_epc *epc;
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct device *dev = pci->dev;
>  	struct device_node *np = dev->of_node;
> +	const struct pci_epc_features *epc_features;
>  
>  	if (!pci->dbi_base || !pci->dbi_base2) {
>  		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
> @@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ep->ops->ep_init)
>  		ep->ops->ep_init(ep);
>  
> -	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> -	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> -		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
> -			hdr_type);
> -		return -EIO;
> -	}
> -
>  	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>  	if (ret < 0)
>  		epc->max_functions = 1;
> @@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>  		return -ENOMEM;
>  	}
> -	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>  
> -	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
> -
> -	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> -	if (offset) {
> -		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> -		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> -			PCI_REBAR_CTRL_NBAR_SHIFT;
> -
> -		dw_pcie_dbi_ro_wr_en(pci);
> -		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> -			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> -		dw_pcie_dbi_ro_wr_dis(pci);
> +	if (ep->ops->get_features) {
> +		epc_features = ep->ops->get_features(ep);
> +		if (epc_features->skip_core_init)
> +			return 0;
>  	}
>  
> -	dw_pcie_setup(pci);
> -
> -	return 0;
> +	return dw_pcie_ep_init_complete(ep);
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 5accdd6bc388..340783e9032e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
>  #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_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,
> @@ -416,6 +417,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)
> +{
> +	return 0;
> +}
> +
>  static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>  {
>  }
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 36644ccd32ac..241e6a6f39fb 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -121,6 +121,7 @@ struct pci_epc_features {
>  	u8	bar_fixed_64bit;
>  	u64	bar_fixed_size[PCI_STD_NUM_BARS];
>  	size_t	align;
> +	bool	skip_core_init;

This looks more like a designware specific change. Why is it added to the core
pci_epc_features?

Thanks
Kishon

Re: [PATCH 2/4] PCI: endpoint: Add notification for core init completion

[Kishon Vijay Abraham I]

Hi,

On 13/11/19 2:38 PM, Vidya Sagar wrote:
> Add support to send notifications to EPF from EPC once the core
> registers initialization is complete.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/endpoint/pci-epc-core.c | 19 ++++++++++++++++++-
>  include/linux/pci-epc.h             |  1 +
>  include/linux/pci-epf.h             |  5 +++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 2f6436599fcb..fcc3f7fb19c0 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
> @@ -542,10 +542,27 @@ void pci_epc_linkup(struct pci_epc *epc)
>  	if (!epc || IS_ERR(epc))
>  		return;
>  
> -	atomic_notifier_call_chain(&epc->notifier, 0, NULL);
> +	atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL);
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_linkup);

Is this based on upstream kernel? or did you create this after applying [1]?


[1] -> https://lkml.org/lkml/2019/6/4/633

Thanks
Kishon

Re: [PATCH 2/4] PCI: endpoint: Add notification for core init completion

[Kishon Vijay Abraham I]

Hi,

On 27/11/19 1:48 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>> Add support to send notifications to EPF from EPC once the core
>> registers initialization is complete.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>  drivers/pci/endpoint/pci-epc-core.c | 19 ++++++++++++++++++-
>>  include/linux/pci-epc.h             |  1 +
>>  include/linux/pci-epf.h             |  5 +++++
>>  3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>> index 2f6436599fcb..fcc3f7fb19c0 100644
>> --- a/drivers/pci/endpoint/pci-epc-core.c
>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>> @@ -542,10 +542,27 @@ void pci_epc_linkup(struct pci_epc *epc)
>>  	if (!epc || IS_ERR(epc))
>>  		return;
>>  
>> -	atomic_notifier_call_chain(&epc->notifier, 0, NULL);
>> +	atomic_notifier_call_chain(&epc->notifier, LINK_UP, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(pci_epc_linkup);
> 
> Is this based on upstream kernel? or did you create this after applying [1]?

never mind, you've mentioned this depends on my other series in your cover letter.

Thanks
Kishon

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Vidya Sagar]

On 11/27/2019 1:44 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>> Add a new feature 'skip_core_init' that can be set by platform drivers
>> of devices that do not have their core registers available until reference
>> clock from host is available (Ex:- Tegra194) to indicate DesignWare
>> endpoint mode sub-system to not perform core registers initialization.
>> Existing dw_pcie_ep_init() is refactored and all the code that touches
>> registers is extracted to form a new API dw_pcie_ep_init_complete() that
>> can be called later by platform drivers setting 'skip_core_init' to '1'.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
>>   include/linux/pci-epc.h                       |  1 +
>>   3 files changed, 51 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index 3dd2e2697294..06f4379be8a3 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -492,19 +492,53 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>>   	return 0;
>>   }
>>   
>> -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> +int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>   {
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	unsigned int offset;
>> +	unsigned int nbars;
>> +	u8 hdr_type;
>> +	u32 reg;
>>   	int i;
>> +
>> +	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>> +	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>> +		dev_err(pci->dev,
>> +			"PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
>> +			hdr_type);
>> +		return -EIO;
>> +	}
>> +
>> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>> +
>> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>> +
>> +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>> +	if (offset) {
>> +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>> +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>> +			PCI_REBAR_CTRL_NBAR_SHIFT;
>> +
>> +		dw_pcie_dbi_ro_wr_en(pci);
>> +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>> +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>> +		dw_pcie_dbi_ro_wr_dis(pci);
>> +	}
>> +
>> +	dw_pcie_setup(pci);
>> +
>> +	return 0;
>> +}
>> +
>> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>>   	int ret;
>> -	u32 reg;
>>   	void *addr;
>> -	u8 hdr_type;
>> -	unsigned int nbars;
>> -	unsigned int offset;
>>   	struct pci_epc *epc;
>>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>   	struct device *dev = pci->dev;
>>   	struct device_node *np = dev->of_node;
>> +	const struct pci_epc_features *epc_features;
>>   
>>   	if (!pci->dbi_base || !pci->dbi_base2) {
>>   		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
>> @@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>   	if (ep->ops->ep_init)
>>   		ep->ops->ep_init(ep);
>>   
>> -	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>> -	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>> -		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
>> -			hdr_type);
>> -		return -EIO;
>> -	}
>> -
>>   	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>>   	if (ret < 0)
>>   		epc->max_functions = 1;
>> @@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>   		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>>   		return -ENOMEM;
>>   	}
>> -	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>>   
>> -	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>> -
>> -	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>> -	if (offset) {
>> -		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>> -		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>> -			PCI_REBAR_CTRL_NBAR_SHIFT;
>> -
>> -		dw_pcie_dbi_ro_wr_en(pci);
>> -		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>> -			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>> -		dw_pcie_dbi_ro_wr_dis(pci);
>> +	if (ep->ops->get_features) {
>> +		epc_features = ep->ops->get_features(ep);
>> +		if (epc_features->skip_core_init)
>> +			return 0;
>>   	}
>>   
>> -	dw_pcie_setup(pci);
>> -
>> -	return 0;
>> +	return dw_pcie_ep_init_complete(ep);
>>   }
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 5accdd6bc388..340783e9032e 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
>>   #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_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,
>> @@ -416,6 +417,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)
>> +{
>> +	return 0;
>> +}
>> +
>>   static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>>   {
>>   }
>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>> index 36644ccd32ac..241e6a6f39fb 100644
>> --- a/include/linux/pci-epc.h
>> +++ b/include/linux/pci-epc.h
>> @@ -121,6 +121,7 @@ struct pci_epc_features {
>>   	u8	bar_fixed_64bit;
>>   	u64	bar_fixed_size[PCI_STD_NUM_BARS];
>>   	size_t	align;
>> +	bool	skip_core_init;
> 
> This looks more like a designware specific change. Why is it added to the core
> pci_epc_features?
Although the changes are done in DesignWare core (as Tegra194 uses DesignWare IP),
core not being available for programming before REFCLK from host is available, seemed
like a very generic case to me, so I added this as part of core features it self.

Thanks,
Vidya Sagar
> 
> Thanks
> Kishon
> 

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Kishon Vijay Abraham I]

Hi,

On 27/11/19 2:10 PM, Vidya Sagar wrote:
> On 11/27/2019 1:44 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>>> Add a new feature 'skip_core_init' that can be set by platform drivers
>>> of devices that do not have their core registers available until reference
>>> clock from host is available (Ex:- Tegra194) to indicate DesignWare
>>> endpoint mode sub-system to not perform core registers initialization.
>>> Existing dw_pcie_ep_init() is refactored and all the code that touches
>>> registers is extracted to form a new API dw_pcie_ep_init_complete() that
>>> can be called later by platform drivers setting 'skip_core_init' to '1'.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
>>>   drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
>>>   include/linux/pci-epc.h                       |  1 +
>>>   3 files changed, 51 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> index 3dd2e2697294..06f4379be8a3 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> @@ -492,19 +492,53 @@ static unsigned int
>>> dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>>>       return 0;
>>>   }
>>>   -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>> +int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>>   {
>>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +    unsigned int offset;
>>> +    unsigned int nbars;
>>> +    u8 hdr_type;
>>> +    u32 reg;
>>>       int i;
>>> +
>>> +    hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>>> +    if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>>> +        dev_err(pci->dev,
>>> +            "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
>>> +            hdr_type);
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>>> +
>>> +    ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>>> +
>>> +    offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>>> +    if (offset) {
>>> +        reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>>> +        nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>>> +            PCI_REBAR_CTRL_NBAR_SHIFT;
>>> +
>>> +        dw_pcie_dbi_ro_wr_en(pci);
>>> +        for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>>> +            dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>>> +        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    }
>>> +
>>> +    dw_pcie_setup(pci);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>> +{
>>>       int ret;
>>> -    u32 reg;
>>>       void *addr;
>>> -    u8 hdr_type;
>>> -    unsigned int nbars;
>>> -    unsigned int offset;
>>>       struct pci_epc *epc;
>>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>>       struct device *dev = pci->dev;
>>>       struct device_node *np = dev->of_node;
>>> +    const struct pci_epc_features *epc_features;
>>>         if (!pci->dbi_base || !pci->dbi_base2) {
>>>           dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
>>> @@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>       if (ep->ops->ep_init)
>>>           ep->ops->ep_init(ep);
>>>   -    hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>>> -    if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>>> -        dev_err(pci->dev, "PCIe controller is not set to EP mode
>>> (hdr_type:0x%x)!\n",
>>> -            hdr_type);
>>> -        return -EIO;
>>> -    }
>>> -
>>>       ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>>>       if (ret < 0)
>>>           epc->max_functions = 1;
>>> @@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>           dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>>>           return -ENOMEM;
>>>       }
>>> -    ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>>>   -    ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>>> -
>>> -    offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>>> -    if (offset) {
>>> -        reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>>> -        nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>>> -            PCI_REBAR_CTRL_NBAR_SHIFT;
>>> -
>>> -        dw_pcie_dbi_ro_wr_en(pci);
>>> -        for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>>> -            dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>>> -        dw_pcie_dbi_ro_wr_dis(pci);
>>> +    if (ep->ops->get_features) {
>>> +        epc_features = ep->ops->get_features(ep);
>>> +        if (epc_features->skip_core_init)
>>> +            return 0;
>>>       }
>>>   -    dw_pcie_setup(pci);
>>> -
>>> -    return 0;
>>> +    return dw_pcie_ep_init_complete(ep);
>>>   }
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
>>> b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 5accdd6bc388..340783e9032e 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct
>>> pcie_port *pp)
>>>   #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_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,
>>> @@ -416,6 +417,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)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>   static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>>>   {
>>>   }
>>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>>> index 36644ccd32ac..241e6a6f39fb 100644
>>> --- a/include/linux/pci-epc.h
>>> +++ b/include/linux/pci-epc.h
>>> @@ -121,6 +121,7 @@ struct pci_epc_features {
>>>       u8    bar_fixed_64bit;
>>>       u64    bar_fixed_size[PCI_STD_NUM_BARS];
>>>       size_t    align;
>>> +    bool    skip_core_init;
>>
>> This looks more like a designware specific change. Why is it added to the core
>> pci_epc_features?
> Although the changes are done in DesignWare core (as Tegra194 uses DesignWare IP),
> core not being available for programming before REFCLK from host is available,
> seemed
> like a very generic case to me, so I added this as part of core features it self.

right, I think you can name the epc_feature as core_init_notifier instead of
skip_core_init (similar to linkup_notifier?) and add that as a first patch.
Then you can use the epc_features in epf_test and designware in subsequent patches.

Thanks
Kishon

Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Kishon Vijay Abraham I]

Hi,

On 13/11/19 2:38 PM, Vidya Sagar wrote:
> Add support to defer core initialization and to receive a notifier
> when core is ready to accommodate platforms where core is not for
> initialization untile reference clock from host is available.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>  1 file changed, 77 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index bddff15052cc..068024fab544 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  			   msecs_to_jiffies(1));
>  }
>  
> -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
> -				 void *data)
> -{
> -	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
> -	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> -
> -	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
> -			   msecs_to_jiffies(1));
> -
> -	return NOTIFY_OK;
> -}
> -
>  static void pci_epf_test_unbind(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static int pci_epf_test_core_init(struct pci_epf *epf)
> +{
> +	struct pci_epf_header *header = epf->header;
> +	const struct pci_epc_features *epc_features;
> +	struct pci_epc *epc = epf->epc;
> +	struct device *dev = &epf->dev;
> +	bool msix_capable = false;
> +	bool msi_capable = true;
> +	int ret;
> +
> +	epc_features = pci_epc_get_features(epc, epf->func_no);
> +	if (epc_features) {
> +		msix_capable = epc_features->msix_capable;
> +		msi_capable = epc_features->msi_capable;
> +	}
> +
> +	ret = pci_epc_write_header(epc, epf->func_no, header);
> +	if (ret) {
> +		dev_err(dev, "Configuration header write failed\n");
> +		return ret;
> +	}
> +
> +	ret = pci_epf_test_set_bar(epf);
> +	if (ret)
> +		return ret;
> +
> +	if (msi_capable) {
> +		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> +		if (ret) {
> +			dev_err(dev, "MSI configuration failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (msix_capable) {
> +		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
> +		if (ret) {
> +			dev_err(dev, "MSI-X configuration failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
> +				 void *data)
> +{
> +	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	int ret;
> +
> +	switch (val) {
> +	case CORE_INIT:
> +		ret = pci_epf_test_core_init(epf);
> +		if (ret)
> +			return NOTIFY_BAD;
> +		break;
> +
> +	case LINK_UP:
> +		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
> +				   msecs_to_jiffies(1));
> +		break;
> +
> +	default:
> +		dev_err(&epf->dev, "Invalid EPF test notifier event\n");
> +		return NOTIFY_BAD;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  {
>  	int ret;
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> -	struct pci_epf_header *header = epf->header;
>  	const struct pci_epc_features *epc_features;
>  	enum pci_barno test_reg_bar = BAR_0;
>  	struct pci_epc *epc = epf->epc;
> -	struct device *dev = &epf->dev;
>  	bool linkup_notifier = false;
> +	bool skip_core_init = false;
>  	bool msix_capable = false;
>  	bool msi_capable = true;
>  
> @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	epc_features = pci_epc_get_features(epc, epf->func_no);
>  	if (epc_features) {
>  		linkup_notifier = epc_features->linkup_notifier;
> +		skip_core_init = epc_features->skip_core_init;
>  		msix_capable = epc_features->msix_capable;
>  		msi_capable = epc_features->msi_capable;

Are these used anywhere in this function?
>  		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	epf_test->test_reg_bar = test_reg_bar;
>  	epf_test->epc_features = epc_features;
>  
> -	ret = pci_epc_write_header(epc, epf->func_no, header);
> -	if (ret) {
> -		dev_err(dev, "Configuration header write failed\n");
> -		return ret;
> -	}
> -
>  	ret = pci_epf_test_alloc_space(epf);
>  	if (ret)
>  		return ret;
>  
> -	ret = pci_epf_test_set_bar(epf);
> -	if (ret)
> -		return ret;
> -
> -	if (msi_capable) {
> -		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> -		if (ret) {
> -			dev_err(dev, "MSI configuration failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	if (msix_capable) {
> -		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
> -		if (ret) {
> -			dev_err(dev, "MSI-X configuration failed\n");
> +	if (!skip_core_init) {
> +		ret = pci_epf_test_core_init(epf);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	if (linkup_notifier) {

This could as well be moved to pci_epf_test_core_init().

Thanks
Kishon

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Christoph Hellwig]

On Wed, Nov 13, 2019 at 02:38:48PM +0530, Vidya Sagar wrote:
> +	if (ep->ops->get_features) {
> +		epc_features = ep->ops->get_features(ep);
> +		if (epc_features->skip_core_init)
> +			return 0;
>  	}
>  
> +	return dw_pcie_ep_init_complete(ep);

This calling convention is strange.  Just split the early part of
dw_pcie_ep_init into an dw_pcie_ep_early and either add a tiny
wrapper like:

int dw_pcie_ep_init(struct dw_pcie_ep *ep)
{
	int error;

	error = dw_pcie_ep_init_early(ep);
	if (error)
		return error;
	return dw_pcie_ep_init_late(ep);
}

or just open code that in the few callers.  That keeps the calling
conventions much simpler and avoids relying on a callback and flag.

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Vidya Sagar]

On 11/27/2019 3:18 PM, Christoph Hellwig wrote:
> On Wed, Nov 13, 2019 at 02:38:48PM +0530, Vidya Sagar wrote:
>> +	if (ep->ops->get_features) {
>> +		epc_features = ep->ops->get_features(ep);
>> +		if (epc_features->skip_core_init)
>> +			return 0;
>>   	}
>>   
>> +	return dw_pcie_ep_init_complete(ep);
> 
> This calling convention is strange.  Just split the early part of
> dw_pcie_ep_init into an dw_pcie_ep_early and either add a tiny
> wrapper like:
> 
> int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> {
> 	int error;
> 
> 	error = dw_pcie_ep_init_early(ep);
> 	if (error)
> 		return error;
> 	return dw_pcie_ep_init_late(ep);
> }
> 
> or just open code that in the few callers.  That keeps the calling
> conventions much simpler and avoids relying on a callback and flag.
I'm not sure if I got this right. I think in any case, code that is going to be
part of dw_pcie_ep_init_late() needs to depend on callback and flag right?
I mean, unless it is confirmed (by calling the get_features() callback and
checking whether or not the core is available for programming) dw_pcie_ep_init_late()
can't be called right?
Please let me know if I'm missing something here.

- Vidya Sagar
> 

Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Vidya Sagar]

On 11/27/2019 2:50 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>> Add support to defer core initialization and to receive a notifier
>> when core is ready to accommodate platforms where core is not for
>> initialization untile reference clock from host is available.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>   1 file changed, 77 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index bddff15052cc..068024fab544 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>>   			   msecs_to_jiffies(1));
>>   }
>>   
>> -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
>> -				 void *data)
>> -{
>> -	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>> -	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>> -
>> -	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>> -			   msecs_to_jiffies(1));
>> -
>> -	return NOTIFY_OK;
>> -}
>> -
>>   static void pci_epf_test_unbind(struct pci_epf *epf)
>>   {
>>   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>> @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>>   	return 0;
>>   }
>>   
>> +static int pci_epf_test_core_init(struct pci_epf *epf)
>> +{
>> +	struct pci_epf_header *header = epf->header;
>> +	const struct pci_epc_features *epc_features;
>> +	struct pci_epc *epc = epf->epc;
>> +	struct device *dev = &epf->dev;
>> +	bool msix_capable = false;
>> +	bool msi_capable = true;
>> +	int ret;
>> +
>> +	epc_features = pci_epc_get_features(epc, epf->func_no);
>> +	if (epc_features) {
>> +		msix_capable = epc_features->msix_capable;
>> +		msi_capable = epc_features->msi_capable;
>> +	}
>> +
>> +	ret = pci_epc_write_header(epc, epf->func_no, header);
>> +	if (ret) {
>> +		dev_err(dev, "Configuration header write failed\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = pci_epf_test_set_bar(epf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (msi_capable) {
>> +		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>> +		if (ret) {
>> +			dev_err(dev, "MSI configuration failed\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (msix_capable) {
>> +		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
>> +		if (ret) {
>> +			dev_err(dev, "MSI-X configuration failed\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
>> +				 void *data)
>> +{
>> +	struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>> +	int ret;
>> +
>> +	switch (val) {
>> +	case CORE_INIT:
>> +		ret = pci_epf_test_core_init(epf);
>> +		if (ret)
>> +			return NOTIFY_BAD;
>> +		break;
>> +
>> +	case LINK_UP:
>> +		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>> +				   msecs_to_jiffies(1));
>> +		break;
>> +
>> +	default:
>> +		dev_err(&epf->dev, "Invalid EPF test notifier event\n");
>> +		return NOTIFY_BAD;
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>>   static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>   {
>>   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>> @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>   {
>>   	int ret;
>>   	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>> -	struct pci_epf_header *header = epf->header;
>>   	const struct pci_epc_features *epc_features;
>>   	enum pci_barno test_reg_bar = BAR_0;
>>   	struct pci_epc *epc = epf->epc;
>> -	struct device *dev = &epf->dev;
>>   	bool linkup_notifier = false;
>> +	bool skip_core_init = false;
>>   	bool msix_capable = false;
>>   	bool msi_capable = true;
>>   
>> @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>   	epc_features = pci_epc_get_features(epc, epf->func_no);
>>   	if (epc_features) {
>>   		linkup_notifier = epc_features->linkup_notifier;
>> +		skip_core_init = epc_features->skip_core_init;
>>   		msix_capable = epc_features->msix_capable;
>>   		msi_capable = epc_features->msi_capable;
> 
> Are these used anywhere in this function?
Nope. I'll remove them.

>>   		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
>> @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>   	epf_test->test_reg_bar = test_reg_bar;
>>   	epf_test->epc_features = epc_features;
>>   
>> -	ret = pci_epc_write_header(epc, epf->func_no, header);
>> -	if (ret) {
>> -		dev_err(dev, "Configuration header write failed\n");
>> -		return ret;
>> -	}
>> -
>>   	ret = pci_epf_test_alloc_space(epf);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = pci_epf_test_set_bar(epf);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (msi_capable) {
>> -		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>> -		if (ret) {
>> -			dev_err(dev, "MSI configuration failed\n");
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	if (msix_capable) {
>> -		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
>> -		if (ret) {
>> -			dev_err(dev, "MSI-X configuration failed\n");
>> +	if (!skip_core_init) {
>> +		ret = pci_epf_test_core_init(epf);
>> +		if (ret)
>>   			return ret;
>> -		}
>>   	}
>>   
>>   	if (linkup_notifier) {
> 
> This could as well be moved to pci_epf_test_core_init().
Yes, but I would like to keep only the code that touches hardware in pci_epf_test_core_init()
to minimize the time it takes to execute it. Is there any strong reason to move it? if not,
I would prefer to leave it here in this function itself.

> 
> Thanks
> Kishon
> 

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Vidya Sagar]

On 11/29/2019 8:10 PM, Vidya Sagar wrote:

Hi Christoph,
Could you please let me know what am I missing here?

Thanks,
Vidya Sagar

> On 11/27/2019 3:18 PM, Christoph Hellwig wrote:
>> On Wed, Nov 13, 2019 at 02:38:48PM +0530, Vidya Sagar wrote:
>>> +    if (ep->ops->get_features) {
>>> +        epc_features = ep->ops->get_features(ep);
>>> +        if (epc_features->skip_core_init)
>>> +            return 0;
>>>       }
>>> +    return dw_pcie_ep_init_complete(ep);
>>
>> This calling convention is strange.  Just split the early part of
>> dw_pcie_ep_init into an dw_pcie_ep_early and either add a tiny
>> wrapper like:
>>
>> int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> {
>>     int error;
>>
>>     error = dw_pcie_ep_init_early(ep);
>>     if (error)
>>         return error;
>>     return dw_pcie_ep_init_late(ep);
>> }
>>
>> or just open code that in the few callers.  That keeps the calling
>> conventions much simpler and avoids relying on a callback and flag.
> I'm not sure if I got this right. I think in any case, code that is going to be
> part of dw_pcie_ep_init_late() needs to depend on callback and flag right?
> I mean, unless it is confirmed (by calling the get_features() callback and
> checking whether or not the core is available for programming) dw_pcie_ep_init_late()
> can't be called right?
> Please let me know if I'm missing something here.
> 
> - Vidya Sagar
>>
> 

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Kishon Vijay Abraham I]

On 13/11/19 2:38 pm, Vidya Sagar wrote:
> Add a new feature 'skip_core_init' that can be set by platform drivers
> of devices that do not have their core registers available until reference
> clock from host is available (Ex:- Tegra194) to indicate DesignWare
> endpoint mode sub-system to not perform core registers initialization.
> Existing dw_pcie_ep_init() is refactored and all the code that touches
> registers is extracted to form a new API dw_pcie_ep_init_complete() that
> can be called later by platform drivers setting 'skip_core_init' to '1'.

No. pci_epc_features should only use constant values. This is used by 
function drivers to know the controller capabilities.

Thanks
Kishon

> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>   .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
>   drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
>   include/linux/pci-epc.h                       |  1 +
>   3 files changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 3dd2e2697294..06f4379be8a3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -492,19 +492,53 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>   	return 0;
>   }
>   
> -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>   {
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	unsigned int offset;
> +	unsigned int nbars;
> +	u8 hdr_type;
> +	u32 reg;
>   	int i;
> +
> +	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> +	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> +		dev_err(pci->dev,
> +			"PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
> +			hdr_type);
> +		return -EIO;
> +	}
> +
> +	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +
> +	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
> +
> +	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> +	if (offset) {
> +		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> +		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> +			PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +		dw_pcie_dbi_ro_wr_en(pci);
> +		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> +			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> +		dw_pcie_dbi_ro_wr_dis(pci);
> +	}
> +
> +	dw_pcie_setup(pci);
> +
> +	return 0;
> +}
> +
> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
>   	int ret;
> -	u32 reg;
>   	void *addr;
> -	u8 hdr_type;
> -	unsigned int nbars;
> -	unsigned int offset;
>   	struct pci_epc *epc;
>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>   	struct device *dev = pci->dev;
>   	struct device_node *np = dev->of_node;
> +	const struct pci_epc_features *epc_features;
>   
>   	if (!pci->dbi_base || !pci->dbi_base2) {
>   		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
> @@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>   	if (ep->ops->ep_init)
>   		ep->ops->ep_init(ep);
>   
> -	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> -	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> -		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
> -			hdr_type);
> -		return -EIO;
> -	}
> -
>   	ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>   	if (ret < 0)
>   		epc->max_functions = 1;
> @@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>   		dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>   		return -ENOMEM;
>   	}
> -	ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>   
> -	ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
> -
> -	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
> -	if (offset) {
> -		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
> -		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
> -			PCI_REBAR_CTRL_NBAR_SHIFT;
> -
> -		dw_pcie_dbi_ro_wr_en(pci);
> -		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
> -			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
> -		dw_pcie_dbi_ro_wr_dis(pci);
> +	if (ep->ops->get_features) {
> +		epc_features = ep->ops->get_features(ep);
> +		if (epc_features->skip_core_init)
> +			return 0;
>   	}
>   
> -	dw_pcie_setup(pci);
> -
> -	return 0;
> +	return dw_pcie_ep_init_complete(ep);
>   }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 5accdd6bc388..340783e9032e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
>   #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_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,
> @@ -416,6 +417,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)
> +{
> +	return 0;
> +}
> +
>   static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>   {
>   }
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index 36644ccd32ac..241e6a6f39fb 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -121,6 +121,7 @@ struct pci_epc_features {
>   	u8	bar_fixed_64bit;
>   	u64	bar_fixed_size[PCI_STD_NUM_BARS];
>   	size_t	align;
> +	bool	skip_core_init;
>   };
>   
>   #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
> 

Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Kishon Vijay Abraham I]

Hi,

On 01/12/19 7:59 pm, Vidya Sagar wrote:
> On 11/27/2019 2:50 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>>> Add support to defer core initialization and to receive a notifier
>>> when core is ready to accommodate platforms where core is not for
>>> initialization untile reference clock from host is available.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>>   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>>   1 file changed, 77 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c 
>>> b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> index bddff15052cc..068024fab544 100644
>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>> @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct 
>>> work_struct *work)
>>>                  msecs_to_jiffies(1));
>>>   }
>>> -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned 
>>> long val,
>>> -                 void *data)
>>> -{
>>> -    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>> -    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> -
>>> -    queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>> -               msecs_to_jiffies(1));
>>> -
>>> -    return NOTIFY_OK;
>>> -}
>>> -
>>>   static void pci_epf_test_unbind(struct pci_epf *epf)
>>>   {
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf 
>>> *epf)
>>>       return 0;
>>>   }
>>> +static int pci_epf_test_core_init(struct pci_epf *epf)
>>> +{
>>> +    struct pci_epf_header *header = epf->header;
>>> +    const struct pci_epc_features *epc_features;
>>> +    struct pci_epc *epc = epf->epc;
>>> +    struct device *dev = &epf->dev;
>>> +    bool msix_capable = false;
>>> +    bool msi_capable = true;
>>> +    int ret;
>>> +
>>> +    epc_features = pci_epc_get_features(epc, epf->func_no);
>>> +    if (epc_features) {
>>> +        msix_capable = epc_features->msix_capable;
>>> +        msi_capable = epc_features->msi_capable;
>>> +    }
>>> +
>>> +    ret = pci_epc_write_header(epc, epf->func_no, header);
>>> +    if (ret) {
>>> +        dev_err(dev, "Configuration header write failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = pci_epf_test_set_bar(epf);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (msi_capable) {
>>> +        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>> +        if (ret) {
>>> +            dev_err(dev, "MSI configuration failed\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if (msix_capable) {
>>> +        ret = pci_epc_set_msix(epc, epf->func_no, 
>>> epf->msix_interrupts);
>>> +        if (ret) {
>>> +            dev_err(dev, "MSI-X configuration failed\n");
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned 
>>> long val,
>>> +                 void *data)
>>> +{
>>> +    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>> +    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> +    int ret;
>>> +
>>> +    switch (val) {
>>> +    case CORE_INIT:
>>> +        ret = pci_epf_test_core_init(epf);
>>> +        if (ret)
>>> +            return NOTIFY_BAD;
>>> +        break;
>>> +
>>> +    case LINK_UP:
>>> +        queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>> +                   msecs_to_jiffies(1));
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_err(&epf->dev, "Invalid EPF test notifier event\n");
>>> +        return NOTIFY_BAD;
>>> +    }
>>> +
>>> +    return NOTIFY_OK;
>>> +}
>>> +
>>>   static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>>   {
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>   {
>>>       int ret;
>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>> -    struct pci_epf_header *header = epf->header;
>>>       const struct pci_epc_features *epc_features;
>>>       enum pci_barno test_reg_bar = BAR_0;
>>>       struct pci_epc *epc = epf->epc;
>>> -    struct device *dev = &epf->dev;
>>>       bool linkup_notifier = false;
>>> +    bool skip_core_init = false;
>>>       bool msix_capable = false;
>>>       bool msi_capable = true;
>>> @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>       epc_features = pci_epc_get_features(epc, epf->func_no);
>>>       if (epc_features) {
>>>           linkup_notifier = epc_features->linkup_notifier;
>>> +        skip_core_init = epc_features->skip_core_init;
>>>           msix_capable = epc_features->msix_capable;
>>>           msi_capable = epc_features->msi_capable;
>>
>> Are these used anywhere in this function?
> Nope. I'll remove them.
> 
>>>           test_reg_bar = pci_epc_get_first_free_bar(epc_features);
>>> @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>       epf_test->test_reg_bar = test_reg_bar;
>>>       epf_test->epc_features = epc_features;
>>> -    ret = pci_epc_write_header(epc, epf->func_no, header);
>>> -    if (ret) {
>>> -        dev_err(dev, "Configuration header write failed\n");
>>> -        return ret;
>>> -    }
>>> -
>>>       ret = pci_epf_test_alloc_space(epf);
>>>       if (ret)
>>>           return ret;
>>> -    ret = pci_epf_test_set_bar(epf);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>> -    if (msi_capable) {
>>> -        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>> -        if (ret) {
>>> -            dev_err(dev, "MSI configuration failed\n");
>>> -            return ret;
>>> -        }
>>> -    }
>>> -
>>> -    if (msix_capable) {
>>> -        ret = pci_epc_set_msix(epc, epf->func_no, 
>>> epf->msix_interrupts);
>>> -        if (ret) {
>>> -            dev_err(dev, "MSI-X configuration failed\n");
>>> +    if (!skip_core_init) {
>>> +        ret = pci_epf_test_core_init(epf);
>>> +        if (ret)
>>>               return ret;
>>> -        }
>>>       }
>>>       if (linkup_notifier) {
>>
>> This could as well be moved to pci_epf_test_core_init().
> Yes, but I would like to keep only the code that touches hardware in 
> pci_epf_test_core_init()
> to minimize the time it takes to execute it. Is there any strong reason 
> to move it? if not,
> I would prefer to leave it here in this function itself.

There is no point in scheduling a work to check for commands from host 
when the EP itself is not initialized.

Thanks
Kishon

Re: [PATCH 1/4] PCI: dwc: Add new feature to skip core initialization

[Vidya Sagar]

On 12/5/2019 3:34 PM, Kishon Vijay Abraham I wrote:
> 
> 
> On 13/11/19 2:38 pm, Vidya Sagar wrote:
>> Add a new feature 'skip_core_init' that can be set by platform drivers
>> of devices that do not have their core registers available until reference
>> clock from host is available (Ex:- Tegra194) to indicate DesignWare
>> endpoint mode sub-system to not perform core registers initialization.
>> Existing dw_pcie_ep_init() is refactored and all the code that touches
>> registers is extracted to form a new API dw_pcie_ep_init_complete() that
>> can be called later by platform drivers setting 'skip_core_init' to '1'.
> 
> No. pci_epc_features should only use constant values. This is used by function drivers to know the controller capabilities.
Yes. I'm going to set EPC features as constant values in pcie-tegra194.c driver.
I'm going to rewrite this commit message in the next patch.
  
> 
> Thanks
> Kishon
> 
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   .../pci/controller/dwc/pcie-designware-ep.c   | 72 +++++++++++--------
>>   drivers/pci/controller/dwc/pcie-designware.h  |  6 ++
>>   include/linux/pci-epc.h                       |  1 +
>>   3 files changed, 51 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> index 3dd2e2697294..06f4379be8a3 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>> @@ -492,19 +492,53 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>>       return 0;
>>   }
>> -int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> +int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>   {
>> +    struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +    unsigned int offset;
>> +    unsigned int nbars;
>> +    u8 hdr_type;
>> +    u32 reg;
>>       int i;
>> +
>> +    hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>> +    if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>> +        dev_err(pci->dev,
>> +            "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
>> +            hdr_type);
>> +        return -EIO;
>> +    }
>> +
>> +    ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>> +
>> +    ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>> +
>> +    offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>> +    if (offset) {
>> +        reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>> +        nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>> +            PCI_REBAR_CTRL_NBAR_SHIFT;
>> +
>> +        dw_pcie_dbi_ro_wr_en(pci);
>> +        for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>> +            dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>> +        dw_pcie_dbi_ro_wr_dis(pci);
>> +    }
>> +
>> +    dw_pcie_setup(pci);
>> +
>> +    return 0;
>> +}
>> +
>> +int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>>       int ret;
>> -    u32 reg;
>>       void *addr;
>> -    u8 hdr_type;
>> -    unsigned int nbars;
>> -    unsigned int offset;
>>       struct pci_epc *epc;
>>       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>       struct device *dev = pci->dev;
>>       struct device_node *np = dev->of_node;
>> +    const struct pci_epc_features *epc_features;
>>       if (!pci->dbi_base || !pci->dbi_base2) {
>>           dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
>> @@ -563,13 +597,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>       if (ep->ops->ep_init)
>>           ep->ops->ep_init(ep);
>> -    hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
>> -    if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
>> -        dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
>> -            hdr_type);
>> -        return -EIO;
>> -    }
>> -
>>       ret = of_property_read_u8(np, "max-functions", &epc->max_functions);
>>       if (ret < 0)
>>           epc->max_functions = 1;
>> @@ -587,23 +614,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>           dev_err(dev, "Failed to reserve memory for MSI/MSI-X\n");
>>           return -ENOMEM;
>>       }
>> -    ep->msi_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
>> -    ep->msix_cap = dw_pcie_find_capability(pci, PCI_CAP_ID_MSIX);
>> -
>> -    offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>> -    if (offset) {
>> -        reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
>> -        nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
>> -            PCI_REBAR_CTRL_NBAR_SHIFT;
>> -
>> -        dw_pcie_dbi_ro_wr_en(pci);
>> -        for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
>> -            dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
>> -        dw_pcie_dbi_ro_wr_dis(pci);
>> +    if (ep->ops->get_features) {
>> +        epc_features = ep->ops->get_features(ep);
>> +        if (epc_features->skip_core_init)
>> +            return 0;
>>       }
>> -    dw_pcie_setup(pci);
>> -
>> -    return 0;
>> +    return dw_pcie_ep_init_complete(ep);
>>   }
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 5accdd6bc388..340783e9032e 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -399,6 +399,7 @@ static inline int dw_pcie_allocate_domains(struct pcie_port *pp)
>>   #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_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,
>> @@ -416,6 +417,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)
>> +{
>> +    return 0;
>> +}
>> +
>>   static inline void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
>>   {
>>   }
>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>> index 36644ccd32ac..241e6a6f39fb 100644
>> --- a/include/linux/pci-epc.h
>> +++ b/include/linux/pci-epc.h
>> @@ -121,6 +121,7 @@ struct pci_epc_features {
>>       u8    bar_fixed_64bit;
>>       u64    bar_fixed_size[PCI_STD_NUM_BARS];
>>       size_t    align;
>> +    bool    skip_core_init;
>>   };
>>   #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
>>

Re: [PATCH 4/4] PCI: pci-epf-test: Add support to defer core initialization

[Vidya Sagar]

On 12/5/2019 4:52 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On 01/12/19 7:59 pm, Vidya Sagar wrote:
>> On 11/27/2019 2:50 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On 13/11/19 2:38 PM, Vidya Sagar wrote:
>>>> Add support to defer core initialization and to receive a notifier
>>>> when core is ready to accommodate platforms where core is not for
>>>> initialization untile reference clock from host is available.
>>>>
>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>> ---
>>>>   drivers/pci/endpoint/functions/pci-epf-test.c | 114 ++++++++++++------
>>>>   1 file changed, 77 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> index bddff15052cc..068024fab544 100644
>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> @@ -360,18 +360,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>>>>                  msecs_to_jiffies(1));
>>>>   }
>>>> -static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
>>>> -                 void *data)
>>>> -{
>>>> -    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>>> -    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>> -
>>>> -    queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>>> -               msecs_to_jiffies(1));
>>>> -
>>>> -    return NOTIFY_OK;
>>>> -}
>>>> -
>>>>   static void pci_epf_test_unbind(struct pci_epf *epf)
>>>>   {
>>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>> @@ -428,6 +416,78 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
>>>>       return 0;
>>>>   }
>>>> +static int pci_epf_test_core_init(struct pci_epf *epf)
>>>> +{
>>>> +    struct pci_epf_header *header = epf->header;
>>>> +    const struct pci_epc_features *epc_features;
>>>> +    struct pci_epc *epc = epf->epc;
>>>> +    struct device *dev = &epf->dev;
>>>> +    bool msix_capable = false;
>>>> +    bool msi_capable = true;
>>>> +    int ret;
>>>> +
>>>> +    epc_features = pci_epc_get_features(epc, epf->func_no);
>>>> +    if (epc_features) {
>>>> +        msix_capable = epc_features->msix_capable;
>>>> +        msi_capable = epc_features->msi_capable;
>>>> +    }
>>>> +
>>>> +    ret = pci_epc_write_header(epc, epf->func_no, header);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Configuration header write failed\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = pci_epf_test_set_bar(epf);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    if (msi_capable) {
>>>> +        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "MSI configuration failed\n");
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (msix_capable) {
>>>> +        ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "MSI-X configuration failed\n");
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
>>>> +                 void *data)
>>>> +{
>>>> +    struct pci_epf *epf = container_of(nb, struct pci_epf, nb);
>>>> +    struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>> +    int ret;
>>>> +
>>>> +    switch (val) {
>>>> +    case CORE_INIT:
>>>> +        ret = pci_epf_test_core_init(epf);
>>>> +        if (ret)
>>>> +            return NOTIFY_BAD;
>>>> +        break;
>>>> +
>>>> +    case LINK_UP:
>>>> +        queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>>>> +                   msecs_to_jiffies(1));
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +        dev_err(&epf->dev, "Invalid EPF test notifier event\n");
>>>> +        return NOTIFY_BAD;
>>>> +    }
>>>> +
>>>> +    return NOTIFY_OK;
>>>> +}
>>>> +
>>>>   static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>>>   {
>>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>> @@ -496,12 +556,11 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>   {
>>>>       int ret;
>>>>       struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>> -    struct pci_epf_header *header = epf->header;
>>>>       const struct pci_epc_features *epc_features;
>>>>       enum pci_barno test_reg_bar = BAR_0;
>>>>       struct pci_epc *epc = epf->epc;
>>>> -    struct device *dev = &epf->dev;
>>>>       bool linkup_notifier = false;
>>>> +    bool skip_core_init = false;
>>>>       bool msix_capable = false;
>>>>       bool msi_capable = true;
>>>> @@ -511,6 +570,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>       epc_features = pci_epc_get_features(epc, epf->func_no);
>>>>       if (epc_features) {
>>>>           linkup_notifier = epc_features->linkup_notifier;
>>>> +        skip_core_init = epc_features->skip_core_init;
>>>>           msix_capable = epc_features->msix_capable;
>>>>           msi_capable = epc_features->msi_capable;
>>>
>>> Are these used anywhere in this function?
>> Nope. I'll remove them.
>>
>>>>           test_reg_bar = pci_epc_get_first_free_bar(epc_features);
>>>> @@ -520,34 +580,14 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>       epf_test->test_reg_bar = test_reg_bar;
>>>>       epf_test->epc_features = epc_features;
>>>> -    ret = pci_epc_write_header(epc, epf->func_no, header);
>>>> -    if (ret) {
>>>> -        dev_err(dev, "Configuration header write failed\n");
>>>> -        return ret;
>>>> -    }
>>>> -
>>>>       ret = pci_epf_test_alloc_space(epf);
>>>>       if (ret)
>>>>           return ret;
>>>> -    ret = pci_epf_test_set_bar(epf);
>>>> -    if (ret)
>>>> -        return ret;
>>>> -
>>>> -    if (msi_capable) {
>>>> -        ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
>>>> -        if (ret) {
>>>> -            dev_err(dev, "MSI configuration failed\n");
>>>> -            return ret;
>>>> -        }
>>>> -    }
>>>> -
>>>> -    if (msix_capable) {
>>>> -        ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
>>>> -        if (ret) {
>>>> -            dev_err(dev, "MSI-X configuration failed\n");
>>>> +    if (!skip_core_init) {
>>>> +        ret = pci_epf_test_core_init(epf);
>>>> +        if (ret)
>>>>               return ret;
>>>> -        }
>>>>       }
>>>>       if (linkup_notifier) {
>>>
>>> This could as well be moved to pci_epf_test_core_init().
>> Yes, but I would like to keep only the code that touches hardware in pci_epf_test_core_init()
>> to minimize the time it takes to execute it. Is there any strong reason to move it? if not,
>> I would prefer to leave it here in this function itself.
> 
> There is no point in scheduling a work to check for commands from host when the EP itself is not initialized.
True. But, since this is more of preparatory work, I thought we should just have it done here itself.
Main reason being, once PERST is perceived, endpoint can't take too much initializing its core. So, I want to
keep that part as minimalistic as possible.

- Vidya Sagar

> 
> Thanks
> Kishon