[PATCH v2 0/6] PCI: Add support for J7200 and AM64

[PATCH v2 0/6] PCI: Add support for J7200 and AM64

[Kishon Vijay Abraham I]

This series adds the compatible specific to J7200 and AM64 and
applies the erratas and configuration specific to them.

This series also includes Nadeem's patch that adds a quirk in
Cadence driver which is used by J7200 [1].

The DT binding for both J7200 and AM64 is already merged.

v1 of the patch series can be found at [2]

Changes from v1:
1) As suggested by Bjorn, used unsigned int :1, instead of bool for
structure members
2) Removed using unnecessary local variables and also fixed some
code alignment

[1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com
[2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com

Kishon Vijay Abraham I (5):
  PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool
  PCI: j721e: Add PCIe support for J7200
  PCI: j721e: Add PCIe support for AM64
  misc: pci_endpoint_test: Do not request or allocate IRQs in probe
  misc: pci_endpoint_test: Add deviceID for AM64 and J7200

Nadeem Athani (1):
  PCI: cadence: Add quirk flag to set minimum delay in LTSSM
    Detect.Quiet state

 drivers/misc/pci_endpoint_test.c              | 27 ++++++--
 drivers/pci/controller/cadence/pci-j721e.c    | 61 +++++++++++++++++--
 .../pci/controller/cadence/pcie-cadence-ep.c  |  4 ++
 .../controller/cadence/pcie-cadence-host.c    |  3 +
 drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++
 drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++-
 6 files changed, 117 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v2 1/6] PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool

[Kishon Vijay Abraham I]

No functional change. As we are intending to add additional 1-bit
members in struct j721e_pcie_data/struct cdns_pcie_rc, use bitfields
instead of bool since it takes less space. As discussed in [1],
the preference is to use bitfileds instead of bool inside structures.

[1] -> https://lore.kernel.org/linux-fsdevel/CA+55aFzKQ6Pj18TB8p4Yr0M4t+S+BsiHH=BJNmn=76-NcjTj-g@mail.gmail.com/

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c    | 2 +-
 drivers/pci/controller/cadence/pcie-cadence.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 35e61048e133..0c5813b230b4 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -66,7 +66,7 @@ enum j721e_pcie_mode {
 
 struct j721e_pcie_data {
 	enum j721e_pcie_mode	mode;
-	bool quirk_retrain_flag;
+	unsigned int		quirk_retrain_flag:1;
 };
 
 static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 927b49e42997..a978a2eeedad 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -308,7 +308,7 @@ struct cdns_pcie_rc {
 	u32			vendor_id;
 	u32			device_id;
 	bool			avail_ib_bar[CDNS_PCIE_RP_MAX_IB];
-	bool                    quirk_retrain_flag;
+	unsigned int		quirk_retrain_flag:1;
 };
 
 /**
-- 
2.17.1

[PATCH v2 2/6] PCI: cadence: Add quirk flag to set minimum delay in LTSSM Detect.Quiet state

[Kishon Vijay Abraham I]

From: Nadeem Athani <nadeem@cadence.com>

PCIe fails to link up if SERDES lanes not used by PCIe are assigned to
another protocol. For example, link training fails if lanes 2 and 3 are
assigned to another protocol while lanes 0 and 1 are used for PCIe to
form a two lane link. This failure is due to an incorrect tie-off on an
internal status signal indicating electrical idle.

Status signals going from SERDES to PCIe Controller are tied-off when a
lane is not assigned to PCIe. Signal indicating electrical idle is
incorrectly tied-off to a state that indicates non-idle. As a result,
PCIe sees unused lanes to be out of electrical idle and this causes
LTSSM to exit Detect.Quiet state without waiting for 12ms timeout to
occur. If a receiver is not detected on the first receiver detection
attempt in Detect.Active state, LTSSM goes back to Detect.Quiet and
again moves forward to Detect.Active state without waiting for 12ms as
required by PCIe base specification. Since wait time in Detect.Quiet is
skipped, multiple receiver detect operations are performed back-to-back
without allowing time for capacitance on the transmit lines to
discharge. This causes subsequent receiver detection to always fail even
if a receiver gets connected eventually.

Add a quirk flag "quirk_detect_quiet_flag" to program the minimum
time the LTSSM should wait on entering Detect.Quiet state here.
This has to be set for J7200 as it has an incorrect tie-off on unused
lanes.

Signed-off-by: Nadeem Athani <nadeem@cadence.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../pci/controller/cadence/pcie-cadence-ep.c    |  4 ++++
 .../pci/controller/cadence/pcie-cadence-host.c  |  3 +++
 drivers/pci/controller/cadence/pcie-cadence.c   | 17 +++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h   | 15 +++++++++++++++
 4 files changed, 39 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 791915054ff4..e05c157afef0 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -804,6 +804,10 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 	ep->irq_pci_addr = CDNS_PCIE_EP_IRQ_PCI_ADDR_NONE;
 	/* Reserve region 0 for IRQs */
 	set_bit(0, &ep->ob_region_map);
+
+	if (ep->quirk_detect_quiet_flag)
+		cdns_pcie_detect_quiet_min_delay_set(&ep->pcie);
+
 	spin_lock_init(&ep->lock);
 
 	return 0;
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index ae1c55503513..fb96d37a135c 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -498,6 +498,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 		return PTR_ERR(rc->cfg_base);
 	rc->cfg_res = res;
 
+	if (rc->quirk_detect_quiet_flag)
+		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
+
 	ret = cdns_pcie_start_link(pcie);
 	if (ret) {
 		dev_err(dev, "Failed to start link\n");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 3c3646502d05..65b6c8bed0d4 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -7,6 +7,23 @@
 
 #include "pcie-cadence.h"
 
+void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
+{
+	u32 delay = 0x3;
+	u32 ltssm_control_cap;
+
+	/*
+	 * Set the LTSSM Detect Quiet state min. delay to 2ms.
+	 */
+
+	ltssm_control_cap = cdns_pcie_readl(pcie, CDNS_PCIE_LTSSM_CONTROL_CAP);
+	ltssm_control_cap = ((ltssm_control_cap &
+			    ~CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK) |
+			    CDNS_PCIE_DETECT_QUIET_MIN_DELAY(delay));
+
+	cdns_pcie_writel(pcie, CDNS_PCIE_LTSSM_CONTROL_CAP, ltssm_control_cap);
+}
+
 void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
 				   u32 r, bool is_io,
 				   u64 cpu_addr, u64 pci_addr, size_t size)
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index a978a2eeedad..cda430d0f6e4 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -194,6 +194,14 @@
 /* AXI link down register */
 #define CDNS_PCIE_AT_LINKDOWN (CDNS_PCIE_AT_BASE + 0x0824)
 
+/* LTSSM Capabilities register */
+#define CDNS_PCIE_LTSSM_CONTROL_CAP             (CDNS_PCIE_LM_BASE + 0x0054)
+#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK  GENMASK(2, 1)
+#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY_SHIFT 1
+#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY(delay) \
+	 (((delay) << CDNS_PCIE_DETECT_QUIET_MIN_DELAY_SHIFT) & \
+	 CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK)
+
 enum cdns_pcie_rp_bar {
 	RP_BAR_UNDEFINED = -1,
 	RP_BAR0,
@@ -300,6 +308,7 @@ struct cdns_pcie {
  * @avail_ib_bar: Satus of RP_BAR0, RP_BAR1 and	RP_NO_BAR if it's free or
  *                available
  * @quirk_retrain_flag: Retrain link as quirk for PCIe Gen2
+ * @quirk_detect_quiet_flag: LTSSM Detect Quiet min delay set as quirk
  */
 struct cdns_pcie_rc {
 	struct cdns_pcie	pcie;
@@ -309,6 +318,7 @@ struct cdns_pcie_rc {
 	u32			device_id;
 	bool			avail_ib_bar[CDNS_PCIE_RP_MAX_IB];
 	unsigned int		quirk_retrain_flag:1;
+	unsigned int		quirk_detect_quiet_flag:1;
 };
 
 /**
@@ -341,6 +351,7 @@ struct cdns_pcie_epf {
  *        registers fields (RMW) accessible by both remote RC and EP to
  *        minimize time between read and write
  * @epf: Structure to hold info about endpoint function
+ * @quirk_detect_quiet_flag: LTSSM Detect Quiet min delay set as quirk
  */
 struct cdns_pcie_ep {
 	struct cdns_pcie	pcie;
@@ -355,6 +366,7 @@ struct cdns_pcie_ep {
 	/* protect writing to PCI_STATUS while raising legacy interrupts */
 	spinlock_t		lock;
 	struct cdns_pcie_epf	*epf;
+	unsigned int		quirk_detect_quiet_flag:1;
 };
 
 
@@ -515,6 +527,9 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 	return 0;
 }
 #endif
+
+void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
+
 void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
 				   u32 r, bool is_io,
 				   u64 cpu_addr, u64 pci_addr, size_t size);
-- 
2.17.1

[PATCH v2 3/6] PCI: j721e: Add PCIe support for J7200

[Kishon Vijay Abraham I]

J7200 has the same PCIe IP as in J721E with minor changes in the
wrapper. J7200 allows byte access of bridge configuration space
registers and the register field for LINK_DOWN interrupt is different.
J7200 also requires "quirk_detect_quiet_flag" to be set. Configure these
changes as part of driver data applicable only to J7200.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 40 +++++++++++++++++++---
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 0c5813b230b4..8e76f2e7e782 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -27,6 +27,7 @@
 #define STATUS_REG_SYS_2	0x508
 #define STATUS_CLR_REG_SYS_2	0x708
 #define LINK_DOWN		BIT(1)
+#define J7200_LINK_DOWN		BIT(10)
 
 #define J721E_PCIE_USER_CMD_STATUS	0x4
 #define LINK_TRAINING_ENABLE		BIT(0)
@@ -57,6 +58,7 @@ struct j721e_pcie {
 	struct cdns_pcie	*cdns_pcie;
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
+	u32			linkdown_irq_regfield;
 };
 
 enum j721e_pcie_mode {
@@ -67,6 +69,9 @@ enum j721e_pcie_mode {
 struct j721e_pcie_data {
 	enum j721e_pcie_mode	mode;
 	unsigned int		quirk_retrain_flag:1;
+	unsigned int		quirk_detect_quiet_flag:1;
+	u32			linkdown_irq_regfield;
+	unsigned int		byte_access_allowed:1;
 };
 
 static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
@@ -98,12 +103,12 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
 	u32 reg;
 
 	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2);
-	if (!(reg & LINK_DOWN))
+	if (!(reg & pcie->linkdown_irq_regfield))
 		return IRQ_NONE;
 
 	dev_err(dev, "LINK DOWN!\n");
 
-	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, LINK_DOWN);
+	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, pcie->linkdown_irq_regfield);
 	return IRQ_HANDLED;
 }
 
@@ -112,7 +117,7 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 	u32 reg;
 
 	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_2);
-	reg |= LINK_DOWN;
+	reg |= pcie->linkdown_irq_regfield;
 	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
 }
 
