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

[PATCH v3 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)?

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 (7):
  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: 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                                   |   3 +-
 drivers/pci/controller/dwc/Kconfig            |  12 +
 drivers/pci/controller/dwc/pcie-al.c          | 367 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   8 +
 .../pci/controller/dwc/pcie-designware-host.c |  31 +-
 drivers/pci/quirks.c                          |  34 ++
 drivers/pci/vpd.c                             |  16 +
 include/linux/pci_ids.h                       |   2 +
 9 files changed, 513 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

-- 
2.17.1

[PATCH v3 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>
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 40015609c4b5..63dfa4bace57 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2569,6 +2569,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 v3 1/8] PCI: Add Amazon's Annapurna Labs vendor ID

[Andrew Murray]

On Tue, Jul 23, 2019 at 12:25:26PM +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 40015609c4b5..63dfa4bace57 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2569,6 +2569,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
>  

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

> -- 
> 2.17.1
> 

[PATCH v3 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>
Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/quirks.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 208aacf39329..23672680dba7 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 v3 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port

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

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>
Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/vpd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 4963c2e2bd4c..c23a8ec08db9 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -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
-- 
2.17.1

[PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Jonathan Chocron]

The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On some
platforms it is configured to not advertise the capability at all, while
on others it (mistakenly) does. This causes a panic during
initialization by the pcieport driver, since it tries to configure the
MSI-X capability. Specifically, when trying to access the MSI-X table
a "non-existing addr" exception occurs.

Example stacktrace snippet:

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

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 23672680dba7..11f843aa96b3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2925,6 +2925,21 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
 			quirk_msi_intx_disable_qca_bug);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
 			quirk_msi_intx_disable_qca_bug);
