diff mbox

PCI: Generic Configuration Access Mechanism support

Message ID 1400422125-2209-1-git-send-email-sthokal@xilinx.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Srikanth Thokala May 18, 2014, 2:08 p.m. UTC
This patch adds support for a generic CAM and ECAM configuration
space accesses.

Signed-off-by: Srikanth Thokala <sthokal@xilinx.com>
---
This patch is created with reference from Will's patch series:
1/3 - "ARM: kconfig: allow PCI support to be selected with ARCH_MULTIPLATFORM"
2/3 - "PCI: ARM: add support for generic PCI host controller"
3/3 - "MAINTAINERS: add entry for generic PCI host controller driver"
---
 drivers/pci/Makefile  |    2 +-
 drivers/pci/pci-cfg.c |  162 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h   |   34 +++++++++++
 3 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/pci-cfg.c

Comments

Will Deacon May 19, 2014, 4:32 p.m. UTC | #1
On Sun, May 18, 2014 at 03:08:45PM +0100, Srikanth Thokala wrote:
> This patch adds support for a generic CAM and ECAM configuration
> space accesses.

Looks good to me, thanks Srikanth!

Arnd: is this the sort of thing you were expecting? The
->is_valid_cfg_access callback is a bit horrible, but I can't think of
anything better.

I can include this in the next posting of my PCI patches.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 19, 2014, 4:59 p.m. UTC | #2
On Monday 19 May 2014 17:32:23 Will Deacon wrote:
> On Sun, May 18, 2014 at 03:08:45PM +0100, Srikanth Thokala wrote:
> > This patch adds support for a generic CAM and ECAM configuration
> > space accesses.
> 
> Looks good to me, thanks Srikanth!
> 
> Arnd: is this the sort of thing you were expecting? The
> ->is_valid_cfg_access callback is a bit horrible, but I can't think of
> anything better.
> 
> I can include this in the next posting of my PCI patches.

Yes, this is roughly what I had in mind, thanks a lot!

I'll comment a bit more on the patch itself

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 19, 2014, 5:03 p.m. UTC | #3
On Sunday 18 May 2014 19:38:45 Srikanth Thokala wrote:
> +
> +	if (cfg->ops->is_valid_cfg_access) {
> +		if (!cfg->ops->is_valid_cfg_access(bus, devfn)) {
> +			*val = PCI_CFG_INVALID_DEVFN;
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +		}
> +	}

Can you explain why this callback is needed? If the space for the
bus is mapped, any access should just work.

> +
> +/* Generic PCI CAM/ECAM Configuration Bus Operations */
> +
> +struct pci_cfg_bus_ops pci_cfg_cam_bus_ops = {
> +	.bus_shift	= PCI_CFG_CAM_BUS_NUM,
> +	.map_bus	= pci_cfg_map_bus_cam,
> +};
> +EXPORT_SYMBOL_GPL(pci_cfg_cam_bus_ops);
> +
> +struct pci_cfg_bus_ops pci_cfg_ecam_bus_ops = {
> +	.bus_shift	= PCI_CFG_ECAM_BUS_NUM,
> +	.map_bus	= pci_cfg_map_bus_ecam,
> +};
> +EXPORT_SYMBOL_GPL(pci_cfg_ecam_bus_ops);
> +
> +struct pci_ops pci_cfg_ops = {
> +	.read	= pci_cfg_read,
> +	.write	= pci_cfg_write,
> +};
> +EXPORT_SYMBOL_GPL(pci_cfg_ops);


If we can find a way to remove the is_valid_cfg_access() check, we're
probably better off removing the cfg_bus_ops as well, and exporting
two sets of pci_ops. There will be a little more duplication here, but
also less complexity in this module, and more importantly in the drivers
using it.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srikanth Thokala May 20, 2014, 2:31 p.m. UTC | #4
Hi Arnd,

