Re: [PATCH 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding

Re: [PATCH 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding

[Shenhar, Talel]

On 7/10/2019 7:45 PM, Jonathan Chocron wrote:
> Document Amazon's Annapurna Labs PCIe host bridge.

That is the way! (best to keep same wordings (Amazon's)

>
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>   .../devicetree/bindings/pci/pcie-al.txt       | 45 +++++++++++++++++++
>   MAINTAINERS                                   |  1 +
>   2 files changed, 46 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt b/Documentation/devicetree/bindings/pci/pcie-al.txt
> new file mode 100644
> index 000000000000..d92cc529490e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> @@ -0,0 +1,45 @@
> +* Amazon Annapurna Labs PCIe host bridge
> +
> +Annapurna Labs' PCIe Host Controller is based on the Synopsys DesignWare

Amazon's


and what is the s' ? should it be for all

> +Example:
> +
> +	pcie-external0 {
probably should have a reference with the address
> +		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_main GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
gic_main->gic
> +		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5a6137df3f0e..d555beb794bb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12205,6 +12205,7 @@ PCIE DRIVER FOR ANNAPURNA LABS
AMAZON!
>   M:	Jonathan Chocron <jonnyc@amazon.com>
>   L:	linux-pci@vger.kernel.org
>   S:	Maintained
> +F:	Documentation/devicetree/bindings/pci/pcie-al.txt
>   F:	drivers/pci/controller/dwc/pcie-al.c
>   
>   PCIE DRIVER FOR AMLOGIC MESON

Re: [PATCH 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding

[Lorenzo Pieralisi]

On Thu, Jul 11, 2019 at 10:12:35AM +0300, Shenhar, Talel wrote:
> 
> On 7/10/2019 7:45 PM, Jonathan Chocron wrote:
> > Document Amazon's Annapurna Labs PCIe host bridge.
> 
> That is the way! (best to keep same wordings (Amazon's)

Guys,

as a heads-up, the original posting, for whatever reason, did not hit
linux-pci@vger, which means that from a PCI patches queue review point
of view it does not exist.

It ought to be fixed otherwise we won't be able to review the code.

Lorenzo

> > 
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> >   .../devicetree/bindings/pci/pcie-al.txt       | 45 +++++++++++++++++++
> >   MAINTAINERS                                   |  1 +
> >   2 files changed, 46 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > new file mode 100644
> > index 000000000000..d92cc529490e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > @@ -0,0 +1,45 @@
> > +* Amazon Annapurna Labs PCIe host bridge
> > +
> > +Annapurna Labs' PCIe Host Controller is based on the Synopsys DesignWare
> 
> Amazon's
> 
> 
> and what is the s' ? should it be for all
> 
> > +Example:
> > +
> > +	pcie-external0 {
> probably should have a reference with the address
> > +		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_main GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
> gic_main->gic
> > +		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
> > +	};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5a6137df3f0e..d555beb794bb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12205,6 +12205,7 @@ PCIE DRIVER FOR ANNAPURNA LABS
> AMAZON!
> >   M:	Jonathan Chocron <jonnyc@amazon.com>
> >   L:	linux-pci@vger.kernel.org
> >   S:	Maintained
> > +F:	Documentation/devicetree/bindings/pci/pcie-al.txt
> >   F:	drivers/pci/controller/dwc/pcie-al.c
> >   PCIE DRIVER FOR AMLOGIC MESON

[PATCH 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding

[Jonathan Chocron]

Document Amazon's Annapurna Labs PCIe host bridge.

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 .../devicetree/bindings/pci/pcie-al.txt       | 45 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt b/Documentation/devicetree/bindings/pci/pcie-al.txt
new file mode 100644
index 000000000000..d92cc529490e
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
@@ -0,0 +1,45 @@
+* Amazon Annapurna Labs PCIe host bridge
+
+Annapurna Labs' PCIe Host Controller is based on the Synopsys DesignWare
+PCI core.
+It shares common functions with the PCIe DesignWare core driver and 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-pcie"
+
+- 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 {
+		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_main GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
+		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 5a6137df3f0e..d555beb794bb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12205,6 +12205,7 @@ PCIE DRIVER FOR ANNAPURNA LABS
 M:	Jonathan Chocron <jonnyc@amazon.com>
 L:	linux-pci@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/pci/pcie-al.txt
 F:	drivers/pci/controller/dwc/pcie-al.c
 
 PCIE DRIVER FOR AMLOGIC MESON
-- 
2.17.1

[PATCH 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver

[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.

Regarding the 2nd DWC patch (PCI flags support), do you think this should
be done in the context of a host-bridge driver at all (as opposed to PCI
system-wide code)?


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

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

 .../devicetree/bindings/pci/pcie-al.txt       |  45 ++
 MAINTAINERS                                   |   1 +
 drivers/pci/controller/dwc/Kconfig            |  11 +
 drivers/pci/controller/dwc/pcie-al.c          | 397 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   8 +
 .../pci/controller/dwc/pcie-designware-host.c |  31 +-
 drivers/pci/quirks.c                          |  27 ++
 drivers/pci/vpd.c                             |  12 +
 include/linux/pci_ids.h                       |   2 +
 9 files changed, 530 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

-- 
2.17.1

[PATCH 1/8] PCI: Add Amazon's Annapurna Labs vendor ID

[Jonathan Chocron]

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

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 include/linux/pci_ids.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 0dd239f11e91..ed350fd522c6 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2568,6 +2568,8 @@
 
 #define PCI_VENDOR_ID_ASMEDIA		0x1b21
 
+#define PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS	0x1c36
+
 #define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
 #define PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD	0x0001
 
-- 
2.17.1

[PATCH 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports

[Jonathan Chocron]

From: Ali Saidi <alisaidi@amazon.com>

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 <alisaidi@amazon.com>
Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 drivers/pci/quirks.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c66c0ca446c4..11850b030637 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4366,6 +4366,23 @@ static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
 	return ret;
 }
 
+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
+	 * peer-to-peer transactions via the root port and each has a unique
+	 * segment number.
+	 * Additionally, the root ports cannot send traffic to each other.
+	 */
+	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 }
 };
 
-- 
2.17.1

[PATCH 3/8] PCI/VPD: Add VPD release quirk for Amazon Annapurna Labs host bridge

[Jonathan Chocron]

The Amazon Annapurna Labs pcie host bridge 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 <jonnyc@amazon.com>
---
 drivers/pci/vpd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 4963c2e2bd4c..b594b2895ffe 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -644,4 +644,16 @@ 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 "Annapurna Labs pcie quirk - Releasing VPD capability (No support for VPD read/write transactions)\n");
+	}
+}
+
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
+			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_vpd_release);
+
 #endif
-- 
2.17.1

[PATCH 4/8] PCI: Add quirk to disable MSI support for Amazon's Annapurna Labs host bridge

[Jonathan Chocron]

On some platforms, the host bridge exposes an MSI-X capability but
doesn't actually support it.
This causes a crash during initialization by the pcieport driver, since
it tries to configure the MSI-X capability.

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 drivers/pci/quirks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 11850b030637..0fb70d755977 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2925,6 +2925,14 @@ 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);
+
+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	dev_warn(&dev->dev, "Annapurna Labs pcie quirk - disabling MSI\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 */
 
 /*
-- 
2.17.1

[PATCH 6/8] PCI: al: Add support for DW based driver type

[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.

All link initializations are handled by the boot FW.

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 drivers/pci/controller/dwc/Kconfig   |  11 +
 drivers/pci/controller/dwc/pcie-al.c | 397 +++++++++++++++++++++++++++
 2 files changed, 408 insertions(+)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index a6ce1ee51b4c..51da9cb219aa 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -230,4 +230,15 @@ 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 Annapurna Labs PCIe controller IP
+	  on Amazon SoCs.
+	  The PCIe controller uses the DesignWare core plus
+	  Amazon-specific hardware wrappers.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
index 3ab58f0584a8..2742a0895aab 100644
--- a/drivers/pci/controller/dwc/pcie-al.c
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -91,3 +91,400 @@ 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
+
+/* The AXI regs are always at the beginning of the PCIE controller reg space. */
+#define AXI_BASE_OFFSET		0x0
+
+#define DEVICE_ID_OFFSET	0x16c
+
+#define DEVICE_REV_ID			0x0
+#define DEVICE_REV_ID_DEV_ID_MASK	0xFFFF0000
+#define DEVICE_REV_ID_DEV_ID_SHIFT	16
+
+#define DEVICE_REV_ID_DEV_ID_X4		(0 << DEVICE_REV_ID_DEV_ID_SHIFT)
+#define DEVICE_REV_ID_DEV_ID_X8		(2 << DEVICE_REV_ID_DEV_ID_SHIFT)
+#define DEVICE_REV_ID_DEV_ID_X16	(4 << DEVICE_REV_ID_DEV_ID_SHIFT)
+
+/* The ob_ctrl offset inside the axi regs is different between revisions.
+ */
+#define OB_CTRL_REV1_2_OFFSET	0x0040
+#define OB_CTRL_REV3_5_OFFSET	0x0030
+
+#define CFG_TARGET_BUS			0x0
+#define CFG_TARGET_BUS_MASK_SHIFT	0
+#define CFG_TARGET_BUS_BUSNUM_SHIFT	8
+
+#define CFG_CONTROL			0x4
+#define CFG_CONTROL_SUBBUS_MASK		0x0000FF00
+#define CFG_CONTROL_SUBBUS_SHIFT	8
+#define CFG_CONTROL_SEC_BUS_MASK	0x00FF0000
+#define CFG_CONTROL_SEC_BUS_SHIFT	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) */
+	void __iomem	*dbi_base;
+	resource_size_t ecam_size;
+	struct device *dev;
+	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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
+{
+	void __iomem *dev_rev_id_addr;
+	u32 dev_rev_id;
+
+	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
+			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET + DEVICE_REV_ID);
+
+	dev_rev_id = readl(dev_rev_id_addr) & DEVICE_REV_ID_DEV_ID_MASK;
+
+	switch (dev_rev_id) {
+	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_rev_id (0x%08x)\n",
+			dev_rev_id);
+		return -EINVAL;
+	}
+
+	dev_dbg(pcie->dev, "dev_rev_id: 0x%08x\n", dev_rev_id);
+
+	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)
+{
+	void __iomem *cfg_control_addr;
+	u32 reg = (target_bus << CFG_TARGET_BUS_BUSNUM_SHIFT) |
+		       mask_target_bus;
+
+	cfg_control_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
+			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
+			   CFG_TARGET_BUS);
+
+	writel(reg, cfg_control_addr);
+}
+
+static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
+					   unsigned int busnr,
+					   unsigned int devfn)
+{
+	void __iomem *pci_base_addr;
+	struct pcie_port *pp = &pcie->pci->pp;
+	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;
+
+	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 %d to %d\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 %#x (pci_addr: 0x%p) - 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 %#x (pci_addr: 0x%p) - 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_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;
+	u8 secondary_bus;
+	u8 subordinate_bus;
+	void __iomem *cfg_control_addr;
+	u32 cfg_control;
+	u32 reg = 0;
+
+	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_addr = pcie->controller_base +
+		AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
+
+	cfg_control = readl(cfg_control_addr);
+
+	reg = (cfg_control & ~CFG_CONTROL_SEC_BUS_MASK) |
+	       (secondary_bus << CFG_CONTROL_SEC_BUS_SHIFT);
+	reg = (reg & ~CFG_CONTROL_SUBBUS_MASK) |
+	       (subordinate_bus << CFG_CONTROL_SUBBUS_SHIFT);
+
+	writel(reg, cfg_control_addr);
+
+	return 0;
+}
+
+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 = 0;
+	u32 reg = 0;
+	int rc;
+
+	reg = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (reg != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set to bridge type (%d)!\n",
+			reg);
+		return -EIO;
+	}
+
+	link_up = dw_pcie_link_up(pci);
+	if (!link_up) {
+		dev_err(pci->dev, "link in not up!\n");
+		return -ENOLINK;
+	}
+
+	dev_info(pci->dev, "link is up\n");
+
+	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;
+
+	rc = al_pcie_config_prepare(pcie);
+	if (rc)
+		return rc;
+
+	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 al_pcie *al_pcie;
+	struct dw_pcie *pci;
+	struct resource *dbi_res;
+	struct resource *controller_res;
+	struct resource *ecam_res;
+	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 %p\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 %p\n",
+			controller_res);
+		return PTR_ERR(al_pcie->controller_base);
+	}
+
+	dev_dbg(dev, "From DT: dbi_base: 0x%llx, controller_base: 0x%llx\n",
+		dbi_res->start, controller_res->start);
+
+	platform_set_drvdata(pdev, al_pcie);
+
+	ret = al_add_pcie_port(&pci->pp, pdev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-pcie",
+	  .data = NULL,
+	},
+	{},
+};
+
+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*/
-- 
2.17.1

