[PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Damien Le Moal]

This patch series fix the PCI address mapping handling of the Rockchip
PCI endpoint driver, refactor some of its code, improves link training
and adds handling of the PERST# signal.

This series is organized as follows:
 - Patch 1 fixes the rockchip ATU programming
 - Patch 2, 3 and 4 introduce small code improvments
 - Patch 5 implements the .align_addr() operation to make the RK3399
   endpoint controller driver fully functional with the new
   pci_epc_mem_map() function
 - Patch 6 uses the new align_addr operation function to fix the ATU
   programming for MSI IRQ data mapping
 - Patch 7, 8, 9 and 10 refactor the driver code to make it more
   readable
 - Patch 11 introduces the .stop() endpoint controller operation to
   correctly disable the endpopint controller after use
 - Patch 12 improves link training
 - Patch 13 implements handling of the #PERST signal
 - Patch 14 adds a DT overlay file to enable EP mode and define the
   PERST# GPIO (reset-gpios) property.

These patches were tested using a Pine Rockpro64 board used as an
endpoint with the test endpoint function driver and a prototype nvme
endpoint function driver.

Changes from v4:
 - Added patch 6
 - Added comments to patch 12 and 13 to clarify link training handling
   and PERST# GPIO use.
 - Added patch 14

Changes from v3:
 - Addressed Mani's comments (see mailing list for details).
 - Removed old patch 11 (dt-binding changes) and instead use in patch 12
   the already defined reset_gpios property.
 - Added patch 6
 - Added review tags

Changes from v2:
 - Split the patch series
 - Corrected patch 11 to add the missing "maxItem"

Changes from v1:
 - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
   1.
 - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
   (former patch 2 of v1)
 - Various typos cleanups all over. Also fixed some blank space
   indentation.
 - Added review tags

Damien Le Moal (14):
  PCI: rockchip-ep: Fix address translation unit programming
  PCI: rockchip-ep: Use a macro to define EP controller .align feature
  PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
  PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
  PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
  PCI: rockchip-ep: Fix MSI IRQ data mapping
  PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
  PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
  PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
  PCI: rockchip-ep: Refactor endpoint link training enable
  PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
  PCI: rockchip-ep: Improve link training
  PCI: rockchip-ep: Handle PERST# signal in endpoint mode
  arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode

 arch/arm64/boot/dts/rockchip/Makefile         |   1 +
 .../rockchip/rk3399-rockpro64-pcie-ep.dtso    |  20 +
 drivers/pci/controller/pcie-rockchip-ep.c     | 432 ++++++++++++++----
 drivers/pci/controller/pcie-rockchip-host.c   |   4 +-
 drivers/pci/controller/pcie-rockchip.c        |  21 +-
 drivers/pci/controller/pcie-rockchip.h        |  24 +-
 6 files changed, 406 insertions(+), 96 deletions(-)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso

-- 
2.47.0

[PATCH v5 01/14] PCI: rockchip-ep: Fix address translation unit programming

[Damien Le Moal]

The rockchip PCIe endpoint controller handles PCIe transfers addresses
by masking the lower bits of the programmed PCI address and using the
same number of lower bits masked from the CPU address space used for the
mapping. For a PCI mapping of <size> bytes starting from <pci_addr>,
the number of bits masked is the number of address bits changing in the
address range [pci_addr..pci_addr + size - 1].

However, rockchip_pcie_prog_ep_ob_atu() calculates num_pass_bits only
using the size of the mapping, resulting in an incorrect number of mask
bits depending on the value of the PCI address to map.

Fix this by introducing the helper function
rockchip_pcie_ep_ob_atu_num_bits() to correctly calculate the number of
mask bits to use to program the address translation unit. The number of
mask bits is calculated depending on both the PCI address and size of
the mapping, and clamped between 8 and 20 using the macros
ROCKCHIP_PCIE_AT_MIN_NUM_BITS and ROCKCHIP_PCIE_AT_MAX_NUM_BITS. As
defined in the Rockchip RK3399 TRM V1.3 Part2, Sections 17.5.5.1.1 and
17.6.8.2.1, this clamping is necessary because:
1) The lower 8 bits of the PCI address to be mapped by the outbound
   region are ignored. So a minimum of 8 address bits are needed and
   imply that the PCI address must be aligned to 256.
2) The outbound memory regions are 1MB in size. So while we can specify
   up to 63-bits for the PCI address (num_bits filed uses bits 0 to 5 of
   the outbound address region 0 register), we must limit the number of
   valid address bits to 20 to match the memory window maximum size (1
   << 20 = 1MB).

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 15 +++++++++++----
 drivers/pci/controller/pcie-rockchip.h    |  4 ++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 136274533656..27a7febb74e0 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -63,16 +63,23 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
 			    ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
 }
 
