Amazon's Annapurna Labs DT-based PCIe host controller driver

Add Amazon's Annapurna Labs vendor ID to pci_ids.h.

Signed-off-by Jonathan Chocron <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

---
 include/linux/pci_ids.h | 2 ++
 1 file changed, 2 insertions(+)

include/linux/pci_ids.h +0 -0

index 40015609c4b5..63dfa4bace57 100644

Signature · 1 line

2.17.1

The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On some

platforms it is configured to not advertise the capability at all, while on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure the MSI-X capability. Specifically, when trying to access the MSI-X table a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous SError Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +2 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*

drivers/pci/quirks.c +3 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
+			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_msi_disable);
 #endif /* CONFIG_PCI_MSI */
 
 /*

Signature · 1 line

2.17.1

Quoted 2 lines · Jonathan Chocron

The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On some

Shouldn't this read [1c36:0031]?

Quoted 81 lines

platforms it is configured to not advertise the capability at all, while
on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure the
MSI-X capability. Specifically, when trying to access the MSI-X table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous SError
Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +3 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-

X, so it
+ * should be disabled on platforms where the device (mistakenly) advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI class.
+ */

drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1, · line 2939

Quoted 4 lines

drivers/pci/quirks.c +4 -0

+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI-X\n");

This will disable both MSI and MSI-X support - is this really the intention
here? Do the root ports support MSI and legacy, or just legacy?

Thanks,

Andrew Murray

Quoted 11 lines

drivers/pci/quirks.c +2 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS,

0x0031,

drivers/pci/quirks.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_msi_disable);
#endif /* CONFIG_PCI_MSI */

/*

Signature · 1 line

2.17.1

Quoted 6 lines · Andrew Murray

Quoted 2 lines · Jonathan Chocron

The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On some

Shouldn't this read [1c36:0031]?

Indeed. Thanks for catching this.

Quoted 81 lines

Quoted 81 lines

platforms it is configured to not advertise the capability at all, while
on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure the
MSI-X capability. Specifically, when trying to access the MSI-X table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous SError
Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +3 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-

X, so it
+ * should be disabled on platforms where the device (mistakenly) advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI class.
+ */

Quoted 9 lines

drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1, · line 2939

Quoted 4 lines

drivers/pci/quirks.c +4 -0

+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI-X\n");

This will disable both MSI and MSI-X support - is this really the intention
here? Do the root ports support MSI and legacy, or just legacy?

The HW should support MSI, but we currently don't have a use case for it so it hasn't been tested and therefore we are okay with disabling it.

For future knowledge, how can just MSI-X be disabled?
I see that in pcie_port_enable_irq_vec(), the pcieport driver calls pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And internally, both __pci_enable_msix_range() and __pci_enable_msi_range() use pci_msi_supported() which doesn't differentiate between MSI and MSI-x.

Quoted 16 lines

Thanks,

Andrew Murray

Quoted 11 lines

drivers/pci/quirks.c +2 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS,

0x0031,

drivers/pci/quirks.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_msi_disable);
#endif /* CONFIG_PCI_MSI */

/*

Signature · 1 line

2.17.1

Quoted 110 lines · Jonathan

Quoted 7 lines · Andrew Murray

Quoted 3 lines · Jonathan Chocron

The Root Port (identified by [1c36:0032]) doesn't support MSI- X. On
some

Shouldn't this read [1c36:0031]?

Indeed. Thanks for catching this.

Quoted 89 lines

Quoted 89 lines

platforms it is configured to not advertise the capability at all,
while
on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure
the
MSI-X capability. Specifically, when trying to access the MSI-X table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous SError
Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +3 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support

MSI-
X, so it
+ * should be disabled on platforms where the device (mistakenly)
advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI
class.
+ */

Quoted 9 lines

drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1, · line 2939

Quoted 4 lines

drivers/pci/quirks.c +4 -0

+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI-X\n");

This will disable both MSI and MSI-X support - is this really the intention
here? Do the root ports support MSI and legacy, or just legacy?

Quoted 5 lines

The HW should support MSI, but we currently don't have a use case for
it so it hasn't been tested and therefore we are okay with disabling
it.

OK - then the commit message and comment (for quirk_al_msi_disable) should
probably be updated to reflect this.

Especially given that the device id
may be used on other device types - implying that a use-case for this may later arise.

Quoted 8 lines

For future knowledge, how can just MSI-X be disabled?
I see that in pcie_port_enable_irq_vec(), the pcieport driver calls pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And internally, both __pci_enable_msix_range() and __pci_enable_msi_range()
use pci_msi_supported() which doesn't differentiate between MSI and MSI-x.

The documentation [1] would suggest that once upon a time pci_disable_msix
was used - but now should let pci_alloc_irq_vectors cap the max number
of vectors. However in your case it's the PCIe port driver that is attempting
to allocate MSI-X's and so the solution isn't an obvious one.

Setting dev->msix_enabled to false (i.e. through pci_disable_msix) would
result in an un-necessary WARN_ON_ONCE.

I think you'd need to ensure
devi->msix_cap is NULL (which makes sense as your hardware shouldn't be
exposing this capability).

I guess the right way of achieving this would be through a quirk, though you'd
be the first to do this and you'd have to ensure the quirk runs before
anyone tests for msix_cap.

That's my view, though others may have different suggestions.

[1] Documentation/PCI/msi-howto.rst

Thanks,

Andrew Murray

Quoted 17 lines

Quoted 17 lines

Thanks,

Andrew Murray

Quoted 12 lines

drivers/pci/quirks.c +2 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L

ABS,
0x0031,

drivers/pci/quirks.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_msi_disable);
#endif /* CONFIG_PCI_MSI */

/*

Signature · 1 line

2.17.1

Quoted 117 lines · Andrew Murray

Quoted 115 lines · Jonathan

Quoted 7 lines · Andrew Murray

Quoted 3 lines · Jonathan Chocron

The Root Port (identified by [1c36:0032]) doesn't support MSI- X. On
some

Shouldn't this read [1c36:0031]?

Indeed. Thanks for catching this.

Quoted 89 lines

Quoted 89 lines

platforms it is configured to not advertise the capability at all,
while
on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure
the
MSI-X capability. Specifically, when trying to access the MSI-X table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous SError
Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +3 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support

MSI-
X, so it
+ * should be disabled on platforms where the device (mistakenly)
advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI
class.
+ */

Quoted 9 lines

drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1, · line 2939

Quoted 4 lines

drivers/pci/quirks.c +4 -0

+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI-X\n");

This will disable both MSI and MSI-X support - is this really the intention
here? Do the root ports support MSI and legacy, or just legacy?

The HW should support MSI, but we currently don't have a use case for
it so it hasn't been tested and therefore we are okay with disabling
it.

Quoted 3 lines

OK - then the commit message and comment (for quirk_al_msi_disable) should
probably be updated to reflect this.

If you mean that we should explicitly state that MSI is possibly supported then I don't think it is very relevant here. If we decide to test and enable it in the future, then the new quirk (which would only disable MSI-x) would reflect the fact that MSI works. Sounds ok?

Quoted 4 lines

Especially given that the device id
may be used on other device types - implying that a use-case for this may later arise.

Not sure I understood that last line. This patch is relevant only for the BRIDGE class 0x0031 device. The other device types, which (sadly) happen to re-use the dev_id, have are totally different and shouldn't be mixed in here.

Quoted 17 lines

Quoted 8 lines

For future knowledge, how can just MSI-X be disabled?
I see that in pcie_port_enable_irq_vec(), the pcieport driver calls pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And internally, both __pci_enable_msix_range() and __pci_enable_msi_range()
use pci_msi_supported() which doesn't differentiate between MSI and MSI-x.

The documentation [1] would suggest that once upon a time pci_disable_msix
was used - but now should let pci_alloc_irq_vectors cap the max number
of vectors. However in your case it's the PCIe port driver that is attempting
to allocate MSI-X's and so the solution isn't an obvious one.

Quoted 3 lines

Setting dev->msix_enabled to false (i.e. through pci_disable_msix) would
result in an un-necessary WARN_ON_ONCE.

Per my understanding, it seems that dev->msix_enabled indicates if the MSI-X capability has already been enabled or not, and not as an indication if it is supported by the device. If that is the case, then not sure pci_disable_msix() would result in the desired effect.

Quoted 12 lines

I think you'd need to ensure
devi->msix_cap is NULL (which makes sense as your hardware shouldn't be
exposing this capability).

I guess the right way of achieving this would be through a quirk, though you'd
be the first to do this and you'd have to ensure the quirk runs before
anyone tests for msix_cap.