[PATCH 7/8] PCI: dw: Add validation that PCIe core is set to correct mode

[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 <jonnyc@amazon.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 2bf5a35c0570..00e59a134b93 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -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");
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f93252d0da5b..d2ca748e4c85 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -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);
@@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	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) {
-- 
2.17.1

[PATCH 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags

[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 <jonnyc@amazon.com>
---
 .../pci/controller/dwc/pcie-designware-host.c | 23 +++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d2ca748e4c85..0a294d8aa21a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -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)
@@ -474,6 +476,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	pp->root_bus_nr = pp->busn->start;
 
+	/* Do not reassign bus nums if probe only */
+	if (!pci_has_flag(PCI_PROBE_ONLY))
+		pci_add_flags(PCI_REASSIGN_ALL_BUS);
+
 	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;
-- 
2.17.1

Re: [PATCH 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding

[Chocron, Jonathan]

On Thu, 2019-07-11 at 10:32 +0100, Lorenzo Pieralisi wrote:
> On Thu, Jul 11, 2019 at 10:12:35AM +0300, Shenhar, Talel wrote:
> > 
> > On 7/10/2019 7:45 PM, Jonathan Chocron wrote:
> > > Document Amazon's Annapurna Labs PCIe host bridge.
> > 
> > That is the way! (best to keep same wordings (Amazon's)
> 
> Guys,
> 
> as a heads-up, the original posting, for whatever reason, did not hit
> linux-pci@vger, which means that from a PCI patches queue review
> point
> of view it does not exist.
> 
> It ought to be fixed otherwise we won't be able to review the code.
> 
> Lorenzo
> 

I've succeeded to redirect the original emails to linux-pci@vger, so
please proceed with reviewing the patches.

Thanks,
   Jonathan

Re: [PATCH 4/8] PCI: Add quirk to disable MSI support for Amazon's Annapurna Labs host bridge

[Bjorn Helgaas]

On Thu, Jul 11, 2019 at 05:56:25PM +0300, Jonathan Chocron wrote:
> On some platforms, the host bridge exposes an MSI-X capability but
> doesn't actually support it.
> This causes a crash during initialization by the pcieport driver, since
> it tries to configure the MSI-X capability.

Nit: The formatting above is jarring to read because I can't tell
whether it's one paragraph or two.

Either rewrap it into a single paragraph or add a blank line to make
two paragraphs.  I noticed this elsewhere, too, in a comment, I think.

s/host bridge/Root Port/, if I understand correctly.

I don't understand the "on some platforms..." part.  Do you mean that
on *every* platform, this particular host bridge (identified by
[1c36:0031]) advertises an MSI-X capability that doesn't work?

Or are there some platforms that configure the bridge so it doesn't
advertise MSI-X at all, while other platforms configure it so it
*does* advertise MSI-X?

If there's a line or two of diagnostics from the crash you could
include here, that would help people who encounter the crash find
the solution.

> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/quirks.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 11850b030637..0fb70d755977 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2925,6 +2925,14 @@ 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);
> +
> +static void quirk_al_msi_disable(struct pci_dev *dev)
> +{
> +	dev->no_msi = 1;
> +	dev_warn(&dev->dev, "Annapurna Labs pcie quirk - disabling MSI\n");

s/pcie/PCIe/ in English text, comments, printk strings, etc.

Actually, I think the whole "Annapurna Labs pcie quirk" part is
probably unnecessary, since we can identify the device via the
dev_printk() info.

Speaking of which, you can use "pci_warn(dev)" here to be consistent
with the rest of the file.

> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
> +			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_msi_disable);

Why do you use the class fixup here instead of the simpler
DECLARE_PCI_FIXUP_FINAL()?  Requiring the class to match
PCI_CLASS_BRIDGE_PCI suggests that there may be other [1c36:0031]
devices that are not Root Ports.  If that's the case, please mention
it so it's clear why we need DECLARE_PCI_FIXUP_CLASS_FINAL().  If not,
just use DECLARE_PCI_FIXUP_FINAL().

>  #endif /* CONFIG_PCI_MSI */
>  
>  /*
> -- 
> 2.17.1
> 
> 

Re: [PATCH 1/8] PCI: Add Amazon's Annapurna Labs vendor ID

[Bjorn Helgaas]

On Thu, Jul 11, 2019 at 05:53:31PM +0300, Jonathan Chocron wrote:
> Add Amazon's Annapurna Labs vendor ID to pci_ids.h.
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  include/linux/pci_ids.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 0dd239f11e91..ed350fd522c6 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2568,6 +2568,8 @@
>  
>  #define PCI_VENDOR_ID_ASMEDIA		0x1b21
>  
> +#define PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS	0x1c36
> +
>  #define PCI_VENDOR_ID_CIRCUITCO		0x1cc8
>  #define PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD	0x0001
>  
> -- 
> 2.17.1
> 
> 

Re: [PATCH 3/8] PCI/VPD: Add VPD release quirk for Amazon Annapurna Labs host bridge

[Bjorn Helgaas]

On Thu, Jul 11, 2019 at 05:55:56PM +0300, Jonathan Chocron wrote:
> The Amazon Annapurna Labs pcie host bridge exposes the VPD capability,
> but there is no actual support for it.

s/pcie/PCIe/
s/host bridge/Root Port/

> 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 <jonnyc@amazon.com>
> ---
>  drivers/pci/vpd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 4963c2e2bd4c..b594b2895ffe 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -644,4 +644,16 @@ 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 "Annapurna Labs pcie quirk - Releasing VPD capability (No support for VPD read/write transactions)\n");

The "Annapurna Labs pcie quirk" text is superfluous.

> +	}
> +}
> +
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
> +			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_vpd_release);

Why DECLARE_PCI_FIXUP_CLASS_FINAL()?  See comments on the MSI-X quirk
patch.

> +
>  #endif
> -- 
> 2.17.1
> 
> 

Re: [PATCH 6/8] PCI: al: Add support for DW based driver type

[Bjorn Helgaas]

On Thu, Jul 11, 2019 at 05:57:05PM +0300, Jonathan Chocron wrote:
> 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 <jonnyc@amazon.com>
> ---
>  drivers/pci/controller/dwc/Kconfig   |  11 +
>  drivers/pci/controller/dwc/pcie-al.c | 397 +++++++++++++++++++++++++++
>  2 files changed, 408 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index a6ce1ee51b4c..51da9cb219aa 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -230,4 +230,15 @@ 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 Annapurna Labs PCIe controller IP
> +	  on Amazon SoCs.
> +	  The PCIe controller uses the DesignWare core plus
> +	  Amazon-specific hardware wrappers.

Is this one paragraph or two?

This should mention the fact that only DT platforms need
CONFIG_PCIE_AL.  ACPI platforms with the Annapurna Labs PCIe
controller don't need this.

> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> index 3ab58f0584a8..2742a0895aab 100644
> --- a/drivers/pci/controller/dwc/pcie-al.c
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -91,3 +91,400 @@ 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
> +
> +/* The AXI regs are always at the beginning of the PCIE controller reg space. */
> +#define AXI_BASE_OFFSET		0x0
> +
> +#define DEVICE_ID_OFFSET	0x16c
> +
> +#define DEVICE_REV_ID			0x0
> +#define DEVICE_REV_ID_DEV_ID_MASK	0xFFFF0000
> +#define DEVICE_REV_ID_DEV_ID_SHIFT	16
> +
> +#define DEVICE_REV_ID_DEV_ID_X4		(0 << DEVICE_REV_ID_DEV_ID_SHIFT)
> +#define DEVICE_REV_ID_DEV_ID_X8		(2 << DEVICE_REV_ID_DEV_ID_SHIFT)
> +#define DEVICE_REV_ID_DEV_ID_X16	(4 << DEVICE_REV_ID_DEV_ID_SHIFT)
> +
> +/* The ob_ctrl offset inside the axi regs is different between revisions.
> + */

s/axi/AXI/ to be consistent.

Fix comment style (either single-line with "/* ... */" or multi-line
with "/*" on first line and "*/" on last line).

> +#define OB_CTRL_REV1_2_OFFSET	0x0040
> +#define OB_CTRL_REV3_5_OFFSET	0x0030
> +
> +#define CFG_TARGET_BUS			0x0
> +#define CFG_TARGET_BUS_MASK_SHIFT	0
> +#define CFG_TARGET_BUS_BUSNUM_SHIFT	8
> +
> +#define CFG_CONTROL			0x4
> +#define CFG_CONTROL_SUBBUS_MASK		0x0000FF00
> +#define CFG_CONTROL_SUBBUS_SHIFT	8
> +#define CFG_CONTROL_SEC_BUS_MASK	0x00FF0000
> +#define CFG_CONTROL_SEC_BUS_SHIFT	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) */
> +	void __iomem	*dbi_base;

I *guess* this is different from the dw_pcie.dbi_base?

> +	resource_size_t ecam_size;
> +	struct device *dev;
> +	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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)

I think al_pcie_rev_id_get() and al_pcie_reg_offsets_set() are more
complicated than necessary, at least for the current code.  All you're
really using is something like this:

  static void al_pcie_rev_id_get(...)
  {
    ...
    switch (dev_rev_id) {
    case DEVICE_REV_ID_DEV_ID_X4:
      pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
      break;
    case DEVICE_REV_ID_DEV_ID_X8:
    case DEVICE_REV_ID_DEV_ID_X16:
      pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
      break;
    default:
      dev_err("unsupported rev_id\n");
      break;
    }
  }

> +{
> +	void __iomem *dev_rev_id_addr;
> +	u32 dev_rev_id;
> +
> +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
> +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET + DEVICE_REV_ID);
> +
> +	dev_rev_id = readl(dev_rev_id_addr) & DEVICE_REV_ID_DEV_ID_MASK;
> +
> +	switch (dev_rev_id) {
> +	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_rev_id (0x%08x)\n",
> +			dev_rev_id);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(pcie->dev, "dev_rev_id: 0x%08x\n", dev_rev_id);
> +
> +	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)
> +{
> +	void __iomem *cfg_control_addr;
> +	u32 reg = (target_bus << CFG_TARGET_BUS_BUSNUM_SHIFT) |
> +		       mask_target_bus;
> +
> +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
> +			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
> +			   CFG_TARGET_BUS);
> +
> +	writel(reg, cfg_control_addr);
> +}
> +
> +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> +					   unsigned int busnr,
> +					   unsigned int devfn)
> +{
> +	void __iomem *pci_base_addr;
> +	struct pcie_port *pp = &pcie->pci->pp;
> +	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;
> +
> +	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 %d to %d\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)

