[PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

A lot of PCI host bridges require different methods for initiating
type 0 and type 1 config space accesses, leading to duplication of
code.

This adds support for the two different kinds at the pci_ops
level, with the newly added map_bridge/read_bridge/write_bridge
operations for type 1 accesses.

When these are not set, we fall back to the regular map_bus/read/write
operations, so all existing drivers keep working, and bridges that
have identical operations continue to only require one set.

In most cases, a driver will only have to override either map_bridge
or read_bridge/write_bridge but not both.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This is slightly refined over what I had in the "Add PCIe driver for
Rockchip Soc" thread earlier, but probably not the final version yet,
so I'd like to get more feedback on it.

In particular, I think it may be useful to add a third set of
functions for the config space of devices that are directly attached
to the host bridge, as those are sometimes (designware, rcar, mvebu)
yet again different from the host bridge itself and from all other
devices. On the other hand, that adds further complexity that we
may want to leave out of the common code, and I honestly can't
seem to come up for a catchy name form the callbacks.

 drivers/pci/access.c | 35 +++++++++++++++++++++++++++--------
 include/linux/pci.h  |  3 +++
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d11cdbb8fba3..263606ece211 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
 	u32 data = 0;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	raw_spin_lock_irqsave(&pci_lock, flags);			\
-	res = bus->ops->read(bus, devfn, pos, len, &data);		\
+	if (!bus->parent == 0 && bus->ops->read_bridge)			\
+		res = bus->ops->read_bridge(bus, devfn, pos, len, &data);	\
+	else								\
+		res = bus->ops->read(bus, devfn, pos, len, &data);	\
 	*value = (type)data;						\
-	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
+	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
@@ -48,8 +51,11 @@ int pci_bus_write_config_##size \
 	unsigned long flags;						\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
 	raw_spin_lock_irqsave(&pci_lock, flags);			\
-	res = bus->ops->write(bus, devfn, pos, len, value);		\
-	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
+	if (!bus->parent && bus->ops->write_bridge)			\
+		res = bus->ops->write_bridge(bus, devfn, pos, len, value);\
+	else								\
+		res = bus->ops->write(bus, devfn, pos, len, value);	\
+	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
 	return res;							\
 }
 
@@ -72,7 +78,11 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where);
+	if (!bus->parent && bus->ops->map_bridge)
+		addr = bus->ops->map_bridge(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where);
+
 	if (!addr) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -94,7 +104,10 @@ int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where);
+	if (!bus->parent && bus->ops->map_bridge)
+		addr = bus->ops->map_bridge(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where);
 	if (!addr)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
@@ -114,7 +127,10 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn,
 {
 	void __iomem *addr;
 
-	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!bus->parent && bus->ops->map_bridge)
+		addr = bus->ops->map_bridge(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
 	if (!addr) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
@@ -135,7 +151,10 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
 	void __iomem *addr;
 	u32 mask, tmp;
 
-	addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
+	if (!bus->parent && bus->ops->map_bridge)
+		addr = bus->ops->map_bridge(bus, devfn, where);
+	else
+		addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
 	if (!addr)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index df41c4645911..2b1d08771b36 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -580,6 +580,9 @@ 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);
+	void __iomem *(*map_bridge)(struct pci_bus *bus, unsigned int devfn, int where);
+	int (*read_bridge)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
+	int (*write_bridge)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
 };
 
 /*
-- 
2.7.0

[PATCH 2/3] pci: dw: use new config space accessors

[Arnd Bergmann]

The PCI core can now use separate callbacks for type 1 config
space accesses, so we can simplify the dw_pcie_wr_conf/dw_pcie_rd_conf
logic that multiplexes between the two kinds.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/pci/host/pcie-designware.c | 73 +++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index aafd766546f3..37e16c159719 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -573,13 +573,24 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
-static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+static int dw_pcie_rd_other_conf(struct pci_bus *bus,
 		u32 devfn, int where, int size, u32 *val)
 {
 	int ret, type;
 	u32 busdev, cfg_size;
 	u64 cpu_addr;
 	void __iomem *va_cfg_base;
+	struct pcie_port *pp = bus->sysdata;
+
+	/*
+	 * If there is no link, then there is no device.
+	 *
+	 * do not read more than one device on the bus directly attached
+	 * to RC's (Virtual Bridge's) DS side.
+	 */
+	if (!dw_pcie_link_up(pp) ||
+	    (bus->primary == pp->root_bus_nr && PCI_SLOT(devfn) > 0))
+		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (pp->ops->rd_other_conf)
 		return pp->ops->rd_other_conf(pp, bus, devfn, where, size, val);
@@ -610,13 +621,18 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	return ret;
 }
 
