Message-ID: <ZdzDCcMjrNVFHg8S@lizhi-Precision-Tower-5810>
Date: Mon, 26 Feb 2024 11:57:45 -0500
From: Frank Li <Frank.li@....com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Jingoo Han <jingoohan1@...il.com>,
	Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Marek Vasut <marek.vasut+renesas@...il.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Kishon Vijay Abraham I <kishon@...com>,
	Vidya Sagar <vidyas@...dia.com>,
	Vignesh Raghavendra <vigneshr@...com>,
	Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Minghuan Lian <minghuan.Lian@....com>,
	Mingkai Hu <mingkai.hu@....com>, Roy Zang <roy.zang@....com>,
	Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-omap@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linuxppc-dev@...ts.ozlabs.org,
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v8 04/10] PCI: dwc: ep: Fix DBI access failure for
 drivers requiring refclk from host

On Sat, Feb 24, 2024 at 12:24:10PM +0530, Manivannan Sadhasivam wrote:
> The DWC glue drivers requiring an active reference clock from the PCIe host
> for initializing their PCIe EP core, set a flag called 'core_init_notifier'
> to let DWC driver know that these drivers need a special attention during
> initialization. In these drivers, access to the hw registers (like DBI)
> before receiving the active refclk from host will result in access failure
> and also could cause a whole system hang.
> 
> But the current DWC EP driver doesn't honor the requirements of the drivers
> setting 'core_init_notifier' flag and tries to access the DBI registers
> during dw_pcie_ep_init(). This causes the system hang for glue drivers such
> as Tegra194 and Qcom EP as they depend on refclk from host and have set the
> above mentioned flag.
> 
> To workaround this issue, users of the affected platforms have to maintain
> the dependency with the PCIe host by booting the PCIe EP after host boot.
> But this won't provide a good user experience, since PCIe EP is _one_ of
> the features of those platforms and it doesn't make sense to delay the
> whole platform booting due to PCIe requiring active refclk.
> 
> So to fix this issue, let's move all the DBI access from
> dw_pcie_ep_init() in the DWC EP driver to the dw_pcie_ep_init_complete()
> API. This API will only be called by the drivers setting
> 'core_init_notifier' flag once refclk is received from host. For the rest
> of the drivers that gets the refclk locally, this API will be called
> within dw_pcie_ep_init().
> 
> Fixes: e966f7390da9 ("PCI: dwc: Refactor core initialization code for EP mode")
> Co-developed-by: Vidya Sagar <vidyas@...dia.com>
> Signed-off-by: Vidya Sagar <vidyas@...dia.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>

Reviewed-by: Frank Li <Frank.Li@....com>

> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 120 ++++++++++++++----------
>  1 file changed, 71 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1205bfba8310..99d66b0fa59b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -606,11 +606,16 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
>  int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct dw_pcie_ep_func *ep_func;
> +	struct device *dev = pci->dev;
> +	struct pci_epc *epc = ep->epc;
>  	unsigned int offset, ptm_cap_base;
>  	unsigned int nbars;
>  	u8 hdr_type;
> +	u8 func_no;
> +	int i, ret;
> +	void *addr;
>  	u32 reg;
> -	int i;
>  
>  	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) &
>  		   PCI_HEADER_TYPE_MASK;
> @@ -621,6 +626,58 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  		return -EIO;
>  	}
>  
> +	dw_pcie_version_detect(pci);
> +
> +	dw_pcie_iatu_detect(pci);
> +
> +	ret = dw_pcie_edma_detect(pci);
> +	if (ret)
> +		return ret;
> +
> +	if (!ep->ib_window_map) {
> +		ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
> +						       GFP_KERNEL);
> +		if (!ep->ib_window_map)
> +			goto err_remove_edma;
> +	}
> +
> +	if (!ep->ob_window_map) {
> +		ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
> +						       GFP_KERNEL);
> +		if (!ep->ob_window_map)
> +			goto err_remove_edma;
> +	}
> +
> +	if (!ep->outbound_addr) {
> +		addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
> +				    GFP_KERNEL);
> +		if (!addr)
> +			goto err_remove_edma;
> +		ep->outbound_addr = addr;
> +	}
> +
> +	for (func_no = 0; func_no < epc->max_functions; func_no++) {
> +
> +		ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
> +		if (ep_func)
> +			continue;
> +
> +		ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
> +		if (!ep_func)
> +			goto err_remove_edma;
> +
> +		ep_func->func_no = func_no;
> +		ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
> +							      PCI_CAP_ID_MSI);
> +		ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
> +							       PCI_CAP_ID_MSIX);
> +
> +		list_add_tail(&ep_func->list, &ep->func_list);
> +	}
> +
> +	if (ep->ops->init)
> +		ep->ops->init(ep);
> +
>  	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
>  	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
>  
> @@ -655,14 +712,17 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep)
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  
>  	return 0;
> +
> +err_remove_edma:
> +	dw_pcie_edma_remove(pci);
> +
> +	return ret;
>  }
>  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);
> @@ -670,7 +730,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct device_node *np = dev->of_node;
>  	const struct pci_epc_features *epc_features;
> -	struct dw_pcie_ep_func *ep_func;
>  
>  	INIT_LIST_HEAD(&ep->func_list);
>  
> @@ -688,26 +747,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ep->ops->pre_init)
>  		ep->ops->pre_init(ep);
>  
> -	dw_pcie_version_detect(pci);
> -
> -	dw_pcie_iatu_detect(pci);
> -
> -	ep->ib_window_map = devm_bitmap_zalloc(dev, pci->num_ib_windows,
> -					       GFP_KERNEL);
> -	if (!ep->ib_window_map)
> -		return -ENOMEM;
> -
> -	ep->ob_window_map = devm_bitmap_zalloc(dev, pci->num_ob_windows,
> -					       GFP_KERNEL);
> -	if (!ep->ob_window_map)
> -		return -ENOMEM;
> -
> -	addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t),
> -			    GFP_KERNEL);
> -	if (!addr)
> -		return -ENOMEM;
> -	ep->outbound_addr = addr;
> -
>  	epc = devm_pci_epc_create(dev, &epc_ops);
>  	if (IS_ERR(epc)) {
>  		dev_err(dev, "Failed to create epc device\n");
> @@ -721,23 +760,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	if (ret < 0)
>  		epc->max_functions = 1;
>  
> -	for (func_no = 0; func_no < epc->max_functions; func_no++) {
> -		ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL);
> -		if (!ep_func)
> -			return -ENOMEM;
> -
> -		ep_func->func_no = func_no;
> -		ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no,
> -							      PCI_CAP_ID_MSI);
> -		ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no,
> -							       PCI_CAP_ID_MSIX);
> -
> -		list_add_tail(&ep_func->list, &ep->func_list);
> -	}
> -
> -	if (ep->ops->init)
> -		ep->ops->init(ep);
> -
>  	ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size,
>  			       ep->page_size);
>  	if (ret < 0) {
> @@ -753,25 +775,25 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		goto err_exit_epc_mem;
>  	}
>  
> -	ret = dw_pcie_edma_detect(pci);
> -	if (ret)
> -		goto err_free_epc_mem;
> -
>  	if (ep->ops->get_features) {
>  		epc_features = ep->ops->get_features(ep);
>  		if (epc_features->core_init_notifier)
>  			return 0;
>  	}
>  
> +	/*
> +	 * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this
> +	 * step as platforms that implement 'core_init_notifier' feature may
> +	 * not have the hardware ready (i.e. core initialized) for access
> +	 * (Ex: tegra194). Any hardware access on such platforms result
> +	 * in system hang.
> +	 */
>  	ret = dw_pcie_ep_init_complete(ep);
>  	if (ret)
> -		goto err_remove_edma;
> +		goto err_free_epc_mem;
>  
>  	return 0;
>  
> -err_remove_edma:
> -	dw_pcie_edma_remove(pci);
> -
>  err_free_epc_mem:
>  	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
>  			      epc->mem->window.page_size);
> 
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists