linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver
@ 2019-07-18  9:45 Jonathan Chocron
  2019-07-18  9:45 ` [PATCH v2 1/8] PCI: Add Amazon's Annapurna Labs vendor ID Jonathan Chocron
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:45 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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 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          | 373 ++++++++++++++++++
 .../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, 519 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-al.txt

-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/8] PCI: Add Amazon's Annapurna Labs vendor ID
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
@ 2019-07-18  9:45 ` Jonathan Chocron
  2019-07-18  9:45 ` [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports Jonathan Chocron
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:45 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
  2019-07-18  9:45 ` [PATCH v2 1/8] PCI: Add Amazon's Annapurna Labs vendor ID Jonathan Chocron
@ 2019-07-18  9:45 ` Jonathan Chocron
  2019-07-19  8:25   ` Gustavo Pimentel
  2019-07-18  9:45 ` [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port Jonathan Chocron
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:45 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

From: Ali Saidi <alisaidi@amazon.com>

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

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

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
  2019-07-18  9:45 ` [PATCH v2 1/8] PCI: Add Amazon's Annapurna Labs vendor ID Jonathan Chocron
  2019-07-18  9:45 ` [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports Jonathan Chocron
@ 2019-07-18  9:45 ` Jonathan Chocron
  2019-07-19  8:24   ` Gustavo Pimentel
  2019-07-18  9:45 ` [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support " Jonathan Chocron
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:45 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
                   ` (2 preceding siblings ...)
  2019-07-18  9:45 ` [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port Jonathan Chocron
@ 2019-07-18  9:45 ` Jonathan Chocron
  2019-07-19  9:17   ` Gustavo Pimentel
  2019-07-18  9:47 ` [PATCH v2 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding Jonathan Chocron
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:45 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
                   ` (3 preceding siblings ...)
  2019-07-18  9:45 ` [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support " Jonathan Chocron
@ 2019-07-18  9:47 ` Jonathan Chocron
  2019-07-18  9:47 ` [PATCH v2 6/8] PCI: al: Add support for DW based driver type Jonathan Chocron
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:47 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
                   ` (4 preceding siblings ...)
  2019-07-18  9:47 ` [PATCH v2 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding Jonathan Chocron
@ 2019-07-18  9:47 ` Jonathan Chocron
  2019-07-19  8:55   ` Gustavo Pimentel
  2019-07-18  9:47 ` [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode Jonathan Chocron
  2019-07-18  9:47 ` [PATCH v2 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags Jonathan Chocron
  7 siblings, 1 reply; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:47 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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 | 373 +++++++++++++++++++++++++++
 2 files changed, 385 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..40555532fb9a 100644
--- a/drivers/pci/controller/dwc/pcie-al.c
+++ b/drivers/pci/controller/dwc/pcie-al.c
@@ -91,3 +91,376 @@ 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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
+{
+	void __iomem *dev_rev_id_addr;
+	u32 dev_rev_id;
+
+	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
+			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET + DEVICE_REV_ID);
+
+	dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
+			       readl(dev_rev_id_addr));
+	switch (dev_rev_id) {
+	case DEVICE_REV_ID_DEV_ID_X4:
+		*rev_id = AL_PCIE_REV_ID_2;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X8:
+		*rev_id = AL_PCIE_REV_ID_3;
+		break;
+	case DEVICE_REV_ID_DEV_ID_X16:
+		*rev_id = AL_PCIE_REV_ID_4;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
+			dev_rev_id);
+		return -EINVAL;
+	}
+
+	dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);
+
+	return 0;
+}
+
+static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
+{
+	switch (pcie->controller_rev_id) {
+	case AL_PCIE_REV_ID_2:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
+		break;
+	case AL_PCIE_REV_ID_3:
+	case AL_PCIE_REV_ID_4:
+		pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
+		break;
+	default:
+		dev_err(pcie->dev, "Unsupported controller rev_id: 0x%x\n",
+			pcie->controller_rev_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
+					  u8 target_bus,
+					  u8 mask_target_bus)
+{
+	void __iomem *cfg_control_addr;
+	u32 reg;
+
+	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
+	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
+
+	cfg_control_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
+			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
+			   CFG_TARGET_BUS);
+
+	writel(reg, cfg_control_addr);
+}
+
+static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
+					   unsigned int busnr,
+					   unsigned int devfn)
+{
+	void __iomem *pci_base_addr;
+	struct pcie_port *pp = &pcie->pci->pp;
+	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie->target_bus_cfg;
+	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
+	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
+
+	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
+					 (busnr_ecam << 20) +
+					 PCIE_ECAM_DEVFN(devfn));
+
+	if (busnr_reg != target_bus_cfg->reg_val) {
+		dev_dbg(pcie->pci->dev, "Changing target bus busnum val from 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 int al_pcie_config_prepare(struct al_pcie *pcie)
+{
+	struct al_pcie_target_bus_cfg *target_bus_cfg;
+	struct pcie_port *pp = &pcie->pci->pp;
+	unsigned int ecam_bus_mask;
+	u8 secondary_bus;
+	u8 subordinate_bus;
+	void __iomem *cfg_control_addr;
+	u32 cfg_control;
+	u32 reg;
+
+	target_bus_cfg = &pcie->target_bus_cfg;
+
+	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
+	if (ecam_bus_mask > 255) {
+		dev_warn(pcie->dev, "ECAM window size is larger than 256MB. Cutting off at 256\n");
+		ecam_bus_mask = 255;
+	}
+
+	/* This portion is taken from the transaction address */
+	target_bus_cfg->ecam_mask = ecam_bus_mask;
+	/* This portion is taken from the cfg_target_bus reg */
+	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
+	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
+
+	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
+			       target_bus_cfg->reg_mask);
+
+	secondary_bus = pp->busn->start + 1;
+	subordinate_bus = pp->busn->end;
+
+	/* Set the valid values of secondary and subordinate buses */
+	cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
+			   pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
+
+	cfg_control = readl(cfg_control_addr);
+
+	reg = cfg_control &
+	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
+
+	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
+	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
+
+	writel(reg, cfg_control_addr);
+
+	return 0;
+}
+
+static int al_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct al_pcie *pcie = to_al_pcie(pci);
+	int link_up;
+	int rc;
+
+	link_up = dw_pcie_link_up(pci);
+	if (!link_up) {
+		dev_err(pci->dev, "link is not up!\n");
+		return -ENOLINK;
+	}
+
+	dev_info(pci->dev, "link is up\n");
+
+	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
+	if (rc)
+		return rc;
+
+	rc = al_pcie_reg_offsets_set(pcie);
+	if (rc)
+		return rc;
+
+	rc = al_pcie_config_prepare(pcie);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops al_pcie_host_ops = {
+	.rd_other_conf = al_pcie_rd_other_conf,
+	.wr_other_conf = al_pcie_wr_other_conf,
+	.host_init = al_pcie_host_init,
+};
+
+static int al_add_pcie_port(struct pcie_port *pp,
+			    struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pp->ops = &al_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+};
+
+static int al_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct al_pcie *al_pcie;
+	struct dw_pcie *pci;
+	struct resource *dbi_res;
+	struct resource *controller_res;
+	struct resource *ecam_res;
+	int ret;
+
+	al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
+	if (!al_pcie)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+
+	al_pcie->pci = pci;
+	al_pcie->dev = dev;
+
+	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
+	if (IS_ERR(pci->dbi_base)) {
+		dev_err(dev, "couldn't remap dbi base %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);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id al_pcie_of_match[] = {
+	{ .compatible = "amazon,al-pcie",
+	},
+	{},
+};
+
+static struct platform_driver al_pcie_driver = {
+	.driver = {
+		.name	= "al-pcie",
+		.of_match_table = al_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = al_pcie_probe,
+};
+builtin_platform_driver(al_pcie_driver);
+
+#endif /* CONFIG_PCIE_AL*/
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
                   ` (5 preceding siblings ...)
  2019-07-18  9:47 ` [PATCH v2 6/8] PCI: al: Add support for DW based driver type Jonathan Chocron
@ 2019-07-18  9:47 ` Jonathan Chocron
  2019-07-19  9:15   ` Gustavo Pimentel
  2019-07-18  9:47 ` [PATCH v2 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags Jonathan Chocron
  7 siblings, 1 reply; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:47 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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

Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 2bf5a35c0570..00e59a134b93 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 	int ret;
 	u32 reg;
 	void *addr;
+	u8 hdr_type;
 	unsigned int nbars;
 	unsigned int offset;
 	struct pci_epc *epc;
@@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return -EINVAL;
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
+		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
 		dev_err(dev, "Unable to read *num-ib-windows* property\n");
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index f93252d0da5b..d2ca748e4c85 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	struct pci_bus *child;
 	struct pci_host_bridge *bridge;
 	struct resource *cfg_res;
+	u8 hdr_type;
 	int ret;
 
 	raw_spin_lock_init(&pci->pp.lock);
@@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
+	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
+	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
+		dev_err(pci->dev, "PCIe controller is not set to bridge type (hdr_type: 0x%x)!\n",
+			hdr_type);
+		return -EIO;
+	}
+
 	pp->mem_base = pp->mem->start;
 
 	if (!pp->va_cfg0_base) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags
  2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
                   ` (6 preceding siblings ...)
  2019-07-18  9:47 ` [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode Jonathan Chocron
@ 2019-07-18  9:47 ` Jonathan Chocron
  7 siblings, 0 replies; 19+ messages in thread
From: Jonathan Chocron @ 2019-07-18  9:47 UTC (permalink / raw)
  To: lorenzo.pieralisi, bhelgaas, jingoohan1, gustavo.pimentel,
	robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree, jonnyc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port
  2019-07-18  9:45 ` [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port Jonathan Chocron
@ 2019-07-19  8:24   ` Gustavo Pimentel
  0 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-19  8:24 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi, bhelgaas, jingoohan1,
	gustavo.pimentel@synopsys.com, robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree

On Thu, Jul 18, 2019 at 10:45:26, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

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

Seems ok.

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




^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports
  2019-07-18  9:45 ` [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports Jonathan Chocron
@ 2019-07-19  8:25   ` Gustavo Pimentel
  0 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-19  8:25 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi, bhelgaas, jingoohan1,
	gustavo.pimentel@synopsys.com, robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree

On Thu, Jul 18, 2019 at 10:45:25, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

> From: Ali Saidi <alisaidi@amazon.com>
> 
> The Amazon's Annapurna Labs root ports don't advertise an ACS
> capability, but they don't allow peer-to-peer transactions and do
> validate bus numbers through the SMMU. Additionally, it's not possible
> for one RP to pass traffic to another RP.
> 
> Signed-off-by: Ali Saidi <alisaidi@amazon.com>
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/quirks.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 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

Seems ok.

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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-18  9:47 ` [PATCH v2 6/8] PCI: al: Add support for DW based driver type Jonathan Chocron
@ 2019-07-19  8:55   ` Gustavo Pimentel
  2019-07-21 15:08     ` Chocron, Jonathan
  0 siblings, 1 reply; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-19  8:55 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi, bhelgaas, jingoohan1,
	gustavo.pimentel@synopsys.com, robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree

On Thu, Jul 18, 2019 at 10:47:16, 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 | 373 +++++++++++++++++++++++++++
>  2 files changed, 385 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..40555532fb9a 100644
> --- a/drivers/pci/controller/dwc/pcie-al.c
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -91,3 +91,376 @@ 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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int *rev_id)
> +{
> +	void __iomem *dev_rev_id_addr;
> +	u32 dev_rev_id;
> +
> +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie->controller_base +
> +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET + DEVICE_REV_ID);
> +
> +	dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> +			       readl(dev_rev_id_addr));
> +	switch (dev_rev_id) {
> +	case DEVICE_REV_ID_DEV_ID_X4:
> +		*rev_id = AL_PCIE_REV_ID_2;
> +		break;
> +	case DEVICE_REV_ID_DEV_ID_X8:
> +		*rev_id = AL_PCIE_REV_ID_3;
> +		break;
> +	case DEVICE_REV_ID_DEV_ID_X16:
> +		*rev_id = AL_PCIE_REV_ID_4;
> +		break;
> +	default:
> +		dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
> +			dev_rev_id);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);

Consider s/dev_dbg/pci_dbg/g

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

Consider s/dev_err/pci_err/g

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

From what I'm seeing you commonly use writel() and readl() with a common 
base address, such as pcie->controller_base + AXI_BASE_OFFSET.
I'd suggest to creating a writel and readl with that offset built-in.

> +}
> +
> +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> +					   unsigned int busnr,
> +					   unsigned int devfn)
> +{
> +	void __iomem *pci_base_addr;

Consider passing this variable declaration to the bottom, following the 
reverse tree order.

> +	struct pcie_port *pp = &pcie->pci->pp;
> +	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie->target_bus_cfg;
> +	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> +	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> +
> +	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> +					 (busnr_ecam << 20) +
> +					 PCIE_ECAM_DEVFN(devfn));
> +
> +	if (busnr_reg != target_bus_cfg->reg_val) {
> +		dev_dbg(pcie->pci->dev, "Changing target bus busnum val from 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 int al_pcie_config_prepare(struct al_pcie *pcie)
> +{
> +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> +	struct pcie_port *pp = &pcie->pci->pp;
> +	unsigned int ecam_bus_mask;
> +	u8 secondary_bus;
> +	u8 subordinate_bus;
> +	void __iomem *cfg_control_addr;
> +	u32 cfg_control;
> +	u32 reg;
> +
> +	target_bus_cfg = &pcie->target_bus_cfg;
> +
> +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> +	if (ecam_bus_mask > 255) {
> +		dev_warn(pcie->dev, "ECAM window size is larger than 256MB. Cutting off at 256\n");
> +		ecam_bus_mask = 255;
> +	}
> +
> +	/* This portion is taken from the transaction address */
> +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> +	/* This portion is taken from the cfg_target_bus reg */
> +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg->reg_mask;
> +
> +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> +			       target_bus_cfg->reg_mask);
> +
> +	secondary_bus = pp->busn->start + 1;
> +	subordinate_bus = pp->busn->end;
> +
> +	/* Set the valid values of secondary and subordinate buses */
> +	cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
> +			   pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> +
> +	cfg_control = readl(cfg_control_addr);
> +
> +	reg = cfg_control &
> +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> +
> +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> +
> +	writel(reg, cfg_control_addr);
> +
> +	return 0;
> +}
> +
> +static int al_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct al_pcie *pcie = to_al_pcie(pci);
> +	int link_up;
> +	int rc;
> +
> +	link_up = dw_pcie_link_up(pci);
> +	if (!link_up) {
> +		dev_err(pci->dev, "link is not up!\n");
> +		return -ENOLINK;
> +	}
> +
> +	dev_info(pci->dev, "link is up\n");

Consider s/dev_info/pci_info/g

> +
> +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> +	if (rc)
> +		return rc;
> +
> +	rc = al_pcie_reg_offsets_set(pcie);
> +	if (rc)
> +		return rc;
> +
> +	rc = al_pcie_config_prepare(pcie);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> +	.rd_other_conf = al_pcie_rd_other_conf,
> +	.wr_other_conf = al_pcie_wr_other_conf,
> +	.host_init = al_pcie_host_init,
> +};
> +
> +static int al_add_pcie_port(struct pcie_port *pp,
> +			    struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	pp->ops = &al_pcie_host_ops;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize host\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +};
> +
> +static int al_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct al_pcie *al_pcie;
> +	struct dw_pcie *pci;
> +	struct resource *dbi_res;
> +	struct resource *controller_res;
> +	struct resource *ecam_res;
> +	int ret;

Please sort the variables following the reverse tree order.

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

> +	if (ret)
> +		return ret;
> +
> +	return 0;

Those operations are redundant, aren't they? They can be replaced just 
by:

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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode
  2019-07-18  9:47 ` [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode Jonathan Chocron
@ 2019-07-19  9:15   ` Gustavo Pimentel
  0 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-19  9:15 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi, bhelgaas, jingoohan1,
	gustavo.pimentel@synopsys.com, robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree

On Thu, Jul 18, 2019 at 10:47:17, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

> Some PCIe controllers can be set to either Host or EP according to some
> early boot FW. To make sure there is no discrepancy (e.g. FW configured
> the port to EP mode while the DT specifies it as a host bridge or vice
> versa), a check has been added for each mode.
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c   | 8 ++++++++
>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 2bf5a35c0570..00e59a134b93 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -531,6 +531,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  	int ret;
>  	u32 reg;
>  	void *addr;
> +	u8 hdr_type;
>  	unsigned int nbars;
>  	unsigned int offset;
>  	struct pci_epc *epc;
> @@ -543,6 +544,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>  		return -EINVAL;
>  	}
>  
> +	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> +	if (hdr_type != PCI_HEADER_TYPE_NORMAL) {
> +		dev_err(pci->dev, "PCIe controller is not set to EP mode (hdr_type:0x%x)!\n",
> +			hdr_type);
> +		return -EIO;
> +	}
> +
>  	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to read *num-ib-windows* property\n");
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index f93252d0da5b..d2ca748e4c85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -323,6 +323,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  	struct pci_bus *child;
>  	struct pci_host_bridge *bridge;
>  	struct resource *cfg_res;
> +	u8 hdr_type;
>  	int ret;
>  
>  	raw_spin_lock_init(&pci->pp.lock);
> @@ -396,6 +397,13 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> +	hdr_type = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> +	if (hdr_type != PCI_HEADER_TYPE_BRIDGE) {
> +		dev_err(pci->dev, "PCIe controller is not set to bridge type (hdr_type: 0x%x)!\n",
> +			hdr_type);
> +		return -EIO;
> +	}
> +
>  	pp->mem_base = pp->mem->start;
>  
>  	if (!pp->va_cfg0_base) {
> -- 
> 2.17.1

It doesn't harm.
Thanks.

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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support for Amazon's Annapurna Labs Root Port
  2019-07-18  9:45 ` [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support " Jonathan Chocron
@ 2019-07-19  9:17   ` Gustavo Pimentel
  0 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-19  9:17 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi, bhelgaas, jingoohan1,
	gustavo.pimentel@synopsys.com, robh+dt, mark.rutland
  Cc: dwmw, benh, alisaidi, ronenk, barakw, talel, hanochu, hhhawa,
	linux-pci, linux-kernel, devicetree

On Thu, Jul 18, 2019 at 10:45:27, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

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


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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-19  8:55   ` Gustavo Pimentel
@ 2019-07-21 15:08     ` Chocron, Jonathan
  2019-07-22  0:47       ` Benjamin Herrenschmidt
                         ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Chocron, Jonathan @ 2019-07-21 15:08 UTC (permalink / raw)
  To: jingoohan1, mark.rutland, lorenzo.pieralisi, Gustavo.Pimentel,
	bhelgaas, robh+dt
  Cc: linux-kernel, Woodhouse, David, Hanoch, Uri, devicetree,
	Wasserstrom, Barak, Saidi, Ali, Hawa, Hanna, Shenhar, Talel,
	Krupnik, Ronen, linux-pci, benh

On Fri, 2019-07-19 at 08:55 +0000, Gustavo Pimentel wrote:
> On Thu, Jul 18, 2019 at 10:47:16, 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 | 373
> > +++++++++++++++++++++++++++
> >  2 files changed, 385 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..40555532fb9a 100644
> > --- a/drivers/pci/controller/dwc/pcie-al.c
> > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > @@ -91,3 +91,376 @@ 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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > *rev_id)
> > +{
> > +	void __iomem *dev_rev_id_addr;
> > +	u32 dev_rev_id;
> > +
> > +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > DEVICE_REV_ID);
> > +
> > +	dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> > +			       readl(dev_rev_id_addr));
> > +	switch (dev_rev_id) {
> > +	case DEVICE_REV_ID_DEV_ID_X4:
> > +		*rev_id = AL_PCIE_REV_ID_2;
> > +		break;
> > +	case DEVICE_REV_ID_DEV_ID_X8:
> > +		*rev_id = AL_PCIE_REV_ID_3;
> > +		break;
> > +	case DEVICE_REV_ID_DEV_ID_X16:
> > +		*rev_id = AL_PCIE_REV_ID_4;
> > +		break;
> > +	default:
> > +		dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
> > +			dev_rev_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);
> 
> Consider s/dev_dbg/pci_dbg/g
> 
There is no struct pci_dev context (the dev belongs to the
platform_device).

> > +
> > +	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);
> 
> Consider s/dev_err/pci_err/g
> 
Same as above.

> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > +					  u8 target_bus,
> > +					  u8 mask_target_bus)
> > +{
> > +	void __iomem *cfg_control_addr;
> > +	u32 reg;
> > +
> > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> > +
> > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > +			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > +
> > +			   CFG_TARGET_BUS);
> > +
> > +	writel(reg, cfg_control_addr);
> 
> From what I'm seeing you commonly use writel() and readl() with a
> common 
> base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> I'd suggest to creating a writel and readl with that offset built-in.
> 
I prefer to keep it generic, since in future revisions we might want to
access regs which are not in the AXI region. You think I should add
wrappers which simply hide the pcie->controller_base part?

> > +}
> > +
> > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > +					   unsigned int busnr,
> > +					   unsigned int devfn)
> > +{
> > +	void __iomem *pci_base_addr;
> 
> Consider passing this variable declaration to the bottom, following
> the 
> reverse tree order.
> 
Done. Moved 'struct pcie_port *pp' as well.

> > +	struct pcie_port *pp = &pcie->pci->pp;
> > +	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > >target_bus_cfg;
> > +	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > +	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > +
> > +	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > +					 (busnr_ecam << 20) +
> > +					 PCIE_ECAM_DEVFN(devfn));
> > +
> > +	if (busnr_reg != target_bus_cfg->reg_val) {
> > +		dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > from 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 int al_pcie_config_prepare(struct al_pcie *pcie)
> > +{
> > +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> > +	struct pcie_port *pp = &pcie->pci->pp;
> > +	unsigned int ecam_bus_mask;
> > +	u8 secondary_bus;
> > +	u8 subordinate_bus;
> > +	void __iomem *cfg_control_addr;
> > +	u32 cfg_control;
> > +	u32 reg;
> > +
> > +	target_bus_cfg = &pcie->target_bus_cfg;
> > +
> > +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > +	if (ecam_bus_mask > 255) {
> > +		dev_warn(pcie->dev, "ECAM window size is larger than
> > 256MB. Cutting off at 256\n");
> > +		ecam_bus_mask = 255;
> > +	}
> > +
> > +	/* This portion is taken from the transaction address */
> > +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> > +	/* This portion is taken from the cfg_target_bus reg */
> > +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > >reg_mask;
> > +
> > +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> > +			       target_bus_cfg->reg_mask);
> > +
> > +	secondary_bus = pp->busn->start + 1;
> > +	subordinate_bus = pp->busn->end;
> > +
> > +	/* Set the valid values of secondary and subordinate buses */
> > +	cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
> > +			   pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> > +
> > +	cfg_control = readl(cfg_control_addr);
> > +
> > +	reg = cfg_control &
> > +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> > +
> > +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> > +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> > +
> > +	writel(reg, cfg_control_addr);
> > +
> > +	return 0;
> > +}
> > +
> > +static int al_pcie_host_init(struct pcie_port *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct al_pcie *pcie = to_al_pcie(pci);
> > +	int link_up;
> > +	int rc;
> > +
> > +	link_up = dw_pcie_link_up(pci);
> > +	if (!link_up) {
> > +		dev_err(pci->dev, "link is not up!\n");
> > +		return -ENOLINK;
> > +	}
> > +
> > +	dev_info(pci->dev, "link is up\n");
> 
> Consider s/dev_info/pci_info/g
> 
Same as the response above.

> > +
> > +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = al_pcie_reg_offsets_set(pcie);
> > +	if (rc)
> > +		return rc;
> > +
> > +	rc = al_pcie_config_prepare(pcie);
> > +	if (rc)
> > +		return rc;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > +	.rd_other_conf = al_pcie_rd_other_conf,
> > +	.wr_other_conf = al_pcie_wr_other_conf,
> > +	.host_init = al_pcie_host_init,
> > +};
> > +
> > +static int al_add_pcie_port(struct pcie_port *pp,
> > +			    struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	pp->ops = &al_pcie_host_ops;
> > +
> > +	ret = dw_pcie_host_init(pp);
> > +	if (ret) {
> > +		dev_err(dev, "failed to initialize host\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > +};
> > +
> > +static int al_pcie_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct al_pcie *al_pcie;
> > +	struct dw_pcie *pci;
> > +	struct resource *dbi_res;
> > +	struct resource *controller_res;
> > +	struct resource *ecam_res;
> > +	int ret;
> 
> Please sort the variables following the reverse tree order.
> 
Done. 

I'd think that it would make sense to group variables which have a
common characteristic (e.g. resources read from the DT), even if it
mildly breaks the convention (as long as the general frame is longest
to shortest). Does this sound ok?

BTW, I couldn't find any documentation regarding the reverse-tree
convention, do you have a pointer to some?

> > +
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Those operations are redundant, aren't they? They can be replaced
> just 
> by:
> 
> return ret;
> 

Ack.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-21 15:08     ` Chocron, Jonathan
@ 2019-07-22  0:47       ` Benjamin Herrenschmidt
  2019-07-22  8:54       ` Gustavo Pimentel
  2019-07-22 21:15       ` Bjorn Helgaas
  2 siblings, 0 replies; 19+ messages in thread
From: Benjamin Herrenschmidt @ 2019-07-22  0:47 UTC (permalink / raw)
  To: Chocron, Jonathan, jingoohan1, mark.rutland, lorenzo.pieralisi,
	Gustavo.Pimentel, bhelgaas, robh+dt
  Cc: linux-kernel, Woodhouse, David, Hanoch, Uri, devicetree,
	Wasserstrom, Barak, Saidi, Ali, Hawa, Hanna, Shenhar, Talel,
	Krupnik, Ronen, linux-pci

On Sun, 2019-07-21 at 15:08 +0000, Chocron, Jonathan wrote:
> > Please sort the variables following the reverse tree order.
> > 
> 
> Done. 
> 
> I'd think that it would make sense to group variables which have a
> common characteristic (e.g. resources read from the DT), even if it
> mildly breaks the convention (as long as the general frame is longest
> to shortest). Does this sound ok?
> 
> BTW, I couldn't find any documentation regarding the reverse-tree
> convention, do you have a pointer to some?

It's a complete stupidity that people who have nothing better to
comment about keep throwing at you when you are trying to get stuff
working.

Yes, we should avoid ugly stuff such as

	int rc;
	struct something_very_long foo;

But this definitely should come second to saner ordering such as the
one you propose of grouping related things together. At least IMHO.

You'll notice that the kernel these days attract way more people
interested in commenting to death on various minor points of coding
style than actual worthwhile technical comments on the code.

Cheers,
Ben.



^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-21 15:08     ` Chocron, Jonathan
  2019-07-22  0:47       ` Benjamin Herrenschmidt
@ 2019-07-22  8:54       ` Gustavo Pimentel
  2019-07-22 15:38         ` Chocron, Jonathan
  2019-07-22 21:15       ` Bjorn Helgaas
  2 siblings, 1 reply; 19+ messages in thread
From: Gustavo Pimentel @ 2019-07-22  8:54 UTC (permalink / raw)
  To: Chocron, Jonathan, jingoohan1, mark.rutland, lorenzo.pieralisi,
	Gustavo.Pimentel, bhelgaas, robh+dt
  Cc: linux-kernel, Woodhouse, David, Hanoch, Uri, devicetree,
	Wasserstrom, Barak, Saidi, Ali, Hawa, Hanna, Shenhar, Talel,
	Krupnik, Ronen, linux-pci, benh

On Sun, Jul 21, 2019 at 16:8:18, Chocron, Jonathan <jonnyc@amazon.com> 
wrote:

> On Fri, 2019-07-19 at 08:55 +0000, Gustavo Pimentel wrote:
> > On Thu, Jul 18, 2019 at 10:47:16, 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 | 373
> > > +++++++++++++++++++++++++++
> > >  2 files changed, 385 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..40555532fb9a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-al.c
> > > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > > @@ -91,3 +91,376 @@ 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 int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > > *rev_id)
> > > +{
> > > +	void __iomem *dev_rev_id_addr;
> > > +	u32 dev_rev_id;
> > > +
> > > +	dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > > >controller_base +
> > > +			  AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > > DEVICE_REV_ID);
> > > +
> > > +	dev_rev_id = FIELD_GET(DEVICE_REV_ID_DEV_ID_MASK,
> > > +			       readl(dev_rev_id_addr));
> > > +	switch (dev_rev_id) {
> > > +	case DEVICE_REV_ID_DEV_ID_X4:
> > > +		*rev_id = AL_PCIE_REV_ID_2;
> > > +		break;
> > > +	case DEVICE_REV_ID_DEV_ID_X8:
> > > +		*rev_id = AL_PCIE_REV_ID_3;
> > > +		break;
> > > +	case DEVICE_REV_ID_DEV_ID_X16:
> > > +		*rev_id = AL_PCIE_REV_ID_4;
> > > +		break;
> > > +	default:
> > > +		dev_err(pcie->dev, "Unsupported dev_rev_id (0x%x)\n",
> > > +			dev_rev_id);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	dev_dbg(pcie->dev, "dev_rev_id: 0x%x\n", dev_rev_id);
> > 
> > Consider s/dev_dbg/pci_dbg/g
> > 
> There is no struct pci_dev context (the dev belongs to the
> platform_device).

It seems so. It sucks...
Disregard this then.

> 
> > > +
> > > +	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);
> > 
> > Consider s/dev_err/pci_err/g
> > 
> Same as above.
> 
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > > +					  u8 target_bus,
> > > +					  u8 mask_target_bus)
> > > +{
> > > +	void __iomem *cfg_control_addr;
> > > +	u32 reg;
> > > +
> > > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK, mask_target_bus) |
> > > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK, target_bus);
> > > +
> > > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > > >controller_base +
> > > +			   AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > > +
> > > +			   CFG_TARGET_BUS);
> > > +
> > > +	writel(reg, cfg_control_addr);
> > 
> > From what I'm seeing you commonly use writel() and readl() with a
> > common 
> > base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> > I'd suggest to creating a writel and readl with that offset built-in.
> > 
> I prefer to keep it generic, since in future revisions we might want to
> access regs which are not in the AXI region. You think I should add
> wrappers which simply hide the pcie->controller_base part?