-static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+static int dw_pcie_wr_other_conf(struct pci_bus *bus,
 		u32 devfn, int where, int size, u32 val)
 {
 	int ret, type;
 	u32 busdev, cfg_size;
 	u64 cpu_addr;
 	void __iomem *va_cfg_base;
+	struct pcie_port *pp = bus->sysdata;
+
+	if (!dw_pcie_link_up(pp) ||
+	    (bus->primary == pp->root_bus_nr && PCI_SLOT(devfn) > 0))
+		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (pp->ops->wr_other_conf)
 		return pp->ops->wr_other_conf(pp, bus, devfn, where, size, val);
@@ -647,62 +663,29 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	return ret;
 }
 
-static int dw_pcie_valid_config(struct pcie_port *pp,
-				struct pci_bus *bus, int dev)
-{
-	/* If there is no link, then there is no device */
-	if (bus->number != pp->root_bus_nr) {
-		if (!dw_pcie_link_up(pp))
-			return 0;
-	}
-
-	/* access only one slot on each root port */
-	if (bus->number == pp->root_bus_nr && dev > 0)
-		return 0;
-
-	/*
-	 * do not read more than one device on the bus directly attached
-	 * to RC's (Virtual Bridge's) DS side.
-	 */
-	if (bus->primary == pp->root_bus_nr && dev > 0)
-		return 0;
-
-	return 1;
-}
-
-static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
+static int dw_pcie_rd_bridge_conf(struct pci_bus *bus, u32 devfn, int where,
 			int size, u32 *val)
 {
-	struct pcie_port *pp = bus->sysdata;
-
-	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) {
-		*val = 0xffffffff;
+	if (PCI_SLOT(devfn) > 0)
 		return PCIBIOS_DEVICE_NOT_FOUND;
-	}
-
-	if (bus->number == pp->root_bus_nr)
-		return dw_pcie_rd_own_conf(pp, where, size, val);
 
-	return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
+	return dw_pcie_rd_own_conf(bus->sysdata, where, size, val);
 }
 
-static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
+static int dw_pcie_wr_bridge_conf(struct pci_bus *bus, u32 devfn,
 			int where, int size, u32 val)
 {
-	struct pcie_port *pp = bus->sysdata;
-
-	if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0)
+	if (PCI_SLOT(devfn) > 0)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	if (bus->number == pp->root_bus_nr)
-		return dw_pcie_wr_own_conf(pp, where, size, val);
-
-	return dw_pcie_wr_other_conf(pp, bus, devfn, where, size, val);
+	return dw_pcie_wr_own_conf(bus->sysdata, where, size, val);
 }
 
 static struct pci_ops dw_pcie_ops = {
-	.read = dw_pcie_rd_conf,
-	.write = dw_pcie_wr_conf,
+	.read_bridge = dw_pcie_rd_bridge_conf,
+	.write_bridge = dw_pcie_wr_bridge_conf,
+	.read = dw_pcie_rd_other_conf,
+	.write = dw_pcie_wr_other_conf,
 };
 
 void dw_pcie_setup_rc(struct pcie_port *pp)
-- 
2.7.0

[PATCH 3/3] pci: mvebu: use bridge config operations

[Arnd Bergmann]

This converts the pci-mvebu host driver to use the newly added pci_ops
callbacks for handling the host config space.

After support for the emulator root bridge is removed, the normal operations
can use pci_generic_config_read/pci_generic_config_write and we just need
to provide a map_bus callback.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/pci/host/pci-mvebu.c | 158 ++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 116 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 6b451df6502c..9dff177e32eb 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -155,6 +155,11 @@ struct mvebu_pcie_port {
 	u32 saved_pcie_stat;
 };
 
+static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
+{
+	return sys->private_data;
+}
+
 static inline void mvebu_writel(struct mvebu_pcie_port *port, u32 val, u32 reg)
 {
 	writel(val, port->base + reg);
@@ -273,56 +278,6 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	mvebu_writel(port, mask, PCIE_MASK_OFF);
 }
 
-static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
-				 struct pci_bus *bus,
-				 u32 devfn, int where, int size, u32 *val)
-{
-	void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
-
-	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
-		     PCIE_CONF_ADDR_OFF);
-
-	switch (size) {
-	case 1:
-		*val = readb_relaxed(conf_data + (where & 3));
-		break;
-	case 2:
-		*val = readw_relaxed(conf_data + (where & 2));
-		break;
-	case 4:
-		*val = readl_relaxed(conf_data);
-		break;
-	}
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
-static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
-				 struct pci_bus *bus,
-				 u32 devfn, int where, int size, u32 val)
-{
-	void __iomem *conf_data = port->base + PCIE_CONF_DATA_OFF;
-
-	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
-		     PCIE_CONF_ADDR_OFF);
-
-	switch (size) {
-	case 1:
-		writeb(val, conf_data + (where & 3));
-		break;
-	case 2:
-		writew(val, conf_data + (where & 2));
-		break;
-	case 4:
-		writel(val, conf_data);
-		break;
-	default:
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-	}
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
 /*
  * Remove windows, starting from the largest ones to the smallest
  * ones.
@@ -624,6 +579,19 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+static int mvebu_pcie_rd_bridge_conf(struct pci_bus *bus, u32 devfn, int where,
+				     int size, u32 *val)
+{
+	struct mvebu_pcie *pcie = sys_to_pcie(bus->sysdata);
+	struct mvebu_pcie_port *port;
+
+	for (port = pcie->ports; port < &pcie->ports[pcie->nports]; port++)
+		if (port->devfn == devfn)
+			return mvebu_sw_pci_bridge_read(port, where, size, val);
+
+	return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
 /* Write to the PCI-to-PCI bridge configuration space */
 static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
 				     unsigned int where, int size, u32 value)