Rewrap to use fewer lines.

> +{
> +	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 %#x (pci_addr: 0x%p) - 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 %#x (pci_addr: 0x%p) - 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_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;
> +	u8 secondary_bus;
> +	u8 subordinate_bus;
> +	void __iomem *cfg_control_addr;
> +	u32 cfg_control;
> +	u32 reg = 0;

Why is this initialized?  It doesn't look like it can be used before
being set.

> +	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_addr = pcie->controller_base +
> +		AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> +
> +	cfg_control = readl(cfg_control_addr);
> +
> +	reg = (cfg_control & ~CFG_CONTROL_SEC_BUS_MASK) |
> +	       (secondary_bus << CFG_CONTROL_SEC_BUS_SHIFT);
> +	reg = (reg & ~CFG_CONTROL_SUBBUS_MASK) |
> +	       (subordinate_bus << CFG_CONTROL_SUBBUS_SHIFT);
> +
> +	writel(reg, cfg_control_addr);
> +
> +	return 0;
> +}
> +
> +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 = 0;
> +	u32 reg = 0;

Why initialize link_up and reg?

> +	int rc;
> +
> +	reg = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> +	if (reg != PCI_HEADER_TYPE_BRIDGE) {
> +		dev_err(pci->dev, "PCIe controller is not set to bridge type (%d)!\n",
> +			reg);
> +		return -EIO;
> +	}
> +
> +	link_up = dw_pcie_link_up(pci);
> +	if (!link_up) {
> +		dev_err(pci->dev, "link in not up!\n");

s/in/is/

> +		return -ENOLINK;
> +	}
> +
> +	dev_info(pci->dev, "link is up\n");
> +
> +	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;
> +
> +	rc = al_pcie_config_prepare(pcie);
> +	if (rc)
> +		return rc;
> +
> +	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 al_pcie *al_pcie;
> +	struct dw_pcie *pci;
> +	struct resource *dbi_res;
> +	struct resource *controller_res;
> +	struct resource *ecam_res;
> +	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 %p\n", dbi_res);