@@ -284,10 +289,25 @@ static struct pci_ops cdns_ti_pcie_host_ops = {
 static const struct j721e_pcie_data j721e_pcie_rc_data = {
 	.mode = PCI_MODE_RC,
 	.quirk_retrain_flag = true,
+	.byte_access_allowed = false,
+	.linkdown_irq_regfield = LINK_DOWN,
 };
 
 static const struct j721e_pcie_data j721e_pcie_ep_data = {
 	.mode = PCI_MODE_EP,
+	.linkdown_irq_regfield = LINK_DOWN,
+};
+
+static const struct j721e_pcie_data j7200_pcie_rc_data = {
+	.mode = PCI_MODE_RC,
+	.quirk_detect_quiet_flag = true,
+	.linkdown_irq_regfield = J7200_LINK_DOWN,
+	.byte_access_allowed = true,
+};
+
+static const struct j721e_pcie_data j7200_pcie_ep_data = {
+	.mode = PCI_MODE_EP,
+	.quirk_detect_quiet_flag = true,
 };
 
 static const struct of_device_id of_j721e_pcie_match[] = {
@@ -299,6 +319,14 @@ static const struct of_device_id of_j721e_pcie_match[] = {
 		.compatible = "ti,j721e-pcie-ep",
 		.data = &j721e_pcie_ep_data,
 	},
+	{
+		.compatible = "ti,j7200-pcie-host",
+		.data = &j7200_pcie_rc_data,
+	},
+	{
+		.compatible = "ti,j7200-pcie-ep",
+		.data = &j7200_pcie_ep_data,
+	},
 	{},
 };
 
@@ -332,6 +360,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	pcie->dev = dev;
 	pcie->mode = mode;
+	pcie->linkdown_irq_regfield =  data->linkdown_irq_regfield;
 
 	base = devm_platform_ioremap_resource_byname(pdev, "intd_cfg");
 	if (IS_ERR(base))
@@ -391,9 +420,11 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
-		bridge->ops = &cdns_ti_pcie_host_ops;
+		if (!data->byte_access_allowed)
+			bridge->ops = &cdns_ti_pcie_host_ops;
 		rc = pci_host_bridge_priv(bridge);
 		rc->quirk_retrain_flag = data->quirk_retrain_flag;
+		rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
 
 		cdns_pcie = &rc->pcie;
 		cdns_pcie->dev = dev;
@@ -459,6 +490,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			ret = -ENOMEM;
 			goto err_get_sync;
 		}
