diff mbox series

[v2,03/40] PCI: dwc: Allow overriding bridge pci_ops

Message ID 20200821035420.380495-4-robh@kernel.org (mailing list archive)
State New, archived
Headers show
Series PCI: dwc: Driver clean-ups | expand

Commit Message

Rob Herring Aug. 21, 2020, 3:53 a.m. UTC
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(-)

Comments

Vidya Sagar Aug. 8, 2021, 3:13 p.m. UTC | #1
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);
>   };
>
Rob Herring Aug. 9, 2021, 2:52 p.m. UTC | #2
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 mbox series

Patch

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);
 };