@@ -750,90 +718,48 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
-{
-	return sys->private_data;
-}
-
-static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
-						    struct pci_bus *bus,
-						    int devfn)
-{
-	int i;
-
-	for (i = 0; i < pcie->nports; i++) {
-		struct mvebu_pcie_port *port = &pcie->ports[i];
-
-		if (bus->number == 0 && port->devfn == devfn)
-			return port;
-		if (bus->number != 0 &&
-		    bus->number >= port->bridge.secondary_bus &&
-		    bus->number <= port->bridge.subordinate_bus)
-			return port;
-	}
-
-	return NULL;
-}
-
-/* PCI configuration space write function */
-static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
-			      int where, int size, u32 val)
+static int mvebu_pcie_wr_bridge_conf(struct pci_bus *bus, u32 devfn,
+				     int where, int size, u32 val)
 {
 	struct mvebu_pcie *pcie = sys_to_pcie(bus->sysdata);
 	struct mvebu_pcie_port *port;
-	int ret;
-
-	port = mvebu_pcie_find_port(pcie, bus, devfn);
-	if (!port)
-		return PCIBIOS_DEVICE_NOT_FOUND;
-
-	/* Access the emulated PCI-to-PCI bridge */
-	if (bus->number == 0)
-		return mvebu_sw_pci_bridge_write(port, where, size, val);
-
-	if (!mvebu_pcie_link_up(port))
-		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	/* Access the real PCIe interface */
-	ret = mvebu_pcie_hw_wr_conf(port, bus, devfn,
-				    where, size, val);
+	for (port = pcie->ports; port < &pcie->ports[pcie->nports]; port++)
+		if (port->devfn == devfn)
+			return mvebu_sw_pci_bridge_write(port, where, size, val);
 
-	return ret;
+	return PCIBIOS_DEVICE_NOT_FOUND;
 }
 
-/* PCI configuration space read function */
-static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
-			      int size, u32 *val)
+/* find normal config registers */
+static void __iomem *mvebu_pcie_map_conf(struct pci_bus *bus, u32 devfn, int where)
 {
 	struct mvebu_pcie *pcie = sys_to_pcie(bus->sysdata);
 	struct mvebu_pcie_port *port;
-	int ret;
+	u32 addr;
 
-	port = mvebu_pcie_find_port(pcie, bus, devfn);
-	if (!port) {
-		*val = 0xffffffff;
-		return PCIBIOS_DEVICE_NOT_FOUND;
-	}
+	for (port = pcie->ports; port < &pcie->ports[pcie->nports]; port++) {
+		if (bus->number >= port->bridge.secondary_bus &&
+		    bus->number <= port->bridge.subordinate_bus) {
+			if (!mvebu_pcie_link_up(port))
+				return NULL;
 
-	/* Access the emulated PCI-to-PCI bridge */
-	if (bus->number == 0)
-		return mvebu_sw_pci_bridge_read(port, where, size, val);
+			addr = PCIE_CONF_ADDR(bus->number, devfn, where);
+			mvebu_writel(port, addr, PCIE_CONF_ADDR_OFF);
 
-	if (!mvebu_pcie_link_up(port)) {
-		*val = 0xffffffff;
-		return PCIBIOS_DEVICE_NOT_FOUND;
+			return port->base + PCIE_CONF_DATA_OFF + (where & 3);
+		}
 	}
 
-	/* Access the real PCIe interface */
-	ret = mvebu_pcie_hw_rd_conf(port, bus, devfn,
-				    where, size, val);
-
-	return ret;
+	return NULL;
 }
 
 static struct pci_ops mvebu_pcie_ops = {
-	.read = mvebu_pcie_rd_conf,
-	.write = mvebu_pcie_wr_conf,
+	.read_bridge = mvebu_pcie_rd_bridge_conf,
+	.write_bridge = mvebu_pcie_wr_bridge_conf,
+	.map_bus = mvebu_pcie_map_conf,
+	.read = pci_generic_config_read,
+	.write = pci_generic_config_write,
 };
 
 static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
-- 
2.7.0

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Bjorn Helgaas]

Hi Arnd,