+static int rockchip_pcie_ep_ob_atu_num_bits(struct rockchip_pcie *rockchip,
+					    u64 pci_addr, size_t size)
+{
+	int num_pass_bits = fls64(pci_addr ^ (pci_addr + size - 1));
+
+	return clamp(num_pass_bits, ROCKCHIP_PCIE_AT_MIN_NUM_BITS,
+		     ROCKCHIP_PCIE_AT_MAX_NUM_BITS);
+}
+
 static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
 					 u32 r, u64 cpu_addr, u64 pci_addr,
 					 size_t size)
 {
-	int num_pass_bits = fls64(size - 1);
+	int num_pass_bits =
+		rockchip_pcie_ep_ob_atu_num_bits(rockchip, pci_addr, size);
 	u32 addr0, addr1, desc0;
 
-	if (num_pass_bits < 8)
-		num_pass_bits = 8;
-
 	addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
 		(lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
 	addr1 = upper_32_bits(pci_addr);
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 6111de35f84c..15ee949f2485 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -245,6 +245,10 @@
 	(PCIE_EP_PF_CONFIG_REGS_BASE + (((fn) << 12) & GENMASK(19, 12)))
 #define ROCKCHIP_PCIE_EP_VIRT_FUNC_BASE(fn) \
 	(PCIE_EP_PF_CONFIG_REGS_BASE + 0x10000 + (((fn) << 12) & GENMASK(19, 12)))
+
+#define ROCKCHIP_PCIE_AT_MIN_NUM_BITS  8
+#define ROCKCHIP_PCIE_AT_MAX_NUM_BITS  20
+
 #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
 	(PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
 #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \
-- 
2.47.0

[PATCH v5 02/14] PCI: rockchip-ep: Use a macro to define EP controller .align feature

[Damien Le Moal]

Introduce the macro ROCKCHIP_PCIE_AT_SIZE_ALIGN to initialize the .align
field of the controller epc_features structure to 256. This is defined
as a shift using the macro ROCKCHIP_PCIE_AT_MIN_NUM_BITS (to avoid
using the "magic" value 8 directly).

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 2 +-
 drivers/pci/controller/pcie-rockchip.h    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 27a7febb74e0..5a07084fb7c4 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -446,7 +446,7 @@ static const struct pci_epc_features rockchip_pcie_epc_features = {
 	.linkup_notifier = false,
 	.msi_capable = true,
 	.msix_capable = false,
-	.align = 256,
+	.align = ROCKCHIP_PCIE_AT_SIZE_ALIGN,
 };
 
 static const struct pci_epc_features*
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 15ee949f2485..02368ce9bd54 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -248,6 +248,7 @@
 
 #define ROCKCHIP_PCIE_AT_MIN_NUM_BITS  8
 #define ROCKCHIP_PCIE_AT_MAX_NUM_BITS  20
+#define ROCKCHIP_PCIE_AT_SIZE_ALIGN    (1UL << ROCKCHIP_PCIE_AT_MIN_NUM_BITS)
 
 #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
 	(PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
-- 
2.47.0

[PATCH v5 03/14] PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()

[Damien Le Moal]

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>

There is no need to loop over all regions to find the memory window used
to map an address. We can use rockchip_ob_region() to determine the
region index, together with a check that the address passed as argument
is the address used to create the mapping. Furthermore, the
ob_region_map bitmap should also be checked to ensure that we are not
attempting to unmap an address that is not mapped.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 5a07084fb7c4..89ebdf3e4737 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -256,13 +256,9 @@ static void rockchip_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
 {
 	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
 	struct rockchip_pcie *rockchip = &ep->rockchip;
-	u32 r;
-
-	for (r = 0; r < ep->max_regions; r++)
-		if (ep->ob_addr[r] == addr)
-			break;
+	u32 r = rockchip_ob_region(addr);
 
-	if (r == ep->max_regions)
+	if (addr != ep->ob_addr[r] || !test_bit(r, &ep->ob_region_map))
 		return;
 
 	rockchip_pcie_clear_ep_ob_atu(rockchip, r);
-- 
2.47.0

[PATCH v5 04/14] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Damien Le Moal]

Add a check to verify that the outbound region to be used for mapping an
address is not already in use.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 89ebdf3e4737..edb84fb1ba39 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -243,6 +243,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
 	struct rockchip_pcie *pcie = &ep->rockchip;
 	u32 r = rockchip_ob_region(addr);
 
+	if (test_bit(r, &ep->ob_region_map))
+		return -EBUSY;
+
 	rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size);
 
 	set_bit(r, &ep->ob_region_map);
-- 
2.47.0

[PATCH v5 05/14] PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation

[Damien Le Moal]

The rockchip PCIe endpoint controller handles PCIe transfers addresses
by masking the lower bits of the programmed PCI address and using the
same number of lower bits from the CPU address space used for the
mapping. For a PCI mapping of size bytes starting from pci_addr, the
number of bits masked is the number of address bits changing in the
address range [pci_addr..pci_addr + size - 1], up to 20 bits, that is,
up to 1MB mappings.

This means that when preparing a PCI address mapping, an endpoint
function driver must use an offset into the allocated controller
memory region that is equal to the mask of the starting PCI address
over rockchip_pcie_ep_ob_atu_num_bits() bits. This offset also
determines the maximum size of the mapping given the starting PCI
address and the fixed 1MB controller memory window size.

Implement the ->align_addr() endpoint controller operation to allow the
mapping alignment to be transparently handled by endpoint function
drivers through the function pci_epc_mem_map().

Co-developed-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 23 +++++++++++++++++++++++
 drivers/pci/controller/pcie-rockchip.h    |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index edb84fb1ba39..f6959f9b94b7 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -235,6 +235,28 @@ static inline u32 rockchip_ob_region(phys_addr_t addr)
 	return (addr >> ilog2(SZ_1M)) & 0x1f;
 }
 
+static u64 rockchip_pcie_ep_align_addr(struct pci_epc *epc, u64 pci_addr,
+				       size_t *pci_size, size_t *offset)
+{
+	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
+	size_t size = *pci_size;
+	u64 ofst, mask;
+	int num_bits;
+
+	num_bits = rockchip_pcie_ep_ob_atu_num_bits(&ep->rockchip,
+						    pci_addr, size);
+	mask = (1ULL << num_bits) - 1;
+
+	ofst = pci_addr & mask;
+	if (size + ofst > SZ_1M)
+		size = SZ_1M - ofst;
+
+	*pci_size = ALIGN(ofst + size, ROCKCHIP_PCIE_AT_SIZE_ALIGN);
+	*offset = ofst;
+
+	return pci_addr & ~mask;
+}
+
 static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
 				     phys_addr_t addr, u64 pci_addr,
 				     size_t size)
@@ -458,6 +480,7 @@ static const struct pci_epc_ops rockchip_pcie_epc_ops = {
 	.write_header	= rockchip_pcie_ep_write_header,
 	.set_bar	= rockchip_pcie_ep_set_bar,
 	.clear_bar	= rockchip_pcie_ep_clear_bar,
+	.align_addr	= rockchip_pcie_ep_align_addr,
 	.map_addr	= rockchip_pcie_ep_map_addr,
 	.unmap_addr	= rockchip_pcie_ep_unmap_addr,
 	.set_msi	= rockchip_pcie_ep_set_msi,
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 02368ce9bd54..30398156095f 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -241,6 +241,11 @@
 #define   ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK		GENMASK(15, 8)
 #define ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR				0x1
 #define ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR		0x3
+
+#define ROCKCHIP_PCIE_AT_MIN_NUM_BITS	8
+#define ROCKCHIP_PCIE_AT_MAX_NUM_BITS	20
+#define ROCKCHIP_PCIE_AT_SIZE_ALIGN	(1UL << ROCKCHIP_PCIE_AT_MIN_NUM_BITS)
+
 #define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) \
 	(PCIE_EP_PF_CONFIG_REGS_BASE + (((fn) << 12) & GENMASK(19, 12)))
 #define ROCKCHIP_PCIE_EP_VIRT_FUNC_BASE(fn) \
-- 
2.47.0

[PATCH v5 06/14] PCI: rockchip-ep: Fix MSI IRQ data mapping

[Damien Le Moal]

The call to rockchip_pcie_prog_ep_ob_atu() used to map the PCI address
of MSI data to the memory window allocated on probe for IRQs is done
in rockchip_pcie_ep_send_msi_irq() assuming a fixed alignment to a
256B boundary of the PCI address.  This is not correct as the alignment
constraint for the RK3399 PCI mapping depends on the number of bits of
address changing in the mapped region. This leads to an unstable system
which sometimes work and sometimes does not (crashing on paging faults
when memcpy_toio() or memcpy_fromio() are used).

Similar to regular data mapping, the MSI data mapping must thus be
handled according to the information provided by
rockchip_pcie_ep_align_addr(). Modify rockchip_pcie_ep_send_msi_irq()
to use rockchip_pcie_ep_align_addr() to correctly program entry 0 of
the ATU for sending MSI IRQs.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index f6959f9b94b7..dcd1b5415602 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -379,9 +379,10 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
 {
 	struct rockchip_pcie *rockchip = &ep->rockchip;
 	u32 flags, mme, data, data_mask;
+	size_t irq_pci_size, offset;
+	u64 irq_pci_addr;
 	u8 msi_count;
 	u64 pci_addr;
-	u32 r;
 
 	/* Check MSI enable bit */
 	flags = rockchip_pcie_read(&ep->rockchip,
@@ -417,18 +418,21 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
 				       PCI_MSI_ADDRESS_LO);
 
 	/* Set the outbound region if needed. */
-	if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
+	irq_pci_size = ~PCIE_ADDR_MASK + 1;
+	irq_pci_addr = rockchip_pcie_ep_align_addr(ep->epc,
+						   pci_addr & PCIE_ADDR_MASK,
+						   &irq_pci_size, &offset);
+	if (unlikely(ep->irq_pci_addr != irq_pci_addr ||
 		     ep->irq_pci_fn != fn)) {
-		r = rockchip_ob_region(ep->irq_phys_addr);
-		rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
-					     ep->irq_phys_addr,
-					     pci_addr & PCIE_ADDR_MASK,
-					     ~PCIE_ADDR_MASK + 1);
-		ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
+		rockchip_pcie_prog_ep_ob_atu(rockchip, fn,
+					rockchip_ob_region(ep->irq_phys_addr),
+					ep->irq_phys_addr,
+					irq_pci_addr, irq_pci_size);
+		ep->irq_pci_addr = irq_pci_addr;
 		ep->irq_pci_fn = fn;
 	}
 
-	writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
+	writew(data, ep->irq_cpu_addr + offset + (pci_addr & ~PCIE_ADDR_MASK));
 	return 0;
 }
 
-- 
2.47.0

[PATCH v5 07/14] PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()

[Damien Le Moal]

To be consistent with the usual "get_resources" naming of driver
functions that acquire controller resources like clocks, PHY etc, rename
the function rockchip_pcie_parse_ep_dt() to
rockchip_pcie_ep_get_resources().

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index dcd1b5415602..705f6741fa53 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -494,8 +494,8 @@ static const struct pci_epc_ops rockchip_pcie_epc_ops = {
 	.get_features	= rockchip_pcie_ep_get_features,
 };
 
-static int rockchip_pcie_parse_ep_dt(struct rockchip_pcie *rockchip,
-				     struct rockchip_pcie_ep *ep)
+static int rockchip_pcie_ep_get_resources(struct rockchip_pcie *rockchip,
+					  struct rockchip_pcie_ep *ep)
 {
 	struct device *dev = rockchip->dev;
 	int err;
@@ -557,7 +557,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	ep->epc = epc;
 	epc_set_drvdata(epc, ep);
 
-	err = rockchip_pcie_parse_ep_dt(rockchip, ep);
+	err = rockchip_pcie_ep_get_resources(rockchip, ep);
 	if (err)
 		return err;
 
-- 
2.47.0

[PATCH v5 08/14] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations

[Damien Le Moal]

Introduce the function rockchip_pcie_ep_init_ob_mem()
allocate the outbound memory regions and memory needed for IRQ handling.

These changes tidy up rockchip_pcie_ep_probe(). No functional change.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 107 ++++++++++++----------
 1 file changed, 61 insertions(+), 46 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 705f6741fa53..8dd2a812e446 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -529,15 +529,66 @@ static const struct of_device_id rockchip_pcie_ep_of_match[] = {
 	{},
 };
 
+static int rockchip_pcie_ep_init_ob_mem(struct rockchip_pcie_ep *ep)
+{
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+	struct device *dev = rockchip->dev;
+	struct pci_epc_mem_window *windows = NULL;
+	int err, i;
+
+	ep->ob_addr = devm_kcalloc(dev, ep->max_regions, sizeof(*ep->ob_addr),
+				   GFP_KERNEL);
+
+	if (!ep->ob_addr)
+		return -ENOMEM;
+
+	windows = devm_kcalloc(dev, ep->max_regions,
+			       sizeof(struct pci_epc_mem_window), GFP_KERNEL);
+	if (!windows)
+		return -ENOMEM;
+
+	for (i = 0; i < ep->max_regions; i++) {
+		windows[i].phys_base = rockchip->mem_res->start + (SZ_1M * i);
+		windows[i].size = SZ_1M;
+		windows[i].page_size = SZ_1M;
+	}
+	err = pci_epc_multi_mem_init(ep->epc, windows, ep->max_regions);
+	devm_kfree(dev, windows);
+
+	if (err < 0) {
+		dev_err(dev, "failed to initialize the memory space\n");
+		return err;
+	}
+
+	ep->irq_cpu_addr = pci_epc_mem_alloc_addr(ep->epc, &ep->irq_phys_addr,
+						  SZ_1M);
+	if (!ep->irq_cpu_addr) {
+		dev_err(dev, "failed to reserve memory space for MSI\n");
+		goto err_epc_mem_exit;
+	}
+
+	ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;
+
+	return 0;
+
+err_epc_mem_exit:
+	pci_epc_mem_exit(ep->epc);
+
+	return err;
+}
+
+static void rockchip_pcie_ep_exit_ob_mem(struct rockchip_pcie_ep *ep)
+{
+	pci_epc_mem_exit(ep->epc);
+}
+
 static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rockchip_pcie_ep *ep;
 	struct rockchip_pcie *rockchip;
 	struct pci_epc *epc;
-	size_t max_regions;
-	struct pci_epc_mem_window *windows = NULL;
-	int err, i;
+	int err;
 	u32 cfg_msi, cfg_msix_cp;
 
 	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
@@ -561,10 +612,14 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	err = rockchip_pcie_enable_clocks(rockchip);
+	err = rockchip_pcie_ep_init_ob_mem(ep);
 	if (err)
 		return err;
 
+	err = rockchip_pcie_enable_clocks(rockchip);
+	if (err)
+		goto err_exit_ob_mem;
+
 	err = rockchip_pcie_init_port(rockchip);
 	if (err)
 		goto err_disable_clocks;
@@ -573,47 +628,9 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	max_regions = ep->max_regions;
-	ep->ob_addr = devm_kcalloc(dev, max_regions, sizeof(*ep->ob_addr),
-				   GFP_KERNEL);
-
-	if (!ep->ob_addr) {
-		err = -ENOMEM;
-		goto err_uninit_port;
-	}
-
 	/* Only enable function 0 by default */
 	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
 
-	windows = devm_kcalloc(dev, ep->max_regions,
-			       sizeof(struct pci_epc_mem_window), GFP_KERNEL);
-	if (!windows) {
-		err = -ENOMEM;
-		goto err_uninit_port;
-	}
-	for (i = 0; i < ep->max_regions; i++) {
-		windows[i].phys_base = rockchip->mem_res->start + (SZ_1M * i);
-		windows[i].size = SZ_1M;
-		windows[i].page_size = SZ_1M;
-	}
-	err = pci_epc_multi_mem_init(epc, windows, ep->max_regions);
-	devm_kfree(dev, windows);
-
-	if (err < 0) {
-		dev_err(dev, "failed to initialize the memory space\n");
-		goto err_uninit_port;
-	}
-
-	ep->irq_cpu_addr = pci_epc_mem_alloc_addr(epc, &ep->irq_phys_addr,
-						  SZ_1M);
-	if (!ep->irq_cpu_addr) {
-		dev_err(dev, "failed to reserve memory space for MSI\n");
-		err = -ENOMEM;
-		goto err_epc_mem_exit;
-	}
-
-	ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;
-
 	/*
 	 * MSI-X is not supported but the controller still advertises the MSI-X
 	 * capability by default, which can lead to the Root Complex side
@@ -643,10 +660,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	pci_epc_init_notify(epc);
 
 	return 0;
-err_epc_mem_exit:
-	pci_epc_mem_exit(epc);
-err_uninit_port:
-	rockchip_pcie_deinit_phys(rockchip);
+err_exit_ob_mem:
+	rockchip_pcie_ep_exit_ob_mem(ep);
 err_disable_clocks:
 	rockchip_pcie_disable_clocks(rockchip);
 	return err;
-- 
2.47.0

[PATCH v5 09/14] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Damien Le Moal]

Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
to its own function, rockchip_pcie_ep_hide_broken_msix_cap().
No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 54 +++++++++++++----------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 8dd2a812e446..d980e0b92745 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -582,6 +582,34 @@ static void rockchip_pcie_ep_exit_ob_mem(struct rockchip_pcie_ep *ep)
 	pci_epc_mem_exit(ep->epc);
 }
 
+static void rockchip_pcie_ep_hide_broken_msix_cap(struct rockchip_pcie *rockchip)
+{
+	u32 cfg_msi, cfg_msix_cp;
+
+	/*
+	 * MSI-X is not supported but the controller still advertises the MSI-X
+	 * capability by default, which can lead to the Root Complex side
+	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
+	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
+	 * the next pointer from the MSI-X entry and set that in the MSI
+	 * capability entry (which is the previous entry). This way the MSI-X
+	 * entry is skipped (left out of the linked-list) and not advertised.
+	 */
+	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
+				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
+
+	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
+
+	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
+					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
+					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
+
+	cfg_msi |= cfg_msix_cp;
+
+	rockchip_pcie_write(rockchip, cfg_msi,
+			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
+}
+
 static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -589,7 +617,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	struct rockchip_pcie *rockchip;
 	struct pci_epc *epc;
 	int err;
-	u32 cfg_msi, cfg_msix_cp;
 
 	ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
 	if (!ep)
@@ -624,6 +651,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	if (err)
 		goto err_disable_clocks;
 
+	rockchip_pcie_ep_hide_broken_msix_cap(rockchip);
+
 	/* Establish the link automatically */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
@@ -631,29 +660,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	/* Only enable function 0 by default */
 	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
 
-	/*
-	 * MSI-X is not supported but the controller still advertises the MSI-X
-	 * capability by default, which can lead to the Root Complex side
-	 * allocating MSI-X vectors which cannot be used. Avoid this by skipping
-	 * the MSI-X capability entry in the PCIe capabilities linked-list: get
-	 * the next pointer from the MSI-X entry and set that in the MSI
-	 * capability entry (which is the previous entry). This way the MSI-X
-	 * entry is skipped (left out of the linked-list) and not advertised.
-	 */
-	cfg_msi = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
-				     ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
-
-	cfg_msi &= ~ROCKCHIP_PCIE_EP_MSI_CP1_MASK;
-
-	cfg_msix_cp = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
-					 ROCKCHIP_PCIE_EP_MSIX_CAP_REG) &
-					 ROCKCHIP_PCIE_EP_MSIX_CAP_CP_MASK;
-
-	cfg_msi |= cfg_msix_cp;
-
-	rockchip_pcie_write(rockchip, cfg_msi,
-			    PCIE_EP_CONFIG_BASE + ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
-
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-- 
2.47.0

[PATCH v5 10/14] PCI: rockchip-ep: Refactor endpoint link training enable

[Damien Le Moal]

The function rockchip_pcie_init_port() enables link training for a
controller configured in EP mode. Enabling link training is again done
in rockchip_pcie_ep_probe() after that function executed
rockchip_pcie_init_port(). Enabling link training only needs to be done
once, and doing so at the probe stage before the controller is actually
started by the user serves no purpose.

Refactor this by removing the link training enablement from both
rockchip_pcie_init_port() and rockchip_pcie_ep_probe() and moving it to
the endpoint start operation defined with rockchip_pcie_ep_start().
Enabling the controller configuration using the PCIE_CLIENT_CONF_ENABLE
bit in the same PCIE_CLIENT_CONFIG register is also moved to
rockchip_pcie_ep_start() and both the controller configuration and link
training enable bits are set with a single call to
rockchip_pcie_write().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 13 ++++++-------
 drivers/pci/controller/pcie-rockchip.c    |  5 +++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index d980e0b92745..256a90485fe4 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -464,6 +464,12 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
 
 	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
 
+	/* Enable configuration and start link training */
+	rockchip_pcie_write(rockchip,
+			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
+			    PCIE_CLIENT_CONF_ENABLE,
+			    PCIE_CLIENT_CONFIG);
+
 	return 0;
 }
 
@@ -653,16 +659,9 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 
 	rockchip_pcie_ep_hide_broken_msix_cap(rockchip);
 
-	/* Establish the link automatically */
-	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
-			    PCIE_CLIENT_CONFIG);
-
 	/* Only enable function 0 by default */
 	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
 
