[PATCH 00/10] PCI: Add generic Conf Type 0/1 helpers

[PATCH 00/10] PCI: Add generic Conf Type 0/1 helpers

[Ilpo Järvinen]

This series replaces PCI_CONF1{,_EXT}_ADDRESS() with more generic
helpers and makes them more widely available by placing the new helpers
into include/linux/pci.h.

Most of what is under drivers/pci/controller is converted to use the
new helpers by this series. I left arch/ changes out from this because
they're quite varied so they would be harder to verify (and review)
except ARM/orion5x that I had to do now due to a naming conflict.
Nonetheless, there is plenty custom type 0/1 code under arch/ that
could now take advantage of the new helpers.

I've postponed touching pcie-mediatek.c because there's odd slot
calculation which I brought up in another thread.

Ilpo Järvinen (10):
  ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way
  PCI: Add helpers to calculate PCI Conf Type 0/1 addresses
  ARM: orion5x: Pass devfn to orion5x_pci_hw_{rd,wr}_conf()
  ARM: orion5x: Use generic PCI Conf Type 1 helper
  PCI: ixp4xx: Use generic PCI Conf Type 0 helper
  PCI: ixp4xx: Replace 1 with PCI_CONF1_TRANSACTION
  PCI: Replace PCI_CONF1{,_EXT}_ADDRESS() with the new helpers
  PCI: tegra: Use generic PCI Conf Type 1 helper
  PCI: mvebu: Use generic PCI Conf Type 1 helper
  PCI: v3: Use generic PCI Conf Type 0/1 helpers

 arch/arm/mach-orion5x/pci.c           | 54 +++++++----------
 drivers/pci/controller/pci-ftpci100.c |  6 +-
 drivers/pci/controller/pci-ixp4xx.c   |  9 ++-
 drivers/pci/controller/pci-mvebu.c    | 13 +---
 drivers/pci/controller/pci-tegra.c    | 12 +---
 drivers/pci/controller/pci-v3-semi.c  |  6 +-
 drivers/pci/controller/pcie-mt7621.c  |  7 +--
 drivers/pci/pci.h                     | 45 --------------
 include/linux/pci.h                   | 86 +++++++++++++++++++++++++++
 9 files changed, 123 insertions(+), 115 deletions(-)

-- 
2.39.2

[PATCH 01/10] ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way

[Ilpo Järvinen]

orion5x defines PCI_CONF_REG() and PCI_CONF_FUNC() that are problematic
because PCI core is going to introduce defines with the same names.

Add ORION5X prefix to those defines to avoid name conflicts.

Note: as this is part of series that replaces the code in question
anyway, only bare minimum renaming is done and other similarly named
macros are not touched.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/arm/mach-orion5x/pci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
index 3313bc5a63ea..77ddab90f448 100644
--- a/arch/arm/mach-orion5x/pci.c
+++ b/arch/arm/mach-orion5x/pci.c
@@ -219,8 +219,8 @@ static int __init pcie_setup(struct pci_sys_data *sys)
 /*
  * PCI_CONF_ADDR bits
  */
-#define PCI_CONF_REG(reg)		((reg) & 0xfc)
-#define PCI_CONF_FUNC(func)		(((func) & 0x3) << 8)
+#define ORION5X_PCI_CONF_REG(reg)	((reg) & 0xfc)
+#define ORION5X_PCI_CONF_FUNC(func)	(((func) & 0x3) << 8)
 #define PCI_CONF_DEV(dev)		(((dev) & 0x1f) << 11)
 #define PCI_CONF_BUS(bus)		(((bus) & 0xff) << 16)
 #define PCI_CONF_ADDR_EN		(1 << 31)
@@ -277,8 +277,8 @@ static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
 	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(dev) | PCI_CONF_REG(where) |
-		PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
+		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
 
 	*val = readl(PCI_CONF_DATA);
 
@@ -301,8 +301,8 @@ static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
 	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(dev) | PCI_CONF_REG(where) |
-		PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
+		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
 
 	if (size == 4) {
 		__raw_writel(val, PCI_CONF_DATA);
-- 
2.39.2

[PATCH 02/10] PCI: Add helpers to calculate PCI Conf Type 0/1 addresses

[Ilpo Järvinen]

Many places in arch and PCI controller code need to calculate PCI
Configuration Space Addresses for Type 0/1 accesses. There are small
variations between archs when it comes to bits outside of [10:2] (Type
0) and [24:2] (Type 1) but the basic calculation can still be
generalized.

drivers/pci/pci.h has PCI_CONF1{,_EXT}_ADDRESS() but due to their
location the use is limited to PCI subsys and the also always enable
PCI_CONF1_ENABLE which is not what all the callers want.

Add generic pci_conf{0,1}_addr() and pci_conf1_ext_addr() helpers into
include/linux/pci.h which can be reused by various parts of the kernel
that have to calculate PCI Conf Type 0/1 addresses.

The PCI_CONF* defines are needed by the new helpers so move also them
to include/linux/pci.h. The new helpers use true bitmasks and
FIELD_PREP() instead of open coded masking and shifting so adjust
PCI_CONF* definitions to match that.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.h   | 43 ++---------------------
 include/linux/pci.h | 85 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..cf0530a60105 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -833,49 +833,12 @@ struct pci_devres {
 
 struct pci_devres *find_pci_dr(struct pci_dev *pdev);
 
-/*
- * Config Address for PCI Configuration Mechanism #1
- *
- * See PCI Local Bus Specification, Revision 3.0,
- * Section 3.2.2.3.2, Figure 3-2, p. 50.
- */
-
-#define PCI_CONF1_BUS_SHIFT	16 /* Bus number */
-#define PCI_CONF1_DEV_SHIFT	11 /* Device number */
-#define PCI_CONF1_FUNC_SHIFT	8  /* Function number */
-
-#define PCI_CONF1_BUS_MASK	0xff
-#define PCI_CONF1_DEV_MASK	0x1f
-#define PCI_CONF1_FUNC_MASK	0x7
-#define PCI_CONF1_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */
-
-#define PCI_CONF1_ENABLE	BIT(31)
-#define PCI_CONF1_BUS(x)	(((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT)
-#define PCI_CONF1_DEV(x)	(((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT)
-#define PCI_CONF1_FUNC(x)	(((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT)
-#define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
-
 #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
 	(PCI_CONF1_ENABLE | \
-	 PCI_CONF1_BUS(bus) | \
-	 PCI_CONF1_DEV(dev) | \
-	 PCI_CONF1_FUNC(func) | \
-	 PCI_CONF1_REG(reg))
-
-/*
- * Extension of PCI Config Address for accessing extended PCIe registers
- *
- * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
- * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
- * are used for specifying additional 4 high bits of PCI Express register.
- */
-
-#define PCI_CONF1_EXT_REG_SHIFT	16
-#define PCI_CONF1_EXT_REG_MASK	0xf00
-#define PCI_CONF1_EXT_REG(x)	(((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
+	 pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
 
 #define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
-	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
-	 PCI_CONF1_EXT_REG(reg))
+	(PCI_CONF1_ENABLE | \
+	 pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
 
 #endif /* DRIVERS_PCI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 16493426a04f..4c4e3bb52a0a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -26,6 +26,8 @@
 #include <linux/args.h>
 #include <linux/mod_devicetable.h>
 
+#include <linux/bits.h>
+#include <linux/bitfield.h>
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
@@ -1183,6 +1185,89 @@ void pci_sort_breadthfirst(void);
 #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
 #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
 
+/*
+ * Config Address for PCI Configuration Mechanism #0/1
+ *
+ * See PCI Local Bus Specification, Revision 3.0,
+ * Section 3.2.2.3.2, Figure 3-1 and 3-2, p. 48-50.
+ */
+#define PCI_CONF_REG		0x000000ffU	/* common for Type 0/1 */
+#define PCI_CONF_FUNC		0x00000700U	/* common for Type 0/1 */
+#define PCI_CONF1_DEV		0x0000f800U
+#define PCI_CONF1_BUS		0x00ff0000U
+#define PCI_CONF1_ENABLE	BIT(31)
+
+/**
+ * pci_conf0_addr - PCI Base Configuration Space address for Type 0 access
+ * @devfn:      Device and function numbers (device number will be ignored)
+ * @reg:        Base configuration space offset
+ *
+ * Calculates the PCI Configuration Space address for Type 0 accesses.
+ *
+ * Note: the caller is responsible for adding the bits outside of [10:0].
+ *
+ * Return: Base Configuration Space address.
+ */
+static inline u32 pci_conf0_addr(u8 devfn, u8 reg)
+{
+	return FIELD_PREP(PCI_CONF_FUNC, PCI_FUNC(devfn)) |
+	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
+}
+
+/**
+ * pci_conf1_addr - PCI Base Configuration Space address for Type 1 access
+ * @bus:	Bus number of the device
+ * @devfn:	Device and function numbers
+ * @reg:	Base configuration space offset
+ * @enable:	Assert enable bit (bit 31)
+ *
+ * Calculates the PCI Base Configuration Space (first 256 bytes) address for
+ * Type 1 accesses.
+ *
+ * Note: the caller is responsible for adding the bits outside of [24:2]
+ * and enable bit.
+ *
+ * Return: PCI Base Configuration Space address.
+ */
+static inline u32 pci_conf1_addr(u8 bus, u8 devfn, u8 reg, bool enable)
+{
+	return (enable ? PCI_CONF1_ENABLE : 0) |
+	       FIELD_PREP(PCI_CONF1_BUS, bus) |
+	       FIELD_PREP(PCI_CONF1_DEV | PCI_CONF_FUNC, devfn) |
+	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
+}
+
+/*
+ * Extension of PCI Config Address for accessing extended PCIe registers
+ *
+ * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
+ * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
+ * are used for specifying additional 4 high bits of PCI Express register.
+ */
+#define PCI_CONF1_EXT_REG	0x0f000000UL
+
+/**
+ * pci_conf1_ext_addr - PCI Configuration Space address for Type 1 access
+ * @bus:	Bus number of the device
+ * @devfn:	Device and function numbers
+ * @reg:	Base or Extended Configuration space offset
+ * @enable:	Assert enable bit (bit 31)
+ *
+ * Calculates the PCI Base and Extended (4096 bytes per PCI function)
+ * Configuration Space address for Type 1 accesses. This function assumes
+ * the Extended Conguration Space is using the reserved bits [27:24].
+ *
+ * Note: the caller is responsible for adding the bits outside of [27:2] and
+ * enable bit.
+ *
+ * Return: PCI Configuration Space address.
+ */
+static inline u32 pci_conf1_ext_addr(u8 bus, u8 devfn, u16 reg, bool enable)
+{
+	return FIELD_PREP(PCI_CONF1_EXT_REG, (reg & 0xf00) >> 8) |
+	       pci_conf1_addr(bus, devfn, reg & 0xff, enable);
+}
+
 /* Generic PCI functions exported to card drivers */
 
 u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
-- 
2.39.2

[PATCH 03/10] ARM: orion5x: Pass devfn to orion5x_pci_hw_{rd,wr}_conf()

[Ilpo Järvinen]

Pass the usual devfn instead of individual components into
orion5x_pci_hw_{rd,wr}_conf() to make the change into
pci_conf1_offset() in an upcoming commit easier.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/arm/mach-orion5x/pci.c | 45 +++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
index 77ddab90f448..6376e1db6386 100644
--- a/arch/arm/mach-orion5x/pci.c
+++ b/arch/arm/mach-orion5x/pci.c
@@ -270,15 +270,15 @@ static int orion5x_pci_local_bus_nr(void)
 	return((conf & PCI_P2P_BUS_MASK) >> PCI_P2P_BUS_OFFS);
 }
 
-static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
-					u32 where, u32 size, u32 *val)
+static int orion5x_pci_hw_rd_conf(int bus, u8 devfn, u32 where,
+				  u32 size, u32 *val)
 {
 	unsigned long flags;
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
 	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
-		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
+		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
 
 	*val = readl(PCI_CONF_DATA);
 
@@ -292,8 +292,8 @@ static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
 	return PCIBIOS_SUCCESSFUL;
 }
 