On Mon, May 19, 2014 at 10:33 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 18 May 2014 19:38:45 Srikanth Thokala wrote:
>> +
>> +     if (cfg->ops->is_valid_cfg_access) {
>> +             if (!cfg->ops->is_valid_cfg_access(bus, devfn)) {
>> +                     *val = PCI_CFG_INVALID_DEVFN;
>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>> +             }
>> +     }
>
> Can you explain why this callback is needed? If the space for the
> bus is mapped, any access should just work.

As I was explaining to Will, there are some controllers which doesn't
return FF's
when a device is not found on the bus (as per the PCI specification) and
accessing such a device address from the kernel results in an external abort.
So, I added this additional logic in my driver to bypass this and
return FF's. Our IP
and even other controllers like Tegra, Renesas have similar implementation.
We cant think of a better solution and please let you us know if you
have any inputs.

Thanks
Srikanth

>
>> +
>> +/* Generic PCI CAM/ECAM Configuration Bus Operations */
>> +
>> +struct pci_cfg_bus_ops pci_cfg_cam_bus_ops = {
>> +     .bus_shift      = PCI_CFG_CAM_BUS_NUM,
>> +     .map_bus        = pci_cfg_map_bus_cam,
>> +};
>> +EXPORT_SYMBOL_GPL(pci_cfg_cam_bus_ops);
>> +
>> +struct pci_cfg_bus_ops pci_cfg_ecam_bus_ops = {
>> +     .bus_shift      = PCI_CFG_ECAM_BUS_NUM,
>> +     .map_bus        = pci_cfg_map_bus_ecam,
>> +};
>> +EXPORT_SYMBOL_GPL(pci_cfg_ecam_bus_ops);
>> +
>> +struct pci_ops pci_cfg_ops = {
>> +     .read   = pci_cfg_read,
>> +     .write  = pci_cfg_write,
>> +};
>> +EXPORT_SYMBOL_GPL(pci_cfg_ops);
>
>
> If we can find a way to remove the is_valid_cfg_access() check, we're
> probably better off removing the cfg_bus_ops as well, and exporting
> two sets of pci_ops. There will be a little more duplication here, but
> also less complexity in this module, and more importantly in the drivers
> using it.
>
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 21, 2014, 7:52 a.m. UTC | #5
On Tuesday 20 May 2014 20:01:01 Srikanth Thokala wrote:
> On Mon, May 19, 2014 at 10:33 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday 18 May 2014 19:38:45 Srikanth Thokala wrote:
> >> +
> >> +     if (cfg->ops->is_valid_cfg_access) {
> >> +             if (!cfg->ops->is_valid_cfg_access(bus, devfn)) {
> >> +                     *val = PCI_CFG_INVALID_DEVFN;
> >> +                     return PCIBIOS_DEVICE_NOT_FOUND;
> >> +             }
> >> +     }
> >
> > Can you explain why this callback is needed? If the space for the
> > bus is mapped, any access should just work.
> 
> As I was explaining to Will, there are some controllers which doesn't
> return FF's
> when a device is not found on the bus (as per the PCI specification) and
> accessing such a device address from the kernel results in an external abort.
> So, I added this additional logic in my driver to bypass this and
> return FF's. Our IP
> and even other controllers like Tegra, Renesas have similar implementation.
> We cant think of a better solution and please let you us know if you
> have any inputs.

