linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/13] Add PCIe support to TI's J721E SoC
@ 2019-12-09  9:21 Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path Kishon Vijay Abraham I
                   ` (12 more replies)
  0 siblings, 13 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

TI's J721E SoC uses Cadence PCIe core to implement both RC mode
and EP mode.

The high level features are:
  *) Supports Legacy, MSI and MSI-X interrupt
  *) Supports upto GEN4 speed mode
  *) Supports SR-IOV
  *) Supports multiple physical function
  *) Ability to route all transactions via SMMU

This patch series
  *) Add support in Cadence PCIe core to be used for TI's J721E SoC
  *) Add a driver for J721E PCIe wrapper

This is a trimmed down series of the initial RFC series [1].

Changes from RFC [1]:
  *) Ability to route all transactions via SMMU is removed
  *) SR-IOV support is removed
  *) Miscellaneous improvement to endpoint core is removed

All these will be sent as smaller series.

I've also pushed the series along with device tree changes [2].

[1] -> https://lkml.org/lkml/2019/6/4/619
[2] -> https://github.com/kishon/linux-wip.git j7_pci_v1

Kishon Vijay Abraham I (13):
  PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path
  linux/kernel.h: Add PTR_ALIGN_DOWN macro
  PCI: cadence: Add support to use custom read and write accessors
  PCI: cadence: Add support to start link and verify link status
  PCI: cadence: Add read and write accessors to perform only 32-bit
    accesses
  PCI: cadence: Allow pci_host_bridge to have custom pci_ops
  PCI: cadence: Add new *ops* for CPU addr fixup
  PCI: cadence: Use local management register to configure Vendor ID
  dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC
  dt-bindings: PCI: Add EP mode dt-bindings for TI's J721E SoC
  PCI: j721e: Add TI J721E PCIe driver
  misc: pci_endpoint_test: Add J721E in pci_device_id table
  MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe

 .../bindings/pci/ti,j721e-pci-ep.yaml         | 113 +++++
 .../bindings/pci/ti,j721e-pci-host.yaml       | 161 +++++++
 MAINTAINERS                                   |   3 +-
 drivers/misc/pci_endpoint_test.c              |   9 +
 drivers/pci/controller/cadence/Kconfig        |  23 +
 drivers/pci/controller/cadence/Makefile       |   1 +
 drivers/pci/controller/cadence/pci-j721e.c    | 430 ++++++++++++++++++
 .../pci/controller/cadence/pcie-cadence-ep.c  |  10 +-
 .../controller/cadence/pcie-cadence-host.c    |  55 ++-
 drivers/pci/controller/cadence/pcie-cadence.c |  48 +-
 drivers/pci/controller/cadence/pcie-cadence.h | 133 +++++-
 include/linux/kernel.h                        |   1 +
 12 files changed, 958 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
 create mode 100644 drivers/pci/controller/cadence/pci-j721e.c

-- 
2.17.1


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

* [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-16 13:45   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 02/13] linux/kernel.h: Add PTR_ALIGN_DOWN macro Kishon Vijay Abraham I
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use
as a core library") while refactoring the Cadence PCIe driver to be
used as library, removed pm_runtime_get_sync() from cdns_pcie_ep_setup()
and cdns_pcie_host_setup() but missed to remove the corresponding
pm_runtime_put_sync() in the error path. Fix it here.

Fixes: commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use
as a core library")

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-ep.c   | 2 --
 drivers/pci/controller/cadence/pcie-cadence-host.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 1c173dad67d1..560f22b4d165 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -473,7 +473,5 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 	pci_epc_mem_exit(epc);
 
  err_init:
-	pm_runtime_put_sync(dev);
-
 	return ret;
 }
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 9b1c3966414b..ccf55e143e1d 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -275,7 +275,5 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	pci_free_resource_list(&resources);
 
  err_init:
-	pm_runtime_put_sync(dev);
-
 	return ret;
 }
-- 
2.17.1


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

* [PATCH 02/13] linux/kernel.h: Add PTR_ALIGN_DOWN macro
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors Kishon Vijay Abraham I
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add a macro for aligning down a pointer. This is useful to get an
aligned register address when a device allows only word access and
doesn't allow half word or byte access.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 include/linux/kernel.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3adcb39fa6f5..888ad70a80aa 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -34,6 +34,7 @@
 #define ALIGN_DOWN(x, a)	__ALIGN_KERNEL((x) - ((a) - 1), (a))
 #define __ALIGN_MASK(x, mask)	__ALIGN_KERNEL_MASK((x), (mask))
 #define PTR_ALIGN(p, a)		((typeof(p))ALIGN((unsigned long)(p), (a)))
+#define PTR_ALIGN_DOWN(p, a)	((typeof(p))ALIGN_DOWN((unsigned long)(p), (a)))
 #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
 
 /* generic data direction definitions */
-- 
2.17.1


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

* [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 02/13] linux/kernel.h: Add PTR_ALIGN_DOWN macro Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-16 14:07   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 04/13] PCI: cadence: Add support to start link and verify link status Kishon Vijay Abraham I
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add support to use custom read and write accessors. Platforms that
doesn't support half word or byte access or any other constraint
while accessing registers can use this feature to populate custom
read and write accessors. These custom accessors are used for both
standard register access and configuration space register access.
This is in preparation for adding PCIe support in TI's J721E SoC which
uses Cadence PCIe core.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence.h | 99 +++++++++++++++++--
 1 file changed, 90 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index a2b28b912ca4..d0d91c69fa1d 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing {
 	MSG_ROUTING_GATHER,
 };
 
+struct cdns_pcie_ops {
+	u32	(*read)(void __iomem *addr, int size);
+	void	(*write)(void __iomem *addr, int size, u32 value);
+};
+
 /**
  * struct cdns_pcie - private data for Cadence PCIe controller drivers
  * @reg_base: IO mapped register base
@@ -239,7 +244,7 @@ struct cdns_pcie {
 	int			phy_count;
 	struct phy		**phy;
 	struct device_link	**link;
-	const struct cdns_pcie_common_ops *ops;
+	const struct cdns_pcie_ops *ops;
 };
 
 /**
@@ -301,21 +306,47 @@ struct cdns_pcie_ep {
 /* Register access */
 static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value)
 {
+	void __iomem *addr = pcie->reg_base + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x1, value);
+		return;
+	}
+
 	writeb(value, pcie->reg_base + reg);
 }
 
 static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value)
 {
+	void __iomem *addr = pcie->reg_base + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x2, value);
+		return;
+	}
+
 	writew(value, pcie->reg_base + reg);
 }
 
 static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value)
 {
+	void __iomem *addr = pcie->reg_base + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x4, value);
+		return;
+	}
+
 	writel(value, pcie->reg_base + reg);
 }
 
 static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
 {
+	void __iomem *addr = pcie->reg_base + reg;
+
+	if (pcie->ops && pcie->ops->read)
+		return pcie->ops->read(addr, 0x4);
+
 	return readl(pcie->reg_base + reg);
 }
 
@@ -323,47 +354,97 @@ static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
 static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie,
 				       u32 reg, u8 value)
 {
-	writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x1, value);
+		return;
+	}
+
+	writeb(value, addr);
 }
 
 static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie,
 				       u32 reg, u16 value)
 {
-	writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x2, value);
+		return;
+	}
+
+	writew(value, addr);
 }
 
 /* Endpoint Function register access */
 static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
 					  u32 reg, u8 value)
 {
-	writeb(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x1, value);
+		return;
+	}
+
+	writeb(value, addr);
 }
 
 static inline void cdns_pcie_ep_fn_writew(struct cdns_pcie *pcie, u8 fn,
 					  u32 reg, u16 value)
 {
-	writew(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x2, value);
+		return;
+	}
+
+	writew(value, addr);
 }
 
 static inline void cdns_pcie_ep_fn_writel(struct cdns_pcie *pcie, u8 fn,
 					  u32 reg, u32 value)
 {
-	writel(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->write) {
+		pcie->ops->write(addr, 0x4, value);
+		return;
+	}
+
+	writel(value, addr);
 }
 
 static inline u8 cdns_pcie_ep_fn_readb(struct cdns_pcie *pcie, u8 fn, u32 reg)
 {
-	return readb(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->read)
+		return pcie->ops->read(addr, 0x1);
+
+	return readb(addr);
 }
 
 static inline u16 cdns_pcie_ep_fn_readw(struct cdns_pcie *pcie, u8 fn, u32 reg)
 {
-	return readw(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->read)
+		return pcie->ops->read(addr, 0x2);
+
+	return readw(addr);
 }
 
 static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
 {
-	return readl(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
+	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
+
+	if (pcie->ops && pcie->ops->read)
+		return pcie->ops->read(addr, 0x4);
+
+	return readl(addr);
 }
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
-- 
2.17.1


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

* [PATCH 04/13] PCI: cadence: Add support to start link and verify link status
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (2 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-17 11:58   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add cdns_pcie_ops to start link and verify link status. The registers
to start link and to check link status is in Platform specific PCIe
wrapper. Add support for platform specific drivers to add callback
functions for the PCIe Cadence core to start link and verify link status.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../pci/controller/cadence/pcie-cadence-ep.c  |  8 ++++++
 .../controller/cadence/pcie-cadence-host.c    | 28 +++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h | 23 +++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 560f22b4d165..088394b6be04 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -355,8 +355,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 {
 	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
 	struct cdns_pcie *pcie = &ep->pcie;
+	struct device *dev = pcie->dev;
 	struct pci_epf *epf;
 	u32 cfg;
+	int ret;
 
 	/*
 	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
@@ -367,6 +369,12 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
 		cfg |= BIT(epf->func_no);
 	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, cfg);
 
+	ret = cdns_pcie_start_link(pcie, true);
+	if (ret) {
+		dev_err(dev, "Failed to start link\n");
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index ccf55e143e1d..0929554f5a81 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -3,6 +3,7 @@
 // Cadence PCIe host controller driver.
 // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
 
+#include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -201,6 +202,23 @@ static int cdns_pcie_host_init(struct device *dev,
 	return err;
 }
 
+static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	int retries;
+
+	/* Check if the link is up or not */
+	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
+		if (cdns_pcie_is_link_up(pcie)) {
+			dev_info(dev, "Link up\n");
+			return 0;
+		}
+		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
+	}
+
+	return -ETIMEDOUT;
+}
+
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	struct device *dev = rc->pcie.dev;
@@ -254,6 +272,16 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 
 	pcie->mem_res = res;
 
+	ret = cdns_pcie_start_link(pcie, true);
+	if (ret) {
+		dev_err(dev, "Failed to start link\n");
+		return ret;
+	}
+
+	ret = cdns_pcie_host_wait_for_link(pcie);
+	if (ret)
+		dev_dbg(dev, "PCIe link never came up\n");
+
 	ret = cdns_pcie_host_init(dev, &resources, rc);
 	if (ret)
 		goto err_init;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index d0d91c69fa1d..f0395eaf9df5 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -10,6 +10,11 @@
 #include <linux/pci.h>
 #include <linux/phy/phy.h>
 
+/* Parameters for the waiting for link up routine */
+#define LINK_WAIT_MAX_RETRIES	10
+#define LINK_WAIT_USLEEP_MIN	90000
+#define LINK_WAIT_USLEEP_MAX	100000
+
 /*
  * Local Management Registers
  */
@@ -226,6 +231,8 @@ enum cdns_pcie_msg_routing {
 struct cdns_pcie_ops {
 	u32	(*read)(void __iomem *addr, int size);
 	void	(*write)(void __iomem *addr, int size, u32 value);
+	int	(*start_link)(struct cdns_pcie *pcie, bool start);
+	bool	(*is_link_up)(struct cdns_pcie *pcie);
 };
 
 /**
@@ -447,6 +454,22 @@ static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
 	return readl(addr);
 }
 
+static inline int cdns_pcie_start_link(struct cdns_pcie *pcie, bool start)
+{
+	if (pcie->ops->start_link)
+		return pcie->ops->start_link(pcie, start);
+
+	return 0;
+}
+
+static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
+{
+	if (pcie->ops->is_link_up)
+		return pcie->ops->is_link_up(pcie);
+
+	return true;
+}
+
 #ifdef CONFIG_PCIE_CADENCE_HOST
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
 #else
-- 
2.17.1


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

* [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (3 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 04/13] PCI: cadence: Add support to start link and verify link status Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-09 21:15   ` Bjorn Helgaas
                     ` (2 more replies)
  2019-12-09  9:21 ` [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops Kishon Vijay Abraham I
                   ` (7 subsequent siblings)
  12 siblings, 3 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Certain platforms like TI's J721E allow only 32-bit register accesses.
Add read and write accessors to perform only 32-bit accesses in order to
support platfroms like TI's J721E.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence.c | 40 +++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h |  2 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index cd795f6fc1e2..de5b3b06f2d0 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -7,6 +7,46 @@
 
 #include "pcie-cadence.h"
 
+u32 cdns_pcie_read32(void __iomem *addr, int size)
+{
+	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
+	unsigned int offset = (unsigned long)addr & 0x3;
+	u32 val = readl(aligned_addr);
+
+	if (!IS_ALIGNED((uintptr_t)addr, size)) {
+		pr_err("Invalid Address in function:%s\n", __func__);
+		return 0;
+	}
+
+	if (size > 2)
+		return val;
+
+	return (val >> (8 * offset)) & ((1 << (size * 8)) - 1);
+}
+
+void cdns_pcie_write32(void __iomem *addr, int size, u32 value)
+{
+	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
+	unsigned int offset = (unsigned long)addr & 0x3;
+	u32 mask;
+	u32 val;
+
+	if (!IS_ALIGNED((uintptr_t)addr, size)) {
+		pr_err("Invalid Address in function:%s\n", __func__);
+		return;
+	}
+
+	if (size > 2) {
+		writel(value, addr);
+		return;
+	}
+
+	mask = ~(((1 << (size * 8)) - 1) << (offset * 8));
+	val = readl(aligned_addr) & mask;
+	val |= value << (offset * 8);
+	writel(val, aligned_addr);
+}
+
 void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, 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 f0395eaf9df5..5171d0da37da 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -498,6 +498,8 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r);
 void cdns_pcie_disable_phy(struct cdns_pcie *pcie);
 int cdns_pcie_enable_phy(struct cdns_pcie *pcie);
 int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie);
+u32 cdns_pcie_read32(void __iomem *addr, int size);
+void cdns_pcie_write32(void __iomem *addr, int size, u32 value);
 extern const struct dev_pm_ops cdns_pcie_pm_ops;
 
 #endif /* _PCIE_CADENCE_H */
-- 
2.17.1


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

* [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (4 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-17 12:32   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup Kishon Vijay Abraham I
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Certain platforms like TI's J721E allows only 32-bit configuration
space access. In such cases pci_generic_config_read and
pci_generic_config_write cannot be used. Add support in Cadence core
to let pci_host_bridge have custom pci_ops.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 7 ++++---
 drivers/pci/controller/cadence/pcie-cadence.h      | 8 ++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 0929554f5a81..2efc33b1cade 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -12,8 +12,8 @@
 
 #include "pcie-cadence.h"
 
-static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
-				      int where)
+void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
+			       int where)
 {
 	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
 	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
@@ -289,7 +289,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	list_splice_init(&resources, &bridge->windows);
 	bridge->dev.parent = dev;
 	bridge->busnr = pcie->bus;
-	bridge->ops = &cdns_pcie_host_ops;
+	if (!bridge->ops)
+		bridge->ops = &cdns_pcie_host_ops;
 	bridge->map_irq = of_irq_parse_and_map_pci;
 	bridge->swizzle_irq = pci_common_swizzle;
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 5171d0da37da..c879dd3d2893 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -472,11 +472,19 @@ static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
+void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
+			       int where);
 #else
 static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	return 0;
 }
+
+static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+}
 #endif
 
 #ifdef CONFIG_PCIE_CADENCE_EP
-- 
2.17.1


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

* [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (5 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-17 12:40   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID Kishon Vijay Abraham I
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Cadence driver uses "mem" memory resource to obtain the offset of
configuration space address region, memory space address region and
message space address region. The obtained offset is used to program
the Address Translation Unit (ATU). However certain platforms like TI's
J721E SoC require the absolute address to be programmed in the ATU and not
just the offset.

The same problem was solved in designware driver using a platform specific
ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware:
Add new *ops* for CPU addr fixup"). Follow a similar mechanism in
Cadence too instead of directly using "mem" memory resource in Cadence
PCIe core.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../pci/controller/cadence/pcie-cadence-host.c    | 15 ++++-----------
 drivers/pci/controller/cadence/pcie-cadence.c     |  8 ++++++--
 drivers/pci/controller/cadence/pcie-cadence.h     |  1 +
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 2efc33b1cade..cf817be237af 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
-	struct resource *mem_res = pcie->mem_res;
 	struct resource *bus_range = rc->bus_range;
 	struct resource *cfg_res = rc->cfg_res;
 	struct device *dev = pcie->dev;
 	struct device_node *np = dev->of_node;
 	struct of_pci_range_parser parser;
+	u64 cpu_addr = cfg_res->start;
 	struct of_pci_range range;
 	u32 addr0, addr1, desc1;
-	u64 cpu_addr;
 	int r, err;
 
 	/*
@@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1);
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1);
 
-	cpu_addr = cfg_res->start - mem_res->start;
+	if (pcie->ops->cpu_addr_fixup)
+		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
+
 	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(12) |
 		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
 	addr1 = upper_32_bits(cpu_addr);
@@ -264,14 +265,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	}
 	rc->cfg_res = res;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem");
-	if (!res) {
-		dev_err(dev, "missing \"mem\"\n");
-		return -EINVAL;
-	}
-
-	pcie->mem_res = res;
-
 	ret = cdns_pcie_start_link(pcie, true);
 	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 de5b3b06f2d0..bd93d0f92f55 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -113,7 +113,9 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 fn,
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(r), desc1);
 
 	/* Set the CPU address */
-	cpu_addr -= pcie->mem_res->start;
+	if (pcie->ops->cpu_addr_fixup)
+		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
+
 	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) |
 		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
 	addr1 = upper_32_bits(cpu_addr);
@@ -140,7 +142,9 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie, u8 fn,
 	}
 
 	/* Set the CPU address */
-	cpu_addr -= pcie->mem_res->start;
+	if (pcie->ops->cpu_addr_fixup)
+		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
+
 	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(17) |
 		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
 	addr1 = upper_32_bits(cpu_addr);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index c879dd3d2893..ffa8b9f78ff8 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -233,6 +233,7 @@ struct cdns_pcie_ops {
 	void	(*write)(void __iomem *addr, int size, u32 value);
 	int	(*start_link)(struct cdns_pcie *pcie, bool start);
 	bool	(*is_link_up)(struct cdns_pcie *pcie);
+	u64     (*cpu_addr_fixup)(struct cdns_pcie *pcie, u64 cpu_addr);
 };
 
 /**
-- 
2.17.1


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

* [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (6 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-17 12:42   ` Andrew Murray
  2019-12-09  9:21 ` [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC Kishon Vijay Abraham I
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

PCI_VENDOR_ID in root port configuration space is read-only register
and writing to it will have no effect. Use local management register to
configure Vendor ID and Subsystem Vendor ID.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index cf817be237af..afb2c96a6538 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -71,6 +71,7 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
 	u32 value, ctrl;
+	u32 id;
 
 	/*
 	 * Set the root complex BAR configuration register:
@@ -90,8 +91,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
 
 	/* Set root port configuration space */
-	if (rc->vendor_id != 0xffff)
-		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);
+	if (rc->vendor_id != 0xffff) {
+		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
+			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
+		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
+	}
+
 	if (rc->device_id != 0xffff)
 		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
 
-- 
2.17.1


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

* [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (7 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-19  0:08   ` Rob Herring
  2019-12-09  9:21 ` [PATCH 10/13] dt-bindings: PCI: Add EP " Kishon Vijay Abraham I
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add host mode dt-bindings for TI's J721E SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,j721e-pci-host.yaml       | 161 ++++++++++++++++++
 1 file changed, 161 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
new file mode 100644
index 000000000000..96184e1f419f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -0,0 +1,161 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-host.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI J721E PCI Host (PCIe Wrapper)
+
+maintainers:
+  - Kishon Vijay Abraham I <kishon@ti.com>
+
+properties:
+  compatible:
+      enum:
+          - ti,j721e-pcie-host
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: intd_cfg
+      - const: user_cfg
+      - const: reg
+      - const: cfg
+
+  ti,syscon-pcie-ctrl:
+    description: Phandle to the SYSCON entry required for configuring PCIe mode
+                 and link speed.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/phandle
+
+  max-link-speed:
+    minimum: 1
+    maximum: 3
+
+  num-lanes:
+    minimum: 1
+    maximum: 2
+
+  power-domains:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: clock-specifier to represent input to the PCIe
+
+  clock-names:
+    items:
+      - const: fck
+
+  "#address-cells":
+    const: 3
+
+  "#size-cells":
+    const: 2
+
+  bus-range:
+    description: Range of bus numbers associated with this controller.
+
+  cdns,max-outbound-regions:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/int32
+      - enum: [16]
+
+  cdns,no-bar-match-nbits:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/int32
+      - enum: [64]
+
+  vendor-id:
+    const: 0x104c
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+  device-id:
+    const: 0xb00d
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+  msi-map: true
+
+  dma-coherent:
+    description: Indicates that the PCIe IP block can ensure the coherency
+
+  ranges: true
+
+  reset-gpios:
+    description: GPIO specifier for the PERST# signal
+
+  phys:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+  phy-names:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - ti,syscon-pcie-ctrl
+  - max-link-speed
+  - num-lanes
+  - power-domains
+  - clocks
+  - clock-names
+  - "#address-cells"
+  - "#size-cells"
+  - bus-range
+  - cdns,max-outbound-regions
+  - cdns,no-bar-match-nbits
+  - vendor-id
+  - device-id
+  - msi-map
+  - dma-coherent
+  - ranges
+  - reset-gpios
+  - phys
+  - phy-names
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+
+    pcie0_rc: pcie@2900000 {
+            compatible = "ti,j721e-pcie-host";
+            reg = <0x00 0x02900000 0x00 0x1000>,
+                  <0x00 0x02907000 0x00 0x400>,
+                  <0x00 0x0d000000 0x00 0x00800000>,
+                  <0x00 0x10000000 0x00 0x00001000>;
+            reg-names = "intd_cfg", "user_cfg", "reg", "cfg";
+            ti,syscon-pcie-ctrl = <&pcie0_ctrl>;
+            max-link-speed = <3>;
+            num-lanes = <2>;
+            power-domains = <&k3_pds 239 TI_SCI_PD_EXCLUSIVE>;
+            clocks = <&k3_clks 239 1>;
+            clock-names = "fck";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            bus-range = <0x0 0xf>;
+            cdns,max-outbound-regions = <16>;
+            cdns,no-bar-match-nbits = <64>;
+            vendor-id = /bits/ 16 <0x104c>;
+            device-id = /bits/ 16 <0xb00d>;
+            msi-map = <0x0 &gic_its 0x0 0x10000>;
+            dma-coherent;
+            reset-gpios = <&exp1 6 GPIO_ACTIVE_HIGH>;
+            phys = <&serdes0_pcie_link>;
+            phy-names = "pcie_phy";
+            ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
+                     <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
+    };
-- 
2.17.1


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

* [PATCH 10/13] dt-bindings: PCI: Add EP mode dt-bindings for TI's J721E SoC
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (8 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-19  0:14   ` Rob Herring
  2019-12-09  9:21 ` [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add PCIe EP mode dt-bindings for TI's J721E SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../bindings/pci/ti,j721e-pci-ep.yaml         | 113 ++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
new file mode 100644
index 000000000000..4e2af4733998
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI J721E PCI EP (PCIe Wrapper)
+
+maintainers:
+  - Kishon Vijay Abraham I <kishon@ti.com>
+
+properties:
+  compatible:
+      enum:
+          - ti,j721e-pcie-ep
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: intd_cfg
+      - const: user_cfg
+      - const: reg
+      - const: mem
+
+  ti,syscon-pcie-ctrl:
+    description: Phandle to the SYSCON entry required for configuring PCIe mode
+                 and link speed.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/phandle
+
+  max-link-speed:
+    minimum: 1
+    maximum: 3
+
+  num-lanes:
+    minimum: 1
+    maximum: 2
+
+  power-domains:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: clock-specifier to represent input to the PCIe
+
+  clock-names:
+    items:
+      - const: fck
+
+  cdns,max-outbound-regions:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/int32
+      - enum: [16]
+
+  max-functions:
+    minimum: 1
+    maximum: 6
+
+  dma-coherent:
+    description: Indicates that the PCIe IP block can ensure the coherency
+
+  phys:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+  phy-names:
+    description: As defined in
+                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - ti,syscon-pcie-ctrl
+  - max-link-speed
+  - num-lanes
+  - power-domains
+  - clocks
+  - clock-names
+  - cdns,max-outbound-regions
+  - dma-coherent
+  - max-functions
+  - phys
+  - phy-names
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+     pcie0_ep: pcie-ep@d000000 {
+            compatible = "ti,j721e-pcie-ep";
+            reg = <0x00 0x02900000 0x00 0x1000>,
+                  <0x00 0x02907000 0x00 0x400>,
+                  <0x00 0x0d000000 0x00 0x00800000>,
+                  <0x00 0x10000000 0x00 0x08000000>;
+            reg-names = "intd_cfg", "user_cfg", "reg", "mem";
+            ti,syscon-pcie-ctrl = <&pcie0_ctrl>;
+            max-link-speed = <3>;
+            num-lanes = <2>;
+            power-domains = <&k3_pds 239 TI_SCI_PD_EXCLUSIVE>;
+            clocks = <&k3_clks 239 1>;
+            clock-names = "fck";
+            cdns,max-outbound-regions = <16>;
+            max-functions = /bits/ 8 <6>;
+            dma-coherent;
+            phys = <&serdes0_pcie_link>;
+            phy-names = "pcie_phy";
+    };
-- 
2.17.1


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

* [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (9 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 10/13] dt-bindings: PCI: Add EP " Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-17 14:23   ` Andrew Murray
  2019-12-19 22:47   ` Bjorn Helgaas
  2019-12-09  9:21 ` [PATCH 12/13] misc: pci_endpoint_test: Add J721E in pci_device_id table Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 13/13] MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe Kishon Vijay Abraham I
  12 siblings, 2 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add support for PCIe controller in J721E SoC. The controller uses the