-static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
-					u32 where, u32 size, u32 val)
+static int orion5x_pci_hw_wr_conf(int bus, u8 devfn, u32 where,
+				  u32 size, u32 val)
 {
 	unsigned long flags;
 	int ret = PCIBIOS_SUCCESSFUL;
@@ -301,8 +301,8 @@ static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
 	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
-		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
+		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
 
 	if (size == 4) {
 		__raw_writel(val, PCI_CONF_DATA);
@@ -347,8 +347,7 @@ static int orion5x_pci_rd_conf(struct pci_bus *bus, u32 devfn,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-	return orion5x_pci_hw_rd_conf(bus->number, PCI_SLOT(devfn),
-					PCI_FUNC(devfn), where, size, val);
+	return orion5x_pci_hw_rd_conf(bus->number, devfn, where, size, val);
 }
 
 static int orion5x_pci_wr_conf(struct pci_bus *bus, u32 devfn,
@@ -357,8 +356,7 @@ static int orion5x_pci_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (!orion5x_pci_valid_config(bus->number, devfn))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	return orion5x_pci_hw_wr_conf(bus->number, PCI_SLOT(devfn),
-					PCI_FUNC(devfn), where, size, val);
+	return orion5x_pci_hw_wr_conf(bus->number, devfn, where, size, val);
 }
 
 static struct pci_ops pci_ops = {
@@ -375,12 +373,14 @@ static void __init orion5x_pci_set_bus_nr(int nr)
 		 * PCI-X mode
 		 */
 		u32 pcix_status, bus, dev;
+		u8 devfn;
 		bus = (p2p & PCI_P2P_BUS_MASK) >> PCI_P2P_BUS_OFFS;
 		dev = (p2p & PCI_P2P_DEV_MASK) >> PCI_P2P_DEV_OFFS;
-		orion5x_pci_hw_rd_conf(bus, dev, 0, PCIX_STAT, 4, &pcix_status);
+		devfn = PCI_DEVFN(dev, 0);
+		orion5x_pci_hw_rd_conf(bus, devfn, PCIX_STAT, 4, &pcix_status);
 		pcix_status &= ~PCIX_STAT_BUS_MASK;
 		pcix_status |= (nr << PCIX_STAT_BUS_OFFS);
-		orion5x_pci_hw_wr_conf(bus, dev, 0, PCIX_STAT, 4, pcix_status);
+		orion5x_pci_hw_wr_conf(bus, devfn, PCIX_STAT, 4, pcix_status);
 	} else {
 		/*
 		 * PCI Conventional mode
@@ -393,15 +393,16 @@ static void __init orion5x_pci_set_bus_nr(int nr)
 
 static void __init orion5x_pci_master_slave_enable(void)
 {
-	int bus_nr, func, reg;
+	int bus_nr, reg;
+	u8 devfn;
 	u32 val;
 
 	bus_nr = orion5x_pci_local_bus_nr();
-	func = PCI_CONF_FUNC_STAT_CMD;
+	devfn = PCI_DEVFN(0, PCI_CONF_FUNC_STAT_CMD);
 	reg = PCI_CONF_REG_STAT_CMD;
-	orion5x_pci_hw_rd_conf(bus_nr, 0, func, reg, 4, &val);
+	orion5x_pci_hw_rd_conf(bus_nr, devfn, reg, 4, &val);
 	val |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-	orion5x_pci_hw_wr_conf(bus_nr, 0, func, reg, 4, val | 0x7);
+	orion5x_pci_hw_wr_conf(bus_nr, devfn, reg, 4, val | 0x7);
 }
 
 static void __init orion5x_setup_pci_wins(void)
@@ -424,7 +425,7 @@ static void __init orion5x_setup_pci_wins(void)
 
 	for (i = 0; i < dram->num_cs; i++) {
 		const struct mbus_dram_window *cs = dram->cs + i;
-		u32 func = PCI_CONF_FUNC_BAR_CS(cs->cs_index);
+		u8 devfn = PCI_DEVFN(0, PCI_CONF_FUNC_BAR_CS(cs->cs_index));
 		u32 reg;
 		u32 val;
 
@@ -432,15 +433,15 @@ static void __init orion5x_setup_pci_wins(void)
 		 * Write DRAM bank base address register.
 		 */
 		reg = PCI_CONF_REG_BAR_LO_CS(cs->cs_index);
-		orion5x_pci_hw_rd_conf(bus, 0, func, reg, 4, &val);
+		orion5x_pci_hw_rd_conf(bus, devfn, reg, 4, &val);
 		val = (cs->base & 0xfffff000) | (val & 0xfff);
-		orion5x_pci_hw_wr_conf(bus, 0, func, reg, 4, val);
+		orion5x_pci_hw_wr_conf(bus, devfn, reg, 4, val);
 
 		/*
 		 * Write DRAM bank size register.
 		 */
 		reg = PCI_CONF_REG_BAR_HI_CS(cs->cs_index);
-		orion5x_pci_hw_wr_conf(bus, 0, func, reg, 4, 0);
+		orion5x_pci_hw_wr_conf(bus, devfn, reg, 4, 0);
 		writel((cs->size - 1) & 0xfffff000,
 			PCI_BAR_SIZE_DDR_CS(cs->cs_index));
 		writel(cs->base & 0xfffff000,
-- 
2.39.2

[PATCH 04/10] ARM: orion5x: Use generic PCI Conf Type 1 helper

[Ilpo Järvinen]

Convert orion5x PCI code to use pci_conf1_ext_addr() from PCI core to
calculate PCI Configuration Type 1 address.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/arm/mach-orion5x/pci.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
index 6376e1db6386..8b7d67549adf 100644
--- a/arch/arm/mach-orion5x/pci.c
+++ b/arch/arm/mach-orion5x/pci.c
@@ -216,15 +216,6 @@ static int __init pcie_setup(struct pci_sys_data *sys)
 #define PCI_P2P_DEV_OFFS		24
 #define PCI_P2P_DEV_MASK		(0x1f << PCI_P2P_DEV_OFFS)
 
-/*
- * PCI_CONF_ADDR bits
- */
-#define ORION5X_PCI_CONF_REG(reg)	((reg) & 0xfc)
-#define ORION5X_PCI_CONF_FUNC(func)	(((func) & 0x3) << 8)
-#define PCI_CONF_DEV(dev)		(((dev) & 0x1f) << 11)
-#define PCI_CONF_BUS(bus)		(((bus) & 0xff) << 16)
-#define PCI_CONF_ADDR_EN		(1 << 31)
-
 /*
  * Internal configuration space
  */
@@ -276,9 +267,7 @@ static int orion5x_pci_hw_rd_conf(int bus, u8 devfn, u32 where,
 	unsigned long flags;
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
-	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
-		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+	writel(pci_conf1_addr(bus, devfn, where, true), PCI_CONF_ADDR);
 
 	*val = readl(PCI_CONF_DATA);
 
@@ -300,9 +289,7 @@ static int orion5x_pci_hw_wr_conf(int bus, u8 devfn, u32 where,
 
 	spin_lock_irqsave(&orion5x_pci_lock, flags);
 
-	writel(PCI_CONF_BUS(bus) |
-		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
-		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
+	writel(pci_conf1_addr(bus, devfn, where, true), PCI_CONF_ADDR);
 
 	if (size == 4) {
 		__raw_writel(val, PCI_CONF_DATA);
-- 
2.39.2

[PATCH 05/10] PCI: ixp4xx: Use generic PCI Conf Type 0 helper

[Ilpo Järvinen]

Convert Type 0 address calculation to use pci_conf0_offset() instead of
abusing PCI_CONF1_ADDRESS().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-ixp4xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-ixp4xx.c b/drivers/pci/controller/pci-ixp4xx.c
index acb85e0d5675..44639838df9c 100644
--- a/drivers/pci/controller/pci-ixp4xx.c
+++ b/drivers/pci/controller/pci-ixp4xx.c
@@ -188,8 +188,8 @@ static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
 	/* Root bus is always 0 in this hardware */
 	if (bus_num == 0) {
 		/* type 0 */
-		return (PCI_CONF1_ADDRESS(0, 0, PCI_FUNC(devfn), where) &
-			~PCI_CONF1_ENABLE) | BIT(32-PCI_SLOT(devfn));
+		return pci_conf0_addr(devfn, where) |
+		       BIT(32 - PCI_SLOT(devfn));
 	} else {
 		/* type 1 */
 		return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
-- 
2.39.2

[PATCH 06/10] PCI: ixp4xx: Replace 1 with PCI_CONF1_TRANSACTION

[Ilpo Järvinen]

Add PCI_CONF1_TRANSACTION and replace literal 1 within PCI
Configuration Space Type 1 address with it.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-ixp4xx.c | 2 +-
 include/linux/pci.h                 | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-ixp4xx.c b/drivers/pci/controller/pci-ixp4xx.c
index 44639838df9c..ec0125344ca1 100644
--- a/drivers/pci/controller/pci-ixp4xx.c
+++ b/drivers/pci/controller/pci-ixp4xx.c
@@ -194,7 +194,7 @@ static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
 		/* type 1 */
 		return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
 					  PCI_FUNC(devfn), where) &
-			~PCI_CONF1_ENABLE) | 1;
+			~PCI_CONF1_ENABLE) | PCI_CONF1_TRANSACTION;
 	}
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4c4e3bb52a0a..df613428bc4d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1191,6 +1191,7 @@ void pci_sort_breadthfirst(void);
  * See PCI Local Bus Specification, Revision 3.0,
  * Section 3.2.2.3.2, Figure 3-1 and 3-2, p. 48-50.
  */
+#define PCI_CONF1_TRANSACTION	BIT(0)
 #define PCI_CONF_REG		0x000000ffU	/* common for Type 0/1 */
 #define PCI_CONF_FUNC		0x00000700U	/* common for Type 0/1 */
 #define PCI_CONF1_DEV		0x0000f800U
-- 
2.39.2

[PATCH 07/10] PCI: Replace PCI_CONF1{,_EXT}_ADDRESS() with the new helpers

[Ilpo Järvinen]

Replace the old PCI_CONF1{,_EXT}_ADDRESS() helpers used to calculate
PCI Configuration Space Type 1 addresses with the new
pci_conf1{,_ext}_offset() helpers that are more generic and more widely
available.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-ftpci100.c | 6 ++----
 drivers/pci/controller/pci-ixp4xx.c   | 5 ++---
 drivers/pci/controller/pcie-mt7621.c  | 7 +++----
 drivers/pci/pci.h                     | 8 --------
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
index ffdeed25e961..a8d0217a0b94 100644
--- a/drivers/pci/controller/pci-ftpci100.c
+++ b/drivers/pci/controller/pci-ftpci100.c
@@ -182,8 +182,7 @@ static int faraday_raw_pci_read_config(struct faraday_pci *p, int bus_number,
 				       unsigned int fn, int config, int size,
 				       u32 *value)
 {
-	writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
-				 PCI_FUNC(fn), config),
+	writel(pci_conf1_addr(bus_number, fn, config, true),
 			p->base + FTPCI_CONFIG);
 
 	*value = readl(p->base + FTPCI_DATA);
@@ -214,8 +213,7 @@ static int faraday_raw_pci_write_config(struct faraday_pci *p, int bus_number,
 {
 	int ret = PCIBIOS_SUCCESSFUL;
 
-	writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
-				 PCI_FUNC(fn), config),
+	writel(pci_conf1_addr(bus_number, fn, config, true),
 			p->base + FTPCI_CONFIG);
 
 	switch (size) {
diff --git a/drivers/pci/controller/pci-ixp4xx.c b/drivers/pci/controller/pci-ixp4xx.c
index ec0125344ca1..fd52f4a3ef31 100644
--- a/drivers/pci/controller/pci-ixp4xx.c
+++ b/drivers/pci/controller/pci-ixp4xx.c
@@ -192,9 +192,8 @@ static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
 		       BIT(32 - PCI_SLOT(devfn));
 	} else {
 		/* type 1 */
-		return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
-					  PCI_FUNC(devfn), where) &
-			~PCI_CONF1_ENABLE) | PCI_CONF1_TRANSACTION;
+		return pci_conf1_addr(bus_num, devfn, where, false) |
+		       PCI_CONF1_TRANSACTION;
 	}
 }
 
diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 79e225edb42a..2b2d9828a910 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -127,8 +127,7 @@ static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
 					 unsigned int devfn, int where)
 {
 	struct mt7621_pcie *pcie = bus->sysdata;
-	u32 address = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn),
-					    PCI_FUNC(devfn), where);
+	u32 address = pci_conf1_ext_addr(bus->number, devfn, where, true);
 
 	writel_relaxed(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
 
@@ -143,7 +142,7 @@ static struct pci_ops mt7621_pcie_ops = {
 
 static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
 {
-	u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
+	u32 address = pci_conf1_ext_addr(0, PCI_DEVFN(dev, 0), reg, true);
 
 	pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
 	return pcie_read(pcie, RALINK_PCI_CONFIG_DATA);
@@ -152,7 +151,7 @@ static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
 static void write_config(struct mt7621_pcie *pcie, unsigned int dev,
 			 u32 reg, u32 val)
 {
-	u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
+	u32 address = pci_conf1_ext_addr(0, PCI_DEVFN(dev, 0), reg, true);
 
 	pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
 	pcie_write(pcie, val, RALINK_PCI_CONFIG_DATA);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index cf0530a60105..fdf9624b0b12 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -833,12 +833,4 @@ struct pci_devres {
 
 struct pci_devres *find_pci_dr(struct pci_dev *pdev);
 
-#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
-	(PCI_CONF1_ENABLE | \
-	 pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
-
-#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
-	(PCI_CONF1_ENABLE | \
-	 pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
-
 #endif /* DRIVERS_PCI_H */
-- 
2.39.2

[PATCH 08/10] PCI: tegra: Use generic PCI Conf Type 1 helper

[Ilpo Järvinen]

Convert tegra to use the pci_conf1_ext_addr() helper from PCI core to
calculate PCI Configuration Space address for Type 1 access.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---

Note: where mask is changed from 0xff -> 0xfc by this change.
---
 drivers/pci/controller/pci-tegra.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 038d974a318e..a86e88c6b87a 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -415,13 +415,6 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
  * address (access to which generates correct config transaction) falls in
  * this 4 KiB region.
  */
-static unsigned int tegra_pcie_conf_offset(u8 bus, unsigned int devfn,
-					   unsigned int where)
-{
-	return ((where & 0xf00) << 16) | (bus << 16) | (PCI_SLOT(devfn) << 11) |
-	       (PCI_FUNC(devfn) << 8) | (where & 0xff);
-}
-
 static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 					unsigned int devfn,
 					int where)
@@ -440,10 +433,9 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 			}
 		}
 	} else {
-		unsigned int offset;
-		u32 base;
+		u32 base, offset;
 
-		offset = tegra_pcie_conf_offset(bus->number, devfn, where);
+		offset = pci_conf1_ext_addr(bus->number, devfn, where, false);
 
 		/* move 4 KiB window to offset within the FPCI region */
 		base = 0xfe100000 + ((offset & ~(SZ_4K - 1)) >> 8);
-- 
2.39.2

[PATCH 09/10] PCI: mvebu: Use generic PCI Conf Type 1 helper

[Ilpo Järvinen]

Convert mvebu to use the pci_conf1_ext_addr() helper from PCI core
to calculate PCI Configuration Space address for Type 1 access.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-mvebu.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 29fe09c99e7d..1908754ee6fd 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -45,15 +45,6 @@
 #define PCIE_WIN5_BASE_OFF	0x1884
 #define PCIE_WIN5_REMAP_OFF	0x188c
 #define PCIE_CONF_ADDR_OFF	0x18f8
-#define  PCIE_CONF_ADDR_EN		0x80000000
-#define  PCIE_CONF_REG(r)		((((r) & 0xf00) << 16) | ((r) & 0xfc))
-#define  PCIE_CONF_BUS(b)		(((b) & 0xff) << 16)
-#define  PCIE_CONF_DEV(d)		(((d) & 0x1f) << 11)
-#define  PCIE_CONF_FUNC(f)		(((f) & 0x7) << 8)
-#define  PCIE_CONF_ADDR(bus, devfn, where) \
-	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))    | \
-	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where) | \
-	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
 #define PCIE_INT_CAUSE_OFF	0x1900
 #define PCIE_INT_UNMASK_OFF	0x1910
@@ -361,7 +352,7 @@ static int mvebu_pcie_child_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 
 	conf_data = port->base + PCIE_CONF_DATA_OFF;
 
-	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
+	mvebu_writel(port, pci_conf1_ext_addr(bus->number, devfn, where, true),
 		     PCIE_CONF_ADDR_OFF);
 
 	switch (size) {
@@ -397,7 +388,7 @@ static int mvebu_pcie_child_wr_conf(struct pci_bus *bus, u32 devfn,
 
 	conf_data = port->base + PCIE_CONF_DATA_OFF;
 
-	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
+	mvebu_writel(port, pci_conf1_ext_addr(bus->number, devfn, where, true),
 		     PCIE_CONF_ADDR_OFF);
 
 	switch (size) {
-- 
2.39.2

[PATCH 10/10] PCI: v3: Use generic PCI Conf Type 0/1 helpers

[Ilpo Järvinen]

Convert v3 to use pci_conf{0,1}_addr() from PCI core to calculate PCI
Configuration Space address for Type 0/1 access.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-v3-semi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
index 460a825325dd..a07323148007 100644
--- a/drivers/pci/controller/pci-v3-semi.c
+++ b/drivers/pci/controller/pci-v3-semi.c
@@ -327,7 +327,7 @@ static void __iomem *v3_map_bus(struct pci_bus *bus,
 		 *  3:1 = config cycle (101)
 		 *  0   = PCI A1 & A0 are 0 (0)
 		 */
-		address = PCI_FUNC(devfn) << 8;
+		address = pci_conf0_addr(devfn, offset);
 		mapaddress = V3_LB_MAP_TYPE_CONFIG;
 
 		if (slot > 12)
@@ -354,7 +354,7 @@ static void __iomem *v3_map_bus(struct pci_bus *bus,
 		 *  0   = PCI A1 & A0 from host bus (1)
 		 */
 		mapaddress = V3_LB_MAP_TYPE_CONFIG | V3_LB_MAP_AD_LOW_EN;
-		address = (busnr << 16) | (devfn << 8);
+		address = pci_conf1_addr(busnr, devfn, offset, false);
 	}
 
 	/*
@@ -375,7 +375,7 @@ static void __iomem *v3_map_bus(struct pci_bus *bus,
 	       v3->base + V3_LB_BASE1);
 	writew(mapaddress, v3->base + V3_LB_MAP1);
 
-	return v3->config_base + address + offset;
+	return v3->config_base + address;
 }
 
 static void v3_unmap_bus(struct v3_pci *v3)
-- 
2.39.2

Re: [PATCH 01/10] ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way

[Andrew Lunn]

On Mon, Apr 29, 2024 at 01:46:24PM +0300, Ilpo Järvinen wrote:
> orion5x defines PCI_CONF_REG() and PCI_CONF_FUNC() that are problematic
> because PCI core is going to introduce defines with the same names.
> 
> Add ORION5X prefix to those defines to avoid name conflicts.
> 
> Note: as this is part of series that replaces the code in question
> anyway, only bare minimum renaming is done and other similarly named
> macros are not touched.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 03/10] ARM: orion5x: Pass devfn to orion5x_pci_hw_{rd,wr}_conf()

[Andrew Lunn]

On Mon, Apr 29, 2024 at 01:46:26PM +0300, Ilpo Järvinen wrote:
> Pass the usual devfn instead of individual components into
> orion5x_pci_hw_{rd,wr}_conf() to make the change into
> pci_conf1_offset() in an upcoming commit easier.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 01/10] ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way

[Andrew Lunn]

On Mon, Apr 29, 2024 at 01:46:24PM +0300, Ilpo Järvinen wrote:
> orion5x defines PCI_CONF_REG() and PCI_CONF_FUNC() that are problematic
> because PCI core is going to introduce defines with the same names.
> 
> Add ORION5X prefix to those defines to avoid name conflicts.
> 
> Note: as this is part of series that replaces the code in question
> anyway, only bare minimum renaming is done and other similarly named
> macros are not touched.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Hi Ilpo

What branch do these apply to? I wanted to test them, but i get hunks
rejected:

git am < 20240429104633.11060-1-ilpo.jarvinen@linux.intel.com.mbx
Applying: ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way
Applying: ARM: orion5x: Use generic PCI Conf Type 1 helper
error: patch failed: arch/arm/mach-orion5x/pci.c:276
error: arch/arm/mach-orion5x/pci.c: patch does not apply
Patch failed at 0002 ARM: orion5x: Use generic PCI Conf Type 1 helper

I tried linux-next, v6.9-rc6, pci:next

Thanks
	Andrew

Re: [PATCH 01/10] ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Mon, 29 Apr 2024, Andrew Lunn wrote:

> On Mon, Apr 29, 2024 at 01:46:24PM +0300, Ilpo Järvinen wrote:
> > orion5x defines PCI_CONF_REG() and PCI_CONF_FUNC() that are problematic
> > because PCI core is going to introduce defines with the same names.
> > 
> > Add ORION5X prefix to those defines to avoid name conflicts.
> > 
> > Note: as this is part of series that replaces the code in question
> > anyway, only bare minimum renaming is done and other similarly named
> > macros are not touched.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Hi Ilpo
> 
> What branch do these apply to? I wanted to test them, but i get hunks
> rejected:
> 
> git am < 20240429104633.11060-1-ilpo.jarvinen@linux.intel.com.mbx
> Applying: ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way
> Applying: ARM: orion5x: Use generic PCI Conf Type 1 helper
> error: patch failed: arch/arm/mach-orion5x/pci.c:276
> error: arch/arm/mach-orion5x/pci.c: patch does not apply
> Patch failed at 0002 ARM: orion5x: Use generic PCI Conf Type 1 helper
> 
> I tried linux-next, v6.9-rc6, pci:next

Hi,

On top of pci:main (so v6.9-rc1).

"ARM: orion5x: Use generic PCI Conf Type 1 helper" should be only 4th 
patch but your command seems to apply it as 2nd patch (is the mbx file 
having them out-of-order?).

-- 
 i.

Re: [PATCH 00/10] PCI: Add generic Conf Type 0/1 helpers

[Pali Rohár]

On Monday 29 April 2024 13:46:23 Ilpo Järvinen wrote:
> This series replaces PCI_CONF1{,_EXT}_ADDRESS() with more generic
> helpers and makes them more widely available by placing the new helpers
> into include/linux/pci.h.

There was a request to move these macros from include/linux/pci.h file
to the driver/pci/pci.h file...

https://lore.kernel.org/linux-pci/20220913211143.GA624473@bhelgaas/

Something was changed that it has to be moved back?

Re: [PATCH 02/10] PCI: Add helpers to calculate PCI Conf Type 0/1 addresses

[Pali Rohár]

On Monday 29 April 2024 13:46:25 Ilpo Järvinen wrote:
> Many places in arch and PCI controller code need to calculate PCI
> Configuration Space Addresses for Type 0/1 accesses. There are small
> variations between archs when it comes to bits outside of [10:2] (Type
> 0) and [24:2] (Type 1) but the basic calculation can still be
> generalized.
> 
> drivers/pci/pci.h has PCI_CONF1{,_EXT}_ADDRESS() but due to their
> location the use is limited to PCI subsys and the also always enable
> PCI_CONF1_ENABLE which is not what all the callers want.
> 
> Add generic pci_conf{0,1}_addr() and pci_conf1_ext_addr() helpers into
> include/linux/pci.h which can be reused by various parts of the kernel
> that have to calculate PCI Conf Type 0/1 addresses.
> 
> The PCI_CONF* defines are needed by the new helpers so move also them
> to include/linux/pci.h. The new helpers use true bitmasks and
> FIELD_PREP() instead of open coded masking and shifting so adjust
> PCI_CONF* definitions to match that.

I introduced these PCI_CONF* macros years ago. At that time I studied
more sources and drivers what they use. To make it clear I will first
try to explain few things. Hopefully I do not write mistakes here, it is
longer time.

Configuration mechanism is a SW API which allows access config space.
Configuration access command is on-wire "format" which allows HW bus to
access config space.

There are two standardized configuration mechanisms #1 and #2. #2 was
removed in PCI 3.0 spec and is out of scope (there are no macros for
them). #1 uses pair of I/O registers (on x86 they are CF8 and CFC; on
ARM they are whatever vendor invented). There is an extension of #1
which uses few reserved bits to describe config space registers above
255. Then there is standardized PCIe ECAM. And then there are lot of
proprietary vendor specific ways. Proprietary vendor way can be for
example composing PCIe packet manually or composing configuration access
command manually.

There are two configuration space access commands: type 0 and type 1.
It is required to issue correct command type based on topology of the
endpoint device. When mechanism #1 is used then it is HW who generate
these commands and it takes care to correctly choose type 0 or 1. But if
some vendor specific mechanism is used which requires SW to compose
access command manually then SW is responsible for correct choose.


In your change you have mixed configuration mechanism #1 together with
configuration command for type 0. This is really a problem as it makes
the code harder to understand and even hard to figure out how to write a
new driver (e.g. how to compose configuration command for type 1?).
Also it took me a little bit more time to understand your change.
Format of command for type 1 and format of configuration mechanism #1
really looks very similar. So there can be a confusion. Bit 31 is a key
there.

I understand that you want to unify drivers, so I would suggest to not
change existing macros for configuration mechanism #1 and #1-ext.
And rather create new macros (or functions) for composing configuration
commands.

This makes it explicitly clear that particular PCI or PCIe controller HW
requires SW driver to compose configuration command (type 0 and 1). Or
that HW requires from SW to compose format of configuration mechanism #1
(or #1-ext). Also it would make pci controller drivers more readable as
from the macro/function it would be easy to understand what it is doing.

Also important is that if you are in pci controller driver composing
command for type 0 then with very high probability the HW is designed in
a way that it requires from SW to also compose command type 1. And it
would be an mistake if the driver compose only type 0 or type 1. I
remember from the past an issue: why endpoint PCIe card is working, but
it is not working if it is connected behind PCIe switch. (mistake was:
HW was always sending type 0 command due to SW issue)

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/pci.h   | 43 ++---------------------
>  include/linux/pci.h | 85 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 17fed1846847..cf0530a60105 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -833,49 +833,12 @@ struct pci_devres {
>  
>  struct pci_devres *find_pci_dr(struct pci_dev *pdev);
>  
> -/*
> - * Config Address for PCI Configuration Mechanism #1
> - *
> - * See PCI Local Bus Specification, Revision 3.0,
> - * Section 3.2.2.3.2, Figure 3-2, p. 50.
> - */
> -
> -#define PCI_CONF1_BUS_SHIFT	16 /* Bus number */
> -#define PCI_CONF1_DEV_SHIFT	11 /* Device number */
> -#define PCI_CONF1_FUNC_SHIFT	8  /* Function number */
> -
> -#define PCI_CONF1_BUS_MASK	0xff
> -#define PCI_CONF1_DEV_MASK	0x1f
> -#define PCI_CONF1_FUNC_MASK	0x7
> -#define PCI_CONF1_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */
> -
> -#define PCI_CONF1_ENABLE	BIT(31)
> -#define PCI_CONF1_BUS(x)	(((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT)
> -#define PCI_CONF1_DEV(x)	(((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT)
> -#define PCI_CONF1_FUNC(x)	(((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT)
> -#define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
> -
>  #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
>  	(PCI_CONF1_ENABLE | \
> -	 PCI_CONF1_BUS(bus) | \
> -	 PCI_CONF1_DEV(dev) | \
> -	 PCI_CONF1_FUNC(func) | \
> -	 PCI_CONF1_REG(reg))
> -
> -/*
> - * Extension of PCI Config Address for accessing extended PCIe registers
> - *
> - * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> - * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> - * are used for specifying additional 4 high bits of PCI Express register.
> - */
> -
> -#define PCI_CONF1_EXT_REG_SHIFT	16
> -#define PCI_CONF1_EXT_REG_MASK	0xf00
> -#define PCI_CONF1_EXT_REG(x)	(((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
> +	 pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
>  
>  #define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
> -	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
> -	 PCI_CONF1_EXT_REG(reg))
> +	(PCI_CONF1_ENABLE | \
> +	 pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
>  
>  #endif /* DRIVERS_PCI_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 16493426a04f..4c4e3bb52a0a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -26,6 +26,8 @@
>  #include <linux/args.h>
>  #include <linux/mod_devicetable.h>
>  
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
>  #include <linux/types.h>
>  #include <linux/init.h>
>  #include <linux/ioport.h>
> @@ -1183,6 +1185,89 @@ void pci_sort_breadthfirst(void);
>  #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
>  #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
>  
> +/*
> + * Config Address for PCI Configuration Mechanism #0/1
> + *
> + * See PCI Local Bus Specification, Revision 3.0,
> + * Section 3.2.2.3.2, Figure 3-1 and 3-2, p. 48-50.
> + */
> +#define PCI_CONF_REG		0x000000ffU	/* common for Type 0/1 */
> +#define PCI_CONF_FUNC		0x00000700U	/* common for Type 0/1 */
> +#define PCI_CONF1_DEV		0x0000f800U
> +#define PCI_CONF1_BUS		0x00ff0000U
> +#define PCI_CONF1_ENABLE	BIT(31)
> +
> +/**
> + * pci_conf0_addr - PCI Base Configuration Space address for Type 0 access
> + * @devfn:      Device and function numbers (device number will be ignored)
> + * @reg:        Base configuration space offset
> + *
> + * Calculates the PCI Configuration Space address for Type 0 accesses.
> + *
> + * Note: the caller is responsible for adding the bits outside of [10:0].
> + *
> + * Return: Base Configuration Space address.
> + */
> +static inline u32 pci_conf0_addr(u8 devfn, u8 reg)
> +{
> +	return FIELD_PREP(PCI_CONF_FUNC, PCI_FUNC(devfn)) |
> +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> +}
> +
> +/**
> + * pci_conf1_addr - PCI Base Configuration Space address for Type 1 access
> + * @bus:	Bus number of the device
> + * @devfn:	Device and function numbers
> + * @reg:	Base configuration space offset
> + * @enable:	Assert enable bit (bit 31)
> + *
> + * Calculates the PCI Base Configuration Space (first 256 bytes) address for
> + * Type 1 accesses.
> + *
> + * Note: the caller is responsible for adding the bits outside of [24:2]
> + * and enable bit.
> + *
> + * Return: PCI Base Configuration Space address.
> + */
> +static inline u32 pci_conf1_addr(u8 bus, u8 devfn, u8 reg, bool enable)
> +{
> +	return (enable ? PCI_CONF1_ENABLE : 0) |
> +	       FIELD_PREP(PCI_CONF1_BUS, bus) |
> +	       FIELD_PREP(PCI_CONF1_DEV | PCI_CONF_FUNC, devfn) |
> +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> +}
> +
> +/*
> + * Extension of PCI Config Address for accessing extended PCIe registers
> + *
> + * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> + * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> + * are used for specifying additional 4 high bits of PCI Express register.
> + */
> +#define PCI_CONF1_EXT_REG	0x0f000000UL
> +
> +/**
> + * pci_conf1_ext_addr - PCI Configuration Space address for Type 1 access
> + * @bus:	Bus number of the device
> + * @devfn:	Device and function numbers
> + * @reg:	Base or Extended Configuration space offset
> + * @enable:	Assert enable bit (bit 31)
> + *
> + * Calculates the PCI Base and Extended (4096 bytes per PCI function)
> + * Configuration Space address for Type 1 accesses. This function assumes
> + * the Extended Conguration Space is using the reserved bits [27:24].
> + *
> + * Note: the caller is responsible for adding the bits outside of [27:2] and
> + * enable bit.
> + *
> + * Return: PCI Configuration Space address.
> + */
> +static inline u32 pci_conf1_ext_addr(u8 bus, u8 devfn, u16 reg, bool enable)
> +{
> +	return FIELD_PREP(PCI_CONF1_EXT_REG, (reg & 0xf00) >> 8) |
> +	       pci_conf1_addr(bus, devfn, reg & 0xff, enable);
> +}
> +
>  /* Generic PCI functions exported to card drivers */
>  
>  u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> -- 
> 2.39.2
> 

Re: [PATCH 09/10] PCI: mvebu: Use generic PCI Conf Type 1 helper

[Pali Rohár]

On Monday 29 April 2024 13:46:32 Ilpo Järvinen wrote:
> Convert mvebu to use the pci_conf1_ext_addr() helper from PCI core
> to calculate PCI Configuration Space address for Type 1 access.

Exampled in other email. mvebu controller uses extended configuration
mechanism #1. mvebu_pcie_child_rd_conf/mvebu_pcie_child_wr_conf
functions are used only for accessing devices behind the PCIe root port,
hence they are for Type 1 access. But the mvebu controller register
PCIE_CONF_ADDR_OFF takes address in the configuration mechanism #1
extended format. It does not take format of the command for Type 1
access. Hence the description and the change here is wrong/misleading.

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/controller/pci-mvebu.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 29fe09c99e7d..1908754ee6fd 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -45,15 +45,6 @@
>  #define PCIE_WIN5_BASE_OFF	0x1884
>  #define PCIE_WIN5_REMAP_OFF	0x188c
>  #define PCIE_CONF_ADDR_OFF	0x18f8
> -#define  PCIE_CONF_ADDR_EN		0x80000000
> -#define  PCIE_CONF_REG(r)		((((r) & 0xf00) << 16) | ((r) & 0xfc))
> -#define  PCIE_CONF_BUS(b)		(((b) & 0xff) << 16)
> -#define  PCIE_CONF_DEV(d)		(((d) & 0x1f) << 11)
> -#define  PCIE_CONF_FUNC(f)		(((f) & 0x7) << 8)
> -#define  PCIE_CONF_ADDR(bus, devfn, where) \
> -	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))    | \
> -	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where) | \
> -	 PCIE_CONF_ADDR_EN)
>  #define PCIE_CONF_DATA_OFF	0x18fc
>  #define PCIE_INT_CAUSE_OFF	0x1900
>  #define PCIE_INT_UNMASK_OFF	0x1910
> @@ -361,7 +352,7 @@ static int mvebu_pcie_child_rd_conf(struct pci_bus *bus, u32 devfn, int where,
>  
>  	conf_data = port->base + PCIE_CONF_DATA_OFF;
>  
> -	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
> +	mvebu_writel(port, pci_conf1_ext_addr(bus->number, devfn, where, true),
>  		     PCIE_CONF_ADDR_OFF);
>  
>  	switch (size) {
> @@ -397,7 +388,7 @@ static int mvebu_pcie_child_wr_conf(struct pci_bus *bus, u32 devfn,
>  
>  	conf_data = port->base + PCIE_CONF_DATA_OFF;
>  
> -	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
> +	mvebu_writel(port, pci_conf1_ext_addr(bus->number, devfn, where, true),
>  		     PCIE_CONF_ADDR_OFF);
>  
>  	switch (size) {
> -- 
> 2.39.2
> 

Re: [PATCH 09/10] PCI: mvebu: Use generic PCI Conf Type 1 helper

[Andrew Lunn]

On Mon, Apr 29, 2024 at 01:46:32PM +0300, Ilpo Järvinen wrote:
> Convert mvebu to use the pci_conf1_ext_addr() helper from PCI core
> to calculate PCI Configuration Space address for Type 1 access.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Tested on a kirkwood QNAP and an Armanda XP. The kirkwood correctly
reports there is nothing on the PCIe bus. The XP finds the two 88W8864
WiFi devices, but there is no mainline driver for it, and it also
finds an Etron Technology USB controller, which enumerates O.K.

Tested-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 02/10] PCI: Add helpers to calculate PCI Conf Type 0/1 addresses

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 14780 bytes --]

On Mon, 29 Apr 2024, Pali Rohár wrote:

> On Monday 29 April 2024 13:46:25 Ilpo Järvinen wrote:
> > Many places in arch and PCI controller code need to calculate PCI
> > Configuration Space Addresses for Type 0/1 accesses. There are small
> > variations between archs when it comes to bits outside of [10:2] (Type
> > 0) and [24:2] (Type 1) but the basic calculation can still be
> > generalized.
> > 
> > drivers/pci/pci.h has PCI_CONF1{,_EXT}_ADDRESS() but due to their
> > location the use is limited to PCI subsys and the also always enable
> > PCI_CONF1_ENABLE which is not what all the callers want.
> > 
> > Add generic pci_conf{0,1}_addr() and pci_conf1_ext_addr() helpers into
> > include/linux/pci.h which can be reused by various parts of the kernel
> > that have to calculate PCI Conf Type 0/1 addresses.
> > 
> > The PCI_CONF* defines are needed by the new helpers so move also them
> > to include/linux/pci.h. The new helpers use true bitmasks and
> > FIELD_PREP() instead of open coded masking and shifting so adjust
> > PCI_CONF* definitions to match that.
>
> I introduced these PCI_CONF* macros years ago. At that time I studied
> more sources and drivers what they use. To make it clear I will first
> try to explain few things. Hopefully I do not write mistakes here, it is
> longer time.
> 
> Configuration mechanism is a SW API which allows access config space.
> Configuration access command is on-wire "format" which allows HW bus to
> access config space.
> 
> There are two standardized configuration mechanisms #1 and #2. #2 was
> removed in PCI 3.0 spec and is out of scope (there are no macros for
> them). #1 uses pair of I/O registers (on x86 they are CF8 and CFC; on
> ARM they are whatever vendor invented). There is an extension of #1
> which uses few reserved bits to describe config space registers above
> 255. Then there is standardized PCIe ECAM. And then there are lot of
> proprietary vendor specific ways. Proprietary vendor way can be for
> example composing PCIe packet manually or composing configuration access
> command manually.
>
> There are two configuration space access commands: type 0 and type 1.
> It is required to issue correct command type based on topology of the
> endpoint device. When mechanism #1 is used then it is HW who generate
> these commands and it takes care to correctly choose type 0 or 1. But if
> some vendor specific mechanism is used which requires SW to compose
> access command manually then SW is responsible for correct choose.
> 
> 
> In your change you have mixed configuration mechanism #1 together with
> configuration command for type 0. This is really a problem as it makes
> the code harder to understand and even hard to figure out how to write a
> new driver (e.g. how to compose configuration command for type 1?).
>
> Also it took me a little bit more time to understand your change.
> Format of command for type 1 and format of configuration mechanism #1
> really looks very similar. So there can be a confusion. Bit 31 is a key  
> there.

Thanks a lot for chimming in!

In practice, the calculation done is very similar for many archs. I 
initially was planning to only convert things in drivers/pci/pci.h to 
FIELD_PREP() and be done with it, but then I came across this (a rough and 
incomplete list):

$ git grep -e 'bus.*<<.*16' -B1 -A3 arch

So I ended up extending the scope and trying to find a common ground, 
which was to make functions into include/linux/pci.h to cover the main 
calculations. This should explain the placement which you asked in the 
other reply. I don't think drivers/pci/pci.h is helpful when arch/ code 
has this very calculation repeated n times.

To me it looked obvious that those calculations relate to what is 
described in PCI Local Bus Spec r3.0 sec 3.2.2.3.1 (some even explicitly 
indicate type 0 or type 1).

I still had to find some name for the functions and didn't see any reason 
why I couldn't just use type 0 and type 1 as in that spec. To me, it's 
hardly an accident that mechanism #1 fields are 1:1 copy of type 1 
calculation but I think I can still see it also from your viewpoint. I 
perhaps just look this from more practical standpoint than you (no offence 
meant in any way).

I don't know if the vendor specific part is even relevant to this series
or if you just noted it for completeness. I'm looking for common ground 
here and if vendor's copying the existing format where they could have 
invented entirely custom formatting, but is that good enough reason to 
name the functions differently? Or falsely claim it's vendor specific when 
it's obviously nothing but copying the existing way (i.e., the fields from 
type 1)?

Perhaps the main wrong here was in the naming step or in the terminology 
I used in the commit messages, or do you think those calcs done under 
arch/ do not related to type 0/1 in any way (despite being 1:1 copy!)?


Lets take a look at a real function in kernel (this is not the only 
example, there are more similar ones under arch/, e.g., in
arch/alpha/kernel/core_lca.c):

static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
{
        /* Root bus is always 0 in this hardware */
        if (bus_num == 0) {
                /* type 0 */
                return (PCI_CONF1_ADDRESS(0, 0, PCI_FUNC(devfn), where) &
                        ~PCI_CONF1_ENABLE) | BIT(32-PCI_SLOT(devfn));
        } else {
                /* type 1 */
                return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
                                          PCI_FUNC(devfn), where) &
                        ~PCI_CONF1_ENABLE) | 1;
        }
}