Does your hardware need this? My first response would otherwise be to
treat that as noncompliant and not handle this case in the generic implementation.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srikanth Thokala May 21, 2014, 8:54 a.m. UTC | #6
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Wednesday, May 21, 2014 1:23 PM
> To: Srikanth Thokala
> Cc: Bjorn Helgaas; will.deacon@arm.com; Michal Simek; linux-
> kernel@vger.kernel.org; linux-pci@vger.kernel.org
> Subject: Re: [PATCH] PCI: Generic Configuration Access Mechanism support
>
> On Tuesday 20 May 2014 20:01:01 Srikanth Thokala wrote:
> > On Mon, May 19, 2014 at 10:33 PM, Arnd Bergmann <arnd@arndb.de>
> wrote:
> > > On Sunday 18 May 2014 19:38:45 Srikanth Thokala wrote:
> > >> +
> > >> +     if (cfg->ops->is_valid_cfg_access) {
> > >> +             if (!cfg->ops->is_valid_cfg_access(bus, devfn)) {
> > >> +                     *val = PCI_CFG_INVALID_DEVFN;
> > >> +                     return PCIBIOS_DEVICE_NOT_FOUND;
> > >> +             }
> > >> +     }
> > >
> > > Can you explain why this callback is needed? If the space for the
> > > bus is mapped, any access should just work.
> >
> > As I was explaining to Will, there are some controllers which doesn't
> > return FF's when a device is not found on the bus (as per the PCI
> > specification) and accessing such a device address from the kernel
> > results in an external abort.
> > So, I added this additional logic in my driver to bypass this and
> > return FF's. Our IP and even other controllers like Tegra, Renesas
> > have similar implementation.
> > We cant think of a better solution and please let you us know if you
> > have any inputs.
>
> Does your hardware need this? My first response would otherwise be to
> treat that as noncompliant and not handle this case in the generic
> implementation.

Yes, my hardware needs this additional logic.

Srikanth

>
>       Arnd


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index e04fe2d..37cfc33 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -4,7 +4,7 @@ 
 
 obj-y		+= access.o bus.o probe.o host-bridge.o remove.o pci.o \
 			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
-			irq.o vpd.o setup-bus.o vc.o
+			irq.o vpd.o setup-bus.o vc.o pci-cfg.o
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_SYSFS) += slot.o
 