+		ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
 
 		cdns_pcie = &ep->pcie;
 		cdns_pcie->dev = dev;
-- 
2.17.1

[PATCH v2 4/6] PCI: j721e: Add PCIe support for AM64

[Kishon Vijay Abraham I]

AM64 has the same PCIe IP as in J7200 with certain erratas not
applicable (quirk_detect_quiet_flag). Add support for "ti,am64-pcie-host"
compatible and "ti,am64-pcie-ep" compatible that is specific to AM64.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 8e76f2e7e782..2ec037c43bd5 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -310,6 +310,17 @@ static const struct j721e_pcie_data j7200_pcie_ep_data = {
 	.quirk_detect_quiet_flag = true,
 };
 
+static const struct j721e_pcie_data am64_pcie_rc_data = {
+	.mode = PCI_MODE_RC,
+	.linkdown_irq_regfield = J7200_LINK_DOWN,
+	.byte_access_allowed = true,
+};
+
+static const struct j721e_pcie_data am64_pcie_ep_data = {
+	.mode = PCI_MODE_EP,
+	.linkdown_irq_regfield = J7200_LINK_DOWN,
+};
+
 static const struct of_device_id of_j721e_pcie_match[] = {
 	{
 		.compatible = "ti,j721e-pcie-host",
@@ -327,6 +338,14 @@ static const struct of_device_id of_j721e_pcie_match[] = {
 		.compatible = "ti,j7200-pcie-ep",
 		.data = &j7200_pcie_ep_data,
 	},
+	{
+		.compatible = "ti,am64-pcie-host",
+		.data = &am64_pcie_rc_data,
+	},
+	{
+		.compatible = "ti,am64-pcie-ep",
+		.data = &am64_pcie_ep_data,
+	},
 	{},
 };
 
-- 
2.17.1

[PATCH v2 5/6] misc: pci_endpoint_test: Do not request or allocate IRQs in probe

[Kishon Vijay Abraham I]

Allocation of IRQ vectors and requesting IRQ is done as part of
PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for
AM654 and J721E so that the user space test script has better control
of the devices for which the IRQs are configured. Since certain user
space scripts could rely on allocation of IRQ vectors during probe,
remove allocation of IRQs only for TI's K3 platform.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index c7ee34013485..9740f2a0e7cd 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -79,6 +79,9 @@
 #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
 #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
 
+#define is_j721e_pci_dev(pdev)         \
+		((pdev)->device == PCI_DEVICE_ID_TI_J721E)
+
 static DEFINE_IDA(pci_endpoint_test_ida);
 
 #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
@@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
-		err = -EINVAL;
-		goto err_disable_irq;
+	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
+		if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
+			err = -EINVAL;
+			goto err_disable_irq;
+		}
 	}
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
@@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_ida_remove;
 	}
 
-	if (!pci_endpoint_test_request_irq(test)) {
-		err = -EINVAL;
-		goto err_kfree_test_name;
+	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
+		if (!pci_endpoint_test_request_irq(test)) {
+			err = -EINVAL;
+			goto err_kfree_test_name;
+		}
 	}
 
 	misc_device = &test->miscdev;
-- 
2.17.1

[PATCH v2 6/6] misc: pci_endpoint_test: Add deviceID for AM64 and J7200

[Kishon Vijay Abraham I]

Add device ID specific to AM64 and J7200 in pci_endpoint_test so that
endpoints configured with those deviceIDs can use pci_endpoint_test
driver.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/misc/pci_endpoint_test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 9740f2a0e7cd..707cf9d7e8c0 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -69,6 +69,8 @@
 #define FLAG_USE_DMA				BIT(0)
 
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
+#define PCI_DEVICE_ID_TI_J7200			0xb00f
+#define PCI_DEVICE_ID_TI_AM64			0xb010
 #define PCI_DEVICE_ID_LS1088A			0x80c0
 
 #define is_am654_pci_dev(pdev)		\
@@ -976,6 +978,12 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
 	  .driver_data = (kernel_ulong_t)&j721e_data,
 	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J7200),
