| Message ID | 20200821035420.380495-4-robh@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | PCI: dwc: Driver clean-ups | expand |
On 8/21/2020 9:23 AM, Rob Herring wrote: > In preparation to allow drivers to set their own root and child pci_ops > instead of using the DWC specific config space ops, we need to make > the pci_host_bridge pointer available and move setting the bridge->ops > and bridge->child_ops pointer to before the .host_init() hook. > > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 15 ++++++++++----- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 1d98554db009..b626cc7cd43a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -344,6 +344,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (!bridge) > return -ENOMEM; > > + pp->bridge = bridge; > + > /* Get the I/O and memory ranges from DT */ > resource_list_for_each_entry(win, &bridge->windows) { > switch (resource_type(win->res)) { > @@ -445,6 +447,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > } > } > > + /* Set default bus ops */ > + bridge->ops = &dw_pcie_ops; > + bridge->child_ops = &dw_pcie_ops; > + > if (pp->ops->host_init) { > ret = pp->ops->host_init(pp); > if (ret) > @@ -452,7 +458,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > } > > bridge->sysdata = pp; > - bridge->ops = &dw_pcie_ops; > > ret = pci_scan_root_bus_bridge(bridge); > if (ret) > @@ -654,11 +659,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > dw_pcie_writel_dbi(pci, PCI_COMMAND, val); > > /* > - * If the platform provides ->rd_other_conf, it means the platform > - * uses its own address translation component rather than ATU, so > - * we should not program the ATU here. > + * If the platform provides its own child bus config accesses, it means > + * the platform uses its own address translation component rather than > + * ATU, so we should not program the ATU here. It is possible that a platform can have its own translation for configuration accesses and use DWC's ATU for memory/IO address translations. IMHO, ATU setup for memory/IO address translations shouldn't be skipped based on platform's '->rd_other_conf' implementation. Ex:- A platform can implement configuration space access through the ECAM mechanism yet choose to use ATU for memory/IO address translations. Thanks, Vidya Sagar > */ > - if (!pp->ops->rd_other_conf) { > + if (pp->bridge->child_ops == &dw_pcie_ops && !pp->ops->rd_other_conf) { > dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0, > PCIE_ATU_TYPE_MEM, pp->mem_base, > pp->mem_bus_addr, pp->mem_size); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index f911760dcc69..8b8ea5f3e7af 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -200,6 +200,7 @@ struct pcie_port { > u32 num_vectors; > u32 irq_mask[MAX_MSI_CTRLS]; > struct pci_bus *root_bus; > + struct pci_host_bridge *bridge; > raw_spinlock_t lock; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; >
On Sun, Aug 8, 2021 at 9:13 AM Vidya Sagar <vidyas@nvidia.com> wrote: > > > > On 8/21/2020 9:23 AM, Rob Herring wrote: > > In preparation to allow drivers to set their own root and child pci_ops > > instead of using the DWC specific config space ops, we need to make > > the pci_host_bridge pointer available and move setting the bridge->ops > > and bridge->child_ops pointer to before the .host_init() hook. > > > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > drivers/pci/controller/dwc/pcie-designware-host.c | 15 ++++++++++----- > > drivers/pci/controller/dwc/pcie-designware.h | 1 + > > 2 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 1d98554db009..b626cc7cd43a 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -344,6 +344,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > > if (!bridge) > > return -ENOMEM; > > > > + pp->bridge = bridge; > > + > > /* Get the I/O and memory ranges from DT */ > > resource_list_for_each_entry(win, &bridge->windows) { > > switch (resource_type(win->res)) { > > @@ -445,6 +447,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > > } > > } > > > > + /* Set default bus ops */ > > + bridge->ops = &dw_pcie_ops; > > + bridge->child_ops = &dw_pcie_ops; > > + > > if (pp->ops->host_init) { > > ret = pp->ops->host_init(pp); > > if (ret) > > @@ -452,7 +458,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > > } > > > > bridge->sysdata = pp; > > - bridge->ops = &dw_pcie_ops; > > > > ret = pci_scan_root_bus_bridge(bridge); > > if (ret) > > @@ -654,11 +659,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > dw_pcie_writel_dbi(pci, PCI_COMMAND, val); > > > > /* > > - * If the platform provides ->rd_other_conf, it means the platform > > - * uses its own address translation component rather than ATU, so > > - * we should not program the ATU here. > > + * If the platform provides its own child bus config accesses, it means > > + * the platform uses its own address translation component rather than > > + * ATU, so we should not program the ATU here. > It is possible that a platform can have its own translation for > configuration accesses and use DWC's ATU for memory/IO address > translations. IMHO, ATU setup for memory/IO address translations > shouldn't be skipped based on platform's '->rd_other_conf' > implementation. Ex:- A platform can implement configuration space access > through the ECAM mechanism yet choose to use ATU for memory/IO address > translations. A platform could, but none of them upstream do that. I'm all for doing ECAM setup (in the kernel) if possible. That could be in the DWC core with a feature flag the platform can set or something to enable it if we do that. It could be based on the config space size as well. I'm not sure what else determines whether ECAM can work besides having enough address space and at least 3 outbound iATU windows. Rob
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 1d98554db009..b626cc7cd43a 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -344,6 +344,8 @@ int dw_pcie_host_init(struct pcie_port *pp) if (!bridge) return -ENOMEM; + pp->bridge = bridge; + /* Get the I/O and memory ranges from DT */ resource_list_for_each_entry(win, &bridge->windows) { switch (resource_type(win->res)) { @@ -445,6 +447,10 @@ int dw_pcie_host_init(struct pcie_port *pp) } } + /* Set default bus ops */ + bridge->ops = &dw_pcie_ops; + bridge->child_ops = &dw_pcie_ops; + if (pp->ops->host_init) { ret = pp->ops->host_init(pp); if (ret) @@ -452,7 +458,6 @@ int dw_pcie_host_init(struct pcie_port *pp) } bridge->sysdata = pp; - bridge->ops = &dw_pcie_ops; ret = pci_scan_root_bus_bridge(bridge); if (ret) @@ -654,11 +659,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp) dw_pcie_writel_dbi(pci, PCI_COMMAND, val); /* - * If the platform provides ->rd_other_conf, it means the platform - * uses its own address translation component rather than ATU, so - * we should not program the ATU here. + * If the platform provides its own child bus config accesses, it means + * the platform uses its own address translation component rather than + * ATU, so we should not program the ATU here. */ - if (!pp->ops->rd_other_conf) { + if (pp->bridge->child_ops == &dw_pcie_ops && !pp->ops->rd_other_conf) { dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0, PCIE_ATU_TYPE_MEM, pp->mem_base, pp->mem_bus_addr, pp->mem_size); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index f911760dcc69..8b8ea5f3e7af 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -200,6 +200,7 @@ struct pcie_port { u32 num_vectors; u32 irq_mask[MAX_MSI_CTRLS]; struct pci_bus *root_bus; + struct pci_host_bridge *bridge; raw_spinlock_t lock; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); };
In preparation to allow drivers to set their own root and child pci_ops instead of using the DWC specific config space ops, we need to make the pci_host_bridge pointer available and move setting the bridge->ops and bridge->child_ops pointer to before the .host_init() hook. Cc: Jingoo Han <jingoohan1@gmail.com> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/pci/controller/dwc/pcie-designware-host.c | 15 ++++++++++----- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 11 insertions(+), 5 deletions(-)