[PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

For controllers supporting the CORE_INIT notifier, the resources are
supposed to be enabled in the init_complete function. Currently,
these controllers are enabling the resources during probe time due to
the DBI access happens in dw_pcie_ep_init().

This creates the dependency with the host PCIe controller since the
resource enablement like PHY depends on host PCIe to be up. For the
standalone endpoint usecase, this would never work. So let's move all DBI
access to init_complete function if CORE_INIT notifier is used. For the
controllers those doesn't support this notifier, this change is a NO-OP.

Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Cc: Om Prakash Singh <omp@nvidia.com>
Cc: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
 drivers/pci/controller/dwc/pcie-designware.h  |   1 +
 2 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0eda8236c125..fb2bf4bf5ba0 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
 	return 0;
 }
 
+static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct device *dev = pci->dev;
+	void *addr;
+
+	dw_pcie_iatu_detect(pci);
+
+	ep->ib_window_map = devm_kcalloc(dev,
+					 BITS_TO_LONGS(pci->num_ib_windows),
+					 sizeof(long),
+					 GFP_KERNEL);
+	if (!ep->ib_window_map)
+		return -ENOMEM;
+
+	ep->ob_window_map = devm_kcalloc(dev,
+					 BITS_TO_LONGS(pci->num_ob_windows),
+					 sizeof(long),
+					 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;
+
+	return 0;
+}
+
+static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct dw_pcie_ep_func *ep_func;
+	struct pci_epc *epc = ep->epc;
+	struct device *dev = pci->dev;
+	u8 func_no;
+
+	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);
+	}
+
+	return 0;
+}
+
 int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
@@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
 	unsigned int nbars;
 	u8 hdr_type;
 	u32 reg;
-	int i;
+	int ret, i;
+
+	if (ep->core_init_notifier) {
+		ret = dw_pcie_iatu_config(ep);
+		if (ret)
+			return ret;
+	}
+
+	if (ep->core_init_notifier) {
+		ret = dw_pcie_ep_func_init(ep);
+		if (ret)
+			return ret;
+
+		if (ep->ops->ep_init)
+			ep->ops->ep_init(ep);
+	}
 
 	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
 		   PCI_HEADER_TYPE_MASK;
@@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
 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);
@@ -686,7 +756,12 @@ 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;
+
+	if (ep->ops->get_features) {
+		epc_features = ep->ops->get_features(ep);
+		if (epc_features->core_init_notifier)
+			ep->core_init_notifier = true;
+	}
 
 	INIT_LIST_HEAD(&ep->func_list);
 
@@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		}
 	}
 
-	dw_pcie_iatu_detect(pci);
+	if (!ep->core_init_notifier) {
+		ret = dw_pcie_iatu_config(ep);
+		if (ret)
+			return ret;
+	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
 	if (!res)
@@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	ep->phys_base = res->start;
 	ep->addr_size = resource_size(res);
 
-	ep->ib_window_map = devm_kcalloc(dev,
-					 BITS_TO_LONGS(pci->num_ib_windows),
-					 sizeof(long),
-					 GFP_KERNEL);
-	if (!ep->ib_window_map)
-		return -ENOMEM;
-
-	ep->ob_window_map = devm_kcalloc(dev,
-					 BITS_TO_LONGS(pci->num_ob_windows),
-					 sizeof(long),
-					 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);
 
@@ -753,23 +812,15 @@ 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);
+	if (!ep->core_init_notifier) {
+		ret = dw_pcie_ep_func_init(ep);
+		if (ret)
+			return ret;
 
-		list_add_tail(&ep_func->list, &ep->func_list);
+		if (ep->ops->ep_init)
+			ep->ops->ep_init(ep);
 	}
 
-	if (ep->ops->ep_init)
-		ep->ops->ep_init(ep);
-
 	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
 			       ep->page_size);
 	if (ret < 0) {
@@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -ENOMEM;
 	}
 
-	if (ep->ops->get_features) {
-		epc_features = ep->ops->get_features(ep);
-		if (epc_features->core_init_notifier)
-			return 0;
-	}
+	if (!ep->core_init_notifier)
+		return dw_pcie_ep_init_complete(ep);
 