Cadence PCIe core programmed by pcie-cadence*.c. The PCIe controller
will work in both host mode and device mode.
Some of the features of the controller are:
  *) Supports both RC mode and EP mode
  *) Supports MSI and MSI-X support
  *) Supports upto GEN3 speed mode
  *) Supports SR-IOV capability
  *) Ability to route all transactions via SMMU (support will be added
     in a later patch).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/cadence/Kconfig     |  23 ++
 drivers/pci/controller/cadence/Makefile    |   1 +
 drivers/pci/controller/cadence/pci-j721e.c | 430 +++++++++++++++++++++
 3 files changed, 454 insertions(+)
 create mode 100644 drivers/pci/controller/cadence/pci-j721e.c

diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
index b76b3cf55ce5..5d30564190e1 100644
--- a/drivers/pci/controller/cadence/Kconfig
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -42,4 +42,27 @@ config PCIE_CADENCE_PLAT_EP
 	  endpoint mode. This PCIe controller may be embedded into many
 	  different vendors SoCs.
 
+config PCI_J721E
+	bool
+
+config PCI_J721E_HOST
+	bool "TI J721E PCIe platform host controller"
+	depends on OF
+	select PCIE_CADENCE_HOST
+	select PCI_J721E
+	help
+	  Say Y here if you want to support the TI J721E PCIe platform
+	  controller in host mode. TI J721E PCIe controller uses Cadence PCIe
+	  core.
+
+config PCI_J721E_EP
+	bool "TI J721E PCIe platform endpoint controller"
+	depends on OF
+	depends on PCI_ENDPOINT
+	select PCIE_CADENCE_EP
+	select PCI_J721E
+	help
+	  Say Y here if you want to support the TI J721E PCIe platform
+	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
+	  core.
 endmenu
diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile
index 232a3f20876a..9bac5fb2f13d 100644
--- a/drivers/pci/controller/cadence/Makefile
+++ b/drivers/pci/controller/cadence/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence.o
 obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
 obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
 obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
