This patch series extends pci-bridge-emul.c driver to emulate PCI Subsystem
Vendor ID capability and PCIe extended capabilities. And then implement
in pci-mvebu.c driver support for PCI Subsystem Vendor IDs, PCIe AER
registers, support for legacy INTx interrupts, configuration for X1/X4
mode and usage of new PCI child_ops API.
Changes in v4:
* rebased on c3bd7dc553eea5a3595ca3aa0adee9bf83622a1f
Changes in v3:
* add Marek's Reviewed-by for first two patches
* split comments from "PCI: mvebu: Implement support for legacy INTx
interrupts" patch into separate patch
Changes in v2:
* use static structures for INTx interrupts
* remove INTx domain after unregistering INTx handler
Pali Rohár (10):
PCI: pci-bridge-emul: Add support for PCI Bridge Subsystem Vendor ID
capability
dt-bindings: PCI: mvebu: Add num-lanes property
PCI: mvebu: Correctly configure x1/x4 mode
PCI: mvebu: Add support for PCI Bridge Subsystem Vendor ID on emulated
bridge
PCI: mvebu: Add support for Advanced Error Reporting registers on
emulated bridge
PCI: mvebu: Use child_ops API
dt-bindings: PCI: mvebu: Update information about intx interrupts
PCI: mvebu: Fix macro names and comments about legacy interrupts
PCI: mvebu: Implement support for legacy INTx interrupts
ARM: dts: armada-385.dtsi: Add definitions for PCIe legacy INTx
interrupts
Russell King (2):
PCI: pci-bridge-emul: Re-arrange register tests
PCI: pci-bridge-emul: Add support for PCIe extended capabilities
.../devicetree/bindings/pci/mvebu-pci.txt | 16 +
arch/arm/boot/dts/armada-385.dtsi | 52 ++-
drivers/pci/controller/pci-mvebu.c | 350 +++++++++++++++---
drivers/pci/pci-bridge-emul.c | 167 ++++++---
drivers/pci/pci-bridge-emul.h | 17 +
5 files changed, 493 insertions(+), 109 deletions(-)
--
2.20.1
If x1/x4 mode is not set correctly then link with endpoint card is not
established.
Use DTS property 'num-lanes' to deteriminate x1/x4 mode.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/controller/pci-mvebu.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 357f0f41f68e..d0a75c3b78c3 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -93,6 +93,7 @@ struct mvebu_pcie_port {
void __iomem *base;
u32 port;
u32 lane;
+ bool is_x4;
int devfn;
unsigned int mem_target;
unsigned int mem_attr;
@@ -233,13 +234,25 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
{
- u32 ctrl, cmd, dev_rev, mask;
+ u32 ctrl, lnkcap, cmd, dev_rev, mask;
/* Setup PCIe controller to Root Complex mode. */
ctrl = mvebu_readl(port, PCIE_CTRL_OFF);
ctrl |= PCIE_CTRL_RC_MODE;
mvebu_writel(port, ctrl, PCIE_CTRL_OFF);
+ /*
+ * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
+ * Capability register. This register is defined by PCIe specification
+ * as read-only but this mvebu controller has it as read-write and must
+ * be set to number of SerDes PCIe lanes (1 or 4). If this register is
+ * not set correctly then link with endpoint card is not established.
+ */
+ lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
+ lnkcap &= ~PCI_EXP_LNKCAP_MLW;
+ lnkcap |= (port->is_x4 ? 4 : 1) << 4;
+ mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
+
/* Disable Root Bridge I/O space, memory space and bus mastering. */
cmd = mvebu_readl(port, PCIE_CMD_OFF);
cmd &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
@@ -982,6 +995,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
struct device *dev = &pcie->pdev->dev;
enum of_gpio_flags flags;
int reset_gpio, ret;
+ u32 num_lanes;
port->pcie = pcie;
@@ -994,6 +1008,9 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
if (of_property_read_u32(child, "marvell,pcie-lane", &port->lane))
port->lane = 0;
+ if (!of_property_read_u32(child, "num-lanes", &num_lanes) && num_lanes == 4)
+ port->is_x4 = true;
+
port->name = devm_kasprintf(dev, GFP_KERNEL, "pcie%d.%d", port->port,
port->lane);
if (!port->name) {
--
2.20.1
Signed-off-by: Pali Rohár <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/pci/mvebu-pci.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
index 24225852bce0..6d022a9d36ee 100644
--- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
+++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
@@ -81,6 +81,11 @@ and the following optional properties:
- reset-gpios: optional GPIO to PERST#
- reset-delay-us: delay in us to wait after reset de-assertion, if not
specified will default to 100ms, as required by the PCIe specification.
+- interrupt-names: list of interrupt names, supported are:
+ - "intx" - interrupt line triggered by one of the legacy interrupt
+- interrupts or interrupts-extended: List of the interrupt sources which
+ corresponding to the "interrupt-names". If non-empty then also additional
+ 'interrupt-controller' subnode must be defined.
Example:
--
2.20.1
AER registers start at mvebu offset 0x0100. Registers PCI_ERR_ROOT_COMMAND,
PCI_ERR_ROOT_STATUS and PCI_ERR_ROOT_ERR_SRC are not supported on pre-XP
hardware and returns zeros.
Note that AER interrupt is not supported yet as mvebu emulated bridge does
not implement interrupts support at all yet.
Also remove custom macro PCIE_HEADER_LOG_4_OFF as it is unused and
correctly this register should be referenced via standard macros with
offset, e.g. as: PCIE_CAP_PCIERR_OFF + PCI_ERR_HEADER_LOG + 4.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/controller/pci-mvebu.c | 67 +++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 566d8382afe6..1cf5c02499cd 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -34,7 +34,7 @@
#define PCIE_BAR_HI_OFF(n) (0x0014 + ((n) << 3))
#define PCIE_SSDEV_ID_OFF 0x002c
#define PCIE_CAP_PCIEXP 0x0060
-#define PCIE_HEADER_LOG_4_OFF 0x0128
+#define PCIE_CAP_PCIERR_OFF 0x0100
#define PCIE_BAR_CTRL_OFF(n) (0x1804 + (((n) - 1) * 4))
#define PCIE_WIN04_CTRL_OFF(n) (0x1820 + ((n) << 4))
#define PCIE_WIN04_BASE_OFF(n) (0x1824 + ((n) << 4))
@@ -603,6 +603,37 @@ mvebu_pci_bridge_emul_pcie_conf_read(struct pci_bridge_emul *bridge,
return PCI_BRIDGE_EMUL_HANDLED;
}
+static pci_bridge_emul_read_status_t
+mvebu_pci_bridge_emul_ext_conf_read(struct pci_bridge_emul *bridge,
+ int reg, u32 *value)
+{
+ struct mvebu_pcie_port *port = bridge->data;
+
+ switch (reg) {
+ case 0:
+ case PCI_ERR_UNCOR_STATUS:
+ case PCI_ERR_UNCOR_MASK:
+ case PCI_ERR_UNCOR_SEVER:
+ case PCI_ERR_COR_STATUS:
+ case PCI_ERR_COR_MASK:
+ case PCI_ERR_CAP:
+ case PCI_ERR_HEADER_LOG+0:
+ case PCI_ERR_HEADER_LOG+4:
+ case PCI_ERR_HEADER_LOG+8:
+ case PCI_ERR_HEADER_LOG+12:
+ case PCI_ERR_ROOT_COMMAND:
+ case PCI_ERR_ROOT_STATUS:
+ case PCI_ERR_ROOT_ERR_SRC:
+ *value = mvebu_readl(port, PCIE_CAP_PCIERR_OFF + reg);
+ break;
+
+ default:
+ return PCI_BRIDGE_EMUL_NOT_HANDLED;
+ }
+
+ return PCI_BRIDGE_EMUL_HANDLED;
+}
+
static void
mvebu_pci_bridge_emul_base_conf_write(struct pci_bridge_emul *bridge,
int reg, u32 old, u32 new, u32 mask)
@@ -715,11 +746,45 @@ mvebu_pci_bridge_emul_pcie_conf_write(struct pci_bridge_emul *bridge,
}
}
+static void
+mvebu_pci_bridge_emul_ext_conf_write(struct pci_bridge_emul *bridge,
+ int reg, u32 old, u32 new, u32 mask)
+{
+ struct mvebu_pcie_port *port = bridge->data;
+
+ switch (reg) {
+ /* These are W1C registers, so clear other bits */
+ case PCI_ERR_UNCOR_STATUS:
+ case PCI_ERR_COR_STATUS:
+ case PCI_ERR_ROOT_STATUS:
+ new &= mask;
+ fallthrough;
+
+ case PCI_ERR_UNCOR_MASK:
+ case PCI_ERR_UNCOR_SEVER:
+ case PCI_ERR_COR_MASK:
+ case PCI_ERR_CAP:
+ case PCI_ERR_HEADER_LOG+0:
+ case PCI_ERR_HEADER_LOG+4:
+ case PCI_ERR_HEADER_LOG+8:
+ case PCI_ERR_HEADER_LOG+12:
+ case PCI_ERR_ROOT_COMMAND:
+ case PCI_ERR_ROOT_ERR_SRC:
+ mvebu_writel(port, new, PCIE_CAP_PCIERR_OFF + reg);
+ break;
+
+ default:
+ break;
+ }
+}
+
static const struct pci_bridge_emul_ops mvebu_pci_bridge_emul_ops = {
.read_base = mvebu_pci_bridge_emul_base_conf_read,
.write_base = mvebu_pci_bridge_emul_base_conf_write,
.read_pcie = mvebu_pci_bridge_emul_pcie_conf_read,
.write_pcie = mvebu_pci_bridge_emul_pcie_conf_write,
+ .read_ext = mvebu_pci_bridge_emul_ext_conf_read,
+ .write_ext = mvebu_pci_bridge_emul_ext_conf_write,
};
/*
--
2.20.1
This is read-only capability in PCI config space. Put it between base PCI
capability and base PCI Express capability.
Driver just have to specify subsystem_vendor_id and subsystem_id fields in
emulated bridge structure and pci-bridge-emul takes care of correctly
compose PCI Bridge Subsystem Vendor ID capability.
Signed-off-by: Pali Rohár <[email protected]>
---
drivers/pci/pci-bridge-emul.c | 69 +++++++++++++++++++++++++----------
drivers/pci/pci-bridge-emul.h | 2 +
2 files changed, 51 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index c4b9837006ff..a5b662cc89d0 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -21,8 +21,11 @@
#include "pci-bridge-emul.h"
#define PCI_BRIDGE_CONF_END PCI_STD_HEADER_SIZEOF
+#define PCI_CAP_SSID_SIZEOF (PCI_SSVID_DEVICE_ID + 2)
+#define PCI_CAP_SSID_START PCI_BRIDGE_CONF_END
+#define PCI_CAP_SSID_END (PCI_CAP_SSID_START + PCI_CAP_SSID_SIZEOF)
#define PCI_CAP_PCIE_SIZEOF (PCI_EXP_SLTSTA2 + 2)
-#define PCI_CAP_PCIE_START PCI_BRIDGE_CONF_END
+#define PCI_CAP_PCIE_START PCI_CAP_SSID_END
#define PCI_CAP_PCIE_END (PCI_CAP_PCIE_START + PCI_CAP_PCIE_SIZEOF)
/**
@@ -315,6 +318,25 @@ struct pci_bridge_reg_behavior pcie_cap_regs_behavior[PCI_CAP_PCIE_SIZEOF / 4] =
},
};
+static pci_bridge_emul_read_status_t
+pci_bridge_emul_read_ssid(struct pci_bridge_emul *bridge, int reg, u32 *value)
+{
+ switch (reg) {
+ case PCI_CAP_LIST_ID:
+ *value = PCI_CAP_ID_SSVID |
+ (bridge->has_pcie ? (PCI_CAP_PCIE_START << 8) : 0);
+ return PCI_BRIDGE_EMUL_HANDLED;
+
+ case PCI_SSVID_VENDOR_ID:
+ *value = bridge->subsystem_vendor_id |
+ (bridge->subsystem_id << 16);
+ return PCI_BRIDGE_EMUL_HANDLED;
+
+ default:
+ return PCI_BRIDGE_EMUL_NOT_HANDLED;
+ }
+}
+
/*
* Initialize a pci_bridge_emul structure to represent a fake PCI
* bridge configuration space. The caller needs to have initialized
@@ -341,9 +363,17 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge,
if (!bridge->pci_regs_behavior)
return -ENOMEM;
- if (bridge->has_pcie) {
+ if (bridge->subsystem_vendor_id)
+ bridge->conf.capabilities_pointer = PCI_CAP_SSID_START;
+ else if (bridge->has_pcie)
bridge->conf.capabilities_pointer = PCI_CAP_PCIE_START;
+ else
+ bridge->conf.capabilities_pointer = 0;
+
+ if (bridge->conf.capabilities_pointer)
bridge->conf.status |= cpu_to_le16(PCI_STATUS_CAP_LIST);
+
+ if (bridge->has_pcie) {
bridge->pcie_conf.cap_id = PCI_CAP_ID_EXP;
bridge->pcie_conf.cap |= cpu_to_le16(PCI_EXP_TYPE_ROOT_PORT << 4);
bridge->pcie_cap_regs_behavior =
@@ -427,26 +457,28 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
read_op = bridge->ops->read_base;
cfgspace = (__le32 *) &bridge->conf;
behavior = bridge->pci_regs_behavior;
- } else if (!bridge->has_pcie) {
- /* PCIe space is not implemented, and no PCI capabilities */
- *value = 0;
- return PCIBIOS_SUCCESSFUL;
- } else if (reg < PCI_CAP_PCIE_END) {
+ } else if (reg >= PCI_CAP_SSID_START && reg < PCI_CAP_SSID_END && bridge->subsystem_vendor_id) {
+ /* Emulated PCI Bridge Subsystem Vendor ID capability */
+ reg -= PCI_CAP_SSID_START;
+ read_op = pci_bridge_emul_read_ssid;
+ cfgspace = NULL;
+ behavior = NULL;
+ } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
/* Our emulated PCIe capability */
reg -= PCI_CAP_PCIE_START;
read_op = bridge->ops->read_pcie;
cfgspace = (__le32 *) &bridge->pcie_conf;
behavior = bridge->pcie_cap_regs_behavior;
- } else if (reg < PCI_CFG_SPACE_SIZE) {
- /* Rest of PCI space not implemented */
- *value = 0;
- return PCIBIOS_SUCCESSFUL;
- } else {
+ } else if (reg >= PCI_CFG_SPACE_SIZE && bridge->has_pcie) {
/* PCIe extended capability space */
reg -= PCI_CFG_SPACE_SIZE;
read_op = bridge->ops->read_ext;
cfgspace = NULL;
behavior = NULL;
+ } else {
+ /* Not implemented */
+ *value = 0;
+ return PCIBIOS_SUCCESSFUL;
}
if (read_op)
@@ -504,24 +536,21 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
write_op = bridge->ops->write_base;
cfgspace = (__le32 *) &bridge->conf;
behavior = bridge->pci_regs_behavior;
- } else if (!bridge->has_pcie) {
- /* PCIe space is not implemented, and no PCI capabilities */
- return PCIBIOS_SUCCESSFUL;
- } else if (reg < PCI_CAP_PCIE_END) {
+ } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
/* Our emulated PCIe capability */
reg -= PCI_CAP_PCIE_START;
write_op = bridge->ops->write_pcie;
cfgspace = (__le32 *) &bridge->pcie_conf;
behavior = bridge->pcie_cap_regs_behavior;
- } else if (reg < PCI_CFG_SPACE_SIZE) {
- /* Rest of PCI space not implemented */
- return PCIBIOS_SUCCESSFUL;
- } else {
+ } else if (reg >= PCI_CFG_SPACE_SIZE && bridge->has_pcie) {
/* PCIe extended capability space */
reg -= PCI_CFG_SPACE_SIZE;
write_op = bridge->ops->write_ext;
cfgspace = NULL;
behavior = NULL;
+ } else {
+ /* Not implemented */
+ return PCIBIOS_SUCCESSFUL;
}
shift = (where & 0x3) * 8;
diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
index 6b5f75b2ad02..71392b67471d 100644
--- a/drivers/pci/pci-bridge-emul.h
+++ b/drivers/pci/pci-bridge-emul.h
@@ -132,6 +132,8 @@ struct pci_bridge_emul {
struct pci_bridge_reg_behavior *pcie_cap_regs_behavior;
void *data;
bool has_pcie;
+ u16 subsystem_vendor_id;
+ u16 subsystem_id;
};
enum {
--
2.20.1
From: Russell King <[email protected]>
Add support for PCIe extended capabilities, which we just redirect to the
emulating driver.
Signed-off-by: Russell King <[email protected]>
[pali: Fix writing new value with W1C bits]
Signed-off-by: Pali Rohár <[email protected]>
Signed-off-by: Marek Behún <[email protected]>
---
drivers/pci/pci-bridge-emul.c | 77 +++++++++++++++++++++++------------
drivers/pci/pci-bridge-emul.h | 15 +++++++
2 files changed, 67 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index a956408834d6..c4b9837006ff 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -437,10 +437,16 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
read_op = bridge->ops->read_pcie;
cfgspace = (__le32 *) &bridge->pcie_conf;
behavior = bridge->pcie_cap_regs_behavior;
- } else {
- /* Beyond our PCIe space */
+ } else if (reg < PCI_CFG_SPACE_SIZE) {
+ /* Rest of PCI space not implemented */
*value = 0;
return PCIBIOS_SUCCESSFUL;
+ } else {
+ /* PCIe extended capability space */
+ reg -= PCI_CFG_SPACE_SIZE;
+ read_op = bridge->ops->read_ext;
+ cfgspace = NULL;
+ behavior = NULL;
}
if (read_op)
@@ -448,15 +454,20 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
else
ret = PCI_BRIDGE_EMUL_NOT_HANDLED;
- if (ret == PCI_BRIDGE_EMUL_NOT_HANDLED)
- *value = le32_to_cpu(cfgspace[reg / 4]);
+ if (ret == PCI_BRIDGE_EMUL_NOT_HANDLED) {
+ if (cfgspace)
+ *value = le32_to_cpu(cfgspace[reg / 4]);
+ else
+ *value = 0;
+ }
/*
* Make sure we never return any reserved bit with a value
* different from 0.
*/
- *value &= behavior[reg / 4].ro | behavior[reg / 4].rw |
- behavior[reg / 4].w1c;
+ if (behavior)
+ *value &= behavior[reg / 4].ro | behavior[reg / 4].rw |
+ behavior[reg / 4].w1c;
if (size == 1)
*value = (*value >> (8 * (where & 3))) & 0xff;
@@ -502,8 +513,15 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
write_op = bridge->ops->write_pcie;
cfgspace = (__le32 *) &bridge->pcie_conf;
behavior = bridge->pcie_cap_regs_behavior;
- } else {
+ } else if (reg < PCI_CFG_SPACE_SIZE) {
+ /* Rest of PCI space not implemented */
return PCIBIOS_SUCCESSFUL;
+ } else {
+ /* PCIe extended capability space */
+ reg -= PCI_CFG_SPACE_SIZE;
+ write_op = bridge->ops->write_ext;
+ cfgspace = NULL;
+ behavior = NULL;
}
shift = (where & 0x3) * 8;
@@ -517,29 +535,38 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
else
return PCIBIOS_BAD_REGISTER_NUMBER;
- /* Keep all bits, except the RW bits */
- new = old & (~mask | ~behavior[reg / 4].rw);
+ if (behavior) {
+ /* Keep all bits, except the RW bits */
+ new = old & (~mask | ~behavior[reg / 4].rw);
- /* Update the value of the RW bits */
- new |= (value << shift) & (behavior[reg / 4].rw & mask);
+ /* Update the value of the RW bits */
+ new |= (value << shift) & (behavior[reg / 4].rw & mask);
- /* Clear the W1C bits */
- new &= ~((value << shift) & (behavior[reg / 4].w1c & mask));
+ /* Clear the W1C bits */
+ new &= ~((value << shift) & (behavior[reg / 4].w1c & mask));
+ } else {
+ new = old & ~mask;
+ new |= (value << shift) & mask;
+ }
- /* Save the new value with the cleared W1C bits into the cfgspace */
- cfgspace[reg / 4] = cpu_to_le32(new);
+ if (cfgspace) {
+ /* Save the new value with the cleared W1C bits into the cfgspace */
+ cfgspace[reg / 4] = cpu_to_le32(new);
+ }
- /*
- * Clear the W1C bits not specified by the write mask, so that the
- * write_op() does not clear them.
- */
- new &= ~(behavior[reg / 4].w1c & ~mask);
+ if (behavior) {
+ /*
+ * Clear the W1C bits not specified by the write mask, so that the
+ * write_op() does not clear them.
+ */
+ new &= ~(behavior[reg / 4].w1c & ~mask);
- /*
- * Set the W1C bits specified by the write mask, so that write_op()
- * knows about that they are to be cleared.
- */
- new |= (value << shift) & (behavior[reg / 4].w1c & mask);
+ /*
+ * Set the W1C bits specified by the write mask, so that write_op()
+ * knows about that they are to be cleared.
+ */
+ new |= (value << shift) & (behavior[reg / 4].w1c & mask);
+ }
if (write_op)
write_op(bridge, reg, old, new, mask);
diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
index 4953274cac18..6b5f75b2ad02 100644
--- a/drivers/pci/pci-bridge-emul.h
+++ b/drivers/pci/pci-bridge-emul.h
@@ -90,6 +90,14 @@ struct pci_bridge_emul_ops {
*/
pci_bridge_emul_read_status_t (*read_pcie)(struct pci_bridge_emul *bridge,
int reg, u32 *value);
+
+ /*
+ * Same as ->read_base(), except it is for reading from the
+ * PCIe extended capability configuration space.
+ */
+ pci_bridge_emul_read_status_t (*read_ext)(struct pci_bridge_emul *bridge,
+ int reg, u32 *value);
+
/*
* Called when writing to the regular PCI bridge configuration
* space. old is the current value, new is the new value being
@@ -105,6 +113,13 @@ struct pci_bridge_emul_ops {
*/
void (*write_pcie)(struct pci_bridge_emul *bridge, int reg,
u32 old, u32 new, u32 mask);
+
+ /*
+ * Same as ->write_base(), except it is for writing from the
+ * PCIe extended capability configuration space.
+ */
+ void (*write_ext)(struct pci_bridge_emul *bridge, int reg,
+ u32 old, u32 new, u32 mask);
};
struct pci_bridge_reg_behavior;
--
2.20.1
Controller driver needs to correctly configure PCIe link if it contains 1
or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for
mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard
way how number of lanes is specified in other PCIe controllers.
Signed-off-by: Pali Rohár <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
index 6173af6885f8..24225852bce0 100644
--- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
+++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
@@ -77,6 +77,7 @@ and the following optional properties:
- marvell,pcie-lane: the physical PCIe lane number, for ports having
multiple lanes. If this property is not found, we assume that the
value is 0.
+- num-lanes: number of SerDes PCIe lanes for this link (1 or 4)
- reset-gpios: optional GPIO to PERST#
- reset-delay-us: delay in us to wait after reset de-assertion, if not
specified will default to 100ms, as required by the PCIe specification.
@@ -141,6 +142,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 58>;
marvell,pcie-port = <0>;
marvell,pcie-lane = <0>;
+ num-lanes = <1>;
/* low-active PERST# reset on GPIO 25 */
reset-gpios = <&gpio0 25 1>;
/* wait 20ms for device settle after reset deassertion */
@@ -161,6 +163,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 59>;
marvell,pcie-port = <0>;
marvell,pcie-lane = <1>;
+ num-lanes = <1>;
clocks = <&gateclk 6>;
};
@@ -177,6 +180,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 60>;
marvell,pcie-port = <0>;
marvell,pcie-lane = <2>;
+ num-lanes = <1>;
clocks = <&gateclk 7>;
};
@@ -193,6 +197,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 61>;
marvell,pcie-port = <0>;
marvell,pcie-lane = <3>;
+ num-lanes = <1>;
clocks = <&gateclk 8>;
};
@@ -209,6 +214,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 62>;
marvell,pcie-port = <1>;
marvell,pcie-lane = <0>;
+ num-lanes = <1>;
clocks = <&gateclk 9>;
};
@@ -225,6 +231,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 63>;
marvell,pcie-port = <1>;
marvell,pcie-lane = <1>;
+ num-lanes = <1>;
clocks = <&gateclk 10>;
};
@@ -241,6 +248,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 64>;
marvell,pcie-port = <1>;
marvell,pcie-lane = <2>;
+ num-lanes = <1>;
clocks = <&gateclk 11>;
};
@@ -257,6 +265,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 65>;
marvell,pcie-port = <1>;
marvell,pcie-lane = <3>;
+ num-lanes = <1>;
clocks = <&gateclk 12>;
};
@@ -273,6 +282,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 99>;
marvell,pcie-port = <2>;
marvell,pcie-lane = <0>;
+ num-lanes = <1>;
clocks = <&gateclk 26>;
};
@@ -289,6 +299,7 @@ pcie-controller {
interrupt-map = <0 0 0 0 &mpic 103>;
marvell,pcie-port = <3>;
marvell,pcie-lane = <0>;
+ num-lanes = <1>;
clocks = <&gateclk 27>;
};
};
--
2.20.1
On Tue, 22 Feb 2022 16:50:18 +0100, Pali Rohár wrote:
> This patch series extends pci-bridge-emul.c driver to emulate PCI Subsystem
> Vendor ID capability and PCIe extended capabilities. And then implement
> in pci-mvebu.c driver support for PCI Subsystem Vendor IDs, PCIe AER
> registers, support for legacy INTx interrupts, configuration for X1/X4
> mode and usage of new PCI child_ops API.
>
> Changes in v4:
> * rebased on c3bd7dc553eea5a3595ca3aa0adee9bf83622a1f
>
> [...]
I can't apply dts changes, patch 12 should go via the arm-soc tree.
Applied the others to pci/mvebu, thanks.
[01/12] PCI: pci-bridge-emul: Re-arrange register tests
https://git.kernel.org/lpieralisi/pci/c/c453bf6f9b
[02/12] PCI: pci-bridge-emul: Add support for PCIe extended capabilities
https://git.kernel.org/lpieralisi/pci/c/c0bd419732
[03/12] PCI: pci-bridge-emul: Add support for PCI Bridge Subsystem Vendor ID capability
https://git.kernel.org/lpieralisi/pci/c/3767a90242
[04/12] dt-bindings: PCI: mvebu: Add num-lanes property
https://git.kernel.org/lpieralisi/pci/c/26b982ca83
[05/12] PCI: mvebu: Correctly configure x1/x4 mode
https://git.kernel.org/lpieralisi/pci/c/2a81dd9fd9
[06/12] PCI: mvebu: Add support for PCI Bridge Subsystem Vendor ID on emulated bridge
https://git.kernel.org/lpieralisi/pci/c/e3e13c9135
[07/12] PCI: mvebu: Add support for Advanced Error Reporting registers on emulated bridge
https://git.kernel.org/lpieralisi/pci/c/2b6ee04c0a
[08/12] PCI: mvebu: Use child_ops API
https://git.kernel.org/lpieralisi/pci/c/c099c2a761
[09/12] dt-bindings: PCI: mvebu: Update information about intx interrupts
https://git.kernel.org/lpieralisi/pci/c/0124989220
[10/12] PCI: mvebu: Fix macro names and comments about legacy interrupts
https://git.kernel.org/lpieralisi/pci/c/d00ea94e62
[11/12] PCI: mvebu: Implement support for legacy INTx interrupts
https://git.kernel.org/lpieralisi/pci/c/ec07526264
Thanks,
Lorenzo
On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Roh?r wrote:
> Controller driver needs to correctly configure PCIe link if it contains 1
> or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for
> mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard
> way how number of lanes is specified in other PCIe controllers.
>
> Signed-off-by: Pali Roh?r <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> index 6173af6885f8..24225852bce0 100644
> --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> @@ -77,6 +77,7 @@ and the following optional properties:
> - marvell,pcie-lane: the physical PCIe lane number, for ports having
> multiple lanes. If this property is not found, we assume that the
> value is 0.
> +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4)
> - reset-gpios: optional GPIO to PERST#
> - reset-delay-us: delay in us to wait after reset de-assertion, if not
> specified will default to 100ms, as required by the PCIe specification.
> @@ -141,6 +142,7 @@ pcie-controller {
> interrupt-map = <0 0 0 0 &mpic 58>;
> marvell,pcie-port = <0>;
> marvell,pcie-lane = <0>;
> + num-lanes = <1>;
Is this patch really necessary? AFAICS, the related driver change
only sets "port->is_x4 = true" when "num-lanes = <4>", and in all
other cases it defaults to a Max Link Width of 1:
lnkcap |= (port->is_x4 ? 4 : 1) << 4;
I don't see the point of adding a value that we don't validate or do
anything with. E.g., I don't see an error message that would catch
"num-lanes = <3>".
Bjorn
On Tue, Feb 22, 2022 at 04:50:23PM +0100, Pali Roh?r wrote:
> If x1/x4 mode is not set correctly then link with endpoint card is not
> established.
>
> Use DTS property 'num-lanes' to deteriminate x1/x4 mode.
I know this is already merged, but if tweaking for any other reason,
s/deteriminate/determine/
> + * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> + * Capability register. This register is defined by PCIe specification
> + * as read-only but this mvebu controller has it as read-write and must
> + * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> + * not set correctly then link with endpoint card is not established.
True, everything in Link Capability is RO or HwInit, but that's for
the architected access via config space. I think a device-specific
mechanism like this is fair game as long as you do it before anybody
can read it via config space.
> + */
> + lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
> + lnkcap &= ~PCI_EXP_LNKCAP_MLW;
> + lnkcap |= (port->is_x4 ? 4 : 1) << 4;
> + mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
On Thursday 24 February 2022 18:08:00 Bjorn Helgaas wrote:
> On Tue, Feb 22, 2022 at 04:50:23PM +0100, Pali Rohár wrote:
> > If x1/x4 mode is not set correctly then link with endpoint card is not
> > established.
> >
> > Use DTS property 'num-lanes' to deteriminate x1/x4 mode.
>
> I know this is already merged, but if tweaking for any other reason,
> s/deteriminate/determine/
>
> > + * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link
> > + * Capability register. This register is defined by PCIe specification
> > + * as read-only but this mvebu controller has it as read-write and must
> > + * be set to number of SerDes PCIe lanes (1 or 4). If this register is
> > + * not set correctly then link with endpoint card is not established.
>
> True, everything in Link Capability is RO or HwInit, but that's for
> the architected access via config space. I think a device-specific
> mechanism like this is fair game as long as you do it before anybody
> can read it via config space.
Maybe I was not clear and explicit in above comment, but this register
sets number of PCIe lanes which HW will use. Armada PCIe controllers
supports only x1 and x4. Sometimes default HW value is 4 for x1 HW and
sometimes default value for x4 HW is 1. First case cause that link never
comes up (HW is trying to setup 4 lanes but in reality there is only
one, so link training never finish) and second case cause degraded
performance (x4 link is established only in x1 mode as HW is via this
register instructed to ignores other 3 lanes).
So basically HW designers misused this Link Capability register for
configuring PCIe Link of PCIe Root Port.
> > + */
> > + lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
> > + lnkcap &= ~PCI_EXP_LNKCAP_MLW;
> > + lnkcap |= (port->is_x4 ? 4 : 1) << 4;
> > + mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
On Thursday 24 February 2022 18:02:26 Bjorn Helgaas wrote:
> On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Rohár wrote:
> > Controller driver needs to correctly configure PCIe link if it contains 1
> > or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for
> > mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard
> > way how number of lanes is specified in other PCIe controllers.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> > Acked-by: Rob Herring <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > index 6173af6885f8..24225852bce0 100644
> > --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > @@ -77,6 +77,7 @@ and the following optional properties:
> > - marvell,pcie-lane: the physical PCIe lane number, for ports having
> > multiple lanes. If this property is not found, we assume that the
> > value is 0.
> > +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4)
> > - reset-gpios: optional GPIO to PERST#
> > - reset-delay-us: delay in us to wait after reset de-assertion, if not
> > specified will default to 100ms, as required by the PCIe specification.
> > @@ -141,6 +142,7 @@ pcie-controller {
> > interrupt-map = <0 0 0 0 &mpic 58>;
> > marvell,pcie-port = <0>;
> > marvell,pcie-lane = <0>;
> > + num-lanes = <1>;
>
> Is this patch really necessary?
This is just documentation patch. And I think that documentation is
always important.
> AFAICS, the related driver change
> only sets "port->is_x4 = true" when "num-lanes = <4>", and in all
> other cases it defaults to a Max Link Width of 1:
>
> lnkcap |= (port->is_x4 ? 4 : 1) << 4;
Yes!
And this registers configures number of lanes in HW.
> I don't see the point of adding a value that we don't validate or do
> anything with. E.g., I don't see an error message that would catch
> "num-lanes = <3>".
>
> Bjorn
In past I was told that kernel should not do validation of DT properties
and it is job of some DT schema validation. That is why I did not added
code into kernel which show error message when value different than 1
and 4 is specified in DT.
But issue here is that there is no DT schema for pci-mvebu as above
.txt file was not converted to YAML schema yet. This is something which
should be improved...
On Fri, Feb 25, 2022 at 01:58:10PM +0100, Pali Roh?r wrote:
> On Thursday 24 February 2022 18:02:26 Bjorn Helgaas wrote:
> > On Tue, Feb 22, 2022 at 04:50:22PM +0100, Pali Roh?r wrote:
> > > Controller driver needs to correctly configure PCIe link if it contains 1
> > > or 4 SerDes PCIe lanes. Therefore add a new 'num-lanes' DT property for
> > > mvebu PCIe controller. Property 'num-lanes' seems to be de-facto standard
> > > way how number of lanes is specified in other PCIe controllers.
> > >
> > > Signed-off-by: Pali Roh?r <[email protected]>
> > > Acked-by: Rob Herring <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/pci/mvebu-pci.txt | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > > index 6173af6885f8..24225852bce0 100644
> > > --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > > +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > > @@ -77,6 +77,7 @@ and the following optional properties:
> > > - marvell,pcie-lane: the physical PCIe lane number, for ports having
> > > multiple lanes. If this property is not found, we assume that the
> > > value is 0.
> > > +- num-lanes: number of SerDes PCIe lanes for this link (1 or 4)
> > > - reset-gpios: optional GPIO to PERST#
> > > - reset-delay-us: delay in us to wait after reset de-assertion, if not
> > > specified will default to 100ms, as required by the PCIe specification.
> > > @@ -141,6 +142,7 @@ pcie-controller {
> > > interrupt-map = <0 0 0 0 &mpic 58>;
> > > marvell,pcie-port = <0>;
> > > marvell,pcie-lane = <0>;
> > > + num-lanes = <1>;
> >
> > Is this patch really necessary?
>
> This is just documentation patch. And I think that documentation is
> always important.
>
> > AFAICS, the related driver change
> > only sets "port->is_x4 = true" when "num-lanes = <4>", and in all
> > other cases it defaults to a Max Link Width of 1:
> >
> > lnkcap |= (port->is_x4 ? 4 : 1) << 4;
>
> Yes!
>
> And this registers configures number of lanes in HW.
>
> > I don't see the point of adding a value that we don't validate or do
> > anything with. E.g., I don't see an error message that would catch
> > "num-lanes = <3>".
> >
> > Bjorn
>
> In past I was told that kernel should not do validation of DT properties
> and it is job of some DT schema validation. That is why I did not added
> code into kernel which show error message when value different than 1
> and 4 is specified in DT.
>
> But issue here is that there is no DT schema for pci-mvebu as above
> .txt file was not converted to YAML schema yet. This is something which
> should be improved...
I'm OK with this patch as-is, especially since Rob acked it.
Bjorn