%pR (see below).

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

Several of these function calls would fit on one line, or at least
fewer than they currently use.

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

Use %pR.  I think this %p prints the address of the struct resource,
not the address you tried to map.

> +		return PTR_ERR(al_pcie->controller_base);
> +	}
> +
> +	dev_dbg(dev, "From DT: dbi_base: 0x%llx, controller_base: 0x%llx\n",
> +		dbi_res->start, controller_res->start);

Use %pR instead of printing dbi_res->start, controller_res->start.

> +
> +	platform_set_drvdata(pdev, al_pcie);
> +
> +	ret = al_add_pcie_port(&pci->pp, pdev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id al_pcie_of_match[] = {
> +	{ .compatible = "amazon,al-pcie",
> +	  .data = NULL,

".data = NULL" is unnecessary.

> +	},
> +	{},
> +};
> +
> +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*/
> -- 
> 2.17.1
> 
> 

Re: [PATCH 3/8] PCI/VPD: Add VPD release quirk for Amazon Annapurna Labs host bridge

[Chocron, Jonathan]

On Fri, 2019-07-12 at 08:10 -0500, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2019 at 05:55:56PM +0300, Jonathan Chocron wrote:
> > The Amazon Annapurna Labs pcie host bridge exposes the VPD
> > capability,
> > but there is no actual support for it.
> 
> s/pcie/PCIe/
> s/host bridge/Root Port/
Ack.

> 
> > 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 <jonnyc@amazon.com>
> > ---
> >  drivers/pci/vpd.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> > index 4963c2e2bd4c..b594b2895ffe 100644
> > --- a/drivers/pci/vpd.c
> > +++ b/drivers/pci/vpd.c
> > @@ -644,4 +644,16 @@ 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 "Annapurna Labs pcie quirk -
> > Releasing VPD capability (No support for VPD read/write
> > transactions)\n");
> 
> The "Annapurna Labs pcie quirk" text is superfluous.
> 
Ack.

> > +	}
> > +}
> > +
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS,
> > 0x0031,
> > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > quirk_al_vpd_release);
> 
> Why DECLARE_PCI_FIXUP_CLASS_FINAL()?  See comments on the MSI-X quirk
> patch.
> 
Responded in the MSI-x quirk patch, but in short, indeed the 0x0031
dev-id is re-used for a non-host bridge device :(

> > +
> >  #endif
> > -- 
> > 2.17.1
> > 
> > 

Re: [PATCH 4/8] PCI: Add quirk to disable MSI support for Amazon's Annapurna Labs host bridge

[Chocron, Jonathan]

On Fri, 2019-07-12 at 08:04 -0500, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2019 at 05:56:25PM +0300, Jonathan Chocron wrote:
> > On some platforms, the host bridge exposes an MSI-X capability but
> > doesn't actually support it.
> > This causes a crash during initialization by the pcieport driver,
> > since
> > it tries to configure the MSI-X capability.
> 
> Nit: The formatting above is jarring to read because I can't tell
> whether it's one paragraph or two.
> 
> Either rewrap it into a single paragraph or add a blank line to make
> two paragraphs.  I noticed this elsewhere, too, in a comment, I
> think.
> 
Ack.

> s/host bridge/Root Port/, if I understand correctly.
> 
Ack.

BTW, what is the main difference between the 2 terms, since they seem
to be (mistakenly?) used interchangeably?

> I don't understand the "on some platforms..." part.  Do you mean that
> on *every* platform, this particular host bridge (identified by
> [1c36:0031]) advertises an MSI-X capability that doesn't work?
> 
> Or are there some platforms that configure the bridge so it doesn't
> advertise MSI-X at all, while other platforms configure it so it
> *does* advertise MSI-X?
> 
The MSI-x capability isn't supported for this specific host bridge
([1c36:0031]). On some platforms, it is configured to not advertise the
capability at all, while on others it (mistakenly) does advertise it.

I've updated the commit message to be more explicit.

> If there's a line or two of diagnostics from the crash you could
> include here, that would help people who encounter the crash find
> the solution.
> 
Sure, I'll add a partial stacktrace (a bit more than a couple of lines,
but I feel it will be too ambiguous otherwise).

> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> >  drivers/pci/quirks.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 11850b030637..0fb70d755977 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -2925,6 +2925,14 @@
> > 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);
> > +
> > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > +{
> > +	dev->no_msi = 1;
> > +	dev_warn(&dev->dev, "Annapurna Labs pcie quirk - disabling
> > MSI\n");
> 
> s/pcie/PCIe/ in English text, comments, printk strings, etc.
> 
Ack.

> Actually, I think the whole "Annapurna Labs pcie quirk" part is
> probably unnecessary, since we can identify the device via the
> dev_printk() info.
> 
Ack.

> Speaking of which, you can use "pci_warn(dev)" here to be consistent
> with the rest of the file.
> 
Ack.

> > +}
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS,
> > 0x0031,
> > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > quirk_al_msi_disable);
> 
> Why do you use the class fixup here instead of the simpler
> DECLARE_PCI_FIXUP_FINAL()?  Requiring the class to match
> PCI_CLASS_BRIDGE_PCI suggests that there may be other [1c36:0031]
> devices that are not Root Ports.  If that's the case, please mention
> it so it's clear why we need DECLARE_PCI_FIXUP_CLASS_FINAL().  If
> not,
> just use DECLARE_PCI_FIXUP_FINAL().
> 
This is indeed the case. What do you say about adding the following
comment before the function's definition:
/*
 * 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
only.
 */

