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

[Rob Herring <robh@kernel.org>]

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