+obj-$(CONFIG_PCI_J721E) += pci-j721e.o
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
new file mode 100644
index 000000000000..9ffb7e88c739
--- /dev/null
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -0,0 +1,430 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * pci-j721e - PCIe controller driver for TI's J721E SoCs
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/io.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "../../pci.h"
+#include "pcie-cadence.h"
+
+#define J721E_PCIE_USER_CMD_STATUS	0x4
+#define LINK_TRAINING_ENABLE		BIT(0)
+
+#define J721E_PCIE_USER_LINKSTATUS	0x14
+#define LINK_STATUS			GENMASK(1, 0)
+
+enum link_status {
+	NO_RECIEVERS_DETECTED,
+	LINK_TRAINING_IN_PROGRESS,
+	LINK_UP_DL_IN_PROGRESS,
+	LINK_UP_DL_COMPLETED,
+};
+
+#define J721E_MODE_RC			BIT(7)
+#define LANE_COUNT_MASK			BIT(8)
+#define LANE_COUNT(n)			((n) << 8)
+
+#define GENERATION_SEL_MASK		GENMASK(1, 0)
+
+#define MAX_LANES			2
+
+struct j721e_pcie {
+	struct device		*dev;
+	struct device_node	*node;
+	u32			mode;
+	u32			num_lanes;
+	struct cdns_pcie	*cdns_pcie;
+	void __iomem		*user_cfg_base;
+};
+
+enum j721e_pcie_mode {
+	PCI_MODE_RC,
+	PCI_MODE_EP,
+};
+
+struct j721e_pcie_data {
+	enum j721e_pcie_mode	mode;
+};
+
+static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
+{
+	return readl(pcie->user_cfg_base + offset);
+}
+
+static inline void j721e_pcie_user_writel(struct j721e_pcie *pcie, u32 offset,
+					  u32 value)
+{
+	writel(value, pcie->user_cfg_base + offset);
+}
+
+static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie, bool start)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
+	u32 reg;
+
+	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_CMD_STATUS);
+	if (start)
+		reg |= LINK_TRAINING_ENABLE;
+	else
+		reg &= ~LINK_TRAINING_ENABLE;
+	j721e_pcie_user_writel(pcie, J721E_PCIE_USER_CMD_STATUS, reg);
+
+	return 0;
+}
+
+static bool j721e_pcie_is_link_up(struct cdns_pcie *cdns_pcie)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
+	u32 reg;
+
+	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_LINKSTATUS);
+	reg &= LINK_STATUS;
+	if (reg == LINK_UP_DL_COMPLETED)
+		return true;
+
+	return false;
+}
+
+static const struct cdns_pcie_ops j721e_ops_ops = {
+	.read = cdns_pcie_read32,
+	.write = cdns_pcie_write32,
+	.start_link = j721e_pcie_start_link,
+	.is_link_up = j721e_pcie_is_link_up,
+};
+
+static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon)
+{
+	struct device *dev = pcie->dev;
+	u32 mask = J721E_MODE_RC;
+	u32 mode = pcie->mode;
+	u32 val = 0;
+	int ret = 0;
+
+	if (mode == PCI_MODE_RC)
+		val = J721E_MODE_RC;
+
+	ret = regmap_update_bits(syscon, 0, mask, val);
+	if (ret)
+		dev_err(dev, "failed to set pcie mode\n");
+
+	return ret;
+}
+
+static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
+				     struct regmap *syscon)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *np = dev->of_node;
+	int link_speed;
+	u32 val = 0;
+	int ret;
+
+	link_speed = of_pci_get_max_link_speed(np);
+	if (link_speed < 2)
+		link_speed = 2;
+
+	val = link_speed - 1;
+	ret = regmap_update_bits(syscon, 0, GENERATION_SEL_MASK, val);
+	if (ret)
+		dev_err(dev, "failed to set link speed\n");
+
+	return ret;
+}
+
+static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
+				     struct regmap *syscon)
+{
+	struct device *dev = pcie->dev;
+	u32 lanes = pcie->num_lanes;
+	u32 val = 0;
+	int ret;
+
+	val = LANE_COUNT(lanes - 1);
+	ret = regmap_update_bits(syscon, 0, LANE_COUNT_MASK, val);
+	if (ret)
+		dev_err(dev, "failed to set link count\n");
+
+	return ret;
+}
+
+static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct regmap *syscon;
+	int ret;
+
+	syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-ctrl");
+	if (IS_ERR(syscon)) {
+		dev_err(dev, "Unable to get ti,syscon-pcie-ctrl regmap\n");
+		return PTR_ERR(syscon);
+	}
+
+	ret = j721e_pcie_set_mode(pcie, syscon);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set pci mode\n");
+		return ret;
+	}
+
+	ret = j721e_pcie_set_link_speed(pcie, syscon);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set link speed\n");
+		return ret;
+	}
+
+	ret = j721e_pcie_set_lane_count(pcie, syscon);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set num-lanes\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cdns_ti_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
+				    int where, int size, u32 *value)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
+	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
+	unsigned int busn = bus->number;
+
+	if (busn == rc->bus_range->start)
+		return pci_generic_config_read32(bus, devfn, where, size,
+						 value);
+
+	return pci_generic_config_read(bus, devfn, where, size, value);
+}
+
+static int cdns_ti_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+				     int where, int size, u32 value)
+{
+	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
+	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
+	unsigned int busn = bus->number;
+
+	if (busn == rc->bus_range->start)
+		return pci_generic_config_write32(bus, devfn, where, size,
+						  value);
+
+	return pci_generic_config_write(bus, devfn, where, size, value);
+}
+
+static struct pci_ops cdns_ti_pcie_host_ops = {
+	.map_bus	= cdns_pci_map_bus,
+	.read		= cdns_ti_pcie_config_read,
+	.write		= cdns_ti_pcie_config_write,
+};
+
+static const struct j721e_pcie_data j721e_pcie_rc_data = {
+	.mode = PCI_MODE_RC,
+};
+
+static const struct j721e_pcie_data j721e_pcie_ep_data = {
+	.mode = PCI_MODE_EP,
+};
+
+static const struct of_device_id of_j721e_pcie_match[] = {
+	{
+		.compatible = "ti,j721e-pcie-host",
+		.data = &j721e_pcie_rc_data,
+	},
+	{
+		.compatible = "ti,j721e-pcie-ep",
+		.data = &j721e_pcie_ep_data,
+	},
+	{},
+};
+
+static int j721e_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const struct of_device_id *match;
+	struct pci_host_bridge *bridge;
+	struct j721e_pcie_data *data;
+	struct cdns_pcie *cdns_pcie;
+	struct j721e_pcie *pcie;
+	struct cdns_pcie_rc *rc;
+	struct cdns_pcie_ep *ep;
+	struct gpio_desc *gpiod;
+	struct resource *res;
+	void __iomem *base;
+	u32 num_lanes;
+	u32 mode;
+	int ret;
+
+	match = of_match_device(of_match_ptr(of_j721e_pcie_match), dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct j721e_pcie_data *)match->data;
+	mode = (u32)data->mode;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = dev;
+	pcie->node = node;
+	pcie->mode = mode;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "user_cfg");
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	pcie->user_cfg_base = base;
+
+	ret = of_property_read_u32(node, "num-lanes", &num_lanes);
+	if (ret || num_lanes > MAX_LANES)
+		num_lanes = 1;
+	pcie->num_lanes = num_lanes;
+
+	dev_set_drvdata(dev, pcie);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed\n");
+		goto err_get_sync;
+	}
+
+	ret = j721e_pcie_ctrl_init(pcie);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed\n");
+		goto err_get_sync;
+	}
+
+	switch (mode) {
+	case PCI_MODE_RC:
+		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) {
+			ret = -ENODEV;
+			goto err_get_sync;
+		}
+
+		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
+		if (!bridge) {
+			ret = -ENOMEM;
+			goto err_get_sync;
+		}
+
+		bridge->ops = &cdns_ti_pcie_host_ops;
+		rc = pci_host_bridge_priv(bridge);
+
+		cdns_pcie = &rc->pcie;
+		cdns_pcie->dev = dev;
+		cdns_pcie->ops = &j721e_ops_ops;
+		pcie->cdns_pcie = cdns_pcie;
+
+		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			ret = PTR_ERR(gpiod);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get reset GPIO\n");
+			goto err_get_sync;
+		}
+
+		ret = cdns_pcie_init_phy(dev, cdns_pcie);
+		if (ret) {
+			dev_err(dev, "Failed to init phy\n");
+			goto err_get_sync;
+		}
+
+		/*
+		 * "Power Sequencing and Reset Signal Timings" table in
+		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
+		 * indicates PERST# should be deasserted after minimum of 100us
+		 * once REFCLK is stable. The REFCLK to the connector in RC
+		 * mode is selected while enabling the PHY. So deassert PERST#
+		 * after 100 us.
+		 */
+		if (gpiod) {
+			usleep_range(100, 200);
+			gpiod_set_value_cansleep(gpiod, 1);
+		}
+
+		ret = cdns_pcie_host_setup(rc);
+		if (ret < 0)
+			goto err_pcie_setup;
+
+		break;
+	case PCI_MODE_EP:
+		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) {
+			ret = -ENODEV;
+			goto err_get_sync;
+		}
+
+		ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
+		if (!ep) {
+			ret = -ENOMEM;
+			goto err_get_sync;
+		}
+
+		cdns_pcie = &ep->pcie;
+		cdns_pcie->dev = dev;
+		cdns_pcie->ops = &j721e_ops_ops;
+		pcie->cdns_pcie = cdns_pcie;
+
+		ret = cdns_pcie_init_phy(dev, cdns_pcie);
+		if (ret) {
+			dev_err(dev, "Failed to init phy\n");
+			goto err_get_sync;
+		}
+
+		ret = cdns_pcie_ep_setup(ep);
+		if (ret < 0)
+			goto err_pcie_setup;
+
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", mode);
+	}
+
+	return 0;
+
+err_pcie_setup:
+	cdns_pcie_disable_phy(cdns_pcie);
+
+err_get_sync:
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
+	return ret;
+}
+
+static int j721e_pcie_remove(struct platform_device *pdev)
+{
+	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
+	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
+	struct device *dev = &pdev->dev;
+
+	cdns_pcie_disable_phy(cdns_pcie);
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+	of_platform_depopulate(dev);
+
+	return 0;
+}
+
+static struct platform_driver j721e_pcie_driver = {
+	.probe  = j721e_pcie_probe,
+	.remove = j721e_pcie_remove,
+	.driver = {
+		.name	= "j721e-pcie",
+		.of_match_table = of_j721e_pcie_match,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver(j721e_pcie_driver);
-- 
2.17.1


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

* [PATCH 12/13] misc: pci_endpoint_test: Add J721E in pci_device_id table
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (10 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  2019-12-09  9:21 ` [PATCH 13/13] MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe Kishon Vijay Abraham I
  12 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add J721E in pci_device_id table so that pci-epf-test can be used
for testing PCIe EP in J721E.

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

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index a5e317073d95..215f9b8432a3 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -64,6 +64,7 @@
 #define PCI_ENDPOINT_TEST_IRQ_TYPE		0x24
 #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
 
+#define PCI_DEVICE_ID_TI_J721E			0xb00d
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 
 #define is_am654_pci_dev(pdev)		\
@@ -789,6 +790,11 @@ static const struct pci_endpoint_test_data am654_data = {
 	.irq_type = IRQ_TYPE_MSI,
 };
 
+static const struct pci_endpoint_test_data j721e_data = {
+	.alignment = 256,
+	.irq_type = IRQ_TYPE_MSI,
+};
+
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
@@ -797,6 +803,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_AM654),
 	  .driver_data = (kernel_ulong_t)&am654_data
 	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
+	  .driver_data = (kernel_ulong_t)&j721e_data,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
-- 
2.17.1


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

* [PATCH 13/13] MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe
  2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
                   ` (11 preceding siblings ...)
  2019-12-09  9:21 ` [PATCH 12/13] misc: pci_endpoint_test: Add J721E in pci_device_id table Kishon Vijay Abraham I
@ 2019-12-09  9:21 ` Kishon Vijay Abraham I
  12 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-09  9:21 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	Andrew Murray
  Cc: linux-pci, devicetree, linux-kernel, linux-omap, Kishon Vijay Abraham I

Add Kishon Vijay Abraham I as MAINTAINER for TI J721E SoC PCIe.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index bd5847e802de..a9533d0752de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12691,13 +12691,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/designware-pcie.txt
 F:	drivers/pci/controller/dwc/*designware*
 
-PCI DRIVER FOR TI DRA7XX
+PCI DRIVER FOR TI DRA7XX/J721E
 M:	Kishon Vijay Abraham I <kishon@ti.com>
 L:	linux-omap@vger.kernel.org
 L:	linux-pci@vger.kernel.org
 S:	Supported
 F:	Documentation/devicetree/bindings/pci/ti-pci.txt
 F:	drivers/pci/controller/dwc/pci-dra7xx.c
+F:	drivers/pci/controller/cadence/pci-j721e.c
 
 PCI DRIVER FOR TI KEYSTONE
 M:	Murali Karicheri <m-karicheri2@ti.com>
-- 
2.17.1


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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
@ 2019-12-09 21:15   ` Bjorn Helgaas
  2019-12-16 14:49   ` Andrew Murray
  2019-12-17 23:36   ` Bjorn Helgaas
  2 siblings, 0 replies; 41+ messages in thread
From: Bjorn Helgaas @ 2019-12-09 21:15 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Lorenzo Pieralisi, Rob Herring, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
> Certain platforms like TI's J721E allow only 32-bit register accesses.
> Add read and write accessors to perform only 32-bit accesses in order to
> support platfroms like TI's J721E.

s/platfroms/platforms/

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

* Re: [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path
  2019-12-09  9:21 ` [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path Kishon Vijay Abraham I
@ 2019-12-16 13:45   ` Andrew Murray
  2019-12-19  8:31     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-16 13:45 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:35PM +0530, Kishon Vijay Abraham I wrote:
> commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use
> as a core library") while refactoring the Cadence PCIe driver to be
> used as library, removed pm_runtime_get_sync() from cdns_pcie_ep_setup()
> and cdns_pcie_host_setup() but missed to remove the corresponding
> pm_runtime_put_sync() in the error path. Fix it here.
> 
> Fixes: commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use

As this is a fix, a commit subject starting with PCI: cadence: Fix ... may
be more obvious.

I'd suggest you use the shorter form of this, i.e. Fixes: %h (\"%s\"))

Fixes: bd22885aa188 ("PCI: cadence: Refactor driver to use as a core library")

> as a core library")
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c   | 2 --
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 1c173dad67d1..560f22b4d165 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -473,7 +473,5 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  	pci_epc_mem_exit(epc);
>  
>   err_init:
> -	pm_runtime_put_sync(dev);
> -
>  	return ret;
>  }
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 9b1c3966414b..ccf55e143e1d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -275,7 +275,5 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	pci_free_resource_list(&resources);
>  
>   err_init:
> -	pm_runtime_put_sync(dev);
> -

There is probably more you can do here for both -host and -ep:

 - Remove the possibly now unused <linux/pm_runtime.h> include
 - Remove the err_init label and return directly from source.

Thanks,

Andrew Murray

>  	return ret;
>  }
> -- 
> 2.17.1
> 

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

* Re: [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors
  2019-12-09  9:21 ` [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors Kishon Vijay Abraham I
@ 2019-12-16 14:07   ` Andrew Murray
  2019-12-19 11:41     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-16 14:07 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:37PM +0530, Kishon Vijay Abraham I wrote:
> Add support to use custom read and write accessors. Platforms that
> doesn't support half word or byte access or any other constraint

s/doesn't/don't/

> while accessing registers can use this feature to populate custom
> read and write accessors. These custom accessors are used for both
> standard register access and configuration space register access.

You can put the following sentence underneath a --- as it's not needed
in the commit message (but may be helpful to reviewers).

> This is in preparation for adding PCIe support in TI's J721E SoC which
> uses Cadence PCIe core.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence.h | 99 +++++++++++++++++--
>  1 file changed, 90 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index a2b28b912ca4..d0d91c69fa1d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing {
>  	MSG_ROUTING_GATHER,
>  };
>  
> +struct cdns_pcie_ops {
> +	u32	(*read)(void __iomem *addr, int size);
> +	void	(*write)(void __iomem *addr, int size, u32 value);
> +};
> +
>  /**
>   * struct cdns_pcie - private data for Cadence PCIe controller drivers
>   * @reg_base: IO mapped register base
> @@ -239,7 +244,7 @@ struct cdns_pcie {
>  	int			phy_count;
>  	struct phy		**phy;
>  	struct device_link	**link;
> -	const struct cdns_pcie_common_ops *ops;

What was cdns_pcie_common_ops? It's not defined in the current tree is it?

> +	const struct cdns_pcie_ops *ops;
>  };
>  
>  /**
> @@ -301,21 +306,47 @@ struct cdns_pcie_ep {
>  /* Register access */
>  static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value)
>  {
> +	void __iomem *addr = pcie->reg_base + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x1, value);
> +		return;
> +	}
> +
>  	writeb(value, pcie->reg_base + reg);

Can you use 'addr' here instead of 'pcie->reg_base + reg'? (And similar for the
rest of them).

Thanks,

Andrew Murray


>  }
>  
>  static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value)
>  {
> +	void __iomem *addr = pcie->reg_base + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x2, value);
> +		return;
> +	}
> +
>  	writew(value, pcie->reg_base + reg);
>  }
>  
>  static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value)
>  {
> +	void __iomem *addr = pcie->reg_base + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x4, value);
> +		return;
> +	}
> +
>  	writel(value, pcie->reg_base + reg);
>  }
>  
>  static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
>  {
> +	void __iomem *addr = pcie->reg_base + reg;
> +
> +	if (pcie->ops && pcie->ops->read)
> +		return pcie->ops->read(addr, 0x4);
> +
>  	return readl(pcie->reg_base + reg);
>  }
>  
> @@ -323,47 +354,97 @@ static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg)
>  static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie,
>  				       u32 reg, u8 value)
>  {
> -	writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x1, value);
> +		return;
> +	}
> +
> +	writeb(value, addr);
>  }
>  
>  static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie,
>  				       u32 reg, u16 value)
>  {
> -	writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x2, value);
> +		return;
> +	}
> +
> +	writew(value, addr);
>  }
>  
>  /* Endpoint Function register access */
>  static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
>  					  u32 reg, u8 value)
>  {
> -	writeb(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x1, value);
> +		return;
> +	}
> +
> +	writeb(value, addr);
>  }
>  
>  static inline void cdns_pcie_ep_fn_writew(struct cdns_pcie *pcie, u8 fn,
>  					  u32 reg, u16 value)
>  {
> -	writew(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x2, value);
> +		return;
> +	}
> +
> +	writew(value, addr);
>  }
>  
>  static inline void cdns_pcie_ep_fn_writel(struct cdns_pcie *pcie, u8 fn,
>  					  u32 reg, u32 value)
>  {
> -	writel(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->write) {
> +		pcie->ops->write(addr, 0x4, value);
> +		return;
> +	}
> +
> +	writel(value, addr);
>  }
>  
>  static inline u8 cdns_pcie_ep_fn_readb(struct cdns_pcie *pcie, u8 fn, u32 reg)
>  {
> -	return readb(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->read)
> +		return pcie->ops->read(addr, 0x1);
> +
> +	return readb(addr);
>  }
>  
>  static inline u16 cdns_pcie_ep_fn_readw(struct cdns_pcie *pcie, u8 fn, u32 reg)
>  {
> -	return readw(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->read)
> +		return pcie->ops->read(addr, 0x2);
> +
> +	return readw(addr);
>  }
>  
>  static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
>  {
> -	return readl(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg);
> +	void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg;
> +
> +	if (pcie->ops && pcie->ops->read)
> +		return pcie->ops->read(addr, 0x4);
> +
> +	return readl(addr);
>  }
>  
>  #ifdef CONFIG_PCIE_CADENCE_HOST
> -- 
> 2.17.1
> 

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
  2019-12-09 21:15   ` Bjorn Helgaas
@ 2019-12-16 14:49   ` Andrew Murray
  2019-12-19 11:56     ` Kishon Vijay Abraham I
  2019-12-17 23:36   ` Bjorn Helgaas
  2 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-16 14:49 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