On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> A lot of PCI host bridges require different methods for initiating
> type 0 and type 1 config space accesses, leading to duplication of
> code.
> 
> This adds support for the two different kinds at the pci_ops
> level, with the newly added map_bridge/read_bridge/write_bridge
> operations for type 1 accesses.
> 
> When these are not set, we fall back to the regular map_bus/read/write
> operations, so all existing drivers keep working, and bridges that
> have identical operations continue to only require one set.

This adds new config accessor functions to struct pci_ops and makes
the callers responsible for figuring out which one to use.  The
benefit is to reduce code duplication in some host bridge drivers
(DesignWare and MVEBU so far).

>From a design perspective, I'm not comfortable with moving this burden
from the host bridge drivers to the callers of the config accessors.

The pci_ops struct is a pretty low-level thing, but it is declared in
include/linux/pci.h and while I think it's ugly to use it outside the
PCI core, there's nothing that actually prevents that, and there are
several places that do use it, including at least the ones below.  We
could argue that many of these don't need updating because they don't
need .read_bridge() accessors, but I don't think pci_ops users should
be making assumptions like that.

  arch/sparc/kernel/pci_schizo.c: pbm->pci_ops->read(pbm->pci_bus, 0, PCI_STATUS, 2, &stat);
  arch/x86/pci/common.c:          return raw_pci_ops->read(domain, bus, devfn, reg, len, val);
  arch/x86/pci/common.c:          return raw_pci_ext_ops->read(domain, bus, devfn, reg, len, val);
  arch/x86/pci/intel_mid_pci.c:           if (raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
  arch/x86/pci/intel_mid_pci.c:                   raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
  arch/x86/pci/intel_mid_pci.c:           raw_pci_ext_ops->read(domain, busnum, devfn,
  arch/x86/pci/intel_mid_pci.c:   return raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
  arch/x86/pci/mmconfig-shared.c: raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0xce, 2, &win);
  arch/x86/pci/mmconfig-shared.c: raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x48, 4, &pciexbar);
  arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), 0, 4, &l);
  arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), extcfg_regnum,
  arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, devfn, 0, 4, &l);
  drivers/pci/access.c:   res = bus->ops->read(bus, devfn, pos, len, &data);              \
  drivers/pci/access.c:   ret = dev->bus->ops->read(dev->bus, dev->devfn,                 \
  drivers/pci/access.c:   return dev->vpd->ops->read(dev, pos, count, buf);
  drivers/pci/pci.c:      bus->ops->read(bus, dev->devfn, PCI_COMMAND, 4, &cmd_status_dword);
  drivers/pci/pcie/aer/aer_inject.c:      rv = ops->read(bus, devfn, where, size, val);

> In most cases, a driver will only have to override either map_bridge
> or read_bridge/write_bridge but not both.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This is slightly refined over what I had in the "Add PCIe driver for
> Rockchip Soc" thread earlier, but probably not the final version yet,
> so I'd like to get more feedback on it.
> 
> In particular, I think it may be useful to add a third set of
> functions for the config space of devices that are directly attached
> to the host bridge, as those are sometimes (designware, rcar, mvebu)
> yet again different from the host bridge itself and from all other
> devices. On the other hand, that adds further complexity that we
> may want to leave out of the common code, and I honestly can't
> seem to come up for a catchy name form the callbacks.
> 
>  drivers/pci/access.c | 35 +++++++++++++++++++++++++++--------
>  include/linux/pci.h  |  3 +++
>  2 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d11cdbb8fba3..263606ece211 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
>  	u32 data = 0;							\
>  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
>  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> -	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> +	if (!bus->parent == 0 && bus->ops->read_bridge)			\

  if (!bus->parent && ...) ?

> +		res = bus->ops->read_bridge(bus, devfn, pos, len, &data);	\
> +	else								\
> +		res = bus->ops->read(bus, devfn, pos, len, &data);	\
>  	*value = (type)data;						\
> -	raw_spin_unlock_irqrestore(&pci_lock, flags);		\
> +	raw_spin_unlock_irqrestore(&pci_lock, flags);			\
>  	return res;							\
>  }

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

On Wednesday, June 1, 2016 10:09:29 AM CEST Bjorn Helgaas wrote:
> Hi Arnd,
> 
> On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> > A lot of PCI host bridges require different methods for initiating
> > type 0 and type 1 config space accesses, leading to duplication of
> > code.
> > 
> > This adds support for the two different kinds at the pci_ops
> > level, with the newly added map_bridge/read_bridge/write_bridge
> > operations for type 1 accesses.
> > 
> > When these are not set, we fall back to the regular map_bus/read/write
> > operations, so all existing drivers keep working, and bridges that
> > have identical operations continue to only require one set.
> 
> This adds new config accessor functions to struct pci_ops and makes
> the callers responsible for figuring out which one to use.  The
> benefit is to reduce code duplication in some host bridge drivers
> (DesignWare and MVEBU so far).
> 
> From a design perspective, I'm not comfortable with moving this burden
> from the host bridge drivers to the callers of the config accessors.