+	  .driver_data = (kernel_ulong_t)&j721e_data,
+	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_AM64),
+	  .driver_data = (kernel_ulong_t)&j721e_data,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
-- 
2.17.1

Re: [PATCH v2 5/6] misc: pci_endpoint_test: Do not request or allocate IRQs in probe

[Lorenzo Pieralisi]

On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote:
> Allocation of IRQ vectors and requesting IRQ is done as part of
> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for
> AM654 and J721E so that the user space test script has better control
> of the devices for which the IRQs are configured. Since certain user
> space scripts could rely on allocation of IRQ vectors during probe,
> remove allocation of IRQs only for TI's K3 platform.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)

I don't claim to understand the inner details of the endpoint test
device but it looks like this approach should be redesigned.

I don't believe using devices quirks is the best approach to
expose/remove a feature to userspace, this can soon become
unmaintenable.

Maybe you can elaborate a bit more on what the real issue is please ?

Thanks,
Lorenzo

> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index c7ee34013485..9740f2a0e7cd 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -79,6 +79,9 @@
>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
>  
> +#define is_j721e_pci_dev(pdev)         \
> +		((pdev)->device == PCI_DEVICE_ID_TI_J721E)
> +
>  static DEFINE_IDA(pci_endpoint_test_ida);
>  
>  #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  
>  	pci_set_master(pdev);
>  
> -	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
> -		err = -EINVAL;
> -		goto err_disable_irq;
> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
> +		if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
> +			err = -EINVAL;
> +			goto err_disable_irq;
> +		}
>  	}
>  
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  		goto err_ida_remove;
>  	}
>  
> -	if (!pci_endpoint_test_request_irq(test)) {
> -		err = -EINVAL;
> -		goto err_kfree_test_name;
> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
> +		if (!pci_endpoint_test_request_irq(test)) {
> +			err = -EINVAL;
> +			goto err_kfree_test_name;
> +		}
>  	}
>  
>  	misc_device = &test->miscdev;
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/6] PCI: cadence: Add quirk flag to set minimum delay in LTSSM Detect.Quiet state

[Lorenzo Pieralisi]