> Certain platforms like TI's J721E allow only 32-bit register accesses.

When I first read this I thought you meant only 32-bit accesses are allowed
and not other sizes (such as 64-bit). However the limitation you address
here is that the J721E allows only 32-bit *aligned* register accesses.

It would be helpful to make this clearer in the commit message.

You can also shorten the commit subject to 'PCI: cadence: Add read/write
accessors for 32-bit aligned accesses' or similar.

> Add read and write accessors to perform only 32-bit accesses in order to
> support platfroms like TI's J721E.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence.c | 40 +++++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h |  2 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
> index cd795f6fc1e2..de5b3b06f2d0 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -7,6 +7,46 @@
>  
>  #include "pcie-cadence.h"
>  
> +u32 cdns_pcie_read32(void __iomem *addr, int size)

Given there is already a cdns_pcie_readl in pcie-cadence.h it may help
to name this in a way that doesn't cause confusion. Here 32 is perhaps
being used to suggest the size of the actual read performed, the
maximum size of 'size' or the alignment.


> +{
> +	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
> +	unsigned int offset = (unsigned long)addr & 0x3;
> +	u32 val = readl(aligned_addr);
> +
> +	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> +		pr_err("Invalid Address in function:%s\n", __func__);

Would this be better as a BUG? Without a BUG this error could get ignored
and yet the device may not behave as expected.


> +		return 0;
> +	}
> +
> +	if (size > 2)
> +		return val;

I think you make the assumption here that if size > 2 then it's 4. It could
be 3 (though unlikely) in which case you'd want to fall through to the next
line.

> +
> +	return (val >> (8 * offset)) & ((1 << (size * 8)) - 1);
> +}
> +
> +void cdns_pcie_write32(void __iomem *addr, int size, u32 value)
> +{

And same feedback for this function.

Thanks,

Andrew Murray

> +	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
> +	unsigned int offset = (unsigned long)addr & 0x3;
> +	u32 mask;
> +	u32 val;
> +
> +	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> +		pr_err("Invalid Address in function:%s\n", __func__);
> +		return;
> +	}
> +
> +	if (size > 2) {
> +		writel(value, addr);
> +		return;
> +	}
> +
> +	mask = ~(((1 << (size * 8)) - 1) << (offset * 8));
> +	val = readl(aligned_addr) & mask;
> +	val |= value << (offset * 8);
> +	writel(val, aligned_addr);
> +}
> +
>  void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, 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 f0395eaf9df5..5171d0da37da 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -498,6 +498,8 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r);
>  void cdns_pcie_disable_phy(struct cdns_pcie *pcie);
>  int cdns_pcie_enable_phy(struct cdns_pcie *pcie);
>  int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie);
> +u32 cdns_pcie_read32(void __iomem *addr, int size);
> +void cdns_pcie_write32(void __iomem *addr, int size, u32 value);
>  extern const struct dev_pm_ops cdns_pcie_pm_ops;
>  
>  #endif /* _PCIE_CADENCE_H */
> -- 
> 2.17.1
> 

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

* Re: [PATCH 04/13] PCI: cadence: Add support to start link and verify link status
  2019-12-09  9:21 ` [PATCH 04/13] PCI: cadence: Add support to start link and verify link status Kishon Vijay Abraham I
@ 2019-12-17 11:58   ` Andrew Murray
  2019-12-19 12:01     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-17 11:58 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:38PM +0530, Kishon Vijay Abraham I wrote:
> Add cdns_pcie_ops to start link and verify link status. The registers
> to start link and to check link status is in Platform specific PCIe
> wrapper. Add support for platform specific drivers to add callback
> functions for the PCIe Cadence core to start link and verify link status.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../pci/controller/cadence/pcie-cadence-ep.c  |  8 ++++++
>  .../controller/cadence/pcie-cadence-host.c    | 28 +++++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h | 23 +++++++++++++++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 560f22b4d165..088394b6be04 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -355,8 +355,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> +	struct device *dev = pcie->dev;
>  	struct pci_epf *epf;
>  	u32 cfg;
> +	int ret;
>  
>  	/*
>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
> @@ -367,6 +369,12 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  		cfg |= BIT(epf->func_no);
>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, cfg);
>  
> +	ret = cdns_pcie_start_link(pcie, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index ccf55e143e1d..0929554f5a81 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -3,6 +3,7 @@
>  // Cadence PCIe host controller driver.
>  // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>  
> +#include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -201,6 +202,23 @@ static int cdns_pcie_host_init(struct device *dev,
>  	return err;
>  }
>  
> +static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	int retries;
> +
> +	/* Check if the link is up or not */
> +	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> +		if (cdns_pcie_is_link_up(pcie)) {
> +			dev_info(dev, "Link up\n");
> +			return 0;
> +		}
> +		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> +	}
> +
> +	return -ETIMEDOUT;
> +}

This patch looks fine, except this function (above) is identical to
dw_pcie_wait_for_link, advk_pcie_wait_for_link and nwl_wait_for_link. Even
the definitions of LINK_WAIT_USLEEP_xx are the same.

I don't see any justification to duplicating this again - can you consolidate
these functions to something that all controller drivers can use?

Thanks,

Andrew Murray

> +
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	struct device *dev = rc->pcie.dev;
> @@ -254,6 +272,16 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  
>  	pcie->mem_res = res;
>  
> +	ret = cdns_pcie_start_link(pcie, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
> +	ret = cdns_pcie_host_wait_for_link(pcie);
> +	if (ret)
> +		dev_dbg(dev, "PCIe link never came up\n");
> +
>  	ret = cdns_pcie_host_init(dev, &resources, rc);
>  	if (ret)
>  		goto err_init;
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index d0d91c69fa1d..f0395eaf9df5 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -10,6 +10,11 @@
>  #include <linux/pci.h>
>  #include <linux/phy/phy.h>
>  
> +/* Parameters for the waiting for link up routine */
> +#define LINK_WAIT_MAX_RETRIES	10
> +#define LINK_WAIT_USLEEP_MIN	90000
> +#define LINK_WAIT_USLEEP_MAX	100000
> +
>  /*
>   * Local Management Registers
>   */
> @@ -226,6 +231,8 @@ enum cdns_pcie_msg_routing {
>  struct cdns_pcie_ops {
>  	u32	(*read)(void __iomem *addr, int size);
>  	void	(*write)(void __iomem *addr, int size, u32 value);
> +	int	(*start_link)(struct cdns_pcie *pcie, bool start);
> +	bool	(*is_link_up)(struct cdns_pcie *pcie);
>  };
>  
>  /**
> @@ -447,6 +454,22 @@ static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
>  	return readl(addr);
>  }
>  
> +static inline int cdns_pcie_start_link(struct cdns_pcie *pcie, bool start)
> +{
> +	if (pcie->ops->start_link)
> +		return pcie->ops->start_link(pcie, start);
> +
> +	return 0;
> +}
> +
> +static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
> +{
> +	if (pcie->ops->is_link_up)
> +		return pcie->ops->is_link_up(pcie);
> +
> +	return true;
> +}
> +
>  #ifdef CONFIG_PCIE_CADENCE_HOST
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
>  #else
> -- 
> 2.17.1
> 

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

* Re: [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops
  2019-12-09  9:21 ` [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops Kishon Vijay Abraham I
@ 2019-12-17 12:32   ` Andrew Murray
  2019-12-19 12:02     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-17 12:32 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:40PM +0530, Kishon Vijay Abraham I wrote:
> Certain platforms like TI's J721E allows only 32-bit configuration
> space access. In such cases pci_generic_config_read and
> pci_generic_config_write cannot be used. Add support in Cadence core
> to let pci_host_bridge have custom pci_ops.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 7 ++++---
>  drivers/pci/controller/cadence/pcie-cadence.h      | 8 ++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 0929554f5a81..2efc33b1cade 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -12,8 +12,8 @@
>  
>  #include "pcie-cadence.h"
>  
> -static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> -				      int where)
> +void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> +			       int where)
>  {
>  	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
>  	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> @@ -289,7 +289,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	list_splice_init(&resources, &bridge->windows);
>  	bridge->dev.parent = dev;
>  	bridge->busnr = pcie->bus;
> -	bridge->ops = &cdns_pcie_host_ops;
> +	if (!bridge->ops)
> +		bridge->ops = &cdns_pcie_host_ops;
>  	bridge->map_irq = of_irq_parse_and_map_pci;
>  	bridge->swizzle_irq = pci_common_swizzle;
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 5171d0da37da..c879dd3d2893 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -472,11 +472,19 @@ static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
>  
>  #ifdef CONFIG_PCIE_CADENCE_HOST
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
> +void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> +			       int where);

The commit message doesn't explain why this change in visibility is needed).

>  #else
>  static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	return 0;
>  }
> +
> +static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus,
> +					     unsigned int devfn,
> +					     int where)
> +{

This still needs to return something right?

Thanks,

Andrew Murray

> +}
>  #endif
>  
>  #ifdef CONFIG_PCIE_CADENCE_EP
> -- 
> 2.17.1
> 

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

* Re: [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup
  2019-12-09  9:21 ` [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup Kishon Vijay Abraham I
@ 2019-12-17 12:40   ` Andrew Murray
  2019-12-19 12:03     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-17 12:40 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:41PM +0530, Kishon Vijay Abraham I wrote:
> Cadence driver uses "mem" memory resource to obtain the offset of
> configuration space address region, memory space address region and
> message space address region. The obtained offset is used to program
> the Address Translation Unit (ATU). However certain platforms like TI's
> J721E SoC require the absolute address to be programmed in the ATU and not
> just the offset.
> 
> The same problem was solved in designware driver using a platform specific
> ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware:

Thanks for this reference, though this doesn't need to be in the commit
log, please put such comments underneath a ---.

> Add new *ops* for CPU addr fixup"). Follow a similar mechanism in
> Cadence too instead of directly using "mem" memory resource in Cadence
> PCIe core.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../pci/controller/cadence/pcie-cadence-host.c    | 15 ++++-----------
>  drivers/pci/controller/cadence/pcie-cadence.c     |  8 ++++++--
>  drivers/pci/controller/cadence/pcie-cadence.h     |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 2efc33b1cade..cf817be237af 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>  {
>  	struct cdns_pcie *pcie = &rc->pcie;
> -	struct resource *mem_res = pcie->mem_res;
>  	struct resource *bus_range = rc->bus_range;
>  	struct resource *cfg_res = rc->cfg_res;
>  	struct device *dev = pcie->dev;
>  	struct device_node *np = dev->of_node;
>  	struct of_pci_range_parser parser;
> +	u64 cpu_addr = cfg_res->start;
>  	struct of_pci_range range;
>  	u32 addr0, addr1, desc1;
> -	u64 cpu_addr;
>  	int r, err;
>  
>  	/*
> @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1);
>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1);
>  
> -	cpu_addr = cfg_res->start - mem_res->start;
> +	if (pcie->ops->cpu_addr_fixup)
> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> +

Won't this patch cause a breakage for existing users that won't have defined a
cpu_addr_fixup? The offset isn't being calculated and so cpu_addr will be wrong?

Thanks,

Andrew Murray

>  	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(12) |
>  		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
>  	addr1 = upper_32_bits(cpu_addr);
> @@ -264,14 +265,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  	}
>  	rc->cfg_res = res;
>  
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem");
> -	if (!res) {
> -		dev_err(dev, "missing \"mem\"\n");
> -		return -EINVAL;
> -	}
> -
> -	pcie->mem_res = res;
> -
>  	ret = cdns_pcie_start_link(pcie, true);
>  	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 de5b3b06f2d0..bd93d0f92f55 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -113,7 +113,9 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 fn,
>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(r), desc1);
>  
>  	/* Set the CPU address */
> -	cpu_addr -= pcie->mem_res->start;
> +	if (pcie->ops->cpu_addr_fixup)
> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> +
>  	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) |
>  		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
>  	addr1 = upper_32_bits(cpu_addr);
> @@ -140,7 +142,9 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie, u8 fn,
>  	}
>  
>  	/* Set the CPU address */
> -	cpu_addr -= pcie->mem_res->start;
> +	if (pcie->ops->cpu_addr_fixup)
> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
> +
>  	addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(17) |
>  		(lower_32_bits(cpu_addr) & GENMASK(31, 8));
>  	addr1 = upper_32_bits(cpu_addr);
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index c879dd3d2893..ffa8b9f78ff8 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -233,6 +233,7 @@ struct cdns_pcie_ops {
>  	void	(*write)(void __iomem *addr, int size, u32 value);
>  	int	(*start_link)(struct cdns_pcie *pcie, bool start);
>  	bool	(*is_link_up)(struct cdns_pcie *pcie);
> +	u64     (*cpu_addr_fixup)(struct cdns_pcie *pcie, u64 cpu_addr);
>  };
>  
>  /**
> -- 
> 2.17.1
> 

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

* Re: [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID
  2019-12-09  9:21 ` [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID Kishon Vijay Abraham I
@ 2019-12-17 12:42   ` Andrew Murray
  2019-12-19 12:12     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Andrew Murray @ 2019-12-17 12:42 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:42PM +0530, Kishon Vijay Abraham I wrote:
> PCI_VENDOR_ID in root port configuration space is read-only register
> and writing to it will have no effect. Use local management register to
> configure Vendor ID and Subsystem Vendor ID.

Is this a bug fix? Can you add a Fixes tag and make that clearer?

Thanks,

Andrew Murray

> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index cf817be237af..afb2c96a6538 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -71,6 +71,7 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  {
>  	struct cdns_pcie *pcie = &rc->pcie;
>  	u32 value, ctrl;
> +	u32 id;
>  
>  	/*
>  	 * Set the root complex BAR configuration register:
> @@ -90,8 +91,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>  
>  	/* Set root port configuration space */
> -	if (rc->vendor_id != 0xffff)
> -		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);
> +	if (rc->vendor_id != 0xffff) {
> +		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
> +			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
> +	}
> +
>  	if (rc->device_id != 0xffff)
>  		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
>  
> -- 
> 2.17.1
> 

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