-	return dw_pcie_ep_init_complete(ep);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 7d6e9b7576be..aadb14159df7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -242,6 +242,7 @@ struct dw_pcie_ep {
 	void __iomem		*msi_mem;
 	phys_addr_t		msi_mem_phys;
 	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
+	bool			core_init_notifier;
 };
 
 struct dw_pcie_ops {
-- 
2.25.1

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> For controllers supporting the CORE_INIT notifier, the resources are
> supposed to be enabled in the init_complete function. Currently,
> these controllers are enabling the resources during probe time due to
> the DBI access happens in dw_pcie_ep_init().
> 
> This creates the dependency with the host PCIe controller since the
> resource enablement like PHY depends on host PCIe to be up. For the
> standalone endpoint usecase, this would never work. So let's move all DBI
> access to init_complete function if CORE_INIT notifier is used. For the
> controllers those doesn't support this notifier, this change is a NO-OP.
> 
> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Cc: Om Prakash Singh <omp@nvidia.com>
> Cc: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

A gentle ping on this patch!

Thanks,
Mani

> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
>  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>  2 files changed, 94 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 0eda8236c125..fb2bf4bf5ba0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>  	return 0;
>  }
>  
> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct device *dev = pci->dev;
> +	void *addr;
> +
> +	dw_pcie_iatu_detect(pci);
> +
> +	ep->ib_window_map = devm_kcalloc(dev,
> +					 BITS_TO_LONGS(pci->num_ib_windows),
> +					 sizeof(long),
> +					 GFP_KERNEL);
> +	if (!ep->ib_window_map)
> +		return -ENOMEM;
> +
> +	ep->ob_window_map = devm_kcalloc(dev,
> +					 BITS_TO_LONGS(pci->num_ob_windows),
> +					 sizeof(long),
> +					 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;
> +
> +	return 0;
> +}
> +
> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct dw_pcie_ep_func *ep_func;
> +	struct pci_epc *epc = ep->epc;
> +	struct device *dev = pci->dev;
> +	u8 func_no;
> +
> +	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);
> +	}
> +
> +	return 0;
> +}
> +
>  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  	unsigned int nbars;
>  	u8 hdr_type;
>  	u32 reg;
> -	int i;
> +	int ret, i;
> +
> +	if (ep->core_init_notifier) {
> +		ret = dw_pcie_iatu_config(ep);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (ep->core_init_notifier) {
> +		ret = dw_pcie_ep_func_init(ep);
> +		if (ret)
> +			return ret;
> +
> +		if (ep->ops->ep_init)
> +			ep->ops->ep_init(ep);
> +	}
>  
>  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>  		   PCI_HEADER_TYPE_MASK;
> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
>  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);
> @@ -686,7 +756,12 @@ 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;
> +
> +	if (ep->ops->get_features) {
> +		epc_features = ep->ops->get_features(ep);
> +		if (epc_features->core_init_notifier)
> +			ep->core_init_notifier = true;
> +	}
>  
>  	INIT_LIST_HEAD(&ep->func_list);
>  
> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		}
>  	}
>  
> -	dw_pcie_iatu_detect(pci);
> +	if (!ep->core_init_notifier) {
> +		ret = dw_pcie_iatu_config(ep);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>  	if (!res)
> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	ep->phys_base = res->start;
>  	ep->addr_size = resource_size(res);
>  
> -	ep->ib_window_map = devm_kcalloc(dev,
> -					 BITS_TO_LONGS(pci->num_ib_windows),
> -					 sizeof(long),
> -					 GFP_KERNEL);
> -	if (!ep->ib_window_map)
> -		return -ENOMEM;
> -
> -	ep->ob_window_map = devm_kcalloc(dev,
> -					 BITS_TO_LONGS(pci->num_ob_windows),
> -					 sizeof(long),
> -					 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);
>  
> @@ -753,23 +812,15 @@ 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);
> +	if (!ep->core_init_notifier) {
> +		ret = dw_pcie_ep_func_init(ep);
> +		if (ret)
> +			return ret;
>  
> -		list_add_tail(&ep_func->list, &ep->func_list);
> +		if (ep->ops->ep_init)
> +			ep->ops->ep_init(ep);
>  	}
>  
> -	if (ep->ops->ep_init)
> -		ep->ops->ep_init(ep);
> -
>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
>  	if (ret < 0) {
> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		return -ENOMEM;
>  	}
>  
> -	if (ep->ops->get_features) {
> -		epc_features = ep->ops->get_features(ep);
> -		if (epc_features->core_init_notifier)
> -			return 0;
> -	}
> +	if (!ep->core_init_notifier)
> +		return dw_pcie_ep_init_complete(ep);
>  
> -	return dw_pcie_ep_init_complete(ep);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 7d6e9b7576be..aadb14159df7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -242,6 +242,7 @@ struct dw_pcie_ep {
>  	void __iomem		*msi_mem;
>  	phys_addr_t		msi_mem_phys;
>  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> +	bool			core_init_notifier;
>  };
>  
>  struct dw_pcie_ops {
> -- 
> 2.25.1
> 

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > For controllers supporting the CORE_INIT notifier, the resources are
> > supposed to be enabled in the init_complete function. Currently,
> > these controllers are enabling the resources during probe time due to
> > the DBI access happens in dw_pcie_ep_init().
> > 
> > This creates the dependency with the host PCIe controller since the
> > resource enablement like PHY depends on host PCIe to be up. For the
> > standalone endpoint usecase, this would never work. So let's move all DBI
> > access to init_complete function if CORE_INIT notifier is used. For the
> > controllers those doesn't support this notifier, this change is a NO-OP.
> > 
> > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > Cc: Om Prakash Singh <omp@nvidia.com>
> > Cc: Vidya Sagar <vidyas@nvidia.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> A gentle ping on this patch!
> 

Ping again!

Thanks,
Mani

> Thanks,
> Mani
> 
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> >  2 files changed, 94 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 0eda8236c125..fb2bf4bf5ba0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> >  	return 0;
> >  }
> >  
> > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct device *dev = pci->dev;
> > +	void *addr;
> > +
> > +	dw_pcie_iatu_detect(pci);
> > +
> > +	ep->ib_window_map = devm_kcalloc(dev,
> > +					 BITS_TO_LONGS(pci->num_ib_windows),
> > +					 sizeof(long),
> > +					 GFP_KERNEL);
> > +	if (!ep->ib_window_map)
> > +		return -ENOMEM;
> > +
> > +	ep->ob_window_map = devm_kcalloc(dev,
> > +					 BITS_TO_LONGS(pci->num_ob_windows),
> > +					 sizeof(long),
> > +					 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;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct dw_pcie_ep_func *ep_func;
> > +	struct pci_epc *epc = ep->epc;
> > +	struct device *dev = pci->dev;
> > +	u8 func_no;
> > +
> > +	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);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> >  	unsigned int nbars;
> >  	u8 hdr_type;
> >  	u32 reg;
> > -	int i;
> > +	int ret, i;
> > +
> > +	if (ep->core_init_notifier) {
> > +		ret = dw_pcie_iatu_config(ep);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (ep->core_init_notifier) {
> > +		ret = dw_pcie_ep_func_init(ep);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (ep->ops->ep_init)
> > +			ep->ops->ep_init(ep);
> > +	}
> >  
> >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> >  		   PCI_HEADER_TYPE_MASK;
> > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> >  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);
> > @@ -686,7 +756,12 @@ 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;
> > +
> > +	if (ep->ops->get_features) {
> > +		epc_features = ep->ops->get_features(ep);
> > +		if (epc_features->core_init_notifier)
> > +			ep->core_init_notifier = true;
> > +	}
> >  
> >  	INIT_LIST_HEAD(&ep->func_list);
> >  
> > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		}
> >  	}
> >  
> > -	dw_pcie_iatu_detect(pci);
> > +	if (!ep->core_init_notifier) {
> > +		ret = dw_pcie_iatu_config(ep);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> >  	if (!res)
> > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	ep->phys_base = res->start;
> >  	ep->addr_size = resource_size(res);
> >  
> > -	ep->ib_window_map = devm_kcalloc(dev,
> > -					 BITS_TO_LONGS(pci->num_ib_windows),
> > -					 sizeof(long),
> > -					 GFP_KERNEL);
> > -	if (!ep->ib_window_map)
> > -		return -ENOMEM;
> > -
> > -	ep->ob_window_map = devm_kcalloc(dev,
> > -					 BITS_TO_LONGS(pci->num_ob_windows),
> > -					 sizeof(long),
> > -					 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);
> >  
> > @@ -753,23 +812,15 @@ 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);
> > +	if (!ep->core_init_notifier) {
> > +		ret = dw_pcie_ep_func_init(ep);
> > +		if (ret)
> > +			return ret;
> >  
> > -		list_add_tail(&ep_func->list, &ep->func_list);
> > +		if (ep->ops->ep_init)
> > +			ep->ops->ep_init(ep);
> >  	}
> >  
> > -	if (ep->ops->ep_init)
> > -		ep->ops->ep_init(ep);
> > -
> >  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> >  			       ep->page_size);
> >  	if (ret < 0) {
> > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	if (ep->ops->get_features) {
> > -		epc_features = ep->ops->get_features(ep);
> > -		if (epc_features->core_init_notifier)
> > -			return 0;
> > -	}
> > +	if (!ep->core_init_notifier)
> > +		return dw_pcie_ep_init_complete(ep);
> >  
> > -	return dw_pcie_ep_init_complete(ep);
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 7d6e9b7576be..aadb14159df7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -242,6 +242,7 @@ struct dw_pcie_ep {
> >  	void __iomem		*msi_mem;
> >  	phys_addr_t		msi_mem_phys;
> >  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> > +	bool			core_init_notifier;
> >  };
> >  
> >  struct dw_pcie_ops {
> > -- 
> > 2.25.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Vidya Sagar]

I have a minor comment. Otherwise this patch looks fine.
I verified it on Tegra194 and it works as expected.

Reviewed-by: Vidya Sagar <vidyas@nvidia.com>
Tested-by: Vidya Sagar <vidyas@nvidia.com>

On 7/7/2022 10:27 PM, Manivannan Sadhasivam wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
>>> For controllers supporting the CORE_INIT notifier, the resources are
>>> supposed to be enabled in the init_complete function. Currently,
>>> these controllers are enabling the resources during probe time due to
>>> the DBI access happens in dw_pcie_ep_init().
>>>
>>> This creates the dependency with the host PCIe controller since the
>>> resource enablement like PHY depends on host PCIe to be up. For the
>>> standalone endpoint usecase, this would never work. So let's move all DBI
>>> access to init_complete function if CORE_INIT notifier is used. For the
>>> controllers those doesn't support this notifier, this change is a NO-OP.
>>>
>>> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>> Cc: Om Prakash Singh <omp@nvidia.com>
>>> Cc: Vidya Sagar <vidyas@nvidia.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> A gentle ping on this patch!
>>
> 
> Ping again!
> 
> Thanks,
> Mani
> 
>> Thanks,
>> Mani
>>
>>> ---
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
>>>   drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>>>   2 files changed, 94 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> index 0eda8236c125..fb2bf4bf5ba0 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>>>      return 0;
>>>   }
>>>
>>> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)

How about using dw_pcie_ep_iatu_config() name? Just added 'ep' to be in 
sync with other API names in this file.