I and other developers typically do that, but it's a suggestion. IMHO it 
helps to keep the code cleaner and more readable.

> 
> > > +}
> > > +
> > > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > > +					   unsigned int busnr,
> > > +					   unsigned int devfn)
> > > +{
> > > +	void __iomem *pci_base_addr;
> > 
> > Consider passing this variable declaration to the bottom, following
> > the 
> > reverse tree order.
> > 
> Done. Moved 'struct pcie_port *pp' as well.
> 
> > > +	struct pcie_port *pp = &pcie->pci->pp;
> > > +	struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > > >target_bus_cfg;
> > > +	unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > > +	unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > > +
> > > +	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > > +					 (busnr_ecam << 20) +
> > > +					 PCIE_ECAM_DEVFN(devfn));
> > > +
> > > +	if (busnr_reg != target_bus_cfg->reg_val) {
> > > +		dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > > from 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 int al_pcie_config_prepare(struct al_pcie *pcie)
> > > +{
> > > +	struct al_pcie_target_bus_cfg *target_bus_cfg;
> > > +	struct pcie_port *pp = &pcie->pci->pp;
> > > +	unsigned int ecam_bus_mask;
> > > +	u8 secondary_bus;
> > > +	u8 subordinate_bus;
> > > +	void __iomem *cfg_control_addr;
> > > +	u32 cfg_control;
> > > +	u32 reg;
> > > +
> > > +	target_bus_cfg = &pcie->target_bus_cfg;
> > > +
> > > +	ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > > +	if (ecam_bus_mask > 255) {
> > > +		dev_warn(pcie->dev, "ECAM window size is larger than
> > > 256MB. Cutting off at 256\n");
> > > +		ecam_bus_mask = 255;
> > > +	}
> > > +
> > > +	/* This portion is taken from the transaction address */
> > > +	target_bus_cfg->ecam_mask = ecam_bus_mask;
> > > +	/* This portion is taken from the cfg_target_bus reg */
> > > +	target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > > +	target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > > >reg_mask;
> > > +
> > > +	al_pcie_target_bus_set(pcie, target_bus_cfg->reg_val,
> > > +			       target_bus_cfg->reg_mask);
> > > +
> > > +	secondary_bus = pp->busn->start + 1;
> > > +	subordinate_bus = pp->busn->end;
> > > +
> > > +	/* Set the valid values of secondary and subordinate buses */
> > > +	cfg_control_addr = pcie->controller_base + AXI_BASE_OFFSET +
> > > +			   pcie->reg_offsets.ob_ctrl + CFG_CONTROL;
> > > +
> > > +	cfg_control = readl(cfg_control_addr);
> > > +
> > > +	reg = cfg_control &
> > > +	      ~(CFG_CONTROL_SEC_BUS_MASK | CFG_CONTROL_SUBBUS_MASK);
> > > +
> > > +	reg |= FIELD_PREP(CFG_CONTROL_SUBBUS_MASK, subordinate_bus) |
> > > +	       FIELD_PREP(CFG_CONTROL_SEC_BUS_MASK, secondary_bus);
> > > +
> > > +	writel(reg, cfg_control_addr);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int al_pcie_host_init(struct pcie_port *pp)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	struct al_pcie *pcie = to_al_pcie(pci);
> > > +	int link_up;
> > > +	int rc;
> > > +
> > > +	link_up = dw_pcie_link_up(pci);
> > > +	if (!link_up) {
> > > +		dev_err(pci->dev, "link is not up!\n");
> > > +		return -ENOLINK;
> > > +	}
> > > +
> > > +	dev_info(pci->dev, "link is up\n");
> > 
> > Consider s/dev_info/pci_info/g
> > 
> Same as the response above.
> 
> > > +
> > > +	rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	rc = al_pcie_reg_offsets_set(pcie);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	rc = al_pcie_config_prepare(pcie);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > > +	.rd_other_conf = al_pcie_rd_other_conf,
> > > +	.wr_other_conf = al_pcie_wr_other_conf,
> > > +	.host_init = al_pcie_host_init,
> > > +};
> > > +
> > > +static int al_add_pcie_port(struct pcie_port *pp,
> > > +			    struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	int ret;
> > > +
> > > +	pp->ops = &al_pcie_host_ops;
> > > +
> > > +	ret = dw_pcie_host_init(pp);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to initialize host\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct dw_pcie_ops dw_pcie_ops = {
> > > +};
> > > +
> > > +static int al_pcie_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct al_pcie *al_pcie;
> > > +	struct dw_pcie *pci;
> > > +	struct resource *dbi_res;
> > > +	struct resource *controller_res;
> > > +	struct resource *ecam_res;
> > > +	int ret;
> > 
> > Please sort the variables following the reverse tree order.
> > 
> Done. 
> 
> I'd think that it would make sense to group variables which have a
> common characteristic (e.g. resources read from the DT), even if it
> mildly breaks the convention (as long as the general frame is longest
> to shortest). Does this sound ok?
> 
> BTW, I couldn't find any documentation regarding the reverse-tree
> convention, do you have a pointer to some?

There isn't as far as I know, but it's a convention normally used in 
several subsystems.
I suppose it's a way to try to keep some organization along with the code 

style. IMHO it doesn't harm, but it's just a suggestion. 😊

> 
> > > +
> > > +	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);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > 
> > Those operations are redundant, aren't they? They can be replaced
> > just 
> > by:
> > 
> > return ret;
> > 
> 
> Ack.

Thanks.

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



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-22  8:54       ` Gustavo Pimentel
@ 2019-07-22 15:38         ` Chocron, Jonathan
  0 siblings, 0 replies; 19+ messages in thread
From: Chocron, Jonathan @ 2019-07-22 15:38 UTC (permalink / raw)
  To: jingoohan1, mark.rutland, lorenzo.pieralisi, Gustavo.Pimentel,
	bhelgaas, robh+dt
  Cc: linux-kernel, Woodhouse, David, Hanoch, Uri, devicetree,
	Wasserstrom, Barak, Saidi, Ali, Hawa, Hanna, Shenhar, Talel,
	Krupnik, Ronen, linux-pci, benh, Chocron, Jonathan

On Mon, 2019-07-22 at 08:54 +0000, Gustavo Pimentel wrote:
> 
> > 
> > > > +static inline void al_pcie_target_bus_set(struct al_pcie
> > > > *pcie,
> > > > +					  u8 target_bus,
> > > > +					  u8 mask_target_bus)
> > > > +{
> > > > +	void __iomem *cfg_control_addr;
> > > > +	u32 reg;
> > > > +
> > > > +	reg = FIELD_PREP(CFG_TARGET_BUS_MASK_MASK,
> > > > mask_target_bus) |
> > > > +	      FIELD_PREP(CFG_TARGET_BUS_BUSNUM_MASK,
> > > > target_bus);
> > > > +
> > > > +	cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > > > > controller_base +
> > > > 
> > > > +			   AXI_BASE_OFFSET + pcie-
> > > > >reg_offsets.ob_ctrl
> > > > +
> > > > +			   CFG_TARGET_BUS);
> > > > +
> > > > +	writel(reg, cfg_control_addr);
> > > 
> > > From what I'm seeing you commonly use writel() and readl() with a
> > > common 
> > > base address, such as pcie->controller_base + AXI_BASE_OFFSET.
> > > I'd suggest to creating a writel and readl with that offset
> > > built-in.
> > > 
> > 
> > I prefer to keep it generic, since in future revisions we might
> > want to
> > access regs which are not in the AXI region. You think I should add
> > wrappers which simply hide the pcie->controller_base part?
> 
> I and other developers typically do that, but it's a suggestion. IMHO
> it 
> helps to keep the code cleaner and more readable.
> 

Added al_pcie_controller_readl/writel wrappers.

-- 
Thanks,
   Jonathan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/8] PCI: al: Add support for DW based driver type
  2019-07-21 15:08     ` Chocron, Jonathan
  2019-07-22  0:47       ` Benjamin Herrenschmidt
  2019-07-22  8:54       ` Gustavo Pimentel
@ 2019-07-22 21:15       ` Bjorn Helgaas
  2 siblings, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2019-07-22 21:15 UTC (permalink / raw)
  To: Chocron, Jonathan
  Cc: jingoohan1, mark.rutland, lorenzo.pieralisi, Gustavo.Pimentel,
	robh+dt, linux-kernel, Woodhouse, David, Hanoch, Uri, devicetree,
	Wasserstrom, Barak, Saidi, Ali, Hawa, Hanna, Shenhar, Talel,
	Krupnik, Ronen, linux-pci, benh

On Sun, Jul 21, 2019 at 03:08:18PM +0000, Chocron, Jonathan wrote:
> On Fri, 2019-07-19 at 08:55 +0000, Gustavo Pimentel wrote:
> > On Thu, Jul 18, 2019 at 10:47:16, Jonathan Chocron <jonnyc@amazon.com> wrote:

> > > +static int al_pcie_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct al_pcie *al_pcie;
> > > +	struct dw_pcie *pci;
> > > +	struct resource *dbi_res;
> > > +	struct resource *controller_res;
> > > +	struct resource *ecam_res;
> > > +	int ret;
> > 
> > Please sort the variables following the reverse tree order.
> > 
> Done. 
> 
> I'd think that it would make sense to group variables which have a
> common characteristic (e.g. resources read from the DT), even if it
> mildly breaks the convention (as long as the general frame is longest
> to shortest). Does this sound ok?
> 
> BTW, I couldn't find any documentation regarding the reverse-tree
> convention, do you have a pointer to some?

What I personally do is sort declarations in the order they're used.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-07-22 21:15 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-18  9:45 [PATCH v2 0/8] Amazon's Annapurna Labs DT-based PCIe host controller driver Jonathan Chocron
2019-07-18  9:45 ` [PATCH v2 1/8] PCI: Add Amazon's Annapurna Labs vendor ID Jonathan Chocron
2019-07-18  9:45 ` [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports Jonathan Chocron
2019-07-19  8:25   ` Gustavo Pimentel
2019-07-18  9:45 ` [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port Jonathan Chocron
2019-07-19  8:24   ` Gustavo Pimentel
2019-07-18  9:45 ` [PATCH v2 4/8] PCI: Add quirk to disable MSI-X support " Jonathan Chocron
2019-07-19  9:17   ` Gustavo Pimentel
2019-07-18  9:47 ` [PATCH v2 5/8] dt-bindings: PCI: Add Amazon's Annapurna Labs PCIe host bridge binding Jonathan Chocron
2019-07-18  9:47 ` [PATCH v2 6/8] PCI: al: Add support for DW based driver type Jonathan Chocron
2019-07-19  8:55   ` Gustavo Pimentel
2019-07-21 15:08     ` Chocron, Jonathan
2019-07-22  0:47       ` Benjamin Herrenschmidt
2019-07-22  8:54       ` Gustavo Pimentel
2019-07-22 15:38         ` Chocron, Jonathan
2019-07-22 21:15       ` Bjorn Helgaas
2019-07-18  9:47 ` [PATCH v2 7/8] PCI: dw: Add validation that PCIe core is set to correct mode Jonathan Chocron
2019-07-19  9:15   ` Gustavo Pimentel
2019-07-18  9:47 ` [PATCH v2 8/8] PCI: dw: Add support for PCI_PROBE_ONLY/PCI_REASSIGN_ALL_BUS flags Jonathan Chocron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).