Re: [PATCH v7 4/6] pci: Introduce a domain number for pci_host_bridge.

[Arnd Bergmann <arnd@arndb.de>]

On Friday 11 April 2014 15:01:09 Benjamin Herrenschmidt wrote:
> On Thu, 2014-04-10 at 22:46 +0200, Arnd Bergmann wrote:
> 
> > Half of it ;-)
> > 
> > I think it would be better to not have an architecture specific data
> > structure, just like it would be better not to have architecture specific
> > pcibios_* functions that get called by the PCI core. Note that the
> > architecture specific functions are the ones that rely on the architecture
> > specific data structures as well. If they only use the common fields,
> > it should also be possible to share the code.
> 
> I don't understand... we'll never get rid of architecture specific hooks
> in one form or another.
> 
> We'll always need to some things in an architecture or host-bridge
> specific way. Now if you don't want to call them arch hooks, then call
> them host bridge ops, but they are needed and thus they will need some
> kind of architecture specific extension to the base host bridge 
> structure.

Absolutely right. The thing I'd like to get rid of in the long run
is global functions defined in the architecture code that are called
by core code for host bridge specific functionality. In a lot of
cases, they should not be needed if we can express the same things
in a generic way. In other cases, we can use function pointers that
are set at the time that the host bridge is registered.

> EEH is one big nasty example on powerpc.
> 
> Another random one that happens to be hot in my brain right now because
> we just finished debugging it: On powernv, we are just fixing a series
> of bugs caused by the generic code trying to do hot resets on PCIe "by
> hand" by directly toggling the secondary reset register in bridges.
> 
> Well, on our root complexes, this triggers a link loss which triggers
> a fatal EEH "ER_all" interrupt which we escalate into a fence and all
> hell breaks loose.
> 
> We need to mask some error traps in the hardware before doing something
> that can cause an intentional link loss... and unmask them when done.
> (Among other things, there are other issues on P7 with hot reset).
> 
> So hot reset must be an architecture hook.

This sounds to me very much host bridge specific, not architecture specific.
If you have the same host bridge in an ARM system, you'd want the same
things to happen, and if you have another host bridge on PowerPC, you
probably don't want that code to be called.

> PERST (fundamental reset) can *only* be a hook. The way to generate a
> PERST is not specified. In fact, on our machines, we have special GPIOs
> we can use to generate PERST on individual slots below a PLX bridge
> and a different methods for slots directly on a PHB.
> 
> Eventually most of those hooks land into firmware, and as such it's akin
> to ACPI which also keeps a separate state structure and a pile of hooks.

On PowerPC, there are currently a bunch of platform specific callbacks
in the ppc_md: pcibios_after_init, pci_exclude_device,
pcibios_fixup_resources, pcibios_fixup_bus, pcibios_enable_device_hook,
pcibios_fixup_phb, pcibios_window_alignment, and possibly some more.
There is some architecture specific code that gets called by the
PCI core, with the main purpose of calling into these.

On ARM32, we have a similar set of callbacks in the architecture
private pci_sys_data: swizzle, map_irq, align_resource, add_bus,
remove_bus, and some more callbacks for setup in the hw_pci
structure that is used at initialization time: setup, scan,
preinit, postinit. Again, these are called from architecture
specific code that gets called by the PCI core.

I'm sure some of the other architectures have similar things, most
of them probably less verbose because there is fewer variability
between subarchitectures.

I think a nice way to deal with these in the long run would be
to have a generic 'struct pci_host_bridge_ops' that can be defined
by the architecture or the platform, or a particular host bridge
driver. We'd have to define exactly which function pointers would
go in there, but a good start would be the set of functions that
are today provided by each architecture. The reset method you describe
above would also fit well into this.

A host bridge driver can fill out the pointers with its own functions,
or put platform or architecture specific function pointers in there,
that get called by the PCI core. There are multiple ways to deal with
default implementations here, one way would be that the core just
falls back to a generic implementation (which is currently the __weak
function) if it sees a NULL pointer. Another way would be to require
each driver to either fill out all pointers or none of them, in which
case we would use a default pci_host_bridge_ops struct that contains
the pointers to the global pcibios_*() functions.

	Arnd