>>> +{
>>> +   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +   struct device *dev = pci->dev;
>>> +   void *addr;
>>> +
>>> +   dw_pcie_iatu_detect(pci);
>>> +
>>> +   ep->ib_window_map = devm_kcalloc(dev,
>>> +                                    BITS_TO_LONGS(pci->num_ib_windows),
>>> +                                    sizeof(long),
>>> +                                    GFP_KERNEL);
>>> +   if (!ep->ib_window_map)
>>> +           return -ENOMEM;
>>> +
>>> +   ep->ob_window_map = devm_kcalloc(dev,
>>> +                                    BITS_TO_LONGS(pci->num_ob_windows),
>>> +                                    sizeof(long),
>>> +                                    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;
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
>>> +{
>>> +   struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +   struct dw_pcie_ep_func *ep_func;
>>> +   struct pci_epc *epc = ep->epc;
>>> +   struct device *dev = pci->dev;
>>> +   u8 func_no;
>>> +
>>> +   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);
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>   int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>>   {
>>>      struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>>      unsigned int nbars;
>>>      u8 hdr_type;
>>>      u32 reg;
>>> -   int i;
>>> +   int ret, i;
>>> +
>>> +   if (ep->core_init_notifier) {
>>> +           ret = dw_pcie_iatu_config(ep);
>>> +           if (ret)
>>> +                   return ret;
>>> +   }
>>> +
>>> +   if (ep->core_init_notifier) {
>>> +           ret = dw_pcie_ep_func_init(ep);
>>> +           if (ret)
>>> +                   return ret;
>>> +
>>> +           if (ep->ops->ep_init)
>>> +                   ep->ops->ep_init(ep);
>>> +   }
>>>
>>>      hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>>>                 PCI_HEADER_TYPE_MASK;
>>> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
>>>   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);
>>> @@ -686,7 +756,12 @@ 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;
>>> +
>>> +   if (ep->ops->get_features) {
>>> +           epc_features = ep->ops->get_features(ep);
>>> +           if (epc_features->core_init_notifier)
>>> +                   ep->core_init_notifier = true;
>>> +   }
>>>
>>>      INIT_LIST_HEAD(&ep->func_list);
>>>
>>> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>              }
>>>      }
>>>
>>> -   dw_pcie_iatu_detect(pci);
>>> +   if (!ep->core_init_notifier) {
>>> +           ret = dw_pcie_iatu_config(ep);
>>> +           if (ret)
>>> +                   return ret;
>>> +   }
>>>
>>>      res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>>>      if (!res)
>>> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>      ep->phys_base = res->start;
>>>      ep->addr_size = resource_size(res);
>>>
>>> -   ep->ib_window_map = devm_kcalloc(dev,
>>> -                                    BITS_TO_LONGS(pci->num_ib_windows),
>>> -                                    sizeof(long),
>>> -                                    GFP_KERNEL);
>>> -   if (!ep->ib_window_map)
>>> -           return -ENOMEM;
>>> -
>>> -   ep->ob_window_map = devm_kcalloc(dev,
>>> -                                    BITS_TO_LONGS(pci->num_ob_windows),
>>> -                                    sizeof(long),
>>> -                                    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);
>>>
>>> @@ -753,23 +812,15 @@ 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);
>>> +   if (!ep->core_init_notifier) {
>>> +           ret = dw_pcie_ep_func_init(ep);
>>> +           if (ret)
>>> +                   return ret;
>>>
>>> -           list_add_tail(&ep_func->list, &ep->func_list);
>>> +           if (ep->ops->ep_init)
>>> +                   ep->ops->ep_init(ep);
>>>      }
>>>
>>> -   if (ep->ops->ep_init)
>>> -           ep->ops->ep_init(ep);
>>> -
>>>      ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>>>                             ep->page_size);
>>>      if (ret < 0) {
>>> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>              return -ENOMEM;
>>>      }
>>>
>>> -   if (ep->ops->get_features) {
>>> -           epc_features = ep->ops->get_features(ep);
>>> -           if (epc_features->core_init_notifier)
>>> -                   return 0;
>>> -   }
>>> +   if (!ep->core_init_notifier)
>>> +           return dw_pcie_ep_init_complete(ep);
>>>
>>> -   return dw_pcie_ep_init_complete(ep);
>>> +   return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 7d6e9b7576be..aadb14159df7 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -242,6 +242,7 @@ struct dw_pcie_ep {
>>>      void __iomem            *msi_mem;
>>>      phys_addr_t             msi_mem_phys;
>>>      struct pci_epf_bar      *epf_bar[PCI_STD_NUM_BARS];
>>> +   bool                    core_init_notifier;
>>>   };
>>>
>>>   struct dw_pcie_ops {
>>> --
>>> 2.25.1
>>>
> 
> --
> மணிவண்ணன் சதாசிவம்
> 

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Kunihiko Hayashi]

Hi Manivannan,

On 2022/07/08 1:57, Manivannan Sadhasivam wrote:
> On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
>>> For controllers supporting the CORE_INIT notifier, the resources are
>>> supposed to be enabled in the init_complete function. Currently,
>>> these controllers are enabling the resources during probe time due to
>>> the DBI access happens in dw_pcie_ep_init().
>>>
>>> This creates the dependency with the host PCIe controller since the
>>> resource enablement like PHY depends on host PCIe to be up. For the
>>> standalone endpoint usecase, this would never work. So let's move all
>>> DBI
>>> access to init_complete function if CORE_INIT notifier is used. For the
>>> controllers those doesn't support this notifier, this change is a NO-OP.
>>>
>>> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
>>> Cc: Om Prakash Singh <omp@nvidia.com>
>>> Cc: Vidya Sagar <vidyas@nvidia.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> A gentle ping on this patch!
>>

Sorry I couldn't check it.
I think the original source seems to have changed in the latest, so please
rebase and fix some conflicts.

> Ping again!
> 
> Thanks,
> Mani
> 
>> Thanks,
>> Mani
>>
>>> ---
>>>   .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
>>>   drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>>>   2 files changed, 94 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> index 0eda8236c125..fb2bf4bf5ba0 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>> @@ -636,6 +636,63 @@ static unsigned int
>>> dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>>>   	return 0;
>>>   }
>>>
>>> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +	struct device *dev = pci->dev;
>>> +	void *addr;
>>> +
>>> +	dw_pcie_iatu_detect(pci);
>>> +
>>> +	ep->ib_window_map = devm_kcalloc(dev,
>>> +					 BITS_TO_LONGS(pci->num_ib_windows),
>>> +					 sizeof(long),
>>> +					 GFP_KERNEL);
>>> +	if (!ep->ib_window_map)
>>> +		return -ENOMEM;
>>> +
>>> +	ep->ob_window_map = devm_kcalloc(dev,
>>> +					 BITS_TO_LONGS(pci->num_ob_windows),
>>> +					 sizeof(long),
>>> +					 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;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +	struct dw_pcie_ep_func *ep_func;
>>> +	struct pci_epc *epc = ep->epc;
>>> +	struct device *dev = pci->dev;
>>> +	u8 func_no;
>>> +
>>> +	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);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>>   {
>>>   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>>>   	unsigned int nbars;
>>>   	u8 hdr_type;
>>>   	u32 reg;
>>> -	int i;
>>> +	int ret, i;
>>> +
>>> +	if (ep->core_init_notifier) {
>>> +		ret = dw_pcie_iatu_config(ep);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (ep->core_init_notifier) {
>>> +		ret = dw_pcie_ep_func_init(ep);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		if (ep->ops->ep_init)
>>> +			ep->ops->ep_init(ep);
>>> +	}

It isn't necessary to divide function calls under same condition
(ep->core_init_notifier).

>>>
>>>   	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>>>   		   PCI_HEADER_TYPE_MASK;
>>> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
>>>   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);
>>> @@ -686,7 +756,12 @@ 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;
>>> +
>>> +	if (ep->ops->get_features) {
>>> +		epc_features = ep->ops->get_features(ep);
>>> +		if (epc_features->core_init_notifier)
>>> +			ep->core_init_notifier = true;
>>> +	}
>>>
>>>   	INIT_LIST_HEAD(&ep->func_list);
>>>
>>> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>   		}
>>>   	}
>>>
>>> -	dw_pcie_iatu_detect(pci);
>>> +	if (!ep->core_init_notifier) {
>>> +		ret = dw_pcie_iatu_config(ep);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>>
>>>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> "addr_space");
>>>   	if (!res)
>>> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>   	ep->phys_base = res->start;
>>>   	ep->addr_size = resource_size(res);
>>>
>>> -	ep->ib_window_map = devm_kcalloc(dev,
>>> -					 BITS_TO_LONGS(pci->num_ib_windows),
>>> -					 sizeof(long),
>>> -					 GFP_KERNEL);
>>> -	if (!ep->ib_window_map)
>>> -		return -ENOMEM;
>>> -
>>> -	ep->ob_window_map = devm_kcalloc(dev,
>>> -					 BITS_TO_LONGS(pci->num_ob_windows),
>>> -					 sizeof(long),
>>> -					 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);
>>>
>>> @@ -753,23 +812,15 @@ 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);
>>> +	if (!ep->core_init_notifier) {
>>> +		ret = dw_pcie_ep_func_init(ep);
>>> +		if (ret)
>>> +			return ret;
>>>
>>> -		list_add_tail(&ep_func->list, &ep->func_list);
>>> +		if (ep->ops->ep_init)
>>> +			ep->ops->ep_init(ep);
>>>   	}
>>>
>>> -	if (ep->ops->ep_init)
>>> -		ep->ops->ep_init(ep);
>>> -
>>>   	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>>>   			       ep->page_size);
>>>   	if (ret < 0) {
>>> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>   		return -ENOMEM;
>>>   	}
>>>
>>> -	if (ep->ops->get_features) {
>>> -		epc_features = ep->ops->get_features(ep);
>>> -		if (epc_features->core_init_notifier)
>>> -			return 0;
>>> -	}
>>> +	if (!ep->core_init_notifier)
>>> +		return dw_pcie_ep_init_complete(ep);
>>>
>>> -	return dw_pcie_ep_init_complete(ep);
>>> +	return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(dw_pcie_ep_init);