>  #endif /* CONFIG_PCI_MSI */
 
 /*
-- 
2.17.1

Re: [PATCH 4/8] PCI: Add quirk to disable MSI support for Amazon's Annapurna Labs host bridge

[Benjamin Herrenschmidt]

On Sun, 2019-07-14 at 15:09 +0000, Chocron, Jonathan wrote:
> > s/host bridge/Root Port/, if I understand correctly.
> > 
> 
> Ack.
> 
> BTW, what is the main difference between the 2 terms, since they seem
> to be (mistakenly?) used interchangeably?

The host bridge is the parent of the root port. You can have several
root ports under a host bridge in fact. They tend to be part of the
same silicon and somewhat intimately linked but they are distinct
logical entities. The root port appears as a PCIe p2p bridge sitting on
the top level bus provided by the host bridge. The Host Bridge doesn't
have to have a representation in config space (it sometimes does
historically, but as a sibling of the devices on that top level bus. In
PCIe land, these are chipset built-in devices).

Ben.

Re: [PATCH 6/8] PCI: al: Add support for DW based driver type

[Chocron, Jonathan]

On Fri, 2019-07-12 at 08:42 -0500, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2019 at 05:57:05PM +0300, Jonathan Chocron wrote:
> > 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 <jonnyc@amazon.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig   |  11 +
> >  drivers/pci/controller/dwc/pcie-al.c | 397
> > +++++++++++++++++++++++++++
> >  2 files changed, 408 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index a6ce1ee51b4c..51da9cb219aa 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -230,4 +230,15 @@ 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 Annapurna Labs PCIe
> > controller IP
> > +	  on Amazon SoCs.
> > +	  The PCIe controller uses the DesignWare core plus
> > +	  Amazon-specific hardware wrappers.
> 
> Is this one paragraph or two?
> 
This was originally a single paragraph, but I hacked it a bit to pass
the checkpatch minimum 4 line description requirement :)

> This should mention the fact that only DT platforms need
> CONFIG_PCIE_AL.  ACPI platforms with the Annapurna Labs PCIe
> controller don't need this.
> 
No problem, will add. The reason I didn't explicitly state this is
because I didn't see it mentioned in PCIE_HISI, which has similar
support of both ACPI and DT-based drivers.
> > +
> >  endmenu
> > diff --git a/drivers/pci/controller/dwc/pcie-al.c
> > b/drivers/pci/controller/dwc/pcie-al.c
> > index 3ab58f0584a8..2742a0895aab 100644
> > --- a/drivers/pci/controller/dwc/pcie-al.c
> > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > @@ -91,3 +91,400 @@ 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
> > +
> > +/* The AXI regs are always at the beginning of the PCIE controller
> > reg space. */
> > +#define AXI_BASE_OFFSET		0x0
> > +
> > +#define DEVICE_ID_OFFSET	0x16c
> > +
> > +#define DEVICE_REV_ID			0x0
> > +#define DEVICE_REV_ID_DEV_ID_MASK	0xFFFF0000
> > +#define DEVICE_REV_ID_DEV_ID_SHIFT	16
> > +
> > +#define DEVICE_REV_ID_DEV_ID_X4		(0 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +#define DEVICE_REV_ID_DEV_ID_X8		(2 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +#define DEVICE_REV_ID_DEV_ID_X16	(4 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +
> > +/* The ob_ctrl offset inside the axi regs is different between
> > revisions.
> > + */
> 
> s/axi/AXI/ to be consistent.
> 
Ack.