On Tue, Aug 03, 2021 at 01:19:28PM +0530, Kishon Vijay Abraham I wrote:
> From: Nadeem Athani <nadeem@cadence.com>
> 
> PCIe fails to link up if SERDES lanes not used by PCIe are assigned to
> another protocol. For example, link training fails if lanes 2 and 3 are
> assigned to another protocol while lanes 0 and 1 are used for PCIe to
> form a two lane link. This failure is due to an incorrect tie-off on an
> internal status signal indicating electrical idle.
> 
> Status signals going from SERDES to PCIe Controller are tied-off when a
> lane is not assigned to PCIe. Signal indicating electrical idle is
> incorrectly tied-off to a state that indicates non-idle. As a result,
> PCIe sees unused lanes to be out of electrical idle and this causes
> LTSSM to exit Detect.Quiet state without waiting for 12ms timeout to
> occur. If a receiver is not detected on the first receiver detection
> attempt in Detect.Active state, LTSSM goes back to Detect.Quiet and
> again moves forward to Detect.Active state without waiting for 12ms as
> required by PCIe base specification. Since wait time in Detect.Quiet is
> skipped, multiple receiver detect operations are performed back-to-back
> without allowing time for capacitance on the transmit lines to
> discharge. This causes subsequent receiver detection to always fail even
> if a receiver gets connected eventually.
> 
> Add a quirk flag "quirk_detect_quiet_flag" to program the minimum
> time the LTSSM should wait on entering Detect.Quiet state here.
> This has to be set for J7200 as it has an incorrect tie-off on unused
> lanes.
> 
> Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../pci/controller/cadence/pcie-cadence-ep.c    |  4 ++++
>  .../pci/controller/cadence/pcie-cadence-host.c  |  3 +++
>  drivers/pci/controller/cadence/pcie-cadence.c   | 17 +++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h   | 15 +++++++++++++++
>  4 files changed, 39 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 791915054ff4..e05c157afef0 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -804,6 +804,10 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  	ep->irq_pci_addr = CDNS_PCIE_EP_IRQ_PCI_ADDR_NONE;
>  	/* Reserve region 0 for IRQs */
>  	set_bit(0, &ep->ob_region_map);
> +
> +	if (ep->quirk_detect_quiet_flag)
> +		cdns_pcie_detect_quiet_min_delay_set(&ep->pcie);
> +
>  	spin_lock_init(&ep->lock);
>  
>  	return 0;
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index ae1c55503513..fb96d37a135c 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -498,6 +498,9 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  		return PTR_ERR(rc->cfg_base);
>  	rc->cfg_res = res;
>  
> +	if (rc->quirk_detect_quiet_flag)
> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
> +
>  	ret = cdns_pcie_start_link(pcie);
>  	if (ret) {
>  		dev_err(dev, "Failed to start link\n");
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
> index 3c3646502d05..65b6c8bed0d4 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -7,6 +7,23 @@
>  
>  #include "pcie-cadence.h"
>  
> +void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
> +{
> +	u32 delay = 0x3;
> +	u32 ltssm_control_cap;
> +
> +	/*
> +	 * Set the LTSSM Detect Quiet state min. delay to 2ms.
> +	 */
> +

Nit: empty line not needed

> +	ltssm_control_cap = cdns_pcie_readl(pcie, CDNS_PCIE_LTSSM_CONTROL_CAP);
> +	ltssm_control_cap = ((ltssm_control_cap &
> +			    ~CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK) |
> +			    CDNS_PCIE_DETECT_QUIET_MIN_DELAY(delay));
> +
> +	cdns_pcie_writel(pcie, CDNS_PCIE_LTSSM_CONTROL_CAP, ltssm_control_cap);
> +}
> +
>  void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
>  				   u32 r, bool is_io,
>  				   u64 cpu_addr, u64 pci_addr, size_t size)
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index a978a2eeedad..cda430d0f6e4 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -194,6 +194,14 @@
>  /* AXI link down register */
>  #define CDNS_PCIE_AT_LINKDOWN (CDNS_PCIE_AT_BASE + 0x0824)
>  
> +/* LTSSM Capabilities register */
> +#define CDNS_PCIE_LTSSM_CONTROL_CAP             (CDNS_PCIE_LM_BASE + 0x0054)
> +#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK  GENMASK(2, 1)
> +#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY_SHIFT 1
> +#define  CDNS_PCIE_DETECT_QUIET_MIN_DELAY(delay) \
> +	 (((delay) << CDNS_PCIE_DETECT_QUIET_MIN_DELAY_SHIFT) & \
> +	 CDNS_PCIE_DETECT_QUIET_MIN_DELAY_MASK)
> +
>  enum cdns_pcie_rp_bar {
>  	RP_BAR_UNDEFINED = -1,
>  	RP_BAR0,
> @@ -300,6 +308,7 @@ struct cdns_pcie {
>   * @avail_ib_bar: Satus of RP_BAR0, RP_BAR1 and	RP_NO_BAR if it's free or
>   *                available
>   * @quirk_retrain_flag: Retrain link as quirk for PCIe Gen2
> + * @quirk_detect_quiet_flag: LTSSM Detect Quiet min delay set as quirk
>   */
>  struct cdns_pcie_rc {
>  	struct cdns_pcie	pcie;
> @@ -309,6 +318,7 @@ struct cdns_pcie_rc {
>  	u32			device_id;
>  	bool			avail_ib_bar[CDNS_PCIE_RP_MAX_IB];
>  	unsigned int		quirk_retrain_flag:1;
> +	unsigned int		quirk_detect_quiet_flag:1;
>  };
>  
>  /**
> @@ -341,6 +351,7 @@ struct cdns_pcie_epf {
>   *        registers fields (RMW) accessible by both remote RC and EP to
>   *        minimize time between read and write
>   * @epf: Structure to hold info about endpoint function
> + * @quirk_detect_quiet_flag: LTSSM Detect Quiet min delay set as quirk
>   */
>  struct cdns_pcie_ep {
>  	struct cdns_pcie	pcie;
> @@ -355,6 +366,7 @@ struct cdns_pcie_ep {
>  	/* protect writing to PCI_STATUS while raising legacy interrupts */
>  	spinlock_t		lock;
>  	struct cdns_pcie_epf	*epf;
> +	unsigned int		quirk_detect_quiet_flag:1;
>  };
>  
>  
> @@ -515,6 +527,9 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  	return 0;
>  }
>  #endif
> +
> +void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
> +
>  void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
>  				   u32 r, bool is_io,
>  				   u64 cpu_addr, u64 pci_addr, size_t size);
> -- 
> 2.17.1
> 

Re: [PATCH v2 3/6] PCI: j721e: Add PCIe support for J7200

[Lorenzo Pieralisi]

On Tue, Aug 03, 2021 at 01:19:29PM +0530, Kishon Vijay Abraham I wrote:
> J7200 has the same PCIe IP as in J721E with minor changes in the
> wrapper. J7200 allows byte access of bridge configuration space
> registers and the register field for LINK_DOWN interrupt is different.
> J7200 also requires "quirk_detect_quiet_flag" to be set. Configure these
> changes as part of driver data applicable only to J7200.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 40 +++++++++++++++++++---
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 0c5813b230b4..8e76f2e7e782 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -27,6 +27,7 @@
>  #define STATUS_REG_SYS_2	0x508
>  #define STATUS_CLR_REG_SYS_2	0x708
>  #define LINK_DOWN		BIT(1)
> +#define J7200_LINK_DOWN		BIT(10)
>  
>  #define J721E_PCIE_USER_CMD_STATUS	0x4
>  #define LINK_TRAINING_ENABLE		BIT(0)
> @@ -57,6 +58,7 @@ struct j721e_pcie {
>  	struct cdns_pcie	*cdns_pcie;
>  	void __iomem		*user_cfg_base;
>  	void __iomem		*intd_cfg_base;
> +	u32			linkdown_irq_regfield;
>  };
>  
>  enum j721e_pcie_mode {
> @@ -67,6 +69,9 @@ enum j721e_pcie_mode {
>  struct j721e_pcie_data {
>  	enum j721e_pcie_mode	mode;
>  	unsigned int		quirk_retrain_flag:1;
> +	unsigned int		quirk_detect_quiet_flag:1;
> +	u32			linkdown_irq_regfield;
> +	unsigned int		byte_access_allowed:1;
>  };
>  
>  static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
> @@ -98,12 +103,12 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
>  	u32 reg;
>  
>  	reg = j721e_pcie_intd_readl(pcie, STATUS_REG_SYS_2);
> -	if (!(reg & LINK_DOWN))
> +	if (!(reg & pcie->linkdown_irq_regfield))
>  		return IRQ_NONE;
>  
>  	dev_err(dev, "LINK DOWN!\n");
>  
> -	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, LINK_DOWN);
> +	j721e_pcie_intd_writel(pcie, STATUS_CLR_REG_SYS_2, pcie->linkdown_irq_regfield);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -112,7 +117,7 @@ static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
>  	u32 reg;
>  
>  	reg = j721e_pcie_intd_readl(pcie, ENABLE_REG_SYS_2);
> -	reg |= LINK_DOWN;
> +	reg |= pcie->linkdown_irq_regfield;
>  	j721e_pcie_intd_writel(pcie, ENABLE_REG_SYS_2, reg);
>  }
>  
> @@ -284,10 +289,25 @@ static struct pci_ops cdns_ti_pcie_host_ops = {
>  static const struct j721e_pcie_data j721e_pcie_rc_data = {
>  	.mode = PCI_MODE_RC,
>  	.quirk_retrain_flag = true,
> +	.byte_access_allowed = false,
> +	.linkdown_irq_regfield = LINK_DOWN,
>  };
>  
>  static const struct j721e_pcie_data j721e_pcie_ep_data = {
>  	.mode = PCI_MODE_EP,
> +	.linkdown_irq_regfield = LINK_DOWN,
> +};
> +
> +static const struct j721e_pcie_data j7200_pcie_rc_data = {
> +	.mode = PCI_MODE_RC,
> +	.quirk_detect_quiet_flag = true,
> +	.linkdown_irq_regfield = J7200_LINK_DOWN,
> +	.byte_access_allowed = true,
> +};
> +
> +static const struct j721e_pcie_data j7200_pcie_ep_data = {
> +	.mode = PCI_MODE_EP,
> +	.quirk_detect_quiet_flag = true,
>  };
>  
>  static const struct of_device_id of_j721e_pcie_match[] = {
> @@ -299,6 +319,14 @@ static const struct of_device_id of_j721e_pcie_match[] = {
>  		.compatible = "ti,j721e-pcie-ep",
>  		.data = &j721e_pcie_ep_data,
>  	},
> +	{
> +		.compatible = "ti,j7200-pcie-host",
> +		.data = &j7200_pcie_rc_data,
> +	},
> +	{
> +		.compatible = "ti,j7200-pcie-ep",
> +		.data = &j7200_pcie_ep_data,
> +	},
>  	{},
>  };
>  
> @@ -332,6 +360,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->dev = dev;
>  	pcie->mode = mode;
> +	pcie->linkdown_irq_regfield =  data->linkdown_irq_regfield;
                                     ^
Nit: too many spaces

>  	base = devm_platform_ioremap_resource_byname(pdev, "intd_cfg");
>  	if (IS_ERR(base))
> @@ -391,9 +420,11 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  			goto err_get_sync;
>  		}
>  
> -		bridge->ops = &cdns_ti_pcie_host_ops;
> +		if (!data->byte_access_allowed)
> +			bridge->ops = &cdns_ti_pcie_host_ops;
>  		rc = pci_host_bridge_priv(bridge);
>  		rc->quirk_retrain_flag = data->quirk_retrain_flag;
> +		rc->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
>  
>  		cdns_pcie = &rc->pcie;
>  		cdns_pcie->dev = dev;
> @@ -459,6 +490,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  			ret = -ENOMEM;
>  			goto err_get_sync;
>  		}
> +		ep->quirk_detect_quiet_flag = data->quirk_detect_quiet_flag;
>  
>  		cdns_pcie = &ep->pcie;
>  		cdns_pcie->dev = dev;
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/6] PCI: Add support for J7200 and AM64