Since devm_pci_epc_create() is called in dw_pcie_ep_init(), EPC functions
are available at this time, however, the driver can't access any controller
registers until PERST# signal interrupt occurs.

I think that EPC functions aren't called until PERST# signal, but
I'm worried about whether controller access will occur.

>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
>>> b/drivers/pci/controller/dwc/pcie-designware.h
>>> index 7d6e9b7576be..aadb14159df7 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>> @@ -242,6 +242,7 @@ struct dw_pcie_ep {
>>>   	void __iomem		*msi_mem;
>>>   	phys_addr_t		msi_mem_phys;
>>>   	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
>>> +	bool			core_init_notifier;
>>>   };
>>>
>>>   struct dw_pcie_ops {
>>> -- 
>>> 2.25.1
>>>

Thank you,

---
Best Regards
Kunihiko Hayashi

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Sat, Jul 09, 2022 at 10:42:32AM +0900, Kunihiko Hayashi wrote:
> Hi Manivannan,
> 
> On 2022/07/08 1:57, Manivannan Sadhasivam wrote:
> > On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > > > For controllers supporting the CORE_INIT notifier, the resources are
> > > > supposed to be enabled in the init_complete function. Currently,
> > > > these controllers are enabling the resources during probe time due to
> > > > the DBI access happens in dw_pcie_ep_init().
> > > > 
> > > > This creates the dependency with the host PCIe controller since the
> > > > resource enablement like PHY depends on host PCIe to be up. For the
> > > > standalone endpoint usecase, this would never work. So let's move all
> > > > DBI
> > > > access to init_complete function if CORE_INIT notifier is used. For the
> > > > controllers those doesn't support this notifier, this change is a NO-OP.
> > > > 
> > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > A gentle ping on this patch!
> > > 
> 
> Sorry I couldn't check it.
> I think the original source seems to have changed in the latest, so please
> rebase and fix some conflicts.
> 

Sure, will do.

> > Ping again!
> > 
> > Thanks,
> > Mani
> > 
> > > Thanks,
> > > Mani
> > > 
> > > > ---
> > > >   .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> > > >   drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> > > >   2 files changed, 94 insertions(+), 45 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 0eda8236c125..fb2bf4bf5ba0 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -636,6 +636,63 @@ static unsigned int
> > > > dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> > > >   	return 0;
> > > >   }
> > > > 
> > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +	struct device *dev = pci->dev;
> > > > +	void *addr;
> > > > +
> > > > +	dw_pcie_iatu_detect(pci);
> > > > +
> > > > +	ep->ib_window_map = devm_kcalloc(dev,
> > > > +					 BITS_TO_LONGS(pci->num_ib_windows),
> > > > +					 sizeof(long),
> > > > +					 GFP_KERNEL);
> > > > +	if (!ep->ib_window_map)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ep->ob_window_map = devm_kcalloc(dev,
> > > > +					 BITS_TO_LONGS(pci->num_ob_windows),
> > > > +					 sizeof(long),
> > > > +					 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;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +	struct dw_pcie_ep_func *ep_func;
> > > > +	struct pci_epc *epc = ep->epc;
> > > > +	struct device *dev = pci->dev;
> > > > +	u8 func_no;
> > > > +
> > > > +	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);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > > >   {
> > > >   	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > > >   	unsigned int nbars;
> > > >   	u8 hdr_type;
> > > >   	u32 reg;
> > > > -	int i;
> > > > +	int ret, i;
> > > > +
> > > > +	if (ep->core_init_notifier) {
> > > > +		ret = dw_pcie_iatu_config(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	if (ep->core_init_notifier) {
> > > > +		ret = dw_pcie_ep_func_init(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		if (ep->ops->ep_init)
> > > > +			ep->ops->ep_init(ep);
> > > > +	}
> 
> It isn't necessary to divide function calls under same condition
> (ep->core_init_notifier).
> 

Right. I think one of the part came from previous patch, so I forgot to merge
them.

> > > > 
> > > >   	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> > > >   		   PCI_HEADER_TYPE_MASK;
> > > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> > > >   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);
> > > > @@ -686,7 +756,12 @@ 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;
> > > > +
> > > > +	if (ep->ops->get_features) {
> > > > +		epc_features = ep->ops->get_features(ep);
> > > > +		if (epc_features->core_init_notifier)
> > > > +			ep->core_init_notifier = true;
> > > > +	}
> > > > 
> > > >   	INIT_LIST_HEAD(&ep->func_list);
> > > > 
> > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >   		}
> > > >   	}
> > > > 
> > > > -	dw_pcie_iatu_detect(pci);
> > > > +	if (!ep->core_init_notifier) {
> > > > +		ret = dw_pcie_iatu_config(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > 
> > > >   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > "addr_space");
> > > >   	if (!res)
> > > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >   	ep->phys_base = res->start;
> > > >   	ep->addr_size = resource_size(res);
> > > > 
> > > > -	ep->ib_window_map = devm_kcalloc(dev,
> > > > -					 BITS_TO_LONGS(pci->num_ib_windows),
> > > > -					 sizeof(long),
> > > > -					 GFP_KERNEL);
> > > > -	if (!ep->ib_window_map)
> > > > -		return -ENOMEM;
> > > > -
> > > > -	ep->ob_window_map = devm_kcalloc(dev,
> > > > -					 BITS_TO_LONGS(pci->num_ob_windows),
> > > > -					 sizeof(long),
> > > > -					 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);
> > > > 
> > > > @@ -753,23 +812,15 @@ 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);
> > > > +	if (!ep->core_init_notifier) {
> > > > +		ret = dw_pcie_ep_func_init(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > 
> > > > -		list_add_tail(&ep_func->list, &ep->func_list);
> > > > +		if (ep->ops->ep_init)
> > > > +			ep->ops->ep_init(ep);
> > > >   	}
> > > > 
> > > > -	if (ep->ops->ep_init)
> > > > -		ep->ops->ep_init(ep);
> > > > -
> > > >   	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> > > >   			       ep->page_size);
> > > >   	if (ret < 0) {
> > > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >   		return -ENOMEM;
> > > >   	}
> > > > 
> > > > -	if (ep->ops->get_features) {
> > > > -		epc_features = ep->ops->get_features(ep);
> > > > -		if (epc_features->core_init_notifier)
> > > > -			return 0;
> > > > -	}
> > > > +	if (!ep->core_init_notifier)
> > > > +		return dw_pcie_ep_init_complete(ep);
> > > > 
> > > > -	return dw_pcie_ep_init_complete(ep);
> > > > +	return 0;
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> 
> Since devm_pci_epc_create() is called in dw_pcie_ep_init(), EPC functions
> are available at this time, however, the driver can't access any controller
> registers until PERST# signal interrupt occurs.
> 
> I think that EPC functions aren't called until PERST# signal, but
> I'm worried about whether controller access will occur.
> 

Most of the EPC accesses are carried out by EPF and that won't be available
until dw_pcie_ep_init_complete() gets completed for core_init_notifier case.
This is one of the concerns Kishon raised but I think here we are safe.

Thanks,
Mani

> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > > > b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 7d6e9b7576be..aadb14159df7 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -242,6 +242,7 @@ struct dw_pcie_ep {
> > > >   	void __iomem		*msi_mem;
> > > >   	phys_addr_t		msi_mem_phys;
> > > >   	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> > > > +	bool			core_init_notifier;
> > > >   };
> > > > 
> > > >   struct dw_pcie_ops {
> > > > -- 
> > > > 2.25.1
> > > > 
> 
> Thank you,
> 
> ---
> Best Regards
> Kunihiko Hayashi

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Bjorn Helgaas]

On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> For controllers supporting the CORE_INIT notifier, the resources are
> supposed to be enabled in the init_complete function. Currently,
> these controllers are enabling the resources during probe time due to
> the DBI access happens in dw_pcie_ep_init().
> 
> This creates the dependency with the host PCIe controller since the
> resource enablement like PHY depends on host PCIe to be up. For the
> standalone endpoint usecase, this would never work. So let's move all DBI
> access to init_complete function if CORE_INIT notifier is used. For the
> controllers those doesn't support this notifier, this change is a NO-OP.
> 
> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> Cc: Om Prakash Singh <omp@nvidia.com>
> Cc: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
>  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>  2 files changed, 94 insertions(+), 45 deletions(-)

Sorry this got missed.  Seems like there are two patches to solve the
same problem:

  1) This patch, and
  2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)