I see

> The pci_ops struct is a pretty low-level thing, but it is declared in
> include/linux/pci.h and while I think it's ugly to use it outside the
> PCI core, there's nothing that actually prevents that, and there are
> several places that do use it, including at least the ones below.  We
> could argue that many of these don't need updating because they don't
> need .read_bridge() accessors, but I don't think pci_ops users should
> be making assumptions like that.
> 
>   arch/x86/pci/common.c:          return raw_pci_ops->read(domain, bus, devfn, reg, len, val);
>   arch/x86/pci/common.c:          return raw_pci_ext_ops->read(domain, bus, devfn, reg, len, val);
>   arch/x86/pci/intel_mid_pci.c:           if (raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
>   arch/x86/pci/intel_mid_pci.c:                   raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
>   arch/x86/pci/intel_mid_pci.c:           raw_pci_ext_ops->read(domain, busnum, devfn,
>   arch/x86/pci/intel_mid_pci.c:   return raw_pci_ext_ops->read(pci_domain_nr(bus), bus->number,
>   arch/x86/pci/mmconfig-shared.c: raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0xce, 2, &win);
>   arch/x86/pci/mmconfig-shared.c: raw_pci_ops->read(0, 0, PCI_DEVFN(0, 0), 0x48, 4, &pciexbar);
>   arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), 0, 4, &l);
>   arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, PCI_DEVFN(0, 0), extcfg_regnum,
>   arch/x86/pci/mmconfig-shared.c:         raw_pci_ops->read(0, bus, devfn, 0, 4, &l);

All the x86 examples are pci_raw_ops though, not pci_ops.

>   drivers/pci/access.c:   res = bus->ops->read(bus, devfn, pos, len, &data);              \
>   drivers/pci/access.c:   ret = dev->bus->ops->read(dev->bus, dev->devfn,                 \

These implement the interface that is expected to be used.

>   drivers/pci/access.c:   return dev->vpd->ops->read(dev, pos, count, buf);

and this is pci_vpd_ops, not pci_ops

>   arch/sparc/kernel/pci_schizo.c: pbm->pci_ops->read(pbm->pci_bus, 0, PCI_STATUS, 2, &stat);
>   drivers/pci/pci.c:      bus->ops->read(bus, dev->devfn, PCI_COMMAND, 4, &cmd_status_dword);
>   drivers/pci/pcie/aer/aer_inject.c:      rv = ops->read(bus, devfn, where, size, val);

These are indeed some that I missed, but there are only very few of them.

Maybe we can simply change them to use the normal API and come up with
a way to make the pci_ops harder to misuse? Would it make you feel better
if we also renamed .read/.write into .read_type0/.write_type0 or something
like that?

> > In most cases, a driver will only have to override either map_bridge
> > or read_bridge/write_bridge but not both.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > This is slightly refined over what I had in the "Add PCIe driver for
> > Rockchip Soc" thread earlier, but probably not the final version yet,
> > so I'd like to get more feedback on it.
> > 
> > In particular, I think it may be useful to add a third set of
> > functions for the config space of devices that are directly attached
> > to the host bridge, as those are sometimes (designware, rcar, mvebu)
> > yet again different from the host bridge itself and from all other
> > devices. On the other hand, that adds further complexity that we
> > may want to leave out of the common code, and I honestly can't
> > seem to come up for a catchy name form the callbacks.
> > 
> >  drivers/pci/access.c | 35 +++++++++++++++++++++++++++--------
> >  include/linux/pci.h  |  3 +++
> >  2 files changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index d11cdbb8fba3..263606ece211 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -34,9 +34,12 @@ int pci_bus_read_config_##size \
> >  	u32 data = 0;							\
> >  	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
> >  	raw_spin_lock_irqsave(&pci_lock, flags);			\
> > -	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> > +	if (!bus->parent == 0 && bus->ops->read_bridge)			\
> 
>   if (!bus->parent && ...) ?

Fixed, thanks for noticing.

	Arnd

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Bjorn Helgaas]

On Wed, Jun 01, 2016 at 05:41:53PM +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 10:09:29 AM CEST Bjorn Helgaas wrote:
> > Hi Arnd,
> > 
> > On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> > > A lot of PCI host bridges require different methods for initiating
> > > type 0 and type 1 config space accesses, leading to duplication of
> > > code.
> > > 
> > > This adds support for the two different kinds at the pci_ops
> > > level, with the newly added map_bridge/read_bridge/write_bridge
> > > operations for type 1 accesses.
> > > 
> > > When these are not set, we fall back to the regular map_bus/read/write
> > > operations, so all existing drivers keep working, and bridges that
> > > have identical operations continue to only require one set.
> > 
> > This adds new config accessor functions to struct pci_ops and makes
> > the callers responsible for figuring out which one to use.  The
> > benefit is to reduce code duplication in some host bridge drivers
> > (DesignWare and MVEBU so far).
> > 
> > From a design perspective, I'm not comfortable with moving this burden
> > from the host bridge drivers to the callers of the config accessors.
> ...