diff --git a/drivers/pci/pci-cfg.c b/drivers/pci/pci-cfg.c
new file mode 100644
index 0000000..2b15fe4
--- /dev/null
+++ b/drivers/pci/pci-cfg.c
@@ -0,0 +1,162 @@ 
+/*
+ * PCI generic configuration access mechanism
+ *
+ * Copyright (C) 2014 ARM Limited
+ * Copyright (c) 2014 Xilinx, Inc.
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/of_pci.h>
+
+/* CAM definitions */
+#define PCI_CFG_CAM_BUS_NUM	16
+#define PCI_CFG_CAM_DEV_NUM	8
+
+/* ECAM definitions */
+#define PCI_CFG_ECAM_BUS_NUM	20
+#define PCI_CFG_ECAM_DEV_NUM	12
+
+/* Invalid device/function value */
+#define PCI_CFG_INVALID_DEVFN	0xFFFFFFFF
+
+/**
+ * pci_cfg_map_bus_cam - Get the CAM based configuration space address
+ * @bus: PCI Bus pointer
+ * @devfn: Device/Function
+ * @where: Offset from base
+ *
+ * Return: Configuration Space address
+ */
+static void __iomem *pci_cfg_map_bus_cam(struct pci_bus *bus,
+					 unsigned int devfn,
+					 int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pci_cfg_windows *cfg = sys->private_data;
+	resource_size_t idx = bus->number - cfg->bus_range.start;
+
+	return cfg->win[idx] + ((devfn << PCI_CFG_CAM_DEV_NUM) | where);
+}
+
+/**
+ * pci_cfg_map_bus_ecam - Get the ECAM based configuration space address
+ * @bus: PCI bus pointer
+ * @devfn: Device/Function
+ * @where: Offset from base
+ *
+ * Return: Configuration space address
+ */
+static void __iomem *pci_cfg_map_bus_ecam(struct pci_bus *bus,
+					  unsigned int devfn,
+					  int where)
+{
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pci_cfg_windows *cfg = sys->private_data;
+	resource_size_t idx = bus->number - cfg->bus_range.start;
+
+	return cfg->win[idx] + ((devfn << PCI_CFG_ECAM_DEV_NUM) | where);
+}
+
+/**
+ * pci_cfg_read - Read configuration space
+ * @bus: PCI bus pointer
+ * @devfn: Device/function
+ * @where: Offset from base
+ * @size: Byte/word/dword
+ * @val: Value to be read
+ *
+ * Return: PCIBIOS_SUCCESSFUL on success
+ *	   PCIBIOS_DEVICE_NOT_FOUND on failure
+ */
+static int pci_cfg_read(struct pci_bus *bus, unsigned int devfn,
+			int where, int size, unsigned int *val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pci_cfg_windows *cfg = sys->private_data;
+
+	if (cfg->ops->is_valid_cfg_access) {
+		if (!cfg->ops->is_valid_cfg_access(bus, devfn)) {
+			*val = PCI_CFG_INVALID_DEVFN;
+			return PCIBIOS_DEVICE_NOT_FOUND;
+		}
+	}
+
+	addr = cfg->ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	default:
+		*val = readl(addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+/**
+ * pci_cfg_write - Write configuration space
+ * @bus: PCI bus pointer
+ * @devfn: Device/function
+ * @where: Offset from base
+ * @size: Byte/word/dword
+ * @val: Value to write
+ *
+ * Return: PCIBIOS_SUCCESSFUL on success
+ *	   PCIBIOS_DEVICE_NOT_FOUND on failure
+ */
+static int pci_cfg_write(struct pci_bus *bus, unsigned int devfn,
+			 int where, int size, unsigned int val)
+{
+	void __iomem *addr;
+	struct pci_sys_data *sys = bus->sysdata;
+	struct pci_cfg_windows *cfg = sys->private_data;
+
+	if (cfg->ops->is_valid_cfg_access)
+		if (!cfg->ops->is_valid_cfg_access(bus, devfn))
+			return PCIBIOS_DEVICE_NOT_FOUND;
+
+	addr = cfg->ops->map_bus(bus, devfn, where);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	default:
+		writel(val, addr);
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+/* Generic PCI CAM/ECAM Configuration Bus Operations */
+
+struct pci_cfg_bus_ops pci_cfg_cam_bus_ops = {
+	.bus_shift	= PCI_CFG_CAM_BUS_NUM,
+	.map_bus	= pci_cfg_map_bus_cam,
+};
+EXPORT_SYMBOL_GPL(pci_cfg_cam_bus_ops);
+
+struct pci_cfg_bus_ops pci_cfg_ecam_bus_ops = {
+	.bus_shift	= PCI_CFG_ECAM_BUS_NUM,
+	.map_bus	= pci_cfg_map_bus_ecam,
+};
+EXPORT_SYMBOL_GPL(pci_cfg_ecam_bus_ops);
+
+struct pci_ops pci_cfg_ops = {
+	.read	= pci_cfg_read,
+	.write	= pci_cfg_write,
+};
+EXPORT_SYMBOL_GPL(pci_cfg_ops);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aab57b4..6ebe21e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1806,4 +1806,38 @@  static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
  */
 struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
 
+/**
+ * struct pci_cfg_bus_ops - PCI bus configuration operations
+ * @bus_shift: Bus number
+ * @map_bus: Function pointer to get the configuration space address
+ * @is_valid_cfg_access: Function pointer to check for a valid device/function
+ */
+struct pci_cfg_bus_ops {
+	u32 bus_shift;
+	void __iomem *(*map_bus)(struct pci_bus *, unsigned int, int);
+	/*
+	 * This function pointer is to check if we are addressing a valid
+	 * device's function under a particular bus.
+	 */
+	int (*is_valid_cfg_access)(struct pci_bus *, unsigned int);
+};
+
+/**
+ * struct pci_cfg_windows - PCI bus configuration memory windows
+ * @res: Configuration space resource
+ * @bus_range: Bus range
+ * @win: Configuration space memory windows
+ * @ops: PCI bus configuration operations
+ */
+struct pci_cfg_windows {
+	struct resource	res;
+	struct resource	bus_range;
+	void __iomem **win;
+	struct pci_cfg_bus_ops *ops;
+};
+
+extern struct pci_ops pci_cfg_ops;
+extern struct pci_cfg_bus_ops pci_cfg_ecam_bus_ops;
+extern struct pci_cfg_bus_ops pci_cfg_cam_bus_ops;
+
 #endif /* LINUX_PCI_H */