On Mon, Jan 5, 2015 at 3:16 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Sunday 04 January 2015 20:19:34 Rob Herring wrote:
>> Many PCI controllers' configuration space accesses are memory mapped
>> varying only in address calculation and access checks. There are 2 main
>> access methods: a decoded address space such as ECAM or a single address
>> and data register similar to x86. This implementation can support both
>> cases as well as be used in cases that need additional pre or post access
>> handling.
>>
>> A new pci_ops member map_bus is introduced which can do access checks and
>> any necessary setup. It returns the address to use for the configuration
>> space access. The access types supported are 32-bit only accesses or
>> correct byte, word, or dword sized accesses.
>>
>> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>
> I think this looks very nice, and I don't mind using it as-is, but I'd
> like to put up some variations for discussions so we get the best
> implementation -- we should try not to change it again soon if someone
> comes up with a slightly better way later ;-)
>
>> I've converted a few drivers already. I'll send patches for them after
>> some feedback on this. Most already have some function similar to what is
>> needed for map_bus, so the conversion is pretty simple. This certainly
>> isn't a complete list of possible users. The diffstat so far looks like
>> this:
>>
>> arch/arm/mach-cns3xxx/pcie.c | 46 +++-------------------
>> arch/arm/mach-integrator/pci_v3.c | 61 +++---------------------------
>> arch/arm/mach-ks8695/pci.c | 75 +++---------------------------------
>> arch/arm/mach-sa1100/pci-nanoengine.c | 94 ++++-----------------------------------------
>> arch/powerpc/platforms/powermac/pci.c | 206 +++++++++++++++++++--------------------------------------------------------------------------------
>> arch/powerpc/sysdev/fsl_pci.c | 46 ++--------------------
>> drivers/pci/host/pci-host-generic.c | 51 ++-----------------------
>> drivers/pci/host/pci-rcar-gen2.c | 51 ++-----------------------
>> drivers/pci/host/pci-tegra.c | 55 ++-------------------------
>> drivers/pci/host/pci-xgene.c | 150 +++++-------------------------------------------------------------------
>> drivers/pci/host/pcie-xilinx.c | 88 +++++-------------------------------------
>> 11 files changed, 93 insertions(+), 830 deletions(-)
>
> Awesome!
>
>> +int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>> + int where, int size, u32 *val)
>> +{
>> + void __iomem *addr;
>> +
>> + addr = bus->ops->map_bus(bus, devfn, where);
>> + if (!addr) {
>> + *val = ~0;
>> + return PCIBIOS_DEVICE_NOT_FOUND;
>> + }
>> +
>> + if (size == 1)
>> + *val = readb(addr);
>> + else if (size == 2)
>> + *val = readw(addr);
>> + else
>> + *val = readl(addr);
>> +
>> + return PCIBIOS_SUCCESSFUL;
>> +}
>
> PCI host controller drivers can be loadable modules these days, so
> the functions clearly need to be exported.
Ah, yes.
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 360a966..e7fd519 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -560,6 +560,7 @@ static inline int pcibios_err_to_errno(int err)
>> /* Low-level architecture-dependent routines */
>>
>> struct pci_ops {
>> + void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
>> int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
>> int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
>> };
>
> In various other contexts, we have recently discussed adding new callbacks
> to struct pci_host_bridge, or an operations structure below it. I don't see
> a strong reason for one place or the other, but maybe someone else does.
> If we put it into pci_host_bridge_ops, the first argument would need to
> be the pci_host_bridge pointer of course.
I think it makes sense to keep map_bus together with read/write. Given
they are all host specific functions being part of pci_host_bridge
would make some sense. However, that would be a pretty painful change
across the tree (Have you seen how many PCI host implementations MIPS
has?).
> For the common map_bus implementations, it would also be nice to put them
> into the same file as your new access functions, but then we need a common
> location to store at least one __iomem pointer. I guess that place could
> either be struct pci_host_bridge or struct pci_bus. In theory, struct pci_ops
> would work too, but then we could no longer mark it 'const' in host bridge
> drivers.
>
> If we have a common set of map_bus functions, we can even export the
> pci_ops structures from drivers/pci/access.c:
>
> const struct pci_ops pci_generic_ecam_ops = {
> .map_bus = ecam_map_bus,
> .read = pci_generic_config_read,
> .write = pci_generic_config_write,
> };
> EXPORT_SYMBOL_GPL(pci_generic_ecam_ops);
>
> That could of course be done in a follow-up patch, it doesn't have to be
> part of your patch, but it would be good to be prepared.
Right, this is what I had in mind for CAM/ECAM. I didn't go this far
because a lot of the map_bus functions do various checks to prevent
certain accesses. Of what I've found, I think only generic host and
Xilinx drivers could be converted to a generic ECAM map_bus. Others
check bus number and/or device number or link-up status or have a
fixup for certain registers, for example. I'm not sure how much of it
is unnecessary or could be common.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