I don't know much about dwc or this issue, but if these patches are
functionally equivalent, I think Vidya's is a little more attractive
because:

  - It's smaller (49 insertions(+), 39 deletions(-)).

  - "core_init_notifier" looks like sort of a corner-case feature and
    Vidya's patch doesn't depend on it so it seems more maintainable.

  - It's more straightforward to read -- it basically just moves
    things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
    is exactly the sort of thing I expect if we're doing something out
    of order.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 0eda8236c125..fb2bf4bf5ba0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>  	return 0;
>  }
>  
> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct device *dev = pci->dev;
> +	void *addr;
> +
> +	dw_pcie_iatu_detect(pci);
> +
> +	ep->ib_window_map = devm_kcalloc(dev,
> +					 BITS_TO_LONGS(pci->num_ib_windows),
> +					 sizeof(long),
> +					 GFP_KERNEL);
> +	if (!ep->ib_window_map)
> +		return -ENOMEM;
> +
> +	ep->ob_window_map = devm_kcalloc(dev,
> +					 BITS_TO_LONGS(pci->num_ob_windows),
> +					 sizeof(long),
> +					 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;
> +
> +	return 0;
> +}
> +
> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct dw_pcie_ep_func *ep_func;
> +	struct pci_epc *epc = ep->epc;
> +	struct device *dev = pci->dev;
> +	u8 func_no;
> +
> +	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);
> +	}
> +
> +	return 0;
> +}
> +
>  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  	unsigned int nbars;
>  	u8 hdr_type;
>  	u32 reg;
> -	int i;
> +	int ret, i;
> +
> +	if (ep->core_init_notifier) {
> +		ret = dw_pcie_iatu_config(ep);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (ep->core_init_notifier) {
> +		ret = dw_pcie_ep_func_init(ep);
> +		if (ret)
> +			return ret;
> +
> +		if (ep->ops->ep_init)
> +			ep->ops->ep_init(ep);
> +	}
>  
>  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>  		   PCI_HEADER_TYPE_MASK;
> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
>  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);
> @@ -686,7 +756,12 @@ 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;
> +
> +	if (ep->ops->get_features) {
> +		epc_features = ep->ops->get_features(ep);
> +		if (epc_features->core_init_notifier)
> +			ep->core_init_notifier = true;
> +	}
>  
>  	INIT_LIST_HEAD(&ep->func_list);
>  
> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		}
>  	}
>  
> -	dw_pcie_iatu_detect(pci);
> +	if (!ep->core_init_notifier) {
> +		ret = dw_pcie_iatu_config(ep);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>  	if (!res)
> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	ep->phys_base = res->start;
>  	ep->addr_size = resource_size(res);
>  
> -	ep->ib_window_map = devm_kcalloc(dev,
> -					 BITS_TO_LONGS(pci->num_ib_windows),
> -					 sizeof(long),
> -					 GFP_KERNEL);
> -	if (!ep->ib_window_map)
> -		return -ENOMEM;
> -
> -	ep->ob_window_map = devm_kcalloc(dev,
> -					 BITS_TO_LONGS(pci->num_ob_windows),
> -					 sizeof(long),
> -					 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);
>  
> @@ -753,23 +812,15 @@ 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);
> +	if (!ep->core_init_notifier) {
> +		ret = dw_pcie_ep_func_init(ep);
> +		if (ret)
> +			return ret;
>  
> -		list_add_tail(&ep_func->list, &ep->func_list);
> +		if (ep->ops->ep_init)
> +			ep->ops->ep_init(ep);
>  	}
>  
> -	if (ep->ops->ep_init)
> -		ep->ops->ep_init(ep);
> -
>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
>  	if (ret < 0) {
> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		return -ENOMEM;
>  	}
>  
> -	if (ep->ops->get_features) {
> -		epc_features = ep->ops->get_features(ep);
> -		if (epc_features->core_init_notifier)
> -			return 0;
> -	}
> +	if (!ep->core_init_notifier)
> +		return dw_pcie_ep_init_complete(ep);
>  
> -	return dw_pcie_ep_init_complete(ep);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 7d6e9b7576be..aadb14159df7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -242,6 +242,7 @@ struct dw_pcie_ep {
>  	void __iomem		*msi_mem;
>  	phys_addr_t		msi_mem_phys;
>  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> +	bool			core_init_notifier;
>  };
>  
>  struct dw_pcie_ops {
> -- 
> 2.25.1
> 

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote:
> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > For controllers supporting the CORE_INIT notifier, the resources are
> > supposed to be enabled in the init_complete function. Currently,
> > these controllers are enabling the resources during probe time due to
> > the DBI access happens in dw_pcie_ep_init().
> > 
> > This creates the dependency with the host PCIe controller since the
> > resource enablement like PHY depends on host PCIe to be up. For the
> > standalone endpoint usecase, this would never work. So let's move all DBI
> > access to init_complete function if CORE_INIT notifier is used. For the
> > controllers those doesn't support this notifier, this change is a NO-OP.
> > 
> > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > Cc: Om Prakash Singh <omp@nvidia.com>
> > Cc: Vidya Sagar <vidyas@nvidia.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> >  2 files changed, 94 insertions(+), 45 deletions(-)
> 
> Sorry this got missed.  Seems like there are two patches to solve the
> same problem:
> 
>   1) This patch, and
>   2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)
> 
> I don't know much about dwc or this issue, but if these patches are
> functionally equivalent, I think Vidya's is a little more attractive
> because:
> 
>   - It's smaller (49 insertions(+), 39 deletions(-)).
> 
>   - "core_init_notifier" looks like sort of a corner-case feature and
>     Vidya's patch doesn't depend on it so it seems more maintainable.
> 
>   - It's more straightforward to read -- it basically just moves
>     things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
>     is exactly the sort of thing I expect if we're doing something out
>     of order.
> 

I agree that Vidya's patch is simple but as per the feedback from Kishon on my
previous patch, I had to add some extra logic to make sure the move of DBI
access doesn't affect the non core_init_notifier platforms.

So with my patch, the logic added is essentailly a NO-OP on those.

Thanks,
Mani

> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 0eda8236c125..fb2bf4bf5ba0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> >  	return 0;
> >  }
> >  
> > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct device *dev = pci->dev;
> > +	void *addr;
> > +
> > +	dw_pcie_iatu_detect(pci);
> > +
> > +	ep->ib_window_map = devm_kcalloc(dev,
> > +					 BITS_TO_LONGS(pci->num_ib_windows),
> > +					 sizeof(long),
> > +					 GFP_KERNEL);
> > +	if (!ep->ib_window_map)
> > +		return -ENOMEM;
> > +
> > +	ep->ob_window_map = devm_kcalloc(dev,
> > +					 BITS_TO_LONGS(pci->num_ob_windows),
> > +					 sizeof(long),
> > +					 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;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct dw_pcie_ep_func *ep_func;
> > +	struct pci_epc *epc = ep->epc;
> > +	struct device *dev = pci->dev;
> > +	u8 func_no;
> > +
> > +	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);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> >  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> >  	unsigned int nbars;
> >  	u8 hdr_type;
> >  	u32 reg;
> > -	int i;
> > +	int ret, i;
> > +
> > +	if (ep->core_init_notifier) {
> > +		ret = dw_pcie_iatu_config(ep);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (ep->core_init_notifier) {
> > +		ret = dw_pcie_ep_func_init(ep);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (ep->ops->ep_init)
> > +			ep->ops->ep_init(ep);
> > +	}
> >  
> >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> >  		   PCI_HEADER_TYPE_MASK;
> > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> >  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);
> > @@ -686,7 +756,12 @@ 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;
> > +
> > +	if (ep->ops->get_features) {
> > +		epc_features = ep->ops->get_features(ep);
> > +		if (epc_features->core_init_notifier)
> > +			ep->core_init_notifier = true;
> > +	}
> >  
> >  	INIT_LIST_HEAD(&ep->func_list);
> >  
> > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		}
> >  	}
> >  
> > -	dw_pcie_iatu_detect(pci);
> > +	if (!ep->core_init_notifier) {
> > +		ret = dw_pcie_iatu_config(ep);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> >  	if (!res)
> > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  	ep->phys_base = res->start;
> >  	ep->addr_size = resource_size(res);
> >  
> > -	ep->ib_window_map = devm_kcalloc(dev,
> > -					 BITS_TO_LONGS(pci->num_ib_windows),
> > -					 sizeof(long),
> > -					 GFP_KERNEL);
> > -	if (!ep->ib_window_map)
> > -		return -ENOMEM;
> > -
> > -	ep->ob_window_map = devm_kcalloc(dev,
> > -					 BITS_TO_LONGS(pci->num_ob_windows),
> > -					 sizeof(long),
> > -					 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);
> >  
> > @@ -753,23 +812,15 @@ 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);
> > +	if (!ep->core_init_notifier) {
> > +		ret = dw_pcie_ep_func_init(ep);
> > +		if (ret)
> > +			return ret;
> >  
> > -		list_add_tail(&ep_func->list, &ep->func_list);
> > +		if (ep->ops->ep_init)
> > +			ep->ops->ep_init(ep);
> >  	}
> >  
> > -	if (ep->ops->ep_init)
> > -		ep->ops->ep_init(ep);
> > -
> >  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> >  			       ep->page_size);
> >  	if (ret < 0) {
> > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  		return -ENOMEM;
> >  	}
> >  
> > -	if (ep->ops->get_features) {
> > -		epc_features = ep->ops->get_features(ep);
> > -		if (epc_features->core_init_notifier)
> > -			return 0;
> > -	}
> > +	if (!ep->core_init_notifier)
> > +		return dw_pcie_ep_init_complete(ep);
> >  
> > -	return dw_pcie_ep_init_complete(ep);
> > +	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 7d6e9b7576be..aadb14159df7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -242,6 +242,7 @@ struct dw_pcie_ep {
> >  	void __iomem		*msi_mem;
> >  	phys_addr_t		msi_mem_phys;
> >  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> > +	bool			core_init_notifier;
> >  };
> >  
> >  struct dw_pcie_ops {
> > -- 
> > 2.25.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Bjorn Helgaas]

On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > > For controllers supporting the CORE_INIT notifier, the resources are
> > > supposed to be enabled in the init_complete function. Currently,
> > > these controllers are enabling the resources during probe time due to
> > > the DBI access happens in dw_pcie_ep_init().
> > > 
> > > This creates the dependency with the host PCIe controller since the
> > > resource enablement like PHY depends on host PCIe to be up. For the
> > > standalone endpoint usecase, this would never work. So let's move all DBI
> > > access to init_complete function if CORE_INIT notifier is used. For the
> > > controllers those doesn't support this notifier, this change is a NO-OP.
> > > 
> > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> > >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> > >  2 files changed, 94 insertions(+), 45 deletions(-)
> > 
> > Sorry this got missed.  Seems like there are two patches to solve the
> > same problem:
> > 
> >   1) This patch, and
> >   2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)
> > 
> > I don't know much about dwc or this issue, but if these patches are
> > functionally equivalent, I think Vidya's is a little more attractive
> > because:
> > 
> >   - It's smaller (49 insertions(+), 39 deletions(-)).
> > 
> >   - "core_init_notifier" looks like sort of a corner-case feature and
> >     Vidya's patch doesn't depend on it so it seems more maintainable.
> > 
> >   - It's more straightforward to read -- it basically just moves
> >     things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
> >     is exactly the sort of thing I expect if we're doing something out
> >     of order.
> 
> I agree that Vidya's patch is simple but as per the feedback from
> Kishon on my previous patch, I had to add some extra logic to make
> sure the move of DBI access doesn't affect the non
> core_init_notifier platforms.
> 
> So with my patch, the logic added is essentailly a NO-OP on those.

Can you include the lore URL for Kishon's feedback?  I can't find it.

If we think moving the DBI access is safe on non-core_init_notifier
platforms, I'd like to do it everywhere so they're all the same.  I
don't want different behavior just to avoid the risk of theoretical
problems that we think should not happen.