> Maybe we can simply change them to use the normal API and come up with
> a way to make the pci_ops harder to misuse? Would it make you feel better
> if we also renamed .read/.write into .read_type0/.write_type0 or something
> like that?

I'm trying to get a better feel for the tradeoff here.  It seems like
an API complication vs. code duplication.

I don't really think the callers should have to figure out which
accessor to use.  How much of a benefit do we really gain by
complicating the callers?  We've managed for quite a few years with
the current scheme, and it seems like only a couple new ARM platforms
would benefit.

Bjorn

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

On Wednesday, June 1, 2016 2:04:30 PM CEST Bjorn Helgaas wrote:
> On Wed, Jun 01, 2016 at 05:41:53PM +0200, Arnd Bergmann wrote:
> > On Wednesday, June 1, 2016 10:09:29 AM CEST Bjorn Helgaas wrote:
> > > Hi Arnd,
> > > 
> > > On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> > > > A lot of PCI host bridges require different methods for initiating
> > > > type 0 and type 1 config space accesses, leading to duplication of
> > > > code.
> > > > 
> > > > This adds support for the two different kinds at the pci_ops
> > > > level, with the newly added map_bridge/read_bridge/write_bridge
> > > > operations for type 1 accesses.
> > > > 
> > > > When these are not set, we fall back to the regular map_bus/read/write
> > > > operations, so all existing drivers keep working, and bridges that
> > > > have identical operations continue to only require one set.
> > > 
> > > This adds new config accessor functions to struct pci_ops and makes
> > > the callers responsible for figuring out which one to use.  The
> > > benefit is to reduce code duplication in some host bridge drivers
> > > (DesignWare and MVEBU so far).
> > > 
> > > From a design perspective, I'm not comfortable with moving this burden
> > > from the host bridge drivers to the callers of the config accessors.
> > ...
> 
> > Maybe we can simply change them to use the normal API and come up with
> > a way to make the pci_ops harder to misuse? Would it make you feel better
> > if we also renamed .read/.write into .read_type0/.write_type0 or something
> > like that?
> 
> I'm trying to get a better feel for the tradeoff here.  It seems like
> an API complication vs. code duplication.
> 
> I don't really think the callers should have to figure out which
> accessor to use.  How much of a benefit do we really gain by
> complicating the callers?  We've managed for quite a few years with
> the current scheme, and it seems like only a couple new ARM platforms
> would benefit.

I just did a count of the implementations of pci_ops: I found 107
instances of 'struct pci_ops', and 67 of them treat type0 and type1
access differently in some form.

I'd estimate that about half of them, or roughly a third of the total
instances would benefit from my change, if we were to do them again.
Clearly there is no need to change the existing code here when it works,
unless the benefit is very clear and the code is actively maintained.

In some cases, the difference is only that the root bus has a limited
set of devices that are allowed to be accessed, so there would
likely be no benefit of this, compared to e.g. yet another callback
that checks the validity.
Some other instances have type0 registers at a different memory location
from type1, some use different layout inside of that space, and some
are completely different.

	Arnd

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Bjorn Helgaas]

On Wed, Jun 01, 2016 at 10:37:28PM +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 2:04:30 PM CEST Bjorn Helgaas wrote:
> > On Wed, Jun 01, 2016 at 05:41:53PM +0200, Arnd Bergmann wrote:
> > > On Wednesday, June 1, 2016 10:09:29 AM CEST Bjorn Helgaas wrote:
> > > > Hi Arnd,
> > > > 
> > > > On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> > > > > A lot of PCI host bridges require different methods for initiating
> > > > > type 0 and type 1 config space accesses, leading to duplication of
> > > > > code.
> > > > > 
> > > > > This adds support for the two different kinds at the pci_ops
> > > > > level, with the newly added map_bridge/read_bridge/write_bridge
> > > > > operations for type 1 accesses.
> > > > > 
> > > > > When these are not set, we fall back to the regular map_bus/read/write
> > > > > operations, so all existing drivers keep working, and bridges that
> > > > > have identical operations continue to only require one set.
> > > > 
> > > > This adds new config accessor functions to struct pci_ops and makes
> > > > the callers responsible for figuring out which one to use.  The
> > > > benefit is to reduce code duplication in some host bridge drivers
> > > > (DesignWare and MVEBU so far).
> > > > 
> > > > From a design perspective, I'm not comfortable with moving this burden
> > > > from the host bridge drivers to the callers of the config accessors.
> > > ...
> > 
> > > Maybe we can simply change them to use the normal API and come up with
> > > a way to make the pci_ops harder to misuse? Would it make you feel better
> > > if we also renamed .read/.write into .read_type0/.write_type0 or something
> > > like that?
> > 
> > I'm trying to get a better feel for the tradeoff here.  It seems like
> > an API complication vs. code duplication.
> > 
> > I don't really think the callers should have to figure out which
> > accessor to use.  How much of a benefit do we really gain by
> > complicating the callers?  We've managed for quite a few years with
> > the current scheme, and it seems like only a couple new ARM platforms
> > would benefit.
> 
> I just did a count of the implementations of pci_ops: I found 107
> instances of 'struct pci_ops', and 67 of them treat type0 and type1
> access differently in some form.
> 
> I'd estimate that about half of them, or roughly a third of the total
> instances would benefit from my change, if we were to do them again.
> Clearly there is no need to change the existing code here when it works,
> unless the benefit is very clear and the code is actively maintained.
> 
> In some cases, the difference is only that the root bus has a limited
> set of devices that are allowed to be accessed, so there would
> likely be no benefit of this, compared to e.g. yet another callback
> that checks the validity.
> Some other instances have type0 registers at a different memory location
> from type1, some use different layout inside of that space, and some
> are completely different.

The type0/type1 distinction still seems out of place to me at the call
site.  Is there any other reason a caller would care about the
difference between type0 and type1?

Bjorn

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

On Thursday, June 2, 2016 9:00:01 AM CEST Bjorn Helgaas wrote:
> > I just did a count of the implementations of pci_ops: I found 107
> > instances of 'struct pci_ops', and 67 of them treat type0 and type1
> > access differently in some form.
> > 
> > I'd estimate that about half of them, or roughly a third of the total
> > instances would benefit from my change, if we were to do them again.
> > Clearly there is no need to change the existing code here when it works,
> > unless the benefit is very clear and the code is actively maintained.
> > 
> > In some cases, the difference is only that the root bus has a limited
> > set of devices that are allowed to be accessed, so there would
> > likely be no benefit of this, compared to e.g. yet another callback
> > that checks the validity.
> > Some other instances have type0 registers at a different memory location
> > from type1, some use different layout inside of that space, and some
> > are completely different.
> 
> The type0/type1 distinction still seems out of place to me at the call
> site.  Is there any other reason a caller would care about the
> difference between type0 and type1?

The callers really shouldn't care, but they also shouldn't call the
pci_ops function pointer (and as we found earlier, there are only
three such callers).

The distinction between type0 and type1 in my mind is an implementation
detail of the pci_{read,write}_config_{byte,word,dword} functions
that call the low-level operations here.

	Arnd

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

On Thursday, June 2, 2016 9:00:01 AM CEST Bjorn Helgaas wrote:
> On Wed, Jun 01, 2016 at 10:37:28PM +0200, Arnd Bergmann wrote:
> > On Wednesday, June 1, 2016 2:04:30 PM CEST Bjorn Helgaas wrote:
> > > On Wed, Jun 01, 2016 at 05:41:53PM +0200, Arnd Bergmann wrote:
> > > > On Wednesday, June 1, 2016 10:09:29 AM CEST Bjorn Helgaas wrote:
> > > > > Hi Arnd,
> > > > > 
> > > > > On Wed, Jun 01, 2016 at 02:31:22PM +0200, Arnd Bergmann wrote:
> > > > > > A lot of PCI host bridges require different methods for initiating
> > > > > > type 0 and type 1 config space accesses, leading to duplication of
> > > > > > code.
> > > > > > 
> > > > > > This adds support for the two different kinds at the pci_ops
> > > > > > level, with the newly added map_bridge/read_bridge/write_bridge
> > > > > > operations for type 1 accesses.
> > > > > > 
> > > > > > When these are not set, we fall back to the regular map_bus/read/write
> > > > > > operations, so all existing drivers keep working, and bridges that
> > > > > > have identical operations continue to only require one set.
> > > > > 
> > > > > This adds new config accessor functions to struct pci_ops and makes
> > > > > the callers responsible for figuring out which one to use.  The
> > > > > benefit is to reduce code duplication in some host bridge drivers
> > > > > (DesignWare and MVEBU so far).
> > > > > 
> > > > > From a design perspective, I'm not comfortable with moving this burden
> > > > > from the host bridge drivers to the callers of the config accessors.
> > > > ...
> > > 
> > > > Maybe we can simply change them to use the normal API and come up with
> > > > a way to make the pci_ops harder to misuse? Would it make you feel better
> > > > if we also renamed .read/.write into .read_type0/.write_type0 or something
> > > > like that?
> > > 
> > > I'm trying to get a better feel for the tradeoff here.  It seems like
> > > an API complication vs. code duplication.
> > > 
> > > I don't really think the callers should have to figure out which
> > > accessor to use.  How much of a benefit do we really gain by
> > > complicating the callers?  We've managed for quite a few years with
> > > the current scheme, and it seems like only a couple new ARM platforms
> > > would benefit.
> > 
> > I just did a count of the implementations of pci_ops: I found 107
> > instances of 'struct pci_ops', and 67 of them treat type0 and type1
> > access differently in some form.
> > 
> > I'd estimate that about half of them, or roughly a third of the total
> > instances would benefit from my change, if we were to do them again.
> > Clearly there is no need to change the existing code here when it works,
> > unless the benefit is very clear and the code is actively maintained.
> > 
> > In some cases, the difference is only that the root bus has a limited
> > set of devices that are allowed to be accessed, so there would
> > likely be no benefit of this, compared to e.g. yet another callback
> > that checks the validity.
> > Some other instances have type0 registers at a different memory location
> > from type1, some use different layout inside of that space, and some
> > are completely different.
> 
> The type0/type1 distinction still seems out of place to me at the call
> site.  Is there any other reason a caller would care about the
> difference between type0 and type1?