Quoted 2 lines

That's my view, though others may have different suggestions.

I think that maybe a dev->no_msix field should be added and then a corresponding pci_msix_supported() function as well. I'd be glad to take it offline or on a dedicated thread, but it shouldn't block this patchset.

Quoted 24 lines

[1] Documentation/PCI/msi-howto.rst

Thanks,

Andrew Murray

Quoted 17 lines

Quoted 17 lines

Thanks,

Andrew Murray

Quoted 12 lines

drivers/pci/quirks.c +2 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L

ABS,
0x0031,

drivers/pci/quirks.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_msi_disable);
#endif /* CONFIG_PCI_MSI */

/*

Signature · 1 line

2.17.1

Quoted 139 lines · Jonathan

Quoted 133 lines · Andrew Murray

Quoted 124 lines · Jonathan

Quoted 9 lines · Andrew Murray

Quoted 4 lines · Jonathan Chocron

The Root Port (identified by [1c36:0032]) doesn't support MSI-
X. On
some

Shouldn't this read [1c36:0031]?

Indeed. Thanks for catching this.

Quoted 101 lines

Quoted 101 lines

platforms it is configured to not advertise the capability at
all,
while
on others it (mistakenly) does. This causes a panic during initialization by the pcieport driver, since it tries to configure
the
MSI-X capability. Specifically, when trying to access the MSI-X
table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

[    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
SError
[    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP
(DT)
[    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
[    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
[    1.632367] lr : __pci_enable_msix_range+0x498/0x608
[    1.632367] sp : ffffff80117db700
[    1.632368] x29: ffffff80117db700 x28: 0000000000000001
[    1.632370] x27: 0000000000000001 x26: 0000000000000000
[    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
[    1.632373] x23: 0000000000000000 x22: 0000000000000000
[    1.632375] x21: 0000000000000001 x20: 0000000000000000
[    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
[    1.632378] x17: 0000000000000000 x16: 0000000000000000
[    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
[    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
[    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
[    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
[    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
[    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
[    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
[    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
[    1.632392] Kernel panic - not syncing: Asynchronous
SError
Interrupt
[    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
5.2.0-
rc1-
Jonny-14847-ge76f1d4a1828-dirty #33
[    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP
(DT)
[    1.632394] Call trace:
[    1.632395]  dump_backtrace+0x0/0x140
[    1.632395]  show_stack+0x14/0x20
[    1.632396]  dump_stack+0xa8/0xcc
[    1.632396]  panic+0x140/0x334
[    1.632397]  nmi_panic+0x6c/0x70
[    1.632398]  arm64_serror_panic+0x74/0x88
[    1.632398]  __pte_error+0x0/0x28
[    1.632399]  el1_error+0x84/0xf8
[    1.632400]  __pci_enable_msix_range+0x4e4/0x608
[    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
[    1.632401]  pcie_port_device_register+0x2b8/0x4e0
[    1.632402]  pcie_portdrv_probe+0x34/0xf0

Signed-off-by Jonathan Chocron <[email protected]>

Reviewed-by Gustavo Pimentel <

[email protected]>

---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/pci/quirks.c +3 -0

index 23672680dba7..11f843aa96b3 100644
@@ -2925,6 +2925,21 @@
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't

support
MSI-
X, so it
+ * should be disabled on platforms where the device (mistakenly)
advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device
types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI
class.
+ */

drivers/pci/quirks.c +2 -0

+static void quirk_al_msi_disable(struct pci_dev *dev)
+{

Quoted 9 lines

drivers/pci/quirks.c DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1, · line 2939

Quoted 2 lines

drivers/pci/quirks.c +2 -0

+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI-X\n");

This will disable both MSI and MSI-X support - is this really the
intention
here? Do the root ports support MSI and legacy, or just legacy?

Quoted 6 lines

The HW should support MSI, but we currently don't have a use case
for
it so it hasn't been tested and therefore we are okay with disabling
it.

Quoted 4 lines

OK - then the commit message and comment (for
quirk_al_msi_disable)
should
probably be updated to reflect this.

Quoted 7 lines

If you mean that we should explicitly state that MSI is possibly supported then I don't think it is very relevant here. If we decide to
test and enable it in the future, then the new quirk (which would only
disable MSI-x) would reflect the fact that MSI works. Sounds ok?

My thought process is that 1c36:0031 probably advertises an MSI capability
reflecting that the device probably supports MSI - if there is no valid
reason for disabling MSI support then we shouldn't disable it.

It doesn't make sense to me to disable it just because that's the path of
least resistance - a better course of action would be to add support to disable MSI-X in the kernel.

The kernel panic generated in attempting to enable MSI-X shows that this
device has a user for its MSI/MSI-X interrupts. Thus if you disabled only
MSI-X the PCIe port driver would use MSI interrupts instead - Due to "no_msi = 1" I guess it will be using legacy.

Quoted 6 lines

Quoted 5 lines

Especially given that the device id
may be used on other device types - implying that a use-case for this
may later arise.

Quoted 8 lines

Not sure I understood that last line. This patch is relevant only for
the BRIDGE class 0x0031 device. The other device types, which (sadly)
happen to re-use the dev_id, have are totally different and shouldn't
be mixed in here.

It's probably unlikely, but if 1c36:0031 was the device ID given to a future incarnation of this hardware in a PCIe switch (which has BRIDGE
class), then using legacy interrupts instead of MSI wouldn't be ideal and an un-necessary limitation.

This is why adding a comment to indicate that MSI may work but hasn't been tested may be helpful for others in the future.

Quoted 25 lines

Quoted 20 lines

Quoted 10 lines

For future knowledge, how can just MSI-X be disabled? I see that in pcie_port_enable_irq_vec(), the pcieport driver calls
pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And internally, both __pci_enable_msix_range() and __pci_enable_msi_range()
use pci_msi_supported() which doesn't differentiate between MSI and
MSI-x.

The documentation [1] would suggest that once upon a time pci_disable_msix
was used - but now should let pci_alloc_irq_vectors cap the max number
of vectors. However in your case it's the PCIe port driver that is
attempting
to allocate MSI-X's and so the solution isn't an obvious one.

Quoted 4 lines

Setting dev->msix_enabled to false (i.e. through
pci_disable_msix)
would
result in an un-necessary WARN_ON_ONCE.

Quoted 6 lines

Per my understanding, it seems that dev->msix_enabled indicates if the
MSI-X capability has already been enabled or not, and not as an indication if it is supported by the device. If that is the case, then
not sure pci_disable_msix() would result in the desired effect.

Yes this isn't the right approach.

Quoted 13 lines

Quoted 13 lines

I think you'd need to ensure
devi->msix_cap is NULL (which makes sense as your hardware shouldn't
be
exposing this capability).

I guess the right way of achieving this would be through a quirk, though you'd
be the first to do this and you'd have to ensure the quirk runs before
anyone tests for msix_cap.

Quoted 8 lines

Quoted 2 lines

That's my view, though others may have different suggestions.

I think that maybe a dev->no_msix field should be added and then a corresponding pci_msix_supported() function as well. I'd be glad to take it offline or on a dedicated thread, but it shouldn't block this
patchset.

Thanks,

Andrew Murray

Quoted 25 lines

Quoted 25 lines

[1] Documentation/PCI/msi-howto.rst

Thanks,

Andrew Murray

Quoted 18 lines

Quoted 18 lines

Thanks,

Andrew Murray

Quoted 13 lines

drivers/pci/quirks.c +2 -0

+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPUR

NA_L
ABS,
0x0031,

drivers/pci/quirks.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_msi_disable);
#endif /* CONFIG_PCI_MSI */