So is this "mixing" #1 and type 0 together? Or how you categorize this?
I suspect, by your definition, it's actually abusing what is meant for 
mechanism #1 to calculate type 1 (obviously because the main calculation 
is undeniably very much the same :-))?

> I understand that you want to unify drivers, so I would suggest to not
> change existing macros for configuration mechanism #1 and #1-ext.
> And rather create new macros (or functions) for composing configuration
> commands.

And what about code under arch/? I see bits 24-27 and bit #31 being 
enabled there as well. Or do those categorize as "vendor specific" methods 
so nothing can be done to them?!?

> This makes it explicitly clear that particular PCI or PCIe controller HW
> requires SW driver to compose configuration command (type 0 and 1). Or
> that HW requires from SW to compose format of configuration mechanism #1
> (or #1-ext). Also it would make pci controller drivers more readable as
> from the macro/function it would be easy to understand what it is doing.

So does this effectively boil down to instead of having a boolean enable
parameter for bit #31, I should create two functions with different names
(well, reusing the existing ones perhaps for one of them but the same 
idea)? Couldn't I just name the function and that enable parameter 
differently and/or document things better without having two functions?
It's not like these these two things came to be mostly the same by chance,
they're very much related.

> Also important is that if you are in pci controller driver composing
> command for type 0 then with very high probability the HW is designed in
> a way that it requires from SW to also compose command type 1. And it
> would be an mistake if the driver compose only type 0 or type 1. I
> remember from the past an issue: why endpoint PCIe card is working, but
> it is not working if it is connected behind PCIe switch. (mistake was:
> HW was always sending type 0 command due to SW issue)

Do you think this would warrant something that actually combines the 
type 0/1 calculations inside? Just asking your opinion, I'm not sure how 
easy the arch code would be to adapt such a bigger change because of the
variations.

-- 
 i. 

> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/pci/pci.h   | 43 ++---------------------
> >  include/linux/pci.h | 85 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 88 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 17fed1846847..cf0530a60105 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -833,49 +833,12 @@ struct pci_devres {
> >  
> >  struct pci_devres *find_pci_dr(struct pci_dev *pdev);
> >  
> > -/*
> > - * Config Address for PCI Configuration Mechanism #1
> > - *
> > - * See PCI Local Bus Specification, Revision 3.0,
> > - * Section 3.2.2.3.2, Figure 3-2, p. 50.
> > - */
> > -
> > -#define PCI_CONF1_BUS_SHIFT	16 /* Bus number */
> > -#define PCI_CONF1_DEV_SHIFT	11 /* Device number */
> > -#define PCI_CONF1_FUNC_SHIFT	8  /* Function number */
> > -
> > -#define PCI_CONF1_BUS_MASK	0xff
> > -#define PCI_CONF1_DEV_MASK	0x1f
> > -#define PCI_CONF1_FUNC_MASK	0x7
> > -#define PCI_CONF1_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */
> > -
> > -#define PCI_CONF1_ENABLE	BIT(31)
> > -#define PCI_CONF1_BUS(x)	(((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT)
> > -#define PCI_CONF1_DEV(x)	(((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT)
> > -#define PCI_CONF1_FUNC(x)	(((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT)
> > -#define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
> > -
> >  #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
> >  	(PCI_CONF1_ENABLE | \
> > -	 PCI_CONF1_BUS(bus) | \
> > -	 PCI_CONF1_DEV(dev) | \
> > -	 PCI_CONF1_FUNC(func) | \
> > -	 PCI_CONF1_REG(reg))
> > -
> > -/*
> > - * Extension of PCI Config Address for accessing extended PCIe registers
> > - *
> > - * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> > - * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> > - * are used for specifying additional 4 high bits of PCI Express register.
> > - */
> > -
> > -#define PCI_CONF1_EXT_REG_SHIFT	16
> > -#define PCI_CONF1_EXT_REG_MASK	0xf00
> > -#define PCI_CONF1_EXT_REG(x)	(((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
> > +	 pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> >  
> >  #define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
> > -	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
> > -	 PCI_CONF1_EXT_REG(reg))
> > +	(PCI_CONF1_ENABLE | \
> > +	 pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> >  
> >  #endif /* DRIVERS_PCI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 16493426a04f..4c4e3bb52a0a 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -26,6 +26,8 @@
> >  #include <linux/args.h>
> >  #include <linux/mod_devicetable.h>
> >  
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/types.h>
> >  #include <linux/init.h>
> >  #include <linux/ioport.h>
> > @@ -1183,6 +1185,89 @@ void pci_sort_breadthfirst(void);
> >  #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
> >  #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
> >  
> > +/*
> > + * Config Address for PCI Configuration Mechanism #0/1
> > + *
> > + * See PCI Local Bus Specification, Revision 3.0,
> > + * Section 3.2.2.3.2, Figure 3-1 and 3-2, p. 48-50.
> > + */
> > +#define PCI_CONF_REG		0x000000ffU	/* common for Type 0/1 */
> > +#define PCI_CONF_FUNC		0x00000700U	/* common for Type 0/1 */
> > +#define PCI_CONF1_DEV		0x0000f800U
> > +#define PCI_CONF1_BUS		0x00ff0000U
> > +#define PCI_CONF1_ENABLE	BIT(31)
> > +
> > +/**
> > + * pci_conf0_addr - PCI Base Configuration Space address for Type 0 access
> > + * @devfn:      Device and function numbers (device number will be ignored)
> > + * @reg:        Base configuration space offset
> > + *
> > + * Calculates the PCI Configuration Space address for Type 0 accesses.
> > + *
> > + * Note: the caller is responsible for adding the bits outside of [10:0].
> > + *
> > + * Return: Base Configuration Space address.
> > + */
> > +static inline u32 pci_conf0_addr(u8 devfn, u8 reg)
> > +{
> > +	return FIELD_PREP(PCI_CONF_FUNC, PCI_FUNC(devfn)) |
> > +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> > +}
> > +
> > +/**
> > + * pci_conf1_addr - PCI Base Configuration Space address for Type 1 access
> > + * @bus:	Bus number of the device
> > + * @devfn:	Device and function numbers
> > + * @reg:	Base configuration space offset
> > + * @enable:	Assert enable bit (bit 31)
> > + *
> > + * Calculates the PCI Base Configuration Space (first 256 bytes) address for
> > + * Type 1 accesses.
> > + *
> > + * Note: the caller is responsible for adding the bits outside of [24:2]
> > + * and enable bit.
> > + *
> > + * Return: PCI Base Configuration Space address.
> > + */
> > +static inline u32 pci_conf1_addr(u8 bus, u8 devfn, u8 reg, bool enable)
> > +{
> > +	return (enable ? PCI_CONF1_ENABLE : 0) |
> > +	       FIELD_PREP(PCI_CONF1_BUS, bus) |
> > +	       FIELD_PREP(PCI_CONF1_DEV | PCI_CONF_FUNC, devfn) |
> > +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> > +}
> > +
> > +/*
> > + * Extension of PCI Config Address for accessing extended PCIe registers
> > + *
> > + * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> > + * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> > + * are used for specifying additional 4 high bits of PCI Express register.
> > + */
> > +#define PCI_CONF1_EXT_REG	0x0f000000UL
> > +
> > +/**
> > + * pci_conf1_ext_addr - PCI Configuration Space address for Type 1 access
> > + * @bus:	Bus number of the device
> > + * @devfn:	Device and function numbers
> > + * @reg:	Base or Extended Configuration space offset
> > + * @enable:	Assert enable bit (bit 31)
> > + *
> > + * Calculates the PCI Base and Extended (4096 bytes per PCI function)
> > + * Configuration Space address for Type 1 accesses. This function assumes
> > + * the Extended Conguration Space is using the reserved bits [27:24].
> > + *
> > + * Note: the caller is responsible for adding the bits outside of [27:2] and
> > + * enable bit.
> > + *
> > + * Return: PCI Configuration Space address.
> > + */
> > +static inline u32 pci_conf1_ext_addr(u8 bus, u8 devfn, u16 reg, bool enable)
> > +{
> > +	return FIELD_PREP(PCI_CONF1_EXT_REG, (reg & 0xf00) >> 8) |
> > +	       pci_conf1_addr(bus, devfn, reg & 0xff, enable);
> > +}
> > +
> >  /* Generic PCI functions exported to card drivers */
> >  
> >  u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> > -- 
> > 2.39.2
> > 
> 

Re: [PATCH 00/10] PCI: Add generic Conf Type 0/1 helpers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Mon, 29 Apr 2024, Pali Rohár wrote:

> On Monday 29 April 2024 13:46:23 Ilpo Järvinen wrote:
> > This series replaces PCI_CONF1{,_EXT}_ADDRESS() with more generic
> > helpers and makes them more widely available by placing the new helpers
> > into include/linux/pci.h.
> 
> There was a request to move these macros from include/linux/pci.h file
> to the driver/pci/pci.h file...
> 
> https://lore.kernel.org/linux-pci/20220913211143.GA624473@bhelgaas/
> 
> Something was changed that it has to be moved back?

I wasn't aware of this thread.

I'm actually a bit surprised by that suggestion because of the abundance 
of arch/ examples but maybe this too boils down to the configuration 
mechanism vs configuration commands thing.

-- 
 i.

Re: [PATCH 02/10] PCI: Add helpers to calculate PCI Conf Type 0/1 addresses

[Pali Rohár]

On Tuesday 30 April 2024 13:21:23 Ilpo Järvinen wrote:
> On Mon, 29 Apr 2024, Pali Rohár wrote:
> 
> > On Monday 29 April 2024 13:46:25 Ilpo Järvinen wrote:
> > > Many places in arch and PCI controller code need to calculate PCI
> > > Configuration Space Addresses for Type 0/1 accesses. There are small
> > > variations between archs when it comes to bits outside of [10:2] (Type
> > > 0) and [24:2] (Type 1) but the basic calculation can still be
> > > generalized.
> > > 
> > > drivers/pci/pci.h has PCI_CONF1{,_EXT}_ADDRESS() but due to their
> > > location the use is limited to PCI subsys and the also always enable
> > > PCI_CONF1_ENABLE which is not what all the callers want.
> > > 
> > > Add generic pci_conf{0,1}_addr() and pci_conf1_ext_addr() helpers into
> > > include/linux/pci.h which can be reused by various parts of the kernel
> > > that have to calculate PCI Conf Type 0/1 addresses.
> > > 
> > > The PCI_CONF* defines are needed by the new helpers so move also them
> > > to include/linux/pci.h. The new helpers use true bitmasks and
> > > FIELD_PREP() instead of open coded masking and shifting so adjust
> > > PCI_CONF* definitions to match that.
> >
> > I introduced these PCI_CONF* macros years ago. At that time I studied
> > more sources and drivers what they use. To make it clear I will first
> > try to explain few things. Hopefully I do not write mistakes here, it is
> > longer time.
> > 
> > Configuration mechanism is a SW API which allows access config space.
> > Configuration access command is on-wire "format" which allows HW bus to
> > access config space.
> > 
> > There are two standardized configuration mechanisms #1 and #2. #2 was
> > removed in PCI 3.0 spec and is out of scope (there are no macros for
> > them). #1 uses pair of I/O registers (on x86 they are CF8 and CFC; on
> > ARM they are whatever vendor invented). There is an extension of #1
> > which uses few reserved bits to describe config space registers above
> > 255. Then there is standardized PCIe ECAM. And then there are lot of
> > proprietary vendor specific ways. Proprietary vendor way can be for
> > example composing PCIe packet manually or composing configuration access
> > command manually.
> >
> > There are two configuration space access commands: type 0 and type 1.
> > It is required to issue correct command type based on topology of the
> > endpoint device. When mechanism #1 is used then it is HW who generate
> > these commands and it takes care to correctly choose type 0 or 1. But if
> > some vendor specific mechanism is used which requires SW to compose
> > access command manually then SW is responsible for correct choose.
> > 
> > 
> > In your change you have mixed configuration mechanism #1 together with
> > configuration command for type 0. This is really a problem as it makes
> > the code harder to understand and even hard to figure out how to write a
> > new driver (e.g. how to compose configuration command for type 1?).
> >
> > Also it took me a little bit more time to understand your change.
> > Format of command for type 1 and format of configuration mechanism #1
> > really looks very similar. So there can be a confusion. Bit 31 is a key  
> > there.
> 
> Thanks a lot for chimming in!
> 
> In practice, the calculation done is very similar for many archs. I 
> initially was planning to only convert things in drivers/pci/pci.h to 
> FIELD_PREP() and be done with it, but then I came across this (a rough and 
> incomplete list):
> 
> $ git grep -e 'bus.*<<.*16' -B1 -A3 arch
> 
> So I ended up extending the scope and trying to find a common ground, 
> which was to make functions into include/linux/pci.h to cover the main 
> calculations. This should explain the placement which you asked in the 
> other reply. I don't think drivers/pci/pci.h is helpful when arch/ code 
> has this very calculation repeated n times.

I know. In past I have called similar git grep commands and at that time
I also come to similar conclusion and that is why I originally also
suggested to provide macros in include/linux/*.h

> To me it looked obvious that those calculations relate to what is 
> described in PCI Local Bus Spec r3.0 sec 3.2.2.3.1 (some even explicitly 
> indicate type 0 or type 1).

It is important to see that there are sections 3.2.2.3.1. and 3.2.2.3.2.
which describes different things.

> I still had to find some name for the functions and didn't see any reason 
> why I couldn't just use type 0 and type 1 as in that spec. To me, it's 
> hardly an accident that mechanism #1 fields are 1:1 copy of type 1 
> calculation but I think I can still see it also from your viewpoint. I 
> perhaps just look this from more practical standpoint than you (no offence 
> meant in any way).

The best names of the functions is to use terminology from the
standards (instead of inventing new custom terminology). Because
standards are "standards", so anybody from the specific industry would
be able to better understand the code.

> I don't know if the vendor specific part is even relevant to this series
> or if you just noted it for completeness.

For completeness because it is very common and it is important to know
about it. Also mechanism #1-ext is vendor specific but somehow used by
lot of vendors.

> I'm looking for common ground 
> here and if vendor's copying the existing format where they could have 
> invented entirely custom formatting, but is that good enough reason to 
> name the functions differently? Or falsely claim it's vendor specific when 
> it's obviously nothing but copying the existing way (i.e., the fields from 
> type 1)?

"type 1 command" has exact definition and well-defined usage. It is used
for communication with device behind the PCI-to-PCI bridge. PCIe
endpoint device has to reply with status unsupported request.

Using "type 1 command" in function name which sends arbitrary command
(including type 0; e.g. for filling mechanism #1 format) is misleading
and would open questions like: why is using function "type 1 command"
for sending "type 0 command"?

Reason that some bits in the formats have same meaning does not mean to
use wrong names.

> Perhaps the main wrong here was in the naming step or in the terminology 
> I used in the commit messages, or do you think those calcs done under 
> arch/ do not related to type 0/1 in any way (despite being 1:1 copy!)?

For example what is wrong is this documentation:

"pci_conf1_ext_addr - PCI Configuration Space address for Type 1 access"

This function does not generate Type 1 command as it does not sets
BIT(0) to 1. It let BIT(0) as 0, which means that it generates Type 0
command.

It needs to be properly investigated if the HW which targets code in
arch/ generates type 0/1 commands in SW or in HW.

> Lets take a look at a real function in kernel (this is not the only 
> example, there are more similar ones under arch/, e.g., in
> arch/alpha/kernel/core_lca.c):
> 
> static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
> {
>         /* Root bus is always 0 in this hardware */
>         if (bus_num == 0) {
>                 /* type 0 */
>                 return (PCI_CONF1_ADDRESS(0, 0, PCI_FUNC(devfn), where) &
>                         ~PCI_CONF1_ENABLE) | BIT(32-PCI_SLOT(devfn));
>         } else {
>                 /* type 1 */
>                 return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
>                                           PCI_FUNC(devfn), where) &
>                         ~PCI_CONF1_ENABLE) | 1;
>         }
> }
> 
> So is this "mixing" #1 and type 0 together? Or how you categorize this?
> I suspect, by your definition, it's actually abusing what is meant for 
> mechanism #1 to calculate type 1 (obviously because the main calculation 
> is undeniably very much the same :-))?