-	rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE,
-			    PCIE_CLIENT_CONFIG);
-
 	pci_epc_init_notify(epc);
 
 	return 0;
diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index c07d7129f1c7..154e78819e6e 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -244,11 +244,12 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
 				    PCIE_CLIENT_CONFIG);
 
-	regs = PCIE_CLIENT_LINK_TRAIN_ENABLE | PCIE_CLIENT_ARI_ENABLE |
+	regs = PCIE_CLIENT_ARI_ENABLE |
 	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);
 
 	if (rockchip->is_rc)
-		regs |= PCIE_CLIENT_CONF_ENABLE | PCIE_CLIENT_MODE_RC;
+		regs |= PCIE_CLIENT_LINK_TRAIN_ENABLE |
+			PCIE_CLIENT_CONF_ENABLE | PCIE_CLIENT_MODE_RC;
 	else
 		regs |= PCIE_CLIENT_CONF_DISABLE | PCIE_CLIENT_MODE_EP;
 
-- 
2.47.0

[PATCH v5 11/14] PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation

[Damien Le Moal]

Define the EPC operation ->stop() for the Rockchip endpoint driver with
the function rockchip_pcie_ep_stop(). This function disables link
training and the controller configuration, as the reverse to what
the start operation defined with rockchip_pcie_ep_start() does.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 13 +++++++++++++
 drivers/pci/controller/pcie-rockchip.h    |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 256a90485fe4..2f7709ba1cac 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -473,6 +473,18 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
 	return 0;
 }
 
+static void rockchip_pcie_ep_stop(struct pci_epc *epc)
+{
+	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+
+	/* Stop link training and disable configuration */
+	rockchip_pcie_write(rockchip,
+			    PCIE_CLIENT_CONF_DISABLE |
+			    PCIE_CLIENT_LINK_TRAIN_DISABLE,
+			    PCIE_CLIENT_CONFIG);
+}
+
 static const struct pci_epc_features rockchip_pcie_epc_features = {
 	.linkup_notifier = false,
 	.msi_capable = true,
@@ -497,6 +509,7 @@ static const struct pci_epc_ops rockchip_pcie_epc_ops = {
 	.get_msi	= rockchip_pcie_ep_get_msi,
 	.raise_irq	= rockchip_pcie_ep_raise_irq,
 	.start		= rockchip_pcie_ep_start,
+	.stop		= rockchip_pcie_ep_stop,
 	.get_features	= rockchip_pcie_ep_get_features,
 };
 
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 30398156095f..0263f158ee8d 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -32,6 +32,7 @@
 #define   PCIE_CLIENT_CONF_ENABLE	  HIWORD_UPDATE_BIT(0x0001)
 #define   PCIE_CLIENT_CONF_DISABLE       HIWORD_UPDATE(0x0001, 0)
 #define   PCIE_CLIENT_LINK_TRAIN_ENABLE	  HIWORD_UPDATE_BIT(0x0002)
+#define   PCIE_CLIENT_LINK_TRAIN_DISABLE  HIWORD_UPDATE(0x0002, 0)
 #define   PCIE_CLIENT_ARI_ENABLE	  HIWORD_UPDATE_BIT(0x0008)
 #define   PCIE_CLIENT_CONF_LANE_NUM(x)	  HIWORD_UPDATE(0x0030, ENCODE_LANES(x))
 #define   PCIE_CLIENT_MODE_RC		  HIWORD_UPDATE_BIT(0x0040)
-- 
2.47.0

[PATCH v5 12/14] PCI: rockchip-ep: Improve link training

[Damien Le Moal]

The Rockchip RK3399 TRM V1.3 Part2, Section 17.5.8.1.2, step 7,
describes the endpoint mode link training process clearly and states
that:
  Insure link training completion and success by observing link_st field
  in PCIe Client BASIC_STATUS1 register change to 2'b11. If both side
  support PCIe Gen2 speed, re-train can be Initiated by asserting the
  Retrain Link field in Link Control and Status Register. The software
  should insure the BASIC_STATUS0[negotiated_speed] changes to "1", that
  indicates re-train to Gen2 successfully.
This procedure is very similar to what is done for the root-port mode
in rockchip_pcie_host_init_port().

Implement this link training procedure for the endpoint mode as well.
Given that the RK3399 SoC does not have an interrupt signaling link
status changes, training is implemented as a delayed work which is
rescheduled until the link training completes or the endpoint controller
is stopped. The link training work is first scheduled in
rockchip_pcie_ep_start() when the endpoint function is started. Link
training completion is signaled to the function using pci_epc_linkup().
Accordingly, the linkup_notifier field of the rockchip pci_epc_features
structure is changed to true.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c | 82 ++++++++++++++++++++++-
 drivers/pci/controller/pcie-rockchip.h    | 11 +++
 2 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 2f7709ba1cac..43480706b8f4 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -10,12 +10,14 @@
 
 #include <linux/configfs.h>
 #include <linux/delay.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/pci-epc.h>
 #include <linux/platform_device.h>
 #include <linux/pci-epf.h>
 #include <linux/sizes.h>
+#include <linux/workqueue.h>
 
 #include "pcie-rockchip.h"
 
@@ -48,6 +50,7 @@ struct rockchip_pcie_ep {
 	u64			irq_pci_addr;
 	u8			irq_pci_fn;
 	u8			irq_pending;
+	struct delayed_work	link_training;
 };
 
 static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
@@ -470,6 +473,8 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
 			    PCIE_CLIENT_CONF_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
+	schedule_delayed_work(&ep->link_training, 0);
+
 	return 0;
 }
 
@@ -478,6 +483,8 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
 	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
 	struct rockchip_pcie *rockchip = &ep->rockchip;
 
+	cancel_delayed_work_sync(&ep->link_training);
+
 	/* Stop link training and disable configuration */
 	rockchip_pcie_write(rockchip,
 			    PCIE_CLIENT_CONF_DISABLE |
@@ -485,8 +492,80 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
 			    PCIE_CLIENT_CONFIG);
 }
 
