Re: [PATCH v15 2/2] PCI: microchip: Add host driver for Microchip PCIe controller

[Rob Herring <robh@kernel.org>]

On Thu, Aug 20, 2020 at 12:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Aug 19, 2020 at 04:33:10PM +0000, Daire.McNamara@microchip.com wrote:
>
> > + *   pcie-altera.c
> > + */
>
> Add a blank line here.
>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/module.h>
>
> > +static struct mc_port *port;
>
> This file scope item is not ideal.  It might work in your situation if
> you can never have more than one device, but it's not a pattern we
> want other people to copy.

Indeed.

> I think I sort of see how it works:
>
>   mc_pci_host_probe
>     pci_host_common_probe
>       ops = of_device_get_match_data()              # mc_ecam_ops
>       gen_pci_init(..., ops)
>         pci_ecam_create(..., ops)
>           ops->init                                 # mc_ecam_ops.init
>             mc_platform_init(pci_config_window *)
>               port = devm_kzalloc(...)              # initialized
>     mc_setup_windows
>       bridge_base_addr = port->axi_base_addr + ...  # used
>
> And you're using the file scope "port" because mc_platform_init()
> doesn't have a pointer to the platform_device.

This is a simple fix. Move platform_set_drvdata to just after
devm_pci_alloc_host_bridge() in pci_host_common_probe(). (Don't fall
into the 'platform problem'[1] and work-around the core code.)

Then pci_host_common_probe can be called directly and mc_setup_windows
can be moved to mc_platform_init().

> But I think this
> abuses the pci_ecam_ops design to do host bridge initialization that
> it is not intended for.

What init should be done then? IMO, given this driver is using ECAM
support already and it's all one time init that fits into init(), it
seems like a fit to me.

> I'd suggest copying the host bridge init design from somewhere else.
> tango_pcie_probe() also calls pci_host_common_probe(), so maybe that's
> a good place.  But the fact that it's the *only* such caller makes me
> think it might not be the best thing to copy.
>
> Rob has been working in this area and probably has better insight.

It was my suggestion to move to the ECAM init. Prior versions had its own probe.

One question I've been wrestling with is if we can do all the firmware
setup for 'generic ECAM' on the same h/w we have drivers for, then
shouldn't we make the kernel driver create an ECAM setup? There's also
the question of why do root port config accesses go thru standard ECAM
config space in ECAM mode, but use different address space for
non-ECAM setup? DWC for example can probably do ECAM on more
platforms. There's some that can't which are generally ones w/o the
iATU or enough iATU entries or not enough memory space. The difference
from a config space standpoint is just DT configuration (in the case
of DWC with an iATU, the config space and memory space DT entries are
just iATU configuration not h/w constraints).

I think we have drivers too much in control of their initialization
ordering and things should be more the other way around with core code
calling h/w ops to do specific things. I don't have any grand plan
yet, but perhaps the generic/common host stuff evolves beyond just
ECAM and almost ECAM. The generic host would just have no h/w ops.

Rob

[1] https://lwn.net/Articles/443531/