> Fix comment style (either single-line with "/* ... */" or multi-line
> with "/*" on first line and "*/" on last line).
> 
Ack. I'd expect checkpatch to catch that, no?

> > +#define OB_CTRL_REV1_2_OFFSET	0x0040
> > +#define OB_CTRL_REV3_5_OFFSET	0x0030
> > +
> > +#define CFG_TARGET_BUS			0x0
> > +#define CFG_TARGET_BUS_MASK_SHIFT	0
> > +#define CFG_TARGET_BUS_BUSNUM_SHIFT	8
> > +
> > +#define CFG_CONTROL			0x4
> > +#define CFG_CONTROL_SUBBUS_MASK		0x0000FF00
> > +#define CFG_CONTROL_SUBBUS_SHIFT	8
> > +#define CFG_CONTROL_SEC_BUS_MASK	0x00FF0000
> > +#define CFG_CONTROL_SEC_BUS_SHIFT	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) */
> > +	void __iomem	*dbi_base;
> 
> I *guess* this is different from the dw_pcie.dbi_base?

Removed (leftover from an early revision).
> 
> > +	resource_size_t ecam_size;
> > +	struct device *dev;
> > +	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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > *rev_id)
> 
> I think al_pcie_rev_id_get() and al_pcie_reg_offsets_set() are more
> complicated than necessary, at least for the current code.  All
> you're
> really using is something like this:
> 
>   static void al_pcie_rev_id_get(...)
>   {
>     ...
>     switch (dev_rev_id) {
>     case DEVICE_REV_ID_DEV_ID_X4:
>       pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
>       break;
>     case DEVICE_REV_ID_DEV_ID_X8:
>     case DEVICE_REV_ID_DEV_ID_X16:
>       pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
>       break;
>     default:
>       dev_err("unsupported rev_id\n");
>       break;
>     }
>   }
> 
I understand, this was more of a future preparation, in case a
configuration would be different for a future HW revision. So you think
I should entirely remove the controller_rev_id from the al_pcie
context, or simply unify the logic into a single
"al_pcie_rev_id_setup()" func?