* Re: [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver
  2019-12-09  9:21 ` [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
@ 2019-12-17 14:23   ` Andrew Murray
  2019-12-19 22:47   ` Bjorn Helgaas
  1 sibling, 0 replies; 41+ messages in thread
From: Andrew Murray @ 2019-12-17 14:23 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:45PM +0530, Kishon Vijay Abraham I wrote:
> Add support for PCIe controller in J721E SoC. The controller uses the
> Cadence PCIe core programmed by pcie-cadence*.c. The PCIe controller
> will work in both host mode and device mode.
> Some of the features of the controller are:
>   *) Supports both RC mode and EP mode
>   *) Supports MSI and MSI-X support
>   *) Supports upto GEN3 speed mode
>   *) Supports SR-IOV capability
>   *) Ability to route all transactions via SMMU (support will be added
>      in a later patch).
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/Kconfig     |  23 ++
>  drivers/pci/controller/cadence/Makefile    |   1 +
>  drivers/pci/controller/cadence/pci-j721e.c | 430 +++++++++++++++++++++
>  3 files changed, 454 insertions(+)
>  create mode 100644 drivers/pci/controller/cadence/pci-j721e.c
> 
> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index b76b3cf55ce5..5d30564190e1 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -42,4 +42,27 @@ config PCIE_CADENCE_PLAT_EP
>  	  endpoint mode. This PCIe controller may be embedded into many
>  	  different vendors SoCs.
>  
> +config PCI_J721E
> +	bool
> +
> +config PCI_J721E_HOST
> +	bool "TI J721E PCIe platform host controller"
> +	depends on OF
> +	select PCIE_CADENCE_HOST
> +	select PCI_J721E
> +	help
> +	  Say Y here if you want to support the TI J721E PCIe platform
> +	  controller in host mode. TI J721E PCIe controller uses Cadence PCIe
> +	  core.
> +
> +config PCI_J721E_EP
> +	bool "TI J721E PCIe platform endpoint controller"
> +	depends on OF
> +	depends on PCI_ENDPOINT
> +	select PCIE_CADENCE_EP
> +	select PCI_J721E
> +	help
> +	  Say Y here if you want to support the TI J721E PCIe platform
> +	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
> +	  core.
>  endmenu
> diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile
> index 232a3f20876a..9bac5fb2f13d 100644
> --- a/drivers/pci/controller/cadence/Makefile
> +++ b/drivers/pci/controller/cadence/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence.o
>  obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
>  obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
>  obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
> +obj-$(CONFIG_PCI_J721E) += pci-j721e.o

Why pci-j721e and not pcie-j721e? Especially given that many of the structures
in the file use pcie (e.g. j721e_pcie_ep_data)


> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> new file mode 100644
> index 000000000000..9ffb7e88c739
> --- /dev/null
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -0,0 +1,430 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * pci-j721e - PCIe controller driver for TI's J721E SoCs
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/io.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include "../../pci.h"
> +#include "pcie-cadence.h"
> +
> +#define J721E_PCIE_USER_CMD_STATUS	0x4
> +#define LINK_TRAINING_ENABLE		BIT(0)
> +
> +#define J721E_PCIE_USER_LINKSTATUS	0x14
> +#define LINK_STATUS			GENMASK(1, 0)
> +
> +enum link_status {
> +	NO_RECIEVERS_DETECTED,
> +	LINK_TRAINING_IN_PROGRESS,
> +	LINK_UP_DL_IN_PROGRESS,
> +	LINK_UP_DL_COMPLETED,
> +};
> +
> +#define J721E_MODE_RC			BIT(7)
> +#define LANE_COUNT_MASK			BIT(8)
> +#define LANE_COUNT(n)			((n) << 8)
> +
> +#define GENERATION_SEL_MASK		GENMASK(1, 0)
> +
> +#define MAX_LANES			2
> +
> +struct j721e_pcie {
> +	struct device		*dev;
> +	struct device_node	*node;
> +	u32			mode;
> +	u32			num_lanes;
> +	struct cdns_pcie	*cdns_pcie;
> +	void __iomem		*user_cfg_base;
> +};
> +
> +enum j721e_pcie_mode {
> +	PCI_MODE_RC,
> +	PCI_MODE_EP,
> +};
> +
> +struct j721e_pcie_data {
> +	enum j721e_pcie_mode	mode;
> +};
> +
> +static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
> +{
> +	return readl(pcie->user_cfg_base + offset);
> +}
> +
> +static inline void j721e_pcie_user_writel(struct j721e_pcie *pcie, u32 offset,
> +					  u32 value)
> +{
> +	writel(value, pcie->user_cfg_base + offset);
> +}
> +
> +static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie, bool start)
> +{
> +	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> +	u32 reg;
> +
> +	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_CMD_STATUS);
> +	if (start)
> +		reg |= LINK_TRAINING_ENABLE;
> +	else
> +		reg &= ~LINK_TRAINING_ENABLE;
> +	j721e_pcie_user_writel(pcie, J721E_PCIE_USER_CMD_STATUS, reg);
> +
> +	return 0;
> +}
> +
> +static bool j721e_pcie_is_link_up(struct cdns_pcie *cdns_pcie)
> +{
> +	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> +	u32 reg;
> +
> +	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_LINKSTATUS);
> +	reg &= LINK_STATUS;
> +	if (reg == LINK_UP_DL_COMPLETED)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct cdns_pcie_ops j721e_ops_ops = {
> +	.read = cdns_pcie_read32,
> +	.write = cdns_pcie_write32,
> +	.start_link = j721e_pcie_start_link,
> +	.is_link_up = j721e_pcie_is_link_up,
> +};
> +
> +static int j721e_pcie_set_mode(struct j721e_pcie *pcie, struct regmap *syscon)
> +{
> +	struct device *dev = pcie->dev;
> +	u32 mask = J721E_MODE_RC;
> +	u32 mode = pcie->mode;
> +	u32 val = 0;
> +	int ret = 0;
> +
> +	if (mode == PCI_MODE_RC)
> +		val = J721E_MODE_RC;
> +
> +	ret = regmap_update_bits(syscon, 0, mask, val);
> +	if (ret)
> +		dev_err(dev, "failed to set pcie mode\n");
> +
> +	return ret;
> +}
> +
> +static int j721e_pcie_set_link_speed(struct j721e_pcie *pcie,
> +				     struct regmap *syscon)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *np = dev->of_node;
> +	int link_speed;
> +	u32 val = 0;
> +	int ret;
> +
> +	link_speed = of_pci_get_max_link_speed(np);
> +	if (link_speed < 2)
> +		link_speed = 2;
> +
> +	val = link_speed - 1;
> +	ret = regmap_update_bits(syscon, 0, GENERATION_SEL_MASK, val);
> +	if (ret)
> +		dev_err(dev, "failed to set link speed\n");
> +
> +	return ret;
> +}
> +
> +static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
> +				     struct regmap *syscon)
> +{
> +	struct device *dev = pcie->dev;
> +	u32 lanes = pcie->num_lanes;
> +	u32 val = 0;
> +	int ret;
> +
> +	val = LANE_COUNT(lanes - 1);
> +	ret = regmap_update_bits(syscon, 0, LANE_COUNT_MASK, val);
> +	if (ret)
> +		dev_err(dev, "failed to set link count\n");
> +
> +	return ret;
> +}
> +
> +static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	struct regmap *syscon;
> +	int ret;
> +
> +	syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-ctrl");
> +	if (IS_ERR(syscon)) {
> +		dev_err(dev, "Unable to get ti,syscon-pcie-ctrl regmap\n");
> +		return PTR_ERR(syscon);
> +	}
> +
> +	ret = j721e_pcie_set_mode(pcie, syscon);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set pci mode\n");
> +		return ret;
> +	}
> +
> +	ret = j721e_pcie_set_link_speed(pcie, syscon);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set link speed\n");
> +		return ret;
> +	}
> +
> +	ret = j721e_pcie_set_lane_count(pcie, syscon);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set num-lanes\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cdns_ti_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> +				    int where, int size, u32 *value)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> +	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> +	unsigned int busn = bus->number;
> +
> +	if (busn == rc->bus_range->start)
> +		return pci_generic_config_read32(bus, devfn, where, size,
> +						 value);
> +
> +	return pci_generic_config_read(bus, devfn, where, size, value);
> +}
> +
> +static int cdns_ti_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
> +				     int where, int size, u32 value)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> +	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> +	unsigned int busn = bus->number;
> +
> +	if (busn == rc->bus_range->start)
> +		return pci_generic_config_write32(bus, devfn, where, size,
> +						  value);
> +
> +	return pci_generic_config_write(bus, devfn, where, size, value);
> +}
> +
> +static struct pci_ops cdns_ti_pcie_host_ops = {
> +	.map_bus	= cdns_pci_map_bus,
> +	.read		= cdns_ti_pcie_config_read,
> +	.write		= cdns_ti_pcie_config_write,
> +};
> +
> +static const struct j721e_pcie_data j721e_pcie_rc_data = {
> +	.mode = PCI_MODE_RC,
> +};
> +
> +static const struct j721e_pcie_data j721e_pcie_ep_data = {
> +	.mode = PCI_MODE_EP,
> +};
> +
> +static const struct of_device_id of_j721e_pcie_match[] = {
> +	{
> +		.compatible = "ti,j721e-pcie-host",
> +		.data = &j721e_pcie_rc_data,
> +	},
> +	{
> +		.compatible = "ti,j721e-pcie-ep",
> +		.data = &j721e_pcie_ep_data,
> +	},
> +	{},
> +};
> +
> +static int j721e_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	const struct of_device_id *match;
> +	struct pci_host_bridge *bridge;
> +	struct j721e_pcie_data *data;
> +	struct cdns_pcie *cdns_pcie;
> +	struct j721e_pcie *pcie;
> +	struct cdns_pcie_rc *rc;
> +	struct cdns_pcie_ep *ep;
> +	struct gpio_desc *gpiod;
> +	struct resource *res;
> +	void __iomem *base;
> +	u32 num_lanes;
> +	u32 mode;
> +	int ret;
> +
> +	match = of_match_device(of_match_ptr(of_j721e_pcie_match), dev);
> +	if (!match)
> +		return -EINVAL;
> +
> +	data = (struct j721e_pcie_data *)match->data;

I think you can use of_device_get_match_data(dev) here instead.


> +	mode = (u32)data->mode;
> +
> +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->dev = dev;
> +	pcie->node = node;
> +	pcie->mode = mode;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "user_cfg");
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +	pcie->user_cfg_base = base;
> +
> +	ret = of_property_read_u32(node, "num-lanes", &num_lanes);
> +	if (ret || num_lanes > MAX_LANES)
> +		num_lanes = 1;
> +	pcie->num_lanes = num_lanes;
> +
> +	dev_set_drvdata(dev, pcie);
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed\n");
> +		goto err_get_sync;

This means we'll call pm_runtime_put - are you sure you want to do that?

Thanks,

Andrew Murray


> +	}
> +
> +	ret = j721e_pcie_ctrl_init(pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed\n");
> +		goto err_get_sync;
> +	}
> +
> +	switch (mode) {
> +	case PCI_MODE_RC:
> +		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)) {
> +			ret = -ENODEV;
> +			goto err_get_sync;
> +		}
> +
> +		bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> +		if (!bridge) {
> +			ret = -ENOMEM;
> +			goto err_get_sync;
> +		}
> +
> +		bridge->ops = &cdns_ti_pcie_host_ops;
> +		rc = pci_host_bridge_priv(bridge);
> +
> +		cdns_pcie = &rc->pcie;
> +		cdns_pcie->dev = dev;
> +		cdns_pcie->ops = &j721e_ops_ops;
> +		pcie->cdns_pcie = cdns_pcie;
> +
> +		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			ret = PTR_ERR(gpiod);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get reset GPIO\n");
> +			goto err_get_sync;
> +		}
> +
> +		ret = cdns_pcie_init_phy(dev, cdns_pcie);
> +		if (ret) {
> +			dev_err(dev, "Failed to init phy\n");
> +			goto err_get_sync;
> +		}
> +
> +		/*
> +		 * "Power Sequencing and Reset Signal Timings" table in
> +		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 3.0
> +		 * indicates PERST# should be deasserted after minimum of 100us
> +		 * once REFCLK is stable. The REFCLK to the connector in RC
> +		 * mode is selected while enabling the PHY. So deassert PERST#
> +		 * after 100 us.
> +		 */
> +		if (gpiod) {
> +			usleep_range(100, 200);
> +			gpiod_set_value_cansleep(gpiod, 1);
> +		}
> +
> +		ret = cdns_pcie_host_setup(rc);
> +		if (ret < 0)
> +			goto err_pcie_setup;
> +
> +		break;
> +	case PCI_MODE_EP:
> +		if (!IS_ENABLED(CONFIG_PCIE_CADENCE_EP)) {
> +			ret = -ENODEV;
> +			goto err_get_sync;
> +		}
> +
> +		ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> +		if (!ep) {
> +			ret = -ENOMEM;
> +			goto err_get_sync;
> +		}
> +
> +		cdns_pcie = &ep->pcie;
> +		cdns_pcie->dev = dev;
> +		cdns_pcie->ops = &j721e_ops_ops;
> +		pcie->cdns_pcie = cdns_pcie;
> +
> +		ret = cdns_pcie_init_phy(dev, cdns_pcie);
> +		if (ret) {
> +			dev_err(dev, "Failed to init phy\n");
> +			goto err_get_sync;
> +		}
> +
> +		ret = cdns_pcie_ep_setup(ep);
> +		if (ret < 0)
> +			goto err_pcie_setup;
> +
> +		break;
> +	default:
> +		dev_err(dev, "INVALID device type %d\n", mode);
> +	}
> +
> +	return 0;
> +
> +err_pcie_setup:
> +	cdns_pcie_disable_phy(cdns_pcie);
> +
> +err_get_sync:
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return ret;
> +}
> +
> +static int j721e_pcie_remove(struct platform_device *pdev)
> +{
> +	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
> +	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
> +	struct device *dev = &pdev->dev;
> +
> +	cdns_pcie_disable_phy(cdns_pcie);
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +	of_platform_depopulate(dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver j721e_pcie_driver = {
> +	.probe  = j721e_pcie_probe,
> +	.remove = j721e_pcie_remove,
> +	.driver = {
> +		.name	= "j721e-pcie",
> +		.of_match_table = of_j721e_pcie_match,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(j721e_pcie_driver);
> -- 
> 2.17.1
> 

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
  2019-12-09 21:15   ` Bjorn Helgaas
  2019-12-16 14:49   ` Andrew Murray
@ 2019-12-17 23:36   ` Bjorn Helgaas
  2019-12-19 12:49     ` Kishon Vijay Abraham I
  2 siblings, 1 reply; 41+ messages in thread
From: Bjorn Helgaas @ 2019-12-17 23:36 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Lorenzo Pieralisi, Rob Herring, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
> Certain platforms like TI's J721E allow only 32-bit register accesses.
> Add read and write accessors to perform only 32-bit accesses in order to
> support platfroms like TI's J721E.

s/platfroms/platforms/

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/pcie-cadence.c | 40 +++++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h |  2 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
> index cd795f6fc1e2..de5b3b06f2d0 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -7,6 +7,46 @@
>  
>  #include "pcie-cadence.h"
>  
> +u32 cdns_pcie_read32(void __iomem *addr, int size)
> +{
> +	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
> +	unsigned int offset = (unsigned long)addr & 0x3;
> +	u32 val = readl(aligned_addr);
> +
> +	if (!IS_ALIGNED((uintptr_t)addr, size)) {
> +		pr_err("Invalid Address in function:%s\n", __func__);

It might be nice to have a hint about *why* it's invalid, e.g., the
addr and size values.

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

* Re: [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC
  2019-12-09  9:21 ` [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC Kishon Vijay Abraham I
@ 2019-12-19  0:08   ` Rob Herring
  2019-12-19 13:13     ` Kishon Vijay Abraham I
  2019-12-24  8:06     ` Kishon Vijay Abraham I
  0 siblings, 2 replies; 41+ messages in thread
From: Rob Herring @ 2019-12-19  0:08 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:43PM +0530, Kishon Vijay Abraham I wrote:
> Add host mode dt-bindings for TI's J721E SoC.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-host.yaml       | 161 ++++++++++++++++++
>  1 file changed, 161 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> new file mode 100644
> index 000000000000..96184e1f419f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-host.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TI J721E PCI Host (PCIe Wrapper)
> +
> +maintainers:
> +  - Kishon Vijay Abraham I <kishon@ti.com>

There's now a PCI bus schema. Reference it here:

allOf:
  - $ref: "/schemas/pci/pci-bus.yaml#"

> +
> +properties:
> +  compatible:
> +      enum:
> +          - ti,j721e-pcie-host

Indentation.

> +
> +  reg:
> +    maxItems: 4
> +
> +  reg-names:
> +    items:
> +      - const: intd_cfg
> +      - const: user_cfg
> +      - const: reg
> +      - const: cfg
> +
> +  ti,syscon-pcie-ctrl:
> +    description: Phandle to the SYSCON entry required for configuring PCIe mode
> +                 and link speed.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/phandle

You can drop the 'allOf' here if there aren't more constraints.

> +
> +  max-link-speed:
> +    minimum: 1
> +    maximum: 3
> +
> +  num-lanes:
> +    minimum: 1
> +    maximum: 2
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: clock-specifier to represent input to the PCIe
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +

> +  "#address-cells":
> +    const: 3
> +
> +  "#size-cells":
> +    const: 2
> +
> +  bus-range:
> +    description: Range of bus numbers associated with this controller.

Drop these 3 as they are all standard.

> +
> +  cdns,max-outbound-regions:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/int32

Can be negative? Use uint32.

The int* definitions are kind of broken until dtc is fixed to maintain 
sign.

> +      - enum: [16]

const: 16

> +
> +  cdns,no-bar-match-nbits:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/int32
> +      - enum: [64]
> +
> +  vendor-id:
> +    const: 0x104c
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt

And elsewhere. Drop the description.

> +
> +  device-id:
> +    const: 0xb00d
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +
> +  msi-map: true
> +
> +  dma-coherent:
> +    description: Indicates that the PCIe IP block can ensure the coherency
> +
> +  ranges: true

Don't you know how many?

> +
> +  reset-gpios:
> +    description: GPIO specifier for the PERST# signal
> +
> +  phys:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +
> +  phy-names:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - ti,syscon-pcie-ctrl
> +  - max-link-speed
> +  - num-lanes
> +  - power-domains
> +  - clocks
> +  - clock-names

> +  - "#address-cells"
> +  - "#size-cells"

Can drop these 2. The bus schema requires them.

> +  - bus-range
> +  - cdns,max-outbound-regions
> +  - cdns,no-bar-match-nbits
> +  - vendor-id
> +  - device-id
> +  - msi-map
> +  - dma-coherent
> +  - ranges

Can drop, too.

> +  - reset-gpios

Isn't having this board dependent?

> +  - phys
> +  - phy-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +
> +    pcie0_rc: pcie@2900000 {
> +            compatible = "ti,j721e-pcie-host";
> +            reg = <0x00 0x02900000 0x00 0x1000>,
> +                  <0x00 0x02907000 0x00 0x400>,
> +                  <0x00 0x0d000000 0x00 0x00800000>,
> +                  <0x00 0x10000000 0x00 0x00001000>;
> +            reg-names = "intd_cfg", "user_cfg", "reg", "cfg";
> +            ti,syscon-pcie-ctrl = <&pcie0_ctrl>;
> +            max-link-speed = <3>;
> +            num-lanes = <2>;
> +            power-domains = <&k3_pds 239 TI_SCI_PD_EXCLUSIVE>;
> +            clocks = <&k3_clks 239 1>;
> +            clock-names = "fck";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            bus-range = <0x0 0xf>;
> +            cdns,max-outbound-regions = <16>;
> +            cdns,no-bar-match-nbits = <64>;
> +            vendor-id = /bits/ 16 <0x104c>;
> +            device-id = /bits/ 16 <0xb00d>;
> +            msi-map = <0x0 &gic_its 0x0 0x10000>;
> +            dma-coherent;
> +            reset-gpios = <&exp1 6 GPIO_ACTIVE_HIGH>;
> +            phys = <&serdes0_pcie_link>;
> +            phy-names = "pcie_phy";
> +            ranges = <0x01000000 0x0 0x10001000  0x00 0x10001000  0x0 0x0010000>,
> +                     <0x02000000 0x0 0x10011000  0x00 0x10011000  0x0 0x7fef000>;
> +    };
> -- 
> 2.17.1
> 

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

* Re: [PATCH 10/13] dt-bindings: PCI: Add EP mode dt-bindings for TI's J721E SoC
  2019-12-09  9:21 ` [PATCH 10/13] dt-bindings: PCI: Add EP " Kishon Vijay Abraham I
@ 2019-12-19  0:14   ` Rob Herring
  2019-12-19 13:14     ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Rob Herring @ 2019-12-19  0:14 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:44PM +0530, Kishon Vijay Abraham I wrote:
> Add PCIe EP mode dt-bindings for TI's J721E SoC.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 113 ++++++++++++++++++
>  1 file changed, 113 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> new file mode 100644
> index 000000000000..4e2af4733998
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-ep.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TI J721E PCI EP (PCIe Wrapper)
> +
> +maintainers:
> +  - Kishon Vijay Abraham I <kishon@ti.com>
> +
> +properties:
> +  compatible:
> +      enum:
> +          - ti,j721e-pcie-ep

Indentation.

> +
> +  reg:
> +    maxItems: 4
> +
> +  reg-names:
> +    items:
> +      - const: intd_cfg
> +      - const: user_cfg
> +      - const: reg
> +      - const: mem
> +
> +  ti,syscon-pcie-ctrl:
> +    description: Phandle to the SYSCON entry required for configuring PCIe mode
> +                 and link speed.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/phandle
> +
> +  max-link-speed:
> +    minimum: 1
> +    maximum: 3
> +
> +  num-lanes:
> +    minimum: 1
> +    maximum: 2
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: clock-specifier to represent input to the PCIe
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +
> +  cdns,max-outbound-regions:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/int32

uint32

> +      - enum: [16]
> +
> +  max-functions:
> +    minimum: 1
> +    maximum: 6

Needs a type ref. Or a common definition.

> +
> +  dma-coherent:
> +    description: Indicates that the PCIe IP block can ensure the coherency
> +
> +  phys:

How many? Need to convert cdns,cdns-pcie-host.txt...

> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> +
> +  phy-names:
> +    description: As defined in
> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt

For all the properties shared with host mode, it might make sense to 
define a common schema with all those properties and then include it in 
the host and endpoint schemas.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - ti,syscon-pcie-ctrl
> +  - max-link-speed
> +  - num-lanes
> +  - power-domains
> +  - clocks
> +  - clock-names
> +  - cdns,max-outbound-regions
> +  - dma-coherent
> +  - max-functions
> +  - phys
> +  - phy-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +
> +     pcie0_ep: pcie-ep@d000000 {
> +            compatible = "ti,j721e-pcie-ep";
> +            reg = <0x00 0x02900000 0x00 0x1000>,
> +                  <0x00 0x02907000 0x00 0x400>,
> +                  <0x00 0x0d000000 0x00 0x00800000>,
> +                  <0x00 0x10000000 0x00 0x08000000>;
> +            reg-names = "intd_cfg", "user_cfg", "reg", "mem";
> +            ti,syscon-pcie-ctrl = <&pcie0_ctrl>;
> +            max-link-speed = <3>;
> +            num-lanes = <2>;
> +            power-domains = <&k3_pds 239 TI_SCI_PD_EXCLUSIVE>;
> +            clocks = <&k3_clks 239 1>;
> +            clock-names = "fck";
> +            cdns,max-outbound-regions = <16>;
> +            max-functions = /bits/ 8 <6>;
> +            dma-coherent;
> +            phys = <&serdes0_pcie_link>;
> +            phy-names = "pcie_phy";
> +    };
> -- 
> 2.17.1
> 

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

* Re: [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path
  2019-12-16 13:45   ` Andrew Murray
@ 2019-12-19  8:31     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19  8:31 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Andrew,

On 16/12/19 7:15 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:35PM +0530, Kishon Vijay Abraham I wrote:
>> commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use
>> as a core library") while refactoring the Cadence PCIe driver to be
>> used as library, removed pm_runtime_get_sync() from cdns_pcie_ep_setup()
>> and cdns_pcie_host_setup() but missed to remove the corresponding
>> pm_runtime_put_sync() in the error path. Fix it here.
>>
>> Fixes: commit bd22885aa188f135fd9 ("PCI: cadence: Refactor driver to use
> 
> As this is a fix, a commit subject starting with PCI: cadence: Fix ... may
> be more obvious.
> 
> I'd suggest you use the shorter form of this, i.e. Fixes: %h (\"%s\"))
> 
> Fixes: bd22885aa188 ("PCI: cadence: Refactor driver to use as a core library")
> 
>> as a core library")

Okay.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>   drivers/pci/controller/cadence/pcie-cadence-ep.c   | 2 --
>>   drivers/pci/controller/cadence/pcie-cadence-host.c | 2 --
>>   2 files changed, 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> index 1c173dad67d1..560f22b4d165 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> @@ -473,7 +473,5 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>>   	pci_epc_mem_exit(epc);
>>   
>>    err_init:
>> -	pm_runtime_put_sync(dev);
>> -
>>   	return ret;
>>   }
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 9b1c3966414b..ccf55e143e1d 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -275,7 +275,5 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>   	pci_free_resource_list(&resources);
>>   
>>    err_init:
>> -	pm_runtime_put_sync(dev);
>> -
> 
> There is probably more you can do here for both -host and -ep:
> 
>   - Remove the possibly now unused <linux/pm_runtime.h> include
>   - Remove the err_init label and return directly from source.

Okay, will fix this up.

Thanks
Kishon

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

* Re: [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors
  2019-12-16 14:07   ` Andrew Murray
@ 2019-12-19 11:41     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 11:41 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi,

On 16/12/19 7:37 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:37PM +0530, Kishon Vijay Abraham I wrote:
>> Add support to use custom read and write accessors. Platforms that
>> doesn't support half word or byte access or any other constraint
> 
> s/doesn't/don't/
> 
>> while accessing registers can use this feature to populate custom
>> read and write accessors. These custom accessors are used for both
>> standard register access and configuration space register access.
> 
> You can put the following sentence underneath a --- as it's not needed
> in the commit message (but may be helpful to reviewers).
> 
>> This is in preparation for adding PCIe support in TI's J721E SoC which
>> uses Cadence PCIe core.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence.h | 99 +++++++++++++++++--
>>  1 file changed, 90 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index a2b28b912ca4..d0d91c69fa1d 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing {
>>  	MSG_ROUTING_GATHER,
>>  };
>>  
>> +struct cdns_pcie_ops {
>> +	u32	(*read)(void __iomem *addr, int size);
>> +	void	(*write)(void __iomem *addr, int size, u32 value);
>> +};
>> +
>>  /**
>>   * struct cdns_pcie - private data for Cadence PCIe controller drivers
>>   * @reg_base: IO mapped register base
>> @@ -239,7 +244,7 @@ struct cdns_pcie {
>>  	int			phy_count;
>>  	struct phy		**phy;
>>  	struct device_link	**link;
>> -	const struct cdns_pcie_common_ops *ops;
> 
> What was cdns_pcie_common_ops? It's not defined in the current tree is it?

Yeah, it's spurious change that has got merged.
> 
>> +	const struct cdns_pcie_ops *ops;
>>  };
>>  
>>  /**
>> @@ -301,21 +306,47 @@ struct cdns_pcie_ep {
>>  /* Register access */
>>  static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value)
>>  {
>> +	void __iomem *addr = pcie->reg_base + reg;
>> +
>> +	if (pcie->ops && pcie->ops->write) {
>> +		pcie->ops->write(addr, 0x1, value);
>> +		return;
>> +	}
>> +
>>  	writeb(value, pcie->reg_base + reg);
> 
> Can you use 'addr' here instead of 'pcie->reg_base + reg'? (And similar for the
> rest of them).

Sure.

Thanks
Kishon

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-16 14:49   ` Andrew Murray
@ 2019-12-19 11:56     ` Kishon Vijay Abraham I
  2019-12-19 12:03       ` Arnd Bergmann
  0 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 11:56 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Andrew,

On 16/12/19 8:19 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
>> Certain platforms like TI's J721E allow only 32-bit register accesses.
> 
> When I first read this I thought you meant only 32-bit accesses are allowed
> and not other sizes (such as 64-bit). However the limitation you address
> here is that the J721E allows only 32-bit *aligned* register accesses.

It's both, it allows only 32-bit aligned accesses and the size should be
only 32 bits. That's why I always use "readl" in the APIs below.
> 
> It would be helpful to make this clearer in the commit message.
> 
> You can also shorten the commit subject to 'PCI: cadence: Add read/write
> accessors for 32-bit aligned accesses' or similar.
> 
>> Add read and write accessors to perform only 32-bit accesses in order to
>> support platfroms like TI's J721E.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence.c | 40 +++++++++++++++++++
>>  drivers/pci/controller/cadence/pcie-cadence.h |  2 +
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
>> index cd795f6fc1e2..de5b3b06f2d0 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
>> @@ -7,6 +7,46 @@
>>  
>>  #include "pcie-cadence.h"
>>  
>> +u32 cdns_pcie_read32(void __iomem *addr, int size)
> 
> Given there is already a cdns_pcie_readl in pcie-cadence.h it may help
> to name this in a way that doesn't cause confusion. Here 32 is perhaps
> being used to suggest the size of the actual read performed, the
> maximum size of 'size' or the alignment.
> 
> 
>> +{
>> +	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
>> +	unsigned int offset = (unsigned long)addr & 0x3;
>> +	u32 val = readl(aligned_addr);
>> +
>> +	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> +		pr_err("Invalid Address in function:%s\n", __func__);
> 
> Would this be better as a BUG? Without a BUG this error could get ignored
> and yet the device may not behave as expected.

yeah.
> 
> 
>> +		return 0;
>> +	}
>> +
>> +	if (size > 2)
>> +		return val;
> 
> I think you make the assumption here that if size > 2 then it's 4. It could
> be 3 (though unlikely) in which case you'd want to fall through to the next
> line.

This assumption is used elsewhere too (e.g drivers/pci/access.c). I
generally don't prefer adding handlers for non-occurring error
scenarios, but If you insist I can fix that.

Thanks
Kishon

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

* Re: [PATCH 04/13] PCI: cadence: Add support to start link and verify link status
  2019-12-17 11:58   ` Andrew Murray
@ 2019-12-19 12:01     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 12:01 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Andrew,

On 17/12/19 5:28 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:38PM +0530, Kishon Vijay Abraham I wrote:
>> Add cdns_pcie_ops to start link and verify link status. The registers
>> to start link and to check link status is in Platform specific PCIe
>> wrapper. Add support for platform specific drivers to add callback
>> functions for the PCIe Cadence core to start link and verify link status.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../pci/controller/cadence/pcie-cadence-ep.c  |  8 ++++++
>>  .../controller/cadence/pcie-cadence-host.c    | 28 +++++++++++++++++++
>>  drivers/pci/controller/cadence/pcie-cadence.h | 23 +++++++++++++++
>>  3 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> index 560f22b4d165..088394b6be04 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> @@ -355,8 +355,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>>  {
>>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>>  	struct cdns_pcie *pcie = &ep->pcie;
>> +	struct device *dev = pcie->dev;
>>  	struct pci_epf *epf;
>>  	u32 cfg;
>> +	int ret;
>>  
>>  	/*
>>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
>> @@ -367,6 +369,12 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>>  		cfg |= BIT(epf->func_no);
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, cfg);
>>  
>> +	ret = cdns_pcie_start_link(pcie, true);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to start link\n");
>> +		return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index ccf55e143e1d..0929554f5a81 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -3,6 +3,7 @@
>>  // Cadence PCIe host controller driver.
>>  // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>>  
>> +#include <linux/delay.h>
>>  #include <linux/kernel.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_pci.h>
>> @@ -201,6 +202,23 @@ static int cdns_pcie_host_init(struct device *dev,
>>  	return err;
>>  }
>>  
>> +static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
>> +{
>> +	struct device *dev = pcie->dev;
>> +	int retries;
>> +
>> +	/* Check if the link is up or not */
>> +	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
>> +		if (cdns_pcie_is_link_up(pcie)) {
>> +			dev_info(dev, "Link up\n");
>> +			return 0;
>> +		}
>> +		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
> 
> This patch looks fine, except this function (above) is identical to
> dw_pcie_wait_for_link, advk_pcie_wait_for_link and nwl_wait_for_link. Even
> the definitions of LINK_WAIT_USLEEP_xx are the same.
> 
> I don't see any justification to duplicating this again - can you consolidate
> these functions to something that all controller drivers can use?

This involves reading a register, so this in entirety cannot be in a
generic layer. We could add "ops" for checking the link status (in
pci_ops?), but I'm not sure if that's really required.

Thanks
Kishon

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

* Re: [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops
  2019-12-17 12:32   ` Andrew Murray
@ 2019-12-19 12:02     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 12:02 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Andrew,

On 17/12/19 6:02 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:40PM +0530, Kishon Vijay Abraham I wrote:
>> Certain platforms like TI's J721E allows only 32-bit configuration
>> space access. In such cases pci_generic_config_read and
>> pci_generic_config_write cannot be used. Add support in Cadence core
>> to let pci_host_bridge have custom pci_ops.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence-host.c | 7 ++++---
>>  drivers/pci/controller/cadence/pcie-cadence.h      | 8 ++++++++
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 0929554f5a81..2efc33b1cade 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -12,8 +12,8 @@
>>  
>>  #include "pcie-cadence.h"
>>  
>> -static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>> -				      int where)
>> +void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>> +			       int where)
>>  {
>>  	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
>>  	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
>> @@ -289,7 +289,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>  	list_splice_init(&resources, &bridge->windows);
>>  	bridge->dev.parent = dev;
>>  	bridge->busnr = pcie->bus;
>> -	bridge->ops = &cdns_pcie_host_ops;
>> +	if (!bridge->ops)
>> +		bridge->ops = &cdns_pcie_host_ops;
>>  	bridge->map_irq = of_irq_parse_and_map_pci;
>>  	bridge->swizzle_irq = pci_common_swizzle;
>>  
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index 5171d0da37da..c879dd3d2893 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -472,11 +472,19 @@ static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
>>  
>>  #ifdef CONFIG_PCIE_CADENCE_HOST
>>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
>> +void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>> +			       int where);
> 
> The commit message doesn't explain why this change in visibility is needed).

So that platform drivers can write custom read() and write() ops and
re-use map_bus. Will add this info in commit message.
> 
>>  #else
>>  static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>>  {
>>  	return 0;
>>  }
>> +
>> +static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus,
>> +					     unsigned int devfn,
>> +					     int where)
>> +{
> 
> This still needs to return something right?

Right, thanks for spotting this.

Thanks
Kishon

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-19 11:56     ` Kishon Vijay Abraham I
@ 2019-12-19 12:03       ` Arnd Bergmann
  2019-12-19 13:19         ` Kishon Vijay Abraham I
  0 siblings, 1 reply; 41+ messages in thread
From: Arnd Bergmann @ 2019-12-19 12:03 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Andrew Murray, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
	linux-pci, DTML, linux-kernel, linux-omap

On Thu, Dec 19, 2019 at 12:54 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi Andrew,
>
> On 16/12/19 8:19 pm, Andrew Murray wrote:
> > On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
> >> Certain platforms like TI's J721E allow only 32-bit register accesses.
> >
> > When I first read this I thought you meant only 32-bit accesses are allowed
> > and not other sizes (such as 64-bit). However the limitation you address
> > here is that the J721E allows only 32-bit *aligned* register accesses.
>
> It's both, it allows only 32-bit aligned accesses and the size should be
> only 32 bits. That's why I always use "readl" in the APIs below.

In that case, can't you use the pci_generic_config_read32/write32
functions with a cadence specific .map_bus() function?

       Arnd

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

* Re: [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup
  2019-12-17 12:40   ` Andrew Murray
@ 2019-12-19 12:03     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 12:03 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi,

On 17/12/19 6:10 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:41PM +0530, Kishon Vijay Abraham I wrote:
>> Cadence driver uses "mem" memory resource to obtain the offset of
>> configuration space address region, memory space address region and
>> message space address region. The obtained offset is used to program
>> the Address Translation Unit (ATU). However certain platforms like TI's
>> J721E SoC require the absolute address to be programmed in the ATU and not
>> just the offset.
>>
>> The same problem was solved in designware driver using a platform specific
>> ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware:
> 
> Thanks for this reference, though this doesn't need to be in the commit
> log, please put such comments underneath a ---.
> 
>> Add new *ops* for CPU addr fixup"). Follow a similar mechanism in
>> Cadence too instead of directly using "mem" memory resource in Cadence
>> PCIe core.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../pci/controller/cadence/pcie-cadence-host.c    | 15 ++++-----------
>>  drivers/pci/controller/cadence/pcie-cadence.c     |  8 ++++++--
>>  drivers/pci/controller/cadence/pcie-cadence.h     |  1 +
>>  3 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 2efc33b1cade..cf817be237af 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>>  {
>>  	struct cdns_pcie *pcie = &rc->pcie;
>> -	struct resource *mem_res = pcie->mem_res;
>>  	struct resource *bus_range = rc->bus_range;
>>  	struct resource *cfg_res = rc->cfg_res;
>>  	struct device *dev = pcie->dev;
>>  	struct device_node *np = dev->of_node;
>>  	struct of_pci_range_parser parser;
>> +	u64 cpu_addr = cfg_res->start;
>>  	struct of_pci_range range;
>>  	u32 addr0, addr1, desc1;
>> -	u64 cpu_addr;
>>  	int r, err;
>>  
>>  	/*
>> @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1);
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1);
>>  
>> -	cpu_addr = cfg_res->start - mem_res->start;
>> +	if (pcie->ops->cpu_addr_fixup)
>> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);
>> +
> 
> Won't this patch cause a breakage for existing users that won't have defined a
> cpu_addr_fixup? The offset isn't being calculated and so cpu_addr will be wrong?

Correct, this will need an additional patch in pcie-cadence-plat.c.

Thanks
Kishon

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

* Re: [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID
  2019-12-17 12:42   ` Andrew Murray
@ 2019-12-19 12:12     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 12:12 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Arnd Bergmann,
	linux-pci, devicetree, linux-kernel, linux-omap, Tom Joseph

+Tom

Hi,

On 17/12/19 6:12 pm, Andrew Murray wrote:
> On Mon, Dec 09, 2019 at 02:51:42PM +0530, Kishon Vijay Abraham I wrote:
>> PCI_VENDOR_ID in root port configuration space is read-only register
>> and writing to it will have no effect. Use local management register to
>> configure Vendor ID and Subsystem Vendor ID.
> 
> Is this a bug fix? Can you add a Fixes tag and make that clearer?

I think this might have worked in Cadence platform? Tom?

Thanks
Kishon

> 
> Thanks,
> 
> Andrew Murray
> 
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence-host.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index cf817be237af..afb2c96a6538 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -71,6 +71,7 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  {
>>  	struct cdns_pcie *pcie = &rc->pcie;
>>  	u32 value, ctrl;
>> +	u32 id;
>>  
>>  	/*
>>  	 * Set the root complex BAR configuration register:
>> @@ -90,8 +91,12 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>>  
>>  	/* Set root port configuration space */
>> -	if (rc->vendor_id != 0xffff)
>> -		cdns_pcie_rp_writew(pcie, PCI_VENDOR_ID, rc->vendor_id);
>> +	if (rc->vendor_id != 0xffff) {
>> +		id = CDNS_PCIE_LM_ID_VENDOR(rc->vendor_id) |
>> +			CDNS_PCIE_LM_ID_SUBSYS(rc->vendor_id);
>> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, id);
>> +	}
>> +
>>  	if (rc->device_id != 0xffff)
>>  		cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
>>  
>> -- 
>> 2.17.1
>>

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-17 23:36   ` Bjorn Helgaas
@ 2019-12-19 12:49     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 12:49 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Lorenzo Pieralisi, Rob Herring, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Andrew,

On 18/12/19 5:06 am, Bjorn Helgaas wrote:
> On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
>> Certain platforms like TI's J721E allow only 32-bit register accesses.
>> Add read and write accessors to perform only 32-bit accesses in order to
>> support platfroms like TI's J721E.
> 
> s/platfroms/platforms/
> 
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/cadence/pcie-cadence.c | 40 +++++++++++++++++++
>>  drivers/pci/controller/cadence/pcie-cadence.h |  2 +
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
>> index cd795f6fc1e2..de5b3b06f2d0 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
>> @@ -7,6 +7,46 @@
>>  
>>  #include "pcie-cadence.h"
>>  
>> +u32 cdns_pcie_read32(void __iomem *addr, int size)
>> +{
>> +	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
>> +	unsigned int offset = (unsigned long)addr & 0x3;
>> +	u32 val = readl(aligned_addr);
>> +
>> +	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> +		pr_err("Invalid Address in function:%s\n", __func__);
> 
> It might be nice to have a hint about *why* it's invalid, e.g., the
> addr and size values.

Sure.

Thanks
Kishon

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

* Re: [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC
  2019-12-19  0:08   ` Rob Herring
@ 2019-12-19 13:13     ` Kishon Vijay Abraham I
  2019-12-24  8:06     ` Kishon Vijay Abraham I
  1 sibling, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 13:13 UTC (permalink / raw)
  To: Rob Herring
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi,

On 19/12/19 5:38 am, Rob Herring wrote:
> On Mon, Dec 09, 2019 at 02:51:43PM +0530, Kishon Vijay Abraham I wrote:
>> Add host mode dt-bindings for TI's J721E SoC.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 161 ++++++++++++++++++
>>  1 file changed, 161 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> new file mode 100644
>> index 000000000000..96184e1f419f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> @@ -0,0 +1,161 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-host.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI J721E PCI Host (PCIe Wrapper)
>> +
>> +maintainers:
>> +  - Kishon Vijay Abraham I <kishon@ti.com>
> 
> There's now a PCI bus schema. Reference it here:
> 
> allOf:
>   - $ref: "/schemas/pci/pci-bus.yaml#"
> 
>> +
>> +properties:
>> +  compatible:
>> +      enum:
>> +          - ti,j721e-pcie-host
> 
> Indentation.
> 
>> +
>> +  reg:
>> +    maxItems: 4
>> +
>> +  reg-names:
>> +    items:
>> +      - const: intd_cfg
>> +      - const: user_cfg
>> +      - const: reg
>> +      - const: cfg
>> +
>> +  ti,syscon-pcie-ctrl:
>> +    description: Phandle to the SYSCON entry required for configuring PCIe mode
>> +                 and link speed.
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/phandle
> 
> You can drop the 'allOf' here if there aren't more constraints.
> 
>> +
>> +  max-link-speed:
>> +    minimum: 1
>> +    maximum: 3
>> +
>> +  num-lanes:
>> +    minimum: 1
>> +    maximum: 2
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: clock-specifier to represent input to the PCIe
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fck
>> +
> 
>> +  "#address-cells":
>> +    const: 3
>> +
>> +  "#size-cells":
>> +    const: 2
>> +
>> +  bus-range:
>> +    description: Range of bus numbers associated with this controller.
> 
> Drop these 3 as they are all standard.
> 
>> +
>> +  cdns,max-outbound-regions:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/int32
> 
> Can be negative? Use uint32.
> 
> The int* definitions are kind of broken until dtc is fixed to maintain 
> sign.
> 
>> +      - enum: [16]
> 
> const: 16
> 
>> +
>> +  cdns,no-bar-match-nbits:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/int32
>> +      - enum: [64]
>> +
>> +  vendor-id:
>> +    const: 0x104c
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> 
> And elsewhere. Drop the description.
> 
>> +
>> +  device-id:
>> +    const: 0xb00d
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +
>> +  msi-map: true
>> +
>> +  dma-coherent:
>> +    description: Indicates that the PCIe IP block can ensure the coherency
>> +
>> +  ranges: true
> 
> Don't you know how many?
> 
>> +
>> +  reset-gpios:
>> +    description: GPIO specifier for the PERST# signal
>> +
>> +  phys:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +
>> +  phy-names:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - ti,syscon-pcie-ctrl
>> +  - max-link-speed
>> +  - num-lanes
>> +  - power-domains
>> +  - clocks
>> +  - clock-names
> 
>> +  - "#address-cells"
>> +  - "#size-cells"
> 
> Can drop these 2. The bus schema requires them.
> 
>> +  - bus-range
>> +  - cdns,max-outbound-regions
>> +  - cdns,no-bar-match-nbits
>> +  - vendor-id
>> +  - device-id
>> +  - msi-map
>> +  - dma-coherent
>> +  - ranges
> 
> Can drop, too.
> 
>> +  - reset-gpios
> 
> Isn't having this board dependent?

All board dts files should have it though.
I'll fix all the comments given here.

Thanks
Kishon

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

* Re: [PATCH 10/13] dt-bindings: PCI: Add EP mode dt-bindings for TI's J721E SoC
  2019-12-19  0:14   ` Rob Herring
@ 2019-12-19 13:14     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 13:14 UTC (permalink / raw)
  To: Rob Herring, Tom Joseph
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

+Tom

On 19/12/19 5:44 am, Rob Herring wrote:
> On Mon, Dec 09, 2019 at 02:51:44PM +0530, Kishon Vijay Abraham I wrote:
>> Add PCIe EP mode dt-bindings for TI's J721E SoC.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-ep.yaml         | 113 ++++++++++++++++++
>>  1 file changed, 113 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>> new file mode 100644
>> index 000000000000..4e2af4733998
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml
>> @@ -0,0 +1,113 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-ep.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI J721E PCI EP (PCIe Wrapper)
>> +
>> +maintainers:
>> +  - Kishon Vijay Abraham I <kishon@ti.com>
>> +
>> +properties:
>> +  compatible:
>> +      enum:
>> +          - ti,j721e-pcie-ep
> 
> Indentation.
> 
>> +
>> +  reg:
>> +    maxItems: 4
>> +
>> +  reg-names:
>> +    items:
>> +      - const: intd_cfg
>> +      - const: user_cfg
>> +      - const: reg
>> +      - const: mem
>> +
>> +  ti,syscon-pcie-ctrl:
>> +    description: Phandle to the SYSCON entry required for configuring PCIe mode
>> +                 and link speed.
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> +  max-link-speed:
>> +    minimum: 1
>> +    maximum: 3
>> +
>> +  num-lanes:
>> +    minimum: 1
>> +    maximum: 2
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: clock-specifier to represent input to the PCIe
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fck
>> +
>> +  cdns,max-outbound-regions:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/int32
> 
> uint32
> 
>> +      - enum: [16]
>> +
>> +  max-functions:
>> +    minimum: 1
>> +    maximum: 6
> 
> Needs a type ref. Or a common definition.
> 
>> +
>> +  dma-coherent:
>> +    description: Indicates that the PCIe IP block can ensure the coherency
>> +
>> +  phys:
> 
> How many? Need to convert cdns,cdns-pcie-host.txt...


Tom, Can you convert cdns,cdns-pcie-host.txt to YAML binding?
> 
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
>> +
>> +  phy-names:
>> +    description: As defined in
>> +                 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> 
> For all the properties shared with host mode, it might make sense to 
> define a common schema with all those properties and then include it in 
> the host and endpoint schemas.

Sure.

Thanks
Kishon

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-19 12:03       ` Arnd Bergmann
@ 2019-12-19 13:19         ` Kishon Vijay Abraham I
  2019-12-19 20:16           ` Arnd Bergmann
  0 siblings, 1 reply; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-19 13:19 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrew Murray, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
	linux-pci, DTML, linux-kernel, linux-omap

Hi Arnd,

On 19/12/19 5:33 pm, Arnd Bergmann wrote:
> On Thu, Dec 19, 2019 at 12:54 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Andrew,
>>
>> On 16/12/19 8:19 pm, Andrew Murray wrote:
>>> On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
>>>> Certain platforms like TI's J721E allow only 32-bit register accesses.
>>>
>>> When I first read this I thought you meant only 32-bit accesses are allowed
>>> and not other sizes (such as 64-bit). However the limitation you address
>>> here is that the J721E allows only 32-bit *aligned* register accesses.
>>
>> It's both, it allows only 32-bit aligned accesses and the size should be
>> only 32 bits. That's why I always use "readl" in the APIs below.
> 
> In that case, can't you use the pci_generic_config_read32/write32
> functions with a cadence specific .map_bus() function?

pci_generic_config_read32() is for reading configuration space registers
only. The accessors I added here are for the controller IP configuration.

For the configuration space access I use
pci_generic_config_read32/write32()([PATCH 11/13] PCI: j721e: Add TI
J721E PCIe driver).

Thanks
Kishon

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

* Re: [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses
  2019-12-19 13:19         ` Kishon Vijay Abraham I
@ 2019-12-19 20:16           ` Arnd Bergmann
  0 siblings, 0 replies; 41+ messages in thread
From: Arnd Bergmann @ 2019-12-19 20:16 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Andrew Murray, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring,
	linux-pci, DTML, linux-kernel, linux-omap

On Thu, Dec 19, 2019 at 2:17 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On 19/12/19 5:33 pm, Arnd Bergmann wrote:
> > On Thu, Dec 19, 2019 at 12:54 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> Hi Andrew,
> >>
> >> On 16/12/19 8:19 pm, Andrew Murray wrote:
> >>> On Mon, Dec 09, 2019 at 02:51:39PM +0530, Kishon Vijay Abraham I wrote:
> >>>> Certain platforms like TI's J721E allow only 32-bit register accesses.
> >>>
> >>> When I first read this I thought you meant only 32-bit accesses are allowed
> >>> and not other sizes (such as 64-bit). However the limitation you address
> >>> here is that the J721E allows only 32-bit *aligned* register accesses.
> >>
> >> It's both, it allows only 32-bit aligned accesses and the size should be
> >> only 32 bits. That's why I always use "readl" in the APIs below.
> >
> > In that case, can't you use the pci_generic_config_read32/write32
> > functions with a cadence specific .map_bus() function?
>
> pci_generic_config_read32() is for reading configuration space registers
> only. The accessors I added here are for the controller IP configuration.
>
> For the configuration space access I use
> pci_generic_config_read32/write32()([PATCH 11/13] PCI: j721e: Add TI
> J721E PCIe driver).

Got it, thanks for the clarification.

       Arnd

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

* Re: [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver
  2019-12-09  9:21 ` [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
  2019-12-17 14:23   ` Andrew Murray
@ 2019-12-19 22:47   ` Bjorn Helgaas
  1 sibling, 0 replies; 41+ messages in thread
From: Bjorn Helgaas @ 2019-12-19 22:47 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Lorenzo Pieralisi, Rob Herring, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

On Mon, Dec 09, 2019 at 02:51:45PM +0530, Kishon Vijay Abraham I wrote:
> Add support for PCIe controller in J721E SoC. The controller uses the
> Cadence PCIe core programmed by pcie-cadence*.c. The PCIe controller
> will work in both host mode and device mode.
> Some of the features of the controller are:
>   *) Supports both RC mode and EP mode
>   *) Supports MSI and MSI-X support
>   *) Supports upto GEN3 speed mode
>   *) Supports SR-IOV capability
>   *) Ability to route all transactions via SMMU (support will be added
>      in a later patch).
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/cadence/Kconfig     |  23 ++
>  drivers/pci/controller/cadence/Makefile    |   1 +
>  drivers/pci/controller/cadence/pci-j721e.c | 430 +++++++++++++++++++++
>  3 files changed, 454 insertions(+)
>  create mode 100644 drivers/pci/controller/cadence/pci-j721e.c
> 
> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index b76b3cf55ce5..5d30564190e1 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -42,4 +42,27 @@ config PCIE_CADENCE_PLAT_EP
>  	  endpoint mode. This PCIe controller may be embedded into many
>  	  different vendors SoCs.
>  
> +config PCI_J721E
> +	bool
> +
> +config PCI_J721E_HOST
> +	bool "TI J721E PCIe platform host controller"
> +	depends on OF
> +	select PCIE_CADENCE_HOST
> +	select PCI_J721E
> +	help
> +	  Say Y here if you want to support the TI J721E PCIe platform
> +	  controller in host mode. TI J721E PCIe controller uses Cadence PCIe
> +	  core.
> +
> +config PCI_J721E_EP
> +	bool "TI J721E PCIe platform endpoint controller"

Most drivers call these "PCIe host controller" and "PCIe endpoint
controller" or similar.  Does adding "platform" indicate something
useful?

> +	depends on OF
> +	depends on PCI_ENDPOINT
> +	select PCIE_CADENCE_EP
> +	select PCI_J721E
> +	help
> +	  Say Y here if you want to support the TI J721E PCIe platform
> +	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
> +	  core.
>  endmenu
> diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile
> index 232a3f20876a..9bac5fb2f13d 100644
> --- a/drivers/pci/controller/cadence/Makefile
> +++ b/drivers/pci/controller/cadence/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence.o
>  obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
>  obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
>  obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
> +obj-$(CONFIG_PCI_J721E) += pci-j721e.o
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> new file mode 100644
> index 000000000000..9ffb7e88c739
> --- /dev/null
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -0,0 +1,430 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * pci-j721e - PCIe controller driver for TI's J721E SoCs
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/io.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include "../../pci.h"
> +#include "pcie-cadence.h"
> +
> +#define J721E_PCIE_USER_CMD_STATUS	0x4
> +#define LINK_TRAINING_ENABLE		BIT(0)
> +
> +#define J721E_PCIE_USER_LINKSTATUS	0x14
> +#define LINK_STATUS			GENMASK(1, 0)
> +
> +enum link_status {
> +	NO_RECIEVERS_DETECTED,

s/NO_RECIEVERS_DETECTED/NO_RECEIVERS_DETECTED/

> +	LINK_TRAINING_IN_PROGRESS,
> +	LINK_UP_DL_IN_PROGRESS,
> +	LINK_UP_DL_COMPLETED,
> +};
> +
> +#define J721E_MODE_RC			BIT(7)
> +#define LANE_COUNT_MASK			BIT(8)
> +#define LANE_COUNT(n)			((n) << 8)
> +
> +#define GENERATION_SEL_MASK		GENMASK(1, 0)
> +
> +#define MAX_LANES			2
> +
> +struct j721e_pcie {
> +	struct device		*dev;
> +	struct device_node	*node;
> +	u32			mode;
> +	u32			num_lanes;
> +	struct cdns_pcie	*cdns_pcie;
> +	void __iomem		*user_cfg_base;
> +};
> +
> +enum j721e_pcie_mode {
> +	PCI_MODE_RC,
> +	PCI_MODE_EP,
> +};
> +
> +struct j721e_pcie_data {
> +	enum j721e_pcie_mode	mode;
> +};
> +
> +static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
> +{
> +	return readl(pcie->user_cfg_base + offset);
> +}
> +
> +static inline void j721e_pcie_user_writel(struct j721e_pcie *pcie, u32 offset,
> +					  u32 value)
> +{
> +	writel(value, pcie->user_cfg_base + offset);
> +}
> +
> +static int j721e_pcie_start_link(struct cdns_pcie *cdns_pcie, bool start)
> +{
> +	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> +	u32 reg;
> +
> +	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_CMD_STATUS);
> +	if (start)
> +		reg |= LINK_TRAINING_ENABLE;
> +	else
> +		reg &= ~LINK_TRAINING_ENABLE;
> +	j721e_pcie_user_writel(pcie, J721E_PCIE_USER_CMD_STATUS, reg);
> +
> +	return 0;
> +}
> +
> +static bool j721e_pcie_is_link_up(struct cdns_pcie *cdns_pcie)

There are many *_pcie_link_up() definitions that looks essentially
like this; maybe this could be simply j721e_pcie_link_up() to match?

> +{
> +	struct j721e_pcie *pcie = dev_get_drvdata(cdns_pcie->dev);
> +	u32 reg;
> +
> +	reg = j721e_pcie_user_readl(pcie, J721E_PCIE_USER_LINKSTATUS);
> +	reg &= LINK_STATUS;
> +	if (reg == LINK_UP_DL_COMPLETED)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct cdns_pcie_ops j721e_ops_ops = {
> +	.read = cdns_pcie_read32,
> +	.write = cdns_pcie_write32,
> +	.start_link = j721e_pcie_start_link,
> +	.is_link_up = j721e_pcie_is_link_up,
> +};

Can these match struct dw_pcie_ops more closely, e.g., ".link_up"
instead of ".is_link_up", ".start_link" and ".stop_link" instead of
".start_link(..., bool)"?

Bjorn

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

* Re: [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC
  2019-12-19  0:08   ` Rob Herring
  2019-12-19 13:13     ` Kishon Vijay Abraham I
@ 2019-12-24  8:06     ` Kishon Vijay Abraham I
  1 sibling, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2019-12-24  8:06 UTC (permalink / raw)
  To: Rob Herring
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Arnd Bergmann, Andrew Murray,
	linux-pci, devicetree, linux-kernel, linux-omap

Hi Rob,

On 19/12/19 5:38 AM, Rob Herring wrote:
> On Mon, Dec 09, 2019 at 02:51:43PM +0530, Kishon Vijay Abraham I wrote:
>> Add host mode dt-bindings for TI's J721E SoC.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  .../bindings/pci/ti,j721e-pci-host.yaml       | 161 ++++++++++++++++++
>>  1 file changed, 161 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> new file mode 100644
>> index 000000000000..96184e1f419f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
>> @@ -0,0 +1,161 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/pci/ti,j721e-pci-host.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: TI J721E PCI Host (PCIe Wrapper)
>> +
>> +maintainers:
>> +  - Kishon Vijay Abraham I <kishon@ti.com>
> 
> There's now a PCI bus schema. Reference it here:
> 
> allOf:
>   - $ref: "/schemas/pci/pci-bus.yaml#"
> 
>> +
>> +properties:
>> +  compatible:
>> +      enum:
>> +          - ti,j721e-pcie-host
> 
> Indentation.
> 
>> +
>> +  reg:
>> +    maxItems: 4
>> +
>> +  reg-names:
>> +    items:
>> +      - const: intd_cfg
>> +      - const: user_cfg
>> +      - const: reg
>> +      - const: cfg
>> +
>> +  ti,syscon-pcie-ctrl:
>> +    description: Phandle to the SYSCON entry required for configuring PCIe mode
>> +                 and link speed.
>> +    allOf:
>> +      - $ref: /schemas/types.yaml#/definitions/phandle
> 
> You can drop the 'allOf' here if there aren't more constraints.

Do you mean I don't have to include phandle schema here? I don't seem to
be able to include $ref without allOf.

Thanks
Kishon

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

end of thread, other threads:[~2019-12-24  8:04 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09  9:21 [PATCH 00/13] Add PCIe support to TI's J721E SoC Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 01/13] PCI: cadence: Remove stray "pm_runtime_put_sync()" in error path Kishon Vijay Abraham I
2019-12-16 13:45   ` Andrew Murray
2019-12-19  8:31     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 02/13] linux/kernel.h: Add PTR_ALIGN_DOWN macro Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 03/13] PCI: cadence: Add support to use custom read and write accessors Kishon Vijay Abraham I
2019-12-16 14:07   ` Andrew Murray
2019-12-19 11:41     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 04/13] PCI: cadence: Add support to start link and verify link status Kishon Vijay Abraham I
2019-12-17 11:58   ` Andrew Murray
2019-12-19 12:01     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 05/13] PCI: cadence: Add read and write accessors to perform only 32-bit accesses Kishon Vijay Abraham I
2019-12-09 21:15   ` Bjorn Helgaas
2019-12-16 14:49   ` Andrew Murray
2019-12-19 11:56     ` Kishon Vijay Abraham I
2019-12-19 12:03       ` Arnd Bergmann
2019-12-19 13:19         ` Kishon Vijay Abraham I
2019-12-19 20:16           ` Arnd Bergmann
2019-12-17 23:36   ` Bjorn Helgaas
2019-12-19 12:49     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 06/13] PCI: cadence: Allow pci_host_bridge to have custom pci_ops Kishon Vijay Abraham I
2019-12-17 12:32   ` Andrew Murray
2019-12-19 12:02     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 07/13] PCI: cadence: Add new *ops* for CPU addr fixup Kishon Vijay Abraham I
2019-12-17 12:40   ` Andrew Murray
2019-12-19 12:03     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 08/13] PCI: cadence: Use local management register to configure Vendor ID Kishon Vijay Abraham I
2019-12-17 12:42   ` Andrew Murray
2019-12-19 12:12     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 09/13] dt-bindings: PCI: Add host mode dt-bindings for TI's J721E SoC Kishon Vijay Abraham I
2019-12-19  0:08   ` Rob Herring
2019-12-19 13:13     ` Kishon Vijay Abraham I
2019-12-24  8:06     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 10/13] dt-bindings: PCI: Add EP " Kishon Vijay Abraham I
2019-12-19  0:14   ` Rob Herring
2019-12-19 13:14     ` Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 11/13] PCI: j721e: Add TI J721E PCIe driver Kishon Vijay Abraham I
2019-12-17 14:23   ` Andrew Murray
2019-12-19 22:47   ` Bjorn Helgaas
2019-12-09  9:21 ` [PATCH 12/13] misc: pci_endpoint_test: Add J721E in pci_device_id table Kishon Vijay Abraham I
2019-12-09  9:21 ` [PATCH 13/13] MAINTAINERS: Add Kishon Vijay Abraham I for TI J721E SoC PCIe Kishon Vijay Abraham I

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