+static void rockchip_pcie_ep_retrain_link(struct rockchip_pcie *rockchip)
+{
+	u32 status;
+
+	status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_LCS);
+	status |= PCI_EXP_LNKCTL_RL;
+	rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_LCS);
+}
+
+static bool rockchip_pcie_ep_link_up(struct rockchip_pcie *rockchip)
+{
+	u32 val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS1);
+
+	return PCIE_LINK_UP(val);
+}
+
+static void rockchip_pcie_ep_link_training(struct work_struct *work)
+{
+	struct rockchip_pcie_ep *ep =
+		container_of(work, struct rockchip_pcie_ep, link_training.work);
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+	struct device *dev = rockchip->dev;
+	u32 val;
+	int ret;
+
+	/* Enable Gen1 training and wait for its completion */
+	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
+				 val, PCIE_LINK_TRAINING_DONE(val), 50,
+				 LINK_TRAIN_TIMEOUT);
+	if (ret)
+		goto again;
+
+	/* Make sure that the link is up */
+	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
+				 val, PCIE_LINK_UP(val), 50,
+				 LINK_TRAIN_TIMEOUT);
+	if (ret)
+		goto again;
+
+	/*
+	 * Check the current speed: if gen2 speed was requested and we are not
+	 * at gen2 speed yet, retrain again for gen2.
+	 */
+	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
+	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
+		/* Enable retrain for gen2 */
+		rockchip_pcie_ep_retrain_link(rockchip);
+		readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
+				   val, PCIE_LINK_IS_GEN2(val), 50,
+				   LINK_TRAIN_TIMEOUT);
+	}
+
+	/* Check again that the link is up */
+	if (!rockchip_pcie_ep_link_up(rockchip))
+		goto again;
+
+	val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0);
+	dev_info(dev,
+		 "Link UP (Negotiated speed: %sGT/s, width: x%lu)\n",
+		 (val & PCIE_CLIENT_NEG_LINK_SPEED) ? "5" : "2.5",
+		 ((val & PCIE_CLIENT_NEG_LINK_WIDTH_MASK) >>
+		  PCIE_CLIENT_NEG_LINK_WIDTH_SHIFT) << 1);
+
+	/* Notify the function */
+	pci_epc_linkup(ep->epc);
+
+	return;
+
+again:
+	schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5));
+}
+
 static const struct pci_epc_features rockchip_pcie_epc_features = {
-	.linkup_notifier = false,
+	.linkup_notifier = true,
 	.msi_capable = true,
 	.msix_capable = false,
 	.align = ROCKCHIP_PCIE_AT_SIZE_ALIGN,
@@ -644,6 +723,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	rockchip = &ep->rockchip;
 	rockchip->is_rc = false;
 	rockchip->dev = dev;
+	INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training);
 
 	epc = devm_pci_epc_create(dev, &rockchip_pcie_epc_ops);
 	if (IS_ERR(epc)) {
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 0263f158ee8d..24796176f658 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -26,6 +26,7 @@
 #define MAX_LANE_NUM			4
 #define MAX_REGION_LIMIT		32
 #define MIN_EP_APERTURE			28
+#define LINK_TRAIN_TIMEOUT		(500 * USEC_PER_MSEC)
 
 #define PCIE_CLIENT_BASE		0x0
 #define PCIE_CLIENT_CONFIG		(PCIE_CLIENT_BASE + 0x00)
@@ -50,6 +51,10 @@
 #define   PCIE_CLIENT_DEBUG_LTSSM_MASK		GENMASK(5, 0)
 #define   PCIE_CLIENT_DEBUG_LTSSM_L1		0x18
 #define   PCIE_CLIENT_DEBUG_LTSSM_L2		0x19
+#define PCIE_CLIENT_BASIC_STATUS0	(PCIE_CLIENT_BASE + 0x44)
+#define   PCIE_CLIENT_NEG_LINK_WIDTH_MASK	GENMASK(7, 6)
+#define   PCIE_CLIENT_NEG_LINK_WIDTH_SHIFT	6
+#define   PCIE_CLIENT_NEG_LINK_SPEED		BIT(5)
 #define PCIE_CLIENT_BASIC_STATUS1	(PCIE_CLIENT_BASE + 0x48)
 #define   PCIE_CLIENT_LINK_STATUS_UP		0x00300000
 #define   PCIE_CLIENT_LINK_STATUS_MASK		0x00300000
@@ -87,6 +92,8 @@
 
 #define PCIE_CORE_CTRL_MGMT_BASE	0x900000
 #define PCIE_CORE_CTRL			(PCIE_CORE_CTRL_MGMT_BASE + 0x000)
+#define   PCIE_CORE_PL_CONF_LS_MASK		0x00000001
+#define   PCIE_CORE_PL_CONF_LS_READY		0x00000001
 #define   PCIE_CORE_PL_CONF_SPEED_5G		0x00000008
 #define   PCIE_CORE_PL_CONF_SPEED_MASK		0x00000018
 #define   PCIE_CORE_PL_CONF_LANE_MASK		0x00000006
@@ -144,6 +151,7 @@
 #define PCIE_RC_CONFIG_BASE		0xa00000
 #define PCIE_EP_CONFIG_BASE		0xa00000
 #define PCIE_EP_CONFIG_DID_VID		(PCIE_EP_CONFIG_BASE + 0x00)
+#define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
 #define PCIE_RC_CONFIG_RID_CCR		(PCIE_RC_CONFIG_BASE + 0x08)
 #define PCIE_RC_CONFIG_DCR		(PCIE_RC_CONFIG_BASE + 0xc4)
 #define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT		18
@@ -155,6 +163,7 @@
 #define PCIE_RC_CONFIG_LINK_CAP		(PCIE_RC_CONFIG_BASE + 0xcc)
 #define   PCIE_RC_CONFIG_LINK_CAP_L0S		BIT(10)
 #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
+#define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
 #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
 #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
 #define   PCIE_RC_CONFIG_THP_CAP_NEXT_MASK	GENMASK(31, 20)
@@ -192,6 +201,8 @@
 #define ROCKCHIP_VENDOR_ID			0x1d87
 #define PCIE_LINK_IS_L2(x) \
 	(((x) & PCIE_CLIENT_DEBUG_LTSSM_MASK) == PCIE_CLIENT_DEBUG_LTSSM_L2)
+#define PCIE_LINK_TRAINING_DONE(x) \
+	(((x) & PCIE_CORE_PL_CONF_LS_MASK) == PCIE_CORE_PL_CONF_LS_READY)
 #define PCIE_LINK_UP(x) \
 	(((x) & PCIE_CLIENT_LINK_STATUS_MASK) == PCIE_CLIENT_LINK_STATUS_UP)
 #define PCIE_LINK_IS_GEN2(x) \
-- 
2.47.0

[PATCH v5 13/14] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Damien Le Moal]

Currently, the Rockchip PCIe endpoint controller driver does not handle
the PERST# signal, which prevents detecting when link training should
actually be started or if the host resets the device. This however can
be supported using the controller reset_gpios property set as an input
GPIO for endpoint mode.

Modify the rockchip PCI endpoint controller driver to get the reset_gpio
and its associated interrupt which is serviced using a threaded IRQ with
the function rockchip_pcie_ep_perst_irq_thread() as handler.

This handler function notifies a link down event corresponding to the RC
side asserting the PERST# signal using pci_epc_linkdown() when the gpio
is high. Once the gpio value goes down, corresponding to the RC
de-asserting the PERST# signal, link training is started. The polarity
of the gpio interrupt trigger is changed from high to low after the RC
asserted PERST#, and conversely changed from low to high after the RC
de-asserts PERST#.

Also, given that the host mode controller and the endpoint mode
controller use two different property names for the same PERST# signal
(ep_gpios property and reset_gpios property respectively), for clarity,
rename the ep_gpio field of struct rockchip_pcie to perst_gpio.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/pci/controller/pcie-rockchip-ep.c   | 124 +++++++++++++++++++-
 drivers/pci/controller/pcie-rockchip-host.c |   4 +-
 drivers/pci/controller/pcie-rockchip.c      |  16 +--
 drivers/pci/controller/pcie-rockchip.h      |   2 +-
 4 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 43480706b8f4..19cfba5230a2 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -10,6 +10,7 @@
 
 #include <linux/configfs.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
@@ -50,6 +51,9 @@ struct rockchip_pcie_ep {
 	u64			irq_pci_addr;
 	u8			irq_pci_fn;
 	u8			irq_pending;
+	int			perst_irq;
+	bool			perst_asserted;
+	bool			link_up;
 	struct delayed_work	link_training;
 };
 
@@ -467,13 +471,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
 
 	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
 
+	if (rockchip->perst_gpio)
+		enable_irq(ep->perst_irq);
+
 	/* Enable configuration and start link training */
 	rockchip_pcie_write(rockchip,
 			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
 			    PCIE_CLIENT_CONF_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	schedule_delayed_work(&ep->link_training, 0);
+	if (!rockchip->perst_gpio)
+		schedule_delayed_work(&ep->link_training, 0);
 
 	return 0;
 }
@@ -483,6 +491,11 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
 	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
 	struct rockchip_pcie *rockchip = &ep->rockchip;
 
+	if (rockchip->perst_gpio) {
+		ep->perst_asserted = true;
+		disable_irq(ep->perst_irq);
+	}
+
 	cancel_delayed_work_sync(&ep->link_training);
 
 	/* Stop link training and disable configuration */
@@ -548,6 +561,13 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
 	if (!rockchip_pcie_ep_link_up(rockchip))
 		goto again;
 
+	/*
+	 * If PERST# was asserted while polling the link, do not notify
+	 * the function.
+	 */
+	if (ep->perst_asserted)
+		return;
+
 	val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0);
 	dev_info(dev,
 		 "Link UP (Negotiated speed: %sGT/s, width: x%lu)\n",
@@ -557,6 +577,7 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
 
 	/* Notify the function */
 	pci_epc_linkup(ep->epc);
+	ep->link_up = true;
 
 	return;
 
@@ -564,6 +585,97 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)
 	schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5));
 }
 
+static void rockchip_pcie_ep_perst_assert(struct rockchip_pcie_ep *ep)
+{
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+
+	dev_dbg(rockchip->dev, "PERST# asserted, link down\n");
+
+	if (ep->perst_asserted)
+		return;
+
+	ep->perst_asserted = true;
+
+	cancel_delayed_work_sync(&ep->link_training);
+
+	if (ep->link_up) {
+		pci_epc_linkdown(ep->epc);
+		ep->link_up = false;
+	}
+}
+
+static void rockchip_pcie_ep_perst_deassert(struct rockchip_pcie_ep *ep)
+{
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+
+	dev_dbg(rockchip->dev, "PERST# de-asserted, starting link training\n");
+
+	if (!ep->perst_asserted)
+		return;
+
+	ep->perst_asserted = false;
+
+	/* Enable link re-training */
+	rockchip_pcie_ep_retrain_link(rockchip);
+
+	/* Start link training */
+	schedule_delayed_work(&ep->link_training, 0);
+}
+
+static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data)
+{
+	struct pci_epc *epc = data;
+	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+	u32 perst = gpiod_get_value(rockchip->perst_gpio);
+
+	if (perst)
+		rockchip_pcie_ep_perst_assert(ep);
+	else
+		rockchip_pcie_ep_perst_deassert(ep);
+
+	irq_set_irq_type(ep->perst_irq,
+			 (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW));
+
+	return IRQ_HANDLED;
+}
+
+static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc)
+{
+	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
+	struct rockchip_pcie *rockchip = &ep->rockchip;
+	struct device *dev = rockchip->dev;
+	int ret;
+
+	if (!rockchip->perst_gpio)
+		return 0;
+
+	/* PCIe reset interrupt */
+	ep->perst_irq = gpiod_to_irq(rockchip->perst_gpio);
+	if (ep->perst_irq < 0) {
+		dev_err(dev, "No IRQ for PERST# GPIO\n");
+		return ep->perst_irq;
+	}
+
+	/*
+	 * The perst_gpio is active low, so when it is inactive on start, it
+	 * is high and will trigger the perst_irq handler. So treat this initial
+	 * IRQ as a dummy one by faking the host asserting PERST#.
+	 */
+	ep->perst_asserted = true;
+	irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
+	ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL,
+					rockchip_pcie_ep_perst_irq_thread,
+					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+					"pcie-ep-perst", epc);
+	if (ret) {
+		dev_err(dev, "Request PERST# GPIO IRQ failed %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static const struct pci_epc_features rockchip_pcie_epc_features = {
 	.linkup_notifier = true,
 	.msi_capable = true,
@@ -757,11 +869,17 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 
 	pci_epc_init_notify(epc);
 
+	err = rockchip_pcie_ep_setup_irq(epc);
+	if (err < 0)
+		goto err_uninit_port;
+
 	return 0;
-err_exit_ob_mem:
-	rockchip_pcie_ep_exit_ob_mem(ep);
+err_uninit_port:
+	rockchip_pcie_deinit_phys(rockchip);
 err_disable_clocks:
 	rockchip_pcie_disable_clocks(rockchip);
+err_exit_ob_mem:
+	rockchip_pcie_ep_exit_ob_mem(ep);
 	return err;
 }
 
diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index cbec71114825..7471d9fd18bc 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -294,7 +294,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	int err, i = MAX_LANE_NUM;
 	u32 status;
 
-	gpiod_set_value_cansleep(rockchip->ep_gpio, 0);
+	gpiod_set_value_cansleep(rockchip->perst_gpio, 0);
 
 	err = rockchip_pcie_init_port(rockchip);
 	if (err)
@@ -323,7 +323,7 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 			    PCIE_CLIENT_CONFIG);
 
 	msleep(PCIE_T_PVPERL_MS);
-	gpiod_set_value_cansleep(rockchip->ep_gpio, 1);
+	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_T_RRS_READY_MS);
 
diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 154e78819e6e..b9ade7632e11 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -119,13 +119,15 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 		return PTR_ERR(rockchip->aclk_rst);
 	}
 