/*

Signature · 1 line

2.17.1

Document Amazon's Annapurna Labs PCIe host bridge.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../devicetree/bindings/pci/pcie-al.txt       | 45 +++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

Documentation/devicetree/bindings/pci/pcie-al.txt new file +0 -0

new file mode 100644
index 000000000000..89876190eb5a

Documentation/devicetree/bindings/pci/pcie-al.txt new file +32 -0

+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Register ranges as listed in the reg-names property
+
+- reg-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Must include the following entries
+			- "config"	PCIe ECAM space
+			- "controller"	AL proprietary registers
+			- "dbi"		Designware PCIe registers
+
+Example:
+
+	pcie-external0: pcie@fb600000 {
+		compatible = "amazon,al-pcie";
+		reg = <0x0 0xfb600000 0x0 0x00100000
+		       0x0 0xfd800000 0x0 0x00010000
+		       0x0 0xfd810000 0x0 0x00001000>;
+		reg-names = "config", "controller", "dbi";
+		bus-range = <0 255>;
+		device_type = "pci";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map-mask = <0x00 0 0 7>;
+		interrupt-map = <0x0000 0 0 1 &gic GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
+		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
+	};

MAINTAINERS +2 -1

index 5a6137df3f0e..29cca14a05a6 100644
@@ -12201,10 +12201,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
 S:	Supported
 F:	drivers/pci/controller/
 
-PCIE DRIVER FOR ANNAPURNA LABS
+PCIE DRIVER FOR AMAZON ANNAPURNA LABS
 M:	Jonathan Chocron <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/pci/pcie-al.txt
 F:	drivers/pci/controller/dwc/pcie-al.c
 
 PCIE DRIVER FOR AMLOGIC MESON

Signature · 1 line

2.17.1

Quoted 11 lines · Jonathan Chocron

Document Amazon's Annapurna Labs PCIe host bridge.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../devicetree/bindings/pci/pcie-al.txt       | 45
+++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-

al.txt

Quoted 13 lines

Documentation/devicetree/bindings/pci/pcie-al.txt new file +3 -0

new file mode 100644
index 000000000000..89876190eb5a
@@ -0,0 +1,45 @@
+* Amazon Annapurna Labs PCIe host bridge
+
+Amazon's Annapurna Labs PCIe Host Controller is based on the

Synopsys DesignWare

Documentation/devicetree/bindings/pci/pcie-al.txt new file +2 -0

+PCI core.
+It shares common functions with the PCIe DesignWare core driver

Quoted 1 line

and inherits

Driver details are irrelevant to the binding.

Documentation/devicetree/bindings/pci/pcie-al.txt line 13

Quoted 9 lines

+common properties defined in Documentation/devicetree/bindings/pci/designware-pcie.txt.

Documentation/devicetree/bindings/pci/pcie-al.txt new file +7 -0

+Properties of the host controller node that differ from it are:
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Value should contain
+			- "amazon,al-pcie"

Needs to be SoC specific.

Quoted 55 lines

Documentation/devicetree/bindings/pci/pcie-al.txt new file +30 -0

+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Register ranges as listed in the reg-names property
+
+- reg-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Must include the following entries
+			- "config"	PCIe ECAM space
+			- "controller"	AL proprietary registers
+			- "dbi"		Designware PCIe registers
+
+Example:
+
+	pcie-external0: pcie@fb600000 {
+		compatible = "amazon,al-pcie";
+		reg = <0x0 0xfb600000 0x0 0x00100000
+		       0x0 0xfd800000 0x0 0x00010000
+		       0x0 0xfd810000 0x0 0x00001000>;
+		reg-names = "config", "controller", "dbi";
+		bus-range = <0 255>;
+		device_type = "pci";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map-mask = <0x00 0 0 7>;
+		interrupt-map = <0x0000 0 0 1 &gic GIC_SPI 41

IRQ_TYPE_LEVEL_HIGH>; /* INTa */

Documentation/devicetree/bindings/pci/pcie-al.txt new file +1 -0

+		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0

0x07ff0000>;

Documentation/devicetree/bindings/pci/pcie-al.txt new file +1 -0

+	};

MAINTAINERS +0 -0

index 5a6137df3f0e..29cca14a05a6 100644
@@ -12201,10 +12201,11 @@ T:	git

git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
S: Supported
F: drivers/pci/controller/

MAINTAINERS +2 -1

-PCIE DRIVER FOR ANNAPURNA LABS
+PCIE DRIVER FOR AMAZON ANNAPURNA LABS
 M:	Jonathan Chocron <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/pci/pcie-al.txt
 F:	drivers/pci/controller/dwc/pcie-al.c
 
 PCIE DRIVER FOR AMLOGIC MESON

Signature · 1 line

2.17.1

Quoted 12 lines · Rob Herring

Quoted 11 lines · Jonathan Chocron

Document Amazon's Annapurna Labs PCIe host bridge.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../devicetree/bindings/pci/pcie-al.txt       | 45
+++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-

al.txt

Quoted 17 lines

Quoted 13 lines

Documentation/devicetree/bindings/pci/pcie-al.txt new file +3 -0

new file mode 100644
index 000000000000..89876190eb5a
@@ -0,0 +1,45 @@
+* Amazon Annapurna Labs PCIe host bridge
+
+Amazon's Annapurna Labs PCIe Host Controller is based on the

Synopsys DesignWare

Documentation/devicetree/bindings/pci/pcie-al.txt new file +2 -0

+PCI core.
+It shares common functions with the PCIe DesignWare core driver

Quoted 1 line

and inherits

Driver details are irrelevant to the binding.

Will remove.

Quoted 12 lines

Documentation/devicetree/bindings/pci/pcie-al.txt line 13

Quoted 9 lines

+common properties defined in Documentation/devicetree/bindings/pci/designware-pcie.txt.

Documentation/devicetree/bindings/pci/pcie-al.txt new file +7 -0

+Properties of the host controller node that differ from it are:
+
+- compatible:
+   Usage: required
+   Value type: <stringlist>
+   Definition: Value should contain
+                   - "amazon,al-pcie"

Needs to be SoC specific.

I'm not sure I follow. The PCIe controller can be implemented in different SoCs. Could you please clarify?

On Tue, Aug 13, 2019 at 10:49 AM Chocron, Jonathan <[email protected]

Quoted 50 lines

wrote:

Quoted 21 lines · Rob Herring

Quoted 20 lines · Jonathan Chocron

Document Amazon's Annapurna Labs PCIe host bridge.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../devicetree/bindings/pci/pcie-al.txt       | 45
+++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-

al.txt

Documentation/devicetree/bindings/pci/pcie-al.txt new file +2 -0

new file mode 100644
index 000000000000..89876190eb5a
@@ -0,0 +1,45 @@
+* Amazon Annapurna Labs PCIe host bridge
+

Quoted 9 lines

Quoted 6 lines

Documentation/devicetree/bindings/pci/pcie-al.txt new file +1 -0

+Amazon's Annapurna Labs PCIe Host Controller is based on the

Synopsys DesignWare

Documentation/devicetree/bindings/pci/pcie-al.txt new file +2 -0

+PCI core.
+It shares common functions with the PCIe DesignWare core

driver
and inherits

Driver details are irrelevant to the binding.

Will remove.

Quoted 4 lines

Quoted 4 lines

+common properties defined in Documentation/devicetree/bindings/pci/designware-pcie.txt. +Properties of the host controller node that differ from it are:

Quoted 9 lines

Documentation/devicetree/bindings/pci/pcie-al.txt line 13

Quoted 6 lines

Documentation/devicetree/bindings/pci/pcie-al.txt new file +6 -0

+
+- compatible:
+   Usage: required
+   Value type: <stringlist>
+   Definition: Value should contain
+                   - "amazon,al-pcie"

Needs to be SoC specific.

Quoted 2 lines

I'm not sure I follow. The PCIe controller can be implemented in different SoCs. Could you please clarify?

All the features, bugs, and integration will be exactly the same on all SoCs and you will never need to distinguish?

This is standard convention for compatible strings and how you avoid updating the DT (part of firmware) when you find some difference the OS needs to handle down the road.

If the next SoC is 'the same', then you do:

compatible = "amazon,newsoc-pcie", "amazon,oldsoc-pcie";

Rob

This driver is DT based and utilizes the DesignWare APIs. It allows using a smaller ECAM range for a larger bus range - usually an entire bus uses 1MB of address space, but the driver can use it for a larger number of buses.

All link initializations are handled by the boot FW.

Signed-off-by Jonathan Chocron <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 367 +++++++++++++++++++++++++++
 2 files changed, 379 insertions(+)

drivers/pci/controller/dwc/Kconfig +12 -0

index 6ea778ae4877..3c6094cbcc3b 100644
@@ -230,4 +230,16 @@ config PCIE_UNIPHIER
 	  Say Y here if you want PCIe controller support on UniPhier SoCs.
 	  This driver supports LD20 and PXs3 SoCs.
 
+config PCIE_AL
+	bool "Amazon Annapurna Labs PCIe controller"
+	depends on OF && (ARM64 || COMPILE_TEST)
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Say Y here to enable support of the Amazon's Annapurna Labs PCIe
+	  controller IP on Amazon SoCs. The PCIe controller uses the DesignWare
+	  core plus Annapurna Labs proprietary hardware wrappers. This is
+	  required only for DT-based platforms. ACPI platforms with the
+	  Annapurna Labs PCIe controller don't need to enable this.
+
 endmenu

drivers/pci/controller/dwc/pcie-al.c +367 -0

index 3ab58f0584a8..3ffdd3c97617 100644
@@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
 };
 
 #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
+
+#ifdef CONFIG_PCIE_AL
+
+#include <linux/of_pci.h>
+#include "pcie-designware.h"
+
+#define AL_PCIE_REV_ID_2	2
+#define AL_PCIE_REV_ID_3	3
+#define AL_PCIE_REV_ID_4	4
+
+#define AXI_BASE_OFFSET		0x0
+
+#define DEVICE_ID_OFFSET	0x16c
+
+#define DEVICE_REV_ID			0x0
+#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
+
+#define DEVICE_REV_ID_DEV_ID_X4		0
+#define DEVICE_REV_ID_DEV_ID_X8		2
+#define DEVICE_REV_ID_DEV_ID_X16	4
+
+#define OB_CTRL_REV1_2_OFFSET	0x0040
+#define OB_CTRL_REV3_5_OFFSET	0x0030
+
+#define CFG_TARGET_BUS			0x0
+#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
+#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
+
+#define CFG_CONTROL			0x4
+#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
+#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
+
+struct al_pcie_reg_offsets {
+	unsigned int ob_ctrl;
+};
+
+struct al_pcie_target_bus_cfg {
+	u8 reg_val;
+	u8 reg_mask;
+	u8 ecam_mask;
+};
+
+struct al_pcie {
+	struct dw_pcie *pci;
+	void __iomem *controller_base; /* base of PCIe unit (not DW core) */
+	struct device *dev;
+	resource_size_t ecam_size;
+	unsigned int controller_rev_id;
+	struct al_pcie_reg_offsets reg_offsets;
+	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)
+{
+	return readl(pcie->controller_base + offset);
+}
+
+static inline void al_pcie_controller_writel(struct al_pcie *pcie, u32 offset,
+					     u32 val)
+{
+	writel(val, pcie->controller_base + offset);
+}
+
+static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
+{
+	u32 dev_rev_id_val;
+	u32 dev_id_val;
+
+	dev_rev_id_val = al_pcie_controller_readl(pcie, AXI_BASE_OFFSET +
+						  DEVICE_ID_OFFSET +
+						  DEVICE_REV_ID);
+	dev_id_val = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK, dev_rev_id_val);
+
+	switch (dev_id_val) {
+	case DEVICE_REV_ID_DEV_ID_X4:
+		*rev_id = AL_PCIE_REV_ID_2;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X8:
+		*rev_id = AL_PCIE_REV_ID_3;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X16:
+		*rev_id = AL_PCIE_REV_ID_4;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported dev_id_val (0x%x)\n",
+			dev_id_val);
+		return -EINVAL;
+	}
+
+	dev_dbg(pcie->dev, "dev_id_val: 0x%x\n", dev_id_val);
+
+	return 0;
+}
+
+static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
+{
+	switch (pcie->controller_rev_id) {
+	case AL_PCIE_REV_ID_2:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
+		break;
+	case AL_PCIE_REV_ID_3:
+	case AL_PCIE_REV_ID_4:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported controller rev_id: 0x%x\n",
+			pcie->controller_rev_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
+					  u8 target_bus,
+					  u8 mask_target_bus)
+{
+	u32 reg;
+
+	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
+	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
+
+	al_pcie_controller_writel(pcie, AXI_BASE_OFFSET +
+				  pcie->reg_offsets.ob_ctrl + CFG_TARGET_BUS,
+				  reg);
+}
+
+static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
+					   unsigned int busnr,
+					   unsigned int devfn)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie->target_bus_cfg;
+	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
+	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
+	struct pcie_port *pp = &pcie->pci->pp;
+	void __iomem *pci_base_addr;
+
+	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
+					 (busnr_ecam << 20) +
+					 PCIE_ECAM_DEVFN(devfn));
+
+	if (busnr_reg != target_bus_cfg->reg_val) {
+		dev_dbg(pcie->pci->dev, "Changing target bus busnum val from 0x%x to 0x%x\n",
+			target_bus_cfg->reg_val, busnr_reg);
+		target_bus_cfg->reg_val = busnr_reg;
+		al_pcie_target_bus_set(pcie,
+				       target_bus_cfg->reg_val,
+				       target_bus_cfg->reg_mask);
+	}
+
+	return pci_base_addr;
+}
+
+static int al_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 *val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;
+	int rc;
+
+	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
+
+	rc = dw_pcie_read(pci_addr + where, size, val);
+
+	dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
+		size, pci_domain_nr(bus), bus->number,
+		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
+		(pci_addr + where), *val);
+
+	return rc;
+}
+
+static int al_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;
+	int rc;
+
+	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
+
+	rc = dw_pcie_write(pci_addr + where, size, val);
+
+	dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
+		size, pci_domain_nr(bus), bus->number,
+		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
+		(pci_addr + where), val);
+
+	return rc;
+}
+
+static void al_pcie_config_prepare(struct al_pcie *pcie)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg;
+	struct pcie_port *pp = &pcie->pci->pp;
+	unsigned int ecam_bus_mask;
+	u32 cfg_control_offset;
+	u8 subordinate_bus;
+	u8 secondary_bus;
+	u32 cfg_control;
+	u32 reg;
+
+	target_bus_cfg = &pcie->target_bus_cfg;
+
+	ecam_bus_mask = (pcie->ecam_size >> 20) - 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;
+	}
+
+	/* This portion is taken from the transaction address */
+	target_bus_cfg->ecam_mask = ecam_bus_mask;
+	/* This portion is taken from the cfg_target_bus reg */
+	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
+	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
+
+	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
+			       target_bus_cfg->reg_mask);
+
+	secondary_bus = pp->busn->start + 1;
+	subordinate_bus = pp->busn->end;
+
+	/* Set the valid values of secondary and subordinate buses */
+	cfg_control_offset = AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
+			     CFG_CONTROL;
+
+	cfg_control = al_pcie_controller_readl(pcie, cfg_control_offset);
+
+	reg = cfg_control &
+	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
+
+	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
+	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
+
+	al_pcie_controller_writel(pcie, cfg_control_offset, reg);
+}
+
+static int al_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	int link_up;
+	int rc;
+
+	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
+	if (rc)
+		return rc;
+
+	rc = al_pcie_reg_offsets_set(pcie);
+	if (rc)
+		return rc;
+
+	al_pcie_config_prepare(pcie);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops al_pcie_host_ops = {
+	.rd_other_conf = al_pcie_rd_other_conf,
+	.wr_other_conf = al_pcie_wr_other_conf,
+	.host_init = al_pcie_host_init,
+};
+
+static int al_add_pcie_port(struct pcie_port *pp,
+			    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pp->ops = &al_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+};
+
+static int al_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *controller_res;
+	struct resource *ecam_res;
+	struct resource *dbi_res;
+	struct al_pcie *al_pcie;
+	struct dw_pcie *pci;
+	int ret;
+
+	al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+	if (!al_pcie)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+
+	al_pcie->pci = pci;
+	al_pcie->dev = dev;
+
+	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
+	if (IS_ERR(pci->dbi_base)) {
+		dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
+		return PTR_ERR(pci->dbi_base);
+	}
+
+	ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (!ecam_res) {
+		dev_err(dev, "couldn't find 'config' reg in DT\n");
+		return -ENOENT;
+	}
+	al_pcie->ecam_size = resource_size(ecam_res);
+
+	controller_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						      "controller");
+	al_pcie->controller_base = devm_ioremap_resource(dev, controller_res);
+	if (IS_ERR(al_pcie->controller_base)) {
+		dev_err(dev, "couldn't remap controller base %pR\n",
+			controller_res);
+		return PTR_ERR(al_pcie->controller_base);
+	}
+
+	dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
+		dbi_res, controller_res);
+
+	platform_set_drvdata(pdev, al_pcie);
+
+	ret = al_add_pcie_port(&pci->pp, pdev);
+
+	return ret;
+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-pcie",
+	},
+	{},
+};
+
+static struct platform_driver al_pcie_driver = {
+	.driver = {
+		.name	= "al-pcie",
+		.of_match_table = al_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = al_pcie_probe,
+};
+builtin_platform_driver(al_pcie_driver);
+
+#endif /* CONFIG_PCIE_AL*/

Signature · 1 line

2.17.1

This driver is DT based and utilizes the DesignWare APIs. It allows using a smaller ECAM range for a larger bus range - usually an entire bus uses 1MB of address space, but the driver can use it for a larger number of buses.

All link initializations are handled by the boot FW.

Signed-off-by Jonathan Chocron <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 367 +++++++++++++++++++++++++++
 2 files changed, 379 insertions(+)

drivers/pci/controller/dwc/Kconfig +12 -0

index 6ea778ae4877..3c6094cbcc3b 100644
@@ -230,4 +230,16 @@ config PCIE_UNIPHIER
 	  Say Y here if you want PCIe controller support on UniPhier SoCs.
 	  This driver supports LD20 and PXs3 SoCs.
 
+config PCIE_AL
+	bool "Amazon Annapurna Labs PCIe controller"
+	depends on OF && (ARM64 || COMPILE_TEST)
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Say Y here to enable support of the Amazon's Annapurna Labs PCIe
+	  controller IP on Amazon SoCs. The PCIe controller uses the DesignWare
+	  core plus Annapurna Labs proprietary hardware wrappers. This is
+	  required only for DT-based platforms. ACPI platforms with the
+	  Annapurna Labs PCIe controller don't need to enable this.
+
 endmenu

drivers/pci/controller/dwc/pcie-al.c +56 -0

index 3ab58f0584a8..3ffdd3c97617 100644
@@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
 };
 
 #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
+
+#ifdef CONFIG_PCIE_AL
+
+#include <linux/of_pci.h>
+#include "pcie-designware.h"
+
+#define AL_PCIE_REV_ID_2	2
+#define AL_PCIE_REV_ID_3	3
+#define AL_PCIE_REV_ID_4	4
+
+#define AXI_BASE_OFFSET		0x0
+
+#define DEVICE_ID_OFFSET	0x16c
+
+#define DEVICE_REV_ID			0x0
+#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
+
+#define DEVICE_REV_ID_DEV_ID_X4		0
+#define DEVICE_REV_ID_DEV_ID_X8		2
+#define DEVICE_REV_ID_DEV_ID_X16	4
+
+#define OB_CTRL_REV1_2_OFFSET	0x0040
+#define OB_CTRL_REV3_5_OFFSET	0x0030
+
+#define CFG_TARGET_BUS			0x0
+#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
+#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
+
+#define CFG_CONTROL			0x4
+#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
+#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
+
+struct al_pcie_reg_offsets {
+	unsigned int ob_ctrl;
+};
+
+struct al_pcie_target_bus_cfg {
+	u8 reg_val;
+	u8 reg_mask;
+	u8 ecam_mask;
+};
+
+struct al_pcie {
+	struct dw_pcie *pci;
+	void __iomem *controller_base; /* base of PCIe unit (not DW core) */
+	struct device *dev;
+	resource_size_t ecam_size;
+	unsigned int controller_rev_id;
+	struct al_pcie_reg_offsets reg_offsets;
+	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)
+

drivers/pci/controller/dwc/pcie-al.c +121 -0

+
+static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
+{
+	u32 dev_rev_id_val;
+	u32 dev_id_val;
+
+	dev_rev_id_val = al_pcie_controller_readl(pcie, AXI_BASE_OFFSET +
+						  DEVICE_ID_OFFSET +
+						  DEVICE_REV_ID);
+	dev_id_val = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK, dev_rev_id_val);
+
+	switch (dev_id_val) {
+	case DEVICE_REV_ID_DEV_ID_X4:
+		*rev_id = AL_PCIE_REV_ID_2;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X8:
+		*rev_id = AL_PCIE_REV_ID_3;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X16:
+		*rev_id = AL_PCIE_REV_ID_4;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported dev_id_val (0x%x)\n",
+			dev_id_val);
+		return -EINVAL;
+	}
+
+	dev_dbg(pcie->dev, "dev_id_val: 0x%x\n", dev_id_val);
+
+	return 0;
+}
+
+static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
+{
+	switch (pcie->controller_rev_id) {
+	case AL_PCIE_REV_ID_2:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
+		break;
+	case AL_PCIE_REV_ID_3:
+	case AL_PCIE_REV_ID_4:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported controller rev_id: 0x%x\n",
+			pcie->controller_rev_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
+					  u8 target_bus,
+					  u8 mask_target_bus)
+{
+	u32 reg;
+
+	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
+	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
+
+	al_pcie_controller_writel(pcie, AXI_BASE_OFFSET +
+				  pcie->reg_offsets.ob_ctrl + CFG_TARGET_BUS,
+				  reg);
+}
+
+static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
+					   unsigned int busnr,
+					   unsigned int devfn)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie->target_bus_cfg;
+	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
+	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
+	struct pcie_port *pp = &pcie->pci->pp;
+	void __iomem *pci_base_addr;
+
+	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
+					 (busnr_ecam << 20) +
+					 PCIE_ECAM_DEVFN(devfn));
+
+	if (busnr_reg != target_bus_cfg->reg_val) {
+		dev_dbg(pcie->pci->dev, "Changing target bus busnum val from 0x%x to 0x%x\n",
+			target_bus_cfg->reg_val, busnr_reg);
+		target_bus_cfg->reg_val = busnr_reg;
+		al_pcie_target_bus_set(pcie,
+				       target_bus_cfg->reg_val,
+				       target_bus_cfg->reg_mask);
+	}
+
+	return pci_base_addr;
+}
+
+static int al_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 *val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;
+	int rc;
+
+	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
+
+	rc = dw_pcie_read(pci_addr + where, size, val);
+
+	dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
+		size, pci_domain_nr(bus), bus->number,
+		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
+		(pci_addr + where), *val);
+
+	return rc;
+}
+
+static int al_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+				 unsigned int devfn, int where, int size,
+				 u32 val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;

drivers/pci/controller/dwc/pcie-al.c +85 -0

+
+	return rc;
+}
+
+static void al_pcie_config_prepare(struct al_pcie *pcie)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg;
+	struct pcie_port *pp = &pcie->pci->pp;
+	unsigned int ecam_bus_mask;
+	u32 cfg_control_offset;
+	u8 subordinate_bus;
+	u8 secondary_bus;
+	u32 cfg_control;
+	u32 reg;
+
+	target_bus_cfg = &pcie->target_bus_cfg;
+
+	ecam_bus_mask = (pcie->ecam_size >> 20) - 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;
+	}
+
+	/* This portion is taken from the transaction address */
+	target_bus_cfg->ecam_mask = ecam_bus_mask;
+	/* This portion is taken from the cfg_target_bus reg */
+	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
+	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
+
+	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
+			       target_bus_cfg->reg_mask);
+
+	secondary_bus = pp->busn->start + 1;
+	subordinate_bus = pp->busn->end;
+
+	/* Set the valid values of secondary and subordinate buses */
+	cfg_control_offset = AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
+			     CFG_CONTROL;
+
+	cfg_control = al_pcie_controller_readl(pcie, cfg_control_offset);
+
+	reg = cfg_control &
+	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
+
+	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
+	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
+
+	al_pcie_controller_writel(pcie, cfg_control_offset, reg);
+}
+
+static int al_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	int link_up;
+	int rc;
+
+	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
+	if (rc)
+		return rc;
+
+	rc = al_pcie_reg_offsets_set(pcie);
+	if (rc)
+		return rc;
+
+	al_pcie_config_prepare(pcie);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops al_pcie_host_ops = {
+	.rd_other_conf = al_pcie_rd_other_conf,
+	.wr_other_conf = al_pcie_wr_other_conf,
+	.host_init = al_pcie_host_init,
+};
+
+static int al_add_pcie_port(struct pcie_port *pp,
+			    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pp->ops = &al_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);

drivers/pci/controller/dwc/pcie-al.c +46 -0

+
+static int al_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *controller_res;
+	struct resource *ecam_res;
+	struct resource *dbi_res;
+	struct al_pcie *al_pcie;
+	struct dw_pcie *pci;
+	int ret;
+
+	al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+	if (!al_pcie)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+
+	al_pcie->pci = pci;
+	al_pcie->dev = dev;
+
+	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
+	if (IS_ERR(pci->dbi_base)) {
+		dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
+		return PTR_ERR(pci->dbi_base);
+	}
+
+	ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	if (!ecam_res) {
+		dev_err(dev, "couldn't find 'config' reg in DT\n");
+		return -ENOENT;
+	}
+	al_pcie->ecam_size = resource_size(ecam_res);
+
+	controller_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						      "controller");
+	al_pcie->controller_base = devm_ioremap_resource(dev, controller_res);
+	if (IS_ERR(al_pcie->controller_base)) {
+		dev_err(dev, "couldn't remap controller base %pR\n",
+			controller_res);
+		return PTR_ERR(al_pcie->controller_base);

drivers/pci/controller/dwc/pcie-al.c +19 -0

+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-pcie",
+	},
+	{},
+};
+
+static struct platform_driver al_pcie_driver = {
+	.driver = {
+		.name	= "al-pcie",
+		.of_match_table = al_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = al_pcie_probe,
+};
+builtin_platform_driver(al_pcie_driver);
+
+#endif /* CONFIG_PCIE_AL*/

Signature · 1 line

2.17.1

"PCI: dwc: al: Add support for DW based driver type"

Make $SUBJECT compliant with other host controllers patches.

Quoted 4 lines

This driver is DT based and utilizes the DesignWare APIs. It allows using a smaller ECAM range for a larger bus range - usually an entire bus uses 1MB of address space, but the driver can use it for a larger number of buses.

I would appreciate if you can add a simple explanation of the mechanism for completeness.

AFAIU, with ACPI you don't support all these variants.

Quoted 107 lines

All link initializations are handled by the boot FW.

Signed-off-by Jonathan Chocron <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 367
+++++++++++++++++++++++++++
 2 files changed, 379 insertions(+)

drivers/pci/controller/dwc/Kconfig +0 -0

index 6ea778ae4877..3c6094cbcc3b 100644
@@ -230,4 +230,16 @@ config PCIE_UNIPHIER
 	  Say Y here if you want PCIe controller support on UniPhier

SoCs.

drivers/pci/controller/dwc/Kconfig +7 -0

 	  This driver supports LD20 and PXs3 SoCs.
 
+config PCIE_AL
+	bool "Amazon Annapurna Labs PCIe controller"
+	depends on OF && (ARM64 || COMPILE_TEST)
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Say Y here to enable support of the Amazon's Annapurna Labs

PCIe

drivers/pci/controller/dwc/Kconfig +1 -0

+	  controller IP on Amazon SoCs. The PCIe controller uses the

DesignWare

drivers/pci/controller/dwc/Kconfig +1 -0

+	  core plus Annapurna Labs proprietary hardware wrappers. This

is

drivers/pci/controller/dwc/Kconfig +3 -0

+	  required only for DT-based platforms. ACPI platforms with the
+	  Annapurna Labs PCIe controller don't need to enable this.
+
 endmenu

drivers/pci/controller/dwc/pcie-al.c +45 -0

index 3ab58f0584a8..3ffdd3c97617 100644
@@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
 };
 
 #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
+
+#ifdef CONFIG_PCIE_AL
+
+#include <linux/of_pci.h>
+#include "pcie-designware.h"
+
+#define AL_PCIE_REV_ID_2	2
+#define AL_PCIE_REV_ID_3	3
+#define AL_PCIE_REV_ID_4	4
+
+#define AXI_BASE_OFFSET		0x0
+
+#define DEVICE_ID_OFFSET	0x16c
+
+#define DEVICE_REV_ID			0x0
+#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
+
+#define DEVICE_REV_ID_DEV_ID_X4		0
+#define DEVICE_REV_ID_DEV_ID_X8		2
+#define DEVICE_REV_ID_DEV_ID_X16	4
+
+#define OB_CTRL_REV1_2_OFFSET	0x0040
+#define OB_CTRL_REV3_5_OFFSET	0x0030
+
+#define CFG_TARGET_BUS			0x0
+#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
+#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
+
+#define CFG_CONTROL			0x4
+#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
+#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
+
+struct al_pcie_reg_offsets {
+	unsigned int ob_ctrl;
+};
+
+struct al_pcie_target_bus_cfg {
+	u8 reg_val;
+	u8 reg_mask;
+	u8 ecam_mask;
+};
+
+struct al_pcie {
+	struct dw_pcie *pci;
+	void __iomem *controller_base; /* base of PCIe unit (not DW

core) */

drivers/pci/controller/dwc/pcie-al.c +12 -0

+	struct device *dev;
+	resource_size_t ecam_size;
+	unsigned int controller_rev_id;
+	struct al_pcie_reg_offsets reg_offsets;
+	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)

drivers/pci/controller/dwc/pcie-al.c +4 -0

+{
+	return readl(pcie->controller_base + offset);
+}
+

drivers/pci/controller/dwc/pcie-al.c struct pci_ecam_ops al_pcie_ops = { · line 159

Quoted 6 lines

+static inline void al_pcie_controller_writel(struct al_pcie *pcie, u32 offset,

drivers/pci/controller/dwc/pcie-al.c +4 -0

+					     u32 val)
+{
+	writel(val, pcie->controller_base + offset);
+}

You should be able to use the read/write{_relaxed} API.

Quoted 138 lines

drivers/pci/controller/dwc/pcie-al.c +2 -0

+
+static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int

*rev_id)

drivers/pci/controller/dwc/pcie-al.c +9 -0

+{
+	u32 dev_rev_id_val;
+	u32 dev_id_val;
+
+	dev_rev_id_val = al_pcie_controller_readl(pcie, AXI_BASE_OFFSET
+
+						  DEVICE_ID_OFFSET +
+						  DEVICE_REV_ID);
+	dev_id_val = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,

dev_rev_id_val);

drivers/pci/controller/dwc/pcie-al.c +34 -0

+
+	switch (dev_id_val) {
+	case DEVICE_REV_ID_DEV_ID_X4:
+		*rev_id = AL_PCIE_REV_ID_2;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X8:
+		*rev_id = AL_PCIE_REV_ID_3;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X16:
+		*rev_id = AL_PCIE_REV_ID_4;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported dev_id_val (0x%x)\n",
+			dev_id_val);
+		return -EINVAL;
+	}
+
+	dev_dbg(pcie->dev, "dev_id_val: 0x%x\n", dev_id_val);
+
+	return 0;
+}
+
+static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
+{
+	switch (pcie->controller_rev_id) {
+	case AL_PCIE_REV_ID_2:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
+		break;
+	case AL_PCIE_REV_ID_3:
+	case AL_PCIE_REV_ID_4:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported controller rev_id:

0x%x\n",

drivers/pci/controller/dwc/pcie-al.c +18 -0

+			pcie->controller_rev_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
+					  u8 target_bus,
+					  u8 mask_target_bus)
+{
+	u32 reg;
+
+	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
+	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
+
+	al_pcie_controller_writel(pcie, AXI_BASE_OFFSET +
+				  pcie->reg_offsets.ob_ctrl +

CFG_TARGET_BUS,

drivers/pci/controller/dwc/pcie-al.c +8 -0

+				  reg);
+}
+
+static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
+					   unsigned int busnr,
+					   unsigned int devfn)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-

Quoted 1 line

target_bus_cfg;

drivers/pci/controller/dwc/pcie-al.c +11 -0

+	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
+	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
+	struct pcie_port *pp = &pcie->pci->pp;
+	void __iomem *pci_base_addr;
+
+	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
+					 (busnr_ecam << 20) +
+					 PCIE_ECAM_DEVFN(devfn));
+
+	if (busnr_reg != target_bus_cfg->reg_val) {
+		dev_dbg(pcie->pci->dev, "Changing target bus busnum val

from 0x%x to 0x%x\n",

drivers/pci/controller/dwc/pcie-al.c +11 -0

+			target_bus_cfg->reg_val, busnr_reg);
+		target_bus_cfg->reg_val = busnr_reg;
+		al_pcie_target_bus_set(pcie,
+				       target_bus_cfg->reg_val,
+				       target_bus_cfg->reg_mask);
+	}
+
+	return pci_base_addr;
+}
+
+static int al_pcie_rd_other_conf(struct pcie_port *pp, struct

pci_bus *bus,

drivers/pci/controller/dwc/pcie-al.c +1 -0

+				 unsigned int devfn, int where, int

size,

drivers/pci/controller/dwc/pcie-al.c +13 -0

+				 u32 *val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;
+	int rc;
+
+	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
+
+	rc = dw_pcie_read(pci_addr + where, size, val);
+
+	dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d

offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",

drivers/pci/controller/dwc/pcie-al.c +8 -0

+		size, pci_domain_nr(bus), bus->number,
+		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
+		(pci_addr + where), *val);
+
+	return rc;
+}
+
+static int al_pcie_wr_other_conf(struct pcie_port *pp, struct

pci_bus *bus,

drivers/pci/controller/dwc/pcie-al.c +1 -0

+				 unsigned int devfn, int where, int

size,

drivers/pci/controller/dwc/pcie-al.c +11 -0

+				 u32 val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	unsigned int busnr = bus->number;
+	void __iomem *pci_addr;
+	int rc;
+
+	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
+
+	rc = dw_pcie_write(pci_addr + where, size, val);

drivers/pci/controller/dwc/pcie-al.c struct pci_ecam_ops al_pcie_ops = { · line 290

Quoted 6 lines

drivers/pci/controller/dwc/pcie-al.c +2 -0

+
+	dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d

offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",

drivers/pci/controller/dwc/pcie-al.c +3 -0

+		size, pci_domain_nr(bus), bus->number,
+		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
+		(pci_addr + where), val);

dev_dbg() ?

Quoted 96 lines

drivers/pci/controller/dwc/pcie-al.c +20 -0

+
+	return rc;
+}
+
+static void al_pcie_config_prepare(struct al_pcie *pcie)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg;
+	struct pcie_port *pp = &pcie->pci->pp;
+	unsigned int ecam_bus_mask;
+	u32 cfg_control_offset;
+	u8 subordinate_bus;
+	u8 secondary_bus;
+	u32 cfg_control;
+	u32 reg;
+
+	target_bus_cfg = &pcie->target_bus_cfg;
+
+	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
+	if (ecam_bus_mask > 255) {
+		dev_warn(pcie->dev, "ECAM window size is larger than

256MB. Cutting off at 256\n");

drivers/pci/controller/dwc/pcie-al.c +8 -0

+		ecam_bus_mask = 255;
+	}
+
+	/* This portion is taken from the transaction address */
+	target_bus_cfg->ecam_mask = ecam_bus_mask;
+	/* This portion is taken from the cfg_target_bus reg */
+	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
+	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-

Quoted 1 line

reg_mask;

drivers/pci/controller/dwc/pcie-al.c +9 -0

+
+	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
+			       target_bus_cfg->reg_mask);
+
+	secondary_bus = pp->busn->start + 1;
+	subordinate_bus = pp->busn->end;
+
+	/* Set the valid values of secondary and subordinate buses */
+	cfg_control_offset = AXI_BASE_OFFSET + pcie-

Quoted 1 line

reg_offsets.ob_ctrl +

drivers/pci/controller/dwc/pcie-al.c +3 -0

+			     CFG_CONTROL;
+
+	cfg_control = al_pcie_controller_readl(pcie,

cfg_control_offset);

drivers/pci/controller/dwc/pcie-al.c +52 -0

+
+	reg = cfg_control &
+	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
+
+	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
+	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
+
+	al_pcie_controller_writel(pcie, cfg_control_offset, reg);
+}
+
+static int al_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	int link_up;
+	int rc;
+
+	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
+	if (rc)
+		return rc;
+
+	rc = al_pcie_reg_offsets_set(pcie);
+	if (rc)
+		return rc;
+
+	al_pcie_config_prepare(pcie);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops al_pcie_host_ops = {
+	.rd_other_conf = al_pcie_rd_other_conf,
+	.wr_other_conf = al_pcie_wr_other_conf,
+	.host_init = al_pcie_host_init,
+};
+
+static int al_add_pcie_port(struct pcie_port *pp,
+			    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pp->ops = &al_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}

drivers/pci/controller/dwc/pcie-al.c struct pci_ecam_ops al_pcie_ops = { · line 385

Quoted 3 lines

drivers/pci/controller/dwc/pcie-al.c +3 -0

+
+static const struct dw_pcie_ops dw_pcie_ops = {
+};

I understand you have to have it - probably we should improve the generic DW layer to check for a pointer before dereferencing so that we avoid this empty struct. Anyway, that's for another series.

Quoted 50 lines

drivers/pci/controller/dwc/pcie-al.c +26 -0

+
+static int al_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *controller_res;
+	struct resource *ecam_res;
+	struct resource *dbi_res;
+	struct al_pcie *al_pcie;
+	struct dw_pcie *pci;
+	int ret;
+
+	al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+	if (!al_pcie)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+
+	al_pcie->pci = pci;
+	al_pcie->dev = dev;
+
+	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

"dbi");

drivers/pci/controller/dwc/pcie-al.c +7 -0

+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
+	if (IS_ERR(pci->dbi_base)) {
+		dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res);
+		return PTR_ERR(pci->dbi_base);
+	}
+
+	ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,

"config");

drivers/pci/controller/dwc/pcie-al.c +7 -0

+	if (!ecam_res) {
+		dev_err(dev, "couldn't find 'config' reg in DT\n");
+		return -ENOENT;
+	}
+	al_pcie->ecam_size = resource_size(ecam_res);
+
+	controller_res = platform_get_resource_byname(pdev,

IORESOURCE_MEM,

drivers/pci/controller/dwc/pcie-al.c +2 -0

+						      "controller");
+	al_pcie->controller_base = devm_ioremap_resource(dev,

controller_res);

drivers/pci/controller/dwc/pcie-al.c +4 -0

+	if (IS_ERR(al_pcie->controller_base)) {
+		dev_err(dev, "couldn't remap controller base %pR\n",
+			controller_res);
+		return PTR_ERR(al_pcie->controller_base);

drivers/pci/controller/dwc/pcie-al.c struct pci_ecam_ops al_pcie_ops = { · line 441

Quoted 10 lines

drivers/pci/controller/dwc/pcie-al.c +10 -0

+	}
+
+	dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
+		dbi_res, controller_res);
+
+	platform_set_drvdata(pdev, al_pcie);
+
+	ret = al_add_pcie_port(&pci->pp, pdev);
+
+	return ret;

Nit:

return al_add_pcie_port(&pci->pp, pdev);

?

Lorenzo

Quoted 22 lines

drivers/pci/controller/dwc/pcie-al.c +19 -0

+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-pcie",
+	},
+	{},
+};
+
+static struct platform_driver al_pcie_driver = {
+	.driver = {
+		.name	= "al-pcie",
+		.of_match_table = al_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = al_pcie_probe,
+};
+builtin_platform_driver(al_pcie_driver);
+
+#endif /* CONFIG_PCIE_AL*/

Signature · 1 line

2.17.1

This basically aligns the usage of PCI_PROBE_ONLY and PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in pci_host_common_probe().

Now it will be possible to control via the devicetree whether to just probe the PCI bus (in cases where FW already configured it) or to fully configure it.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../pci/controller/dwc/pcie-designware-host.c | 23 +++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-designware-host.c +2 -0

index d2ca748e4c85..0a294d8aa21a 100644
@@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!bridge)
 		return -ENOMEM;
 
+	of_pci_check_probe_only();
+
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
 					&bridge->windows, &pp->io_base);
 	if (ret)

drivers/pci/controller/dwc/pcie-designware-host.c +14 -4

+
 	bridge->dev.parent = dev;
 	bridge->sysdata = pp;
 	bridge->busnr = pp->root_bus_nr;

@@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
 
-	pci_bus_size_bridges(pp->root_bus);
-	pci_bus_assign_resources(pp->root_bus);
+	/*
+	 * We insert PCI resources into the iomem_resource and
+	 * ioport_resource trees in either pci_bus_claim_resources()
+	 * or pci_bus_assign_resources().
+	 */
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_claim_resources(pp->root_bus);
+	} else {
+		pci_bus_size_bridges(pp->root_bus);
+		pci_bus_assign_resources(pp->root_bus);
 
-	list_for_each_entry(child, &pp->root_bus->children, node)
-		pcie_bus_configure_settings(child);
+		list_for_each_entry(child, &pp->root_bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
 
 	pci_bus_add_devices(pp->root_bus);
 	return 0;

Signature · 1 line

2.17.1

Quoted 35 lines · Jonathan Chocron

This basically aligns the usage of PCI_PROBE_ONLY and PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in pci_host_common_probe().

Now it will be possible to control via the devicetree whether to just
probe the PCI bus (in cases where FW already configured it) or to fully
configure it.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../pci/controller/dwc/pcie-designware-host.c | 23
+++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-designware-host.c +2 -0

index d2ca748e4c85..0a294d8aa21a 100644
@@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!bridge)
 		return -ENOMEM;
 
+	of_pci_check_probe_only();
+
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
 					&bridge->windows, &pp-
 >io_base);

drivers/pci/controller/dwc/pcie-designware-host.c +0 -0

 	if (ret)

@@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	pp->root_bus_nr = pp->busn->start;
 

drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 481

Quoted 3 lines

drivers/pci/controller/dwc/pcie-designware-host.c +3 -0

+	/* Do not reassign bus nums if probe only */
+	if (!pci_has_flag(PCI_PROBE_ONLY))
+		pci_add_flags(PCI_REASSIGN_ALL_BUS);

This changes the default for bus reassignment on all DWC host (that are
!PCI_PROBE_ONLY), we should drop this line, it can trigger regressions.

If we still want to merge it as a separate change we must test it on all
DWC host bridges to make sure it does not trigger any issues with current set-ups, that's not going to be easy though.

Lorenzo

Quoted 33 lines

drivers/pci/controller/dwc/pcie-designware-host.c +14 -4

