[PATCH] pci: introduce common pci config space accessors

[PATCH] pci: introduce common pci config space accessors

[Rob Herring]

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@kernel.org>
---

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(-)

Rob

 drivers/pci/access.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  | 11 +++++++
 2 files changed, 94 insertions(+)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 49dd766..9a29f7a 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -67,6 +67,89 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
 EXPORT_SYMBOL(pci_bus_write_config_word);
 EXPORT_SYMBOL(pci_bus_write_config_dword);

+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;
+}
+
+int pci_generic_config_write(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)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	if (size == 1)
+		writeb(val, addr);
+	else if (size == 2)
+		writew(val, addr);
+	else
+		writel(val, addr);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val)
+{
+	void __iomem *addr;
+
+	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!addr) {
+		*val = ~0;
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	*val = readl(addr);
+
+	if (size <= 2)
+		*val = (*val >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 val)
+{
+	void __iomem *addr;
+	u32 mask, tmp;
+
+	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!addr)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	if (size == 4) {
+		writel(val, addr);
+		return PCIBIOS_SUCCESSFUL;
+	} else {
+		mask = ~(((1 << (size * 8)) - 1) << ((where & 0x3) * 8));
+	}
+
+	tmp = readl(addr) & mask;
+	tmp |= val << ((where & 0x3) * 8);
+	writel(tmp, addr);
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
 /**
  * pci_bus_set_ops - Set raw operations of pci bus
  * @bus:	pci bus struct
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);
 };
@@ -857,6 +858,16 @@ int pci_bus_write_config_word(struct pci_bus *bus, unsigned int devfn,
 			      int where, u16 val);
 int pci_bus_write_config_dword(struct pci_bus *bus, unsigned int devfn,
 			       int where, u32 val);
+
+int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
+			    int where, int size, u32 *val);
+int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
+			    int where, int size, u32 val);
+int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
+			      int where, int size, u32 *val);
+int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 val);
+
 struct pci_ops *pci_bus_set_ops(struct pci_bus *bus, struct pci_ops *ops);

 static inline int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
--
2.1.0

Re: [PATCH] pci: introduce common pci config space accessors

[Arnd Bergmann]

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@kernel.org>

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.

> 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.

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.

	Arnd

Re: [PATCH] pci: introduce common pci config space accessors

[Rob Herring]

On Mon, Jan 5, 2015 at 3:16 AM, Arnd Bergmann <arnd@arndb.de> 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@kernel.org>
>
> 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

Re: [PATCH] pci: introduce common pci config space accessors

[Arnd Bergmann]

On Monday 05 January 2015 08:46:09 Rob Herring wrote:
> 
> >> 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?).

We could go wild and do both: if the bus has pci_ops, we use those,
otherwise we fall back to the map_bus/read/write from pci_host_bridge_ops.

> > 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.

How do you want to deal with the overrides? I don't see a way to
do that in map_bus (with the current definition) if the idea is that
for certain registers we return hardcoded values instead of accessing
mmio registers.

	Arnd

Re: [PATCH] pci: introduce common pci config space accessors

[Rob Herring]

On Mon, Jan 5, 2015 at 2:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 05 January 2015 08:46:09 Rob Herring wrote:
>>
>> >> 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?).
>
> We could go wild and do both: if the bus has pci_ops, we use those,
> otherwise we fall back to the map_bus/read/write from pci_host_bridge_ops.

True. I'll leave it for Bjorn to weigh in.

>> > 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.
>
> How do you want to deal with the overrides? I don't see a way to
> do that in map_bus (with the current definition) if the idea is that
> for certain registers we return hardcoded values instead of accessing
> mmio registers.

It is not done in map_bus unless you want all FFs by returning NULL,
but by simply by wrapping the generic call with a host specific read
or write function. Here's one example that modifies one field in a
register. This means you can do any pre or post processing you need.
Even the crazy stuff Integrator PCI does.

static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,
                                   int where, int size, u32 *val)
{
        u32 mask = (0x1ull << (size * 8)) - 1;
        int shift = (where % 4) * 8;

        ret = pci_generic_config_read32(bus, devfn, where, size, val);

        if (ret == PCIBIOS_SUCCESSFUL && bus->number == 0 && devfn == 0 &&
            (where & 0xffc) == PCI_CLASS_REVISION)
                /*
                 * RC's class is 0xb, but Linux PCI driver needs 0x604
                 * for a PCIe bridge. So we must fixup the class code
                 * to 0x604 here.
                 */
                *val = ((*val << shift) & 0xff) | (0x604 << 16)) >>
shift) & mask;

        return ret;
}

Rob

Re: [PATCH] pci: introduce common pci config space accessors

[Arnd Bergmann]

On Monday 05 January 2015 16:28:48 Rob Herring wrote:
> On Mon, Jan 5, 2015 at 2:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 05 January 2015 08:46:09 Rob Herring wrote:
> >> 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.
> >
> > How do you want to deal with the overrides? I don't see a way to
> > do that in map_bus (with the current definition) if the idea is that
> > for certain registers we return hardcoded values instead of accessing
> > mmio registers.
> 
> It is not done in map_bus unless you want all FFs by returning NULL,
> but by simply by wrapping the generic call with a host specific read
> or write function. Here's one example that modifies one field in a
> register. This means you can do any pre or post processing you need.
> Even the crazy stuff Integrator PCI does.
> 
> static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,
>                                    int where, int size, u32 *val)
> {
>         u32 mask = (0x1ull << (size * 8)) - 1;
>         int shift = (where % 4) * 8;
> 
>         ret = pci_generic_config_read32(bus, devfn, where, size, val);
> 
>         if (ret == PCIBIOS_SUCCESSFUL && bus->number == 0 && devfn == 0 &&
>             (where & 0xffc) == PCI_CLASS_REVISION)
>                 /*
>                  * RC's class is 0xb, but Linux PCI driver needs 0x604
>                  * for a PCIe bridge. So we must fixup the class code
>                  * to 0x604 here.
>                  */
>                 *val = ((*val << shift) & 0xff) | (0x604 << 16)) >>
> shift) & mask;
> 
>         return ret;
> }

Ah, I see. Yes this should work fine. 

I had no idea about the Integrator craziness. Does that actually work? ;-)
If I were to write a driver for this hardware, I'd probably give up and
either not support prefetchable memory space, or not support PIO.

	Arnd

Re: [PATCH] pci: introduce common pci config space accessors

[Andreas Mohr]

Hi,

> +	if (size == 1)
> +		*val = readb(addr);
> +	else if (size == 2)
> +		*val = readw(addr);
> +	else
> +		*val = readl(addr);
(pci_generic_config_read() and pci_generic_config_write())

switch()
default: WARN_ON(1);
?

Thanks,

Andreas Mohr

Re: [PATCH] pci: introduce common pci config space accessors

[Rob Herring]

On Mon, Jan 5, 2015 at 12:48 PM, Andreas Mohr <andi@lisas.de> wrote:
> Hi,
>
>> +     if (size == 1)
>> +             *val = readb(addr);
>> +     else if (size == 2)
>> +             *val = readw(addr);
>> +     else
>> +             *val = readl(addr);
> (pci_generic_config_read() and pci_generic_config_write())
>
> switch()
> default: WARN_ON(1);
> ?

If you look at PCI_OP_READ/PCI_OP_WRITE defines, the caller can only
have size of 1, 2 or 4. AFAIK, config accesses don't go thru any other
caller.

Rob