-	if (rockchip->is_rc) {
-		rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep",
-							    GPIOD_OUT_LOW);
-		if (IS_ERR(rockchip->ep_gpio))
-			return dev_err_probe(dev, PTR_ERR(rockchip->ep_gpio),
-					     "failed to get ep GPIO\n");
-	}
+	if (rockchip->is_rc)
+		rockchip->perst_gpio = devm_gpiod_get_optional(dev, "ep",
+							       GPIOD_OUT_LOW);
+	else
+		rockchip->perst_gpio = devm_gpiod_get_optional(dev, "reset",
+							       GPIOD_IN);
+	if (IS_ERR(rockchip->perst_gpio))
+		return dev_err_probe(dev, PTR_ERR(rockchip->perst_gpio),
+				     "failed to get PERST# GPIO\n");
 
 	rockchip->aclk_pcie = devm_clk_get(dev, "aclk");
 	if (IS_ERR(rockchip->aclk_pcie)) {
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 24796176f658..a51b087ce878 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -329,7 +329,7 @@ struct rockchip_pcie {
 	struct	regulator *vpcie3v3; /* 3.3V power supply */
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
-	struct	gpio_desc *ep_gpio;
+	struct	gpio_desc *perst_gpio;
 	u32	lanes;
 	u8      lanes_map;
 	int	link_gen;
-- 
2.47.0

[PATCH v5 14/14] arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode

[Damien Le Moal]

Add a DTS overlay file for the rk3399-rockpro64 DT for enabling PCIe
endpoint mode support.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 arch/arm64/boot/dts/rockchip/Makefile         |  1 +
 .../rockchip/rk3399-rockpro64-pcie-ep.dtso    | 20 +++++++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 09423070c992..184131a58704 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -73,6 +73,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock-pi-4c.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rock960.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64-v2.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-rockpro64-pcie-ep.dtbo
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-sapphire-excavator.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399pro-rock-pi-n10.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
new file mode 100644
index 000000000000..cebfb71bebfc
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * DT-overlay to run the PCIe Dual Mode controller in Endpoint mode
+ * with the #PERST signal handled with gpio2.
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pinctrl/rockchip.h>
+
+/dts-v1/;
+/plugin/;
+
+&pcie0 {
+	status = "disabled";
+};
+
+&pcie0_ep {
+	reset-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_LOW>;
+	status = "okay";
+};
-- 
2.47.0

Re: [PATCH v5 06/14] PCI: rockchip-ep: Fix MSI IRQ data mapping

[Niklas Cassel]

On Thu, Oct 17, 2024 at 10:58:41AM +0900, Damien Le Moal wrote:
> The call to rockchip_pcie_prog_ep_ob_atu() used to map the PCI address
> of MSI data to the memory window allocated on probe for IRQs is done
> in rockchip_pcie_ep_send_msi_irq() assuming a fixed alignment to a
> 256B boundary of the PCI address.  This is not correct as the alignment
> constraint for the RK3399 PCI mapping depends on the number of bits of
> address changing in the mapped region. This leads to an unstable system
> which sometimes work and sometimes does not (crashing on paging faults
> when memcpy_toio() or memcpy_fromio() are used).
> 
> Similar to regular data mapping, the MSI data mapping must thus be
> handled according to the information provided by
> rockchip_pcie_ep_align_addr(). Modify rockchip_pcie_ep_send_msi_irq()
> to use rockchip_pcie_ep_align_addr() to correctly program entry 0 of
> the ATU for sending MSI IRQs.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index f6959f9b94b7..dcd1b5415602 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -379,9 +379,10 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
>  {
>  	struct rockchip_pcie *rockchip = &ep->rockchip;
>  	u32 flags, mme, data, data_mask;
> +	size_t irq_pci_size, offset;
> +	u64 irq_pci_addr;
>  	u8 msi_count;
>  	u64 pci_addr;
> -	u32 r;
>  
>  	/* Check MSI enable bit */
>  	flags = rockchip_pcie_read(&ep->rockchip,
> @@ -417,18 +418,21 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
>  				       PCI_MSI_ADDRESS_LO);
>  
>  	/* Set the outbound region if needed. */
> -	if (unlikely(ep->irq_pci_addr != (pci_addr & PCIE_ADDR_MASK) ||
> +	irq_pci_size = ~PCIE_ADDR_MASK + 1;
> +	irq_pci_addr = rockchip_pcie_ep_align_addr(ep->epc,
> +						   pci_addr & PCIE_ADDR_MASK,
> +						   &irq_pci_size, &offset);
> +	if (unlikely(ep->irq_pci_addr != irq_pci_addr ||
>  		     ep->irq_pci_fn != fn)) {
> -		r = rockchip_ob_region(ep->irq_phys_addr);
> -		rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> -					     ep->irq_phys_addr,
> -					     pci_addr & PCIE_ADDR_MASK,
> -					     ~PCIE_ADDR_MASK + 1);
> -		ep->irq_pci_addr = (pci_addr & PCIE_ADDR_MASK);
> +		rockchip_pcie_prog_ep_ob_atu(rockchip, fn,
> +					rockchip_ob_region(ep->irq_phys_addr),
> +					ep->irq_phys_addr,
> +					irq_pci_addr, irq_pci_size);
> +		ep->irq_pci_addr = irq_pci_addr;
>  		ep->irq_pci_fn = fn;
>  	}
>  
> -	writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> +	writew(data, ep->irq_cpu_addr + offset + (pci_addr & ~PCIE_ADDR_MASK));
>  	return 0;
>  }
>  
> -- 
> 2.47.0
> 

Nice catch.

For DWC, in dw_pcie_ep_raise_msi_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L519-L522

and in dw_pcie_ep_raise_msix_irq()
https://github.com/torvalds/linux/blob/v6.12-rc3/drivers/pci/controller/dwc/pcie-designware-ep.c#L603-L606

We also make sure that the address that we map is aligned,
and then write to the correct offset within that mapping:
ep->msi_mem + aligned_offset;
in order to write to the actual MSI address.

To me, it looks like doing a similar change as this patch does,
to dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq(),
would make the PCI endpoint code more consistent overall.

Thoughts?


Kind regards,
Niklas

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Damien Le Moal]

On 10/17/24 10:58, Damien Le Moal wrote:
> This patch series fix the PCI address mapping handling of the Rockchip
> PCI endpoint driver, refactor some of its code, improves link training
> and adds handling of the PERST# signal.
> 
> This series is organized as follows:
>  - Patch 1 fixes the rockchip ATU programming
>  - Patch 2, 3 and 4 introduce small code improvments
>  - Patch 5 implements the .align_addr() operation to make the RK3399
>    endpoint controller driver fully functional with the new
>    pci_epc_mem_map() function
>  - Patch 6 uses the new align_addr operation function to fix the ATU
>    programming for MSI IRQ data mapping
>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
>    readable
>  - Patch 11 introduces the .stop() endpoint controller operation to
>    correctly disable the endpopint controller after use
>  - Patch 12 improves link training
>  - Patch 13 implements handling of the #PERST signal
>  - Patch 14 adds a DT overlay file to enable EP mode and define the
>    PERST# GPIO (reset-gpios) property.
> 
> These patches were tested using a Pine Rockpro64 board used as an
> endpoint with the test endpoint function driver and a prototype nvme
> endpoint function driver.

Ping ? If there are no issues, can we get this queued up ?

> 
> Changes from v4:
>  - Added patch 6
>  - Added comments to patch 12 and 13 to clarify link training handling
>    and PERST# GPIO use.
>  - Added patch 14
> 
> Changes from v3:
>  - Addressed Mani's comments (see mailing list for details).
>  - Removed old patch 11 (dt-binding changes) and instead use in patch 12
>    the already defined reset_gpios property.
>  - Added patch 6
>  - Added review tags
> 
> Changes from v2:
>  - Split the patch series
>  - Corrected patch 11 to add the missing "maxItem"
> 
> Changes from v1:
>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>    1.
>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>    (former patch 2 of v1)
>  - Various typos cleanups all over. Also fixed some blank space
>    indentation.
>  - Added review tags
> 
> Damien Le Moal (14):
>   PCI: rockchip-ep: Fix address translation unit programming
>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
>   PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
>   PCI: rockchip-ep: Fix MSI IRQ data mapping
>   PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
>   PCI: rockchip-ep: Refactor endpoint link training enable
>   PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
>   arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode
> 
>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../rockchip/rk3399-rockpro64-pcie-ep.dtso    |  20 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 432 ++++++++++++++----
>  drivers/pci/controller/pcie-rockchip-host.c   |   4 +-
>  drivers/pci/controller/pcie-rockchip.c        |  21 +-
>  drivers/pci/controller/pcie-rockchip.h        |  24 +-
>  6 files changed, 406 insertions(+), 96 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Damien Le Moal]

On 10/29/24 19:35, Damien Le Moal wrote:
> On 10/17/24 10:58, Damien Le Moal wrote:
>> This patch series fix the PCI address mapping handling of the Rockchip
>> PCI endpoint driver, refactor some of its code, improves link training
>> and adds handling of the PERST# signal.
>>
>> This series is organized as follows:
>>  - Patch 1 fixes the rockchip ATU programming
>>  - Patch 2, 3 and 4 introduce small code improvments
>>  - Patch 5 implements the .align_addr() operation to make the RK3399
>>    endpoint controller driver fully functional with the new
>>    pci_epc_mem_map() function
>>  - Patch 6 uses the new align_addr operation function to fix the ATU
>>    programming for MSI IRQ data mapping
>>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
>>    readable
>>  - Patch 11 introduces the .stop() endpoint controller operation to
>>    correctly disable the endpopint controller after use
>>  - Patch 12 improves link training
>>  - Patch 13 implements handling of the #PERST signal
>>  - Patch 14 adds a DT overlay file to enable EP mode and define the
>>    PERST# GPIO (reset-gpios) property.
>>
>> These patches were tested using a Pine Rockpro64 board used as an
>> endpoint with the test endpoint function driver and a prototype nvme
>> endpoint function driver.
> 
> Ping ? If there are no issues, can we get this queued up ?

Mani,

Ping AGAIN !!!!

I do not see anything queued in pci/next. What is the blocker ?
These patches have been sitting on the list for nearly a month now, PLEASE DO
SOMETHING. Comment or apply, but please reply something.

> 
>>
>> Changes from v4:
>>  - Added patch 6
>>  - Added comments to patch 12 and 13 to clarify link training handling
>>    and PERST# GPIO use.
>>  - Added patch 14
>>
>> Changes from v3:
>>  - Addressed Mani's comments (see mailing list for details).
>>  - Removed old patch 11 (dt-binding changes) and instead use in patch 12
>>    the already defined reset_gpios property.
>>  - Added patch 6
>>  - Added review tags
>>
>> Changes from v2:
>>  - Split the patch series
>>  - Corrected patch 11 to add the missing "maxItem"
>>
>> Changes from v1:
>>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>>    1.
>>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>>    (former patch 2 of v1)
>>  - Various typos cleanups all over. Also fixed some blank space
>>    indentation.
>>  - Added review tags
>>
>> Damien Le Moal (14):
>>   PCI: rockchip-ep: Fix address translation unit programming
>>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
>>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
>>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
>>   PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
>>   PCI: rockchip-ep: Fix MSI IRQ data mapping
>>   PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
>>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
>>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
>>   PCI: rockchip-ep: Refactor endpoint link training enable
>>   PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
>>   PCI: rockchip-ep: Improve link training
>>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
>>   arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode
>>
>>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>>  .../rockchip/rk3399-rockpro64-pcie-ep.dtso    |  20 +
>>  drivers/pci/controller/pcie-rockchip-ep.c     | 432 ++++++++++++++----
>>  drivers/pci/controller/pcie-rockchip-host.c   |   4 +-
>>  drivers/pci/controller/pcie-rockchip.c        |  21 +-
>>  drivers/pci/controller/pcie-rockchip.h        |  24 +-
>>  6 files changed, 406 insertions(+), 96 deletions(-)
>>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
>>
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Manivannan Sadhasivam]