> > +{
> > +	void __iomem *dev_rev_id_addr;
> > +	u32 dev_rev_id;
> > +
> > +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > DEVICE_REV_ID);
> > +
> > +	dev_rev_id = readl(dev_rev_id_addr) &
> > DEVICE_REV_ID_DEV_ID_MASK;
> > +
> > +	switch (dev_rev_id) {
> > +	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_rev_id (0x%08x)\n",
> > +			dev_rev_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(pcie->dev, "dev_rev_id: 0x%08x\n", dev_rev_id);
> > +
> > +	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)
> > +{
> > +	void __iomem *cfg_control_addr;
> > +	u32 reg = (target_bus << CFG_TARGET_BUS_BUSNUM_SHIFT) |
> > +		       mask_target_bus;
> > +
> > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > +			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > +
> > +			   CFG_TARGET_BUS);
> > +
> > +	writel(reg, cfg_control_addr);
> > +}
> > +
> > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > +					   unsigned int busnr,
> > +					   unsigned int devfn)
> > +{
> > +	void __iomem *pci_base_addr;
> > +	struct pcie_port *pp = &pcie->pci->pp;
> > +	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;
> > +
> > +	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 %d to %d\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)
> 
> Rewrap to use fewer lines.
> 
Done.

> > +{
> > +	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 %#x (pci_addr: 0x%p) - val:0x%x\n",

Do you think I should change the %p here to %px?

> > +		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 %#x (pci_addr: 0x%p) - 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_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;
> > +	u8 secondary_bus;
> > +	u8 subordinate_bus;
> > +	void __iomem *cfg_control_addr;
> > +	u32 cfg_control;
> > +	u32 reg = 0;
> 
> Why is this initialized?  It doesn't look like it can be used before
> being set.

Removed initialization.

> 
> > +	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_addr = pcie->controller_base +
> > +		AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
> > CFG_CONTROL;
> > +
> > +	cfg_control = readl(cfg_control_addr);
> > +
> > +	reg = (cfg_control & ~CFG_CONTROL_SEC_BUS_MASK) |
> > +	       (secondary_bus << CFG_CONTROL_SEC_BUS_SHIFT);
> > +	reg = (reg & ~CFG_CONTROL_SUBBUS_MASK) |
> > +	       (subordinate_bus << CFG_CONTROL_SUBBUS_SHIFT);
> > +
> > +	writel(reg, cfg_control_addr);
> > +
> > +	return 0;
> > +}
> > +
> > +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 = 0;
> > +	u32 reg = 0;
> 
> Why initialize link_up and reg?
> 
Removed initialization.

> > +	int rc;
> > +
> > +	reg = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> > +	if (reg != PCI_HEADER_TYPE_BRIDGE) {
> > +		dev_err(pci->dev, "PCIe controller is not set to bridge
> > type (%d)!\n",
> > +			reg);
> > +		return -EIO;
> > +	}
> > +

Removing this validation as it is done generically in PATCH 7/8.

> > +	link_up = dw_pcie_link_up(pci);
> > +	if (!link_up) {
> > +		dev_err(pci->dev, "link in not up!\n");
> 
> s/in/is/

Ack.

> 
> > +		return -ENOLINK;
> > +	}
> > +
> > +	dev_info(pci->dev, "link is up\n");
> > +
> > +	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;
> > +
> > +	rc = al_pcie_config_prepare(pcie);
> > +	if (rc)
> > +		return rc;
> > +
> > +	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 al_pcie *al_pcie;
> > +	struct dw_pcie *pci;
> > +	struct resource *dbi_res;
> > +	struct resource *controller_res;
> > +	struct resource *ecam_res;
> > +	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 %p\n", dbi_res);
> 
> %pR (see below).
> 
Fixed.

> > +		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
> > );
> 
> Several of these function calls would fit on one line, or at least
> fewer than they currently use.
> 
Fixed.

> > +	if (IS_ERR(al_pcie->controller_base)) {
> > +		dev_err(dev, "couldn't remap controller base %p\n",
> > +			controller_res);
> 
> Use %pR.  I think this %p prints the address of the struct resource,
> not the address you tried to map.
> 
Fixed.

> > +		return PTR_ERR(al_pcie->controller_base);
> > +	}
> > +
> > +	dev_dbg(dev, "From DT: dbi_base: 0x%llx, controller_base:
> > 0x%llx\n",
> > +		dbi_res->start, controller_res->start);
> 
> Use %pR instead of printing dbi_res->start, controller_res->start.
> 
Done.

> > +
> > +	platform_set_drvdata(pdev, al_pcie);
> > +
> > +	ret = al_add_pcie_port(&pci->pp, pdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id al_pcie_of_match[] = {
> > +	{ .compatible = "amazon,al-pcie",
> > +	  .data = NULL,
> 
> ".data = NULL" is unnecessary.
> 
Done.

> > +	},
> > +	{},
> > +};
> > +
> > +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*/
> > -- 
> > 2.17.1
> >