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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Aug 21, 2020 at 11:57:47AM -0600, Rob Herring wrote:
> 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:
> >
> > > +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.

Oh, OK.  If you can solve this as you outlined above, that's fine with
me.  It didn't look like a common pattern yet, but maybe it will be.

Thanks for chiming in.  I didn't have a good idea for how to fix the
file-scope variable problem.

Bjorn