On Wed, Nov 13, 2024 at 11:29:48PM +0900, Damien Le Moal wrote:
> On 10/29/24 19:35, Damien Le Moal wrote:
> > On 10/17/24 10:58, Damien Le Moal wrote:
> >> This patch series fix the PCI address mapping handling of the Rockchip
> >> PCI endpoint driver, refactor some of its code, improves link training
> >> and adds handling of the PERST# signal.
> >>
> >> This series is organized as follows:
> >>  - Patch 1 fixes the rockchip ATU programming
> >>  - Patch 2, 3 and 4 introduce small code improvments
> >>  - Patch 5 implements the .align_addr() operation to make the RK3399
> >>    endpoint controller driver fully functional with the new
> >>    pci_epc_mem_map() function
> >>  - Patch 6 uses the new align_addr operation function to fix the ATU
> >>    programming for MSI IRQ data mapping
> >>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
> >>    readable
> >>  - Patch 11 introduces the .stop() endpoint controller operation to
> >>    correctly disable the endpopint controller after use
> >>  - Patch 12 improves link training
> >>  - Patch 13 implements handling of the #PERST signal
> >>  - Patch 14 adds a DT overlay file to enable EP mode and define the
> >>    PERST# GPIO (reset-gpios) property.
> >>
> >> These patches were tested using a Pine Rockpro64 board used as an
> >> endpoint with the test endpoint function driver and a prototype nvme
> >> endpoint function driver.
> > 
> > Ping ? If there are no issues, can we get this queued up ?
> 
> Mani,
> 
> Ping AGAIN !!!!
> 
> I do not see anything queued in pci/next. What is the blocker ?
> These patches have been sitting on the list for nearly a month now, PLEASE DO
> SOMETHING. Comment or apply, but please reply something.
> 

Damien,

Sorry for the late reply. Things got a bit hectic due to company onsite meeting.
I'm going through my queue now.

But FYI, I don't merge patches outside drivers/pci/endpoint/

- Mani

> > 
> >>
> >> Changes from v4:
> >>  - Added patch 6
> >>  - Added comments to patch 12 and 13 to clarify link training handling
> >>    and PERST# GPIO use.
> >>  - Added patch 14
> >>
> >> Changes from v3:
> >>  - Addressed Mani's comments (see mailing list for details).
> >>  - Removed old patch 11 (dt-binding changes) and instead use in patch 12
> >>    the already defined reset_gpios property.
> >>  - Added patch 6
> >>  - Added review tags
> >>
> >> Changes from v2:
> >>  - Split the patch series
> >>  - Corrected patch 11 to add the missing "maxItem"
> >>
> >> Changes from v1:
> >>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
> >>    1.
> >>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
> >>    (former patch 2 of v1)
> >>  - Various typos cleanups all over. Also fixed some blank space
> >>    indentation.
> >>  - Added review tags
> >>
> >> Damien Le Moal (14):
> >>   PCI: rockchip-ep: Fix address translation unit programming
> >>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
> >>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
> >>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
> >>   PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
> >>   PCI: rockchip-ep: Fix MSI IRQ data mapping
> >>   PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
> >>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
> >>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
> >>   PCI: rockchip-ep: Refactor endpoint link training enable
> >>   PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
> >>   PCI: rockchip-ep: Improve link training
> >>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
> >>   arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode
> >>
> >>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
> >>  .../rockchip/rk3399-rockpro64-pcie-ep.dtso    |  20 +
> >>  drivers/pci/controller/pcie-rockchip-ep.c     | 432 ++++++++++++++----
> >>  drivers/pci/controller/pcie-rockchip-host.c   |   4 +-
> >>  drivers/pci/controller/pcie-rockchip.c        |  21 +-
> >>  drivers/pci/controller/pcie-rockchip.h        |  24 +-
> >>  6 files changed, 406 insertions(+), 96 deletions(-)
> >>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
> >>
> > 
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Krzysztof Wilczyński]

Hello,

> This patch series fix the PCI address mapping handling of the Rockchip
> PCI endpoint driver, refactor some of its code, improves link training
> and adds handling of the PERST# signal.
> 
> This series is organized as follows:
>  - Patch 1 fixes the rockchip ATU programming
>  - Patch 2, 3 and 4 introduce small code improvments
>  - Patch 5 implements the .align_addr() operation to make the RK3399
>    endpoint controller driver fully functional with the new
>    pci_epc_mem_map() function
>  - Patch 6 uses the new align_addr operation function to fix the ATU
>    programming for MSI IRQ data mapping
>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
>    readable
>  - Patch 11 introduces the .stop() endpoint controller operation to
>    correctly disable the endpopint controller after use
>  - Patch 12 improves link training
>  - Patch 13 implements handling of the #PERST signal
>  - Patch 14 adds a DT overlay file to enable EP mode and define the
>    PERST# GPIO (reset-gpios) property.
> 
> These patches were tested using a Pine Rockpro64 board used as an
> endpoint with the test endpoint function driver and a prototype nvme
> endpoint function driver.

Applied to controller/rockchip, thank you!

[01/13] PCI: rockchip-ep: Fix address translation unit programming
        https://git.kernel.org/pci/pci/c/289cd5c0db35

[02/13] PCI: rockchip-ep: Use a macro to define EP controller .align feature
        https://git.kernel.org/pci/pci/c/8ba3b41eb7ec

[03/13] PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
        https://git.kernel.org/pci/pci/c/db68baa5d884

[04/13] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
        https://git.kernel.org/pci/pci/c/c5b097d2a295

[05/13] PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
        https://git.kernel.org/pci/pci/c/75b011d9006e

[06/13] PCI: rockchip-ep: Fix MSI IRQ data mapping
        https://git.kernel.org/pci/pci/c/42c55124c3b2

[07/13] PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
        https://git.kernel.org/pci/pci/c/c8b915ec5338

[08/13] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
        https://git.kernel.org/pci/pci/c/c0473caa87f1

[09/13] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
        https://git.kernel.org/pci/pci/c/48e848c8727c

[10/13] PCI: rockchip-ep: Refactor endpoint link training enable
        https://git.kernel.org/pci/pci/c/c6de5dd3fd0c

[11/13] PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
        https://git.kernel.org/pci/pci/c/8a9424d83e20

	Krzysztof

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Krzysztof Wilczyński]

Hello,

> > >> These patches were tested using a Pine Rockpro64 board used as an
> > >> endpoint with the test endpoint function driver and a prototype nvme
> > >> endpoint function driver.
> > > 
> > > Ping ? If there are no issues, can we get this queued up ?
> > 
> > Mani,
> > 
> > Ping AGAIN !!!!
> > 
> > I do not see anything queued in pci/next. What is the blocker ?
> > These patches have been sitting on the list for nearly a month now, PLEASE DO
> > SOMETHING. Comment or apply, but please reply something.
> > 
> 
> Damien,
> 
> Sorry for the late reply. Things got a bit hectic due to company onsite meeting.
> I'm going through my queue now.

Thank you, Mani!  I took this over and pulled this series.

You and Bjorn can have a look over the changes, if you have a moment.  That
said, at least to me, the changes looked good.

	Krzysztof

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Damien Le Moal]

On 11/14/24 05:59, Krzysztof Wilczyński wrote:
> Hello,
> 
>>>>> These patches were tested using a Pine Rockpro64 board used as an
>>>>> endpoint with the test endpoint function driver and a prototype nvme
>>>>> endpoint function driver.
>>>>
>>>> Ping ? If there are no issues, can we get this queued up ?
>>>
>>> Mani,
>>>
>>> Ping AGAIN !!!!
>>>
>>> I do not see anything queued in pci/next. What is the blocker ?
>>> These patches have been sitting on the list for nearly a month now, PLEASE DO
>>> SOMETHING. Comment or apply, but please reply something.
>>>
>>
>> Damien,
>>
>> Sorry for the late reply. Things got a bit hectic due to company onsite meeting.
>> I'm going through my queue now.
> 
> Thank you, Mani!  I took this over and pulled this series.
> 
> You and Bjorn can have a look over the changes, if you have a moment.  That
> said, at least to me, the changes looked good.

Krzysztof,

Thanks. But the kernel test robot already complained about a build failure for
the rockchip branch. The series needs to go through the endpoint branch as the
.align_addr method is only defined in that branch at the moment.

> 
> 	Krzysztof


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Krzysztof Wilczyński]

Hello,

> >> Sorry for the late reply. Things got a bit hectic due to company onsite meeting.
> >> I'm going through my queue now.
> > 
> > Thank you, Mani!  I took this over and pulled this series.
> > 
> > You and Bjorn can have a look over the changes, if you have a moment.  That
> > said, at least to me, the changes looked good.
> 
> Thanks. But the kernel test robot already complained about a build failure for
> the rockchip branch.

We did see this in our local testing environment, too.  Albeit, I didn't
get around to moving the commits before bot picked the branch up.

> The series needs to go through the endpoint branch as the .align_addr
> method is only defined in that branch at the moment.

The changes are now together.  Thank you!

	Krzysztof

Re: [PATCH v5 01/14] PCI: rockchip-ep: Fix address translation unit programming

[Bjorn Helgaas]

