Linux-Rockchip Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers
@ 2020-10-01 22:02 Krzysztof Wilczyński
  2020-10-02 15:19 ` Rob Herring
  2020-10-02 21:29 ` Bjorn Helgaas
  0 siblings, 2 replies; 5+ messages in thread
From: Krzysztof Wilczyński @ 2020-10-01 22:02 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rob Herring, Lorenzo Pieralisi, Heiko Stuebner, linux-pci,
	Shawn Lin, Jonathan Cameron, Michal Simek, linux-rockchip,
	Zhou Wang, Robert Richter, Jonathan Chocron, Toan Le,
	Will Deacon, linux-arm-kernel

Unify ECAM-related constants into a single set of standard constants
defining memory address shift values for the byte-level address that can
be used when accessing the PCI Express Configuration Space, and then
move native PCI Express controller drivers to use newly introduced
definitions retiring any driver-specific ones.

The ECAM ("Enhanced Configuration Access Mechanism") is defined by the
PCI Express specification (see PCI Express Base Specification, Revision
5.0, Version 1.0, Section 7.2.2, p. 676), thus most hardware should
implement it the same way.  Most of the native PCI Express controller
drivers define their ECAM-related constants, many of these could be
shared, or use open-coded values when setting the .bus_shift field of
the struct pci_ecam_ops.

All of the newly added constants should remove ambiguity and reduce the
number of open-coded values, and also correlate more strongly with the
descriptions in the aforementioned specification (see Table 7-1
"Enhanced Configuration Address Mapping", p. 677).

There is no change to functionality.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
Changes in v3:
  Updated commit message wording.
  Updated regarding custom ECAM bus shift values and concerning PCI base
  configuration space access for Type 1 access.
  Refactored rockchip_pcie_rd_other_conf() and rockchip_pcie_wr_other_conf()
  and removed the "busdev" variable.
  Removed surplus "relbus" variable from nwl_pcie_map_bus() and
  xilinx_pcie_map_bus().
  Renamed the PCIE_ECAM_ADDR() macro to PCIE_ECAM_OFFSET().

Changes in v2:
  Use PCIE_ECAM_ADDR macro when computing ECAM address offset, but drop
  PCI_SLOT and PCI_FUNC macros from the PCIE_ECAM_ADDR macro in favour
  of using a single value for the device/function.

 drivers/pci/controller/dwc/pcie-al.c        |  8 ++----
 drivers/pci/controller/dwc/pcie-hisi.c      |  4 +--
 drivers/pci/controller/pci-host-generic.c   |  4 +--
 drivers/pci/controller/pci-thunder-ecam.c   |  2 +-
 drivers/pci/controller/pci-thunder-pem.c    | 13 +++++++--
 drivers/pci/controller/pci-xgene.c          | 13 +++++++--
 drivers/pci/controller/pcie-rockchip-host.c | 27 +++++++++--------
 drivers/pci/controller/pcie-rockchip.h      |  8 +-----
 drivers/pci/controller/pcie-tango.c         |  2 +-
 drivers/pci/controller/pcie-xilinx-nwl.c    |  9 ++----
 drivers/pci/controller/pcie-xilinx.c        | 11 ++-----
 drivers/pci/ecam.c                          |  4 +--
 include/linux/pci-ecam.h                    | 32 +++++++++++++++++++++
 13 files changed, 83 insertions(+), 54 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