+
+/*
+ * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-X, so it
+ * should be disabled on platforms where the device (mistakenly) advertises it.
+ *
+ * The 0031 device id is reused for other non Root Port device types,
+ * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI class.
+ */
+static void quirk_al_msi_disable(struct pci_dev *dev)
+{
+	dev->no_msi = 1;
+	pci_warn(dev, "Disabling 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 */
 
 /*
-- 
2.17.1

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Andrew Murray]

On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron wrote:
> The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On some

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


> platforms it is configured to not advertise the capability at all, while
> on others it (mistakenly) does. This causes a panic during
> initialization by the pcieport driver, since it tries to configure the
> MSI-X capability. Specifically, when trying to access the MSI-X table
> a "non-existing addr" exception occurs.
> 
> Example stacktrace snippet:
> 
> [    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
> [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-Jonny-14847-ge76f1d4a1828-dirty #33
> [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> [    1.632367] sp : ffffff80117db700
> [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> [    1.632392] Kernel panic - not syncing: Asynchronous SError Interrupt
> [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-Jonny-14847-ge76f1d4a1828-dirty #33
> [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> [    1.632394] Call trace:
> [    1.632395]  dump_backtrace+0x0/0x140
> [    1.632395]  show_stack+0x14/0x20
> [    1.632396]  dump_stack+0xa8/0xcc
> [    1.632396]  panic+0x140/0x334
> [    1.632397]  nmi_panic+0x6c/0x70
> [    1.632398]  arm64_serror_panic+0x74/0x88
> [    1.632398]  __pte_error+0x0/0x28
> [    1.632399]  el1_error+0x84/0xf8
> [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 23672680dba7..11f843aa96b3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2925,6 +2925,21 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
>  			quirk_msi_intx_disable_qca_bug);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
>  			quirk_msi_intx_disable_qca_bug);
> +
> +/*
> + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-X, so it
> + * should be disabled on platforms where the device (mistakenly) advertises it.
> + *
> + * The 0031 device id is reused for other non Root Port device types,
> + * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI class.
> + */
> +static void quirk_al_msi_disable(struct pci_dev *dev)
> +{
> +	dev->no_msi = 1;
> +	pci_warn(dev, "Disabling MSI-X\n");

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

Thanks,

Andrew Murray

> +}
> +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
> 

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Chocron, Jonathan]

On Mon, 2019-08-19 at 19:23 +0100, Andrew Murray wrote:
> On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron wrote:
> > The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On
> > some
> 
> Shouldn't this read [1c36:0031]?
> 
Indeed. Thanks for catching this.

> 
> > platforms it is configured to not advertise the capability at all,
> > while
> > on others it (mistakenly) does. This causes a panic during
> > initialization by the pcieport driver, since it tries to configure
> > the
> > MSI-X capability. Specifically, when trying to access the MSI-X
> > table
> > a "non-existing addr" exception occurs.
> > 
> > Example stacktrace snippet:
> > 
> > [    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
> > [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
> > Jonny-14847-ge76f1d4a1828-dirty #33
> > [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> > [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> > [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> > [    1.632367] sp : ffffff80117db700
> > [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> > [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> > [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> > [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> > [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> > [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> > [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> > [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> > [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> > [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> > [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> > [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> > [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> > [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> > [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> > [    1.632392] Kernel panic - not syncing: Asynchronous SError
> > Interrupt
> > [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
> > Jonny-14847-ge76f1d4a1828-dirty #33
> > [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > [    1.632394] Call trace:
> > [    1.632395]  dump_backtrace+0x0/0x140
> > [    1.632395]  show_stack+0x14/0x20
> > [    1.632396]  dump_stack+0xa8/0xcc
> > [    1.632396]  panic+0x140/0x334
> > [    1.632397]  nmi_panic+0x6c/0x70
> > [    1.632398]  arm64_serror_panic+0x74/0x88
> > [    1.632398]  __pte_error+0x0/0x28
> > [    1.632399]  el1_error+0x84/0xf8
> > [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> > [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> > [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> > [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> > 
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> >  drivers/pci/quirks.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 23672680dba7..11f843aa96b3 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -2925,6 +2925,21 @@
> > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
> >  			quirk_msi_intx_disable_qca_bug);
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
> >  			quirk_msi_intx_disable_qca_bug);
> > +
> > +/*
> > + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-
> > X, so it
> > + * should be disabled on platforms where the device (mistakenly)
> > advertises it.
> > + *
> > + * The 0031 device id is reused for other non Root Port device
> > types,
> > + * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI
> > class.
> > + */
> > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > +{
> > +	dev->no_msi = 1;
> > +	pci_warn(dev, "Disabling MSI-X\n");
> 
> This will disable both MSI and MSI-X support - is this really the
> intention
> here? Do the root ports support MSI and legacy, or just legacy?
> 
The HW should support MSI, but we currently don't have a use case for
it so it hasn't been tested and therefore we are okay with disabling
it.

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

> Thanks,
> 
> Andrew Murray
> 
> > +}
> > +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
> > 

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Andrew Murray]

On Tue, Aug 20, 2019 at 02:52:30PM +0000, Chocron, Jonathan wrote:
> On Mon, 2019-08-19 at 19:23 +0100, Andrew Murray wrote:
> > On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron wrote:
> > > The Root Port (identified by [1c36:0032]) doesn't support MSI-X. On
> > > some
> > 
> > Shouldn't this read [1c36:0031]?
> > 
> Indeed. Thanks for catching this.
> 
> > 
> > > platforms it is configured to not advertise the capability at all,
> > > while
> > > on others it (mistakenly) does. This causes a panic during
> > > initialization by the pcieport driver, since it tries to configure
> > > the
> > > MSI-X capability. Specifically, when trying to access the MSI-X
> > > table
> > > a "non-existing addr" exception occurs.
> > > 
> > > Example stacktrace snippet:
> > > 
> > > [    1.632363] SError Interrupt on CPU2, code 0xbf000000 -- SError
> > > [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
> > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> > > [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> > > [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> > > [    1.632367] sp : ffffff80117db700
> > > [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> > > [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> > > [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> > > [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> > > [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> > > [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> > > [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> > > [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> > > [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> > > [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> > > [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> > > [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> > > [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> > > [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> > > [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> > > [    1.632392] Kernel panic - not syncing: Asynchronous SError
> > > Interrupt
> > > [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc1-
> > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > [    1.632394] Call trace:
> > > [    1.632395]  dump_backtrace+0x0/0x140
> > > [    1.632395]  show_stack+0x14/0x20
> > > [    1.632396]  dump_stack+0xa8/0xcc
> > > [    1.632396]  panic+0x140/0x334
> > > [    1.632397]  nmi_panic+0x6c/0x70
> > > [    1.632398]  arm64_serror_panic+0x74/0x88
> > > [    1.632398]  __pte_error+0x0/0x28
> > > [    1.632399]  el1_error+0x84/0xf8
> > > [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> > > [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> > > [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> > > [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> > > 
> > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > ---
> > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index 23672680dba7..11f843aa96b3 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -2925,6 +2925,21 @@
> > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
> > >  			quirk_msi_intx_disable_qca_bug);
> > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
> > >  			quirk_msi_intx_disable_qca_bug);
> > > +
> > > +/*
> > > + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support MSI-
> > > X, so it
> > > + * should be disabled on platforms where the device (mistakenly)
> > > advertises it.
> > > + *
> > > + * The 0031 device id is reused for other non Root Port device
> > > types,
> > > + * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI
> > > class.
> > > + */
> > > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > > +{
> > > +	dev->no_msi = 1;
> > > +	pci_warn(dev, "Disabling MSI-X\n");
> > 
> > This will disable both MSI and MSI-X support - is this really the
> > intention
> > here? Do the root ports support MSI and legacy, or just legacy?
> > 
> The HW should support MSI, but we currently don't have a use case for
> it so it hasn't been tested and therefore we are okay with disabling
> it.

OK - then the commit message and comment (for quirk_al_msi_disable) should
probably be updated to reflect this. Especially given that the device id
may be used on other device types - implying that a use-case for this
may later arise.

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

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

Setting dev->msix_enabled to false (i.e. through pci_disable_msix) would
result in an un-necessary WARN_ON_ONCE. I think you'd need to ensure
devi->msix_cap is NULL (which makes sense as your hardware shouldn't be
exposing this capability).

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

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

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

Thanks,

Andrew Murray

> 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > > +}
> > > +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
> > > 

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Chocron, Jonathan]

On Tue, 2019-08-20 at 16:25 +0100, Andrew Murray wrote:
> On Tue, Aug 20, 2019 at 02:52:30PM +0000, Chocron, Jonathan wrote:
> > On Mon, 2019-08-19 at 19:23 +0100, Andrew Murray wrote:
> > > On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron wrote:
> > > > The Root Port (identified by [1c36:0032]) doesn't support MSI-
> > > > X. On
> > > > some
> > > 
> > > Shouldn't this read [1c36:0031]?
> > > 
> > 
> > Indeed. Thanks for catching this.
> > 
> > > 
> > > > platforms it is configured to not advertise the capability at
> > > > all,
> > > > while
> > > > on others it (mistakenly) does. This causes a panic during
> > > > initialization by the pcieport driver, since it tries to
> > > > configure
> > > > the
> > > > MSI-X capability. Specifically, when trying to access the MSI-X
> > > > table
> > > > a "non-existing addr" exception occurs.
> > > > 
> > > > Example stacktrace snippet:
> > > > 
> > > > [    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
> > > > SError
> > > > [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
> > > > rc1-
> > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > > [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> > > > [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> > > > [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> > > > [    1.632367] sp : ffffff80117db700
> > > > [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> > > > [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> > > > [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> > > > [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> > > > [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> > > > [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> > > > [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> > > > [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> > > > [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> > > > [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> > > > [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> > > > [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> > > > [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> > > > [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> > > > [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> > > > [    1.632392] Kernel panic - not syncing: Asynchronous SError
> > > > Interrupt
> > > > [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
> > > > rc1-
> > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > > [    1.632394] Call trace:
> > > > [    1.632395]  dump_backtrace+0x0/0x140
> > > > [    1.632395]  show_stack+0x14/0x20
> > > > [    1.632396]  dump_stack+0xa8/0xcc
> > > > [    1.632396]  panic+0x140/0x334
> > > > [    1.632397]  nmi_panic+0x6c/0x70
> > > > [    1.632398]  arm64_serror_panic+0x74/0x88
> > > > [    1.632398]  __pte_error+0x0/0x28
> > > > [    1.632399]  el1_error+0x84/0xf8
> > > > [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> > > > [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> > > > [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> > > > [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> > > > 
> > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > > ---
> > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > index 23672680dba7..11f843aa96b3 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -2925,6 +2925,21 @@
> > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
> > > >  			quirk_msi_intx_disable_qca_bug);
> > > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
> > > >  			quirk_msi_intx_disable_qca_bug);
> > > > +
> > > > +/*
> > > > + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support
> > > > MSI-
> > > > X, so it
> > > > + * should be disabled on platforms where the device
> > > > (mistakenly)
> > > > advertises it.
> > > > + *
> > > > + * The 0031 device id is reused for other non Root Port device
> > > > types,
> > > > + * therefore the quirk is registered for the
> > > > PCI_CLASS_BRIDGE_PCI
> > > > class.
> > > > + */
> > > > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > > > +{
> > > > +	dev->no_msi = 1;
> > > > +	pci_warn(dev, "Disabling MSI-X\n");
> > > 
> > > This will disable both MSI and MSI-X support - is this really the
> > > intention
> > > here? Do the root ports support MSI and legacy, or just legacy?
> > > 
> > 
> > The HW should support MSI, but we currently don't have a use case
> > for
> > it so it hasn't been tested and therefore we are okay with
> > disabling
> > it.
> 
> OK - then the commit message and comment (for quirk_al_msi_disable)
> should
> probably be updated to reflect this. 

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

> Especially given that the device id
> may be used on other device types - implying that a use-case for this
> may later arise.
> 
Not sure I understood that last line. This patch is relevant only for
the BRIDGE class 0x0031 device. The other device types, which (sadly)
happen to re-use the dev_id, have are totally different and shouldn't
be mixed in here.

> > 
> > For future knowledge, how can just MSI-X be disabled?
> > I see that in pcie_port_enable_irq_vec(), the pcieport driver calls
> > pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And
> > internally, both __pci_enable_msix_range() and
> > __pci_enable_msi_range()
> > use pci_msi_supported() which doesn't differentiate between MSI and
> > MSI-x.
> 
> The documentation [1] would suggest that once upon a time
> pci_disable_msix
> was used - but now should let pci_alloc_irq_vectors cap the max
> number
> of vectors. However in your case it's the PCIe port driver that is
> attempting
> to allocate MSI-X's and so the solution isn't an obvious one.
> 
> Setting dev->msix_enabled to false (i.e. through pci_disable_msix)
> would
> result in an un-necessary WARN_ON_ONCE. 

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

> I think you'd need to ensure
> devi->msix_cap is NULL (which makes sense as your hardware shouldn't
> be
> exposing this capability).
> 
> I guess the right way of achieving this would be through a quirk,
> though you'd
> be the first to do this and you'd have to ensure the quirk runs
> before
> anyone tests for msix_cap.
> 
> That's my view, though others may have different suggestions.
> 
I think that maybe a dev->no_msix field should be added and then a
corresponding pci_msix_supported() function as well. I'd be glad to
take it offline or on a dedicated thread, but it shouldn't block this
patchset.

> [1] Documentation/PCI/msi-howto.rst
> 
> Thanks,
> 
> Andrew Murray
> 
> > 
> > > Thanks,
> > > 
> > > Andrew Murray
> > > 
> > > > +}
> > > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L
> > > > ABS,
> > > > 0x0031,
> > > > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > > > quirk_al_msi_disable);
> > > >  #endif /* CONFIG_PCI_MSI */
> > > >  
> > > >  /*
> > > > -- 
> > > > 2.17.1
> > > > 

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Andrew Murray]

On Tue, Aug 20, 2019 at 05:06:22PM +0000, Chocron, Jonathan wrote:
> On Tue, 2019-08-20 at 16:25 +0100, Andrew Murray wrote:
> > On Tue, Aug 20, 2019 at 02:52:30PM +0000, Chocron, Jonathan wrote:
> > > On Mon, 2019-08-19 at 19:23 +0100, Andrew Murray wrote:
> > > > On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron wrote:
> > > > > The Root Port (identified by [1c36:0032]) doesn't support MSI-
> > > > > X. On
> > > > > some
> > > > 
> > > > Shouldn't this read [1c36:0031]?
> > > > 
> > > 
> > > Indeed. Thanks for catching this.
> > > 
> > > > 
> > > > > platforms it is configured to not advertise the capability at
> > > > > all,
> > > > > while
> > > > > on others it (mistakenly) does. This causes a panic during
> > > > > initialization by the pcieport driver, since it tries to
> > > > > configure
> > > > > the
> > > > > MSI-X capability. Specifically, when trying to access the MSI-X
> > > > > table
> > > > > a "non-existing addr" exception occurs.
> > > > > 
> > > > > Example stacktrace snippet:
> > > > > 
> > > > > [    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
> > > > > SError
> > > > > [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
> > > > > rc1-
> > > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > > [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > > > [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> > > > > [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> > > > > [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> > > > > [    1.632367] sp : ffffff80117db700
> > > > > [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> > > > > [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> > > > > [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> > > > > [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> > > > > [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> > > > > [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> > > > > [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> > > > > [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> > > > > [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> > > > > [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> > > > > [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> > > > > [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> > > > > [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> > > > > [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> > > > > [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> > > > > [    1.632392] Kernel panic - not syncing: Asynchronous SError
> > > > > Interrupt
> > > > > [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.2.0-
> > > > > rc1-
> > > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > > [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP (DT)
> > > > > [    1.632394] Call trace:
> > > > > [    1.632395]  dump_backtrace+0x0/0x140
> > > > > [    1.632395]  show_stack+0x14/0x20
> > > > > [    1.632396]  dump_stack+0xa8/0xcc
> > > > > [    1.632396]  panic+0x140/0x334
> > > > > [    1.632397]  nmi_panic+0x6c/0x70
> > > > > [    1.632398]  arm64_serror_panic+0x74/0x88
> > > > > [    1.632398]  __pte_error+0x0/0x28
> > > > > [    1.632399]  el1_error+0x84/0xf8
> > > > > [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> > > > > [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> > > > > [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> > > > > [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> > > > > 
> > > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > > > Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > > > > ---
> > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > >  1 file changed, 15 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > > index 23672680dba7..11f843aa96b3 100644
> > > > > --- a/drivers/pci/quirks.c
> > > > > +++ b/drivers/pci/quirks.c
> > > > > @@ -2925,6 +2925,21 @@
> > > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
> > > > >  			quirk_msi_intx_disable_qca_bug);
> > > > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
> > > > >  			quirk_msi_intx_disable_qca_bug);
> > > > > +
> > > > > +/*
> > > > > + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't support
> > > > > MSI-
> > > > > X, so it
> > > > > + * should be disabled on platforms where the device
> > > > > (mistakenly)
> > > > > advertises it.
> > > > > + *
> > > > > + * The 0031 device id is reused for other non Root Port device
> > > > > types,
> > > > > + * therefore the quirk is registered for the
> > > > > PCI_CLASS_BRIDGE_PCI
> > > > > class.
> > > > > + */
> > > > > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > > > > +{
> > > > > +	dev->no_msi = 1;
> > > > > +	pci_warn(dev, "Disabling MSI-X\n");
> > > > 
> > > > This will disable both MSI and MSI-X support - is this really the
> > > > intention
> > > > here? Do the root ports support MSI and legacy, or just legacy?
> > > > 
> > > 
> > > The HW should support MSI, but we currently don't have a use case
> > > for
> > > it so it hasn't been tested and therefore we are okay with
> > > disabling
> > > it.
> > 
> > OK - then the commit message and comment (for quirk_al_msi_disable)
> > should
> > probably be updated to reflect this. 
> 
> If you mean that we should explicitly state that MSI is possibly
> supported then I don't think it is very relevant here. If we decide to
> test and enable it in the future, then the new quirk (which would only
> disable MSI-x) would reflect the fact that MSI works. Sounds ok?
> 

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

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

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

> > Especially given that the device id
> > may be used on other device types - implying that a use-case for this
> > may later arise.
> > 
> Not sure I understood that last line. This patch is relevant only for
> the BRIDGE class 0x0031 device. The other device types, which (sadly)
> happen to re-use the dev_id, have are totally different and shouldn't
> be mixed in here.
> 

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

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

> > > 
> > > For future knowledge, how can just MSI-X be disabled?
> > > I see that in pcie_port_enable_irq_vec(), the pcieport driver calls
> > > pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And
> > > internally, both __pci_enable_msix_range() and
> > > __pci_enable_msi_range()
> > > use pci_msi_supported() which doesn't differentiate between MSI and
> > > MSI-x.
> > 
> > The documentation [1] would suggest that once upon a time
> > pci_disable_msix
> > was used - but now should let pci_alloc_irq_vectors cap the max
> > number
> > of vectors. However in your case it's the PCIe port driver that is
> > attempting
> > to allocate MSI-X's and so the solution isn't an obvious one.
> > 
> > Setting dev->msix_enabled to false (i.e. through pci_disable_msix)
> > would
> > result in an un-necessary WARN_ON_ONCE. 
> 
> Per my understanding, it seems that dev->msix_enabled indicates if the
> MSI-X capability has already been enabled or not, and not as an
> indication if it is supported by the device. If that is the case, then
> not sure pci_disable_msix() would result in the desired effect.

Yes this isn't the right approach.

> 
> > I think you'd need to ensure
> > devi->msix_cap is NULL (which makes sense as your hardware shouldn't
> > be
> > exposing this capability).
> > 
> > I guess the right way of achieving this would be through a quirk,
> > though you'd
> > be the first to do this and you'd have to ensure the quirk runs
> > before
> > anyone tests for msix_cap.
> > 
> > That's my view, though others may have different suggestions.
> > 
> I think that maybe a dev->no_msix field should be added and then a
> corresponding pci_msix_supported() function as well. I'd be glad to
> take it offline or on a dedicated thread, but it shouldn't block this
> patchset.

Thanks,

Andrew Murray

> 
> > [1] Documentation/PCI/msi-howto.rst
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > > 
> > > > Thanks,
> > > > 
> > > > Andrew Murray
> > > > 
> > > > > +}
> > > > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_L
> > > > > ABS,
> > > > > 0x0031,
> > > > > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > > > > quirk_al_msi_disable);
> > > > >  #endif /* CONFIG_PCI_MSI */
> > > > >  
> > > > >  /*
> > > > > -- 
> > > > > 2.17.1
> > > > > 

Re: [PATCH v3 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port

[Chocron, Jonathan]

On Tue, 2019-08-20 at 20:54 +0100, Andrew Murray wrote:
> On Tue, Aug 20, 2019 at 05:06:22PM +0000, Chocron, Jonathan wrote:
> > On Tue, 2019-08-20 at 16:25 +0100, Andrew Murray wrote:
> > > On Tue, Aug 20, 2019 at 02:52:30PM +0000, Chocron, Jonathan
> > > wrote:
> > > > On Mon, 2019-08-19 at 19:23 +0100, Andrew Murray wrote:
> > > > > On Tue, Jul 23, 2019 at 12:25:29PM +0300, Jonathan Chocron
> > > > > wrote:
> > > > > > The Root Port (identified by [1c36:0032]) doesn't support
> > > > > > MSI-
> > > > > > X. On
> > > > > > some
> > > > > 
> > > > > Shouldn't this read [1c36:0031]?
> > > > > 
> > > > 
> > > > Indeed. Thanks for catching this.
> > > > 
> > > > > 
> > > > > > platforms it is configured to not advertise the capability
> > > > > > at
> > > > > > all,
> > > > > > while
> > > > > > on others it (mistakenly) does. This causes a panic during
> > > > > > initialization by the pcieport driver, since it tries to
> > > > > > configure
> > > > > > the
> > > > > > MSI-X capability. Specifically, when trying to access the
> > > > > > MSI-X
> > > > > > table
> > > > > > a "non-existing addr" exception occurs.
> > > > > > 
> > > > > > Example stacktrace snippet:
> > > > > > 
> > > > > > [    1.632363] SError Interrupt on CPU2, code 0xbf000000 --
> > > > > > SError
> > > > > > [    1.632364] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> > > > > > 5.2.0-
> > > > > > rc1-
> > > > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > > > [    1.632365] Hardware name: Annapurna Labs Alpine V3 EVP
> > > > > > (DT)
> > > > > > [    1.632365] pstate: 80000005 (Nzcv daif -PAN -UAO)
> > > > > > [    1.632366] pc : __pci_enable_msix_range+0x4e4/0x608
> > > > > > [    1.632367] lr : __pci_enable_msix_range+0x498/0x608
> > > > > > [    1.632367] sp : ffffff80117db700
> > > > > > [    1.632368] x29: ffffff80117db700 x28: 0000000000000001
> > > > > > [    1.632370] x27: 0000000000000001 x26: 0000000000000000
> > > > > > [    1.632372] x25: ffffffd3e9d8c0b0 x24: 0000000000000000
> > > > > > [    1.632373] x23: 0000000000000000 x22: 0000000000000000
> > > > > > [    1.632375] x21: 0000000000000001 x20: 0000000000000000
> > > > > > [    1.632376] x19: ffffffd3e9d8c000 x18: ffffffffffffffff
> > > > > > [    1.632378] x17: 0000000000000000 x16: 0000000000000000
> > > > > > [    1.632379] x15: ffffff80116496c8 x14: ffffffd3e9844503
> > > > > > [    1.632380] x13: ffffffd3e9844502 x12: 0000000000000038
> > > > > > [    1.632382] x11: ffffffffffffff00 x10: 0000000000000040
> > > > > > [    1.632384] x9 : ffffff801165e270 x8 : ffffff801165e268
> > > > > > [    1.632385] x7 : 0000000000000002 x6 : 00000000000000b2
> > > > > > [    1.632387] x5 : ffffffd3e9d8c2c0 x4 : 0000000000000000
> > > > > > [    1.632388] x3 : 0000000000000000 x2 : 0000000000000000
> > > > > > [    1.632390] x1 : 0000000000000000 x0 : ffffffd3e9844680
> > > > > > [    1.632392] Kernel panic - not syncing: Asynchronous
> > > > > > SError
> > > > > > Interrupt
> > > > > > [    1.632393] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> > > > > > 5.2.0-
> > > > > > rc1-
> > > > > > Jonny-14847-ge76f1d4a1828-dirty #33
> > > > > > [    1.632394] Hardware name: Annapurna Labs Alpine V3 EVP
> > > > > > (DT)
> > > > > > [    1.632394] Call trace:
> > > > > > [    1.632395]  dump_backtrace+0x0/0x140
> > > > > > [    1.632395]  show_stack+0x14/0x20
> > > > > > [    1.632396]  dump_stack+0xa8/0xcc
> > > > > > [    1.632396]  panic+0x140/0x334
> > > > > > [    1.632397]  nmi_panic+0x6c/0x70
> > > > > > [    1.632398]  arm64_serror_panic+0x74/0x88
> > > > > > [    1.632398]  __pte_error+0x0/0x28
> > > > > > [    1.632399]  el1_error+0x84/0xf8
> > > > > > [    1.632400]  __pci_enable_msix_range+0x4e4/0x608
> > > > > > [    1.632400]  pci_alloc_irq_vectors_affinity+0xdc/0x150
> > > > > > [    1.632401]  pcie_port_device_register+0x2b8/0x4e0
> > > > > > [    1.632402]  pcie_portdrv_probe+0x34/0xf0
> > > > > > 
> > > > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > > > > Reviewed-by: Gustavo Pimentel <
> > > > > > gustavo.pimentel@synopsys.com>
> > > > > > ---
> > > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > > >  1 file changed, 15 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > > > > index 23672680dba7..11f843aa96b3 100644
> > > > > > --- a/drivers/pci/quirks.c
> > > > > > +++ b/drivers/pci/quirks.c
> > > > > > @@ -2925,6 +2925,21 @@
> > > > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0x10a1,
> > > > > >  			quirk_msi_intx_disable_qca_bug);
> > > > > >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATTANSIC, 0xe091,
> > > > > >  			quirk_msi_intx_disable_qca_bug);
> > > > > > +
> > > > > > +/*
> > > > > > + * Amazon's Annapurna Labs 1c36:0031 Root Ports don't
> > > > > > support
> > > > > > MSI-
> > > > > > X, so it
> > > > > > + * should be disabled on platforms where the device
> > > > > > (mistakenly)
> > > > > > advertises it.
> > > > > > + *
> > > > > > + * The 0031 device id is reused for other non Root Port
> > > > > > device
> > > > > > types,
> > > > > > + * therefore the quirk is registered for the
> > > > > > PCI_CLASS_BRIDGE_PCI
> > > > > > class.
> > > > > > + */
> > > > > > +static void quirk_al_msi_disable(struct pci_dev *dev)
> > > > > > +{
> > > > > > +	dev->no_msi = 1;
> > > > > > +	pci_warn(dev, "Disabling MSI-X\n");
> > > > > 
> > > > > This will disable both MSI and MSI-X support - is this really
> > > > > the
> > > > > intention
> > > > > here? Do the root ports support MSI and legacy, or just
> > > > > legacy?
> > > > > 
> > > > 
> > > > The HW should support MSI, but we currently don't have a use
> > > > case
> > > > for
> > > > it so it hasn't been tested and therefore we are okay with
> > > > disabling
> > > > it.
> > > 
> > > OK - then the commit message and comment (for
> > > quirk_al_msi_disable)
> > > should
> > > probably be updated to reflect this. 
> > 
> > If you mean that we should explicitly state that MSI is possibly
> > supported then I don't think it is very relevant here. If we decide
> > to
> > test and enable it in the future, then the new quirk (which would
> > only
> > disable MSI-x) would reflect the fact that MSI works. Sounds ok?
> > 
> 
> My thought process is that 1c36:0031 probably advertises an MSI
> capability
> reflecting that the device probably supports MSI - if there is no
> valid
> reason for disabling MSI support then we shouldn't disable it.
> 
> It doesn't make sense to me to disable it just because that's the
> path of
> least resistance - a better course of action would be to add support
> to disable MSI-X in the kernel.
> 
It's not because it's the path of least resistance. As I've stated, we
currently have no use case for it. Adding support in the kernel for
differentiation between MSI and MSI-X sounds like a good idea, but as
far as I see it, isn't a prerequisite for this patch.

> The kernel panic generated in attempting to enable MSI-X shows that
> this
> device has a user for its MSI/MSI-X interrupts. Thus if you disabled
> only
> MSI-X the PCIe port driver would use MSI interrupts instead - Due to
> "no_msi = 1" I guess it will be using legacy.
> 
This doesn't necessarily mean that there is a user for Message signaled
interrupts, since it could simply be a HW default of the core to expose
these capabilities while in the specific instantiation, proper support
wasn't implemented (which is the case for MSI-X, resulting in this
patch). Theoretically, it is possible that MSI works, but since it
isn't required and no existing way to keep just MSI enabled, I don't
see why this patch should be blocked by this.
 
> > > Especially given that the device id
> > > may be used on other device types - implying that a use-case for
> > > this
> > > may later arise.
> > > 
> > 
> > Not sure I understood that last line. This patch is relevant only
> > for
> > the BRIDGE class 0x0031 device. The other device types, which
> > (sadly)
> > happen to re-use the dev_id, have are totally different and
> > shouldn't
> > be mixed in here.
> > 
> 
> It's probably unlikely, but if 1c36:0031 was the device ID given to a
> future incarnation of this hardware in a PCIe switch (which has
> BRIDGE
> class), then using legacy interrupts instead of MSI wouldn't be ideal
> and an un-necessary limitation.
> 
> This is why adding a comment to indicate that MSI may work but hasn't
> been tested may be helpful for others in the future.
> 
No problem, will be added in v4.

> > > > 
> > > > For future knowledge, how can just MSI-X be disabled?
> > > > I see that in pcie_port_enable_irq_vec(), the pcieport driver
> > > > calls
> > > > pci_alloc_irq_vectors() with PCI_IRQ_MSIX | PCI_IRQ_MSI. And
> > > > internally, both __pci_enable_msix_range() and
> > > > __pci_enable_msi_range()
> > > > use pci_msi_supported() which doesn't differentiate between MSI
> > > > and
> > > > MSI-x.
> > > 
> > > The documentation [1] would suggest that once upon a time
> > > pci_disable_msix
> > > was used - but now should let pci_alloc_irq_vectors cap the max
> > > number
> > > of vectors. However in your case it's the PCIe port driver that
> > > is
> > > attempting
> > > to allocate MSI-X's and so the solution isn't an obvious one.
> > > 
> > > Setting dev->msix_enabled to false (i.e. through
> > > pci_disable_msix)
> > > would
> > > result in an un-necessary WARN_ON_ONCE. 
> > 
> > Per my understanding, it seems that dev->msix_enabled indicates if
> > the
> > MSI-X capability has already been enabled or not, and not as an
> > indication if it is supported by the device. If that is the case,
> > then
> > not sure pci_disable_msix() would result in the desired effect.
> 
> Yes this isn't the right approach.
> 
> > 
> > > I think you'd need to ensure
> > > devi->msix_cap is NULL (which makes sense as your hardware
> > > shouldn't
> > > be
> > > exposing this capability).
> > > 
> > > I guess the right way of achieving this would be through a quirk,
> > > though you'd
> > > be the first to do this and you'd have to ensure the quirk runs
> > > before
> > > anyone tests for msix_cap.
> > > 
> > > That's my view, though others may have different suggestions.
> > > 
> > 
> > I think that maybe a dev->no_msix field should be added and then a
> > corresponding pci_msix_supported() function as well. I'd be glad to
> > take it offline or on a dedicated thread, but it shouldn't block
> > this
> > patchset.
> 
> Thanks,
> 
> Andrew Murray
> 
Thanks for your review!

> > 
> > > [1] Documentation/PCI/msi-howto.rst
> > > 
> > > Thanks,
> > > 
> > > Andrew Murray
> > > 
> > > > 
> > > > > Thanks,
> > > > > 
> > > > > Andrew Murray
> > > > > 
> > > > > > +}
> > > > > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPUR
> > > > > > NA_L
> > > > > > ABS,
> > > > > > 0x0031,
> > > > > > +			      PCI_CLASS_BRIDGE_PCI, 8,
> > > > > > quirk_al_msi_disable);
> > > > > >  #endif /* CONFIG_PCI_MSI */
> > > > > >  
> > > > > >  /*
> > > > > > -- 
> > > > > > 2.17.1
> > > > > > 

[PATCH v3 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                                   |  3 +-
 2 files changed, 47 insertions(+), 1 deletion(-)
 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..89876190eb5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
@@ -0,0 +1,45 @@
+* Amazon Annapurna Labs PCIe host bridge
+
+Amazon's 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: pcie@fb600000 {
+		compatible = "amazon,al-pcie";
+		reg = <0x0 0xfb600000 0x0 0x00100000
+		       0x0 0xfd800000 0x0 0x00010000
+		       0x0 0xfd810000 0x0 0x00001000>;
+		reg-names = "config", "controller", "dbi";
+		bus-range = <0 255>;
+		device_type = "pci";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map-mask = <0x00 0 0 7>;
+		interrupt-map = <0x0000 0 0 1 &gic GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
+		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 5a6137df3f0e..29cca14a05a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12201,10 +12201,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
 S:	Supported
 F:	drivers/pci/controller/
 
-PCIE DRIVER FOR ANNAPURNA LABS
+PCIE DRIVER FOR AMAZON ANNAPURNA LABS
 M:	Jonathan Chocron <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

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

[Rob Herring]

On Tue, Jul 23, 2019 at 12:27:08PM +0300, Jonathan Chocron wrote:
> Document Amazon's Annapurna Labs PCIe host bridge.
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  .../devicetree/bindings/pci/pcie-al.txt       | 45 +++++++++++++++++++
>  MAINTAINERS                                   |  3 +-
>  2 files changed, 47 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt b/Documentation/devicetree/bindings/pci/pcie-al.txt
> new file mode 100644
> index 000000000000..89876190eb5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> @@ -0,0 +1,45 @@
> +* Amazon Annapurna Labs PCIe host bridge
> +
> +Amazon's 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

Driver details are irrelevant to the binding.

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

Needs to be SoC specific.

> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: Register ranges as listed in the reg-names property
> +
> +- reg-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Must include the following entries
> +			- "config"	PCIe ECAM space
> +			- "controller"	AL proprietary registers
> +			- "dbi"		Designware PCIe registers
> +
> +Example:
> +
> +	pcie-external0: pcie@fb600000 {
> +		compatible = "amazon,al-pcie";
> +		reg = <0x0 0xfb600000 0x0 0x00100000
> +		       0x0 0xfd800000 0x0 0x00010000
> +		       0x0 0xfd810000 0x0 0x00001000>;
> +		reg-names = "config", "controller", "dbi";
> +		bus-range = <0 255>;
> +		device_type = "pci";
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		#interrupt-cells = <1>;
> +		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-map-mask = <0x00 0 0 7>;
> +		interrupt-map = <0x0000 0 0 1 &gic GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; /* INTa */
> +		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0 0x07ff0000>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5a6137df3f0e..29cca14a05a6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12201,10 +12201,11 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
>  S:	Supported
>  F:	drivers/pci/controller/
>  
> -PCIE DRIVER FOR ANNAPURNA LABS
> +PCIE DRIVER FOR AMAZON ANNAPURNA LABS
>  M:	Jonathan Chocron <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
> 

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

[Chocron, Jonathan]

On Tue, 2019-08-13 at 09:30 -0600, Rob Herring wrote:
> On Tue, Jul 23, 2019 at 12:27:08PM +0300, Jonathan Chocron wrote:
> > Document Amazon's Annapurna Labs PCIe host bridge.
> > 
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> >  .../devicetree/bindings/pci/pcie-al.txt       | 45
> > +++++++++++++++++++
> >  MAINTAINERS                                   |  3 +-
> >  2 files changed, 47 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/pci/pcie-
> > al.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt
> > b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > new file mode 100644
> > index 000000000000..89876190eb5a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > @@ -0,0 +1,45 @@
> > +* Amazon Annapurna Labs PCIe host bridge
> > +
> > +Amazon's 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
> 
> Driver details are irrelevant to the binding.
> 
Will remove.

> > +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"
> 
> Needs to be SoC specific.
> 
I'm not sure I follow. The PCIe controller can be implemented in
different SoCs. Could you please clarify?

> > +
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: Register ranges as listed in the reg-names property
> > +
> > +- reg-names:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: Must include the following entries
> > +			- "config"	PCIe ECAM space
> > +			- "controller"	AL proprietary registers
> > +			- "dbi"		Designware PCIe registers
> > +
> > +Example:
> > +
> > +	pcie-external0: pcie@fb600000 {
> > +		compatible = "amazon,al-pcie";
> > +		reg = <0x0 0xfb600000 0x0 0x00100000
> > +		       0x0 0xfd800000 0x0 0x00010000
> > +		       0x0 0xfd810000 0x0 0x00001000>;
> > +		reg-names = "config", "controller", "dbi";
> > +		bus-range = <0 255>;
> > +		device_type = "pci";
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		#interrupt-cells = <1>;
> > +		interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> > +		interrupt-map-mask = <0x00 0 0 7>;
> > +		interrupt-map = <0x0000 0 0 1 &gic GIC_SPI 41
> > IRQ_TYPE_LEVEL_HIGH>; /* INTa */
> > +		ranges = <0x02000000 0x0 0xc0010000 0x0 0xc0010000 0x0
> > 0x07ff0000>;
> > +	};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5a6137df3f0e..29cca14a05a6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12201,10 +12201,11 @@ T:	git
> > git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
> >  S:	Supported
> >  F:	drivers/pci/controller/
> >  
> > -PCIE DRIVER FOR ANNAPURNA LABS
> > +PCIE DRIVER FOR AMAZON ANNAPURNA LABS
> >  M:	Jonathan Chocron <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
> > 

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

[Rob Herring]

On Tue, Aug 13, 2019 at 10:49 AM Chocron, Jonathan <jonnyc@amazon.com> wrote:
>
> On Tue, 2019-08-13 at 09:30 -0600, Rob Herring wrote:
> > On Tue, Jul 23, 2019 at 12:27:08PM +0300, Jonathan Chocron wrote:
> > > Document Amazon's Annapurna Labs PCIe host bridge.
> > >
> > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > ---
> > >  .../devicetree/bindings/pci/pcie-al.txt       | 45
> > > +++++++++++++++++++
> > >  MAINTAINERS                                   |  3 +-
> > >  2 files changed, 47 insertions(+), 1 deletion(-)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/pcie-
> > > al.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > new file mode 100644
> > > index 000000000000..89876190eb5a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > @@ -0,0 +1,45 @@
> > > +* Amazon Annapurna Labs PCIe host bridge
> > > +
> > > +Amazon's 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
> >
> > Driver details are irrelevant to the binding.
> >
> Will remove.
>
> > > +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"
> >
> > Needs to be SoC specific.
> >
> I'm not sure I follow. The PCIe controller can be implemented in
> different SoCs. Could you please clarify?

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

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

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

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

Rob

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

[Chocron, Jonathan]

On Tue, 2019-08-13 at 14:46 -0600, Rob Herring wrote:
> On Tue, Aug 13, 2019 at 10:49 AM Chocron, Jonathan <jonnyc@amazon.com
> > wrote:
> > 
> > On Tue, 2019-08-13 at 09:30 -0600, Rob Herring wrote:
> > > On Tue, Jul 23, 2019 at 12:27:08PM +0300, Jonathan Chocron wrote:
> > > > Document Amazon's Annapurna Labs PCIe host bridge.
> > > > 
> > > > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > > > ---
> > > >  .../devicetree/bindings/pci/pcie-al.txt       | 45
> > > > +++++++++++++++++++
> > > >  MAINTAINERS                                   |  3 +-
> > > >  2 files changed, 47 insertions(+), 1 deletion(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/pci/pcie-
> > > > al.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > > b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > > new file mode 100644
> > > > index 000000000000..89876190eb5a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/pcie-al.txt
> > > > @@ -0,0 +1,45 @@
> > > > +* Amazon Annapurna Labs PCIe host bridge
> > > > +
> > > > +Amazon's 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
> > > 
> > > Driver details are irrelevant to the binding.
> > > 
> > 
> > Will remove.
> > 
> > > > +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"
> > > 
> > > Needs to be SoC specific.
> > > 
> > 
> > I'm not sure I follow. The PCIe controller can be implemented in
> > different SoCs. Could you please clarify?
> 
> All the features, bugs, and integration will be exactly the same on
> all SoCs and you will never need to distinguish?
> 
> This is standard convention for compatible strings and how you avoid
> updating the DT (part of firmware) when you find some difference the
> OS needs to handle down the road.
> 
> If the next SoC is 'the same', then you do:
> 
> compatible = "amazon,newsoc-pcie", "amazon,oldsoc-pcie";
> 
Got it. So currently the relevant SoC compatibles will be:
"amazon,al-alpine-v2-pcie"
"amazon,al-alpine-v3-pcie"

Sidenote: In the driver I'll add them to the of_device_id match table
with an empty .data field for both. Sounds sane?

I'll update all this as part of v4.

> Rob

[PATCH v3 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   |  12 +
 drivers/pci/controller/dwc/pcie-al.c | 367 +++++++++++++++++++++++++++
 2 files changed, 379 insertions(+)

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

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

[Gustavo Pimentel]

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


Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

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

[Lorenzo Pieralisi]

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

Make $SUBJECT compliant with other host controllers patches.

On Tue, Jul 23, 2019 at 12:27:09PM +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.

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

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

> All link initializations are handled by the boot FW.
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/controller/dwc/Kconfig   |  12 +
>  drivers/pci/controller/dwc/pcie-al.c | 367 +++++++++++++++++++++++++++
>  2 files changed, 379 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 6ea778ae4877..3c6094cbcc3b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -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
> diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c
> index 3ab58f0584a8..3ffdd3c97617 100644
> --- a/drivers/pci/controller/dwc/pcie-al.c
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
>  };
>  
>  #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> +
> +#ifdef CONFIG_PCIE_AL
> +
> +#include <linux/of_pci.h>
> +#include "pcie-designware.h"
> +
> +#define AL_PCIE_REV_ID_2	2
> +#define AL_PCIE_REV_ID_3	3
> +#define AL_PCIE_REV_ID_4	4
> +
> +#define AXI_BASE_OFFSET		0x0
> +
> +#define DEVICE_ID_OFFSET	0x16c
> +
> +#define DEVICE_REV_ID			0x0
> +#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
> +
> +#define DEVICE_REV_ID_DEV_ID_X4		0
> +#define DEVICE_REV_ID_DEV_ID_X8		2
> +#define DEVICE_REV_ID_DEV_ID_X16	4
> +
> +#define OB_CTRL_REV1_2_OFFSET	0x0040
> +#define OB_CTRL_REV3_5_OFFSET	0x0030
> +
> +#define CFG_TARGET_BUS			0x0
> +#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
> +#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
> +
> +#define CFG_CONTROL			0x4
> +#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
> +#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
> +
> +struct al_pcie_reg_offsets {
> +	unsigned int ob_ctrl;
> +};
> +
> +struct al_pcie_target_bus_cfg {
> +	u8 reg_val;
> +	u8 reg_mask;
> +	u8 ecam_mask;
> +};
> +
> +struct al_pcie {
> +	struct dw_pcie *pci;
> +	void __iomem *controller_base; /* base of PCIe unit (not DW core) */
> +	struct device *dev;
> +	resource_size_t ecam_size;
> +	unsigned int controller_rev_id;
> +	struct al_pcie_reg_offsets reg_offsets;
> +	struct al_pcie_target_bus_cfg target_bus_cfg;
> +};
> +
> +#define PCIE_ECAM_DEVFN(x)		(((x) & 0xff) << 12)
> +
> +#define to_al_pcie(x)		dev_get_drvdata((x)->dev)
> +
> +static inline u32 al_pcie_controller_readl(struct al_pcie *pcie, u32 offset)
> +{
> +	return readl(pcie->controller_base + offset);
> +}
> +
> +static inline void al_pcie_controller_writel(struct al_pcie *pcie, u32 offset,
> +					     u32 val)
> +{
> +	writel(val, pcie->controller_base + offset);
> +}

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

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

dev_dbg() ?

> +
> +	return rc;
> +}
> +
> +static void al_pcie_config_prepare(struct al_pcie *pcie)
> +{
> +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> +	struct pcie_port *pp = &pcie->pci->pp;
> +	unsigned int ecam_bus_mask;
> +	u32 cfg_control_offset;
> +	u8 subordinate_bus;
> +	u8 secondary_bus;
> +	u32 cfg_control;
> +	u32 reg;
> +
> +	target_bus_cfg = &pcie->target_bus_cfg;
> +
> +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> +	if (ecam_bus_mask > 255) {
> +		dev_warn(pcie->dev, "ECAM window size is larger than 256MB. Cutting off at 256\n");
> +		ecam_bus_mask = 255;
> +	}
> +
> +	/* This portion is taken from the transaction address */
> +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> +	/* This portion is taken from the cfg_target_bus reg */
> +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
> +
> +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> +			       target_bus_cfg->reg_mask);
> +
> +	secondary_bus = pp->busn->start + 1;
> +	subordinate_bus = pp->busn->end;
> +
> +	/* Set the valid values of secondary and subordinate buses */
> +	cfg_control_offset = AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
> +			     CFG_CONTROL;
> +
> +	cfg_control = al_pcie_controller_readl(pcie, cfg_control_offset);
> +
> +	reg = cfg_control &
> +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> +
> +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> +
> +	al_pcie_controller_writel(pcie, cfg_control_offset, reg);
> +}
> +
> +static int al_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct al_pcie *pcie = to_al_pcie(pci);
> +	int link_up;
> +	int rc;
> +
> +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> +	if (rc)
> +		return rc;
> +
> +	rc = al_pcie_reg_offsets_set(pcie);
> +	if (rc)
> +		return rc;
> +
> +	al_pcie_config_prepare(pcie);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> +	.rd_other_conf = al_pcie_rd_other_conf,
> +	.wr_other_conf = al_pcie_wr_other_conf,
> +	.host_init = al_pcie_host_init,
> +};
> +
> +static int al_add_pcie_port(struct pcie_port *pp,
> +			    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	pp->ops = &al_pcie_host_ops;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize host\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +};

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

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

Nit:

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

?

Lorenzo

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

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

[Chocron, Jonathan]

On Mon, 2019-08-12 at 18:03 +0100, Lorenzo Pieralisi wrote:
> "PCI: dwc: al: Add support for DW based driver type"
> 
> Make $SUBJECT compliant with other host controllers patches.
> 
Will do.

BTW,I actually see that many of the other dwc controllers don't have
the 'dwc:' (and some are inconsistent with its usage), but I agree that
the folder structure here makes sense.

> On Tue, Jul 23, 2019 at 12:27:09PM +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.
> 
> I would appreciate if you can add a simple explanation of
> the mechanism for completeness.
> 
Sure, will be added as part of v4. In a nutshell, we can set a reg
which will tells the HW how to construct the BUS part of the
transaction - which bits will be taken from the ECAM part of the
transaction and which from a specific HW reg.

> AFAIU, with ACPI you don't support all these variants.
> 
You are correct. This flexibility isn't needed for the ACPI flavor so
we kept it simple.

> > All link initializations are handled by the boot FW.
> > 
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig   |  12 +
> >  drivers/pci/controller/dwc/pcie-al.c | 367
> > +++++++++++++++++++++++++++
> >  2 files changed, 379 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index 6ea778ae4877..3c6094cbcc3b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -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
> > diff --git a/drivers/pci/controller/dwc/pcie-al.c
> > b/drivers/pci/controller/dwc/pcie-al.c
> > index 3ab58f0584a8..3ffdd3c97617 100644
> > --- a/drivers/pci/controller/dwc/pcie-al.c
> > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > @@ -91,3 +91,370 @@ struct pci_ecam_ops al_pcie_ops = {
> >  };
> >  
> >  #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> > +
> > +#ifdef CONFIG_PCIE_AL
> > +
> > +#include <linux/of_pci.h>
> > +#include "pcie-designware.h"
> > +
> > +#define AL_PCIE_REV_ID_2	2
> > +#define AL_PCIE_REV_ID_3	3
> > +#define AL_PCIE_REV_ID_4	4
> > +
> > +#define AXI_BASE_OFFSET		0x0
> > +
> > +#define DEVICE_ID_OFFSET	0x16c
> > +
> > +#define DEVICE_REV_ID			0x0
> > +#define DEVICE_REV_ID_DEV_ID_MASK	GENMASK(31, 16)
> > +
> > +#define DEVICE_REV_ID_DEV_ID_X4		0
> > +#define DEVICE_REV_ID_DEV_ID_X8		2
> > +#define DEVICE_REV_ID_DEV_ID_X16	4
> > +
> > +#define OB_CTRL_REV1_2_OFFSET	0x0040
> > +#define OB_CTRL_REV3_5_OFFSET	0x0030
> > +
> > +#define CFG_TARGET_BUS			0x0
> > +#define CFG_TARGET_BUS_MASK_MASK	GENMASK(7, 0)
> > +#define CFG_TARGET_BUS_BUSNUM_MASK	GENMASK(15, 8)
> > +
> > +#define CFG_CONTROL			0x4
> > +#define CFG_CONTROL_SUBBUS_MASK		GENMASK(15, 8)
> > +#define CFG_CONTROL_SEC_BUS_MASK	GENMASK(23, 16)
> > +
> > +struct al_pcie_reg_offsets {
> > +	unsigned int ob_ctrl;
> > +};
> > +
> > +struct al_pcie_target_bus_cfg {
> > +	u8 reg_val;
> > +	u8 reg_mask;
> > +	u8 ecam_mask;
> > +};
> > +
> > +struct al_pcie {
> > +	struct dw_pcie *pci;
> > +	void __iomem *controller_base; /* base of PCIe unit (not DW
> > core) */
> > +	struct device *dev;
> > +	resource_size_t ecam_size;
> > +	unsigned int controller_rev_id;
> > +	struct al_pcie_reg_offsets reg_offsets;
> > +	struct al_pcie_target_bus_cfg target_bus_cfg;
> > +};
> > +
> > +#define PCIE_ECAM_DEVFN(x)		(((x) & 0xff) << 12)
> > +
> > +#define to_al_pcie(x)		dev_get_drvdata((x)->dev)
> > +
> > +static inline u32 al_pcie_controller_readl(struct al_pcie *pcie,
> > u32 offset)
> > +{
> > +	return readl(pcie->controller_base + offset);
> > +}
> > +
> > +static inline void al_pcie_controller_writel(struct al_pcie *pcie,
> > u32 offset,
> > +					     u32 val)
> > +{
> > +	writel(val, pcie->controller_base + offset);
> > +}
> 
> You should be able to use the read/write{_relaxed} API.
> 
Will change.
> > +
> > +static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > *rev_id)
> > +{
> > +	u32 dev_rev_id_val;
> > +	u32 dev_id_val;
> > +
> > +	dev_rev_id_val = al_pcie_controller_readl(pcie, AXI_BASE_OFFSET
> > +
> > +						  DEVICE_ID_OFFSET +
> > +						  DEVICE_REV_ID);
> > +	dev_id_val = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> > dev_rev_id_val);
> > +
> > +	switch (dev_id_val) {
> > +	case DEVICE_REV_ID_DEV_ID_X4:
> > +		*rev_id = AL_PCIE_REV_ID_2;
> > +		break;
> > +	case DEVICE_REV_ID_DEV_ID_X8:
> > +		*rev_id = AL_PCIE_REV_ID_3;
> > +		break;
> > +	case DEVICE_REV_ID_DEV_ID_X16:
> > +		*rev_id = AL_PCIE_REV_ID_4;
> > +		break;
> > +	default:
> > +		dev_err(pcie->dev, "Unsupported dev_id_val (0x%x)\n",
> > +			dev_id_val);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(pcie->dev, "dev_id_val: 0x%x\n", dev_id_val);
> > +
> > +	return 0;
> > +}
> > +
> > +static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
> > +{
> > +	switch (pcie->controller_rev_id) {
> > +	case AL_PCIE_REV_ID_2:
> > +		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> > +		break;
> > +	case AL_PCIE_REV_ID_3:
> > +	case AL_PCIE_REV_ID_4:
> > +		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> > +		break;
> > +	default:
> > +		dev_err(pcie->dev, "Unsupported controller rev_id:
> > 0x%x\n",
> > +			pcie->controller_rev_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > +					  u8 target_bus,
> > +					  u8 mask_target_bus)
> > +{
> > +	u32 reg;
> > +
> > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> > +
> > +	al_pcie_controller_writel(pcie, AXI_BASE_OFFSET +
> > +				  pcie->reg_offsets.ob_ctrl +
> > CFG_TARGET_BUS,
> > +				  reg);
> > +}
> > +
> > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > +					   unsigned int busnr,
> > +					   unsigned int devfn)
> > +{
> > +	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > >target_bus_cfg;
> > +	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > +	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > +	struct pcie_port *pp = &pcie->pci->pp;
> > +	void __iomem *pci_base_addr;
> > +
> > +	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > +					 (busnr_ecam << 20) +
> > +					 PCIE_ECAM_DEVFN(devfn));
> > +
> > +	if (busnr_reg != target_bus_cfg->reg_val) {
> > +		dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > from 0x%x to 0x%x\n",
> > +			target_bus_cfg->reg_val, busnr_reg);
> > +		target_bus_cfg->reg_val = busnr_reg;
> > +		al_pcie_target_bus_set(pcie,
> > +				       target_bus_cfg->reg_val,
> > +				       target_bus_cfg->reg_mask);
> > +	}
> > +
> > +	return pci_base_addr;
> > +}
> > +
> > +static int al_pcie_rd_other_conf(struct pcie_port *pp, struct
> > pci_bus *bus,
> > +				 unsigned int devfn, int where, int
> > size,
> > +				 u32 *val)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct al_pcie *pcie = to_al_pcie(pci);
> > +	unsigned int busnr = bus->number;
> > +	void __iomem *pci_addr;
> > +	int rc;
> > +
> > +	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > +	rc = dw_pcie_read(pci_addr + where, size, val);
> > +
> > +	dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d
> > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > +		size, pci_domain_nr(bus), bus->number,
> > +		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > +		(pci_addr + where), *val);
> > +
> > +	return rc;
> > +}
> > +
> > +static int al_pcie_wr_other_conf(struct pcie_port *pp, struct
> > pci_bus *bus,
> > +				 unsigned int devfn, int where, int
> > size,
> > +				 u32 val)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct al_pcie *pcie = to_al_pcie(pci);
> > +	unsigned int busnr = bus->number;
> > +	void __iomem *pci_addr;
> > +	int rc;
> > +
> > +	pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > +	rc = dw_pcie_write(pci_addr + where, size, val);
> > +
> > +	dev_err(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d
> > offset 0x%x (pci_addr: 0x%px) - val:0x%x\n",
> > +		size, pci_domain_nr(bus), bus->number,
> > +		PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > +		(pci_addr + where), val);
> 
> dev_dbg() ?
> 
Indeed. Was dev_dbg() in v1, but I played with it during testing for
later versions and must have forgot to change it back.

> > +
> > +	return rc;
> > +}
> > +
> > +static void al_pcie_config_prepare(struct al_pcie *pcie)
> > +{
> > +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> > +	struct pcie_port *pp = &pcie->pci->pp;
> > +	unsigned int ecam_bus_mask;
> > +	u32 cfg_control_offset;
> > +	u8 subordinate_bus;
> > +	u8 secondary_bus;
> > +	u32 cfg_control;
> > +	u32 reg;
> > +
> > +	target_bus_cfg = &pcie->target_bus_cfg;
> > +
> > +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > +	if (ecam_bus_mask > 255) {
> > +		dev_warn(pcie->dev, "ECAM window size is larger than
> > 256MB. Cutting off at 256\n");
> > +		ecam_bus_mask = 255;
> > +	}
> > +
> > +	/* This portion is taken from the transaction address */
> > +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> > +	/* This portion is taken from the cfg_target_bus reg */
> > +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > >reg_mask;
> > +
> > +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> > +			       target_bus_cfg->reg_mask);
> > +
> > +	secondary_bus = pp->busn->start + 1;
> > +	subordinate_bus = pp->busn->end;
> > +
> > +	/* Set the valid values of secondary and subordinate buses */
> > +	cfg_control_offset = AXI_BASE_OFFSET + pcie-
> > >reg_offsets.ob_ctrl +
> > +			     CFG_CONTROL;
> > +
> > +	cfg_control = al_pcie_controller_readl(pcie,
> > cfg_control_offset);
> > +
> > +	reg = cfg_control &
> > +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> > +
> > +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> > +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> > +
> > +	al_pcie_controller_writel(pcie, cfg_control_offset, reg);
> > +}
> > +
> > +static int al_pcie_host_init(struct pcie_port *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct al_pcie *pcie = to_al_pcie(pci);
> > +	int link_up;
> > +	int rc;
> > +
> > +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = al_pcie_reg_offsets_set(pcie);
> > +	if (rc)
> > +		return rc;
> > +
> > +	al_pcie_config_prepare(pcie);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > +	.rd_other_conf = al_pcie_rd_other_conf,
> > +	.wr_other_conf = al_pcie_wr_other_conf,
> > +	.host_init = al_pcie_host_init,
> > +};
> > +
> > +static int al_add_pcie_port(struct pcie_port *pp,
> > +			    struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	pp->ops = &al_pcie_host_ops;
> > +
> > +	ret = dw_pcie_host_init(pp);
> > +	if (ret) {
> > +		dev_err(dev, "failed to initialize host\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > +};
> 
> I understand you have to have it - probably we should improve
> the generic DW layer to check for a pointer before dereferencing
> so that we avoid this empty struct. Anyway, that's for another
> series.
> 
Made my eyes soar as well :)

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

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

[PATCH v3 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>
Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.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 v3 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 v3 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags

[Lorenzo Pieralisi]

On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote:
> 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);

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

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

Lorenzo

> +
>  	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 v3 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags

[Chocron, Jonathan]

On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote:
> > 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);
> 
> This changes the default for bus reassignment on all DWC host (that
> are
> !PCI_PROBE_ONLY), we should drop this line, it can trigger
> regressions.
> 
Will be dropped as part of v4. There might also be a behavioral
difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)).
Is that still ok?

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


> If we still want to merge it as a separate change we must test it on
> all
> DWC host bridges to make sure it does not trigger any issues with
> current set-ups, that's not going to be easy though.
> 
Just out of curiosity, how are such exhaustive tests achieved when a
patch requires them?

> Lorenzo
> 
> > +
> >  	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 v3 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags

[Lorenzo Pieralisi]

Side note, run:

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

Eg PCI: dw: should be PCI: dwc:

On Thu, Aug 08, 2019 at 09:30:05AM +0000, Chocron, Jonathan wrote:
> On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote:
> > > 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);
> > 
> > This changes the default for bus reassignment on all DWC host (that
> > are
> > !PCI_PROBE_ONLY), we should drop this line, it can trigger
> > regressions.
> > 
> Will be dropped as part of v4. There might also be a behavioral
> difference below where I added the if (pci_has_flag(PCI_PROBE_ONLY)).
> Is that still ok?

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

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

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

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

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

> > If we still want to merge it as a separate change we must test it on
> > all
> > DWC host bridges to make sure it does not trigger any issues with
> > current set-ups, that's not going to be easy though.
> > 
> Just out of curiosity, how are such exhaustive tests achieved when a
> patch requires them?

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

Thanks,
Lorenzo

> 
> > Lorenzo
> > 
> > > +
> > >  	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 v3 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags

[Chocron, Jonathan]

On Thu, 2019-08-08 at 11:58 +0100, Lorenzo Pieralisi wrote:
> Side note, run:
> 
> git log --oneline on drivers/pci/controller/dwc existing files and
> make sure commit subjects are in line with those.
> 
> Eg PCI: dw: should be PCI: dwc:
> 
Done.

> On Thu, Aug 08, 2019 at 09:30:05AM +0000, Chocron, Jonathan wrote:
> > On Wed, 2019-08-07 at 17:36 +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Jul 23, 2019 at 12:27:11PM +0300, Jonathan Chocron wrote:
> > > > 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);
> > > 
> > > This changes the default for bus reassignment on all DWC host
> > > (that
> > > are
> > > !PCI_PROBE_ONLY), we should drop this line, it can trigger
> > > regressions.
> > > 
> > 
> > Will be dropped as part of v4. There might also be a behavioral
> > difference below where I added the if
> > (pci_has_flag(PCI_PROBE_ONLY)).
> > Is that still ok?
> 
> That's true but I doubt any DWC host has a DT firmware with
> "linux,pci-probe-only" in it.
> 
> It is trial and error I am afraid, please make sure all DWC
> maintainers are copied in.
> 
> > As I pointed out in the cover letter, since PCI_PROBE_ONLY is a
> > system
> > wide flag, does it make sense to call of_pci_check_probe_only()
> > here,
> > in the context of a specific driver (including the existing
> > invocation
> > in pci_host_common_probe()), as opposed to a platform/arch context?
> 
> It is an ongoing discussion to define how we should handle
> PCI_PROBE_ONLY. Adding this code into DWC I do not think it
> would hurt but if we can postpone it for the next (v5.5) merge
> window after we debate this at LPC within the PCI microconference
> it would be great.
> 
The preceding patches are more urgent, so I'm fine with postponing this
patch to the next merge window (I'll drop it from v4).

> Please sync with Benjamin as a first step, I trust he would ask
> you to do the right thing.
> 
Of course, I'll sync with him. But again, let's not have this patch
delay the others please.

> > > If we still want to merge it as a separate change we must test it
> > > on
> > > all
> > > DWC host bridges to make sure it does not trigger any issues with
> > > current set-ups, that's not going to be easy though.
> > > 
> > 
> > Just out of curiosity, how are such exhaustive tests achieved when
> > a
> > patch requires them?
> 
> CC DWC host bridge maintainers and ask them to test it. I do not have
> the HW (and FW) required, I am sorry, that's the only option I can
> give
> you. -next coverage would help too but to a minor extent.
> 
Thanks for the info!

> Thanks,
> Lorenzo
> 
> > 
> > > Lorenzo
> > > 
> > > > +
> > > >  	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
> > > >