So first is: Is this code correct? Are not you just trying to create
some new functions and abstractions for something which is wrong? As I
wrote in previous email, I have an experience with wrongly written SW
which incorrectly generated config requests.

The example is for some alpha HW, I guess something not commonly used,
so if there is a bug in that code, there is very little chance that
somebody will report it.

The worst thing which can happen is to design some abstraction based on
wrong assumptions (or wrong code). As it would lead to new bugs in newly
written code. New designed abstraction should prevent it.

> > I understand that you want to unify drivers, so I would suggest to not
> > change existing macros for configuration mechanism #1 and #1-ext.
> > And rather create new macros (or functions) for composing configuration
> > commands.
> 
> And what about code under arch/? I see bits 24-27 and bit #31 being 
> enabled there as well. Or do those categorize as "vendor specific" methods 
> so nothing can be done to them?!?

Do you mean above ixp4xx_config_addr? The correct answer is to look into
HW documentation. It is hard to predict but I guess that it is
constructions of the format for raw commands. When bus_num is not 0 then
is is type 1 command. And if bus_num is 0 then bits 11-31 are reserved
(in HW can be hardwired to zero) so BIT(32-PCI_SLOT(devfn)) can do
nothing. But this is only my assumption and reality can be different.
It is really common do to such mistake if you do not read documentation
properly or have assumptions from wrong input information (like mixing
formats of type 0 command and mech #1).

> > This makes it explicitly clear that particular PCI or PCIe controller HW
> > requires SW driver to compose configuration command (type 0 and 1). Or
> > that HW requires from SW to compose format of configuration mechanism #1
> > (or #1-ext). Also it would make pci controller drivers more readable as
> > from the macro/function it would be easy to understand what it is doing.
> 
> So does this effectively boil down to instead of having a boolean enable
> parameter for bit #31, I should create two functions with different names
> (well, reusing the existing ones perhaps for one of them but the same 
> idea)? Couldn't I just name the function and that enable parameter 
> differently and/or document things better without having two functions?
> It's not like these these two things came to be mostly the same by chance,
> they're very much related.

What I'm trying to say. If your HW requires from SW to generate RAW
commands, then driver has to implement logic for generating _both_ type0
and type1 commands and also needs to have logic to correctly choose
which command to generate. If you are developer of the driver for such
HW then the worst what can happen is that you forgot to add logic for
generating type 0 commands. And finding such bug can be really hard for
somebody who does not know difference between mech #1 and type 1.

mech #1 is something which generates _both_ type 0 and type 1.

When working with mech #1 I do not see any reason why driver would want
to disable access (by setting enable bit to 0). So I think that helper
kernel function should always set enable bit to 1. If there is a reason
to disable access then there could be another function (without
arguments) which will do it.

It is wrong to call function with enable=0 and expecting that it would
_enable_ access to HW.


So I'm suggesting to:

1) let macros for mech #1 and #1-ext there as is

2) introduce new macros (functions, abstractions, ...) for generating
configuration commands type 0 and type 1.

