On Thu, Aug 20, 2020 at 12:10 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Wed, Aug 19, 2020 at 04:33:10PM +0000, Daire.McNamara@xxxxxxxxxxxxx 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/