> Thanks,
> Mani
> 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 0eda8236c125..fb2bf4bf5ba0 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> > >  	return 0;
> > >  }
> > >  
> > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > +	struct device *dev = pci->dev;
> > > +	void *addr;
> > > +
> > > +	dw_pcie_iatu_detect(pci);
> > > +
> > > +	ep->ib_window_map = devm_kcalloc(dev,
> > > +					 BITS_TO_LONGS(pci->num_ib_windows),
> > > +					 sizeof(long),
> > > +					 GFP_KERNEL);
> > > +	if (!ep->ib_window_map)
> > > +		return -ENOMEM;
> > > +
> > > +	ep->ob_window_map = devm_kcalloc(dev,
> > > +					 BITS_TO_LONGS(pci->num_ob_windows),
> > > +					 sizeof(long),
> > > +					 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;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > +	struct dw_pcie_ep_func *ep_func;
> > > +	struct pci_epc *epc = ep->epc;
> > > +	struct device *dev = pci->dev;
> > > +	u8 func_no;
> > > +
> > > +	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);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > >  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > >  	unsigned int nbars;
> > >  	u8 hdr_type;
> > >  	u32 reg;
> > > -	int i;
> > > +	int ret, i;
> > > +
> > > +	if (ep->core_init_notifier) {
> > > +		ret = dw_pcie_iatu_config(ep);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (ep->core_init_notifier) {
> > > +		ret = dw_pcie_ep_func_init(ep);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		if (ep->ops->ep_init)
> > > +			ep->ops->ep_init(ep);
> > > +	}
> > >  
> > >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> > >  		   PCI_HEADER_TYPE_MASK;
> > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> > >  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);
> > > @@ -686,7 +756,12 @@ 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;
> > > +
> > > +	if (ep->ops->get_features) {
> > > +		epc_features = ep->ops->get_features(ep);
> > > +		if (epc_features->core_init_notifier)
> > > +			ep->core_init_notifier = true;
> > > +	}
> > >  
> > >  	INIT_LIST_HEAD(&ep->func_list);
> > >  
> > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > >  		}
> > >  	}
> > >  
> > > -	dw_pcie_iatu_detect(pci);
> > > +	if (!ep->core_init_notifier) {
> > > +		ret = dw_pcie_iatu_config(ep);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > >  
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> > >  	if (!res)
> > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > >  	ep->phys_base = res->start;
> > >  	ep->addr_size = resource_size(res);
> > >  
> > > -	ep->ib_window_map = devm_kcalloc(dev,
> > > -					 BITS_TO_LONGS(pci->num_ib_windows),
> > > -					 sizeof(long),
> > > -					 GFP_KERNEL);
> > > -	if (!ep->ib_window_map)
> > > -		return -ENOMEM;
> > > -
> > > -	ep->ob_window_map = devm_kcalloc(dev,
> > > -					 BITS_TO_LONGS(pci->num_ob_windows),
> > > -					 sizeof(long),
> > > -					 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);
> > >  
> > > @@ -753,23 +812,15 @@ 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);
> > > +	if (!ep->core_init_notifier) {
> > > +		ret = dw_pcie_ep_func_init(ep);
> > > +		if (ret)
> > > +			return ret;
> > >  
> > > -		list_add_tail(&ep_func->list, &ep->func_list);
> > > +		if (ep->ops->ep_init)
> > > +			ep->ops->ep_init(ep);
> > >  	}
> > >  
> > > -	if (ep->ops->ep_init)
> > > -		ep->ops->ep_init(ep);
> > > -
> > >  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> > >  			       ep->page_size);
> > >  	if (ret < 0) {
> > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > >  		return -ENOMEM;
> > >  	}
> > >  
> > > -	if (ep->ops->get_features) {
> > > -		epc_features = ep->ops->get_features(ep);
> > > -		if (epc_features->core_init_notifier)
> > > -			return 0;
> > > -	}
> > > +	if (!ep->core_init_notifier)
> > > +		return dw_pcie_ep_init_complete(ep);
> > >  
> > > -	return dw_pcie_ep_init_complete(ep);
> > > +	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 7d6e9b7576be..aadb14159df7 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -242,6 +242,7 @@ struct dw_pcie_ep {
> > >  	void __iomem		*msi_mem;
> > >  	phys_addr_t		msi_mem_phys;
> > >  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> > > +	bool			core_init_notifier;
> > >  };
> > >  
> > >  struct dw_pcie_ops {
> > > -- 
> > > 2.25.1
> > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > > > For controllers supporting the CORE_INIT notifier, the resources are
> > > > supposed to be enabled in the init_complete function. Currently,
> > > > these controllers are enabling the resources during probe time due to
> > > > the DBI access happens in dw_pcie_ep_init().
> > > > 
> > > > This creates the dependency with the host PCIe controller since the
> > > > resource enablement like PHY depends on host PCIe to be up. For the
> > > > standalone endpoint usecase, this would never work. So let's move all DBI
> > > > access to init_complete function if CORE_INIT notifier is used. For the
> > > > controllers those doesn't support this notifier, this change is a NO-OP.
> > > > 
> > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> > > >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> > > >  2 files changed, 94 insertions(+), 45 deletions(-)
> > > 
> > > Sorry this got missed.  Seems like there are two patches to solve the
> > > same problem:
> > > 
> > >   1) This patch, and
> > >   2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)
> > > 
> > > I don't know much about dwc or this issue, but if these patches are
> > > functionally equivalent, I think Vidya's is a little more attractive
> > > because:
> > > 
> > >   - It's smaller (49 insertions(+), 39 deletions(-)).
> > > 
> > >   - "core_init_notifier" looks like sort of a corner-case feature and
> > >     Vidya's patch doesn't depend on it so it seems more maintainable.
> > > 
> > >   - It's more straightforward to read -- it basically just moves
> > >     things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
> > >     is exactly the sort of thing I expect if we're doing something out
> > >     of order.
> > 
> > I agree that Vidya's patch is simple but as per the feedback from
> > Kishon on my previous patch, I had to add some extra logic to make
> > sure the move of DBI access doesn't affect the non
> > core_init_notifier platforms.
> > 
> > So with my patch, the logic added is essentailly a NO-OP on those.
> 
> Can you include the lore URL for Kishon's feedback?  I can't find it.
> 

https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629

> If we think moving the DBI access is safe on non-core_init_notifier
> platforms, I'd like to do it everywhere so they're all the same.  I
> don't want different behavior just to avoid the risk of theoretical
> problems that we think should not happen.
> 

One more issue Kishon pointed out was that, in the patch the endpoint
controller is configured after pci_epc_create(). So he raised a concern that
if ecp_ops is invoked before the controller gets configured fully, it could
result in aborts.

While the concern may be true for non-core_init_notifier platforms (I'm not sure
though) but I'm certain not for the core_init_notifier ones as the EFP drivers
only access EPC ops after dw_pcie_ep_init_complete().

Thanks,
Mani 

> > Thanks,
> > Mani
> > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > index 0eda8236c125..fb2bf4bf5ba0 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +	struct device *dev = pci->dev;
> > > > +	void *addr;
> > > > +
> > > > +	dw_pcie_iatu_detect(pci);
> > > > +
> > > > +	ep->ib_window_map = devm_kcalloc(dev,
> > > > +					 BITS_TO_LONGS(pci->num_ib_windows),
> > > > +					 sizeof(long),
> > > > +					 GFP_KERNEL);
> > > > +	if (!ep->ib_window_map)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ep->ob_window_map = devm_kcalloc(dev,
> > > > +					 BITS_TO_LONGS(pci->num_ob_windows),
> > > > +					 sizeof(long),
> > > > +					 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;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +	struct dw_pcie_ep_func *ep_func;
> > > > +	struct pci_epc *epc = ep->epc;
> > > > +	struct device *dev = pci->dev;
> > > > +	u8 func_no;
> > > > +
> > > > +	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);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > > >  {
> > > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
> > > >  	unsigned int nbars;
> > > >  	u8 hdr_type;
> > > >  	u32 reg;
> > > > -	int i;
> > > > +	int ret, i;
> > > > +
> > > > +	if (ep->core_init_notifier) {
> > > > +		ret = dw_pcie_iatu_config(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	if (ep->core_init_notifier) {
> > > > +		ret = dw_pcie_ep_func_init(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		if (ep->ops->ep_init)
> > > > +			ep->ops->ep_init(ep);
> > > > +	}
> > > >  
> > > >  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
> > > >  		   PCI_HEADER_TYPE_MASK;
> > > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete);
> > > >  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);
> > > > @@ -686,7 +756,12 @@ 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;
> > > > +
> > > > +	if (ep->ops->get_features) {
> > > > +		epc_features = ep->ops->get_features(ep);
> > > > +		if (epc_features->core_init_notifier)
> > > > +			ep->core_init_notifier = true;
> > > > +	}
> > > >  
> > > >  	INIT_LIST_HEAD(&ep->func_list);
> > > >  
> > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >  		}
> > > >  	}
> > > >  
> > > > -	dw_pcie_iatu_detect(pci);
> > > > +	if (!ep->core_init_notifier) {
> > > > +		ret = dw_pcie_iatu_config(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > >  
> > > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> > > >  	if (!res)
> > > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >  	ep->phys_base = res->start;
> > > >  	ep->addr_size = resource_size(res);
> > > >  
> > > > -	ep->ib_window_map = devm_kcalloc(dev,
> > > > -					 BITS_TO_LONGS(pci->num_ib_windows),
> > > > -					 sizeof(long),
> > > > -					 GFP_KERNEL);
> > > > -	if (!ep->ib_window_map)
> > > > -		return -ENOMEM;
> > > > -
> > > > -	ep->ob_window_map = devm_kcalloc(dev,
> > > > -					 BITS_TO_LONGS(pci->num_ob_windows),
> > > > -					 sizeof(long),
> > > > -					 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);
> > > >  
> > > > @@ -753,23 +812,15 @@ 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);
> > > > +	if (!ep->core_init_notifier) {
> > > > +		ret = dw_pcie_ep_func_init(ep);
> > > > +		if (ret)
> > > > +			return ret;
> > > >  
> > > > -		list_add_tail(&ep_func->list, &ep->func_list);
> > > > +		if (ep->ops->ep_init)
> > > > +			ep->ops->ep_init(ep);
> > > >  	}
> > > >  
> > > > -	if (ep->ops->ep_init)
> > > > -		ep->ops->ep_init(ep);
> > > > -
> > > >  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
> > > >  			       ep->page_size);
> > > >  	if (ret < 0) {
> > > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > -	if (ep->ops->get_features) {
> > > > -		epc_features = ep->ops->get_features(ep);
> > > > -		if (epc_features->core_init_notifier)
> > > > -			return 0;
> > > > -	}
> > > > +	if (!ep->core_init_notifier)
> > > > +		return dw_pcie_ep_init_complete(ep);
> > > >  
> > > > -	return dw_pcie_ep_init_complete(ep);
> > > > +	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 7d6e9b7576be..aadb14159df7 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -242,6 +242,7 @@ struct dw_pcie_ep {
> > > >  	void __iomem		*msi_mem;
> > > >  	phys_addr_t		msi_mem_phys;
> > > >  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
> > > > +	bool			core_init_notifier;
> > > >  };
> > > >  
> > > >  struct dw_pcie_ops {
> > > > -- 
> > > > 2.25.1
> > > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Bjorn Helgaas]

On Thu, Jul 21, 2022 at 02:08:45PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > > > > For controllers supporting the CORE_INIT notifier, the resources are
> > > > > supposed to be enabled in the init_complete function. Currently,
> > > > > these controllers are enabling the resources during probe time due to
> > > > > the DBI access happens in dw_pcie_ep_init().
> > > > > 
> > > > > This creates the dependency with the host PCIe controller since the
> > > > > resource enablement like PHY depends on host PCIe to be up. For the
> > > > > standalone endpoint usecase, this would never work. So let's move all DBI
> > > > > access to init_complete function if CORE_INIT notifier is used. For the
> > > > > controllers those doesn't support this notifier, this change is a NO-OP.
> > > > > 
> > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> > > > >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> > > > >  2 files changed, 94 insertions(+), 45 deletions(-)
> > > > 
> > > > Sorry this got missed.  Seems like there are two patches to solve the
> > > > same problem:
> > > > 
> > > >   1) This patch, and
> > > >   2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)
> > > > 
> > > > I don't know much about dwc or this issue, but if these patches are
> > > > functionally equivalent, I think Vidya's is a little more attractive
> > > > because:
> > > > 
> > > >   - It's smaller (49 insertions(+), 39 deletions(-)).
> > > > 
> > > >   - "core_init_notifier" looks like sort of a corner-case feature and
> > > >     Vidya's patch doesn't depend on it so it seems more maintainable.
> > > > 
> > > >   - It's more straightforward to read -- it basically just moves
> > > >     things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
> > > >     is exactly the sort of thing I expect if we're doing something out
> > > >     of order.
> > > 
> > > I agree that Vidya's patch is simple but as per the feedback from
> > > Kishon on my previous patch, I had to add some extra logic to make
> > > sure the move of DBI access doesn't affect the non
> > > core_init_notifier platforms.
> > > 
> > > So with my patch, the logic added is essentailly a NO-OP on those.
> > 
> > Can you include the lore URL for Kishon's feedback?  I can't find it.
> 
> https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629