It can be either one macro which takes an argument type of the command
(0 or 1, ideally asserted that no other value is valid). Or it can be
two commands, one for type 0 and one for type 1.

Type of the command is defined by BIT(1) and BIT(0).

Personally I'm for having two macros, one for type 0 and one for type 1.
And that is because type 0 command has only function number and register
number. Whether type 1 has additionally bus number and device number. So
it would be easier to look if correct parameters are passed for
particular format.

So if author of driver figure out that has to write a code which needs
to issue type 1 command, would be also forced to write a code for
issuing type 0 command. And for other people it would be easy to review
if driver has code for both type 0 and type 1.

For example having macros:

  PCI_CONF1_ADDRESS(bus, dev, func, reg)

  PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg)

  PCI_CONF_COMMAND_TYPE0_ADDRESS(func, reg)

  PCI_CONF_COMMAND_TYPE1_ADDRESS(bus, dev, func, reg)

(and maybe PCI_CONF_COMMAND_TYPE1_EXT_ADDRESS if there is such vendor ext)

I'm not sure if the PCI_CONF_COMMAND_TYPEX_ADDRESS is the best name, but
I wrote something as an example.

3) switch drivers to use new macros. And figure out if the drivers are
   not buggy and are trying to sets wire-0 bits...

> > Also important is that if you are in pci controller driver composing
> > command for type 0 then with very high probability the HW is designed in
> > a way that it requires from SW to also compose command type 1. And it
> > would be an mistake if the driver compose only type 0 or type 1. I
> > remember from the past an issue: why endpoint PCIe card is working, but
> > it is not working if it is connected behind PCIe switch. (mistake was:
> > HW was always sending type 0 command due to SW issue)
> 
> Do you think this would warrant something that actually combines the 
> type 0/1 calculations inside? Just asking your opinion, I'm not sure how 
> easy the arch code would be to adapt such a bigger change because of the
> variations.

