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

From: Ali Saidi <[email protected]>

The Amazon's Annapurna Labs root ports don't advertise an ACS capability, but they don't allow peer-to-peer transactions and do validate bus numbers through the SMMU. Additionally, it's not possible for one RP to pass traffic to another RP.

Signed-off-by Ali Saidi <[email protected]>

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

Reviewed-by Gustavo Pimentel <[email protected]>

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

drivers/pci/quirks.c +0 -0

index 208aacf39329..23672680dba7 100644
@@ -4366,6 +4366,23 @@ static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
 	return ret;

drivers/pci/quirks.c +11 -0

+	 */
+	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
+
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return -ENOTTY;
+
+	return acs_flags ? 0 : 1;
+}
+
 /*
  * Sunrise Point PCH root ports implement ACS, but unfortunately as shown in
  * the datasheet (Intel 100 Series Chipset Family PCH Datasheet, Vol. 2,

@@ -4559,6 +4576,8 @@ static const struct pci_dev_acs_enabled {
 	{ PCI_VENDOR_ID_AMPERE, 0xE00A, pci_quirk_xgene_acs },
 	{ PCI_VENDOR_ID_AMPERE, 0xE00B, pci_quirk_xgene_acs },
 	{ PCI_VENDOR_ID_AMPERE, 0xE00C, pci_quirk_xgene_acs },
+	/* Amazon Annapurna Labs */
+	{ PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031, pci_quirk_al_acs },
 	{ 0 }
 };
 

Signature · 1 line

2.17.1

Quoted 24 lines · Jonathan Chocron

From: Ali Saidi <[email protected]>

The Amazon's Annapurna Labs root ports don't advertise an ACS capability, but they don't allow peer-to-peer transactions and do validate bus numbers through the SMMU. Additionally, it's not possible
for one RP to pass traffic to another RP.

Signed-off-by Ali Saidi <[email protected]>

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

Reviewed-by Gustavo Pimentel <[email protected]>

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

drivers/pci/quirks.c +0 -0

index 208aacf39329..23672680dba7 100644
@@ -4366,6 +4366,23 @@ static int pci_quirk_qcom_rp_acs(struct
 pci_dev *dev, u16 acs_flags)
 	return ret;
 }
 

Quoted 11 lines

drivers/pci/quirks.c +5 -0

+static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	/*
+	 * Amazon's Annapurna Labs root ports don't include an ACS
 capability,
+	 * but do include ACS-like functionality. The hardware doesn't

support

drivers/pci/quirks.c +1 -0

+	 * peer-to-peer transactions via the root port and each has a

unique

drivers/pci/quirks.c +2 -0

+	 * segment number.
+	 * Additionally, the root ports cannot send traffic to each

Quoted 1 line

other.

Nit: I'd probably put a new line between the above two lines, or start the
'Additionally' sentence on the first line. But either way...

Reviewed-by Andrew Murray <[email protected]>

Quoted 28 lines

drivers/pci/quirks.c +2 -0

+	 */
+	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV |

PCI_ACS_UF);

drivers/pci/quirks.c +7 -0

+
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return -ENOTTY;
+
+	return acs_flags ? 0 : 1;
+}
+
 /*
  * Sunrise Point PCH root ports implement ACS, but unfortunately

as shown in
* the datasheet (Intel 100 Series Chipset Family PCH Datasheet,
Vol. 2,

drivers/pci/quirks.c +2 -0

@@ -4559,6 +4576,8 @@ static const struct pci_dev_acs_enabled {
 	{ PCI_VENDOR_ID_AMPERE, 0xE00A, pci_quirk_xgene_acs },
 	{ PCI_VENDOR_ID_AMPERE, 0xE00B, pci_quirk_xgene_acs },
 	{ PCI_VENDOR_ID_AMPERE, 0xE00C, pci_quirk_xgene_acs },
+	/* Amazon Annapurna Labs */
+	{ PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031, pci_quirk_al_acs
 },
 	{ 0 }
 };

Signature · 2 lines

-- 
2.17.1

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability, but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd() is that, although this fails pci_vpd_read/write, the 'vpd' sysfs entry still exists. When running lspci -vv, for example, this results in the following error:

pcilib: sysfs_read_vpd: read failed: Input/output error

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +16 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability (No support for VPD read/write transactions)\n");
+	}
+}
+
+/*
+ * 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.
+ */
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
+			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_vpd_release);
+
 #endif

Signature · 1 line

2.17.1

Quoted 9 lines · Jonathan Chocron

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd() is that, although this fails pci_vpd_read/write, the 'vpd' sysfs entry still exists. When running lspci -vv, for example, this results in the following error:

Quoted 1 line

pcilib: sysfs_read_vpd: read failed: Input/output error

Oh that's not nice. It's probably triggered by the -EIO in pci_vpd_read.
A quick search online seems to show that other people have experienced
this too - though from as far as I can tell this just gives you a warning and pcilib will continnue to give other output?

I guess every vpd blacklist'd driver will have the same issue. And for
this reason I don't think that this patch is the right solution - as otherwise all the other blacklisted drivers could follow your lead.

I don't think you need to fix this specifically for the AL driver and so
I'd suggest that you can probably drop this patch. (Ideally pciutils could be updated to not warn for this specific use-case).

Thanks,

Andrew Murray

Quoted 43 lines

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +6 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void quirk_chelsio_extend_vpd(struct
 pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability (No

support for VPD read/write transactions)\n");

drivers/pci/vpd.c +5 -0

+	}
+}
+
+/*
+ * 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.
+ */ +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,

drivers/pci/vpd.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_vpd_release);
+
#endif

Signature · 1 line

2.17.1

Quoted 10 lines · Andrew Murray

Quoted 9 lines · Jonathan Chocron

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd() is that, although this fails pci_vpd_read/write, the 'vpd' sysfs entry still exists. When running lspci -vv, for example, this results in the following error:

Quoted 9 lines

Quoted 1 line

pcilib: sysfs_read_vpd: read failed: Input/output error

Oh that's not nice. It's probably triggered by the -EIO in pci_vpd_read.
A quick search online seems to show that other people have experienced
this too - though from as far as I can tell this just gives you a warning and pcilib will continnue to give other output?

Correct.

Quoted 5 lines

I guess every vpd blacklist'd driver will have the same issue. And for
this reason I don't think that this patch is the right solution - as otherwise all the other blacklisted drivers could follow your lead.

I think that going forward, they should follow my lead, I just didn't want to possibly break any assumptions other vendors' tools might have regarding the existence/non-existence of the vpd sysfs entry.

Quoted 5 lines

I don't think you need to fix this specifically for the AL driver and so
I'd suggest that you can probably drop this patch. (Ideally pciutils could be updated to not warn for this specific use-case).

I don't think that solution should be implemented in pcituils. It rightfully warns when it fails to read from the vpd sysfs file - it first 'open's the file which succeeds, and then fails when trying to 'read' from it.

I don't think that it should specifically "mask" out
-EIO, since it shouldn't have to "know" that the underlying reason is a

VPD quirk (or more precisely vpd->len == 0). Furthermore, it is possible that this error code would be returned for some other reason (not sure if currently this occurs).

I think that if the device doesn't properly support vpd, the kernel shouldn't expose the "empty" sysfs file in the first place.

In the long run, quirk_blacklist_vpd() should probably be modified to do what our quirk does or something similar (and then the al quirk can be removed). What do you think?

Quoted 48 lines

Thanks,

Andrew Murray

Quoted 43 lines

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +6 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void quirk_chelsio_extend_vpd(struct
 pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability (No

support for VPD read/write transactions)\n");

drivers/pci/vpd.c +5 -0

+	}
+}
+
+/*
+ * 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.
+ */ +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,

drivers/pci/vpd.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_vpd_release);
+
#endif

Signature · 1 line

2.17.1

Quoted 48 lines · Jonathan

Quoted 12 lines · Andrew Murray

Quoted 11 lines · Jonathan Chocron

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd()
is that, although this fails pci_vpd_read/write, the 'vpd' sysfs
entry still exists. When running lspci -vv, for example, this results in the following error:

Quoted 9 lines

Quoted 1 line

pcilib: sysfs_read_vpd: read failed: Input/output error

Oh that's not nice. It's probably triggered by the -EIO in pci_vpd_read.
A quick search online seems to show that other people have experienced
this too - though from as far as I can tell this just gives you a warning and pcilib will continnue to give other output?

Correct.

Quoted 8 lines

I guess every vpd blacklist'd driver will have the same issue. And
for
this reason I don't think that this patch is the right solution - as
otherwise all the other blacklisted drivers could follow your lead.

I think that going forward, they should follow my lead, I just didn't
want to possibly break any assumptions other vendors' tools might have
regarding the existence/non-existence of the vpd sysfs entry.

Quoted 7 lines

I don't think you need to fix this specifically for the AL driver and
so
I'd suggest that you can probably drop this patch. (Ideally pciutils
could be updated to not warn for this specific use-case).

Quoted 5 lines

I don't think that solution should be implemented in pcituils. It rightfully warns when it fails to read from the vpd sysfs file - it first 'open's the file which succeeds, and then fails when trying to
'read' from it.

Indeed - this is correct.

Quoted 3 lines

I don't think that it should specifically "mask" out -EIO, since it shouldn't have to "know" that the underlying reason is a

You're probably right - I guess the kernel should document somewhere (ABI/testing/sysfs-bus-pci?) what the kernel does when such a quirk exists,
then userspace can conform. For example if -EIO cannot be returned any
other way then it would be OK for pciutils to mask it out - but its ambigious at the moment.

Quoted 5 lines

VPD quirk (or more precisely vpd->len == 0). Furthermore, it is possible that this error code would be returned for some other reason
(not sure if currently this occurs).

Quoted 8 lines

I think that if the device doesn't properly support vpd, the kernel shouldn't expose the "empty" sysfs file in the first place.

In the long run, quirk_blacklist_vpd() should probably be modified to
do what our quirk does or something similar (and then the al quirk can
be removed). What do you think?

When I first saw your quirk, I did wonder why quirk_blacklist_vpd doesn't
do what your quirk does. Perhaps there isn't a reason. It was first introduced in 2016:

7c20078a8197 ("PCI: Prevent VPD access for buggy devices")

Some may argue that actually because your hardware has a VPD capability
it should have the sysfs file - but the capability doesn't work and so
the sysfs file should return an error.

I'd be keen to change quirk_blacklist_vpd - Babu, Bjorn any objections?

Thanks,

Andrew Murray

Quoted 53 lines

Quoted 53 lines

Thanks,

Andrew Murray

Quoted 48 lines

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +0 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void

quirk_chelsio_extend_vpd(struct
pci_dev *dev)
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,

drivers/pci/vpd.c +6 -0

 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability

(No
support for VPD read/write transactions)\n");

drivers/pci/vpd.c +5 -0

+	}
+}
+
+/*
+ * 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.
+ */ +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L ABS,
0x0031,

drivers/pci/vpd.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_vpd_release);
+
#endif

Signature · 1 line

2.17.1

Quoted 96 lines · Andrew Murray

Quoted 48 lines · Jonathan

Quoted 12 lines · Andrew Murray

Quoted 11 lines · Jonathan Chocron

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd()
is that, although this fails pci_vpd_read/write, the 'vpd' sysfs
entry still exists. When running lspci -vv, for example, this results in the following error:

Quoted 9 lines

Quoted 1 line

pcilib: sysfs_read_vpd: read failed: Input/output error

Oh that's not nice. It's probably triggered by the -EIO in pci_vpd_read.
A quick search online seems to show that other people have experienced
this too - though from as far as I can tell this just gives you a warning and pcilib will continnue to give other output?

Correct.

Quoted 8 lines

I guess every vpd blacklist'd driver will have the same issue. And
for
this reason I don't think that this patch is the right solution - as
otherwise all the other blacklisted drivers could follow your lead.

I think that going forward, they should follow my lead, I just didn't
want to possibly break any assumptions other vendors' tools might have
regarding the existence/non-existence of the vpd sysfs entry.

Quoted 7 lines

I don't think you need to fix this specifically for the AL driver and
so
I'd suggest that you can probably drop this patch. (Ideally pciutils
could be updated to not warn for this specific use-case).

Quoted 5 lines

I don't think that solution should be implemented in pcituils. It rightfully warns when it fails to read from the vpd sysfs file - it first 'open's the file which succeeds, and then fails when trying to
'read' from it.

Indeed - this is correct.

Quoted 3 lines

I don't think that it should specifically "mask" out -EIO, since it shouldn't have to "know" that the underlying reason is a

You're probably right - I guess the kernel should document somewhere (ABI/testing/sysfs-bus-pci?) what the kernel does when such a quirk exists,
then userspace can conform. For example if -EIO cannot be returned any
other way then it would be OK for pciutils to mask it out - but its ambigious at the moment.

Quoted 5 lines

VPD quirk (or more precisely vpd->len == 0). Furthermore, it is possible that this error code would be returned for some other reason
(not sure if currently this occurs).

Quoted 8 lines

I think that if the device doesn't properly support vpd, the kernel shouldn't expose the "empty" sysfs file in the first place.

In the long run, quirk_blacklist_vpd() should probably be modified to
do what our quirk does or something similar (and then the al quirk can
be removed). What do you think?

When I first saw your quirk, I did wonder why quirk_blacklist_vpd doesn't
do what your quirk does. Perhaps there isn't a reason. It was first introduced in 2016:

7c20078a8197 ("PCI: Prevent VPD access for buggy devices")

Some may argue that actually because your hardware has a VPD capability
it should have the sysfs file - but the capability doesn't work and so
the sysfs file should return an error.

Quoted 3 lines

I'd be keen to change quirk_blacklist_vpd - Babu, Bjorn any objections?

Since the merge window is closing and I don't want to affect any other PCIe controllers without having their maintainers testing this change, I'll remove this function and register our device_id with the existing quirk_blacklist_vpd. This will be part of v5.

I'll then submit a separate patch (for the next kernel version) which changes the quirk_blacklist_vpd to do what I originally intended.

Quoted 58 lines

Thanks,

Andrew Murray

Quoted 53 lines

Quoted 53 lines

Thanks,

Andrew Murray

Quoted 48 lines

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +0 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void

quirk_chelsio_extend_vpd(struct
pci_dev *dev)
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,

drivers/pci/vpd.c +6 -0

 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability

(No
support for VPD read/write transactions)\n");

drivers/pci/vpd.c +5 -0

+	}
+}
+
+/*
+ * 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.
+ */ +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L ABS,
0x0031,

drivers/pci/vpd.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_vpd_release);
+
#endif

Signature · 1 line

2.17.1

Quoted 10 lines · Andrew Murray

Quoted 9 lines · Jonathan Chocron

The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
but there is no actual support for it.

The reason for not using the already existing quirk_blacklist_vpd() is that, although this fails pci_vpd_read/write, the 'vpd' sysfs entry still exists. When running lspci -vv, for example, this results in the following error:

Quoted 9 lines

Quoted 1 line

pcilib: sysfs_read_vpd: read failed: Input/output error

Oh that's not nice. It's probably triggered by the -EIO in pci_vpd_read.
A quick search online seems to show that other people have experienced
this too - though from as far as I can tell this just gives you a warning and pcilib will continnue to give other output?

Correct.

Quoted 5 lines

I guess every vpd blacklist'd driver will have the same issue. And for
this reason I don't think that this patch is the right solution - as otherwise all the other blacklisted drivers could follow your lead.

I think that going forward, they should follow my lead, I just didn't want to possibly break any assumptions other vendors' tools might have regarding the existence/non-existence of the vpd sysfs entry.

Quoted 5 lines

I don't think you need to fix this specifically for the AL driver and so
I'd suggest that you can probably drop this patch. (Ideally pciutils could be updated to not warn for this specific use-case).

I don't think that solution should be implemented in pcituils. It rightfully warns when it fails to read from the vpd sysfs file - it first 'open's the file which succeeds, and then fails when trying to 'read' from it. I don't think that it should specifically "mask" out -EIO, since it shouldn't have to "know" that the underlying reason is a VPD quirk (or more precisely vpd->len == 0). Furthermore, it is possible that this error code would be returned for some other reason (not sure if currently this occurs).

I think that if the device doesn't properly support vpd, the kernel shouldn't expose the "empty" sysfs file in the first place.

In the long run, quirk_blacklist_vpd() should probably be modified to do what our quirk does or something similar (and then the al quirk can be removed). What do you think?

Quoted 47 lines

Thanks,

Andrew Murray

Quoted 43 lines

This quirk removes the sysfs entry, which avoids the error print.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/pci/vpd.c +6 -0

index 4963c2e2bd4c..c23a8ec08db9 100644
@@ -644,4 +644,20 @@ static void quirk_chelsio_extend_vpd(struct
 pci_dev *dev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 			quirk_chelsio_extend_vpd);
 
+static void quirk_al_vpd_release(struct pci_dev *dev)
+{
+	if (dev->vpd) {
+		pci_vpd_release(dev);
+		dev->vpd = NULL;
+		pci_warn(dev, FW_BUG "Releasing VPD capability (No

support for VPD read/write transactions)\n");

drivers/pci/vpd.c +5 -0

+	}
+}
+
+/*
+ * 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.
+ */ +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,

drivers/pci/vpd.c +1 -0

+			      PCI_CLASS_BRIDGE_PCI, 8,

quirk_al_vpd_release);
+
#endif

Signature · 1 line

2.17.1

The Root Port (identified by [1c36:0031]) 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

Notice that this quirk also disables MSI (which may work, but hasn't been tested nor has a current use case), since currently there is no standard way to disable only MSI-X.

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

Reviewed-by Gustavo Pimentel <[email protected]>

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

drivers/pci/quirks.c +0 -0

index 23672680dba7..b6e6e7df3f7b 100644

drivers/pci/quirks.c +11 -0

+ *
+ * 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.
+ */
+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling MSI/MSI-X\n");
+}
+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

Document Amazon's Annapurna Labs PCIe host bridge.

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

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

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

new file mode 100644
index 000000000000..557a5089229d
@@ -0,0 +1,46 @@
+* Amazon Annapurna Labs PCIe host bridge
+
+Amazon's Annapurna Labs PCIe Host Controller is based on the Synopsys DesignWare
+PCI core. It inherits common properties defined in
+Documentation/devicetree/bindings/pci/designware-pcie.txt.
+
+Properties of the host controller node that differ from it are:
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Value should contain
+			- "amazon,al-alpine-v2-pcie" for alpine_v2
+			- "amazon,al-alpine-v3-pcie" for alpine_v3
+
+- 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-alpine-v3-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 +0 -0

index 783569e3c4b4..d200b16fa95c 100644
@@ -12448,10 +12448,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
 S:	Supported
 F:	drivers/pci/controller/

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       | 46 +++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

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. This is achieved by using a HW mechanism which allows changing the BUS part of the "final" outgoing config transaction. There are 2 HW regs, one which is basically a bitmask determining which bits to take from the AXI transaction itself and another which holds the complementary part programmed by the driver.

All link initializations are handled by the boot FW.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 365 +++++++++++++++++++++++++++
 2 files changed, 377 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 +1 -0

index 3ab58f0584a8..1eeda2f6371f 100644
@@ -91,3 +91,368 @@ struct pci_ecam_ops al_pcie_ops = {
 };
 
 #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
+

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

+
+#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_relaxed(pcie->controller_base + offset);
+}
+
+static inline void al_pcie_controller_writel(struct al_pcie *pcie, u32 offset,
+					     u32 val)
+{
+	writel_relaxed(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_dbg(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;

drivers/pci/controller/dwc/pcie-al.c +93 -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->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 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;
+
+	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)) {

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

+
+	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);
+
+	return al_add_pcie_port(&pci->pp, pdev);
+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-alpine-v2-pcie",
+	},
+	{ .compatible = "amazon,al-alpine-v3-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

Quoted 58 lines · Jonathan Chocron

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. This is achieved by using a HW
mechanism which allows changing the BUS part of the "final" outgoing
config transaction. There are 2 HW regs, one which is basically a bitmask determining which bits to take from the AXI transaction itself
and another which holds the complementary part programmed by the driver.

All link initializations are handled by the boot FW.

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

Reviewed-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/Kconfig   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 365
+++++++++++++++++++++++++++
 2 files changed, 377 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 +0 -0

index 3ab58f0584a8..1eeda2f6371f 100644
@@ -91,3 +91,368 @@ struct pci_ecam_ops al_pcie_ops = {
 };
 

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

Quoted 12 lines

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

 #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

Is this really needed? It cleanly describes how the hardware is composed but
given that it's 0 and will always be 0, it seems to add some bloat.

Quoted 231 lines

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

+
+#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 +5 -0

+{
+	return readl_relaxed(pcie->controller_base + offset);
+}
+
+static inline void al_pcie_controller_writel(struct al_pcie *pcie,

u32 offset,

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

+					     u32 val)
+{
+	writel_relaxed(val, pcie->controller_base + offset);
+}
+
+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 +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_write(pci_addr + where, size, val);
+
+	dev_dbg(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 +23 -0

+		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");

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

Quoted 1 line

reg_mask;

If I understand correctly - there is an optimisation in al_pcie_conf_addr_map
that prevents an unnecessary write to the controller if it's already programmed
to the bus we want to talk to. Due to this optimisation we need to ensure the
default values in target_bus_cfg match what the hardware is initially programmed
to. So to be on the safe side we program them at the start of day.

It sounds like the controller isn't reset anywhere post-firmware? so we have
no idea what value the hardware may be programmed to?

Quoted 104 lines

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 +79 -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 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;
+
+	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 struct pci_ecam_ops al_pcie_ops = { · line 421

Quoted 5 lines

drivers/pci/controller/dwc/pcie-al.c +5 -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);

Instead of getting the ecam_size from the device tree here, you could just use
al_pcie->pci->pp.cfg0_size instead. I.e. in al_pcie_config_prepare...

	ecam_bus_mask = (pcie->pci->pp.cfg0_size >> 19) - 1;

Though you could argue that accessing something private to the dwc host driver
unnecessarily increases the coupling between the two and makes the AL driver
more fragile. But just an observation I thought I'd point out.

va_cfg0_base, since it is more straightforward (and was already in use

Quoted 1 line

Even without any more changes to this patch:

Reviewed-by Andrew Murray <[email protected]>

Quoted 42 lines

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

+
+	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 +33 -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);
+	}
+
+	dev_dbg(dev, "From DT: dbi_base: %pR, controller_base: %pR\n",
+		dbi_res, controller_res);
+
+	platform_set_drvdata(pdev, al_pcie);
+
+	return al_add_pcie_port(&pci->pp, pdev);
+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-alpine-v2-pcie",
+	},
+	{ .compatible = "amazon,al-alpine-v3-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

Some PCIe controllers can be set to either Host or EP according to some early boot FW. To make sure there is no discrepancy (e.g. FW configured the port to EP mode while the DT specifies it as a host bridge or vice versa), a check has been added for each mode.

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

Acked-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

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

index 2bf5a35c0570..00e59a134b93 100644
@@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	int ret;
 	u32 reg;
 	void *addr;
+	u8 hdr_type;
 	unsigned int nbars;
 	unsigned int offset;
 	struct pci_epc *epc;

@@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
 		dev_err(dev, "Unable to read *num-ib-windows* property\n");

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

index f93252d0da5b..d2ca748e4c85 100644
@@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct pci_bus *child;
 	struct pci_host_bridge *bridge;
 	struct resource *cfg_res;
+	u8 hdr_type;
 	int ret;
 
 	raw_spin_lock_init(&pci->pp.lock);

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

+	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set to bridge type (hdr_type: 0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	pp->mem_base = pp->mem->start;
 
 	if (!pp->va_cfg0_base) {

Signature · 1 line

2.17.1

Quoted 59 lines · Jonathan Chocron

Some PCIe controllers can be set to either Host or EP according to some
early boot FW. To make sure there is no discrepancy (e.g. FW configured
the port to EP mode while the DT specifies it as a host bridge or vice
versa), a check has been added for each mode.

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

Acked-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

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

index 2bf5a35c0570..00e59a134b93 100644
@@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	int ret;
 	u32 reg;
 	void *addr;
+	u8 hdr_type;
 	unsigned int nbars;
 	unsigned int offset;
 	struct pci_epc *epc;

@@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev, "PCIe controller is not set to EP
 mode (hdr_type:0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	ret = of_property_read_u32(np, "num-ib-windows", &ep-
 >num_ib_windows);

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

 	if (ret < 0) {
 		dev_err(dev, "Unable to read *num-ib-windows*

property\n");

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

index f93252d0da5b..d2ca748e4c85 100644
@@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct pci_bus *child;
 	struct pci_host_bridge *bridge;
 	struct resource *cfg_res;
+	u8 hdr_type;
 	int ret;
 
 	raw_spin_lock_init(&pci->pp.lock);

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

Quoted 4 lines

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

@@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);

Do we know if it's always safe to read these registers at this point in time?

Later in dw_pcie_host_init we call pp->ops->host_init - looking at the
implementations of .host_init I can see:

- resets being performed (qcom_ep_reset_assert, artpec6_pcie_assert_core_reset, imx6_pcie_assert_core_reset)
- changes to config space registers (ks_pcie_init_id,
dw_pcie_setup_rc)
including setting PCI_CLASS_DEVICE
- and clocks being enabled (qcom_pcie_init_1_0_0)

I'm not sure if your changes would cause anything to break for these other
controllers (or future controllers) as I couldn't see any other reads to the
config.

Given that we are reading config space should dw_pcie_rd_own_conf be used?

(For example kirin_pcie_rd_own_conf does something special).

Thanks,

Andrew Murray

Quoted 13 lines

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

+	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set to bridge
 type (hdr_type: 0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	pp->mem_base = pp->mem->start;
 

if (!pp->va_cfg0_base) {

Signature · 1 line

2.17.1

Quoted 72 lines · Andrew Murray

Quoted 59 lines · Jonathan Chocron

Some PCIe controllers can be set to either Host or EP according to some
early boot FW. To make sure there is no discrepancy (e.g. FW configured
the port to EP mode while the DT specifies it as a host bridge or vice
versa), a check has been added for each mode.

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

Acked-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

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

index 2bf5a35c0570..00e59a134b93 100644
@@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	int ret;
 	u32 reg;
 	void *addr;
+	u8 hdr_type;
 	unsigned int nbars;
 	unsigned int offset;
 	struct pci_epc *epc;

@@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev, "PCIe controller is not set to EP
 mode (hdr_type:0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	ret = of_property_read_u32(np, "num-ib-windows", &ep-
 >num_ib_windows);

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

 	if (ret < 0) {
 		dev_err(dev, "Unable to read *num-ib-windows*

property\n");

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

index f93252d0da5b..d2ca748e4c85 100644
@@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct pci_bus *child;
 	struct pci_host_bridge *bridge;
 	struct resource *cfg_res;
+	u8 hdr_type;
 	int ret;
 
 	raw_spin_lock_init(&pci->pp.lock);

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

Quoted 4 lines

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

@@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);

Do we know if it's always safe to read these registers at this point in time?

Later in dw_pcie_host_init we call pp->ops->host_init - looking at the
implementations of .host_init I can see:

Quoted 7 lines

- resets being performed (qcom_ep_reset_assert, artpec6_pcie_assert_core_reset, imx6_pcie_assert_core_reset)
- changes to config space registers (ks_pcie_init_id,
dw_pcie_setup_rc)
including setting PCI_CLASS_DEVICE
- and clocks being enabled (qcom_pcie_init_1_0_0)

Good point! This indeed might break host drivers which actually setup the rc in the kernel, and don't depend on early boot FW. So the validation should probably be moved to after pp->ops->host_init() (and similarly after ep->ops->ep_init() for the ep driver), right?

Quoted 7 lines

I'm not sure if your changes would cause anything to break for these other
controllers (or future controllers) as I couldn't see any other reads to the
config.

Quoted 2 lines

Given that we are reading config space should dw_pcie_rd_own_conf be used?

The config space of the DW core is located at the beginning of the DBI regspace. Furthermore, this would break the "symmetry" between the host and ep validations (since the ep has no notion of reading from config space nor a .read callback in struct dw_pcie_ep_ops).

I agree that
there is some sort of overlap between the dw_pcie_read{/write}_dbi dw_pcie_rd{/wr}_own_conf APIs, when accessing the host mode config space, but I assume that any host driver which supplies a callback for .rd_own_conf() must supply an equivalent .read_dbi() one as well.

Quoted 2 lines

(For example kirin_pcie_rd_own_conf does something special).

They specifically define an equivalent kirin_pcie_read_dbi().

Quoted 18 lines

Thanks,

Andrew Murray

Quoted 13 lines

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

+	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set to bridge
 type (hdr_type: 0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	pp->mem_base = pp->mem->start;
 

if (!pp->va_cfg0_base) {

Signature · 1 line

2.17.1

Quoted 88 lines · Jonathan

Quoted 86 lines · Andrew Murray

Quoted 63 lines · Jonathan Chocron

Some PCIe controllers can be set to either Host or EP according to
some
early boot FW. To make sure there is no discrepancy (e.g. FW configured
the port to EP mode while the DT specifies it as a host bridge or
vice
versa), a check has been added for each mode.

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

Acked-by Gustavo Pimentel <[email protected]>

---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

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

index 2bf5a35c0570..00e59a134b93 100644
@@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	int ret;
 	u32 reg;
 	void *addr;
+	u8 hdr_type;
 	unsigned int nbars;
 	unsigned int offset;
 	struct pci_epc *epc;

@@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev, "PCIe controller is not set

to EP
mode (hdr_type:0x%x)!\n",

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

+			hdr_type);
+		return -EIO;
+	}
+
 	ret = of_property_read_u32(np, "num-ib-windows", &ep-

Quoted 1 line

num_ib_windows);

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

 	if (ret < 0) {
 		dev_err(dev, "Unable to read *num-ib-windows*

property\n");

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

index f93252d0da5b..d2ca748e4c85 100644
@@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct pci_bus *child;
 	struct pci_host_bridge *bridge;
 	struct resource *cfg_res;
+	u8 hdr_type;
 	int ret;
 
 	raw_spin_lock_init(&pci->pp.lock);

drivers/pci/controller/dwc/pcie-designware-host.c int dw_pcie_host_init(struct pcie_port · line 401

Quoted 5 lines

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

@@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port
 *pp)
 		}
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);

Do we know if it's always safe to read these registers at this point
in time?

Later in dw_pcie_host_init we call pp->ops->host_init - looking at
the
implementations of .host_init I can see:

- resets being performed (qcom_ep_reset_assert, artpec6_pcie_assert_core_reset, imx6_pcie_assert_core_reset)
- changes to config space registers (ks_pcie_init_id,
dw_pcie_setup_rc)
including setting PCI_CLASS_DEVICE
- and clocks being enabled (qcom_pcie_init_1_0_0)

Quoted 6 lines

Good point! This indeed might break host drivers which actually setup
the rc in the kernel, and don't depend on early boot FW. So the validation should probably be moved to after pp->ops->host_init() (and
similarly after ep->ops->ep_init() for the ep driver), right?

Yes I think so.

Quoted 14 lines

Quoted 10 lines

I'm not sure if your changes would cause anything to break for these
other
controllers (or future controllers) as I couldn't see any other reads
to the
config.

Quoted 3 lines

Given that we are reading config space should dw_pcie_rd_own_conf be
used?

Quoted 7 lines

The config space of the DW core is located at the beginning of the DBI
regspace. Furthermore, this would break the "symmetry" between the host
and ep validations (since the ep has no notion of reading from config
space nor a .read callback in struct dw_pcie_ep_ops).

This is true, though given how different the two drivers are - this is only
really 'nice to have'.

Quoted 11 lines

I agree that
there is some sort of overlap between the dw_pcie_read{/write}_dbi dw_pcie_rd{/wr}_own_conf APIs, when accessing the host mode config space, but I assume that any host driver which supplies a callback for
.rd_own_conf() must supply an equivalent .read_dbi() one as well.

Quoted 2 lines

(For example kirin_pcie_rd_own_conf does something special).

They specifically define an equivalent kirin_pcie_read_dbi().

This may also be true. However given that the dwc framework gives host drivers
the ability to provide their own rd_own_conf callback - it's very possible
that these drivers can use the callback to handle quirks - now or in the
future. Potentially a quirk that your direct reading won't handle.

Looking at the existing drivers which provide a .rd_own_conf:

pci-exynos.c:
- Uses dw_pcie_read to directly read from registers, sandwich'd
between writes
to set/clear PCIE_ELBI_SLV_ARMISC. This driver provides a
.read_dbi which
means dw_pcie_readb_dbi probably does the same thing as
rd_own_conf.

pci-menson.c:
- Uses dw_pcie_read to directly read from registers, it then applies
a read
quirk for the PCI_CLASS_DEVICE register. This driver doesn't
provide a
.read_dbi - thus dw_pcie_readb_dbi probably does the same thing as rd_own_conf provided you don't read the PCI_CLASS_DEVICE register.

pcie-hisi.c:
- Uses dw_pcie_readl_dbi to read from registers, but the controller
only
supports 32bit aligned reads - it doesn't provide a .read_dbi.
This means
that if you used dw_pcie_readb_dbi it'd probably break because you
can't
read 1 byte at the offset where HEADER_TYPE is.

pcie-histb.c, pcie-kirin.c: looks like pci-exynos.c

It looks like we're going to have to break symmetry...

Thanks,

Andrew Murray

Quoted 19 lines

Quoted 19 lines

Thanks,

Andrew Murray

Quoted 14 lines

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

+	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set

to bridge
type (hdr_type: 0x%x)!\n",

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

+			hdr_type);
+		return -EIO;
+	}
+
 	pp->mem_base = pp->mem->start;
 
 	if (!pp->va_cfg0_base) {

Signature · 1 line

2.17.1

This series adds support for Amazon's Annapurna Labs DT-based PCIe host controller driver.
Additionally, it adds 3 quirks (ACS, VPD and MSI-X) and 2 generic DWC patches.

Changes since v3:
- Removed PATCH 8/8 since the usage of the PCI flags will be discussed in the upcoming LPC
- Align commit subject with the folder convention
- Added explanation regarding ECAM "overload" mechanism
- Switched to read/write{_relaxed} APIs
- Modified a dev_err to dev_dbg
- Removed unnecessary variable
- Removed driver details from dt-binding description
- Changed to SoC specific compatibles
- Fixed typo in a commit message
- Added comment regarding MSI in the MSI-X quirk

Changes since v2:
- Added al_pcie_controller_readl/writel() wrappers
- Reorganized local vars in several functions according to reverse tree structure
- Removed unnecessary check of ret value
- Changed return type of al_pcie_config_prepare() from int to void
- Removed check if link is up from probe() [done internally in dw_pcie_rd/wr_conf()]

Changes since v1:
- Added comment regarding 0x0031 being used as a dev_id for non root-port devices as well
- Fixed different message/comment/print wordings
- Added panic stacktrace to commit message of MSI-x quirk patch
- Changed to pci_warn() instead of dev_warn()
- Added unit_address after node_name in dt-binding
- Updated Kconfig help description
- Used GENMASK and FIELD_PREP/GET where appropriate
- Removed leftover field from struct al_pcie and moved all ptrs to
the beginning
- Re-wrapped function definitions and invocations to use fewer lines
- Change %p to %px in dbg prints in rd/wr_conf() functions
- Removed validation that the port is configured to RC mode (as this is added generically in PATCH 7/8)
- Removed unnecessary variable initializations
- Swtiched to %pR for printing resources

Ali Saidi (1):
PCI: Add ACS quirk for Amazon Annapurna Labs root ports

Jonathan Chocron (6):
PCI: Add Amazon's Annapurna Labs vendor ID
PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port
PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs
Root Port
dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding
PCI: dwc: al: Add support for DW based driver type
PCI: dwc: Add validation that PCIe core is set to correct mode

 .../devicetree/bindings/pci/pcie-al.txt       |  46 +++
 MAINTAINERS                                   |   3 +-
 drivers/pci/controller/dwc/Kconfig            |  12 +
 drivers/pci/controller/dwc/pcie-al.c          | 365 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   8 +
 .../pci/controller/dwc/pcie-designware-host.c |   8 +
 drivers/pci/quirks.c                          |  37 ++
 drivers/pci/vpd.c                             |  16 +
 include/linux/pci_ids.h                       |   2 +
 9 files changed, 496 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

Quoted 77 lines · Jonathan Chocron

This series adds support for Amazon's Annapurna Labs DT-based PCIe host
controller driver.
Additionally, it adds 3 quirks (ACS, VPD and MSI-X) and 2 generic DWC patches.

Changes since v3:
- Removed PATCH 8/8 since the usage of the PCI flags will be discussed
in the upcoming LPC
- Align commit subject with the folder convention
- Added explanation regarding ECAM "overload" mechanism
- Switched to read/write{_relaxed} APIs
- Modified a dev_err to dev_dbg
- Removed unnecessary variable
- Removed driver details from dt-binding description
- Changed to SoC specific compatibles
- Fixed typo in a commit message
- Added comment regarding MSI in the MSI-X quirk

Changes since v2:
- Added al_pcie_controller_readl/writel() wrappers
- Reorganized local vars in several functions according to reverse tree structure
- Removed unnecessary check of ret value
- Changed return type of al_pcie_config_prepare() from int to void
- Removed check if link is up from probe() [done internally in dw_pcie_rd/wr_conf()]

Changes since v1:
- Added comment regarding 0x0031 being used as a dev_id for non root-port devices as well
- Fixed different message/comment/print wordings
- Added panic stacktrace to commit message of MSI-x quirk patch
- Changed to pci_warn() instead of dev_warn()
- Added unit_address after node_name in dt-binding
- Updated Kconfig help description
- Used GENMASK and FIELD_PREP/GET where appropriate
- Removed leftover field from struct al_pcie and moved all ptrs to the beginning
- Re-wrapped function definitions and invocations to use fewer lines
- Change %p to %px in dbg prints in rd/wr_conf() functions
- Removed validation that the port is configured to RC mode (as this is
added generically in PATCH 7/8)
- Removed unnecessary variable initializations
- Swtiched to %pR for printing resources

Ali Saidi (1):
PCI: Add ACS quirk for Amazon Annapurna Labs root ports

Jonathan Chocron (6):
PCI: Add Amazon's Annapurna Labs vendor ID
PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root
Port
PCI: Add quirk to disable MSI-X support for Amazon's Annapurna
Labs
Root Port
dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge
binding
PCI: dwc: al: Add support for DW based driver type
PCI: dwc: Add validation that PCIe core is set to correct mode

 .../devicetree/bindings/pci/pcie-al.txt       |  46 +++
 MAINTAINERS                                   |   3 +-
 drivers/pci/controller/dwc/Kconfig            |  12 +
 drivers/pci/controller/dwc/pcie-al.c          | 365
++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   8 +
 .../pci/controller/dwc/pcie-designware-host.c |   8 +
 drivers/pci/quirks.c                          |  37 ++
 drivers/pci/vpd.c                             |  16 +
 include/linux/pci_ids.h                       |   2 +
 9 files changed, 496 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-

Quoted 1 line

al.txt

Hi Jonathan,

are you going to send a v5 for this series ? If we should consider it for v5.4 I expect it to be on the list this week as soon as possible.

Thanks,
Lorenzo