On Thu, Oct 17, 2024 at 10:58:36AM +0900, Damien Le Moal wrote:
> The rockchip PCIe endpoint controller handles PCIe transfers addresses
> by masking the lower bits of the programmed PCI address and using the
> same number of lower bits masked from the CPU address space used for the
> mapping. For a PCI mapping of <size> bytes starting from <pci_addr>,
> the number of bits masked is the number of address bits changing in the
> address range [pci_addr..pci_addr + size - 1].
> 
> However, rockchip_pcie_prog_ep_ob_atu() calculates num_pass_bits only
> using the size of the mapping, resulting in an incorrect number of mask
> bits depending on the value of the PCI address to map.
> 
> Fix this by introducing the helper function
> rockchip_pcie_ep_ob_atu_num_bits() to correctly calculate the number of
> mask bits to use to program the address translation unit. The number of
> mask bits is calculated depending on both the PCI address and size of
> the mapping, and clamped between 8 and 20 using the macros
> ROCKCHIP_PCIE_AT_MIN_NUM_BITS and ROCKCHIP_PCIE_AT_MAX_NUM_BITS. As
> defined in the Rockchip RK3399 TRM V1.3 Part2, Sections 17.5.5.1.1 and
> 17.6.8.2.1, this clamping is necessary because:
> 1) The lower 8 bits of the PCI address to be mapped by the outbound
>    region are ignored. So a minimum of 8 address bits are needed and
>    imply that the PCI address must be aligned to 256.
> 2) The outbound memory regions are 1MB in size. So while we can specify
>    up to 63-bits for the PCI address (num_bits filed uses bits 0 to 5 of
>    the outbound address region 0 register), we must limit the number of
>    valid address bits to 20 to match the memory window maximum size (1
>    << 20 = 1MB).
> 
> Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 15 +++++++++++----
>  drivers/pci/controller/pcie-rockchip.h    |  4 ++++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 136274533656..27a7febb74e0 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -63,16 +63,23 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
>  			    ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
>  }
>  
> +static int rockchip_pcie_ep_ob_atu_num_bits(struct rockchip_pcie *rockchip,
> +					    u64 pci_addr, size_t size)
> +{
> +	int num_pass_bits = fls64(pci_addr ^ (pci_addr + size - 1));
> +
> +	return clamp(num_pass_bits, ROCKCHIP_PCIE_AT_MIN_NUM_BITS,
> +		     ROCKCHIP_PCIE_AT_MAX_NUM_BITS);
> +}
> +
>  static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
>  					 u32 r, u64 cpu_addr, u64 pci_addr,
>  					 size_t size)
>  {
> -	int num_pass_bits = fls64(size - 1);
> +	int num_pass_bits =
> +		rockchip_pcie_ep_ob_atu_num_bits(rockchip, pci_addr, size);
>  	u32 addr0, addr1, desc0;
>  
> -	if (num_pass_bits < 8)
> -		num_pass_bits = 8;
> -
>  	addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
>  		(lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);

PCIE_CORE_OB_REGION_ADDR0_NUM_BITS is 0x3f and
rockchip_pcie_ep_ob_atu_num_bits() returns something between 8 and
0x14, inclusive?  So masking with PCIE_CORE_OB_REGION_ADDR0_NUM_BITS
doesn't do anything, does it?

Also, "..._NUM_BITS" is kind of a weird name for a mask.

rockchip_pcie_prog_ob_atu() in pcie-rockchip-host.c is similar but
different; it looks like all callers supply num_pass_bits=19.  I
assume it doesn't need a similar change?

>  	addr1 = upper_32_bits(pci_addr);
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index 6111de35f84c..15ee949f2485 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -245,6 +245,10 @@
>  	(PCIE_EP_PF_CONFIG_REGS_BASE + (((fn) << 12) & GENMASK(19, 12)))
>  #define ROCKCHIP_PCIE_EP_VIRT_FUNC_BASE(fn) \
>  	(PCIE_EP_PF_CONFIG_REGS_BASE + 0x10000 + (((fn) << 12) & GENMASK(19, 12)))
> +
> +#define ROCKCHIP_PCIE_AT_MIN_NUM_BITS  8
> +#define ROCKCHIP_PCIE_AT_MAX_NUM_BITS  20
> +
>  #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
>  	(PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
>  #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \
> -- 
> 2.47.0
> 

Re: [PATCH v5 12/14] PCI: rockchip-ep: Improve link training

[Bjorn Helgaas]

On Thu, Oct 17, 2024 at 10:58:47AM +0900, Damien Le Moal wrote:
> The Rockchip RK3399 TRM V1.3 Part2, Section 17.5.8.1.2, step 7,
> describes the endpoint mode link training process clearly and states
> that:
>   Insure link training completion and success by observing link_st field
>   in PCIe Client BASIC_STATUS1 register change to 2'b11. If both side
>   support PCIe Gen2 speed, re-train can be Initiated by asserting the
>   Retrain Link field in Link Control and Status Register. The software
>   should insure the BASIC_STATUS0[negotiated_speed] changes to "1", that
>   indicates re-train to Gen2 successfully.

Since this only adds code and doesn't change existing code, I assume
this hardware doesn't automatically train to gen2 without this new
software assistance?

So the effect of this change is to use gen2 speed when supported by
both partners, when previously we only got gen1?

> This procedure is very similar to what is done for the root-port mode
> in rockchip_pcie_host_init_port().
> 
> Implement this link training procedure for the endpoint mode as well.
> Given that the RK3399 SoC does not have an interrupt signaling link
> status changes, training is implemented as a delayed work which is
> rescheduled until the link training completes or the endpoint controller
> is stopped. The link training work is first scheduled in
> rockchip_pcie_ep_start() when the endpoint function is started. Link
> training completion is signaled to the function using pci_epc_linkup().
> Accordingly, the linkup_notifier field of the rockchip pci_epc_features
> structure is changed to true.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 82 ++++++++++++++++++++++-
>  drivers/pci/controller/pcie-rockchip.h    | 11 +++
>  2 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 2f7709ba1cac..43480706b8f4 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -10,12 +10,14 @@
>  
>  #include <linux/configfs.h>
>  #include <linux/delay.h>
> +#include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
>  #include <linux/pci-epc.h>
>  #include <linux/platform_device.h>
>  #include <linux/pci-epf.h>
>  #include <linux/sizes.h>
> +#include <linux/workqueue.h>
>  
>  #include "pcie-rockchip.h"
>  
> @@ -48,6 +50,7 @@ struct rockchip_pcie_ep {
>  	u64			irq_pci_addr;
>  	u8			irq_pci_fn;
>  	u8			irq_pending;
> +	struct delayed_work	link_training;
>  };
>  
>  static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
> @@ -470,6 +473,8 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
>  			    PCIE_CLIENT_CONF_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
>  
> +	schedule_delayed_work(&ep->link_training, 0);
> +
>  	return 0;
>  }
>  
> @@ -478,6 +483,8 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
>  	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct rockchip_pcie *rockchip = &ep->rockchip;
>  
> +	cancel_delayed_work_sync(&ep->link_training);
> +
>  	/* Stop link training and disable configuration */
>  	rockchip_pcie_write(rockchip,
>  			    PCIE_CLIENT_CONF_DISABLE |
> @@ -485,8 +492,80 @@ static void rockchip_pcie_ep_stop(struct pci_epc *epc)
>  			    PCIE_CLIENT_CONFIG);
>  }
>  
> +static void rockchip_pcie_ep_retrain_link(struct rockchip_pcie *rockchip)
> +{
> +	u32 status;
> +
> +	status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_LCS);
> +	status |= PCI_EXP_LNKCTL_RL;
> +	rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_LCS);
> +}
> +
> +static bool rockchip_pcie_ep_link_up(struct rockchip_pcie *rockchip)
> +{
> +	u32 val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS1);
> +
> +	return PCIE_LINK_UP(val);
> +}
> +
> +static void rockchip_pcie_ep_link_training(struct work_struct *work)
> +{
> +	struct rockchip_pcie_ep *ep =
> +		container_of(work, struct rockchip_pcie_ep, link_training.work);
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	struct device *dev = rockchip->dev;
> +	u32 val;
> +	int ret;
> +
> +	/* Enable Gen1 training and wait for its completion */
> +	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
> +				 val, PCIE_LINK_TRAINING_DONE(val), 50,
> +				 LINK_TRAIN_TIMEOUT);
> +	if (ret)
> +		goto again;
> +
> +	/* Make sure that the link is up */
> +	ret = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> +				 val, PCIE_LINK_UP(val), 50,
> +				 LINK_TRAIN_TIMEOUT);
> +	if (ret)
> +		goto again;
> +
> +	/*
> +	 * Check the current speed: if gen2 speed was requested and we are not
> +	 * at gen2 speed yet, retrain again for gen2.
> +	 */
> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
> +		/* Enable retrain for gen2 */
> +		rockchip_pcie_ep_retrain_link(rockchip);
> +		readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
> +				   val, PCIE_LINK_IS_GEN2(val), 50,
> +				   LINK_TRAIN_TIMEOUT);
> +	}
> +
> +	/* Check again that the link is up */
> +	if (!rockchip_pcie_ep_link_up(rockchip))
> +		goto again;
> +
> +	val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0);
> +	dev_info(dev,
> +		 "Link UP (Negotiated speed: %sGT/s, width: x%lu)\n",
> +		 (val & PCIE_CLIENT_NEG_LINK_SPEED) ? "5" : "2.5",
> +		 ((val & PCIE_CLIENT_NEG_LINK_WIDTH_MASK) >>
> +		  PCIE_CLIENT_NEG_LINK_WIDTH_SHIFT) << 1);
> +
> +	/* Notify the function */
> +	pci_epc_linkup(ep->epc);
> +
> +	return;
> +
> +again:
> +	schedule_delayed_work(&ep->link_training, msecs_to_jiffies(5));
> +}
> +
>  static const struct pci_epc_features rockchip_pcie_epc_features = {
> -	.linkup_notifier = false,
> +	.linkup_notifier = true,
>  	.msi_capable = true,
>  	.msix_capable = false,
>  	.align = ROCKCHIP_PCIE_AT_SIZE_ALIGN,
> @@ -644,6 +723,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	rockchip = &ep->rockchip;
>  	rockchip->is_rc = false;
>  	rockchip->dev = dev;
> +	INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training);
>  
>  	epc = devm_pci_epc_create(dev, &rockchip_pcie_epc_ops);
>  	if (IS_ERR(epc)) {
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index 0263f158ee8d..24796176f658 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -26,6 +26,7 @@
>  #define MAX_LANE_NUM			4
>  #define MAX_REGION_LIMIT		32
>  #define MIN_EP_APERTURE			28
> +#define LINK_TRAIN_TIMEOUT		(500 * USEC_PER_MSEC)
>  
>  #define PCIE_CLIENT_BASE		0x0
>  #define PCIE_CLIENT_CONFIG		(PCIE_CLIENT_BASE + 0x00)
> @@ -50,6 +51,10 @@
>  #define   PCIE_CLIENT_DEBUG_LTSSM_MASK		GENMASK(5, 0)
>  #define   PCIE_CLIENT_DEBUG_LTSSM_L1		0x18
>  #define   PCIE_CLIENT_DEBUG_LTSSM_L2		0x19
> +#define PCIE_CLIENT_BASIC_STATUS0	(PCIE_CLIENT_BASE + 0x44)
> +#define   PCIE_CLIENT_NEG_LINK_WIDTH_MASK	GENMASK(7, 6)
> +#define   PCIE_CLIENT_NEG_LINK_WIDTH_SHIFT	6
> +#define   PCIE_CLIENT_NEG_LINK_SPEED		BIT(5)
>  #define PCIE_CLIENT_BASIC_STATUS1	(PCIE_CLIENT_BASE + 0x48)
>  #define   PCIE_CLIENT_LINK_STATUS_UP		0x00300000
>  #define   PCIE_CLIENT_LINK_STATUS_MASK		0x00300000
> @@ -87,6 +92,8 @@
>  
>  #define PCIE_CORE_CTRL_MGMT_BASE	0x900000
>  #define PCIE_CORE_CTRL			(PCIE_CORE_CTRL_MGMT_BASE + 0x000)
> +#define   PCIE_CORE_PL_CONF_LS_MASK		0x00000001
> +#define   PCIE_CORE_PL_CONF_LS_READY		0x00000001
>  #define   PCIE_CORE_PL_CONF_SPEED_5G		0x00000008
>  #define   PCIE_CORE_PL_CONF_SPEED_MASK		0x00000018
>  #define   PCIE_CORE_PL_CONF_LANE_MASK		0x00000006
> @@ -144,6 +151,7 @@
>  #define PCIE_RC_CONFIG_BASE		0xa00000
>  #define PCIE_EP_CONFIG_BASE		0xa00000
>  #define PCIE_EP_CONFIG_DID_VID		(PCIE_EP_CONFIG_BASE + 0x00)
> +#define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
>  #define PCIE_RC_CONFIG_RID_CCR		(PCIE_RC_CONFIG_BASE + 0x08)
>  #define PCIE_RC_CONFIG_DCR		(PCIE_RC_CONFIG_BASE + 0xc4)
>  #define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT		18
> @@ -155,6 +163,7 @@
>  #define PCIE_RC_CONFIG_LINK_CAP		(PCIE_RC_CONFIG_BASE + 0xcc)
>  #define   PCIE_RC_CONFIG_LINK_CAP_L0S		BIT(10)
>  #define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
> +#define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
>  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
>  #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
>  #define   PCIE_RC_CONFIG_THP_CAP_NEXT_MASK	GENMASK(31, 20)
> @@ -192,6 +201,8 @@
>  #define ROCKCHIP_VENDOR_ID			0x1d87
>  #define PCIE_LINK_IS_L2(x) \
>  	(((x) & PCIE_CLIENT_DEBUG_LTSSM_MASK) == PCIE_CLIENT_DEBUG_LTSSM_L2)
> +#define PCIE_LINK_TRAINING_DONE(x) \
> +	(((x) & PCIE_CORE_PL_CONF_LS_MASK) == PCIE_CORE_PL_CONF_LS_READY)
>  #define PCIE_LINK_UP(x) \
>  	(((x) & PCIE_CLIENT_LINK_STATUS_MASK) == PCIE_CLIENT_LINK_STATUS_UP)
>  #define PCIE_LINK_IS_GEN2(x) \
> -- 
> 2.47.0
> 

Re: [PATCH v5 12/14] PCI: rockchip-ep: Improve link training

[Damien Le Moal]

On 11/16/24 08:03, Bjorn Helgaas wrote:
> On Thu, Oct 17, 2024 at 10:58:47AM +0900, Damien Le Moal wrote:
>> The Rockchip RK3399 TRM V1.3 Part2, Section 17.5.8.1.2, step 7,
>> describes the endpoint mode link training process clearly and states
>> that:
>>   Insure link training completion and success by observing link_st field
>>   in PCIe Client BASIC_STATUS1 register change to 2'b11. If both side
>>   support PCIe Gen2 speed, re-train can be Initiated by asserting the
>>   Retrain Link field in Link Control and Status Register. The software
>>   should insure the BASIC_STATUS0[negotiated_speed] changes to "1", that
>>   indicates re-train to Gen2 successfully.
> 
> Since this only adds code and doesn't change existing code, I assume
> this hardware doesn't automatically train to gen2 without this new
> software assistance?
> 
> So the effect of this change is to use gen2 speed when supported by
> both partners, when previously we only got gen1?

Yes. The host side has something similar as well.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 01/14] PCI: rockchip-ep: Fix address translation unit programming