I do not know how hard is to refactor all arch/ code. I guess there are
lot of code from history, which may be wrong/buggy and for which nobody
is doing periodic testing. Nowadays new pci controller drivers are in
the drivers/pci/ subdir. So maybe the best option is to let arch/ pci
code as is?

> -- 
>  i. 
> 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > >  drivers/pci/pci.h   | 43 ++---------------------
> > >  include/linux/pci.h | 85 +++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 88 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 17fed1846847..cf0530a60105 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -833,49 +833,12 @@ struct pci_devres {
> > >  
> > >  struct pci_devres *find_pci_dr(struct pci_dev *pdev);
> > >  
> > > -/*
> > > - * Config Address for PCI Configuration Mechanism #1
> > > - *
> > > - * See PCI Local Bus Specification, Revision 3.0,
> > > - * Section 3.2.2.3.2, Figure 3-2, p. 50.
> > > - */
> > > -
> > > -#define PCI_CONF1_BUS_SHIFT	16 /* Bus number */
> > > -#define PCI_CONF1_DEV_SHIFT	11 /* Device number */
> > > -#define PCI_CONF1_FUNC_SHIFT	8  /* Function number */
> > > -
> > > -#define PCI_CONF1_BUS_MASK	0xff
> > > -#define PCI_CONF1_DEV_MASK	0x1f
> > > -#define PCI_CONF1_FUNC_MASK	0x7
> > > -#define PCI_CONF1_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */
> > > -
> > > -#define PCI_CONF1_ENABLE	BIT(31)
> > > -#define PCI_CONF1_BUS(x)	(((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT)
> > > -#define PCI_CONF1_DEV(x)	(((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT)
> > > -#define PCI_CONF1_FUNC(x)	(((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT)
> > > -#define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
> > > -
> > >  #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
> > >  	(PCI_CONF1_ENABLE | \
> > > -	 PCI_CONF1_BUS(bus) | \
> > > -	 PCI_CONF1_DEV(dev) | \
> > > -	 PCI_CONF1_FUNC(func) | \
> > > -	 PCI_CONF1_REG(reg))
> > > -
> > > -/*
> > > - * Extension of PCI Config Address for accessing extended PCIe registers
> > > - *
> > > - * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> > > - * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> > > - * are used for specifying additional 4 high bits of PCI Express register.
> > > - */
> > > -
> > > -#define PCI_CONF1_EXT_REG_SHIFT	16
> > > -#define PCI_CONF1_EXT_REG_MASK	0xf00
> > > -#define PCI_CONF1_EXT_REG(x)	(((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
> > > +	 pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> > >  
> > >  #define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
> > > -	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
> > > -	 PCI_CONF1_EXT_REG(reg))
> > > +	(PCI_CONF1_ENABLE | \
> > > +	 pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> > >  
> > >  #endif /* DRIVERS_PCI_H */
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 16493426a04f..4c4e3bb52a0a 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -26,6 +26,8 @@
> > >  #include <linux/args.h>
> > >  #include <linux/mod_devicetable.h>
> > >  
> > > +#include <linux/bits.h>
> > > +#include <linux/bitfield.h>
> > >  #include <linux/types.h>
> > >  #include <linux/init.h>
> > >  #include <linux/ioport.h>
> > > @@ -1183,6 +1185,89 @@ void pci_sort_breadthfirst(void);
> > >  #define dev_is_pci(d) ((d)->bus == &pci_bus_type)
> > >  #define dev_is_pf(d) ((dev_is_pci(d) ? to_pci_dev(d)->is_physfn : false))
> > >  
> > > +/*
> > > + * Config Address for PCI Configuration Mechanism #0/1
> > > + *
> > > + * See PCI Local Bus Specification, Revision 3.0,
> > > + * Section 3.2.2.3.2, Figure 3-1 and 3-2, p. 48-50.
> > > + */
> > > +#define PCI_CONF_REG		0x000000ffU	/* common for Type 0/1 */
> > > +#define PCI_CONF_FUNC		0x00000700U	/* common for Type 0/1 */
> > > +#define PCI_CONF1_DEV		0x0000f800U
> > > +#define PCI_CONF1_BUS		0x00ff0000U
> > > +#define PCI_CONF1_ENABLE	BIT(31)
> > > +
> > > +/**
> > > + * pci_conf0_addr - PCI Base Configuration Space address for Type 0 access
> > > + * @devfn:      Device and function numbers (device number will be ignored)
> > > + * @reg:        Base configuration space offset
> > > + *
> > > + * Calculates the PCI Configuration Space address for Type 0 accesses.
> > > + *
> > > + * Note: the caller is responsible for adding the bits outside of [10:0].
> > > + *
> > > + * Return: Base Configuration Space address.
> > > + */
> > > +static inline u32 pci_conf0_addr(u8 devfn, u8 reg)
> > > +{
> > > +	return FIELD_PREP(PCI_CONF_FUNC, PCI_FUNC(devfn)) |
> > > +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> > > +}
> > > +
> > > +/**
> > > + * pci_conf1_addr - PCI Base Configuration Space address for Type 1 access
> > > + * @bus:	Bus number of the device
> > > + * @devfn:	Device and function numbers
> > > + * @reg:	Base configuration space offset
> > > + * @enable:	Assert enable bit (bit 31)
> > > + *
> > > + * Calculates the PCI Base Configuration Space (first 256 bytes) address for
> > > + * Type 1 accesses.
> > > + *
> > > + * Note: the caller is responsible for adding the bits outside of [24:2]
> > > + * and enable bit.
> > > + *
> > > + * Return: PCI Base Configuration Space address.
> > > + */
> > > +static inline u32 pci_conf1_addr(u8 bus, u8 devfn, u8 reg, bool enable)
> > > +{
> > > +	return (enable ? PCI_CONF1_ENABLE : 0) |
> > > +	       FIELD_PREP(PCI_CONF1_BUS, bus) |
> > > +	       FIELD_PREP(PCI_CONF1_DEV | PCI_CONF_FUNC, devfn) |
> > > +	       FIELD_PREP(PCI_CONF_REG, reg & ~3);
> > > +}
> > > +
> > > +/*
> > > + * Extension of PCI Config Address for accessing extended PCIe registers
> > > + *
> > > + * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
> > > + * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
> > > + * are used for specifying additional 4 high bits of PCI Express register.
> > > + */
> > > +#define PCI_CONF1_EXT_REG	0x0f000000UL
> > > +
> > > +/**
> > > + * pci_conf1_ext_addr - PCI Configuration Space address for Type 1 access
> > > + * @bus:	Bus number of the device
> > > + * @devfn:	Device and function numbers
> > > + * @reg:	Base or Extended Configuration space offset
> > > + * @enable:	Assert enable bit (bit 31)
> > > + *
> > > + * Calculates the PCI Base and Extended (4096 bytes per PCI function)
> > > + * Configuration Space address for Type 1 accesses. This function assumes
> > > + * the Extended Conguration Space is using the reserved bits [27:24].
> > > + *
> > > + * Note: the caller is responsible for adding the bits outside of [27:2] and
> > > + * enable bit.
> > > + *
> > > + * Return: PCI Configuration Space address.
> > > + */
> > > +static inline u32 pci_conf1_ext_addr(u8 bus, u8 devfn, u16 reg, bool enable)
> > > +{
> > > +	return FIELD_PREP(PCI_CONF1_EXT_REG, (reg & 0xf00) >> 8) |
> > > +	       pci_conf1_addr(bus, devfn, reg & 0xff, enable);
> > > +}
> > > +
> > >  /* Generic PCI functions exported to card drivers */
> > >  
> > >  u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> > > -- 
> > > 2.39.2
> > > 
> > 

Re: [PATCH 05/10] PCI: ixp4xx: Use generic PCI Conf Type 0 helper

[Linus Walleij]

On Mon, Apr 29, 2024 at 12:47 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:

> Convert Type 0 address calculation to use pci_conf0_offset() instead of
> abusing PCI_CONF1_ADDRESS().
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Looks better indeed.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 06/10] PCI: ixp4xx: Replace 1 with PCI_CONF1_TRANSACTION