+
 	bridge->dev.parent = dev;
 	bridge->sysdata = pp;
 	bridge->busnr = pp->root_bus_nr;

@@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
 
-	pci_bus_size_bridges(pp->root_bus);
-	pci_bus_assign_resources(pp->root_bus);
+	/*
+	 * We insert PCI resources into the iomem_resource and
+	 * ioport_resource trees in either pci_bus_claim_resources()
+	 * or pci_bus_assign_resources().
+	 */
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_claim_resources(pp->root_bus);
+	} else {
+		pci_bus_size_bridges(pp->root_bus);
+		pci_bus_assign_resources(pp->root_bus);
 
-	list_for_each_entry(child, &pp->root_bus->children, node)
-		pcie_bus_configure_settings(child);
+		list_for_each_entry(child, &pp->root_bus->children,
 node)
+			pcie_bus_configure_settings(child);
+	}
 
 	pci_bus_add_devices(pp->root_bus);

return 0;

Signature · 1 line

2.17.1

Quoted 36 lines · Lorenzo Pieralisi

Quoted 35 lines · Jonathan Chocron

This basically aligns the usage of PCI_PROBE_ONLY and PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in pci_host_common_probe().

Now it will be possible to control via the devicetree whether to just
probe the PCI bus (in cases where FW already configured it) or to fully
configure it.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../pci/controller/dwc/pcie-designware-host.c | 23
+++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-designware-host.c +2 -0

index d2ca748e4c85..0a294d8aa21a 100644
@@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!bridge)
 		return -ENOMEM;
 
+	of_pci_check_probe_only();
+
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
 					&bridge->windows, &pp-
 >io_base);

drivers/pci/controller/dwc/pcie-designware-host.c +0 -0

 	if (ret)

@@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	pp->root_bus_nr = pp->busn->start;
 

Quoted 9 lines

drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 481

Quoted 3 lines

drivers/pci/controller/dwc/pcie-designware-host.c +3 -0

+	/* Do not reassign bus nums if probe only */
+	if (!pci_has_flag(PCI_PROBE_ONLY))
+		pci_add_flags(PCI_REASSIGN_ALL_BUS);

This changes the default for bus reassignment on all DWC host (that are
!PCI_PROBE_ONLY), we should drop this line, it can trigger regressions.

Will be dropped as part of v4. There might also be a behavioral difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)). Is that still ok?

As I pointed out in the cover letter, since PCI_PROBE_ONLY is a system wide flag, does it make sense to call of_pci_check_probe_only() here, in the context of a specific driver (including the existing invocation in pci_host_common_probe()), as opposed to a platform/arch context?

Quoted 5 lines

If we still want to merge it as a separate change we must test it on all
DWC host bridges to make sure it does not trigger any issues with current set-ups, that's not going to be easy though.

Just out of curiosity, how are such exhaustive tests achieved when a patch requires them?

Quoted 36 lines

Lorenzo

Quoted 33 lines

drivers/pci/controller/dwc/pcie-designware-host.c +14 -4

+
 	bridge->dev.parent = dev;
 	bridge->sysdata = pp;
 	bridge->busnr = pp->root_bus_nr;

@@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
 
-	pci_bus_size_bridges(pp->root_bus);
-	pci_bus_assign_resources(pp->root_bus);
+	/*
+	 * We insert PCI resources into the iomem_resource and
+	 * ioport_resource trees in either pci_bus_claim_resources()
+	 * or pci_bus_assign_resources().
+	 */
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_claim_resources(pp->root_bus);
+	} else {
+		pci_bus_size_bridges(pp->root_bus);
+		pci_bus_assign_resources(pp->root_bus);
 
-	list_for_each_entry(child, &pp->root_bus->children, node)
-		pcie_bus_configure_settings(child);
+		list_for_each_entry(child, &pp->root_bus->children,
 node)
+			pcie_bus_configure_settings(child);
+	}
 
 	pci_bus_add_devices(pp->root_bus);

return 0;

Signature · 1 line

2.17.1

Side note, run:

git log --oneline on drivers/pci/controller/dwc existing files and make sure commit subjects are in line with those.

Eg PCI: dw: should be PCI: dwc:

Quoted 53 lines · Lorenzo Pieralisi

Quoted 41 lines · Jonathan Chocron

Quoted 40 lines

This basically aligns the usage of PCI_PROBE_ONLY and PCI_REASSIGN_ALL_BUS in dw_pcie_host_init() with the logic in pci_host_common_probe().

Now it will be possible to control via the devicetree whether to
just
probe the PCI bus (in cases where FW already configured it) or to
fully
configure it.

Signed-off-by Jonathan Chocron <[email protected]>

---
 .../pci/controller/dwc/pcie-designware-host.c | 23
+++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

drivers/pci/controller/dwc/pcie-designware-host.c +2 -0

index d2ca748e4c85..0a294d8aa21a 100644
@@ -342,6 +342,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	if (!bridge)
 		return -ENOMEM;
 
+	of_pci_check_probe_only();
+
 	ret = devm_of_pci_get_host_bridge_resources(dev, 0,
 0xff,
 					&bridge->windows, &pp-

Quoted 1 line

io_base);

drivers/pci/controller/dwc/pcie-designware-host.c +0 -0

 	if (ret)

@@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port
 *pp)
 
 	pp->root_bus_nr = pp->busn->start;
 

Quoted 10 lines

drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port *pp) · line 481

Quoted 3 lines

drivers/pci/controller/dwc/pcie-designware-host.c +3 -0

+	/* Do not reassign bus nums if probe only */
+	if (!pci_has_flag(PCI_PROBE_ONLY))
+		pci_add_flags(PCI_REASSIGN_ALL_BUS);

This changes the default for bus reassignment on all DWC host (that
are
!PCI_PROBE_ONLY), we should drop this line, it can trigger regressions.

Quoted 4 lines

Will be dropped as part of v4. There might also be a behavioral difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)).
Is that still ok?

That's true but I doubt any DWC host has a DT firmware with "linux,pci-probe-only" in it.

It is trial and error I am afraid, please make sure all DWC maintainers are copied in.

Quoted 7 lines

As I pointed out in the cover letter, since PCI_PROBE_ONLY is a system
wide flag, does it make sense to call of_pci_check_probe_only() here,
in the context of a specific driver (including the existing invocation
in pci_host_common_probe()), as opposed to a platform/arch context?

It is an ongoing discussion to define how we should handle PCI_PROBE_ONLY. Adding this code into DWC I do not think it would hurt but if we can postpone it for the next (v5.5) merge window after we debate this at LPC within the PCI microconference it would be great.

Please sync with Benjamin as a first step, I trust he would ask you to do the right thing.

Quoted 7 lines

Quoted 6 lines

If we still want to merge it as a separate change we must test it on
all
DWC host bridges to make sure it does not trigger any issues with current set-ups, that's not going to be easy though.

Quoted 3 lines

Just out of curiosity, how are such exhaustive tests achieved when a
patch requires them?

CC DWC host bridge maintainers and ask them to test it. I do not have the HW (and FW) required, I am sorry, that's the only option I can give
you. -next coverage would help too but to a minor extent.

Thanks,
Lorenzo

Quoted 40 lines

Quoted 40 lines

Lorenzo

Quoted 37 lines

drivers/pci/controller/dwc/pcie-designware-host.c +1 -0

+
 	bridge->dev.parent = dev;
 	bridge->sysdata = pp;

drivers/pci/controller/dwc/pcie-designware-host.c +11 -4

 	bridge->busnr = pp->root_bus_nr;

@@ -490,11 +496,20 @@ int dw_pcie_host_init(struct pcie_port
 *pp)
 	if (pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
 
-	pci_bus_size_bridges(pp->root_bus);
-	pci_bus_assign_resources(pp->root_bus);
+	/*
+	 * We insert PCI resources into the iomem_resource and
+	 * ioport_resource trees in either
 pci_bus_claim_resources()
+	 * or pci_bus_assign_resources().
+	 */
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_claim_resources(pp->root_bus);
+	} else {
+		pci_bus_size_bridges(pp->root_bus);
+		pci_bus_assign_resources(pp->root_bus);
 
-	list_for_each_entry(child, &pp->root_bus->children,
 node)
-		pcie_bus_configure_settings(child);
+		list_for_each_entry(child, &pp->root_bus-

Quoted 1 line

children,

node)

drivers/pci/controller/dwc/pcie-designware-host.c +2 -0

+			pcie_bus_configure_settings(child);
+	}
 
 	pci_bus_add_devices(pp->root_bus);
 	return 0;

Signature · 1 line

2.17.1