Thanks!  (Or the canonical permanent URL:
https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@ti.com)

> > If we think moving the DBI access is safe on non-core_init_notifier
> > platforms, I'd like to do it everywhere so they're all the same.  I
> > don't want different behavior just to avoid the risk of theoretical
> > problems that we think should not happen.
> 
> One more issue Kishon pointed out was that, in the patch the
> endpoint controller is configured after pci_epc_create(). So he
> raised a concern that if ecp_ops is invoked before the controller
> gets configured fully, it could result in aborts.
> 
> While the concern may be true for non-core_init_notifier platforms
> (I'm not sure though) but I'm certain not for the core_init_notifier
> ones as the EFP drivers only access EPC ops after
> dw_pcie_ep_init_complete().

I really don't understand what "core_init_notifier" does, but it seems
incidental to this issue.  Is there really a connection?  It sounds
like the only reason to check for it is to limit the scope of the
change, not because DBI access is inherently related to
core_init_notifier.

Maybe Vidya's patch needs some enhancement to cover other paths where
DBI may be accessed before we're ready?

Bjorn

Re: [PATCH] PCI: designware-ep: Move DBI access to init_complete if notifier is used

[Manivannan Sadhasivam]

On Thu, Jul 21, 2022 at 10:30:55AM -0500, Bjorn Helgaas wrote:
> On Thu, Jul 21, 2022 at 02:08:45PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote:
> > > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote:
> > > > > > For controllers supporting the CORE_INIT notifier, the resources are
> > > > > > supposed to be enabled in the init_complete function. Currently,
> > > > > > these controllers are enabling the resources during probe time due to
> > > > > > the DBI access happens in dw_pcie_ep_init().
> > > > > > 
> > > > > > This creates the dependency with the host PCIe controller since the
> > > > > > resource enablement like PHY depends on host PCIe to be up. For the
> > > > > > standalone endpoint usecase, this would never work. So let's move all DBI
> > > > > > access to init_complete function if CORE_INIT notifier is used. For the
> > > > > > controllers those doesn't support this notifier, this change is a NO-OP.
> > > > > > 
> > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > > > > Cc: Om Prakash Singh <omp@nvidia.com>
> > > > > > Cc: Vidya Sagar <vidyas@nvidia.com>
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > ---
> > > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 138 ++++++++++++------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
> > > > > >  2 files changed, 94 insertions(+), 45 deletions(-)
> > > > > 
> > > > > Sorry this got missed.  Seems like there are two patches to solve the
> > > > > same problem:
> > > > > 
> > > > >   1) This patch, and
> > > > >   2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/)
> > > > > 
> > > > > I don't know much about dwc or this issue, but if these patches are
> > > > > functionally equivalent, I think Vidya's is a little more attractive
> > > > > because:
> > > > > 
> > > > >   - It's smaller (49 insertions(+), 39 deletions(-)).
> > > > > 
> > > > >   - "core_init_notifier" looks like sort of a corner-case feature and
> > > > >     Vidya's patch doesn't depend on it so it seems more maintainable.
> > > > > 
> > > > >   - It's more straightforward to read -- it basically just moves
> > > > >     things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which
> > > > >     is exactly the sort of thing I expect if we're doing something out
> > > > >     of order.
> > > > 
> > > > I agree that Vidya's patch is simple but as per the feedback from
> > > > Kishon on my previous patch, I had to add some extra logic to make
> > > > sure the move of DBI access doesn't affect the non
> > > > core_init_notifier platforms.
> > > > 
> > > > So with my patch, the logic added is essentailly a NO-OP on those.
> > > 
> > > Can you include the lore URL for Kishon's feedback?  I can't find it.
> > 
> > https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629
> 
> Thanks!  (Or the canonical permanent URL:
> https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@ti.com)
> 
> > > If we think moving the DBI access is safe on non-core_init_notifier
> > > platforms, I'd like to do it everywhere so they're all the same.  I
> > > don't want different behavior just to avoid the risk of theoretical
> > > problems that we think should not happen.
> > 
> > One more issue Kishon pointed out was that, in the patch the
> > endpoint controller is configured after pci_epc_create(). So he
> > raised a concern that if ecp_ops is invoked before the controller
> > gets configured fully, it could result in aborts.
> > 
> > While the concern may be true for non-core_init_notifier platforms
> > (I'm not sure though) but I'm certain not for the core_init_notifier
> > ones as the EFP drivers only access EPC ops after
> > dw_pcie_ep_init_complete().
> 
> I really don't understand what "core_init_notifier" does, but it seems
> incidental to this issue.

"core_init_notifer" flag was added to let the EPF driver know that the
endpoint controller has completed its initialization. This is needed for
platforms that require refclk from the host to be active before accessing
any controller specific registers. Tegra194 and Qcom are one of those kinds.

> Is there really a connection?  It sounds
> like the only reason to check for it is to limit the scope of the
> change, not because DBI access is inherently related to
> core_init_notifier.
> 

DBI access requires refclk on the above mentioned platforms, so yes
there is a strong connection.

This patch (also mine) moves away all of the DBI access to the
init_complete() notifier callback. Because, without this patch we need
to bring-up the host first and then power-on the endpoint. Otherwise,
if the endpoint is powered-on first, it will crash as there would no
refclk from host.

> Maybe Vidya's patch needs some enhancement to cover other paths where
> DBI may be accessed before we're ready?
> 

FWIW, Vidya's patch looks good to me. I pointed out my patch just
because it covers the theoretical issue mentioned by Kishon.

Thanks,
Mani

> Bjorn