[Damien Le Moal]

On 11/16/24 07:41, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
>> index 136274533656..27a7febb74e0 100644
>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>> @@ -63,16 +63,23 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
>>  			    ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
>>  }
>>  
>> +static int rockchip_pcie_ep_ob_atu_num_bits(struct rockchip_pcie *rockchip,
>> +					    u64 pci_addr, size_t size)
>> +{
>> +	int num_pass_bits = fls64(pci_addr ^ (pci_addr + size - 1));
>> +
>> +	return clamp(num_pass_bits, ROCKCHIP_PCIE_AT_MIN_NUM_BITS,
>> +		     ROCKCHIP_PCIE_AT_MAX_NUM_BITS);
>> +}
>> +
>>  static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
>>  					 u32 r, u64 cpu_addr, u64 pci_addr,
>>  					 size_t size)
>>  {
>> -	int num_pass_bits = fls64(size - 1);
>> +	int num_pass_bits =
>> +		rockchip_pcie_ep_ob_atu_num_bits(rockchip, pci_addr, size);
>>  	u32 addr0, addr1, desc0;
>>  
>> -	if (num_pass_bits < 8)
>> -		num_pass_bits = 8;
>> -
>>  	addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
>>  		(lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> 
> PCIE_CORE_OB_REGION_ADDR0_NUM_BITS is 0x3f and
> rockchip_pcie_ep_ob_atu_num_bits() returns something between 8 and
> 0x14, inclusive?  So masking with PCIE_CORE_OB_REGION_ADDR0_NUM_BITS
> doesn't do anything, does it?

Indeed, we could remove that mask.

> Also, "..._NUM_BITS" is kind of a weird name for a mask.

Well, I did not change that. It was like this. Can clean that up too. Do you
want me to send a patch ?

> rockchip_pcie_prog_ob_atu() in pcie-rockchip-host.c is similar but
> different; it looks like all callers supply num_pass_bits=19.  I
> assume it doesn't need a similar change?

I did not check the TRM for host mode. But for my tests, I used 2 rockpro64, one
as RC and the other as EP, and the RC side was working just fine without any
change. So I assume it is OK as-is.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 13/14] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Bjorn Helgaas]

On Thu, Oct 17, 2024 at 10:58:48AM +0900, Damien Le Moal wrote:
> Currently, the Rockchip PCIe endpoint controller driver does not handle
> the PERST# signal, which prevents detecting when link training should
> actually be started or if the host resets the device. This however can
> be supported using the controller reset_gpios property set as an input
> GPIO for endpoint mode.

> @@ -50,6 +51,9 @@ struct rockchip_pcie_ep {
>  	u64			irq_pci_addr;
>  	u8			irq_pci_fn;
>  	u8			irq_pending;
> +	int			perst_irq;
> +	bool			perst_asserted;
> +	bool			link_up;
>  	struct delayed_work	link_training;
>  };

I should have caught this last cycle, but just noticed this:

  $ make W=1 -k drivers/pci/ drivers/misc/pci_*
  ...
    CC      drivers/pci/controller/pcie-rockchip-ep.o
  drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'perst_irq' not described in 'rockchip_pcie_ep'
  drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'perst_asserted' not described in 'rockchip_pcie_ep'
  drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'link_up' not described in 'rockchip_pcie_ep'
  drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'link_training' not described in 'rockchip_pcie_ep'

Re: [PATCH v5 13/14] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Damien Le Moal]

On 12/15/24 09:13, Bjorn Helgaas wrote:
> On Thu, Oct 17, 2024 at 10:58:48AM +0900, Damien Le Moal wrote:
>> Currently, the Rockchip PCIe endpoint controller driver does not handle
>> the PERST# signal, which prevents detecting when link training should
>> actually be started or if the host resets the device. This however can
>> be supported using the controller reset_gpios property set as an input
>> GPIO for endpoint mode.
> 
>> @@ -50,6 +51,9 @@ struct rockchip_pcie_ep {
>>  	u64			irq_pci_addr;
>>  	u8			irq_pci_fn;
>>  	u8			irq_pending;
>> +	int			perst_irq;
>> +	bool			perst_asserted;
>> +	bool			link_up;
>>  	struct delayed_work	link_training;
>>  };
> 
> I should have caught this last cycle, but just noticed this:
> 
>   $ make W=1 -k drivers/pci/ drivers/misc/pci_*
>   ...
>     CC      drivers/pci/controller/pcie-rockchip-ep.o
>   drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'perst_irq' not described in 'rockchip_pcie_ep'
>   drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'perst_asserted' not described in 'rockchip_pcie_ep'
>   drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'link_up' not described in 'rockchip_pcie_ep'
>   drivers/pci/controller/pcie-rockchip-ep.c:59: warning: Function parameter or struct member 'link_training' not described in 'rockchip_pcie_ep'

Oops... Sending a fix.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Manivannan Sadhasivam]

On Thu, Oct 17, 2024 at 10:58:35AM +0900, Damien Le Moal wrote:
> This patch series fix the PCI address mapping handling of the Rockchip
> PCI endpoint driver, refactor some of its code, improves link training
> and adds handling of the PERST# signal.
> 
> This series is organized as follows:
>  - Patch 1 fixes the rockchip ATU programming
>  - Patch 2, 3 and 4 introduce small code improvments
>  - Patch 5 implements the .align_addr() operation to make the RK3399
>    endpoint controller driver fully functional with the new
>    pci_epc_mem_map() function
>  - Patch 6 uses the new align_addr operation function to fix the ATU
>    programming for MSI IRQ data mapping
>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
>    readable
>  - Patch 11 introduces the .stop() endpoint controller operation to
>    correctly disable the endpopint controller after use
>  - Patch 12 improves link training
>  - Patch 13 implements handling of the #PERST signal
>  - Patch 14 adds a DT overlay file to enable EP mode and define the
>    PERST# GPIO (reset-gpios) property.
> 

Damien, please wait for my review before spinning the next revision. Sorry for
the delay.

- Mani

> These patches were tested using a Pine Rockpro64 board used as an
> endpoint with the test endpoint function driver and a prototype nvme
> endpoint function driver.
> 
> Changes from v4:
>  - Added patch 6
>  - Added comments to patch 12 and 13 to clarify link training handling
>    and PERST# GPIO use.
>  - Added patch 14
> 
> Changes from v3:
>  - Addressed Mani's comments (see mailing list for details).
>  - Removed old patch 11 (dt-binding changes) and instead use in patch 12
>    the already defined reset_gpios property.
>  - Added patch 6
>  - Added review tags
> 
> Changes from v2:
>  - Split the patch series
>  - Corrected patch 11 to add the missing "maxItem"
> 
> Changes from v1:
>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>    1.
>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>    (former patch 2 of v1)
>  - Various typos cleanups all over. Also fixed some blank space
>    indentation.
>  - Added review tags
> 
> Damien Le Moal (14):
>   PCI: rockchip-ep: Fix address translation unit programming
>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
>   PCI: rockchip-ep: Implement the pci_epc_ops::align_addr() operation
>   PCI: rockchip-ep: Fix MSI IRQ data mapping
>   PCI: rockchip-ep: Rename rockchip_pcie_parse_ep_dt()
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
>   PCI: rockchip-ep: Refactor endpoint link training enable
>   PCI: rockship-ep: Implement the pci_epc_ops::stop_link() operation
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
>   arm64: dts: rockchip: Add rockpro64 overlay for PCIe endpoint mode
> 
>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../rockchip/rk3399-rockpro64-pcie-ep.dtso    |  20 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 432 ++++++++++++++----
>  drivers/pci/controller/pcie-rockchip-host.c   |   4 +-
>  drivers/pci/controller/pcie-rockchip.c        |  21 +-
>  drivers/pci/controller/pcie-rockchip.h        |  24 +-
>  6 files changed, 406 insertions(+), 96 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-rockpro64-pcie-ep.dtso
> 
> -- 
> 2.47.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Niklas Cassel]

On 16 December 2024 06:49:53 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
>On Thu, Oct 17, 2024 at 10:58:35AM +0900, Damien Le Moal wrote:
>> This patch series fix the PCI address mapping handling of the Rockchip
>> PCI endpoint driver, refactor some of its code, improves link training
>> and adds handling of the PERST# signal.
>> 
>> This series is organized as follows:
>>  - Patch 1 fixes the rockchip ATU programming
>>  - Patch 2, 3 and 4 introduce small code improvments
>>  - Patch 5 implements the .align_addr() operation to make the RK3399
>>    endpoint controller driver fully functional with the new
>>    pci_epc_mem_map() function
>>  - Patch 6 uses the new align_addr operation function to fix the ATU
>>    programming for MSI IRQ data mapping
>>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
>>    readable
>>  - Patch 11 introduces the .stop() endpoint controller operation to
>>    correctly disable the endpopint controller after use
>>  - Patch 12 improves link training
>>  - Patch 13 implements handling of the #PERST signal
>>  - Patch 14 adds a DT overlay file to enable EP mode and define the
>>    PERST# GPIO (reset-gpios) property.
>> 
>
>Damien, please wait for my review before spinning the next revision. Sorry for
>the delay.

Mani,

This series has already been merged, and is in Torvalds tree.

Except for patch 14/14 which is a device tree overlay, so it should go via the rockchip tree.


Kind regards,
Niklas

Re: [PATCH v5 00/14] Fix and improve the Rockchip endpoint driver

[Manivannan Sadhasivam]

On Mon, Dec 16, 2024 at 07:00:16AM +0100, Niklas Cassel wrote:
> 
> 
> On 16 December 2024 06:49:53 CET, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote:
> >On Thu, Oct 17, 2024 at 10:58:35AM +0900, Damien Le Moal wrote:
> >> This patch series fix the PCI address mapping handling of the Rockchip
> >> PCI endpoint driver, refactor some of its code, improves link training
> >> and adds handling of the PERST# signal.
> >> 
> >> This series is organized as follows:
> >>  - Patch 1 fixes the rockchip ATU programming
> >>  - Patch 2, 3 and 4 introduce small code improvments
> >>  - Patch 5 implements the .align_addr() operation to make the RK3399
> >>    endpoint controller driver fully functional with the new
> >>    pci_epc_mem_map() function
> >>  - Patch 6 uses the new align_addr operation function to fix the ATU
> >>    programming for MSI IRQ data mapping
> >>  - Patch 7, 8, 9 and 10 refactor the driver code to make it more
> >>    readable
> >>  - Patch 11 introduces the .stop() endpoint controller operation to
> >>    correctly disable the endpopint controller after use
> >>  - Patch 12 improves link training
> >>  - Patch 13 implements handling of the #PERST signal
> >>  - Patch 14 adds a DT overlay file to enable EP mode and define the
> >>    PERST# GPIO (reset-gpios) property.
> >> 
> >
> >Damien, please wait for my review before spinning the next revision. Sorry for
> >the delay.
> 
> Mani,
> 
> This series has already been merged, and is in Torvalds tree.
> 

Doh! I was referring to the NVMe EPF series. Let me reply there (and also get
coffee).

- Mani

> Except for patch 14/14 which is a device tree overlay, so it should go via the rockchip tree.
> 
> 
> Kind regards,
> Niklas

-- 
மணிவண்ணன் சதாசிவம்