[Lorenzo Pieralisi]

On Tue, Aug 03, 2021 at 01:19:26PM +0530, Kishon Vijay Abraham I wrote:
> This series adds the compatible specific to J7200 and AM64 and
> applies the erratas and configuration specific to them.
> 
> This series also includes Nadeem's patch that adds a quirk in
> Cadence driver which is used by J7200 [1].
> 
> The DT binding for both J7200 and AM64 is already merged.
> 
> v1 of the patch series can be found at [2]
> 
> Changes from v1:
> 1) As suggested by Bjorn, used unsigned int :1, instead of bool for
> structure members
> 2) Removed using unnecessary local variables and also fixed some
> code alignment
> 
> [1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com
> [2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com
> 
> Kishon Vijay Abraham I (5):
>   PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool
>   PCI: j721e: Add PCIe support for J7200
>   PCI: j721e: Add PCIe support for AM64
>   misc: pci_endpoint_test: Do not request or allocate IRQs in probe
>   misc: pci_endpoint_test: Add deviceID for AM64 and J7200
> 
> Nadeem Athani (1):
>   PCI: cadence: Add quirk flag to set minimum delay in LTSSM
>     Detect.Quiet state
> 
>  drivers/misc/pci_endpoint_test.c              | 27 ++++++--
>  drivers/pci/controller/cadence/pci-j721e.c    | 61 +++++++++++++++++--
>  .../pci/controller/cadence/pcie-cadence-ep.c  |  4 ++
>  .../controller/cadence/pcie-cadence-host.c    |  3 +
>  drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++
>  drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++-
>  6 files changed, 117 insertions(+), 12 deletions(-)

I am not convinced about patch (5) the rest of the series can be
merged (even though I assume patch (6) depends on (5)).

Please let me know,
Lorenzo

Re: [PATCH v2 5/6] misc: pci_endpoint_test: Do not request or allocate IRQs in probe

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote:
> On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote:
>> Allocation of IRQ vectors and requesting IRQ is done as part of
>> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for
>> AM654 and J721E so that the user space test script has better control
>> of the devices for which the IRQs are configured. Since certain user
>> space scripts could rely on allocation of IRQ vectors during probe,
>> remove allocation of IRQs only for TI's K3 platform.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> I don't claim to understand the inner details of the endpoint test
> device but it looks like this approach should be redesigned.
> 
> I don't believe using devices quirks is the best approach to
> expose/remove a feature to userspace, this can soon become
> unmaintenable.
> 
> Maybe you can elaborate a bit more on what the real issue is please ?

The actual reason for introducing this patch (affects only AM654 and
J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior
(https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5
devices use GIC ITS simultaneously, GIC fails to raise interrupts.

Though this patch is not an actual workaround for the issue (the
workaround is in GIC ITS driver provided in the errata document), it
helps to keep testing PCIe RC/EP using pci-endpoint-test even when
multiple pci-epf-test endpoint devices are connected (Normal test-setup
having J721E-J721E back to back connection can support 21 pci-epf-test
devices). So this patch lets user to individually enable interrupts for
each of the devices and could disable after the interrupt test.

Since pci_endpoint_test is used only for testing PCIE RC/EP
communication and pci-endpoint-test has already implemented
PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not
enable the interrupts of all the devices by default in the probe (for
AM654 and J721E where this errata applies).

Thanks,
Kishon

> 
> Thanks,
> Lorenzo
> 
>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>> index c7ee34013485..9740f2a0e7cd 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -79,6 +79,9 @@
>>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
>>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
>>  
>> +#define is_j721e_pci_dev(pdev)         \
>> +		((pdev)->device == PCI_DEVICE_ID_TI_J721E)
>> +
>>  static DEFINE_IDA(pci_endpoint_test_ida);
>>  
>>  #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
>> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>  
>>  	pci_set_master(pdev);
>>  
>> -	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
>> -		err = -EINVAL;
>> -		goto err_disable_irq;
>> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
>> +		if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
>> +			err = -EINVAL;
>> +			goto err_disable_irq;
>> +		}
>>  	}
>>  
>>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>  		goto err_ida_remove;
>>  	}
>>  
>> -	if (!pci_endpoint_test_request_irq(test)) {
>> -		err = -EINVAL;
>> -		goto err_kfree_test_name;
>> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
>> +		if (!pci_endpoint_test_request_irq(test)) {
>> +			err = -EINVAL;
>> +			goto err_kfree_test_name;
>> +		}
>>  	}
>>  
>>  	misc_device = &test->miscdev;
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 5/6] misc: pci_endpoint_test: Do not request or allocate IRQs in probe

