Re: [PATCH v8 04/10] PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host

Frank Li
2024-02-26T16:58:26Z

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@nvidia.com>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Frank Li <Frank.Li@nxp.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