| Message ID | 20220622040133.31058-1-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V1] PCI: designware-ep: Fix DBI access before core init | expand |
Hi, Anyone has review comments for this change? Without this change, Tegra194's endpoint mode is effectively broken. Thanks & Regards, Vidya Sagar On 6/22/2022 9:31 AM, Vidya Sagar wrote: > Platforms that cannot support their core initialization without the > reference clock from the host, implement the feature 'core_init_notifier' > to indicate the DesignWare sub-system about when their core is getting > initialized. Any accesses to the core (Ex:- DBI) would result in system > hang in such systems (Ex:- tegra194). This patch moves any access to the > core to dw_pcie_ep_init_complete() API which is effectively called only > after the core initialization. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > 1 file changed, 49 insertions(+), 39 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 0eda8236c125..9feec720175f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -639,9 +639,14 @@ 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; > unsigned int nbars; > u8 hdr_type; > + u8 func_no; > + void *addr; > u32 reg; > int i; > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > return -EIO; > } > > + 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; > + > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > + if (!ep_func) > + return -ENOMEM; > + > + ep_func->func_no = func_no; > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSI); > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSIX); > + > + list_add_tail(&ep_func->list, &ep->func_list); > + } > + > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > dw_pcie_dbi_ro_wr_en(pci); > @@ -677,8 +718,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 +725,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); > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > } > } > > - dw_pcie_iatu_detect(pci); > - > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > if (!res) > return -EINVAL; > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > if (ret < 0) > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > - if (!ep_func) > - return -ENOMEM; > - > - ep_func->func_no = func_no; > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSI); > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSIX); > - > - list_add_tail(&ep_func->list, &ep->func_list); > - } > - > if (ep->ops->ep_init) > ep->ops->ep_init(ep); > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return 0; > } > > + /* > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > + * step as platforms that implement 'core_init_notifier' feature may > + * not have the hardware ready (i.e. core initialized) for access > + * (Ex: tegra194). Any hardware access on such platforms result > + * in system hard hang. > + */ > + > return dw_pcie_ep_init_complete(ep); > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); >
[+cc Serge] On Thu, Jul 07, 2022 at 02:39:08PM +0530, Vidya Sagar wrote: > Hi, > Anyone has review comments for this change? > Without this change, Tegra194's endpoint mode is effectively broken. Did Tegra194 endpoint mode ever work, or has this always been broken. Wondering if there's a Fixes: or stable tag we should add. Also cc'd Serge since he's been doing similar work on dwc. > On 6/22/2022 9:31 AM, Vidya Sagar wrote: > > Platforms that cannot support their core initialization without the > > reference clock from the host, implement the feature 'core_init_notifier' > > to indicate the DesignWare sub-system about when their core is getting > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > core to dw_pcie_ep_init_complete() API which is effectively called only > > after the core initialization. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 0eda8236c125..9feec720175f 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -639,9 +639,14 @@ 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; > > unsigned int nbars; > > u8 hdr_type; > > + u8 func_no; > > + void *addr; > > u32 reg; > > int i; > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > return -EIO; > > } > > + 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; > > + > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > + if (!ep_func) > > + return -ENOMEM; > > + > > + ep_func->func_no = func_no; > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSI); > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSIX); > > + > > + list_add_tail(&ep_func->list, &ep->func_list); > > + } > > + > > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > dw_pcie_dbi_ro_wr_en(pci); > > @@ -677,8 +718,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 +725,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); > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > } > > } > > - dw_pcie_iatu_detect(pci); > > - > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > if (!res) > > return -EINVAL; > > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > if (ret < 0) > > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > - if (!ep_func) > > - return -ENOMEM; > > - > > - ep_func->func_no = func_no; > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSI); > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSIX); > > - > > - list_add_tail(&ep_func->list, &ep->func_list); > > - } > > - > > if (ep->ops->ep_init) > > ep->ops->ep_init(ep); > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > return 0; > > } > > + /* > > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > > + * step as platforms that implement 'core_init_notifier' feature may > > + * not have the hardware ready (i.e. core initialized) for access > > + * (Ex: tegra194). Any hardware access on such platforms result > > + * in system hard hang. > > + */ > > + > > return dw_pcie_ep_init_complete(ep); > > } > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > >
On 7/7/2022 9:30 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > [+cc Serge] > > On Thu, Jul 07, 2022 at 02:39:08PM +0530, Vidya Sagar wrote: >> Hi, >> Anyone has review comments for this change? >> Without this change, Tegra194's endpoint mode is effectively broken. > > Did Tegra194 endpoint mode ever work, or has this always been broken. > Wondering if there's a Fixes: or stable tag we should add. Also cc'd > Serge since he's been doing similar work on dwc. Yes. The endpoint mode of Tegra194 did work particularly after the commit ac37dde72177 ("PCI: dwc: Add API to notify core initialization completion") There are many changes that got merged after this and hence couldn't really point to one change to quote in Fixes: tag. Thanks & Regards, Vidya Sagar > >> On 6/22/2022 9:31 AM, Vidya Sagar wrote: >>> Platforms that cannot support their core initialization without the >>> reference clock from the host, implement the feature 'core_init_notifier' >>> to indicate the DesignWare sub-system about when their core is getting >>> initialized. Any accesses to the core (Ex:- DBI) would result in system >>> hang in such systems (Ex:- tegra194). This patch moves any access to the >>> core to dw_pcie_ep_init_complete() API which is effectively called only >>> after the core initialization. >>> >>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>> --- >>> .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- >>> 1 file changed, 49 insertions(+), 39 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> index 0eda8236c125..9feec720175f 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> @@ -639,9 +639,14 @@ 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; >>> unsigned int nbars; >>> u8 hdr_type; >>> + u8 func_no; >>> + void *addr; >>> u32 reg; >>> int i; >>> @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >>> return -EIO; >>> } >>> + 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; >>> + >>> + for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> + if (!ep_func) >>> + return -ENOMEM; >>> + >>> + ep_func->func_no = func_no; >>> + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSI); >>> + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSIX); >>> + >>> + list_add_tail(&ep_func->list, &ep->func_list); >>> + } >>> + >>> offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); >>> dw_pcie_dbi_ro_wr_en(pci); >>> @@ -677,8 +718,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 +725,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); >>> @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> } >>> } >>> - dw_pcie_iatu_detect(pci); >>> - >>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); >>> if (!res) >>> return -EINVAL; >>> @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> if (ret < 0) >>> epc->max_functions = 1; >>> - for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> - if (!ep_func) >>> - return -ENOMEM; >>> - >>> - ep_func->func_no = func_no; >>> - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSI); >>> - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSIX); >>> - >>> - list_add_tail(&ep_func->list, &ep->func_list); >>> - } >>> - >>> if (ep->ops->ep_init) >>> ep->ops->ep_init(ep); >>> @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> return 0; >>> } >>> + /* >>> + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this >>> + * step as platforms that implement 'core_init_notifier' feature may >>> + * not have the hardware ready (i.e. core initialized) for access >>> + * (Ex: tegra194). Any hardware access on such platforms result >>> + * in system hard hang. >>> + */ >>> + >>> return dw_pcie_ep_init_complete(ep); >>> } >>> EXPORT_SYMBOL_GPL(dw_pcie_ep_init); >>>
Hi, On Thu, Jul 07, 2022 at 02:39:08PM +0530, Vidya Sagar wrote: > Hi, > Anyone has review comments for this change? > Without this change, Tegra194's endpoint mode is effectively broken. > I submitted a patch that fixes this issue in March and I did CC you on that. https://lore.kernel.org/lkml/20220330060515.22328-1-manivannan.sadhasivam@linaro.org/ That patch incorporated comments from Kishon on the previous patch from Kunihiko Hayashi. But there was no response afterwards. Thanks, Mani > Thanks & Regards, > Vidya Sagar > > On 6/22/2022 9:31 AM, Vidya Sagar wrote: > > Platforms that cannot support their core initialization without the > > reference clock from the host, implement the feature 'core_init_notifier' > > to indicate the DesignWare sub-system about when their core is getting > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > core to dw_pcie_ep_init_complete() API which is effectively called only > > after the core initialization. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 0eda8236c125..9feec720175f 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -639,9 +639,14 @@ 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; > > unsigned int nbars; > > u8 hdr_type; > > + u8 func_no; > > + void *addr; > > u32 reg; > > int i; > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > return -EIO; > > } > > + 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; > > + > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > + if (!ep_func) > > + return -ENOMEM; > > + > > + ep_func->func_no = func_no; > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSI); > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSIX); > > + > > + list_add_tail(&ep_func->list, &ep->func_list); > > + } > > + > > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > dw_pcie_dbi_ro_wr_en(pci); > > @@ -677,8 +718,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 +725,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); > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > } > > } > > - dw_pcie_iatu_detect(pci); > > - > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > if (!res) > > return -EINVAL; > > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > if (ret < 0) > > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > - if (!ep_func) > > - return -ENOMEM; > > - > > - ep_func->func_no = func_no; > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSI); > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSIX); > > - > > - list_add_tail(&ep_func->list, &ep->func_list); > > - } > > - > > if (ep->ops->ep_init) > > ep->ops->ep_init(ep); > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > return 0; > > } > > + /* > > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > > + * step as platforms that implement 'core_init_notifier' feature may > > + * not have the hardware ready (i.e. core initialized) for access > > + * (Ex: tegra194). Any hardware access on such platforms result > > + * in system hard hang. > > + */ > > + > > return dw_pcie_ep_init_complete(ep); > > } > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > >
On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > Platforms that cannot support their core initialization without the > reference clock from the host, implement the feature 'core_init_notifier' > to indicate the DesignWare sub-system about when their core is getting > initialized. Any accesses to the core (Ex:- DBI) would result in system > hang in such systems (Ex:- tegra194). This patch moves any access to the > core to dw_pcie_ep_init_complete() API which is effectively called only > after the core initialization. I assume this is still broken. I want to fix it. I assume this patch fixes it and there are no known problems with it. I assume this can be fixed so it works on all platforms, whether they use core_init_notifier or not. I'd like the commit log to be specific about where the hang occurs so it's easy for a non-DesignWare expert (me!) to see the problem. E.g., on tegra194, X depends on Y, but Y is initialized after X. Say specifically what functions X and Y are. > --- > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > 1 file changed, 49 insertions(+), 39 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 0eda8236c125..9feec720175f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -639,9 +639,14 @@ 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; > unsigned int nbars; > u8 hdr_type; > + u8 func_no; > + void *addr; > u32 reg; > int i; > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > return -EIO; > } > > + 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; > + > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > + if (!ep_func) > + return -ENOMEM; > + > + ep_func->func_no = func_no; > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSI); > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSIX); > + > + list_add_tail(&ep_func->list, &ep->func_list); > + } > + > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > dw_pcie_dbi_ro_wr_en(pci); > @@ -677,8 +718,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 +725,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); > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > } > } > > - dw_pcie_iatu_detect(pci); > - > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > if (!res) > return -EINVAL; > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > if (ret < 0) > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > - if (!ep_func) > - return -ENOMEM; > - > - ep_func->func_no = func_no; > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSI); > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSIX); > - > - list_add_tail(&ep_func->list, &ep->func_list); > - } > - > if (ep->ops->ep_init) > ep->ops->ep_init(ep); > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return 0; > } > > + /* > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > + * step as platforms that implement 'core_init_notifier' feature may > + * not have the hardware ready (i.e. core initialized) for access > + * (Ex: tegra194). Any hardware access on such platforms result > + * in system hard hang. > + */ > + > return dw_pcie_ep_init_complete(ep); > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > -- > 2.17.1 >
On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: >> Platforms that cannot support their core initialization without the >> reference clock from the host, implement the feature 'core_init_notifier' >> to indicate the DesignWare sub-system about when their core is getting >> initialized. Any accesses to the core (Ex:- DBI) would result in system >> hang in such systems (Ex:- tegra194). This patch moves any access to the >> core to dw_pcie_ep_init_complete() API which is effectively called only >> after the core initialization. > > I assume this is still broken. I want to fix it. I assume this patch > fixes it and there are no known problems with it. I assume this can > be fixed so it works on all platforms, whether they use > core_init_notifier or not. Yes. All your assumptions are correct. > > I'd like the commit log to be specific about where the hang occurs so > it's easy for a non-DesignWare expert (me!) to see the problem. E.g., > on tegra194, X depends on Y, but Y is initialized after X. Say > specifically what functions X and Y are. X = DBI accesses Y = Core initialization which in turn depends on the REFCLK from the host Without this patch, hang happens when DBI registers are accessed without core being initialized. In the case of Tegra194 at least, core gets initialized only after REFCLK is available from the host. The way we make sure that the REFCLK is available from the host is by checking for PERST# de-assertion interrupt. (PCIe spec mandates that the host must supply REFCLK before de-asserting PERST# signal). This patch prevents any accesses to the DBI/Core registers if the platform says that it supports core_init_notifier. Thanks, Vidya Sagar > >> --- >> .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- >> 1 file changed, 49 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c >> index 0eda8236c125..9feec720175f 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c >> @@ -639,9 +639,14 @@ 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; >> unsigned int nbars; >> u8 hdr_type; >> + u8 func_no; >> + void *addr; >> u32 reg; >> int i; >> >> @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >> return -EIO; >> } >> >> + 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; >> + >> + for (func_no = 0; func_no < epc->max_functions; func_no++) { >> + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >> + if (!ep_func) >> + return -ENOMEM; >> + >> + ep_func->func_no = func_no; >> + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >> + PCI_CAP_ID_MSI); >> + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >> + PCI_CAP_ID_MSIX); >> + >> + list_add_tail(&ep_func->list, &ep->func_list); >> + } >> + >> offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); >> >> dw_pcie_dbi_ro_wr_en(pci); >> @@ -677,8 +718,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 +725,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); >> >> @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >> } >> } >> >> - dw_pcie_iatu_detect(pci); >> - >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); >> if (!res) >> return -EINVAL; >> @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >> if (ret < 0) >> epc->max_functions = 1; >> >> - for (func_no = 0; func_no < epc->max_functions; func_no++) { >> - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >> - if (!ep_func) >> - return -ENOMEM; >> - >> - ep_func->func_no = func_no; >> - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >> - PCI_CAP_ID_MSI); >> - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >> - PCI_CAP_ID_MSIX); >> - >> - list_add_tail(&ep_func->list, &ep->func_list); >> - } >> - >> if (ep->ops->ep_init) >> ep->ops->ep_init(ep); >> >> @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >> return 0; >> } >> >> + /* >> + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this >> + * step as platforms that implement 'core_init_notifier' feature may >> + * not have the hardware ready (i.e. core initialized) for access >> + * (Ex: tegra194). Any hardware access on such platforms result >> + * in system hard hang. >> + */ >> + >> return dw_pcie_ep_init_complete(ep); >> } >> EXPORT_SYMBOL_GPL(dw_pcie_ep_init); >> -- >> 2.17.1 >>
On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > External email: Use caution opening links or attachments > > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > Platforms that cannot support their core initialization without the > > > reference clock from the host, implement the feature 'core_init_notifier' > > > to indicate the DesignWare sub-system about when their core is getting > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > after the core initialization. > > > > I assume this is still broken. I want to fix it. I assume this patch > > fixes it and there are no known problems with it. I assume this can > > be fixed so it works on all platforms, whether they use > > core_init_notifier or not. > Yes. All your assumptions are correct. > > > > > I'd like the commit log to be specific about where the hang occurs so > > it's easy for a non-DesignWare expert (me!) to see the problem. E.g., > > on tegra194, X depends on Y, but Y is initialized after X. Say > > specifically what functions X and Y are. > X = DBI accesses > Y = Core initialization which in turn depends on the REFCLK from the host > > Without this patch, hang happens when DBI registers are accessed without > core being initialized. In the case of Tegra194 at least, core gets > initialized only after REFCLK is available from the host. The way we make > sure that the REFCLK is available from the host is by checking for PERST# > de-assertion interrupt. (PCIe spec mandates that the host must supply REFCLK > before de-asserting PERST# signal). > This patch prevents any accesses to the DBI/Core registers if the platform > says that it supports core_init_notifier. That's basically what the commit log already said. Can you give me actual function names as a hint for where to look? Bjorn
[+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > Platforms that cannot support their core initialization without the > > > reference clock from the host, implement the feature 'core_init_notifier' > > > to indicate the DesignWare sub-system about when their core is getting > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > after the core initialization. > > > > I assume this is still broken. I want to fix it. I assume this patch > > fixes it and there are no known problems with it. I assume this can > > be fixed so it works on all platforms, whether they use > > core_init_notifier or not. > > Yes. All your assumptions are correct. > > > I'd like the commit log to be specific about where the hang occurs so > > it's easy for a non-DesignWare expert (me!) to see the problem. E.g., > > on tegra194, X depends on Y, but Y is initialized after X. Say > > specifically what functions X and Y are. > > X = DBI accesses > Y = Core initialization which in turn depends on the REFCLK from the host > > Without this patch, hang happens when DBI registers are accessed without > core being initialized. In the case of Tegra194 at least, core gets > initialized only after REFCLK is available from the host. The way we make > sure that the REFCLK is available from the host is by checking for PERST# > de-assertion interrupt. (PCIe spec mandates that the host must supply REFCLK > before de-asserting PERST# signal). > This patch prevents any accesses to the DBI/Core registers if the platform > says that it supports core_init_notifier. There are several commits involved in this, oldest to newest: e966f7390da9 ("PCI: dwc: Refactor core initialization code for EP mode") Split EP initialization into dw_pcie_ep_init() (which doesn't touch core registers) and dw_pcie_ep_init_complete() (which does). dw_pcie_ep_init of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows) ep->ib_window_map = devm_kcalloc(..., ep->num_ib_windows, ...) dw_pcie_ep_init_complete # core regs are available dw_pcie_readb_dbi # <-- read core regs 6bfc9c3a2c70 ("PCI: designware-ep: Move the function of getting MSI capability forward") Move MSI capability lookup from dw_pcie_ep_init_complete() to dw_pcie_ep_init(). dw_pcie_ep_init dw_pcie_find_capability(pci, PCI_CAP_ID_MSI) dw_pcie_readw_dbi # <-- read core regs dw_pcie_ep_init_complete # core regs are available This is broken because we access the DBI registers before the core is initialized. 281f1f99cf3a ("PCI: dwc: Detect number of iATU windows") Read num_ib_windows from iATU registers instead of from DT property. dw_pcie_ep_init ib_window_map = devm_kcalloc(..., pci->num_ib_windows, ...) # use dw_pcie_ep_init_complete dw_pcie_setup dw_pcie_iatu_detect_regions dw_pcie_readl_dbi pci->num_ib_windows = ib # init This ordering is broken because we use pci->num_ib_windows before we initialize it. 8bcca2658558 ("PCI: dwc: Move iATU detection earlier") Fix the use-before-init problem by moving init earlier: dw_pcie_ep_init dw_pcie_iatu_detect dw_pcie_readl_dbi # <-- read core regs pci->num_ib_windows = ib # init ib_window_map = devm_kcalloc(..., pci->num_ib_windows, ...) # use dw_pcie_ep_init_complete # core regs are available dw_pcie_setup This fixed the use-before-init, but it causes the hang by accessing the DBI registers before the core is initialized. This patch addresses the problems of 6bfc9c3a2c70 and 8bcca2658558 by moving dw_pcie_iatu_detect() and dw_pcie_ep_find_capability() from dw_pcie_ep_init() to dw_pcie_ep_init_complete(). We probably need Fixes: tags for both. But I don't think this patch is enough, and I have a lot of questions: 1) The "core_init_notifier" mechanism is kind of obtuse. IIUC, any driver that sets ".core_init_notifier = true" is responsible for calling dw_pcie_ep_init_complete() at some point. But that's not explicit at all in the code. There's no "notifier" involved. I suppose the idea is that the *interrupt* is the notifier, but the naming doesn't help connect things. 2) I still want to know exactly where (what function) the critical transition that enables DBI access is. I guess it's qcom_pcie_perst_deassert() for qcom and pex_ep_event_pex_rst_deassert() for tegra? And for drivers that don't set .core_init_notifier, DBI is accessible any time in dw_pcie_ep_init() or later? 3) How do these interrupt handlers work? I'm used to the model of: Install interrupt handler Do something to the device so it will generate an interrupt Later, the device generates the interrupt Execute the interrupt handler We register tegra_pcie_ep_pex_rst_irq() as an interrupt handler, but what touches the device and starts the process that causes the interrupt? 4) 6bfc9c3a2c70 moved the MSI capability lookup to dw_pcie_ep_init() because .ep_init() functions, e.g., ls_pcie_ep_init(), depend on ep_func->msi_cap. Moving the MSI lookup to dw_pcie_ep_init_complete() will break that again, won't it? 5) dw_pcie_ep_init() calls ep->ops->ep_init(ep), and almost all of those .ep_init() methods, e.g., dra7xx_pcie_ep_init(), ls_pcie_ep_init(), etc., call dw_pcie_ep_reset_bar(), which does DBI writes. These are before dw_pcie_ep_init_complete(), so they are broken per the design (though they probably *work* because the drivers don't set .core_init_notifier, so DBI accesses probably work earlier). The only .ep_init() in a driver that sets .core_init_notifier is qcom_pcie_ep_init(). That *looks* like it should actually be broken because it runs before dw_pcie_ep_init_complete(). 6) What's going on with the CORE_INIT and LINK_UP notifiers? dw_pcie_ep_init_notify() is only called by qcom and tegra. dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. As far as I can tell, nobody at all registers to handle those events except a test. I think it's pointless to have that code if nobody uses it. > > > --- > > > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > index 0eda8236c125..9feec720175f 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > @@ -639,9 +639,14 @@ 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; > > > unsigned int nbars; > > > u8 hdr_type; > > > + u8 func_no; > > > + void *addr; > > > u32 reg; > > > int i; > > > > > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > return -EIO; > > > } > > > > > > + 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; > > > + > > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > + if (!ep_func) > > > + return -ENOMEM; > > > + > > > + ep_func->func_no = func_no; > > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > + PCI_CAP_ID_MSI); > > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > + PCI_CAP_ID_MSIX); > > > + > > > + list_add_tail(&ep_func->list, &ep->func_list); > > > + } > > > + > > > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > > > > > dw_pcie_dbi_ro_wr_en(pci); > > > @@ -677,8 +718,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 +725,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); > > > > > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > } > > > } > > > > > > - dw_pcie_iatu_detect(pci); > > > - > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > > if (!res) > > > return -EINVAL; > > > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > if (ret < 0) > > > epc->max_functions = 1; > > > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > - if (!ep_func) > > > - return -ENOMEM; > > > - > > > - ep_func->func_no = func_no; > > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > - PCI_CAP_ID_MSI); > > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > - PCI_CAP_ID_MSIX); > > > - > > > - list_add_tail(&ep_func->list, &ep->func_list); > > > - } > > > - > > > if (ep->ops->ep_init) > > > ep->ops->ep_init(ep); > > > > > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > return 0; > > > } > > > > > > + /* > > > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > > > + * step as platforms that implement 'core_init_notifier' feature may > > > + * not have the hardware ready (i.e. core initialized) for access > > > + * (Ex: tegra194). Any hardware access on such platforms result > > > + * in system hard hang. > > > + */ > > > + > > > return dw_pcie_ep_init_complete(ep); > > > } > > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > > -- > > > 2.17.1 > > >
On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > Platforms that cannot support their core initialization without the > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > to indicate the DesignWare sub-system about when their core is getting > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > after the core initialization. > > > > > > I assume this is still broken. I want to fix it. I assume this patch > > > fixes it and there are no known problems with it. I assume this can > > > be fixed so it works on all platforms, whether they use > > > core_init_notifier or not. > > > > Yes. All your assumptions are correct. > > > > > I'd like the commit log to be specific about where the hang occurs so > > > it's easy for a non-DesignWare expert (me!) to see the problem. E.g., > > > on tegra194, X depends on Y, but Y is initialized after X. Say > > > specifically what functions X and Y are. > > > > X = DBI accesses > > Y = Core initialization which in turn depends on the REFCLK from the host > > > > Without this patch, hang happens when DBI registers are accessed without > > core being initialized. In the case of Tegra194 at least, core gets > > initialized only after REFCLK is available from the host. The way we make > > sure that the REFCLK is available from the host is by checking for PERST# > > de-assertion interrupt. (PCIe spec mandates that the host must supply REFCLK > > before de-asserting PERST# signal). > > This patch prevents any accesses to the DBI/Core registers if the platform > > says that it supports core_init_notifier. > > There are several commits involved in this, oldest to newest: > > e966f7390da9 ("PCI: dwc: Refactor core initialization code for EP mode") > Split EP initialization into dw_pcie_ep_init() (which doesn't > touch core registers) and dw_pcie_ep_init_complete() (which does). > > dw_pcie_ep_init > of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows) > ep->ib_window_map = devm_kcalloc(..., ep->num_ib_windows, ...) > dw_pcie_ep_init_complete # core regs are available > dw_pcie_readb_dbi # <-- read core regs > > 6bfc9c3a2c70 ("PCI: designware-ep: Move the function of getting MSI capability forward") > Move MSI capability lookup from dw_pcie_ep_init_complete() to > dw_pcie_ep_init(). > > dw_pcie_ep_init > dw_pcie_find_capability(pci, PCI_CAP_ID_MSI) > dw_pcie_readw_dbi # <-- read core regs > dw_pcie_ep_init_complete # core regs are available > > This is broken because we access the DBI registers before the core > is initialized. > > 281f1f99cf3a ("PCI: dwc: Detect number of iATU windows") > Read num_ib_windows from iATU registers instead of from DT > property. > > dw_pcie_ep_init > ib_window_map = devm_kcalloc(..., pci->num_ib_windows, ...) # use > dw_pcie_ep_init_complete > dw_pcie_setup > dw_pcie_iatu_detect_regions > dw_pcie_readl_dbi > pci->num_ib_windows = ib # init > > This ordering is broken because we use pci->num_ib_windows before > we initialize it. > > 8bcca2658558 ("PCI: dwc: Move iATU detection earlier") > Fix the use-before-init problem by moving init earlier: > > dw_pcie_ep_init > dw_pcie_iatu_detect > dw_pcie_readl_dbi # <-- read core regs > pci->num_ib_windows = ib # init > ib_window_map = devm_kcalloc(..., pci->num_ib_windows, ...) # use > dw_pcie_ep_init_complete # core regs are available > dw_pcie_setup > > This fixed the use-before-init, but it causes the hang by > accessing the DBI registers before the core is initialized. > > This patch addresses the problems of 6bfc9c3a2c70 and 8bcca2658558 by > moving dw_pcie_iatu_detect() and dw_pcie_ep_find_capability() from > dw_pcie_ep_init() to dw_pcie_ep_init_complete(). > > We probably need Fixes: tags for both. > That sounds right to me. > But I don't think this patch is enough, and I have a lot of questions: > > 1) The "core_init_notifier" mechanism is kind of obtuse. IIUC, any > driver that sets ".core_init_notifier = true" is responsible for > calling dw_pcie_ep_init_complete() at some point. But that's not > explicit at all in the code. There's no "notifier" involved. I > suppose the idea is that the *interrupt* is the notifier, but the > naming doesn't help connect things. The actual notifier is used *inside* the interrupt handler. dw_pcie_ep_init_notify(). This notifies the EPF drivers. LINK_UP notifier is used with dw_pcie_ep_linkup(). > > 2) I still want to know exactly where (what function) the critical > transition that enables DBI access is. I guess it's > qcom_pcie_perst_deassert() for qcom and > pex_ep_event_pex_rst_deassert() for tegra? And for drivers that > don't set .core_init_notifier, DBI is accessible any time in > dw_pcie_ep_init() or later? > On the Qcom platforms, qcom_pcie_enable_resources() is what enabling all resources required for DBI access. This function turns ON clocks, brings the endpoint IP block out of reset and turns ON the PHY. The last step (turning ON the PHY) requires active refclk from the host. So with mainline pcie-qcom-ep driver, the PCIe host should be active by the time the ep driver probes. Then only refclk will be active and phy_power_on() will succeed. Now, this creates a hard dependency with the PCIe host even for ep driver probing (remember the phy_power_on() gets called during probe time), and this order won't work if we'd like to use the pcie-qcom-ep driver in a standalone product like a PCIe modem, where the modem would power up independently of the PCIe host. And this is the reason, *I* wanted to move all the DBI access to dw_pcie_ep_init_complete(), so that the ep driver can probe successfully independent of the host and becomes operational when the PCIe host comes up and deasserts the PERST#. I have mentioned this in my version of the patch: https://lore.kernel.org/lkml/20220330060515.22328-1-manivannan.sadhasivam@linaro.org/ The reason why I decided to use core_init_notifier in the first place was, it ensures that the endpoint gets initialized only when the PCIe host de-asserts the PERST# signal. And this behaviour matches the PCIe spec. > 3) How do these interrupt handlers work? I'm used to the model of: > > Install interrupt handler > Do something to the device so it will generate an interrupt > Later, the device generates the interrupt > Execute the interrupt handler > > We register tegra_pcie_ep_pex_rst_irq() as an interrupt handler, > but what touches the device and starts the process that causes > the interrupt? > > 4) 6bfc9c3a2c70 moved the MSI capability lookup to dw_pcie_ep_init() > because .ep_init() functions, e.g., ls_pcie_ep_init(), depend on > ep_func->msi_cap. Moving the MSI lookup to > dw_pcie_ep_init_complete() will break that again, won't it? > Good catch! Yes, it will break the NXP driver. But the rest of the ep drivers set the msi{x}_capable flags statically and this information should be known beforehand IMO. > 5) dw_pcie_ep_init() calls ep->ops->ep_init(ep), and almost all of > those .ep_init() methods, e.g., dra7xx_pcie_ep_init(), > ls_pcie_ep_init(), etc., call dw_pcie_ep_reset_bar(), which does > DBI writes. These are before dw_pcie_ep_init_complete(), so the > are broken per the design (though they probably *work* because > the drivers don't set .core_init_notifier, so DBI accesses > probably work earlier). > > The only .ep_init() in a driver that sets .core_init_notifier is > qcom_pcie_ep_init(). That *looks* like it should actually be > broken because it runs before dw_pcie_ep_init_complete(). As per my above justification, it is not broken technically. > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > dw_pcie_ep_init_notify() is only called by qcom and tegra. > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > As far as I can tell, nobody at all registers to handle those > events except a test. I think it's pointless to have that code > if nobody uses it. > I have submitted an actual driver that makes use of these notifiers: https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ Thanks, Mani > > > > --- > > > > .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- > > > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > index 0eda8236c125..9feec720175f 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > @@ -639,9 +639,14 @@ 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; > > > > unsigned int nbars; > > > > u8 hdr_type; > > > > + u8 func_no; > > > > + void *addr; > > > > u32 reg; > > > > int i; > > > > > > > > @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > > return -EIO; > > > > } > > > > > > > > + 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; > > > > + > > > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > + if (!ep_func) > > > > + return -ENOMEM; > > > > + > > > > + ep_func->func_no = func_no; > > > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSI); > > > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSIX); > > > > + > > > > + list_add_tail(&ep_func->list, &ep->func_list); > > > > + } > > > > + > > > > offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); > > > > > > > > dw_pcie_dbi_ro_wr_en(pci); > > > > @@ -677,8 +718,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 +725,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); > > > > > > > > @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > } > > > > } > > > > > > > > - dw_pcie_iatu_detect(pci); > > > > - > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > > > if (!res) > > > > return -EINVAL; > > > > @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > if (ret < 0) > > > > epc->max_functions = 1; > > > > > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > - if (!ep_func) > > > > - return -ENOMEM; > > > > - > > > > - ep_func->func_no = func_no; > > > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSI); > > > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSIX); > > > > - > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > > > - } > > > > - > > > > if (ep->ops->ep_init) > > > > ep->ops->ep_init(ep); > > > > > > > > @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > return 0; > > > > } > > > > > > > > + /* > > > > + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > > > > + * step as platforms that implement 'core_init_notifier' feature may > > > > + * not have the hardware ready (i.e. core initialized) for access > > > > + * (Ex: tegra194). Any hardware access on such platforms result > > > > + * in system hard hang. > > > > + */ > > > > + > > > > return dw_pcie_ep_init_complete(ep); > > > > } > > > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > > > -- > > > > 2.17.1 > > > >
On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > Platforms that cannot support their core initialization without the > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > after the core initialization. > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > As far as I can tell, nobody at all registers to handle those > > events except a test. I think it's pointless to have that code > > if nobody uses it. > > > > I have submitted an actual driver that makes use of these notifiers: > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ Notifiers aren't the best interface in the kernel. I think they are best used if there's no real linkage between the sender and receiver. For an EPC and EPF that's a fixed interface, so define a proper interface. Rob
On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: > On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > > Platforms that cannot support their core initialization without the > > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > > after the core initialization. > > > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > > As far as I can tell, nobody at all registers to handle those > > > events except a test. I think it's pointless to have that code > > > if nobody uses it. > > > > > > > I have submitted an actual driver that makes use of these notifiers: > > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ > > Notifiers aren't the best interface in the kernel. I think they are > best used if there's no real linkage between the sender and receiver. > For an EPC and EPF that's a fixed interface, so define a proper > interface. > Fair point! The use of notifiers also suffer from an issue where the notifier chain in EPC is atomic but the EPF calls some of the functions like pci_epc_write_header() could potentially sleep. I'll try to come up with an interface. Thanks, Mani > Rob
On Tue, Aug 02, 2022 at 12:54:37PM +0530, Manivannan Sadhasivam wrote: > On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: > > On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > > > Platforms that cannot support their core initialization without the > > > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > > > after the core initialization. > > > > > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > > > As far as I can tell, nobody at all registers to handle those > > > > events except a test. I think it's pointless to have that code > > > > if nobody uses it. > > > > > > > > > > I have submitted an actual driver that makes use of these notifiers: > > > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ > > > > Notifiers aren't the best interface in the kernel. I think they are > > best used if there's no real linkage between the sender and receiver. > > For an EPC and EPF that's a fixed interface, so define a proper > > interface. > > > > Fair point! The use of notifiers also suffer from an issue where the notifier > chain in EPC is atomic but the EPF calls some of the functions like > pci_epc_write_header() could potentially sleep. > > I'll try to come up with an interface. > I thought about using a new set of callbacks that define the EPC events and have the EPF drivers populate them during probe time. Like below, ``` diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h index e03c57129ed5..45247802d6f7 100644 --- a/include/linux/pci-epf.h +++ b/include/linux/pci-epf.h @@ -74,6 +74,20 @@ struct pci_epf_ops { struct config_group *group); }; +/** + * struct pci_epf_events - Callbacks for capturing the EPC events + * @init_complete: Callback for the EPC initialization complete event + * @link_up: Callback for the EPC link up event + */ +struct pci_epc_events { + void (*init_complete)(struct pci_epf *epf); + void (*link_up)(struct pci_epf *epf); +}; + /** * struct pci_epf_driver - represents the PCI EPF driver * @probe: ops to perform when a new EPF device has been bound to the EPF driver @@ -172,6 +186,7 @@ struct pci_epf { unsigned int is_vf; unsigned long vfunction_num_map; struct list_head pci_vepf; + struct pci_epc_events *events; }; /** ``` When each of the event is received by the EPC driver, it will use the EPC API to call the relevant event callback for _each_ EPF. Like below: ``` diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c index 6ad9b38b63a9..4b0b30b91403 100644 --- a/drivers/pci/endpoint/pci-epc-core.c +++ b/drivers/pci/endpoint/pci-epc-core.c @@ -724,10 +724,15 @@ EXPORT_SYMBOL_GPL(pci_epc_linkdown); */ void pci_epc_init_notify(struct pci_epc *epc) { + struct pci_epf *epf; + if (!epc || IS_ERR(epc)) return; - blocking_notifier_call_chain(&epc->notifier, CORE_INIT, NULL); + list_for_each_entry(epf, &epc->pci_epf, list) { + if (epf->events->init_complete) + epf->events->init_complete(epf); + } } EXPORT_SYMBOL_GPL(pci_epc_init_notify); ``` Does this look good to you? I can spin up an RFC series, but wanted to check the interface design beforehand. Thanks, Mani > Thanks, > Mani > > > Rob > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Aug 02, 2022 at 07:37:38PM +0530, Manivannan Sadhasivam wrote: > On Tue, Aug 02, 2022 at 12:54:37PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: > > > On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam > > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > > > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > > > > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > > > > Platforms that cannot support their core initialization without the > > > > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > > > > after the core initialization. > > > > > > > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > > > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > > > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > > > > As far as I can tell, nobody at all registers to handle those > > > > > events except a test. I think it's pointless to have that code > > > > > if nobody uses it. > > > > > > > > > > > > > I have submitted an actual driver that makes use of these notifiers: > > > > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ > > > > > > Notifiers aren't the best interface in the kernel. I think they are > > > best used if there's no real linkage between the sender and receiver. > > > For an EPC and EPF that's a fixed interface, so define a proper > > > interface. > > > > > > > Fair point! The use of notifiers also suffer from an issue where the notifier > > chain in EPC is atomic but the EPF calls some of the functions like > > pci_epc_write_header() could potentially sleep. > > > > I'll try to come up with an interface. > > > > I thought about using a new set of callbacks that define the EPC events and > have the EPF drivers populate them during probe time. Like below, > > ``` > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h > index e03c57129ed5..45247802d6f7 100644 > --- a/include/linux/pci-epf.h > +++ b/include/linux/pci-epf.h > @@ -74,6 +74,20 @@ struct pci_epf_ops { > struct config_group *group); > }; > > +/** > + * struct pci_epf_events - Callbacks for capturing the EPC events > + * @init_complete: Callback for the EPC initialization complete event > + * @link_up: Callback for the EPC link up event > + */ > +struct pci_epc_events { pci_epc_event_ops > + void (*init_complete)(struct pci_epf *epf); > + void (*link_up)(struct pci_epf *epf); link down? > +}; > + > /** > * struct pci_epf_driver - represents the PCI EPF driver > * @probe: ops to perform when a new EPF device has been bound to the EPF driver > @@ -172,6 +186,7 @@ struct pci_epf { > unsigned int is_vf; > unsigned long vfunction_num_map; > struct list_head pci_vepf; > + struct pci_epc_events *events; *event_ops And 'const' > }; > > /** > ``` > > When each of the event is received by the EPC driver, it will use the EPC API > to call the relevant event callback for _each_ EPF. Like below: > > ``` > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > index 6ad9b38b63a9..4b0b30b91403 100644 > --- a/drivers/pci/endpoint/pci-epc-core.c > +++ b/drivers/pci/endpoint/pci-epc-core.c > @@ -724,10 +724,15 @@ EXPORT_SYMBOL_GPL(pci_epc_linkdown); > */ > void pci_epc_init_notify(struct pci_epc *epc) > { > + struct pci_epf *epf; > + > if (!epc || IS_ERR(epc)) > return; > > - blocking_notifier_call_chain(&epc->notifier, CORE_INIT, NULL); > + list_for_each_entry(epf, &epc->pci_epf, list) { > + if (epf->events->init_complete) > + epf->events->init_complete(epf); > + } > } > EXPORT_SYMBOL_GPL(pci_epc_init_notify); > ``` > > Does this look good to you? I can spin up an RFC series, but wanted to check the > interface design beforehand. I guess. Honestly, the I'm not all that familiar with the endpoint stuff. Rob
On Tue, Aug 02, 2022 at 07:37:38PM +0530, Manivannan Sadhasivam wrote: > On Tue, Aug 02, 2022 at 12:54:37PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: > > > On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam > > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > > > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > > > > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > > > > Platforms that cannot support their core initialization without the > > > > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > > > > after the core initialization. > > > > > > > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > > > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > > > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > > > > As far as I can tell, nobody at all registers to handle those > > > > > events except a test. I think it's pointless to have that code > > > > > if nobody uses it. > > > > > > > > > > > > > I have submitted an actual driver that makes use of these notifiers: > > > > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ > > > > > > Notifiers aren't the best interface in the kernel. I think they are > > > best used if there's no real linkage between the sender and receiver. > > > For an EPC and EPF that's a fixed interface, so define a proper > > > interface. > > > > > > > Fair point! The use of notifiers also suffer from an issue where the notifier > > chain in EPC is atomic but the EPF calls some of the functions like > > pci_epc_write_header() could potentially sleep. > > > > I'll try to come up with an interface. > > > > I thought about using a new set of callbacks that define the EPC events and > have the EPF drivers populate them during probe time. Like below, > > ``` > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h > index e03c57129ed5..45247802d6f7 100644 > --- a/include/linux/pci-epf.h > +++ b/include/linux/pci-epf.h > @@ -74,6 +74,20 @@ struct pci_epf_ops { > struct config_group *group); > }; > > +/** > + * struct pci_epf_events - Callbacks for capturing the EPC events > + * @init_complete: Callback for the EPC initialization complete event > + * @link_up: Callback for the EPC link up event > + */ > +struct pci_epc_events { > + void (*init_complete)(struct pci_epf *epf); > + void (*link_up)(struct pci_epf *epf); > +}; > + > /** > * struct pci_epf_driver - represents the PCI EPF driver > * @probe: ops to perform when a new EPF device has been bound to the EPF driver > @@ -172,6 +186,7 @@ struct pci_epf { > unsigned int is_vf; > unsigned long vfunction_num_map; > struct list_head pci_vepf; > + struct pci_epc_events *events; > }; > > /** > ``` > > When each of the event is received by the EPC driver, it will use the EPC API > to call the relevant event callback for _each_ EPF. Like below: > > ``` > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > index 6ad9b38b63a9..4b0b30b91403 100644 > --- a/drivers/pci/endpoint/pci-epc-core.c > +++ b/drivers/pci/endpoint/pci-epc-core.c > @@ -724,10 +724,15 @@ EXPORT_SYMBOL_GPL(pci_epc_linkdown); > */ > void pci_epc_init_notify(struct pci_epc *epc) > { > + struct pci_epf *epf; > + > if (!epc || IS_ERR(epc)) > return; > > - blocking_notifier_call_chain(&epc->notifier, CORE_INIT, NULL); > + list_for_each_entry(epf, &epc->pci_epf, list) { > + if (epf->events->init_complete) > + epf->events->init_complete(epf); > + } > } > EXPORT_SYMBOL_GPL(pci_epc_init_notify); > ``` > > Does this look good to you? I can spin up an RFC series, but wanted to check the > interface design beforehand. I am resuming patch reviews, have you posted a follow up ? Just to understand where we are with this thread and start reviewing from there, I will update patchwork accordingly (you should add a Link: to this thread anyway in the new series). Thanks, Lorenzo > Thanks, > Mani > > > Thanks, > > Mani > > > > > Rob > > > > -- > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
On 8/16/2022 7:45 PM, Lorenzo Pieralisi wrote: > External email: Use caution opening links or attachments > > > On Tue, Aug 02, 2022 at 07:37:38PM +0530, Manivannan Sadhasivam wrote: >> On Tue, Aug 02, 2022 at 12:54:37PM +0530, Manivannan Sadhasivam wrote: >>> On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: >>>> On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam >>>> <manivannan.sadhasivam@linaro.org> wrote: >>>>> >>>>> On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: >>>>>> [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] >>>>>> >>>>>> On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: >>>>>>> On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: >>>>>>>> On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: >>>>>>>>> Platforms that cannot support their core initialization without the >>>>>>>>> reference clock from the host, implement the feature 'core_init_notifier' >>>>>>>>> to indicate the DesignWare sub-system about when their core is getting >>>>>>>>> initialized. Any accesses to the core (Ex:- DBI) would result in system >>>>>>>>> hang in such systems (Ex:- tegra194). This patch moves any access to the >>>>>>>>> core to dw_pcie_ep_init_complete() API which is effectively called only >>>>>>>>> after the core initialization. >>>> >>>>>> 6) What's going on with the CORE_INIT and LINK_UP notifiers? >>>>>> dw_pcie_ep_init_notify() is only called by qcom and tegra. >>>>>> dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. >>>>>> As far as I can tell, nobody at all registers to handle those >>>>>> events except a test. I think it's pointless to have that code >>>>>> if nobody uses it. >>>>>> >>>>> >>>>> I have submitted an actual driver that makes use of these notifiers: >>>>> https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ >>>> >>>> Notifiers aren't the best interface in the kernel. I think they are >>>> best used if there's no real linkage between the sender and receiver. >>>> For an EPC and EPF that's a fixed interface, so define a proper >>>> interface. >>>> >>> >>> Fair point! The use of notifiers also suffer from an issue where the notifier >>> chain in EPC is atomic but the EPF calls some of the functions like >>> pci_epc_write_header() could potentially sleep. >>> >>> I'll try to come up with an interface. >>> >> >> I thought about using a new set of callbacks that define the EPC events and >> have the EPF drivers populate them during probe time. Like below, >> >> ``` >> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h >> index e03c57129ed5..45247802d6f7 100644 >> --- a/include/linux/pci-epf.h >> +++ b/include/linux/pci-epf.h >> @@ -74,6 +74,20 @@ struct pci_epf_ops { >> struct config_group *group); >> }; >> >> +/** >> + * struct pci_epf_events - Callbacks for capturing the EPC events >> + * @init_complete: Callback for the EPC initialization complete event >> + * @link_up: Callback for the EPC link up event >> + */ >> +struct pci_epc_events { >> + void (*init_complete)(struct pci_epf *epf); >> + void (*link_up)(struct pci_epf *epf); >> +}; >> + >> /** >> * struct pci_epf_driver - represents the PCI EPF driver >> * @probe: ops to perform when a new EPF device has been bound to the EPF driver >> @@ -172,6 +186,7 @@ struct pci_epf { >> unsigned int is_vf; >> unsigned long vfunction_num_map; >> struct list_head pci_vepf; >> + struct pci_epc_events *events; >> }; >> >> /** >> ``` >> >> When each of the event is received by the EPC driver, it will use the EPC API >> to call the relevant event callback for _each_ EPF. Like below: >> >> ``` >> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c >> index 6ad9b38b63a9..4b0b30b91403 100644 >> --- a/drivers/pci/endpoint/pci-epc-core.c >> +++ b/drivers/pci/endpoint/pci-epc-core.c >> @@ -724,10 +724,15 @@ EXPORT_SYMBOL_GPL(pci_epc_linkdown); >> */ >> void pci_epc_init_notify(struct pci_epc *epc) >> { >> + struct pci_epf *epf; >> + >> if (!epc || IS_ERR(epc)) >> return; >> >> - blocking_notifier_call_chain(&epc->notifier, CORE_INIT, NULL); >> + list_for_each_entry(epf, &epc->pci_epf, list) { >> + if (epf->events->init_complete) >> + epf->events->init_complete(epf); >> + } >> } >> EXPORT_SYMBOL_GPL(pci_epc_init_notify); >> ``` >> >> Does this look good to you? I can spin up an RFC series, but wanted to check the >> interface design beforehand. > > I am resuming patch reviews, have you posted a follow up ? > > Just to understand where we are with this thread and start reviewing > from there, I will update patchwork accordingly (you should add > a Link: to this thread anyway in the new series). Manivannan posted a new patch set at https://patchwork.kernel.org/project/linux-pci/list/?series=666818 to address concerns with the notifier mechanism. I would be sending a follow-up patch for the current patch soon. Thanks, Vidya Sagar > > Thanks, > Lorenzo > >> Thanks, >> Mani >> >>> Thanks, >>> Mani >>> >>>> Rob >>> >>> -- >>> மணிவண்ணன் சதாசிவம் >> >> -- >> மணிவண்ணன் சதாசிவம்
On Tue, Aug 16, 2022 at 08:05:08PM +0530, Vidya Sagar wrote: > > > On 8/16/2022 7:45 PM, Lorenzo Pieralisi wrote: > > External email: Use caution opening links or attachments > > > > > > On Tue, Aug 02, 2022 at 07:37:38PM +0530, Manivannan Sadhasivam wrote: > > > On Tue, Aug 02, 2022 at 12:54:37PM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Aug 01, 2022 at 02:27:14PM -0600, Rob Herring wrote: > > > > > On Sat, Jul 30, 2022 at 8:50 AM Manivannan Sadhasivam > > > > > <manivannan.sadhasivam@linaro.org> wrote: > > > > > > > > > > > > On Fri, Jul 29, 2022 at 05:44:04PM -0500, Bjorn Helgaas wrote: > > > > > > > [+cc Xiaowei (author of 6bfc9c3a2c70), Hou (author of 8bcca2658558)] > > > > > > > > > > > > > > On Thu, Jul 28, 2022 at 05:56:28PM +0530, Vidya Sagar wrote: > > > > > > > > On 7/28/2022 3:44 AM, Bjorn Helgaas wrote: > > > > > > > > > On Wed, Jun 22, 2022 at 09:31:33AM +0530, Vidya Sagar wrote: > > > > > > > > > > Platforms that cannot support their core initialization without the > > > > > > > > > > reference clock from the host, implement the feature 'core_init_notifier' > > > > > > > > > > to indicate the DesignWare sub-system about when their core is getting > > > > > > > > > > initialized. Any accesses to the core (Ex:- DBI) would result in system > > > > > > > > > > hang in such systems (Ex:- tegra194). This patch moves any access to the > > > > > > > > > > core to dw_pcie_ep_init_complete() API which is effectively called only > > > > > > > > > > after the core initialization. > > > > > > > > > > > > 6) What's going on with the CORE_INIT and LINK_UP notifiers? > > > > > > > dw_pcie_ep_init_notify() is only called by qcom and tegra. > > > > > > > dw_pcie_ep_linkup() is only called by dra7xx, qcom, and tegra. > > > > > > > As far as I can tell, nobody at all registers to handle those > > > > > > > events except a test. I think it's pointless to have that code > > > > > > > if nobody uses it. > > > > > > > > > > > > > > > > > > > I have submitted an actual driver that makes use of these notifiers: > > > > > > https://lore.kernel.org/lkml/20220502060611.58987-9-manivannan.sadhasivam@linaro.org/ > > > > > > > > > > Notifiers aren't the best interface in the kernel. I think they are > > > > > best used if there's no real linkage between the sender and receiver. > > > > > For an EPC and EPF that's a fixed interface, so define a proper > > > > > interface. > > > > > > > > > > > > > Fair point! The use of notifiers also suffer from an issue where the notifier > > > > chain in EPC is atomic but the EPF calls some of the functions like > > > > pci_epc_write_header() could potentially sleep. > > > > > > > > I'll try to come up with an interface. > > > > > > > > > > I thought about using a new set of callbacks that define the EPC events and > > > have the EPF drivers populate them during probe time. Like below, > > > > > > ``` > > > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h > > > index e03c57129ed5..45247802d6f7 100644 > > > --- a/include/linux/pci-epf.h > > > +++ b/include/linux/pci-epf.h > > > @@ -74,6 +74,20 @@ struct pci_epf_ops { > > > struct config_group *group); > > > }; > > > > > > +/** > > > + * struct pci_epf_events - Callbacks for capturing the EPC events > > > + * @init_complete: Callback for the EPC initialization complete event > > > + * @link_up: Callback for the EPC link up event > > > + */ > > > +struct pci_epc_events { > > > + void (*init_complete)(struct pci_epf *epf); > > > + void (*link_up)(struct pci_epf *epf); > > > +}; > > > + > > > /** > > > * struct pci_epf_driver - represents the PCI EPF driver > > > * @probe: ops to perform when a new EPF device has been bound to the EPF driver > > > @@ -172,6 +186,7 @@ struct pci_epf { > > > unsigned int is_vf; > > > unsigned long vfunction_num_map; > > > struct list_head pci_vepf; > > > + struct pci_epc_events *events; > > > }; > > > > > > /** > > > ``` > > > > > > When each of the event is received by the EPC driver, it will use the EPC API > > > to call the relevant event callback for _each_ EPF. Like below: > > > > > > ``` > > > diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c > > > index 6ad9b38b63a9..4b0b30b91403 100644 > > > --- a/drivers/pci/endpoint/pci-epc-core.c > > > +++ b/drivers/pci/endpoint/pci-epc-core.c > > > @@ -724,10 +724,15 @@ EXPORT_SYMBOL_GPL(pci_epc_linkdown); > > > */ > > > void pci_epc_init_notify(struct pci_epc *epc) > > > { > > > + struct pci_epf *epf; > > > + > > > if (!epc || IS_ERR(epc)) > > > return; > > > > > > - blocking_notifier_call_chain(&epc->notifier, CORE_INIT, NULL); > > > + list_for_each_entry(epf, &epc->pci_epf, list) { > > > + if (epf->events->init_complete) > > > + epf->events->init_complete(epf); > > > + } > > > } > > > EXPORT_SYMBOL_GPL(pci_epc_init_notify); > > > ``` > > > > > > Does this look good to you? I can spin up an RFC series, but wanted to check the > > > interface design beforehand. > > > > I am resuming patch reviews, have you posted a follow up ? > > > > Just to understand where we are with this thread and start reviewing > > from there, I will update patchwork accordingly (you should add > > a Link: to this thread anyway in the new series). > > Manivannan posted a new patch set at > https://patchwork.kernel.org/project/linux-pci/list/?series=666818 to > address concerns with the notifier mechanism. > > I would be sending a follow-up patch for the current patch soon. > While posting the new revision, please move dw_pcie_edma_detect() and ep_init() to init_complete function. They also perform DBI access. Thanks, Mani > Thanks, > Vidya Sagar > > > > > Thanks, > > Lorenzo > > > > > Thanks, > > > Mani > > > > > > > Thanks, > > > > Mani > > > > > > > > > Rob > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > > > > > -- > > > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 0eda8236c125..9feec720175f 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -639,9 +639,14 @@ 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; unsigned int nbars; u8 hdr_type; + u8 func_no; + void *addr; u32 reg; int i; @@ -654,6 +659,42 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) return -EIO; } + 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; + + for (func_no = 0; func_no < epc->max_functions; func_no++) { + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); + if (!ep_func) + return -ENOMEM; + + ep_func->func_no = func_no; + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, + PCI_CAP_ID_MSI); + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, + PCI_CAP_ID_MSIX); + + list_add_tail(&ep_func->list, &ep->func_list); + } + offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR); dw_pcie_dbi_ro_wr_en(pci); @@ -677,8 +718,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 +725,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); @@ -708,8 +746,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) } } - dw_pcie_iatu_detect(pci); - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); if (!res) return -EINVAL; @@ -717,26 +753,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,20 +769,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) if (ret < 0) epc->max_functions = 1; - for (func_no = 0; func_no < epc->max_functions; func_no++) { - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); - if (!ep_func) - return -ENOMEM; - - ep_func->func_no = func_no; - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, - PCI_CAP_ID_MSI); - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, - PCI_CAP_ID_MSIX); - - list_add_tail(&ep_func->list, &ep->func_list); - } - if (ep->ops->ep_init) ep->ops->ep_init(ep); @@ -790,6 +792,14 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) return 0; } + /* + * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this + * step as platforms that implement 'core_init_notifier' feature may + * not have the hardware ready (i.e. core initialized) for access + * (Ex: tegra194). Any hardware access on such platforms result + * in system hard hang. + */ + return dw_pcie_ep_init_complete(ep); } EXPORT_SYMBOL_GPL(dw_pcie_ep_init);
Platforms that cannot support their core initialization without the reference clock from the host, implement the feature 'core_init_notifier' to indicate the DesignWare sub-system about when their core is getting initialized. Any accesses to the core (Ex:- DBI) would result in system hang in such systems (Ex:- tegra194). This patch moves any access to the core to dw_pcie_ep_init_complete() API which is effectively called only after the core initialization. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- .../pci/controller/dwc/pcie-designware-ep.c | 88 +++++++++++-------- 1 file changed, 49 insertions(+), 39 deletions(-)