[Lorenzo Pieralisi]

On Wed, Aug 04, 2021 at 07:32:44PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote:
> > On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote:
> >> Allocation of IRQ vectors and requesting IRQ is done as part of
> >> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for
> >> AM654 and J721E so that the user space test script has better control
> >> of the devices for which the IRQs are configured. Since certain user
> >> space scripts could rely on allocation of IRQ vectors during probe,
> >> remove allocation of IRQs only for TI's K3 platform.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------
> >>  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > I don't claim to understand the inner details of the endpoint test
> > device but it looks like this approach should be redesigned.
> > 
> > I don't believe using devices quirks is the best approach to
> > expose/remove a feature to userspace, this can soon become
> > unmaintenable.
> > 
> > Maybe you can elaborate a bit more on what the real issue is please ?
> 
> The actual reason for introducing this patch (affects only AM654 and
> J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior
> (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5
> devices use GIC ITS simultaneously, GIC fails to raise interrupts.
> 
> Though this patch is not an actual workaround for the issue (the
> workaround is in GIC ITS driver provided in the errata document), it
> helps to keep testing PCIe RC/EP using pci-endpoint-test even when
> multiple pci-epf-test endpoint devices are connected (Normal test-setup
> having J721E-J721E back to back connection can support 21 pci-epf-test
> devices). So this patch lets user to individually enable interrupts for
> each of the devices and could disable after the interrupt test.
> 
> Since pci_endpoint_test is used only for testing PCIE RC/EP
> communication and pci-endpoint-test has already implemented
> PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not
> enable the interrupts of all the devices by default in the probe (for
> AM654 and J721E where this errata applies).

I understand - what I am asking is:

is it possible, instead of applying this patch, to make

pci_endpoint_test_alloc_irq_vectors() and pci_endpoint_test_request_irq()

fail in the target platforms instead of preventing to call them ?

My worry is that you may end up with more corner cases in the future
and peppering code with is_() calls to work around them which does
not look right.

Thanks,
Lorenzo

> Thanks,
> Kishon
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> >> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> >> index c7ee34013485..9740f2a0e7cd 100644
> >> --- a/drivers/misc/pci_endpoint_test.c
> >> +++ b/drivers/misc/pci_endpoint_test.c
> >> @@ -79,6 +79,9 @@
> >>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> >>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> >>  
> >> +#define is_j721e_pci_dev(pdev)         \
> >> +		((pdev)->device == PCI_DEVICE_ID_TI_J721E)
> >> +
> >>  static DEFINE_IDA(pci_endpoint_test_ida);
> >>  
> >>  #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
> >> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
> >>  
> >>  	pci_set_master(pdev);
> >>  
> >> -	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
> >> -		err = -EINVAL;
> >> -		goto err_disable_irq;
> >> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
> >> +		if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
> >> +			err = -EINVAL;
> >> +			goto err_disable_irq;
> >> +		}
> >>  	}
> >>  
> >>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> >> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
> >>  		goto err_ida_remove;
> >>  	}
> >>  
> >> -	if (!pci_endpoint_test_request_irq(test)) {
> >> -		err = -EINVAL;
> >> -		goto err_kfree_test_name;
> >> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
> >> +		if (!pci_endpoint_test_request_irq(test)) {
> >> +			err = -EINVAL;
> >> +			goto err_kfree_test_name;
> >> +		}
> >>  	}
> >>  
> >>  	misc_device = &test->miscdev;
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v2 5/6] misc: pci_endpoint_test: Do not request or allocate IRQs in probe

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 05/08/21 4:56 pm, Lorenzo Pieralisi wrote:
> On Wed, Aug 04, 2021 at 07:32:44PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On 03/08/21 3:28 pm, Lorenzo Pieralisi wrote:
>>> On Tue, Aug 03, 2021 at 01:19:31PM +0530, Kishon Vijay Abraham I wrote:
>>>> Allocation of IRQ vectors and requesting IRQ is done as part of
>>>> PCITEST_SET_IRQTYPE. Do not request or allocate IRQs in probe for
>>>> AM654 and J721E so that the user space test script has better control
>>>> of the devices for which the IRQs are configured. Since certain user
>>>> space scripts could rely on allocation of IRQ vectors during probe,
>>>> remove allocation of IRQs only for TI's K3 platform.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/misc/pci_endpoint_test.c | 19 +++++++++++++------
>>>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> I don't claim to understand the inner details of the endpoint test
>>> device but it looks like this approach should be redesigned.
>>>
>>> I don't believe using devices quirks is the best approach to
>>> expose/remove a feature to userspace, this can soon become
>>> unmaintenable.
>>>
>>> Maybe you can elaborate a bit more on what the real issue is please ?
>>
>> The actual reason for introducing this patch (affects only AM654 and
>> J721E) is due to Errata ID #i2101 GIC: ITS Misbehavior
>> (https://www.ti.com/lit/er/sprz455a/sprz455a.pdf). So if more than 5
>> devices use GIC ITS simultaneously, GIC fails to raise interrupts.
>>
>> Though this patch is not an actual workaround for the issue (the
>> workaround is in GIC ITS driver provided in the errata document), it
>> helps to keep testing PCIe RC/EP using pci-endpoint-test even when
>> multiple pci-epf-test endpoint devices are connected (Normal test-setup
>> having J721E-J721E back to back connection can support 21 pci-epf-test
>> devices). So this patch lets user to individually enable interrupts for
>> each of the devices and could disable after the interrupt test.
>>
>> Since pci_endpoint_test is used only for testing PCIE RC/EP
>> communication and pci-endpoint-test has already implemented
>> PCITEST_SET_IRQTYPE for the userspace to enable interrupt, tried to not
>> enable the interrupts of all the devices by default in the probe (for
>> AM654 and J721E where this errata applies).
> 
> I understand - what I am asking is:
> 
> is it possible, instead of applying this patch, to make
> 
> pci_endpoint_test_alloc_irq_vectors() and pci_endpoint_test_request_irq()
> 
> fail in the target platforms instead of preventing to call them ?

hmm.. since both the platforms use GIC ITS it would need modifications
in the irq-gic-v3-its.c driver. I could add the workaround mentioned in
the errata document and send the patch for review to see what others think.

Thanks,
Kishon

> 
> My worry is that you may end up with more corner cases in the future
> and peppering code with is_() calls to work around them which does
> not look right.
> 
> Thanks,
> Lorenzo
> 
>> Thanks,
>> Kishon
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>>> index c7ee34013485..9740f2a0e7cd 100644
>>>> --- a/drivers/misc/pci_endpoint_test.c
>>>> +++ b/drivers/misc/pci_endpoint_test.c
>>>> @@ -79,6 +79,9 @@
>>>>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
>>>>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
>>>>  
>>>> +#define is_j721e_pci_dev(pdev)         \
>>>> +		((pdev)->device == PCI_DEVICE_ID_TI_J721E)
>>>> +
>>>>  static DEFINE_IDA(pci_endpoint_test_ida);
>>>>  
>>>>  #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
>>>> @@ -810,9 +813,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>  
>>>>  	pci_set_master(pdev);
>>>>  
>>>> -	if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
>>>> -		err = -EINVAL;
>>>> -		goto err_disable_irq;
>>>> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
>>>> +		if (!pci_endpoint_test_alloc_irq_vectors(test, irq_type)) {
>>>> +			err = -EINVAL;
>>>> +			goto err_disable_irq;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>>>> @@ -850,9 +855,11 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>  		goto err_ida_remove;
>>>>  	}
>>>>  
>>>> -	if (!pci_endpoint_test_request_irq(test)) {
>>>> -		err = -EINVAL;
>>>> -		goto err_kfree_test_name;
>>>> +	if (!(is_am654_pci_dev(pdev) || is_j721e_pci_dev(pdev))) {
>>>> +		if (!pci_endpoint_test_request_irq(test)) {
>>>> +			err = -EINVAL;
>>>> +			goto err_kfree_test_name;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	misc_device = &test->miscdev;
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v2 0/6] PCI: Add support for J7200 and AM64

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 03/08/21 4:22 pm, Lorenzo Pieralisi wrote:
> On Tue, Aug 03, 2021 at 01:19:26PM +0530, Kishon Vijay Abraham I wrote:
>> This series adds the compatible specific to J7200 and AM64 and
>> applies the erratas and configuration specific to them.
>>
>> This series also includes Nadeem's patch that adds a quirk in
>> Cadence driver which is used by J7200 [1].
>>
>> The DT binding for both J7200 and AM64 is already merged.
>>
>> v1 of the patch series can be found at [2]
>>
>> Changes from v1:
>> 1) As suggested by Bjorn, used unsigned int :1, instead of bool for
>> structure members
>> 2) Removed using unnecessary local variables and also fixed some
>> code alignment
>>
>> [1] -> https://lore.kernel.org/r/20210528155626.21793-1-nadeem@cadence.com
>> [2] -> https://lore.kernel.org/r/20210706105035.9915-1-kishon@ti.com
>>
>> Kishon Vijay Abraham I (5):
>>   PCI: cadence: Use bitfield for *quirk_retrain_flag* instead of bool
>>   PCI: j721e: Add PCIe support for J7200
>>   PCI: j721e: Add PCIe support for AM64
>>   misc: pci_endpoint_test: Do not request or allocate IRQs in probe
>>   misc: pci_endpoint_test: Add deviceID for AM64 and J7200
>>
>> Nadeem Athani (1):
>>   PCI: cadence: Add quirk flag to set minimum delay in LTSSM
>>     Detect.Quiet state
>>
>>  drivers/misc/pci_endpoint_test.c              | 27 ++++++--
>>  drivers/pci/controller/cadence/pci-j721e.c    | 61 +++++++++++++++++--
>>  .../pci/controller/cadence/pcie-cadence-ep.c  |  4 ++
>>  .../controller/cadence/pcie-cadence-host.c    |  3 +
>>  drivers/pci/controller/cadence/pcie-cadence.c | 17 ++++++
>>  drivers/pci/controller/cadence/pcie-cadence.h | 17 +++++-
>>  6 files changed, 117 insertions(+), 12 deletions(-)
> 
> I am not convinced about patch (5) the rest of the series can be
> merged (even though I assume patch (6) depends on (5)).

I'll drop patch 5, fix your other comments and send a new revision.

Thanks,
Kishon