Another idea based on my RFC patches to make pci_host_bridge the primary
structure for probing PCI: we could split out the old 'bus::pci_ops' with
the traditional read/write interface from a new structure that becomes
pci_host_bridge::pci_host_bridge_ops, and also contains the other callbacks
that we recently added to pci_ops, alongside type0/type1 accessors.

We could then have a set of default pci_ops that call
pci_host_bridge_ops->type0_read/type0_write/type1_read/type1_write,
and those in turn get a pci_host_bridge as an argument along with the
bus, device, function and register numbers instead of bus pointer
and devfn/where.

This way all existing code can keep working, but we can convert host
drivers (if desired) to provide only pci_host_bridge_ops and no
pci_ops, while making it easier to define those with a more modern
interface.

	Arnd

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Bjorn Helgaas]

On Thu, Jun 02, 2016 at 05:06:55PM +0200, Arnd Bergmann wrote:
> On Thursday, June 2, 2016 9:00:01 AM CEST Bjorn Helgaas wrote:
> > > I just did a count of the implementations of pci_ops: I found 107
> > > instances of 'struct pci_ops', and 67 of them treat type0 and type1
> > > access differently in some form.
> > > 
> > > I'd estimate that about half of them, or roughly a third of the total
> > > instances would benefit from my change, if we were to do them again.
> > > Clearly there is no need to change the existing code here when it works,
> > > unless the benefit is very clear and the code is actively maintained.
> > > 
> > > In some cases, the difference is only that the root bus has a limited
> > > set of devices that are allowed to be accessed, so there would
> > > likely be no benefit of this, compared to e.g. yet another callback
> > > that checks the validity.
> > > Some other instances have type0 registers at a different memory location
> > > from type1, some use different layout inside of that space, and some
> > > are completely different.
> > 
> > The type0/type1 distinction still seems out of place to me at the call
> > site.  Is there any other reason a caller would care about the
> > difference between type0 and type1?
> 
> The callers really shouldn't care, but they also shouldn't call the
> pci_ops function pointer (and as we found earlier, there are only
> three such callers).
> 
> The distinction between type0 and type1 in my mind is an implementation
> detail of the pci_{read,write}_config_{byte,word,dword} functions
> that call the low-level operations here.

The caller is performing one abstract operation: reading or writing
config space of a PCI device.  In the caller's context, it makes no
difference whether it's a type0 or type1 access.

Moving the test from the host bridge driver to pci_read_config_byte()
does move a little code from the callee to the caller, and there are
more callees than callers, so in that sense it does remove code
overall.  If you consider a single driver by itself, I'm not sure it
makes much difference.

The pcie-designware.c patch is a net removal of 17 lines, but that's
not all from moving the type0/type1 test: restructuring along the same
lines but keeping the original type0/type1 test removes about 9 lines.

Anyway, I think I'd rather work first on your RFC patches to make
pci_host_bridge the primary structure for probing PCI.  I think
there will be a *lot* of benefit there.

Bjorn

Re: [PATCH 1/3] pci: introduce read_bridge/write_bridge pci ops

[Arnd Bergmann]

On Monday, June 6, 2016 7:28:22 PM CEST Bjorn Helgaas wrote:
> The caller is performing one abstract operation: reading or writing
> config space of a PCI device.  In the caller's context, it makes no
> difference whether it's a type0 or type1 access.
> 
> Moving the test from the host bridge driver to pci_read_config_byte()
> does move a little code from the callee to the caller, and there are
> more callees than callers, so in that sense it does remove code
> overall.  If you consider a single driver by itself, I'm not sure it
> makes much difference.
> 
> The pcie-designware.c patch is a net removal of 17 lines, but that's
> not all from moving the type0/type1 test: restructuring along the same
> lines but keeping the original type0/type1 test removes about 9 lines.
> 
> Anyway, I think I'd rather work first on your RFC patches to make
> pci_host_bridge the primary structure for probing PCI.  I think
> there will be a *lot* of benefit there.

Fair enough. This series started from the review of the Rockchip
driver, and the idea was to make that one simpler, but given that
we already have several dozen drivers doing the same thing, adding
one more isn't going to have a significant impact now.

	Arnd