| Message ID | 20220330060515.22328-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: designware-ep: Move DBI access to init_complete if notifier is used | expand |
On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > For controllers supporting the CORE_INIT notifier, the resources are > supposed to be enabled in the init_complete function. Currently, > these controllers are enabling the resources during probe time due to > the DBI access happens in dw_pcie_ep_init(). > > This creates the dependency with the host PCIe controller since the > resource enablement like PHY depends on host PCIe to be up. For the > standalone endpoint usecase, this would never work. So let's move all DBI > access to init_complete function if CORE_INIT notifier is used. For the > controllers those doesn't support this notifier, this change is a NO-OP. > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > Cc: Om Prakash Singh <omp@nvidia.com> > Cc: Vidya Sagar <vidyas@nvidia.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> A gentle ping on this patch! Thanks, Mani > --- > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 94 insertions(+), 45 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 0eda8236c125..fb2bf4bf5ba0 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > return 0; > } > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + struct device *dev = pci->dev; > + void *addr; > + > + dw_pcie_iatu_detect(pci); > + > + ep->ib_window_map = devm_kcalloc(dev, > + BITS_TO_LONGS(pci->num_ib_windows), > + sizeof(long), > + GFP_KERNEL); > + if (!ep->ib_window_map) > + return -ENOMEM; > + > + ep->ob_window_map = devm_kcalloc(dev, > + BITS_TO_LONGS(pci->num_ob_windows), > + sizeof(long), > + GFP_KERNEL); > + if (!ep->ob_window_map) > + return -ENOMEM; > + > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > + GFP_KERNEL); > + if (!addr) > + return -ENOMEM; > + > + ep->outbound_addr = addr; > + > + return 0; > +} > + > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + struct dw_pcie_ep_func *ep_func; > + struct pci_epc *epc = ep->epc; > + struct device *dev = pci->dev; > + u8 func_no; > + > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > + if (!ep_func) > + return -ENOMEM; > + > + ep_func->func_no = func_no; > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSI); > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSIX); > + > + list_add_tail(&ep_func->list, &ep->func_list); > + } > + > + return 0; > +} > + > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > { > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > unsigned int nbars; > u8 hdr_type; > u32 reg; > - int i; > + int ret, i; > + > + if (ep->core_init_notifier) { > + ret = dw_pcie_iatu_config(ep); > + if (ret) > + return ret; > + } > + > + if (ep->core_init_notifier) { > + ret = dw_pcie_ep_func_init(ep); > + if (ret) > + return ret; > + > + if (ep->ops->ep_init) > + ep->ops->ep_init(ep); > + } > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > PCI_HEADER_TYPE_MASK; > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > { > int ret; > - void *addr; > - u8 func_no; > struct resource *res; > struct pci_epc *epc; > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > struct platform_device *pdev = to_platform_device(dev); > struct device_node *np = dev->of_node; > const struct pci_epc_features *epc_features; > - struct dw_pcie_ep_func *ep_func; > + > + if (ep->ops->get_features) { > + epc_features = ep->ops->get_features(ep); > + if (epc_features->core_init_notifier) > + ep->core_init_notifier = true; > + } > > INIT_LIST_HEAD(&ep->func_list); > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > } > } > > - dw_pcie_iatu_detect(pci); > + if (!ep->core_init_notifier) { > + ret = dw_pcie_iatu_config(ep); > + if (ret) > + return ret; > + } > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > if (!res) > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > ep->phys_base = res->start; > ep->addr_size = resource_size(res); > > - ep->ib_window_map = devm_kcalloc(dev, > - BITS_TO_LONGS(pci->num_ib_windows), > - sizeof(long), > - GFP_KERNEL); > - if (!ep->ib_window_map) > - return -ENOMEM; > - > - ep->ob_window_map = devm_kcalloc(dev, > - BITS_TO_LONGS(pci->num_ob_windows), > - sizeof(long), > - GFP_KERNEL); > - if (!ep->ob_window_map) > - return -ENOMEM; > - > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > - GFP_KERNEL); > - if (!addr) > - return -ENOMEM; > - ep->outbound_addr = addr; > - > if (pci->link_gen < 1) > pci->link_gen = of_pci_get_max_link_speed(np); > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > if (ret < 0) > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > - if (!ep_func) > - return -ENOMEM; > - > - ep_func->func_no = func_no; > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSI); > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSIX); > + if (!ep->core_init_notifier) { > + ret = dw_pcie_ep_func_init(ep); > + if (ret) > + return ret; > > - list_add_tail(&ep_func->list, &ep->func_list); > + if (ep->ops->ep_init) > + ep->ops->ep_init(ep); > } > > - if (ep->ops->ep_init) > - ep->ops->ep_init(ep); > - > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > ep->page_size); > if (ret < 0) { > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return -ENOMEM; > } > > - if (ep->ops->get_features) { > - epc_features = ep->ops->get_features(ep); > - if (epc_features->core_init_notifier) > - return 0; > - } > + if (!ep->core_init_notifier) > + return dw_pcie_ep_init_complete(ep); > > - return dw_pcie_ep_init_complete(ep); > + return 0; > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 7d6e9b7576be..aadb14159df7 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > void __iomem *msi_mem; > phys_addr_t msi_mem_phys; > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > + bool core_init_notifier; > }; > > struct dw_pcie_ops { > -- > 2.25.1 >
On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote: > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > For controllers supporting the CORE_INIT notifier, the resources are > > supposed to be enabled in the init_complete function. Currently, > > these controllers are enabling the resources during probe time due to > > the DBI access happens in dw_pcie_ep_init(). > > > > This creates the dependency with the host PCIe controller since the > > resource enablement like PHY depends on host PCIe to be up. For the > > standalone endpoint usecase, this would never work. So let's move all DBI > > access to init_complete function if CORE_INIT notifier is used. For the > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > Cc: Om Prakash Singh <omp@nvidia.com> > > Cc: Vidya Sagar <vidyas@nvidia.com> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > A gentle ping on this patch! > Ping again! Thanks, Mani > Thanks, > Mani > > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 0eda8236c125..fb2bf4bf5ba0 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > > return 0; > > } > > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + struct device *dev = pci->dev; > > + void *addr; > > + > > + dw_pcie_iatu_detect(pci); > > + > > + ep->ib_window_map = devm_kcalloc(dev, > > + BITS_TO_LONGS(pci->num_ib_windows), > > + sizeof(long), > > + GFP_KERNEL); > > + if (!ep->ib_window_map) > > + return -ENOMEM; > > + > > + ep->ob_window_map = devm_kcalloc(dev, > > + BITS_TO_LONGS(pci->num_ob_windows), > > + sizeof(long), > > + GFP_KERNEL); > > + if (!ep->ob_window_map) > > + return -ENOMEM; > > + > > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > + GFP_KERNEL); > > + if (!addr) > > + return -ENOMEM; > > + > > + ep->outbound_addr = addr; > > + > > + return 0; > > +} > > + > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + struct dw_pcie_ep_func *ep_func; > > + struct pci_epc *epc = ep->epc; > > + struct device *dev = pci->dev; > > + u8 func_no; > > + > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > + if (!ep_func) > > + return -ENOMEM; > > + > > + ep_func->func_no = func_no; > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSI); > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSIX); > > + > > + list_add_tail(&ep_func->list, &ep->func_list); > > + } > > + > > + return 0; > > +} > > + > > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > { > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > unsigned int nbars; > > u8 hdr_type; > > u32 reg; > > - int i; > > + int ret, i; > > + > > + if (ep->core_init_notifier) { > > + ret = dw_pcie_iatu_config(ep); > > + if (ret) > > + return ret; > > + } > > + > > + if (ep->core_init_notifier) { > > + ret = dw_pcie_ep_func_init(ep); > > + if (ret) > > + return ret; > > + > > + if (ep->ops->ep_init) > > + ep->ops->ep_init(ep); > > + } > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > PCI_HEADER_TYPE_MASK; > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > { > > int ret; > > - void *addr; > > - u8 func_no; > > struct resource *res; > > struct pci_epc *epc; > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > struct platform_device *pdev = to_platform_device(dev); > > struct device_node *np = dev->of_node; > > const struct pci_epc_features *epc_features; > > - struct dw_pcie_ep_func *ep_func; > > + > > + if (ep->ops->get_features) { > > + epc_features = ep->ops->get_features(ep); > > + if (epc_features->core_init_notifier) > > + ep->core_init_notifier = true; > > + } > > > > INIT_LIST_HEAD(&ep->func_list); > > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > } > > } > > > > - dw_pcie_iatu_detect(pci); > > + if (!ep->core_init_notifier) { > > + ret = dw_pcie_iatu_config(ep); > > + if (ret) > > + return ret; > > + } > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > if (!res) > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > ep->phys_base = res->start; > > ep->addr_size = resource_size(res); > > > > - ep->ib_window_map = devm_kcalloc(dev, > > - BITS_TO_LONGS(pci->num_ib_windows), > > - sizeof(long), > > - GFP_KERNEL); > > - if (!ep->ib_window_map) > > - return -ENOMEM; > > - > > - ep->ob_window_map = devm_kcalloc(dev, > > - BITS_TO_LONGS(pci->num_ob_windows), > > - sizeof(long), > > - GFP_KERNEL); > > - if (!ep->ob_window_map) > > - return -ENOMEM; > > - > > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > - GFP_KERNEL); > > - if (!addr) > > - return -ENOMEM; > > - ep->outbound_addr = addr; > > - > > if (pci->link_gen < 1) > > pci->link_gen = of_pci_get_max_link_speed(np); > > > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > if (ret < 0) > > epc->max_functions = 1; > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > - if (!ep_func) > > - return -ENOMEM; > > - > > - ep_func->func_no = func_no; > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSI); > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSIX); > > + if (!ep->core_init_notifier) { > > + ret = dw_pcie_ep_func_init(ep); > > + if (ret) > > + return ret; > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > + if (ep->ops->ep_init) > > + ep->ops->ep_init(ep); > > } > > > > - if (ep->ops->ep_init) > > - ep->ops->ep_init(ep); > > - > > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > > ep->page_size); > > if (ret < 0) { > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > return -ENOMEM; > > } > > > > - if (ep->ops->get_features) { > > - epc_features = ep->ops->get_features(ep); > > - if (epc_features->core_init_notifier) > > - return 0; > > - } > > + if (!ep->core_init_notifier) > > + return dw_pcie_ep_init_complete(ep); > > > > - return dw_pcie_ep_init_complete(ep); > > + return 0; > > } > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > index 7d6e9b7576be..aadb14159df7 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > > void __iomem *msi_mem; > > phys_addr_t msi_mem_phys; > > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > > + bool core_init_notifier; > > }; > > > > struct dw_pcie_ops { > > -- > > 2.25.1 > >
I have a minor comment. Otherwise this patch looks fine. I verified it on Tegra194 and it works as expected. Reviewed-by: Vidya Sagar <vidyas@nvidia.com> Tested-by: Vidya Sagar <vidyas@nvidia.com> On 7/7/2022 10:27 PM, Manivannan Sadhasivam wrote: > External email: Use caution opening links or attachments > > > On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote: >> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: >>> For controllers supporting the CORE_INIT notifier, the resources are >>> supposed to be enabled in the init_complete function. Currently, >>> these controllers are enabling the resources during probe time due to >>> the DBI access happens in dw_pcie_ep_init(). >>> >>> This creates the dependency with the host PCIe controller since the >>> resource enablement like PHY depends on host PCIe to be up. For the >>> standalone endpoint usecase, this would never work. So let's move all DBI >>> access to init_complete function if CORE_INIT notifier is used. For the >>> controllers those doesn't support this notifier, this change is a NO-OP. >>> >>> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> Cc: Om Prakash Singh <omp@nvidia.com> >>> Cc: Vidya Sagar <vidyas@nvidia.com> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >> A gentle ping on this patch! >> > > Ping again! > > Thanks, > Mani > >> Thanks, >> Mani >> >>> --- >>> .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ >>> drivers/pci/controller/dwc/pcie-designware.h | 1 + >>> 2 files changed, 94 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> index 0eda8236c125..fb2bf4bf5ba0 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) >>> return 0; >>> } >>> >>> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) How about using dw_pcie_ep_iatu_config() name? Just added 'ep' to be in sync with other API names in this file. >>> +{ >>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> + struct device *dev = pci->dev; >>> + void *addr; >>> + >>> + dw_pcie_iatu_detect(pci); >>> + >>> + ep->ib_window_map = devm_kcalloc(dev, >>> + BITS_TO_LONGS(pci->num_ib_windows), >>> + sizeof(long), >>> + GFP_KERNEL); >>> + if (!ep->ib_window_map) >>> + return -ENOMEM; >>> + >>> + ep->ob_window_map = devm_kcalloc(dev, >>> + BITS_TO_LONGS(pci->num_ob_windows), >>> + sizeof(long), >>> + GFP_KERNEL); >>> + if (!ep->ob_window_map) >>> + return -ENOMEM; >>> + >>> + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), >>> + GFP_KERNEL); >>> + if (!addr) >>> + return -ENOMEM; >>> + >>> + ep->outbound_addr = addr; >>> + >>> + return 0; >>> +} >>> + >>> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) >>> +{ >>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> + struct dw_pcie_ep_func *ep_func; >>> + struct pci_epc *epc = ep->epc; >>> + struct device *dev = pci->dev; >>> + u8 func_no; >>> + >>> + for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> + if (!ep_func) >>> + return -ENOMEM; >>> + >>> + ep_func->func_no = func_no; >>> + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSI); >>> + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSIX); >>> + >>> + list_add_tail(&ep_func->list, &ep->func_list); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >>> { >>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >>> unsigned int nbars; >>> u8 hdr_type; >>> u32 reg; >>> - int i; >>> + int ret, i; >>> + >>> + if (ep->core_init_notifier) { >>> + ret = dw_pcie_iatu_config(ep); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (ep->core_init_notifier) { >>> + ret = dw_pcie_ep_func_init(ep); >>> + if (ret) >>> + return ret; >>> + >>> + if (ep->ops->ep_init) >>> + ep->ops->ep_init(ep); >>> + } >>> >>> hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & >>> PCI_HEADER_TYPE_MASK; >>> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); >>> int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> { >>> int ret; >>> - void *addr; >>> - u8 func_no; >>> struct resource *res; >>> struct pci_epc *epc; >>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> struct platform_device *pdev = to_platform_device(dev); >>> struct device_node *np = dev->of_node; >>> const struct pci_epc_features *epc_features; >>> - struct dw_pcie_ep_func *ep_func; >>> + >>> + if (ep->ops->get_features) { >>> + epc_features = ep->ops->get_features(ep); >>> + if (epc_features->core_init_notifier) >>> + ep->core_init_notifier = true; >>> + } >>> >>> INIT_LIST_HEAD(&ep->func_list); >>> >>> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> } >>> } >>> >>> - dw_pcie_iatu_detect(pci); >>> + if (!ep->core_init_notifier) { >>> + ret = dw_pcie_iatu_config(ep); >>> + if (ret) >>> + return ret; >>> + } >>> >>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); >>> if (!res) >>> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> ep->phys_base = res->start; >>> ep->addr_size = resource_size(res); >>> >>> - ep->ib_window_map = devm_kcalloc(dev, >>> - BITS_TO_LONGS(pci->num_ib_windows), >>> - sizeof(long), >>> - GFP_KERNEL); >>> - if (!ep->ib_window_map) >>> - return -ENOMEM; >>> - >>> - ep->ob_window_map = devm_kcalloc(dev, >>> - BITS_TO_LONGS(pci->num_ob_windows), >>> - sizeof(long), >>> - GFP_KERNEL); >>> - if (!ep->ob_window_map) >>> - return -ENOMEM; >>> - >>> - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), >>> - GFP_KERNEL); >>> - if (!addr) >>> - return -ENOMEM; >>> - ep->outbound_addr = addr; >>> - >>> if (pci->link_gen < 1) >>> pci->link_gen = of_pci_get_max_link_speed(np); >>> >>> @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> if (ret < 0) >>> epc->max_functions = 1; >>> >>> - for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> - if (!ep_func) >>> - return -ENOMEM; >>> - >>> - ep_func->func_no = func_no; >>> - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSI); >>> - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSIX); >>> + if (!ep->core_init_notifier) { >>> + ret = dw_pcie_ep_func_init(ep); >>> + if (ret) >>> + return ret; >>> >>> - list_add_tail(&ep_func->list, &ep->func_list); >>> + if (ep->ops->ep_init) >>> + ep->ops->ep_init(ep); >>> } >>> >>> - if (ep->ops->ep_init) >>> - ep->ops->ep_init(ep); >>> - >>> ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, >>> ep->page_size); >>> if (ret < 0) { >>> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> return -ENOMEM; >>> } >>> >>> - if (ep->ops->get_features) { >>> - epc_features = ep->ops->get_features(ep); >>> - if (epc_features->core_init_notifier) >>> - return 0; >>> - } >>> + if (!ep->core_init_notifier) >>> + return dw_pcie_ep_init_complete(ep); >>> >>> - return dw_pcie_ep_init_complete(ep); >>> + return 0; >>> } >>> EXPORT_SYMBOL_GPL(dw_pcie_ep_init); >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >>> index 7d6e9b7576be..aadb14159df7 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>> @@ -242,6 +242,7 @@ struct dw_pcie_ep { >>> void __iomem *msi_mem; >>> phys_addr_t msi_mem_phys; >>> struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; >>> + bool core_init_notifier; >>> }; >>> >>> struct dw_pcie_ops { >>> -- >>> 2.25.1 >>> > > -- > மணிவண்ணன் சதாசிவம் >
Hi Manivannan, On 2022/07/08 1:57, Manivannan Sadhasivam wrote: > On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote: >> On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: >>> For controllers supporting the CORE_INIT notifier, the resources are >>> supposed to be enabled in the init_complete function. Currently, >>> these controllers are enabling the resources during probe time due to >>> the DBI access happens in dw_pcie_ep_init(). >>> >>> This creates the dependency with the host PCIe controller since the >>> resource enablement like PHY depends on host PCIe to be up. For the >>> standalone endpoint usecase, this would never work. So let's move all >>> DBI >>> access to init_complete function if CORE_INIT notifier is used. For the >>> controllers those doesn't support this notifier, this change is a NO-OP. >>> >>> Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> Cc: Om Prakash Singh <omp@nvidia.com> >>> Cc: Vidya Sagar <vidyas@nvidia.com> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >> A gentle ping on this patch! >> Sorry I couldn't check it. I think the original source seems to have changed in the latest, so please rebase and fix some conflicts. > Ping again! > > Thanks, > Mani > >> Thanks, >> Mani >> >>> --- >>> .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ >>> drivers/pci/controller/dwc/pcie-designware.h | 1 + >>> 2 files changed, 94 insertions(+), 45 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c >>> b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> index 0eda8236c125..fb2bf4bf5ba0 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c >>> @@ -636,6 +636,63 @@ static unsigned int >>> dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) >>> return 0; >>> } >>> >>> +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) >>> +{ >>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> + struct device *dev = pci->dev; >>> + void *addr; >>> + >>> + dw_pcie_iatu_detect(pci); >>> + >>> + ep->ib_window_map = devm_kcalloc(dev, >>> + BITS_TO_LONGS(pci->num_ib_windows), >>> + sizeof(long), >>> + GFP_KERNEL); >>> + if (!ep->ib_window_map) >>> + return -ENOMEM; >>> + >>> + ep->ob_window_map = devm_kcalloc(dev, >>> + BITS_TO_LONGS(pci->num_ob_windows), >>> + sizeof(long), >>> + GFP_KERNEL); >>> + if (!ep->ob_window_map) >>> + return -ENOMEM; >>> + >>> + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), >>> + GFP_KERNEL); >>> + if (!addr) >>> + return -ENOMEM; >>> + >>> + ep->outbound_addr = addr; >>> + >>> + return 0; >>> +} >>> + >>> +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) >>> +{ >>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> + struct dw_pcie_ep_func *ep_func; >>> + struct pci_epc *epc = ep->epc; >>> + struct device *dev = pci->dev; >>> + u8 func_no; >>> + >>> + for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> + if (!ep_func) >>> + return -ENOMEM; >>> + >>> + ep_func->func_no = func_no; >>> + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSI); >>> + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> + PCI_CAP_ID_MSIX); >>> + >>> + list_add_tail(&ep_func->list, &ep->func_list); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >>> { >>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) >>> unsigned int nbars; >>> u8 hdr_type; >>> u32 reg; >>> - int i; >>> + int ret, i; >>> + >>> + if (ep->core_init_notifier) { >>> + ret = dw_pcie_iatu_config(ep); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (ep->core_init_notifier) { >>> + ret = dw_pcie_ep_func_init(ep); >>> + if (ret) >>> + return ret; >>> + >>> + if (ep->ops->ep_init) >>> + ep->ops->ep_init(ep); >>> + } It isn't necessary to divide function calls under same condition (ep->core_init_notifier). >>> >>> hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & >>> PCI_HEADER_TYPE_MASK; >>> @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); >>> int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> { >>> int ret; >>> - void *addr; >>> - u8 func_no; >>> struct resource *res; >>> struct pci_epc *epc; >>> struct dw_pcie *pci = to_dw_pcie_from_ep(ep); >>> @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> struct platform_device *pdev = to_platform_device(dev); >>> struct device_node *np = dev->of_node; >>> const struct pci_epc_features *epc_features; >>> - struct dw_pcie_ep_func *ep_func; >>> + >>> + if (ep->ops->get_features) { >>> + epc_features = ep->ops->get_features(ep); >>> + if (epc_features->core_init_notifier) >>> + ep->core_init_notifier = true; >>> + } >>> >>> INIT_LIST_HEAD(&ep->func_list); >>> >>> @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> } >>> } >>> >>> - dw_pcie_iatu_detect(pci); >>> + if (!ep->core_init_notifier) { >>> + ret = dw_pcie_iatu_config(ep); >>> + if (ret) >>> + return ret; >>> + } >>> >>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >>> "addr_space"); >>> if (!res) >>> @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> ep->phys_base = res->start; >>> ep->addr_size = resource_size(res); >>> >>> - ep->ib_window_map = devm_kcalloc(dev, >>> - BITS_TO_LONGS(pci->num_ib_windows), >>> - sizeof(long), >>> - GFP_KERNEL); >>> - if (!ep->ib_window_map) >>> - return -ENOMEM; >>> - >>> - ep->ob_window_map = devm_kcalloc(dev, >>> - BITS_TO_LONGS(pci->num_ob_windows), >>> - sizeof(long), >>> - GFP_KERNEL); >>> - if (!ep->ob_window_map) >>> - return -ENOMEM; >>> - >>> - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), >>> - GFP_KERNEL); >>> - if (!addr) >>> - return -ENOMEM; >>> - ep->outbound_addr = addr; >>> - >>> if (pci->link_gen < 1) >>> pci->link_gen = of_pci_get_max_link_speed(np); >>> >>> @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> if (ret < 0) >>> epc->max_functions = 1; >>> >>> - for (func_no = 0; func_no < epc->max_functions; func_no++) { >>> - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); >>> - if (!ep_func) >>> - return -ENOMEM; >>> - >>> - ep_func->func_no = func_no; >>> - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSI); >>> - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, >>> - PCI_CAP_ID_MSIX); >>> + if (!ep->core_init_notifier) { >>> + ret = dw_pcie_ep_func_init(ep); >>> + if (ret) >>> + return ret; >>> >>> - list_add_tail(&ep_func->list, &ep->func_list); >>> + if (ep->ops->ep_init) >>> + ep->ops->ep_init(ep); >>> } >>> >>> - if (ep->ops->ep_init) >>> - ep->ops->ep_init(ep); >>> - >>> ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, >>> ep->page_size); >>> if (ret < 0) { >>> @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) >>> return -ENOMEM; >>> } >>> >>> - if (ep->ops->get_features) { >>> - epc_features = ep->ops->get_features(ep); >>> - if (epc_features->core_init_notifier) >>> - return 0; >>> - } >>> + if (!ep->core_init_notifier) >>> + return dw_pcie_ep_init_complete(ep); >>> >>> - return dw_pcie_ep_init_complete(ep); >>> + return 0; >>> } >>> EXPORT_SYMBOL_GPL(dw_pcie_ep_init); Since devm_pci_epc_create() is called in dw_pcie_ep_init(), EPC functions are available at this time, however, the driver can't access any controller registers until PERST# signal interrupt occurs. I think that EPC functions aren't called until PERST# signal, but I'm worried about whether controller access will occur. >>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h >>> b/drivers/pci/controller/dwc/pcie-designware.h >>> index 7d6e9b7576be..aadb14159df7 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware.h >>> +++ b/drivers/pci/controller/dwc/pcie-designware.h >>> @@ -242,6 +242,7 @@ struct dw_pcie_ep { >>> void __iomem *msi_mem; >>> phys_addr_t msi_mem_phys; >>> struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; >>> + bool core_init_notifier; >>> }; >>> >>> struct dw_pcie_ops { >>> -- >>> 2.25.1 >>> Thank you, --- Best Regards Kunihiko Hayashi
On Sat, Jul 09, 2022 at 10:42:32AM +0900, Kunihiko Hayashi wrote: > Hi Manivannan, > > On 2022/07/08 1:57, Manivannan Sadhasivam wrote: > > On Wed, Apr 27, 2022 at 03:48:26PM +0530, Manivannan Sadhasivam wrote: > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > > For controllers supporting the CORE_INIT notifier, the resources are > > > > supposed to be enabled in the init_complete function. Currently, > > > > these controllers are enabling the resources during probe time due to > > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > > > This creates the dependency with the host PCIe controller since the > > > > resource enablement like PHY depends on host PCIe to be up. For the > > > > standalone endpoint usecase, this would never work. So let's move all > > > > DBI > > > > access to init_complete function if CORE_INIT notifier is used. For the > > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > Cc: Om Prakash Singh <omp@nvidia.com> > > > > Cc: Vidya Sagar <vidyas@nvidia.com> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > A gentle ping on this patch! > > > > > Sorry I couldn't check it. > I think the original source seems to have changed in the latest, so please > rebase and fix some conflicts. > Sure, will do. > > Ping again! > > > > Thanks, > > Mani > > > > > Thanks, > > > Mani > > > > > > > --- > > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > index 0eda8236c125..fb2bf4bf5ba0 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > @@ -636,6 +636,63 @@ static unsigned int > > > > dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > > > > return 0; > > > > } > > > > > > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > > > > +{ > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > + struct device *dev = pci->dev; > > > > + void *addr; > > > > + > > > > + dw_pcie_iatu_detect(pci); > > > > + > > > > + ep->ib_window_map = devm_kcalloc(dev, > > > > + BITS_TO_LONGS(pci->num_ib_windows), > > > > + sizeof(long), > > > > + GFP_KERNEL); > > > > + if (!ep->ib_window_map) > > > > + return -ENOMEM; > > > > + > > > > + ep->ob_window_map = devm_kcalloc(dev, > > > > + BITS_TO_LONGS(pci->num_ob_windows), > > > > + sizeof(long), > > > > + GFP_KERNEL); > > > > + if (!ep->ob_window_map) > > > > + return -ENOMEM; > > > > + > > > > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > > + GFP_KERNEL); > > > > + if (!addr) > > > > + return -ENOMEM; > > > > + > > > > + ep->outbound_addr = addr; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > > > > +{ > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > + struct dw_pcie_ep_func *ep_func; > > > > + struct pci_epc *epc = ep->epc; > > > > + struct device *dev = pci->dev; > > > > + u8 func_no; > > > > + > > > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > + if (!ep_func) > > > > + return -ENOMEM; > > > > + > > > > + ep_func->func_no = func_no; > > > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSI); > > > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSIX); > > > > + > > > > + list_add_tail(&ep_func->list, &ep->func_list); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > > { > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > > unsigned int nbars; > > > > u8 hdr_type; > > > > u32 reg; > > > > - int i; > > > > + int ret, i; > > > > + > > > > + if (ep->core_init_notifier) { > > > > + ret = dw_pcie_iatu_config(ep); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + if (ep->core_init_notifier) { > > > > + ret = dw_pcie_ep_func_init(ep); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (ep->ops->ep_init) > > > > + ep->ops->ep_init(ep); > > > > + } > > It isn't necessary to divide function calls under same condition > (ep->core_init_notifier). > Right. I think one of the part came from previous patch, so I forgot to merge them. > > > > > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > > > PCI_HEADER_TYPE_MASK; > > > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > > > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > { > > > > int ret; > > > > - void *addr; > > > > - u8 func_no; > > > > struct resource *res; > > > > struct pci_epc *epc; > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > struct platform_device *pdev = to_platform_device(dev); > > > > struct device_node *np = dev->of_node; > > > > const struct pci_epc_features *epc_features; > > > > - struct dw_pcie_ep_func *ep_func; > > > > + > > > > + if (ep->ops->get_features) { > > > > + epc_features = ep->ops->get_features(ep); > > > > + if (epc_features->core_init_notifier) > > > > + ep->core_init_notifier = true; > > > > + } > > > > > > > > INIT_LIST_HEAD(&ep->func_list); > > > > > > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > } > > > > } > > > > > > > > - dw_pcie_iatu_detect(pci); > > > > + if (!ep->core_init_notifier) { > > > > + ret = dw_pcie_iatu_config(ep); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > "addr_space"); > > > > if (!res) > > > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > ep->phys_base = res->start; > > > > ep->addr_size = resource_size(res); > > > > > > > > - ep->ib_window_map = devm_kcalloc(dev, > > > > - BITS_TO_LONGS(pci->num_ib_windows), > > > > - sizeof(long), > > > > - GFP_KERNEL); > > > > - if (!ep->ib_window_map) > > > > - return -ENOMEM; > > > > - > > > > - ep->ob_window_map = devm_kcalloc(dev, > > > > - BITS_TO_LONGS(pci->num_ob_windows), > > > > - sizeof(long), > > > > - GFP_KERNEL); > > > > - if (!ep->ob_window_map) > > > > - return -ENOMEM; > > > > - > > > > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > > - GFP_KERNEL); > > > > - if (!addr) > > > > - return -ENOMEM; > > > > - ep->outbound_addr = addr; > > > > - > > > > if (pci->link_gen < 1) > > > > pci->link_gen = of_pci_get_max_link_speed(np); > > > > > > > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > if (ret < 0) > > > > epc->max_functions = 1; > > > > > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > - if (!ep_func) > > > > - return -ENOMEM; > > > > - > > > > - ep_func->func_no = func_no; > > > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSI); > > > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSIX); > > > > + if (!ep->core_init_notifier) { > > > > + ret = dw_pcie_ep_func_init(ep); > > > > + if (ret) > > > > + return ret; > > > > > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > > > + if (ep->ops->ep_init) > > > > + ep->ops->ep_init(ep); > > > > } > > > > > > > > - if (ep->ops->ep_init) > > > > - ep->ops->ep_init(ep); > > > > - > > > > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > > > > ep->page_size); > > > > if (ret < 0) { > > > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > return -ENOMEM; > > > > } > > > > > > > > - if (ep->ops->get_features) { > > > > - epc_features = ep->ops->get_features(ep); > > > > - if (epc_features->core_init_notifier) > > > > - return 0; > > > > - } > > > > + if (!ep->core_init_notifier) > > > > + return dw_pcie_ep_init_complete(ep); > > > > > > > > - return dw_pcie_ep_init_complete(ep); > > > > + return 0; > > > > } > > > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > Since devm_pci_epc_create() is called in dw_pcie_ep_init(), EPC functions > are available at this time, however, the driver can't access any controller > registers until PERST# signal interrupt occurs. > > I think that EPC functions aren't called until PERST# signal, but > I'm worried about whether controller access will occur. > Most of the EPC accesses are carried out by EPF and that won't be available until dw_pcie_ep_init_complete() gets completed for core_init_notifier case. This is one of the concerns Kishon raised but I think here we are safe. Thanks, Mani > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h > > > > b/drivers/pci/controller/dwc/pcie-designware.h > > > > index 7d6e9b7576be..aadb14159df7 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > > > > void __iomem *msi_mem; > > > > phys_addr_t msi_mem_phys; > > > > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > > > > + bool core_init_notifier; > > > > }; > > > > > > > > struct dw_pcie_ops { > > > > -- > > > > 2.25.1 > > > > > > Thank you, > > --- > Best Regards > Kunihiko Hayashi
On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > For controllers supporting the CORE_INIT notifier, the resources are > supposed to be enabled in the init_complete function. Currently, > these controllers are enabling the resources during probe time due to > the DBI access happens in dw_pcie_ep_init(). > > This creates the dependency with the host PCIe controller since the > resource enablement like PHY depends on host PCIe to be up. For the > standalone endpoint usecase, this would never work. So let's move all DBI > access to init_complete function if CORE_INIT notifier is used. For the > controllers those doesn't support this notifier, this change is a NO-OP. > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > Cc: Om Prakash Singh <omp@nvidia.com> > Cc: Vidya Sagar <vidyas@nvidia.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 94 insertions(+), 45 deletions(-) Sorry this got missed. Seems like there are two patches to solve the same problem: 1) This patch, and 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) I don't know much about dwc or this issue, but if these patches are functionally equivalent, I think Vidya's is a little more attractive because: - It's smaller (49 insertions(+), 39 deletions(-)). - "core_init_notifier" looks like sort of a corner-case feature and Vidya's patch doesn't depend on it so it seems more maintainable. - It's more straightforward to read -- it basically just moves things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which is exactly the sort of thing I expect if we're doing something out of order. > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 0eda8236c125..fb2bf4bf5ba0 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > return 0; > } > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + struct device *dev = pci->dev; > + void *addr; > + > + dw_pcie_iatu_detect(pci); > + > + ep->ib_window_map = devm_kcalloc(dev, > + BITS_TO_LONGS(pci->num_ib_windows), > + sizeof(long), > + GFP_KERNEL); > + if (!ep->ib_window_map) > + return -ENOMEM; > + > + ep->ob_window_map = devm_kcalloc(dev, > + BITS_TO_LONGS(pci->num_ob_windows), > + sizeof(long), > + GFP_KERNEL); > + if (!ep->ob_window_map) > + return -ENOMEM; > + > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > + GFP_KERNEL); > + if (!addr) > + return -ENOMEM; > + > + ep->outbound_addr = addr; > + > + return 0; > +} > + > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + struct dw_pcie_ep_func *ep_func; > + struct pci_epc *epc = ep->epc; > + struct device *dev = pci->dev; > + u8 func_no; > + > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > + if (!ep_func) > + return -ENOMEM; > + > + ep_func->func_no = func_no; > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSI); > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > + PCI_CAP_ID_MSIX); > + > + list_add_tail(&ep_func->list, &ep->func_list); > + } > + > + return 0; > +} > + > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > { > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > unsigned int nbars; > u8 hdr_type; > u32 reg; > - int i; > + int ret, i; > + > + if (ep->core_init_notifier) { > + ret = dw_pcie_iatu_config(ep); > + if (ret) > + return ret; > + } > + > + if (ep->core_init_notifier) { > + ret = dw_pcie_ep_func_init(ep); > + if (ret) > + return ret; > + > + if (ep->ops->ep_init) > + ep->ops->ep_init(ep); > + } > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > PCI_HEADER_TYPE_MASK; > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > { > int ret; > - void *addr; > - u8 func_no; > struct resource *res; > struct pci_epc *epc; > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > struct platform_device *pdev = to_platform_device(dev); > struct device_node *np = dev->of_node; > const struct pci_epc_features *epc_features; > - struct dw_pcie_ep_func *ep_func; > + > + if (ep->ops->get_features) { > + epc_features = ep->ops->get_features(ep); > + if (epc_features->core_init_notifier) > + ep->core_init_notifier = true; > + } > > INIT_LIST_HEAD(&ep->func_list); > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > } > } > > - dw_pcie_iatu_detect(pci); > + if (!ep->core_init_notifier) { > + ret = dw_pcie_iatu_config(ep); > + if (ret) > + return ret; > + } > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > if (!res) > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > ep->phys_base = res->start; > ep->addr_size = resource_size(res); > > - ep->ib_window_map = devm_kcalloc(dev, > - BITS_TO_LONGS(pci->num_ib_windows), > - sizeof(long), > - GFP_KERNEL); > - if (!ep->ib_window_map) > - return -ENOMEM; > - > - ep->ob_window_map = devm_kcalloc(dev, > - BITS_TO_LONGS(pci->num_ob_windows), > - sizeof(long), > - GFP_KERNEL); > - if (!ep->ob_window_map) > - return -ENOMEM; > - > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > - GFP_KERNEL); > - if (!addr) > - return -ENOMEM; > - ep->outbound_addr = addr; > - > if (pci->link_gen < 1) > pci->link_gen = of_pci_get_max_link_speed(np); > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > if (ret < 0) > epc->max_functions = 1; > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > - if (!ep_func) > - return -ENOMEM; > - > - ep_func->func_no = func_no; > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSI); > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > - PCI_CAP_ID_MSIX); > + if (!ep->core_init_notifier) { > + ret = dw_pcie_ep_func_init(ep); > + if (ret) > + return ret; > > - list_add_tail(&ep_func->list, &ep->func_list); > + if (ep->ops->ep_init) > + ep->ops->ep_init(ep); > } > > - if (ep->ops->ep_init) > - ep->ops->ep_init(ep); > - > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > ep->page_size); > if (ret < 0) { > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return -ENOMEM; > } > > - if (ep->ops->get_features) { > - epc_features = ep->ops->get_features(ep); > - if (epc_features->core_init_notifier) > - return 0; > - } > + if (!ep->core_init_notifier) > + return dw_pcie_ep_init_complete(ep); > > - return dw_pcie_ep_init_complete(ep); > + return 0; > } > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 7d6e9b7576be..aadb14159df7 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > void __iomem *msi_mem; > phys_addr_t msi_mem_phys; > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > + bool core_init_notifier; > }; > > struct dw_pcie_ops { > -- > 2.25.1 >
On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > For controllers supporting the CORE_INIT notifier, the resources are > > supposed to be enabled in the init_complete function. Currently, > > these controllers are enabling the resources during probe time due to > > the DBI access happens in dw_pcie_ep_init(). > > > > This creates the dependency with the host PCIe controller since the > > resource enablement like PHY depends on host PCIe to be up. For the > > standalone endpoint usecase, this would never work. So let's move all DBI > > access to init_complete function if CORE_INIT notifier is used. For the > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > Cc: Om Prakash Singh <omp@nvidia.com> > > Cc: Vidya Sagar <vidyas@nvidia.com> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 94 insertions(+), 45 deletions(-) > > Sorry this got missed. Seems like there are two patches to solve the > same problem: > > 1) This patch, and > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) > > I don't know much about dwc or this issue, but if these patches are > functionally equivalent, I think Vidya's is a little more attractive > because: > > - It's smaller (49 insertions(+), 39 deletions(-)). > > - "core_init_notifier" looks like sort of a corner-case feature and > Vidya's patch doesn't depend on it so it seems more maintainable. > > - It's more straightforward to read -- it basically just moves > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > is exactly the sort of thing I expect if we're doing something out > of order. > I agree that Vidya's patch is simple but as per the feedback from Kishon on my previous patch, I had to add some extra logic to make sure the move of DBI access doesn't affect the non core_init_notifier platforms. So with my patch, the logic added is essentailly a NO-OP on those. Thanks, Mani > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index 0eda8236c125..fb2bf4bf5ba0 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > > return 0; > > } > > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + struct device *dev = pci->dev; > > + void *addr; > > + > > + dw_pcie_iatu_detect(pci); > > + > > + ep->ib_window_map = devm_kcalloc(dev, > > + BITS_TO_LONGS(pci->num_ib_windows), > > + sizeof(long), > > + GFP_KERNEL); > > + if (!ep->ib_window_map) > > + return -ENOMEM; > > + > > + ep->ob_window_map = devm_kcalloc(dev, > > + BITS_TO_LONGS(pci->num_ob_windows), > > + sizeof(long), > > + GFP_KERNEL); > > + if (!ep->ob_window_map) > > + return -ENOMEM; > > + > > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > + GFP_KERNEL); > > + if (!addr) > > + return -ENOMEM; > > + > > + ep->outbound_addr = addr; > > + > > + return 0; > > +} > > + > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + struct dw_pcie_ep_func *ep_func; > > + struct pci_epc *epc = ep->epc; > > + struct device *dev = pci->dev; > > + u8 func_no; > > + > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > + if (!ep_func) > > + return -ENOMEM; > > + > > + ep_func->func_no = func_no; > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSI); > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > + PCI_CAP_ID_MSIX); > > + > > + list_add_tail(&ep_func->list, &ep->func_list); > > + } > > + > > + return 0; > > +} > > + > > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > { > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > unsigned int nbars; > > u8 hdr_type; > > u32 reg; > > - int i; > > + int ret, i; > > + > > + if (ep->core_init_notifier) { > > + ret = dw_pcie_iatu_config(ep); > > + if (ret) > > + return ret; > > + } > > + > > + if (ep->core_init_notifier) { > > + ret = dw_pcie_ep_func_init(ep); > > + if (ret) > > + return ret; > > + > > + if (ep->ops->ep_init) > > + ep->ops->ep_init(ep); > > + } > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > PCI_HEADER_TYPE_MASK; > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > { > > int ret; > > - void *addr; > > - u8 func_no; > > struct resource *res; > > struct pci_epc *epc; > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > struct platform_device *pdev = to_platform_device(dev); > > struct device_node *np = dev->of_node; > > const struct pci_epc_features *epc_features; > > - struct dw_pcie_ep_func *ep_func; > > + > > + if (ep->ops->get_features) { > > + epc_features = ep->ops->get_features(ep); > > + if (epc_features->core_init_notifier) > > + ep->core_init_notifier = true; > > + } > > > > INIT_LIST_HEAD(&ep->func_list); > > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > } > > } > > > > - dw_pcie_iatu_detect(pci); > > + if (!ep->core_init_notifier) { > > + ret = dw_pcie_iatu_config(ep); > > + if (ret) > > + return ret; > > + } > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > if (!res) > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > ep->phys_base = res->start; > > ep->addr_size = resource_size(res); > > > > - ep->ib_window_map = devm_kcalloc(dev, > > - BITS_TO_LONGS(pci->num_ib_windows), > > - sizeof(long), > > - GFP_KERNEL); > > - if (!ep->ib_window_map) > > - return -ENOMEM; > > - > > - ep->ob_window_map = devm_kcalloc(dev, > > - BITS_TO_LONGS(pci->num_ob_windows), > > - sizeof(long), > > - GFP_KERNEL); > > - if (!ep->ob_window_map) > > - return -ENOMEM; > > - > > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > - GFP_KERNEL); > > - if (!addr) > > - return -ENOMEM; > > - ep->outbound_addr = addr; > > - > > if (pci->link_gen < 1) > > pci->link_gen = of_pci_get_max_link_speed(np); > > > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > if (ret < 0) > > epc->max_functions = 1; > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > - if (!ep_func) > > - return -ENOMEM; > > - > > - ep_func->func_no = func_no; > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSI); > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > - PCI_CAP_ID_MSIX); > > + if (!ep->core_init_notifier) { > > + ret = dw_pcie_ep_func_init(ep); > > + if (ret) > > + return ret; > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > + if (ep->ops->ep_init) > > + ep->ops->ep_init(ep); > > } > > > > - if (ep->ops->ep_init) > > - ep->ops->ep_init(ep); > > - > > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > > ep->page_size); > > if (ret < 0) { > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > return -ENOMEM; > > } > > > > - if (ep->ops->get_features) { > > - epc_features = ep->ops->get_features(ep); > > - if (epc_features->core_init_notifier) > > - return 0; > > - } > > + if (!ep->core_init_notifier) > > + return dw_pcie_ep_init_complete(ep); > > > > - return dw_pcie_ep_init_complete(ep); > > + return 0; > > } > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > index 7d6e9b7576be..aadb14159df7 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > > void __iomem *msi_mem; > > phys_addr_t msi_mem_phys; > > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > > + bool core_init_notifier; > > }; > > > > struct dw_pcie_ops { > > -- > > 2.25.1 > >
On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote: > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > For controllers supporting the CORE_INIT notifier, the resources are > > > supposed to be enabled in the init_complete function. Currently, > > > these controllers are enabling the resources during probe time due to > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > This creates the dependency with the host PCIe controller since the > > > resource enablement like PHY depends on host PCIe to be up. For the > > > standalone endpoint usecase, this would never work. So let's move all DBI > > > access to init_complete function if CORE_INIT notifier is used. For the > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > Cc: Om Prakash Singh <omp@nvidia.com> > > > Cc: Vidya Sagar <vidyas@nvidia.com> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > Sorry this got missed. Seems like there are two patches to solve the > > same problem: > > > > 1) This patch, and > > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) > > > > I don't know much about dwc or this issue, but if these patches are > > functionally equivalent, I think Vidya's is a little more attractive > > because: > > > > - It's smaller (49 insertions(+), 39 deletions(-)). > > > > - "core_init_notifier" looks like sort of a corner-case feature and > > Vidya's patch doesn't depend on it so it seems more maintainable. > > > > - It's more straightforward to read -- it basically just moves > > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > > is exactly the sort of thing I expect if we're doing something out > > of order. > > I agree that Vidya's patch is simple but as per the feedback from > Kishon on my previous patch, I had to add some extra logic to make > sure the move of DBI access doesn't affect the non > core_init_notifier platforms. > > So with my patch, the logic added is essentailly a NO-OP on those. Can you include the lore URL for Kishon's feedback? I can't find it. If we think moving the DBI access is safe on non-core_init_notifier platforms, I'd like to do it everywhere so they're all the same. I don't want different behavior just to avoid the risk of theoretical problems that we think should not happen. > Thanks, > Mani > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > index 0eda8236c125..fb2bf4bf5ba0 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > > > return 0; > > > } > > > > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > > > +{ > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > + struct device *dev = pci->dev; > > > + void *addr; > > > + > > > + dw_pcie_iatu_detect(pci); > > > + > > > + ep->ib_window_map = devm_kcalloc(dev, > > > + BITS_TO_LONGS(pci->num_ib_windows), > > > + sizeof(long), > > > + GFP_KERNEL); > > > + if (!ep->ib_window_map) > > > + return -ENOMEM; > > > + > > > + ep->ob_window_map = devm_kcalloc(dev, > > > + BITS_TO_LONGS(pci->num_ob_windows), > > > + sizeof(long), > > > + GFP_KERNEL); > > > + if (!ep->ob_window_map) > > > + return -ENOMEM; > > > + > > > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > + GFP_KERNEL); > > > + if (!addr) > > > + return -ENOMEM; > > > + > > > + ep->outbound_addr = addr; > > > + > > > + return 0; > > > +} > > > + > > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > > > +{ > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > + struct dw_pcie_ep_func *ep_func; > > > + struct pci_epc *epc = ep->epc; > > > + struct device *dev = pci->dev; > > > + u8 func_no; > > > + > > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > + if (!ep_func) > > > + return -ENOMEM; > > > + > > > + ep_func->func_no = func_no; > > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > + PCI_CAP_ID_MSI); > > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > + PCI_CAP_ID_MSIX); > > > + > > > + list_add_tail(&ep_func->list, &ep->func_list); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > { > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > unsigned int nbars; > > > u8 hdr_type; > > > u32 reg; > > > - int i; > > > + int ret, i; > > > + > > > + if (ep->core_init_notifier) { > > > + ret = dw_pcie_iatu_config(ep); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + if (ep->core_init_notifier) { > > > + ret = dw_pcie_ep_func_init(ep); > > > + if (ret) > > > + return ret; > > > + > > > + if (ep->ops->ep_init) > > > + ep->ops->ep_init(ep); > > > + } > > > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > > PCI_HEADER_TYPE_MASK; > > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > { > > > int ret; > > > - void *addr; > > > - u8 func_no; > > > struct resource *res; > > > struct pci_epc *epc; > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > struct platform_device *pdev = to_platform_device(dev); > > > struct device_node *np = dev->of_node; > > > const struct pci_epc_features *epc_features; > > > - struct dw_pcie_ep_func *ep_func; > > > + > > > + if (ep->ops->get_features) { > > > + epc_features = ep->ops->get_features(ep); > > > + if (epc_features->core_init_notifier) > > > + ep->core_init_notifier = true; > > > + } > > > > > > INIT_LIST_HEAD(&ep->func_list); > > > > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > } > > > } > > > > > > - dw_pcie_iatu_detect(pci); > > > + if (!ep->core_init_notifier) { > > > + ret = dw_pcie_iatu_config(ep); > > > + if (ret) > > > + return ret; > > > + } > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > > if (!res) > > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > ep->phys_base = res->start; > > > ep->addr_size = resource_size(res); > > > > > > - ep->ib_window_map = devm_kcalloc(dev, > > > - BITS_TO_LONGS(pci->num_ib_windows), > > > - sizeof(long), > > > - GFP_KERNEL); > > > - if (!ep->ib_window_map) > > > - return -ENOMEM; > > > - > > > - ep->ob_window_map = devm_kcalloc(dev, > > > - BITS_TO_LONGS(pci->num_ob_windows), > > > - sizeof(long), > > > - GFP_KERNEL); > > > - if (!ep->ob_window_map) > > > - return -ENOMEM; > > > - > > > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > - GFP_KERNEL); > > > - if (!addr) > > > - return -ENOMEM; > > > - ep->outbound_addr = addr; > > > - > > > if (pci->link_gen < 1) > > > pci->link_gen = of_pci_get_max_link_speed(np); > > > > > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > if (ret < 0) > > > epc->max_functions = 1; > > > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > - if (!ep_func) > > > - return -ENOMEM; > > > - > > > - ep_func->func_no = func_no; > > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > - PCI_CAP_ID_MSI); > > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > - PCI_CAP_ID_MSIX); > > > + if (!ep->core_init_notifier) { > > > + ret = dw_pcie_ep_func_init(ep); > > > + if (ret) > > > + return ret; > > > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > > + if (ep->ops->ep_init) > > > + ep->ops->ep_init(ep); > > > } > > > > > > - if (ep->ops->ep_init) > > > - ep->ops->ep_init(ep); > > > - > > > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > > > ep->page_size); > > > if (ret < 0) { > > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > return -ENOMEM; > > > } > > > > > > - if (ep->ops->get_features) { > > > - epc_features = ep->ops->get_features(ep); > > > - if (epc_features->core_init_notifier) > > > - return 0; > > > - } > > > + if (!ep->core_init_notifier) > > > + return dw_pcie_ep_init_complete(ep); > > > > > > - return dw_pcie_ep_init_complete(ep); > > > + return 0; > > > } > > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > index 7d6e9b7576be..aadb14159df7 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > > > void __iomem *msi_mem; > > > phys_addr_t msi_mem_phys; > > > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > > > + bool core_init_notifier; > > > }; > > > > > > struct dw_pcie_ops { > > > -- > > > 2.25.1 > > > > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote: > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote: > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > > For controllers supporting the CORE_INIT notifier, the resources are > > > > supposed to be enabled in the init_complete function. Currently, > > > > these controllers are enabling the resources during probe time due to > > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > > > This creates the dependency with the host PCIe controller since the > > > > resource enablement like PHY depends on host PCIe to be up. For the > > > > standalone endpoint usecase, this would never work. So let's move all DBI > > > > access to init_complete function if CORE_INIT notifier is used. For the > > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > Cc: Om Prakash Singh <omp@nvidia.com> > > > > Cc: Vidya Sagar <vidyas@nvidia.com> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > > > Sorry this got missed. Seems like there are two patches to solve the > > > same problem: > > > > > > 1) This patch, and > > > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) > > > > > > I don't know much about dwc or this issue, but if these patches are > > > functionally equivalent, I think Vidya's is a little more attractive > > > because: > > > > > > - It's smaller (49 insertions(+), 39 deletions(-)). > > > > > > - "core_init_notifier" looks like sort of a corner-case feature and > > > Vidya's patch doesn't depend on it so it seems more maintainable. > > > > > > - It's more straightforward to read -- it basically just moves > > > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > > > is exactly the sort of thing I expect if we're doing something out > > > of order. > > > > I agree that Vidya's patch is simple but as per the feedback from > > Kishon on my previous patch, I had to add some extra logic to make > > sure the move of DBI access doesn't affect the non > > core_init_notifier platforms. > > > > So with my patch, the logic added is essentailly a NO-OP on those. > > Can you include the lore URL for Kishon's feedback? I can't find it. > https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629 > If we think moving the DBI access is safe on non-core_init_notifier > platforms, I'd like to do it everywhere so they're all the same. I > don't want different behavior just to avoid the risk of theoretical > problems that we think should not happen. > One more issue Kishon pointed out was that, in the patch the endpoint controller is configured after pci_epc_create(). So he raised a concern that if ecp_ops is invoked before the controller gets configured fully, it could result in aborts. While the concern may be true for non-core_init_notifier platforms (I'm not sure though) but I'm certain not for the core_init_notifier ones as the EFP drivers only access EPC ops after dw_pcie_ep_init_complete(). Thanks, Mani > > Thanks, > > Mani > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > index 0eda8236c125..fb2bf4bf5ba0 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) > > > > return 0; > > > > } > > > > > > > > +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) > > > > +{ > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > + struct device *dev = pci->dev; > > > > + void *addr; > > > > + > > > > + dw_pcie_iatu_detect(pci); > > > > + > > > > + ep->ib_window_map = devm_kcalloc(dev, > > > > + BITS_TO_LONGS(pci->num_ib_windows), > > > > + sizeof(long), > > > > + GFP_KERNEL); > > > > + if (!ep->ib_window_map) > > > > + return -ENOMEM; > > > > + > > > > + ep->ob_window_map = devm_kcalloc(dev, > > > > + BITS_TO_LONGS(pci->num_ob_windows), > > > > + sizeof(long), > > > > + GFP_KERNEL); > > > > + if (!ep->ob_window_map) > > > > + return -ENOMEM; > > > > + > > > > + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > > + GFP_KERNEL); > > > > + if (!addr) > > > > + return -ENOMEM; > > > > + > > > > + ep->outbound_addr = addr; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) > > > > +{ > > > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > + struct dw_pcie_ep_func *ep_func; > > > > + struct pci_epc *epc = ep->epc; > > > > + struct device *dev = pci->dev; > > > > + u8 func_no; > > > > + > > > > + for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > + if (!ep_func) > > > > + return -ENOMEM; > > > > + > > > > + ep_func->func_no = func_no; > > > > + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSI); > > > > + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > + PCI_CAP_ID_MSIX); > > > > + > > > > + list_add_tail(&ep_func->list, &ep->func_list); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > > { > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) > > > > unsigned int nbars; > > > > u8 hdr_type; > > > > u32 reg; > > > > - int i; > > > > + int ret, i; > > > > + > > > > + if (ep->core_init_notifier) { > > > > + ret = dw_pcie_iatu_config(ep); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + if (ep->core_init_notifier) { > > > > + ret = dw_pcie_ep_func_init(ep); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (ep->ops->ep_init) > > > > + ep->ops->ep_init(ep); > > > > + } > > > > > > > > hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & > > > > PCI_HEADER_TYPE_MASK; > > > > @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); > > > > int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > { > > > > int ret; > > > > - void *addr; > > > > - u8 func_no; > > > > struct resource *res; > > > > struct pci_epc *epc; > > > > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > > > @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > struct platform_device *pdev = to_platform_device(dev); > > > > struct device_node *np = dev->of_node; > > > > const struct pci_epc_features *epc_features; > > > > - struct dw_pcie_ep_func *ep_func; > > > > + > > > > + if (ep->ops->get_features) { > > > > + epc_features = ep->ops->get_features(ep); > > > > + if (epc_features->core_init_notifier) > > > > + ep->core_init_notifier = true; > > > > + } > > > > > > > > INIT_LIST_HEAD(&ep->func_list); > > > > > > > > @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > } > > > > } > > > > > > > > - dw_pcie_iatu_detect(pci); > > > > + if (!ep->core_init_notifier) { > > > > + ret = dw_pcie_iatu_config(ep); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > > > > if (!res) > > > > @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > ep->phys_base = res->start; > > > > ep->addr_size = resource_size(res); > > > > > > > > - ep->ib_window_map = devm_kcalloc(dev, > > > > - BITS_TO_LONGS(pci->num_ib_windows), > > > > - sizeof(long), > > > > - GFP_KERNEL); > > > > - if (!ep->ib_window_map) > > > > - return -ENOMEM; > > > > - > > > > - ep->ob_window_map = devm_kcalloc(dev, > > > > - BITS_TO_LONGS(pci->num_ob_windows), > > > > - sizeof(long), > > > > - GFP_KERNEL); > > > > - if (!ep->ob_window_map) > > > > - return -ENOMEM; > > > > - > > > > - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), > > > > - GFP_KERNEL); > > > > - if (!addr) > > > > - return -ENOMEM; > > > > - ep->outbound_addr = addr; > > > > - > > > > if (pci->link_gen < 1) > > > > pci->link_gen = of_pci_get_max_link_speed(np); > > > > > > > > @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > if (ret < 0) > > > > epc->max_functions = 1; > > > > > > > > - for (func_no = 0; func_no < epc->max_functions; func_no++) { > > > > - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); > > > > - if (!ep_func) > > > > - return -ENOMEM; > > > > - > > > > - ep_func->func_no = func_no; > > > > - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSI); > > > > - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, > > > > - PCI_CAP_ID_MSIX); > > > > + if (!ep->core_init_notifier) { > > > > + ret = dw_pcie_ep_func_init(ep); > > > > + if (ret) > > > > + return ret; > > > > > > > > - list_add_tail(&ep_func->list, &ep->func_list); > > > > + if (ep->ops->ep_init) > > > > + ep->ops->ep_init(ep); > > > > } > > > > > > > > - if (ep->ops->ep_init) > > > > - ep->ops->ep_init(ep); > > > > - > > > > ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, > > > > ep->page_size); > > > > if (ret < 0) { > > > > @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > > > > return -ENOMEM; > > > > } > > > > > > > > - if (ep->ops->get_features) { > > > > - epc_features = ep->ops->get_features(ep); > > > > - if (epc_features->core_init_notifier) > > > > - return 0; > > > > - } > > > > + if (!ep->core_init_notifier) > > > > + return dw_pcie_ep_init_complete(ep); > > > > > > > > - return dw_pcie_ep_init_complete(ep); > > > > + return 0; > > > > } > > > > EXPORT_SYMBOL_GPL(dw_pcie_ep_init); > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > > index 7d6e9b7576be..aadb14159df7 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > @@ -242,6 +242,7 @@ struct dw_pcie_ep { > > > > void __iomem *msi_mem; > > > > phys_addr_t msi_mem_phys; > > > > struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; > > > > + bool core_init_notifier; > > > > }; > > > > > > > > struct dw_pcie_ops { > > > > -- > > > > 2.25.1 > > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
On Thu, Jul 21, 2022 at 02:08:45PM +0530, Manivannan Sadhasivam wrote: > On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote: > > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote: > > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > > > For controllers supporting the CORE_INIT notifier, the resources are > > > > > supposed to be enabled in the init_complete function. Currently, > > > > > these controllers are enabling the resources during probe time due to > > > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > > > > > This creates the dependency with the host PCIe controller since the > > > > > resource enablement like PHY depends on host PCIe to be up. For the > > > > > standalone endpoint usecase, this would never work. So let's move all DBI > > > > > access to init_complete function if CORE_INIT notifier is used. For the > > > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > > Cc: Om Prakash Singh <omp@nvidia.com> > > > > > Cc: Vidya Sagar <vidyas@nvidia.com> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > > > > > Sorry this got missed. Seems like there are two patches to solve the > > > > same problem: > > > > > > > > 1) This patch, and > > > > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) > > > > > > > > I don't know much about dwc or this issue, but if these patches are > > > > functionally equivalent, I think Vidya's is a little more attractive > > > > because: > > > > > > > > - It's smaller (49 insertions(+), 39 deletions(-)). > > > > > > > > - "core_init_notifier" looks like sort of a corner-case feature and > > > > Vidya's patch doesn't depend on it so it seems more maintainable. > > > > > > > > - It's more straightforward to read -- it basically just moves > > > > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > > > > is exactly the sort of thing I expect if we're doing something out > > > > of order. > > > > > > I agree that Vidya's patch is simple but as per the feedback from > > > Kishon on my previous patch, I had to add some extra logic to make > > > sure the move of DBI access doesn't affect the non > > > core_init_notifier platforms. > > > > > > So with my patch, the logic added is essentailly a NO-OP on those. > > > > Can you include the lore URL for Kishon's feedback? I can't find it. > > https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629 Thanks! (Or the canonical permanent URL: https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@ti.com) > > If we think moving the DBI access is safe on non-core_init_notifier > > platforms, I'd like to do it everywhere so they're all the same. I > > don't want different behavior just to avoid the risk of theoretical > > problems that we think should not happen. > > One more issue Kishon pointed out was that, in the patch the > endpoint controller is configured after pci_epc_create(). So he > raised a concern that if ecp_ops is invoked before the controller > gets configured fully, it could result in aborts. > > While the concern may be true for non-core_init_notifier platforms > (I'm not sure though) but I'm certain not for the core_init_notifier > ones as the EFP drivers only access EPC ops after > dw_pcie_ep_init_complete(). I really don't understand what "core_init_notifier" does, but it seems incidental to this issue. Is there really a connection? It sounds like the only reason to check for it is to limit the scope of the change, not because DBI access is inherently related to core_init_notifier. Maybe Vidya's patch needs some enhancement to cover other paths where DBI may be accessed before we're ready? Bjorn
On Thu, Jul 21, 2022 at 10:30:55AM -0500, Bjorn Helgaas wrote: > On Thu, Jul 21, 2022 at 02:08:45PM +0530, Manivannan Sadhasivam wrote: > > On Tue, Jul 19, 2022 at 05:38:14PM -0500, Bjorn Helgaas wrote: > > > On Tue, Jul 19, 2022 at 09:13:58AM +0530, Manivannan Sadhasivam wrote: > > > > On Fri, Jul 15, 2022 at 04:39:58PM -0500, Bjorn Helgaas wrote: > > > > > On Wed, Mar 30, 2022 at 11:35:15AM +0530, Manivannan Sadhasivam wrote: > > > > > > For controllers supporting the CORE_INIT notifier, the resources are > > > > > > supposed to be enabled in the init_complete function. Currently, > > > > > > these controllers are enabling the resources during probe time due to > > > > > > the DBI access happens in dw_pcie_ep_init(). > > > > > > > > > > > > This creates the dependency with the host PCIe controller since the > > > > > > resource enablement like PHY depends on host PCIe to be up. For the > > > > > > standalone endpoint usecase, this would never work. So let's move all DBI > > > > > > access to init_complete function if CORE_INIT notifier is used. For the > > > > > > controllers those doesn't support this notifier, this change is a NO-OP. > > > > > > > > > > > > Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > > > Cc: Om Prakash Singh <omp@nvidia.com> > > > > > > Cc: Vidya Sagar <vidyas@nvidia.com> > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > --- > > > > > > .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ > > > > > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > > > > > 2 files changed, 94 insertions(+), 45 deletions(-) > > > > > > > > > > Sorry this got missed. Seems like there are two patches to solve the > > > > > same problem: > > > > > > > > > > 1) This patch, and > > > > > 2) Vidya's patch (https://lore.kernel.org/linux-pci/20220622040133.31058-1-vidyas@nvidia.com/) > > > > > > > > > > I don't know much about dwc or this issue, but if these patches are > > > > > functionally equivalent, I think Vidya's is a little more attractive > > > > > because: > > > > > > > > > > - It's smaller (49 insertions(+), 39 deletions(-)). > > > > > > > > > > - "core_init_notifier" looks like sort of a corner-case feature and > > > > > Vidya's patch doesn't depend on it so it seems more maintainable. > > > > > > > > > > - It's more straightforward to read -- it basically just moves > > > > > things from dw_pcie_ep_init() to dw_pcie_ep_init_complete(), which > > > > > is exactly the sort of thing I expect if we're doing something out > > > > > of order. > > > > > > > > I agree that Vidya's patch is simple but as per the feedback from > > > > Kishon on my previous patch, I had to add some extra logic to make > > > > sure the move of DBI access doesn't affect the non > > > > core_init_notifier platforms. > > > > > > > > So with my patch, the logic added is essentailly a NO-OP on those. > > > > > > Can you include the lore URL for Kishon's feedback? I can't find it. > > > > https://patchwork.kernel.org/project/linux-pci/patch/1630473361-27198-3-git-send-email-hayashi.kunihiko@socionext.com/#24633629 > > Thanks! (Or the canonical permanent URL: > https://lore.kernel.org/r/576457dd-3e66-a3b9-f51c-ea94bc267fdb@ti.com) > > > > If we think moving the DBI access is safe on non-core_init_notifier > > > platforms, I'd like to do it everywhere so they're all the same. I > > > don't want different behavior just to avoid the risk of theoretical > > > problems that we think should not happen. > > > > One more issue Kishon pointed out was that, in the patch the > > endpoint controller is configured after pci_epc_create(). So he > > raised a concern that if ecp_ops is invoked before the controller > > gets configured fully, it could result in aborts. > > > > While the concern may be true for non-core_init_notifier platforms > > (I'm not sure though) but I'm certain not for the core_init_notifier > > ones as the EFP drivers only access EPC ops after > > dw_pcie_ep_init_complete(). > > I really don't understand what "core_init_notifier" does, but it seems > incidental to this issue. "core_init_notifer" flag was added to let the EPF driver know that the endpoint controller has completed its initialization. This is needed for platforms that require refclk from the host to be active before accessing any controller specific registers. Tegra194 and Qcom are one of those kinds. > Is there really a connection? It sounds > like the only reason to check for it is to limit the scope of the > change, not because DBI access is inherently related to > core_init_notifier. > DBI access requires refclk on the above mentioned platforms, so yes there is a strong connection. This patch (also mine) moves away all of the DBI access to the init_complete() notifier callback. Because, without this patch we need to bring-up the host first and then power-on the endpoint. Otherwise, if the endpoint is powered-on first, it will crash as there would no refclk from host. > Maybe Vidya's patch needs some enhancement to cover other paths where > DBI may be accessed before we're ready? > FWIW, Vidya's patch looks good to me. I pointed out my patch just because it covers the theoretical issue mentioned by Kishon. Thanks, Mani > Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index 0eda8236c125..fb2bf4bf5ba0 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -636,6 +636,63 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap) return 0; } +static int dw_pcie_iatu_config(struct dw_pcie_ep *ep) +{ + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + struct device *dev = pci->dev; + void *addr; + + dw_pcie_iatu_detect(pci); + + ep->ib_window_map = devm_kcalloc(dev, + BITS_TO_LONGS(pci->num_ib_windows), + sizeof(long), + GFP_KERNEL); + if (!ep->ib_window_map) + return -ENOMEM; + + ep->ob_window_map = devm_kcalloc(dev, + BITS_TO_LONGS(pci->num_ob_windows), + sizeof(long), + GFP_KERNEL); + if (!ep->ob_window_map) + return -ENOMEM; + + addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), + GFP_KERNEL); + if (!addr) + return -ENOMEM; + + ep->outbound_addr = addr; + + return 0; +} + +static int dw_pcie_ep_func_init(struct dw_pcie_ep *ep) +{ + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); + struct dw_pcie_ep_func *ep_func; + struct pci_epc *epc = ep->epc; + struct device *dev = pci->dev; + u8 func_no; + + for (func_no = 0; func_no < epc->max_functions; func_no++) { + ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); + if (!ep_func) + return -ENOMEM; + + ep_func->func_no = func_no; + ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, + PCI_CAP_ID_MSI); + ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, + PCI_CAP_ID_MSIX); + + list_add_tail(&ep_func->list, &ep->func_list); + } + + return 0; +} + int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) { struct dw_pcie *pci = to_dw_pcie_from_ep(ep); @@ -643,7 +700,22 @@ int dw_pcie_ep_init_complete(struct dw_pcie_ep *ep) unsigned int nbars; u8 hdr_type; u32 reg; - int i; + int ret, i; + + if (ep->core_init_notifier) { + ret = dw_pcie_iatu_config(ep); + if (ret) + return ret; + } + + if (ep->core_init_notifier) { + ret = dw_pcie_ep_func_init(ep); + if (ret) + return ret; + + if (ep->ops->ep_init) + ep->ops->ep_init(ep); + } hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE) & PCI_HEADER_TYPE_MASK; @@ -677,8 +749,6 @@ EXPORT_SYMBOL_GPL(dw_pcie_ep_init_complete); int dw_pcie_ep_init(struct dw_pcie_ep *ep) { int ret; - void *addr; - u8 func_no; struct resource *res; struct pci_epc *epc; struct dw_pcie *pci = to_dw_pcie_from_ep(ep); @@ -686,7 +756,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) struct platform_device *pdev = to_platform_device(dev); struct device_node *np = dev->of_node; const struct pci_epc_features *epc_features; - struct dw_pcie_ep_func *ep_func; + + if (ep->ops->get_features) { + epc_features = ep->ops->get_features(ep); + if (epc_features->core_init_notifier) + ep->core_init_notifier = true; + } INIT_LIST_HEAD(&ep->func_list); @@ -708,7 +783,11 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) } } - dw_pcie_iatu_detect(pci); + if (!ep->core_init_notifier) { + ret = dw_pcie_iatu_config(ep); + if (ret) + return ret; + } res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); if (!res) @@ -717,26 +796,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) ep->phys_base = res->start; ep->addr_size = resource_size(res); - ep->ib_window_map = devm_kcalloc(dev, - BITS_TO_LONGS(pci->num_ib_windows), - sizeof(long), - GFP_KERNEL); - if (!ep->ib_window_map) - return -ENOMEM; - - ep->ob_window_map = devm_kcalloc(dev, - BITS_TO_LONGS(pci->num_ob_windows), - sizeof(long), - GFP_KERNEL); - if (!ep->ob_window_map) - return -ENOMEM; - - addr = devm_kcalloc(dev, pci->num_ob_windows, sizeof(phys_addr_t), - GFP_KERNEL); - if (!addr) - return -ENOMEM; - ep->outbound_addr = addr; - if (pci->link_gen < 1) pci->link_gen = of_pci_get_max_link_speed(np); @@ -753,23 +812,15 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) if (ret < 0) epc->max_functions = 1; - for (func_no = 0; func_no < epc->max_functions; func_no++) { - ep_func = devm_kzalloc(dev, sizeof(*ep_func), GFP_KERNEL); - if (!ep_func) - return -ENOMEM; - - ep_func->func_no = func_no; - ep_func->msi_cap = dw_pcie_ep_find_capability(ep, func_no, - PCI_CAP_ID_MSI); - ep_func->msix_cap = dw_pcie_ep_find_capability(ep, func_no, - PCI_CAP_ID_MSIX); + if (!ep->core_init_notifier) { + ret = dw_pcie_ep_func_init(ep); + if (ret) + return ret; - list_add_tail(&ep_func->list, &ep->func_list); + if (ep->ops->ep_init) + ep->ops->ep_init(ep); } - if (ep->ops->ep_init) - ep->ops->ep_init(ep); - ret = pci_epc_mem_init(epc, ep->phys_base, ep->addr_size, ep->page_size); if (ret < 0) { @@ -784,12 +835,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) return -ENOMEM; } - if (ep->ops->get_features) { - epc_features = ep->ops->get_features(ep); - if (epc_features->core_init_notifier) - return 0; - } + if (!ep->core_init_notifier) + return dw_pcie_ep_init_complete(ep); - return dw_pcie_ep_init_complete(ep); + return 0; } EXPORT_SYMBOL_GPL(dw_pcie_ep_init); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 7d6e9b7576be..aadb14159df7 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -242,6 +242,7 @@ struct dw_pcie_ep { void __iomem *msi_mem; phys_addr_t msi_mem_phys; struct pci_epf_bar *epf_bar[PCI_STD_NUM_BARS]; + bool core_init_notifier; }; struct dw_pcie_ops {
For controllers supporting the CORE_INIT notifier, the resources are supposed to be enabled in the init_complete function. Currently, these controllers are enabling the resources during probe time due to the DBI access happens in dw_pcie_ep_init(). This creates the dependency with the host PCIe controller since the resource enablement like PHY depends on host PCIe to be up. For the standalone endpoint usecase, this would never work. So let's move all DBI access to init_complete function if CORE_INIT notifier is used. For the controllers those doesn't support this notifier, this change is a NO-OP. Cc: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> Cc: Om Prakash Singh <omp@nvidia.com> Cc: Vidya Sagar <vidyas@nvidia.com> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- .../pci/controller/dwc/pcie-designware-ep.c | 138 ++++++++++++------ drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 94 insertions(+), 45 deletions(-)