[Linus Walleij]

On Mon, Apr 29, 2024 at 12:47 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:

> Add PCI_CONF1_TRANSACTION and replace literal 1 within PCI
> Configuration Space Type 1 address with it.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 07/10] PCI: Replace PCI_CONF1{,_EXT}_ADDRESS() with the new helpers

[Linus Walleij]

On Mon, Apr 29, 2024 at 12:47 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:

> Replace the old PCI_CONF1{,_EXT}_ADDRESS() helpers used to calculate
> PCI Configuration Space Type 1 addresses with the new
> pci_conf1{,_ext}_offset() helpers that are more generic and more widely
> available.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 10/10] PCI: v3: Use generic PCI Conf Type 0/1 helpers

[Linus Walleij]

On Mon, Apr 29, 2024 at 12:48 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:

> Convert v3 to use pci_conf{0,1}_addr() from PCI core to calculate PCI
> Configuration Space address for Type 0/1 access.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 07/10] PCI: Replace PCI_CONF1{,_EXT}_ADDRESS() with the new helpers

[Sergio Paracuellos]

Hi,

On Mon, Apr 29, 2024 at 12:47 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Replace the old PCI_CONF1{,_EXT}_ADDRESS() helpers used to calculate
> PCI Configuration Space Type 1 addresses with the new
> pci_conf1{,_ext}_offset() helpers that are more generic and more widely
> available.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/controller/pci-ftpci100.c | 6 ++----
>  drivers/pci/controller/pci-ixp4xx.c   | 5 ++---
>  drivers/pci/controller/pcie-mt7621.c  | 7 +++----
>  drivers/pci/pci.h                     | 8 --------
>  4 files changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
> index ffdeed25e961..a8d0217a0b94 100644
> --- a/drivers/pci/controller/pci-ftpci100.c
> +++ b/drivers/pci/controller/pci-ftpci100.c
> @@ -182,8 +182,7 @@ static int faraday_raw_pci_read_config(struct faraday_pci *p, int bus_number,
>                                        unsigned int fn, int config, int size,
>                                        u32 *value)
>  {
> -       writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
> -                                PCI_FUNC(fn), config),
> +       writel(pci_conf1_addr(bus_number, fn, config, true),
>                         p->base + FTPCI_CONFIG);
>
>         *value = readl(p->base + FTPCI_DATA);
> @@ -214,8 +213,7 @@ static int faraday_raw_pci_write_config(struct faraday_pci *p, int bus_number,
>  {
>         int ret = PCIBIOS_SUCCESSFUL;
>
> -       writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
> -                                PCI_FUNC(fn), config),
> +       writel(pci_conf1_addr(bus_number, fn, config, true),
>                         p->base + FTPCI_CONFIG);
>
>         switch (size) {
> diff --git a/drivers/pci/controller/pci-ixp4xx.c b/drivers/pci/controller/pci-ixp4xx.c
> index ec0125344ca1..fd52f4a3ef31 100644
> --- a/drivers/pci/controller/pci-ixp4xx.c
> +++ b/drivers/pci/controller/pci-ixp4xx.c
> @@ -192,9 +192,8 @@ static u32 ixp4xx_config_addr(u8 bus_num, u16 devfn, int where)
>                        BIT(32 - PCI_SLOT(devfn));
>         } else {
>                 /* type 1 */
> -               return (PCI_CONF1_ADDRESS(bus_num, PCI_SLOT(devfn),
> -                                         PCI_FUNC(devfn), where) &
> -                       ~PCI_CONF1_ENABLE) | PCI_CONF1_TRANSACTION;
> +               return pci_conf1_addr(bus_num, devfn, where, false) |
> +                      PCI_CONF1_TRANSACTION;
>         }
>  }
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 79e225edb42a..2b2d9828a910 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -127,8 +127,7 @@ static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
>                                          unsigned int devfn, int where)
>  {
>         struct mt7621_pcie *pcie = bus->sysdata;
> -       u32 address = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn),
> -                                           PCI_FUNC(devfn), where);
> +       u32 address = pci_conf1_ext_addr(bus->number, devfn, where, true);
>
>         writel_relaxed(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
>
> @@ -143,7 +142,7 @@ static struct pci_ops mt7621_pcie_ops = {
>
>  static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
>  {
> -       u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
> +       u32 address = pci_conf1_ext_addr(0, PCI_DEVFN(dev, 0), reg, true);
>
>         pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
>         return pcie_read(pcie, RALINK_PCI_CONFIG_DATA);
> @@ -152,7 +151,7 @@ static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
>  static void write_config(struct mt7621_pcie *pcie, unsigned int dev,
>                          u32 reg, u32 val)
>  {
> -       u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
> +       u32 address = pci_conf1_ext_addr(0, PCI_DEVFN(dev, 0), reg, true);
>
>         pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
>         pcie_write(pcie, val, RALINK_PCI_CONFIG_DATA);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index cf0530a60105..fdf9624b0b12 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -833,12 +833,4 @@ struct pci_devres {
>
>  struct pci_devres *find_pci_dr(struct pci_dev *pdev);
>
> -#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
> -       (PCI_CONF1_ENABLE | \
> -        pci_conf1_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> -
> -#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
> -       (PCI_CONF1_ENABLE | \
> -        pci_conf1_ext_addr(bus, PCI_DEVFN(dev, func), reg & ~0x3U))
> -
>  #endif /* DRIVERS_PCI_H */
> --
> 2.39.2
>

I have tested in a GnuBee v1 board based on mt7621 and all PCI
enumeration and so on is working properly. Hence, for MT7621:

Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

Re: [PATCH 01/10] ARM: orion5x: Rename PCI_CONF_{REG,FUNC}() out of the way

[Gregory CLEMENT]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> orion5x defines PCI_CONF_REG() and PCI_CONF_FUNC() that are problematic
> because PCI core is going to introduce defines with the same names.
>
> Add ORION5X prefix to those defines to avoid name conflicts.
>
> Note: as this is part of series that replaces the code in question
> anyway, only bare minimum renaming is done and other similarly named
> macros are not touched.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

As some other patches of the series depend on patches in the PCIe
subsystem, the best approach would be to let you apply the series
through the PCIe subsystem.

Thanks,

Gregory
> ---
>  arch/arm/mach-orion5x/pci.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 3313bc5a63ea..77ddab90f448 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -219,8 +219,8 @@ static int __init pcie_setup(struct pci_sys_data *sys)
>  /*
>   * PCI_CONF_ADDR bits
>   */
> -#define PCI_CONF_REG(reg)		((reg) & 0xfc)
> -#define PCI_CONF_FUNC(func)		(((func) & 0x3) << 8)
> +#define ORION5X_PCI_CONF_REG(reg)	((reg) & 0xfc)
> +#define ORION5X_PCI_CONF_FUNC(func)	(((func) & 0x3) << 8)
>  #define PCI_CONF_DEV(dev)		(((dev) & 0x1f) << 11)
>  #define PCI_CONF_BUS(bus)		(((bus) & 0xff) << 16)
>  #define PCI_CONF_ADDR_EN		(1 << 31)
> @@ -277,8 +277,8 @@ static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
>  	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(dev) | PCI_CONF_REG(where) |
> -		PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
> +		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
>  
>  	*val = readl(PCI_CONF_DATA);
>  
> @@ -301,8 +301,8 @@ static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
>  	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(dev) | PCI_CONF_REG(where) |
> -		PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
> +		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
>  
>  	if (size == 4) {
>  		__raw_writel(val, PCI_CONF_DATA);
> -- 
> 2.39.2

Re: [PATCH 03/10] ARM: orion5x: Pass devfn to orion5x_pci_hw_{rd,wr}_conf()

[Gregory CLEMENT]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> Pass the usual devfn instead of individual components into
> orion5x_pci_hw_{rd,wr}_conf() to make the change into
> pci_conf1_offset() in an upcoming commit easier.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

As some other patches of the series depend on patches in the PCIe
subsystem, the best approach would be to let you apply the series
through the PCIe subsystem.

Thanks,

Gregory


> ---
>  arch/arm/mach-orion5x/pci.c | 45 +++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 77ddab90f448..6376e1db6386 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -270,15 +270,15 @@ static int orion5x_pci_local_bus_nr(void)
>  	return((conf & PCI_P2P_BUS_MASK) >> PCI_P2P_BUS_OFFS);
>  }
>  
> -static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
> -					u32 where, u32 size, u32 *val)
> +static int orion5x_pci_hw_rd_conf(int bus, u8 devfn, u32 where,
> +				  u32 size, u32 *val)
>  {
>  	unsigned long flags;
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
>  	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
> -		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
> +		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
>  
>  	*val = readl(PCI_CONF_DATA);
>  
> @@ -292,8 +292,8 @@ static int orion5x_pci_hw_rd_conf(int bus, int dev, u32 func,
>  	return PCIBIOS_SUCCESSFUL;
>  }
>  
> -static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
> -					u32 where, u32 size, u32 val)
> +static int orion5x_pci_hw_wr_conf(int bus, u8 devfn, u32 where,
> +				  u32 size, u32 val)
>  {
>  	unsigned long flags;
>  	int ret = PCIBIOS_SUCCESSFUL;
> @@ -301,8 +301,8 @@ static int orion5x_pci_hw_wr_conf(int bus, int dev, u32 func,
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
>  	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(dev) | ORION5X_PCI_CONF_REG(where) |
> -		ORION5X_PCI_CONF_FUNC(func) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
> +		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
>  
>  	if (size == 4) {
>  		__raw_writel(val, PCI_CONF_DATA);
> @@ -347,8 +347,7 @@ static int orion5x_pci_rd_conf(struct pci_bus *bus, u32 devfn,
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
>  
> -	return orion5x_pci_hw_rd_conf(bus->number, PCI_SLOT(devfn),
> -					PCI_FUNC(devfn), where, size, val);
> +	return orion5x_pci_hw_rd_conf(bus->number, devfn, where, size, val);
>  }
>  
>  static int orion5x_pci_wr_conf(struct pci_bus *bus, u32 devfn,
> @@ -357,8 +356,7 @@ static int orion5x_pci_wr_conf(struct pci_bus *bus, u32 devfn,
>  	if (!orion5x_pci_valid_config(bus->number, devfn))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
> -	return orion5x_pci_hw_wr_conf(bus->number, PCI_SLOT(devfn),
> -					PCI_FUNC(devfn), where, size, val);
> +	return orion5x_pci_hw_wr_conf(bus->number, devfn, where, size, val);
>  }
>  
>  static struct pci_ops pci_ops = {
> @@ -375,12 +373,14 @@ static void __init orion5x_pci_set_bus_nr(int nr)
>  		 * PCI-X mode
>  		 */
>  		u32 pcix_status, bus, dev;
> +		u8 devfn;
>  		bus = (p2p & PCI_P2P_BUS_MASK) >> PCI_P2P_BUS_OFFS;
>  		dev = (p2p & PCI_P2P_DEV_MASK) >> PCI_P2P_DEV_OFFS;
> -		orion5x_pci_hw_rd_conf(bus, dev, 0, PCIX_STAT, 4, &pcix_status);
> +		devfn = PCI_DEVFN(dev, 0);
> +		orion5x_pci_hw_rd_conf(bus, devfn, PCIX_STAT, 4, &pcix_status);
>  		pcix_status &= ~PCIX_STAT_BUS_MASK;
>  		pcix_status |= (nr << PCIX_STAT_BUS_OFFS);
> -		orion5x_pci_hw_wr_conf(bus, dev, 0, PCIX_STAT, 4, pcix_status);
> +		orion5x_pci_hw_wr_conf(bus, devfn, PCIX_STAT, 4, pcix_status);
>  	} else {
>  		/*
>  		 * PCI Conventional mode
> @@ -393,15 +393,16 @@ static void __init orion5x_pci_set_bus_nr(int nr)
>  
>  static void __init orion5x_pci_master_slave_enable(void)
>  {
> -	int bus_nr, func, reg;
> +	int bus_nr, reg;
> +	u8 devfn;
>  	u32 val;
>  
>  	bus_nr = orion5x_pci_local_bus_nr();
> -	func = PCI_CONF_FUNC_STAT_CMD;
> +	devfn = PCI_DEVFN(0, PCI_CONF_FUNC_STAT_CMD);
>  	reg = PCI_CONF_REG_STAT_CMD;
> -	orion5x_pci_hw_rd_conf(bus_nr, 0, func, reg, 4, &val);
> +	orion5x_pci_hw_rd_conf(bus_nr, devfn, reg, 4, &val);
>  	val |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -	orion5x_pci_hw_wr_conf(bus_nr, 0, func, reg, 4, val | 0x7);
> +	orion5x_pci_hw_wr_conf(bus_nr, devfn, reg, 4, val | 0x7);
>  }
>  
>  static void __init orion5x_setup_pci_wins(void)
> @@ -424,7 +425,7 @@ static void __init orion5x_setup_pci_wins(void)
>  
>  	for (i = 0; i < dram->num_cs; i++) {
>  		const struct mbus_dram_window *cs = dram->cs + i;
> -		u32 func = PCI_CONF_FUNC_BAR_CS(cs->cs_index);
> +		u8 devfn = PCI_DEVFN(0, PCI_CONF_FUNC_BAR_CS(cs->cs_index));
>  		u32 reg;
>  		u32 val;
>  
> @@ -432,15 +433,15 @@ static void __init orion5x_setup_pci_wins(void)
>  		 * Write DRAM bank base address register.
>  		 */
>  		reg = PCI_CONF_REG_BAR_LO_CS(cs->cs_index);
> -		orion5x_pci_hw_rd_conf(bus, 0, func, reg, 4, &val);
> +		orion5x_pci_hw_rd_conf(bus, devfn, reg, 4, &val);
>  		val = (cs->base & 0xfffff000) | (val & 0xfff);
> -		orion5x_pci_hw_wr_conf(bus, 0, func, reg, 4, val);
> +		orion5x_pci_hw_wr_conf(bus, devfn, reg, 4, val);
>  
>  		/*
>  		 * Write DRAM bank size register.
>  		 */
>  		reg = PCI_CONF_REG_BAR_HI_CS(cs->cs_index);
> -		orion5x_pci_hw_wr_conf(bus, 0, func, reg, 4, 0);
> +		orion5x_pci_hw_wr_conf(bus, devfn, reg, 4, 0);
>  		writel((cs->size - 1) & 0xfffff000,
>  			PCI_BAR_SIZE_DDR_CS(cs->cs_index));
>  		writel(cs->base & 0xfffff000,
> -- 
> 2.39.2

Re: [PATCH 04/10] ARM: orion5x: Use generic PCI Conf Type 1 helper

[Gregory CLEMENT]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> Convert orion5x PCI code to use pci_conf1_ext_addr() from PCI core to
> calculate PCI Configuration Type 1 address.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

As some other patches of the series depend on patches in the PCIe
subsystem, the best approach would be to let you apply the series
through the PCIe subsystem.

Thanks,

Gregory

> ---
>  arch/arm/mach-orion5x/pci.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 6376e1db6386..8b7d67549adf 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -216,15 +216,6 @@ static int __init pcie_setup(struct pci_sys_data *sys)
>  #define PCI_P2P_DEV_OFFS		24
>  #define PCI_P2P_DEV_MASK		(0x1f << PCI_P2P_DEV_OFFS)
>  
> -/*
> - * PCI_CONF_ADDR bits
> - */
> -#define ORION5X_PCI_CONF_REG(reg)	((reg) & 0xfc)
> -#define ORION5X_PCI_CONF_FUNC(func)	(((func) & 0x3) << 8)
> -#define PCI_CONF_DEV(dev)		(((dev) & 0x1f) << 11)
> -#define PCI_CONF_BUS(bus)		(((bus) & 0xff) << 16)
> -#define PCI_CONF_ADDR_EN		(1 << 31)
> -
>  /*
>   * Internal configuration space
>   */
> @@ -276,9 +267,7 @@ static int orion5x_pci_hw_rd_conf(int bus, u8 devfn, u32 where,
>  	unsigned long flags;
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
> -	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
> -		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +	writel(pci_conf1_addr(bus, devfn, where, true), PCI_CONF_ADDR);
>  
>  	*val = readl(PCI_CONF_DATA);
>  
> @@ -300,9 +289,7 @@ static int orion5x_pci_hw_wr_conf(int bus, u8 devfn, u32 where,
>  
>  	spin_lock_irqsave(&orion5x_pci_lock, flags);
>  
> -	writel(PCI_CONF_BUS(bus) |
> -		PCI_CONF_DEV(PCI_SLOT(devfn)) | ORION5X_PCI_CONF_REG(where) |
> -		ORION5X_PCI_CONF_FUNC(PCI_FUNC(devfn)) | PCI_CONF_ADDR_EN, PCI_CONF_ADDR);
> +	writel(pci_conf1_addr(bus, devfn, where, true), PCI_CONF_ADDR);
>  
>  	if (size == 4) {
>  		__raw_writel(val, PCI_CONF_DATA);
> -- 
> 2.39.2