index d57d4ee15848..7c2aa049113c 100644
--- a/drivers/pci/controller/dwc/pcie-al.c
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -76,7 +76,7 @@ static int al_pcie_init(struct pci_config_window *cfg)
 }
 
 const struct pci_ecam_ops al_pcie_ops = {
-	.bus_shift    = 20,
+	.bus_shift    = PCIE_ECAM_BUS_SHIFT,
 	.init         =  al_pcie_init,
 	.pci_ops      = {
 		.map_bus    = al_pcie_map_bus,
@@ -138,8 +138,6 @@ struct al_pcie {
 	struct al_pcie_target_bus_cfg target_bus_cfg;
 };
 
-#define PCIE_ECAM_DEVFN(x)		(((x) & 0xff) << 12)
-
 #define to_al_pcie(x)		dev_get_drvdata((x)->dev)
 
 static inline u32 al_pcie_controller_readl(struct al_pcie *pcie, u32 offset)
@@ -228,7 +226,7 @@ static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
 	void __iomem *pci_base_addr;
 
 	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
-					 (busnr_ecam << 20) +
+					 PCIE_ECAM_BUS(busnr_ecam) +
 					 PCIE_ECAM_DEVFN(devfn));
 
 	if (busnr_reg != target_bus_cfg->reg_val) {
@@ -300,7 +298,7 @@ static void al_pcie_config_prepare(struct al_pcie *pcie)
 
 	target_bus_cfg = &pcie->target_bus_cfg;
 
-	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
+	ecam_bus_mask = (pcie->ecam_size >> PCIE_ECAM_BUS_SHIFT) - 1;
 	if (ecam_bus_mask > 255) {
 		dev_warn(pcie->dev, "ECAM window size is larger than 256MB. Cutting off at 256\n");
 		ecam_bus_mask = 255;
diff --git a/drivers/pci/controller/dwc/pcie-hisi.c b/drivers/pci/controller/dwc/pcie-hisi.c
index 5ca86796d43a..b7afbf1d4bd9 100644
--- a/drivers/pci/controller/dwc/pcie-hisi.c
+++ b/drivers/pci/controller/dwc/pcie-hisi.c
@@ -100,7 +100,7 @@ static int hisi_pcie_init(struct pci_config_window *cfg)
 }
 
 const struct pci_ecam_ops hisi_pcie_ops = {
-	.bus_shift    = 20,
+	.bus_shift    = PCIE_ECAM_BUS_SHIFT,
 	.init         =  hisi_pcie_init,
 	.pci_ops      = {
 		.map_bus    = hisi_pcie_map_bus,
@@ -135,7 +135,7 @@ static int hisi_pcie_platform_init(struct pci_config_window *cfg)
 }
 
 static const struct pci_ecam_ops hisi_pcie_platform_ops = {
-	.bus_shift    = 20,
+	.bus_shift    = PCIE_ECAM_BUS_SHIFT,
 	.init         =  hisi_pcie_platform_init,
 	.pci_ops      = {
 		.map_bus    = hisi_pcie_map_bus,
diff --git a/drivers/pci/controller/pci-host-generic.c b/drivers/pci/controller/pci-host-generic.c
index b51977abfdf1..593436aee7a0 100644
--- a/drivers/pci/controller/pci-host-generic.c
+++ b/drivers/pci/controller/pci-host-generic.c
@@ -15,7 +15,7 @@
 #include <linux/platform_device.h>
 
 static const struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
-	.bus_shift	= 16,
+	.bus_shift	= PCIE_CAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
 		.read		= pci_generic_config_read,
@@ -49,7 +49,7 @@ static void __iomem *pci_dw_ecam_map_bus(struct pci_bus *bus,
 }
 
 static const struct pci_ecam_ops pci_dw_ecam_bus_ops = {
-	.bus_shift	= 20,
+	.bus_shift	= PCIE_ECAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus	= pci_dw_ecam_map_bus,
 		.read		= pci_generic_config_read,
diff --git a/drivers/pci/controller/pci-thunder-ecam.c b/drivers/pci/controller/pci-thunder-ecam.c
index 7e8835fee5f7..22ed7e995b39 100644
--- a/drivers/pci/controller/pci-thunder-ecam.c
+++ b/drivers/pci/controller/pci-thunder-ecam.c
@@ -346,7 +346,7 @@ static int thunder_ecam_config_write(struct pci_bus *bus, unsigned int devfn,
 }
 
 const struct pci_ecam_ops pci_thunder_ecam_ops = {
-	.bus_shift	= 20,
+	.bus_shift	= PCIE_ECAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus        = pci_ecam_map_bus,
 		.read           = thunder_ecam_config_read,
diff --git a/drivers/pci/controller/pci-thunder-pem.c b/drivers/pci/controller/pci-thunder-pem.c
index 3f847969143e..1a3f70ac61fc 100644
--- a/drivers/pci/controller/pci-thunder-pem.c
+++ b/drivers/pci/controller/pci-thunder-pem.c
@@ -19,6 +19,15 @@
 #define PEM_CFG_WR 0x28
 #define PEM_CFG_RD 0x30
 
+/*
+ * Enhanced Configuration Access Mechanism (ECAM)
+ *
+ * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
+ * standard values defined in the PCI Express Base Specification see
+ * include/linux/pci-ecam.h.
+ */
+#define THUNDER_PCIE_ECAM_BUS_SHIFT	24
+
 struct thunder_pem_pci {
 	u32		ea_entry[3];
 	void __iomem	*pem_reg_base;
@@ -404,7 +413,7 @@ static int thunder_pem_acpi_init(struct pci_config_window *cfg)
 }
 
 const struct pci_ecam_ops thunder_pem_ecam_ops = {
-	.bus_shift	= 24,
+	.bus_shift	= THUNDER_PCIE_ECAM_BUS_SHIFT,
 	.init		= thunder_pem_acpi_init,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
@@ -441,7 +450,7 @@ static int thunder_pem_platform_init(struct pci_config_window *cfg)
 }
 
 static const struct pci_ecam_ops pci_thunder_pem_ops = {
-	.bus_shift	= 24,
+	.bus_shift	= THUNDER_PCIE_ECAM_BUS_SHIFT,
 	.init		= thunder_pem_platform_init,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 8e0db84f089d..e67e72818d24 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -60,6 +60,15 @@
 #define XGENE_PCIE_IP_VER_1		1
 #define XGENE_PCIE_IP_VER_2		2
 
+/*
+ * Enhanced Configuration Access Mechanism (ECAM)
+ *
+ * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
+ * standard values defined in the PCI Express Base Specification see
+ * include/linux/pci-ecam.h.
+ */
+#define XGENE_PCIE_ECAM_BUS_SHIFT	16
+
 #if defined(CONFIG_PCI_XGENE) || (defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS))
 struct xgene_pcie_port {
 	struct device_node	*node;
@@ -257,7 +266,7 @@ static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
 }
 
 const struct pci_ecam_ops xgene_v1_pcie_ecam_ops = {
-	.bus_shift	= 16,
+	.bus_shift	= XGENE_PCIE_ECAM_BUS_SHIFT,
 	.init		= xgene_v1_pcie_ecam_init,
 	.pci_ops	= {
 		.map_bus	= xgene_pcie_map_bus,
@@ -272,7 +281,7 @@ static int xgene_v2_pcie_ecam_init(struct pci_config_window *cfg)
 }
 
 const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = {
-	.bus_shift	= 16,
+	.bus_shift	= XGENE_PCIE_ECAM_BUS_SHIFT,
 	.init		= xgene_v2_pcie_ecam_init,
 	.pci_ops	= {
 		.map_bus	= xgene_pcie_map_bus,
diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 0bb2fb3e8a0b..4c069f8fa420 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -160,12 +160,11 @@ static int rockchip_pcie_rd_other_conf(struct rockchip_pcie *rockchip,
 				       struct pci_bus *bus, u32 devfn,
 				       int where, int size, u32 *val)
 {
-	u32 busdev;
+	void __iomem *addr;
 
-	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
-				PCI_FUNC(devfn), where);
+	addr = rockchip->reg_base + PCIE_ECAM_OFFSET(bus, devfn, where);
 
-	if (!IS_ALIGNED(busdev, size)) {
+	if (!IS_ALIGNED((uintptr_t)addr, size)) {
 		*val = 0;
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 	}
@@ -178,11 +177,11 @@ static int rockchip_pcie_rd_other_conf(struct rockchip_pcie *rockchip,
 						AXI_WRAPPER_TYPE1_CFG);
 
 	if (size == 4) {
-		*val = readl(rockchip->reg_base + busdev);
+		*val = readl(addr);
 	} else if (size == 2) {
-		*val = readw(rockchip->reg_base + busdev);
+		*val = readw(addr);
 	} else if (size == 1) {
-		*val = readb(rockchip->reg_base + busdev);
+		*val = readb(addr);
 	} else {
 		*val = 0;
 		return PCIBIOS_BAD_REGISTER_NUMBER;
@@ -194,11 +193,11 @@ static int rockchip_pcie_wr_other_conf(struct rockchip_pcie *rockchip,
 				       struct pci_bus *bus, u32 devfn,
 				       int where, int size, u32 val)
 {
-	u32 busdev;
+	void __iomem *addr;
 
-	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
-				PCI_FUNC(devfn), where);
-	if (!IS_ALIGNED(busdev, size))
+	addr = rockchip->reg_base + PCIE_ECAM_OFFSET(bus, devfn, where);
+
+	if (!IS_ALIGNED((uintptr_t)addr, size))
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
 	if (pci_is_root_bus(bus->parent))
@@ -209,11 +208,11 @@ static int rockchip_pcie_wr_other_conf(struct rockchip_pcie *rockchip,
 						AXI_WRAPPER_TYPE1_CFG);
 
 	if (size == 4)
-		writel(val, rockchip->reg_base + busdev);
+		writel(val, addr);
 	else if (size == 2)
-		writew(val, rockchip->reg_base + busdev);
+		writew(val, addr);
 	else if (size == 1)
-		writeb(val, rockchip->reg_base + busdev);
+		writeb(val, addr);
 	else
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index c7d0178fc8c2..1650a5087450 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -13,6 +13,7 @@
 
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/pci-ecam.h>
 
 /*
  * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
@@ -178,13 +179,6 @@
 #define MIN_AXI_ADDR_BITS_PASSED		8
 #define PCIE_RC_SEND_PME_OFF			0x11960
 #define ROCKCHIP_VENDOR_ID			0x1d87
-#define PCIE_ECAM_BUS(x)			(((x) & 0xff) << 20)
-#define PCIE_ECAM_DEV(x)			(((x) & 0x1f) << 15)
-#define PCIE_ECAM_FUNC(x)			(((x) & 0x7) << 12)
-#define PCIE_ECAM_REG(x)			(((x) & 0xfff) << 0)
-#define PCIE_ECAM_ADDR(bus, dev, func, reg) \
-	  (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \
-	   PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg))
 #define PCIE_LINK_IS_L2(x) \
 	(((x) & PCIE_CLIENT_DEBUG_LTSSM_MASK) == PCIE_CLIENT_DEBUG_LTSSM_L2)
 #define PCIE_LINK_UP(x) \
diff --git a/drivers/pci/controller/pcie-tango.c b/drivers/pci/controller/pcie-tango.c
index d093a8ce4bb1..8f0d695afbde 100644
--- a/drivers/pci/controller/pcie-tango.c
+++ b/drivers/pci/controller/pcie-tango.c
@@ -208,7 +208,7 @@ static int smp8759_config_write(struct pci_bus *bus, unsigned int devfn,
 }
 
 static const struct pci_ecam_ops smp8759_ecam_ops = {
-	.bus_shift	= 20,
+	.bus_shift	= PCIE_ECAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
 		.read		= smp8759_config_read,
diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index f3cf7d61924f..cfd12b75bacb 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -18,6 +18,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_irq.h>
 #include <linux/pci.h>
+#include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 #include <linux/irqchip/chained_irq.h>
 
@@ -124,8 +125,6 @@
 #define E_ECAM_CR_ENABLE		BIT(0)
 #define E_ECAM_SIZE_LOC			GENMASK(20, 16)
 #define E_ECAM_SIZE_SHIFT		16
-#define ECAM_BUS_LOC_SHIFT		20
-#define ECAM_DEV_LOC_SHIFT		12
 #define NWL_ECAM_VALUE_DEFAULT		12
 
 #define CFG_DMA_REG_BAR			GENMASK(2, 0)
@@ -240,15 +239,11 @@ static void __iomem *nwl_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
 				      int where)
 {
 	struct nwl_pcie *pcie = bus->sysdata;
-	int relbus;
 
 	if (!nwl_pcie_valid_device(bus, devfn))
 		return NULL;
 
-	relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
-			(devfn << ECAM_DEV_LOC_SHIFT);
-
-	return pcie->ecam_base + relbus + where;
+	return pcie->ecam_base + PCIE_ECAM_OFFSET(bus, devfn, where);
 }
 
 /* PCIe operations */
diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 8523be61bba5..49bde5266aa2 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -21,6 +21,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_irq.h>
 #include <linux/pci.h>
+#include <linux/pci-ecam.h>
 #include <linux/platform_device.h>
 
 #include "../pci.h"
@@ -86,10 +87,6 @@
 /* Phy Status/Control Register definitions */
 #define XILINX_PCIE_REG_PSCR_LNKUP	BIT(11)
 
-/* ECAM definitions */
-#define ECAM_BUS_NUM_SHIFT		20
-#define ECAM_DEV_NUM_SHIFT		12
-
 /* Number of MSI IRQs */
 #define XILINX_NUM_MSI_IRQS		128
 
@@ -183,15 +180,11 @@ static void __iomem *xilinx_pcie_map_bus(struct pci_bus *bus,
 					 unsigned int devfn, int where)
 {
 	struct xilinx_pcie_port *port = bus->sysdata;
-	int relbus;
 
 	if (!xilinx_pcie_valid_device(bus, devfn))
 		return NULL;
 
-	relbus = (bus->number << ECAM_BUS_NUM_SHIFT) |
-		 (devfn << ECAM_DEV_NUM_SHIFT);
-
-	return port->reg_base + relbus + where;
+	return port->reg_base + PCIE_ECAM_OFFSET(bus, devfn, where);
 }
 
 /* PCIe operations */
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 8f065a42fc1a..ffd010290084 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -149,7 +149,7 @@ EXPORT_SYMBOL_GPL(pci_ecam_map_bus);
 
 /* ECAM ops */
 const struct pci_ecam_ops pci_generic_ecam_ops = {
-	.bus_shift	= 20,
+	.bus_shift	= PCIE_ECAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
 		.read		= pci_generic_config_read,
@@ -161,7 +161,7 @@ EXPORT_SYMBOL_GPL(pci_generic_ecam_ops);
 #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
 /* ECAM ops for 32-bit access only (non-compliant) */
 const struct pci_ecam_ops pci_32b_ops = {
-	.bus_shift	= 20,
+	.bus_shift	= PCIE_ECAM_BUS_SHIFT,
 	.pci_ops	= {
 		.map_bus	= pci_ecam_map_bus,
 		.read		= pci_generic_config_read32,
diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h
index 1af5cb02ef7f..2c22da6bf818 100644
--- a/include/linux/pci-ecam.h
+++ b/include/linux/pci-ecam.h
@@ -9,6 +9,38 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 
+/*
+ * Memory address shift values for the byte-level address that
+ * can be used when accessing the PCI/PCI Express Configuration Space.
+ */
+
+/*
+ * Configuration Access Mechanism (CAM)
+ *
+ * See PCI Local Bus Specification, Revision 3.0, Section 3.2.2.3,
+ * p. 48 and Section 6.1, p. 213.
+ */
+#define PCIE_CAM_BUS_SHIFT	16 /* Bus Number */
+
+/*
+ * Enhanced Configuration Access Mechanism (ECAM)
+ *
+ * See PCI Express Base Specification, Revision 5.0, Version 1.0,
+ * Section 7.2.2, Table 7-1, p. 677.
+ */
+#define PCIE_ECAM_BUS_SHIFT	20 /* Bus Number */
+#define PCIE_ECAM_DEV_SHIFT	15 /* Device Number */
+#define PCIE_ECAM_FUN_SHIFT	12 /* Function Number */
+
+#define PCIE_ECAM_BUS(x)	(((x) & 0xff) << PCIE_ECAM_BUS_SHIFT)
+#define PCIE_ECAM_DEVFN(x)	(((x) & 0xff) << PCIE_ECAM_FUN_SHIFT)
+#define PCIE_ECAM_REG(x)	((x) & 0xfff)
+
+#define PCIE_ECAM_OFFSET(bus, devfn, where) \
+    (PCIE_ECAM_BUS(bus->number) | \
+     PCIE_ECAM_DEVFN(devfn) | \
+     PCIE_ECAM_REG(where))
+
 /*
  * struct to hold pci ops and bus shift of the config window
  * for a PCI controller.
-- 
2.28.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers
  2020-10-01 22:02 [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers Krzysztof Wilczyński
@ 2020-10-02 15:19 ` Rob Herring
  2020-10-02 20:20   ` Krzysztof Wilczyński
  2020-10-02 21:29 ` Bjorn Helgaas
  1 sibling, 1 reply; 5+ messages in thread
From: Rob Herring @ 2020-10-02 15:19 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Lorenzo Pieralisi, Heiko Stuebner, PCI, Shawn Lin,
	Jonathan Cameron, Michal Simek, open list:ARM/Rockchip SoC...,
	Zhou Wang, Robert Richter, Jonathan Chocron, Bjorn Helgaas,
	Toan Le, Will Deacon, linux-arm-kernel

On Thu, Oct 1, 2020 at 5:02 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Unify ECAM-related constants into a single set of standard constants
> defining memory address shift values for the byte-level address that can
> be used when accessing the PCI Express Configuration Space, and then
> move native PCI Express controller drivers to use newly introduced
> definitions retiring any driver-specific ones.
>
> The ECAM ("Enhanced Configuration Access Mechanism") is defined by the
> PCI Express specification (see PCI Express Base Specification, Revision
> 5.0, Version 1.0, Section 7.2.2, p. 676), thus most hardware should
> implement it the same way.  Most of the native PCI Express controller
> drivers define their ECAM-related constants, many of these could be
> shared, or use open-coded values when setting the .bus_shift field of
> the struct pci_ecam_ops.
>
> All of the newly added constants should remove ambiguity and reduce the
> number of open-coded values, and also correlate more strongly with the
> descriptions in the aforementioned specification (see Table 7-1
> "Enhanced Configuration Address Mapping", p. 677).
>
> There is no change to functionality.
>
> Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
> Changes in v3:
>   Updated commit message wording.
>   Updated regarding custom ECAM bus shift values and concerning PCI base
>   configuration space access for Type 1 access.
>   Refactored rockchip_pcie_rd_other_conf() and rockchip_pcie_wr_other_conf()
>   and removed the "busdev" variable.
>   Removed surplus "relbus" variable from nwl_pcie_map_bus() and
>   xilinx_pcie_map_bus().
>   Renamed the PCIE_ECAM_ADDR() macro to PCIE_ECAM_OFFSET().
>
> Changes in v2:
>   Use PCIE_ECAM_ADDR macro when computing ECAM address offset, but drop
>   PCI_SLOT and PCI_FUNC macros from the PCIE_ECAM_ADDR macro in favour
>   of using a single value for the device/function.
>
>  drivers/pci/controller/dwc/pcie-al.c        |  8 ++----
>  drivers/pci/controller/dwc/pcie-hisi.c      |  4 +--
>  drivers/pci/controller/pci-host-generic.c   |  4 +--
>  drivers/pci/controller/pci-thunder-ecam.c   |  2 +-
>  drivers/pci/controller/pci-thunder-pem.c    | 13 +++++++--
>  drivers/pci/controller/pci-xgene.c          | 13 +++++++--
>  drivers/pci/controller/pcie-rockchip-host.c | 27 +++++++++--------
>  drivers/pci/controller/pcie-rockchip.h      |  8 +-----
>  drivers/pci/controller/pcie-tango.c         |  2 +-
>  drivers/pci/controller/pcie-xilinx-nwl.c    |  9 ++----
>  drivers/pci/controller/pcie-xilinx.c        | 11 ++-----
>  drivers/pci/ecam.c                          |  4 +--
>  include/linux/pci-ecam.h                    | 32 +++++++++++++++++++++

What about vmd which I mentioned? I also found iproc and brcmstb are
ECAM (well, same shifts, but indirect addressing).

[...]

> +/*
> + * Enhanced Configuration Access Mechanism (ECAM)
> + *
> + * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
> + * standard values defined in the PCI Express Base Specification see
> + * include/linux/pci-ecam.h.
> + */
> +#define XGENE_PCIE_ECAM_BUS_SHIFT      16

Isn't this just CAM? Though perhaps CAM on PCIe is not standard...

For CAM, there's also tegra, ftpci100, mvebu, and versatile. I think
I'd drop CAM from this patch and do all of those in a separate patch.

Rob

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers
  2020-10-02 15:19 ` Rob Herring
@ 2020-10-02 20:20   ` Krzysztof Wilczyński
  2020-10-02 21:37     ` Bjorn Helgaas
  0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Wilczyński @ 2020-10-02 20:20 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lorenzo Pieralisi, Heiko Stuebner, PCI, Shawn Lin,
	Jonathan Cameron, Michal Simek, open list:ARM/Rockchip SoC...,
	Zhou Wang, Robert Richter, Jonathan Chocron, Bjorn Helgaas,
	Toan Le, Will Deacon, linux-arm-kernel

Hi Rob,

[...]
> What about vmd which I mentioned? I also found iproc and brcmstb are
> ECAM (well, same shifts, but indirect addressing).
[...]

I wanted to cover these (and some others I also found) in a separate
patch, especially since some of the drivers don't explicitly claim to
support ECAM - but I will include these changes in the v4. 
 
> > +/
> > + * Enhanced Configuration Access Mechanism (ECAM)
> > + *
> > + * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
> > + * standard values defined in the PCI Express Base Specification see
> > + * include/linux/pci-ecam.h.
> > + */
> > +#define XGENE_PCIE_ECAM_BUS_SHIFT      16
> 
> Isn't this just CAM? Though perhaps CAM on PCIe is not standard...
> 
> For CAM, there's also tegra, ftpci100, mvebu, and versatile. I think
> I'd drop CAM from this patch and do all of those in a separate patch.

Will do.

Bjorn was also not convinced about referring to things as "CAM" since
the specification (the one I quoted in the patch) does not name it as
such, and rather refers to it as Type 1 access of the PCI bus
configuration space.

Krzysztof

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers
  2020-10-01 22:02 [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers Krzysztof Wilczyński
  2020-10-02 15:19 ` Rob Herring
@ 2020-10-02 21:29 ` Bjorn Helgaas
  1 sibling, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2020-10-02 21:29 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Rob Herring, Lorenzo Pieralisi, Heiko Stuebner, linux-pci,
	Shawn Lin, Jonathan Cameron, Michal Simek, linux-rockchip,
	Zhou Wang, Robert Richter, Jonathan Chocron, Bjorn Helgaas,
	Toan Le, Will Deacon, linux-arm-kernel

On Thu, Oct 01, 2020 at 10:02:44PM +0000, Krzysztof Wilczyński wrote:
> Unify ECAM-related constants into a single set of standard constants
> defining memory address shift values for the byte-level address that can
> be used when accessing the PCI Express Configuration Space, and then
> move native PCI Express controller drivers to use newly introduced
> definitions retiring any driver-specific ones.
> 
> The ECAM ("Enhanced Configuration Access Mechanism") is defined by the
> PCI Express specification (see PCI Express Base Specification, Revision
> 5.0, Version 1.0, Section 7.2.2, p. 676), thus most hardware should
> implement it the same way.  Most of the native PCI Express controller
> drivers define their ECAM-related constants, many of these could be
> shared, or use open-coded values when setting the .bus_shift field of
> the struct pci_ecam_ops.
> 
> All of the newly added constants should remove ambiguity and reduce the
> number of open-coded values, and also correlate more strongly with the
> descriptions in the aforementioned specification (see Table 7-1
> "Enhanced Configuration Address Mapping", p. 677).

> --- a/drivers/pci/controller/pci-host-generic.c
> +++ b/drivers/pci/controller/pci-host-generic.c
> @@ -15,7 +15,7 @@
>  #include <linux/platform_device.h>
>  
>  static const struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = {
> -	.bus_shift	= 16,
> +	.bus_shift	= PCIE_CAM_BUS_SHIFT,

I'm not sure this code was safe even before you touched it.
pci_ecam_map_bus() doesn't limit "where" at all, so if we try to
access extended config space (offset 0x100 - 0xfff), I think we'll
generate

  (busnr << 16) | (devfn << 8) + where

If "where >= 0x100", we'll target the wrong device.

Even for ECAM, it doesn't look like anything prevents a defective or
malicious caller from supplying a config offset of, say, 0x2000 and
targeting the wrong device.

>  	.pci_ops	= {
>  		.map_bus	= pci_ecam_map_bus,
>  		.read		= pci_generic_config_read,

> --- a/drivers/pci/controller/pci-xgene.c
> +++ b/drivers/pci/controller/pci-xgene.c
> @@ -60,6 +60,15 @@
>  #define XGENE_PCIE_IP_VER_1		1
>  #define XGENE_PCIE_IP_VER_2		2
>  
> +/*
> + * Enhanced Configuration Access Mechanism (ECAM)
> + *
> + * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
> + * standard values defined in the PCI Express Base Specification see
> + * include/linux/pci-ecam.h.
> + */
> +#define XGENE_PCIE_ECAM_BUS_SHIFT	16

Is this even used anywhere?  xgene_pcie_map_bus() doesn't use
bus_shift.  Maybe we can just drop the .bus_shift initializers?

>  #if defined(CONFIG_PCI_XGENE) || (defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS))
>  struct xgene_pcie_port {
>  	struct device_node	*node;
> @@ -257,7 +266,7 @@ static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>  }
>  
>  const struct pci_ecam_ops xgene_v1_pcie_ecam_ops = {
> -	.bus_shift	= 16,
> +	.bus_shift	= XGENE_PCIE_ECAM_BUS_SHIFT,
>  	.init		= xgene_v1_pcie_ecam_init,
>  	.pci_ops	= {
>  		.map_bus	= xgene_pcie_map_bus,
> @@ -272,7 +281,7 @@ static int xgene_v2_pcie_ecam_init(struct pci_config_window *cfg)
>  }
>  
>  const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = {
> -	.bus_shift	= 16,
> +	.bus_shift	= XGENE_PCIE_ECAM_BUS_SHIFT,
>  	.init		= xgene_v2_pcie_ecam_init,
>  	.pci_ops	= {
>  		.map_bus	= xgene_pcie_map_bus,

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers
  2020-10-02 20:20   ` Krzysztof Wilczyński
@ 2020-10-02 21:37     ` Bjorn Helgaas
  0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2020-10-02 21:37 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Rob Herring, Lorenzo Pieralisi, Heiko Stuebner, PCI, Shawn Lin,
	Jonathan Cameron, Michal Simek, open list:ARM/Rockchip SoC...,
	Zhou Wang, Robert Richter, Jonathan Chocron, Bjorn Helgaas,
	Toan Le, Will Deacon, linux-arm-kernel

On Fri, Oct 02, 2020 at 10:20:17PM +0200, Krzysztof Wilczyński wrote:
> Hi Rob,
> 
> [...]
> > What about vmd which I mentioned? I also found iproc and brcmstb are
> > ECAM (well, same shifts, but indirect addressing).
> [...]
> 
> I wanted to cover these (and some others I also found) in a separate
> patch, especially since some of the drivers don't explicitly claim to
> support ECAM - but I will include these changes in the v4. 
>  
> > > +/
> > > + * Enhanced Configuration Access Mechanism (ECAM)
> > > + *
> > > + * N.B. This is a non-standard platform-specific ECAM bus shift value.  For
> > > + * standard values defined in the PCI Express Base Specification see
> > > + * include/linux/pci-ecam.h.
> > > + */
> > > +#define XGENE_PCIE_ECAM_BUS_SHIFT      16
> > 
> > Isn't this just CAM? Though perhaps CAM on PCIe is not standard...
> > 
> > For CAM, there's also tegra, ftpci100, mvebu, and versatile. I think
> > I'd drop CAM from this patch and do all of those in a separate patch.
> 
> Will do.
> 
> Bjorn was also not convinced about referring to things as "CAM" since
> the specification (the one I quoted in the patch) does not name it as
> such, and rather refers to it as Type 1 access of the PCI bus
> configuration space.

"Type 1" has two specific meanings in PCI, and neither is quite this
config access mechanism:

  1) "Type 0 Functions" have a "Type 0 Configuration Space Header" --
     these are basically endpoint devices.  "Type 1" Functions have
     Type 1 headers and are PCI-to-PCI bridges (Root Ports, Switches,
     Bridges in PCIe).

  2) "Type 0" config transactions target a device on the current bus,
     i.e., the recipient does not need to decode the bus number in the
     transaction.  A "Type 1" config transaction needs to be routed
     through one or more bridges.  The last bridge, where the bus
     number in the transaction matches the bridge's secondary bus
     number, converts the transaction to a Type 0 transaction on its
     secondary bus.

The "CAM" devices that use a 16-bit shift for the bus number are sort
of similar to the "Configuration Mechanism #1" description in PCI
r3.0, sec 3.2.2.3.2, but it's not really a good match because they
don't implement the x86-specific parts like I/O port registers at
0xcf8 and 0xcfc.  Also, that mechanism only allows access to the first
256 bytes of config space, and some/all of these extend the address
format so they can address extended config space (offsets
0x100-0xfff).

Bjorn

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 22:02 [PATCH v3] PCI: Unify ECAM constants in native PCI Express drivers Krzysztof Wilczyński
2020-10-02 15:19 ` Rob Herring
2020-10-02 20:20   ` Krzysztof Wilczyński
2020-10-02 21:37     ` Bjorn Helgaas
2020-10-02 21:29 ` Bjorn Helgaas

Linux-Rockchip Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rockchip/0 linux-rockchip/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rockchip linux-rockchip/ https://lore.kernel.org/linux-rockchip \
		linux-rockchip@lists.infradead.org
	public-inbox-index linux-rockchip

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-rockchip


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git