[PATCH v3 00/12]

[PATCH v3 00/12]

[Damien Le Moal]

This patch series is the second part of the former version 2 of the
patch series "Improve PCI memory mapping API". This second part is split
out as it deals solely with the rockchip/rk3399 PCI endpoint controller
driver.

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 .map_align() operation to make the rk3399
   endpoint controller driver fully functional
 - Patch 6, 7 and 8 refactor the driver code to make it more readable
 - Patch 9 introduces the .stop() endpoint controller operation to
   correctly disable the endpopint controller after use
 - Patch 10 improves link training
 - Patch 11 and 12 implement handling of the #PERST signal

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 (11):
  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 .map_align() controller operation
  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: Introduce rockchip_pcie_ep_stop()
  PCI: rockchip-ep: Improve link training
  PCI: rockchip-ep: Handle PERST# signal in endpoint mode

Wilfred Mallawa (1):
  dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

 .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   4 +
 drivers/pci/controller/pcie-rockchip-ep.c     | 392 +++++++++++++++---
 drivers/pci/controller/pcie-rockchip.c        |  17 +-
 drivers/pci/controller/pcie-rockchip.h        |  22 +
 4 files changed, 358 insertions(+), 77 deletions(-)

-- 
2.46.2

[PATCH v3 01/12] 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 iscalculated 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.

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

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

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:07PM +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 iscalculated 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.
> 

How did you end up with these clamping values? Are the values (at least MAX
applicable to all SoCs)?

Btw, it would be helpful if you referenced the TRM and the section that
describes the outbound mapping. I'm able to find the reference:

Rockchip RK3399 TRM V1.3 Part2, Section 17.5.5.1.1

- Mani

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

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

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

[Damien Le Moal]

On 2024/10/10 16:02, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:07PM +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 iscalculated 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.
>>
> 
> How did you end up with these clamping values? Are the values (at least MAX
> applicable to all SoCs)?
> 
> Btw, it would be helpful if you referenced the TRM and the section that
> describes the outbound mapping. I'm able to find the reference:
> 
> Rockchip RK3399 TRM V1.3 Part2, Section 17.5.5.1.1

OK. Will add that.

I really appreciate very much all the reviews you are sending, but given that
this patch series depends on the series "[PATCH v4 0/7] Improve PCI memory
mapping API", could we start with that one and get it queued ASAP ?

Thanks !

-- 
Damien Le Moal
Western Digital Research

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

[Manivannan Sadhasivam]

On Thu, Oct 10, 2024 at 05:41:56PM +0900, Damien Le Moal wrote:
> On 2024/10/10 16:02, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:12:07PM +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 iscalculated 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.
> >>
> > 
> > How did you end up with these clamping values? Are the values (at least MAX
> > applicable to all SoCs)?
> > 
> > Btw, it would be helpful if you referenced the TRM and the section that
> > describes the outbound mapping. I'm able to find the reference:
> > 
> > Rockchip RK3399 TRM V1.3 Part2, Section 17.5.5.1.1
> 
> OK. Will add that.
> 
> I really appreciate very much all the reviews you are sending, but given that
> this patch series depends on the series "[PATCH v4 0/7] Improve PCI memory
> mapping API", could we start with that one and get it queued ASAP ?
> 

Sure. Sorry for being late btw. Personal errands are eating up the review time.

- Mani

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

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

[Damien Le Moal]

Introduce the macro ROCKCHIP_PCIE_AT_SIZE_ALIGN defined using
ROCKCHIP_PCIE_AT_MIN_NUM_BITS to initialize the .align field of the
controller epc_features structure, avoiding using the "magic" value 8
directly.

Signed-off-by: Damien Le Moal <dlemoal@kernel.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.46.2

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

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:08PM +0900, Damien Le Moal wrote:
> Introduce the macro ROCKCHIP_PCIE_AT_SIZE_ALIGN defined using
> ROCKCHIP_PCIE_AT_MIN_NUM_BITS to initialize the .align field of the
> controller epc_features structure, avoiding using the "magic" value 8
> directly.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

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

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

[PATCH v3 03/12] 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.46.2

Re: [PATCH v3 03/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:09PM +0900, Damien Le Moal wrote:
> 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))

Having these two checks looks redundant to me. Is it possible that an address
could pass only one check?

- Mani

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

Re: [PATCH v3 03/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()

[Damien Le Moal]

On 10/10/24 16:09, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:09PM +0900, Damien Le Moal wrote:
>> 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))
> 
> Having these two checks looks redundant to me. Is it possible that an address
> could pass only one check?

Yes, if the wrong address is passed to rockchip_pcie_ep_unmap_addr() but that
address still correspond to an ob_region that is being used.

We could do add a WARN_ON_ONCE() around that if condition as calling that
function with an invalid address would mean that either the epc core or the
function driver is buggy.




-- 
Damien Le Moal
Western Digital Research

[PATCH v3 04/12] 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>
---
 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.46.2

Re: [PATCH v3 04/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote:
> 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>

- Mani

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

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

Re: [PATCH v3 04/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Manivannan Sadhasivam]

On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote:
> > 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>
> 

I'm trying to understand the ob window mapping here. So if rockchip_ob_region()
returns same index for different *CPU* addresses, then you cannot map both of
them? Is this a hardware ATU limitation?

Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr.
So I'm not sure how the mapping happens either.

- Mani

> - Mani
> 
> > ---
> >  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.46.2
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v3 04/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Damien Le Moal]

On 10/12/24 18:31, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote:
>>> 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>
>>
> 
> I'm trying to understand the ob window mapping here. So if rockchip_ob_region()
> returns same index for different *CPU* addresses, then you cannot map both of
> them? Is this a hardware ATU limitation?

The CPU addresses mapped are (under normal use) addresses from one of the 32 1MB
memory windows that pci_epc_alloc_addr() will give us. To map a memory window
address, we use the ATU entry at the index given by:

static inline u32 rockchip_ob_region(phys_addr_t addr)
{
        return (addr >> ilog2(SZ_1M)) & 0x1f;
}

So each memory window always use the same ATU entry and addresses from different
windows cannot use the same entry, ever.

But if there is a bug in the endpoint driver and map() or unmap() are called
with an invalid address (e.g. one that is not from a memory window), we will
still get a valid ATU entry number for that address. Hence the check that the
address passed to unmap is the one that is actually mapped, and also why we
check that the entry for an address to map is not being used.

> Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr.
> So I'm not sure how the mapping happens either.

Each ATU entry is for the corresponding memory window at the same index in the
controller memory. So the cpu address is not needed when programming the ATU as
it is implied from the entry we are programming.

So we could remove the cpu_addr argument of this function.

I also suspect that we potentially could play games with the .align_addr() to
assume a fixed 1MB alignment constraint for a PCI address mapping and always
pass 20 to the num_bits field of the ADDR0 register. However, I have not tried that.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 04/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Manivannan Sadhasivam]

On Sat, Oct 12, 2024 at 09:02:43PM +0900, Damien Le Moal wrote:
> On 10/12/24 18:31, Manivannan Sadhasivam wrote:
> > On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote:
> >> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote:
> >>> 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>
> >>
> > 
> > I'm trying to understand the ob window mapping here. So if rockchip_ob_region()
> > returns same index for different *CPU* addresses, then you cannot map both of
> > them? Is this a hardware ATU limitation?
> 
> The CPU addresses mapped are (under normal use) addresses from one of the 32 1MB
> memory windows that pci_epc_alloc_addr() will give us. To map a memory window
> address, we use the ATU entry at the index given by:
> 
> static inline u32 rockchip_ob_region(phys_addr_t addr)
> {
>         return (addr >> ilog2(SZ_1M)) & 0x1f;
> }
> 
> So each memory window always use the same ATU entry and addresses from different
> windows cannot use the same entry, ever.
> 
> But if there is a bug in the endpoint driver and map() or unmap() are called
> with an invalid address (e.g. one that is not from a memory window), we will
> still get a valid ATU entry number for that address. Hence the check that the
> address passed to unmap is the one that is actually mapped, and also why we
> check that the entry for an address to map is not being used.
> 
> > Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr.
> > So I'm not sure how the mapping happens either.
> 
> Each ATU entry is for the corresponding memory window at the same index in the
> controller memory. So the cpu address is not needed when programming the ATU as
> it is implied from the entry we are programming.
> 

Ah okay, thanks a lot for the explanation.

> So we could remove the cpu_addr argument of this function.
> 

yeah, and may be a comment would also help.

> I also suspect that we potentially could play games with the .align_addr() to
> assume a fixed 1MB alignment constraint for a PCI address mapping and always
> pass 20 to the num_bits field of the ADDR0 register. However, I have not tried that.
> 

Ok!

- Mani

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

[PATCH v3 05/12] PCI: rockchip-ep: Implement the .map_align() controller 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 ->map_align() endpoint controller operation to allow the
mapping alignment to be transparently handled by endpoint function
drivers through the function pci_epc_map_align().

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 | 22 ++++++++++++++++++++++
 drivers/pci/controller/pcie-rockchip.h    |  5 +++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index edb84fb1ba39..a9b319d4e507 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -235,6 +235,27 @@ static inline u32 rockchip_ob_region(phys_addr_t addr)
 	return (addr >> ilog2(SZ_1M)) & 0x1f;
 }
 
+static int rockchip_pcie_ep_map_align(struct pci_epc *epc, u8 fn, u8 vfn,
+				      struct pci_epc_map *map)
+{
+	struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
+	int num_bits;
+
+	num_bits = rockchip_pcie_ep_ob_atu_num_bits(&ep->rockchip,
+						map->pci_addr, map->pci_size);
+
+	map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
+	map->map_ofst = map->pci_addr - map->map_pci_addr;
+
+	if (map->map_ofst + map->pci_size > SZ_1M)
+		map->pci_size = SZ_1M - map->map_ofst;
+
+	map->map_size = ALIGN(map->map_ofst + map->pci_size,
+			      ROCKCHIP_PCIE_AT_SIZE_ALIGN);
+
+	return 0;
+}
+
 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 +479,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,
+	.map_align	= rockchip_pcie_ep_map_align,
 	.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.46.2

Re: [PATCH v3 05/12] PCI: rockchip-ep: Implement the .map_align() controller operation

[kernel test robot]

Hi Damien,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus mani-mhi/mhi-next linus/master v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/PCI-rockchip-ep-Use-a-macro-to-define-EP-controller-align-feature/20241007-131224
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241007041218.157516-6-dlemoal%40kernel.org
patch subject: [PATCH v3 05/12] PCI: rockchip-ep: Implement the .map_align() controller operation
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20241010/202410101044.3nr7XIR9-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101044.3nr7XIR9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410101044.3nr7XIR9-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/pci/controller/pcie-rockchip-ep.c:239:46: warning: 'struct pci_epc_map' declared inside parameter list will not be visible outside of this definition or declaration
     239 |                                       struct pci_epc_map *map)
         |                                              ^~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_map_align':
>> drivers/pci/controller/pcie-rockchip-ep.c:245:52: error: invalid use of undefined type 'struct pci_epc_map'
     245 |                                                 map->pci_addr, map->pci_size);
         |                                                    ^~
   drivers/pci/controller/pcie-rockchip-ep.c:245:67: error: invalid use of undefined type 'struct pci_epc_map'
     245 |                                                 map->pci_addr, map->pci_size);
         |                                                                   ^~
   drivers/pci/controller/pcie-rockchip-ep.c:247:12: error: invalid use of undefined type 'struct pci_epc_map'
     247 |         map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
         |            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:247:32: error: invalid use of undefined type 'struct pci_epc_map'
     247 |         map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
         |                                ^~
   drivers/pci/controller/pcie-rockchip-ep.c:248:12: error: invalid use of undefined type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:248:28: error: invalid use of undefined type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:248:44: error: invalid use of undefined type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |                                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:250:16: error: invalid use of undefined type 'struct pci_epc_map'
     250 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |                ^~
   drivers/pci/controller/pcie-rockchip-ep.c:250:32: error: invalid use of undefined type 'struct pci_epc_map'
     250 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |                                ^~
   drivers/pci/controller/pcie-rockchip-ep.c:251:20: error: invalid use of undefined type 'struct pci_epc_map'
     251 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                    ^~
   drivers/pci/controller/pcie-rockchip-ep.c:251:44: error: invalid use of undefined type 'struct pci_epc_map'
     251 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:253:12: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |            ^~
   In file included from include/vdso/const.h:5,
                    from include/linux/const.h:4,
                    from include/uapi/linux/kernel.h:6,
                    from include/linux/cache.h:5,
                    from include/linux/time.h:5,
                    from include/linux/stat.h:19,
                    from include/linux/configfs.h:22,
                    from drivers/pci/controller/pcie-rockchip-ep.c:11:
   drivers/pci/controller/pcie-rockchip-ep.c:253:34: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:50: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:34: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                  ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:50: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                  ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:34: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                             ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:50: error: invalid use of undefined type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                             ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:253:25: note: in expansion of macro 'ALIGN'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c: At top level:
>> drivers/pci/controller/pcie-rockchip-ep.c:482:10: error: 'const struct pci_epc_ops' has no member named 'map_align'
     482 |         .map_align      = rockchip_pcie_ep_map_align,
         |          ^~~~~~~~~
>> drivers/pci/controller/pcie-rockchip-ep.c:482:27: error: initialization of 'int (*)(struct pci_epc *, u8,  u8,  phys_addr_t,  u64,  size_t)' {aka 'int (*)(struct pci_epc *, unsigned char,  unsigned char,  long long unsigned int,  long long unsigned int,  long unsigned int)'} from incompatible pointer type 'int (*)(struct pci_epc *, u8,  u8,  struct pci_epc_map *)' {aka 'int (*)(struct pci_epc *, unsigned char,  unsigned char,  struct pci_epc_map *)'} [-Wincompatible-pointer-types]
     482 |         .map_align      = rockchip_pcie_ep_map_align,
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:482:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')
   drivers/pci/controller/pcie-rockchip-ep.c:483:27: warning: initialized field overwritten [-Woverride-init]
     483 |         .map_addr       = rockchip_pcie_ep_map_addr,
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:483:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')


vim +245 drivers/pci/controller/pcie-rockchip-ep.c

   237	
   238	static int rockchip_pcie_ep_map_align(struct pci_epc *epc, u8 fn, u8 vfn,
 > 239					      struct pci_epc_map *map)
   240	{
   241		struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
   242		int num_bits;
   243	
   244		num_bits = rockchip_pcie_ep_ob_atu_num_bits(&ep->rockchip,
 > 245							map->pci_addr, map->pci_size);
   246	
   247		map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
   248		map->map_ofst = map->pci_addr - map->map_pci_addr;
   249	
   250		if (map->map_ofst + map->pci_size > SZ_1M)
   251			map->pci_size = SZ_1M - map->map_ofst;
   252	
   253		map->map_size = ALIGN(map->map_ofst + map->pci_size,
   254				      ROCKCHIP_PCIE_AT_SIZE_ALIGN);
   255	
   256		return 0;
   257	}
   258	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 05/12] PCI: rockchip-ep: Implement the .map_align() controller operation

[kernel test robot]

Hi Damien,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus mani-mhi/mhi-next linus/master v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/PCI-rockchip-ep-Use-a-macro-to-define-EP-controller-align-feature/20241007-131224
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241007041218.157516-6-dlemoal%40kernel.org
patch subject: [PATCH v3 05/12] PCI: rockchip-ep: Implement the .map_align() controller operation
config: i386-buildonly-randconfig-003-20241010 (https://download.01.org/0day-ci/archive/20241010/202410101109.J2ej9dSg-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101109.J2ej9dSg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410101109.J2ej9dSg-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/pci/controller/pcie-rockchip-ep.c:239:18: warning: declaration of 'struct pci_epc_map' will not be visible outside of this function [-Wvisibility]
     239 |                                       struct pci_epc_map *map)
         |                                              ^
>> drivers/pci/controller/pcie-rockchip-ep.c:245:10: error: incomplete definition of type 'struct pci_epc_map'
     245 |                                                 map->pci_addr, map->pci_size);
         |                                                 ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:245:25: error: incomplete definition of type 'struct pci_epc_map'
     245 |                                                 map->pci_addr, map->pci_size);
         |                                                                ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:247:5: error: incomplete definition of type 'struct pci_epc_map'
     247 |         map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
         |         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:247:25: error: incomplete definition of type 'struct pci_epc_map'
     247 |         map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
         |                             ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:248:5: error: incomplete definition of type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:248:21: error: incomplete definition of type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |                         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:248:37: error: incomplete definition of type 'struct pci_epc_map'
     248 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |                                         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:250:9: error: incomplete definition of type 'struct pci_epc_map'
     250 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |             ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:250:25: error: incomplete definition of type 'struct pci_epc_map'
     250 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |                             ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:251:6: error: incomplete definition of type 'struct pci_epc_map'
     251 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                 ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:251:30: error: incomplete definition of type 'struct pci_epc_map'
     251 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                                         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:253:5: error: incomplete definition of type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |         ~~~^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:253:27: error: incomplete definition of type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                               ~~~^
   include/linux/align.h:8:38: note: expanded from macro 'ALIGN'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                                 ^
   include/uapi/linux/const.h:48:51: note: expanded from macro '__ALIGN_KERNEL'
      48 | #define __ALIGN_KERNEL(x, a)            __ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
         |                                                             ^
   include/uapi/linux/const.h:49:41: note: expanded from macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)
         |                                              ^
   drivers/pci/controller/pcie-rockchip-ep.c:253:43: error: incomplete definition of type 'struct pci_epc_map'
     253 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                               ~~~^
   include/linux/align.h:8:38: note: expanded from macro 'ALIGN'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                                 ^
   include/uapi/linux/const.h:48:51: note: expanded from macro '__ALIGN_KERNEL'
      48 | #define __ALIGN_KERNEL(x, a)            __ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
         |                                                             ^
   include/uapi/linux/const.h:49:41: note: expanded from macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   drivers/pci/controller/pcie-rockchip-ep.c:239:18: note: forward declaration of 'struct pci_epc_map'
     239 |                                       struct pci_epc_map *map)


vim +245 drivers/pci/controller/pcie-rockchip-ep.c

   237	
   238	static int rockchip_pcie_ep_map_align(struct pci_epc *epc, u8 fn, u8 vfn,
 > 239					      struct pci_epc_map *map)
   240	{
   241		struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
   242		int num_bits;
   243	
   244		num_bits = rockchip_pcie_ep_ob_atu_num_bits(&ep->rockchip,
 > 245							map->pci_addr, map->pci_size);
   246	
   247		map->map_pci_addr = map->pci_addr & ~((1ULL << num_bits) - 1);
   248		map->map_ofst = map->pci_addr - map->map_pci_addr;
   249	
   250		if (map->map_ofst + map->pci_size > SZ_1M)
   251			map->pci_size = SZ_1M - map->map_ofst;
   252	
   253		map->map_size = ALIGN(map->map_ofst + map->pci_size,
   254				      ROCKCHIP_PCIE_AT_SIZE_ALIGN);
   255	
   256		return 0;
   257	}
   258	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 06/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations

[Damien Le Moal]

Introduce the function rockchip_pcie_ep_get_resources() to parse the DT
node of a rockchip PCIe endpoint controller and allocate the outbound
memory region and memory needed for IRQ handling. This function tidies
up rockchip_pcie_ep_probe(). No functional change.

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

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index a9b319d4e507..523e9cdfd241 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -524,15 +524,70 @@ static const struct of_device_id rockchip_pcie_ep_of_match[] = {
 	{},
 };
 
+static int rockchip_pcie_ep_get_resources(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;
+
+	err = rockchip_pcie_parse_ep_dt(rockchip, ep);
+	if (err)
+		return err;
+
+	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_release_resources(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);
@@ -552,13 +607,13 @@ 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(ep);
 	if (err)
 		return err;
 
 	err = rockchip_pcie_enable_clocks(rockchip);
 	if (err)
-		return err;
+		goto err_release_resources;
 
 	err = rockchip_pcie_init_port(rockchip);
 	if (err)
@@ -568,47 +623,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
@@ -638,10 +655,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_release_resources:
+	rockchip_pcie_ep_release_resources(ep);
 err_disable_clocks:
 	rockchip_pcie_disable_clocks(rockchip);
 	return err;
-- 
2.46.2

Re: [PATCH v3 06/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:12PM +0900, Damien Le Moal wrote:
> Introduce the function rockchip_pcie_ep_get_resources() to parse the DT
> node of a rockchip PCIe endpoint controller and allocate the outbound
> memory region and memory needed for IRQ handling. This function tidies
> up rockchip_pcie_ep_probe(). No functional change.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 109 ++++++++++++----------
>  1 file changed, 62 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index a9b319d4e507..523e9cdfd241 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -524,15 +524,70 @@ static const struct of_device_id rockchip_pcie_ep_of_match[] = {
>  	{},
>  };
>  
> +static int rockchip_pcie_ep_get_resources(struct rockchip_pcie_ep *ep)

Almost all controller drivers use get_resources() function to acquire controller
resources like MMIO, clk, PHY etc... So if you were to refactor, I'd suggest to
first rename rockchip_pcie_parse_ep_dt() to rockchip_pcie_get_resources() to
maintain uniformity.

And if you want to move ob memory allocation to a single function to keep
probe() shorter, you should use a different function like
rockchip_pcie_ob_alloc() or something similar.

- Mani

> +{
> +	struct rockchip_pcie *rockchip = &ep->rockchip;
> +	struct device *dev = rockchip->dev;
> +	struct pci_epc_mem_window *windows = NULL;
> +	int err, i;
> +
> +	err = rockchip_pcie_parse_ep_dt(rockchip, ep);
> +	if (err)
> +		return err;
> +
> +	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_release_resources(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);
> @@ -552,13 +607,13 @@ 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(ep);
>  	if (err)
>  		return err;
>  
>  	err = rockchip_pcie_enable_clocks(rockchip);
>  	if (err)
> -		return err;
> +		goto err_release_resources;
>  
>  	err = rockchip_pcie_init_port(rockchip);
>  	if (err)
> @@ -568,47 +623,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
> @@ -638,10 +655,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_release_resources:
> +	rockchip_pcie_ep_release_resources(ep);
>  err_disable_clocks:
>  	rockchip_pcie_disable_clocks(rockchip);
>  	return err;
> -- 
> 2.46.2
> 

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

[PATCH v3 07/12] 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_msix_cap(). No functional
changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.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 523e9cdfd241..7a1798fcc2ad 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
 	pci_epc_mem_exit(ep->epc);
 }
 
+static void rockchip_pcie_ep_hide_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;
@@ -588,7 +616,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)
@@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	if (err)
 		goto err_disable_clocks;
 
+	rockchip_pcie_ep_hide_msix_cap(rockchip);
+
 	/* Establish the link automatically */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
@@ -626,29 +655,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.46.2

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Btw, can someone from Rockchip confirm if this hiding is necessary for all the
SoCs? It looks to me like an SoC quirk.

- Mani

> ---
>  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 523e9cdfd241..7a1798fcc2ad 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>  	pci_epc_mem_exit(ep->epc);
>  }
>  
> +static void rockchip_pcie_ep_hide_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;
> @@ -588,7 +616,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)
> @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_disable_clocks;
>  
> +	rockchip_pcie_ep_hide_msix_cap(rockchip);
> +
>  	/* Establish the link automatically */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
> @@ -626,29 +655,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.46.2
> 

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

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Manivannan Sadhasivam]

On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> > Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> > to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> > changes.
> > 
> > Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> SoCs? It looks to me like an SoC quirk.
> 
> - Mani
> 
> > ---
> >  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 523e9cdfd241..7a1798fcc2ad 100644
> > --- a/drivers/pci/controller/pcie-rockchip-ep.c
> > +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> > @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
> >  	pci_epc_mem_exit(ep->epc);
> >  }
> >  
> > +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)

Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
function essentially disables MSIx which is broken. Again, it'd be good to know
if this applies to all SoCs or just a few.

- Mani

> > +{
> > +	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;
> > @@ -588,7 +616,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)
> > @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> >  	if (err)
> >  		goto err_disable_clocks;
> >  
> > +	rockchip_pcie_ep_hide_msix_cap(rockchip);
> > +
> >  	/* Establish the link automatically */
> >  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> >  			    PCIE_CLIENT_CONFIG);
> > @@ -626,29 +655,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.46.2
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Damien Le Moal]

On 2024/10/10 17:09, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>>> changes.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
>> SoCs? It looks to me like an SoC quirk.
>>
>> - Mani
>>
>>> ---
>>>  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 523e9cdfd241..7a1798fcc2ad 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>>>  	pci_epc_mem_exit(ep->epc);
>>>  }
>>>  
>>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> 
> Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> function essentially disables MSIx which is broken. Again, it'd be good to know
> if this applies to all SoCs or just a few.

This is for the rk3399... I am not aware of multiple versions of that SoC.
The pcie_rockchip driver is for that SoC only as far as I know. This is unlike
the Designware IP block which is used in multiple SoCs.

> 
> - Mani
> 
>>> +{
>>> +	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;
>>> @@ -588,7 +616,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)
>>> @@ -619,6 +646,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>>>  	if (err)
>>>  		goto err_disable_clocks;
>>>  
>>> +	rockchip_pcie_ep_hide_msix_cap(rockchip);
>>> +
>>>  	/* Establish the link automatically */
>>>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>>>  			    PCIE_CLIENT_CONFIG);
>>> @@ -626,29 +655,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.46.2
>>>
>>
>> -- 
>> மணிவண்ணன் சதாசிவம்
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Damien Le Moal]

On 10/10/24 17:09, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>>> changes.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
>> SoCs? It looks to me like an SoC quirk.
>>
>> - Mani
>>
>>> ---
>>>  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 523e9cdfd241..7a1798fcc2ad 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
>>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
>>>  	pci_epc_mem_exit(ep->epc);
>>>  }
>>>  
>>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> 
> Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> function essentially disables MSIx which is broken. Again, it'd be good to know
> if this applies to all SoCs or just a few.

The function does not disable anything but *really* simply hides the capability
in the capability list so that the host does not see it. So I think the better
name is:

rockchip_pcie_ep_hide_broken_msix_cap()

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 05:30:00PM +0900, Damien Le Moal wrote:
> On 10/10/24 17:09, Manivannan Sadhasivam wrote:
> > On Thu, Oct 10, 2024 at 12:55:12PM +0530, Manivannan Sadhasivam wrote:
> >> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> >>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> >>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> >>> changes.
> >>>
> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>
> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>
> >> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> >> SoCs? It looks to me like an SoC quirk.
> >>
> >> - Mani
> >>
> >>> ---
> >>>  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 523e9cdfd241..7a1798fcc2ad 100644
> >>> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> >>> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> >>> @@ -581,6 +581,34 @@ static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
> >>>  	pci_epc_mem_exit(ep->epc);
> >>>  }
> >>>  
> >>> +static void rockchip_pcie_ep_hide_msix_cap(struct rockchip_pcie *rockchip)
> > 
> > Perhaps a better name would be rockchip_pcie_disable_broken_msix()? As the
> > function essentially disables MSIx which is broken. Again, it'd be good to know
> > if this applies to all SoCs or just a few.
> 
> The function does not disable anything but *really* simply hides the capability
> in the capability list so that the host does not see it. So I think the better
> name is:
> 
> rockchip_pcie_ep_hide_broken_msix_cap()

Sounds good to me.

- Mani

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

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Damien Le Moal]

On 10/10/24 16:25, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
>> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
>> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
>> changes.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> SoCs? It looks to me like an SoC quirk.

All SoCs ? Are there several versions of the RK3399 ?
As far as I know, there is only one. This is unlike the designware IP block used
in the RK3588 which can also be found in other SoC and may have some variations
due to different synthesis parameters.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 07/12] PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 05:25:56PM +0900, Damien Le Moal wrote:
> On 10/10/24 16:25, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:12:13PM +0900, Damien Le Moal wrote:
> >> Move the code in rockchip_pcie_ep_probe() to hide the MSI-X capability
> >> to its own function, rockchip_pcie_ep_hide_msix_cap(). No functional
> >> changes.
> >>
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Btw, can someone from Rockchip confirm if this hiding is necessary for all the
> > SoCs? It looks to me like an SoC quirk.
> 
> All SoCs ? Are there several versions of the RK3399 ?

There seems to be some:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=296602b8e5f7

> As far as I know, there is only one. This is unlike the designware IP block used
> in the RK3588 which can also be found in other SoC and may have some variations
> due to different synthesis parameters.
> 

But anyway, let's keep the quirk until we hear otherwise.

- Mani

> -- 
> Damien Le Moal
> Western Digital Research

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

[PATCH v3 08/12] 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 move 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 | 11 ++++++-----
 drivers/pci/controller/pcie-rockchip.c    |  5 +++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 7a1798fcc2ad..99f26f4a485b 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -459,6 +459,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;
 }
 
@@ -537,7 +543,6 @@ static int rockchip_pcie_ep_get_resources(struct rockchip_pcie_ep *ep)
 
 	ep->ob_addr = devm_kcalloc(dev, ep->max_regions, sizeof(*ep->ob_addr),
 				   GFP_KERNEL);
-
 	if (!ep->ob_addr)
 		return -ENOMEM;
 
@@ -648,10 +653,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 
 	rockchip_pcie_ep_hide_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);
 
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.46.2

Re: [PATCH v3 08/12] PCI: rockchip-ep: Refactor endpoint link training enable

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:14PM +0900, Damien Le Moal wrote:
> 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.
> 

I hope that the dual enablement is done as a mistake and not on 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 move 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().
> 

But you didn't remove the existing code in probe() that sets
PCIE_CLIENT_CONF_ENABLE.

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 11 ++++++-----
>  drivers/pci/controller/pcie-rockchip.c    |  5 +++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 7a1798fcc2ad..99f26f4a485b 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -459,6 +459,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;
>  }
>  
> @@ -537,7 +543,6 @@ static int rockchip_pcie_ep_get_resources(struct rockchip_pcie_ep *ep)
>  
>  	ep->ob_addr = devm_kcalloc(dev, ep->max_regions, sizeof(*ep->ob_addr),
>  				   GFP_KERNEL);
> -

Spurious change.

- Mani

>  	if (!ep->ob_addr)
>  		return -ENOMEM;
>  
> @@ -648,10 +653,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  
>  	rockchip_pcie_ep_hide_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);
>  
> 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.46.2
> 

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

Re: [PATCH v3 08/12] PCI: rockchip-ep: Refactor endpoint link training enable

[Damien Le Moal]

On 10/10/24 17:22, Manivannan Sadhasivam wrote:
> On Mon, Oct 07, 2024 at 01:12:14PM +0900, Damien Le Moal wrote:
>> 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.
>>
> 
> I hope that the dual enablement is done as a mistake and not on purpose...

Yes, I think that was a mistake, likely when the EP mode support was added.

>> 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 move 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().
>>
> 
> But you didn't remove the existing code in probe() that sets
> PCIE_CLIENT_CONF_ENABLE.

Indeed. Removed. It does not seem to hurt though.

-- 
Damien Le Moal
Western Digital Research

[PATCH v3 09/12] PCI: rockship-ep: Introduce rockchip_pcie_ep_stop()

[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>
---
 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 99f26f4a485b..a801e040bcad 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -468,6 +468,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,
@@ -492,6 +504,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.46.2

Re: [PATCH v3 09/12] PCI: rockship-ep: Introduce rockchip_pcie_ep_stop()

[Manivannan Sadhasivam]

I think the subject should reflect callback implementation. Like,

PCI: rockchip-ep: Implement pci_epc_ops::stop_link() callback

On Mon, Oct 07, 2024 at 01:12:15PM +0900, Damien Le Moal wrote:
> 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>

- Mani

> ---
>  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 99f26f4a485b..a801e040bcad 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -468,6 +468,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,
> @@ -492,6 +504,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.46.2
> 

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

[PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Damien Le Moal]

The Rockchip rk339 technical reference manual describe 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 | 79 ++++++++++++++++++++++-
 drivers/pci/controller/pcie-rockchip.h    | 11 ++++
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index a801e040bcad..af50432525b4 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -16,6 +16,8 @@
 #include <linux/platform_device.h>
 #include <linux/pci-epf.h>
 #include <linux/sizes.h>
+#include <linux/workqueue.h>
+#include <linux/iopoll.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,
@@ -465,6 +468,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;
 }
 
@@ -473,6 +478,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 |
@@ -480,8 +487,77 @@ 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 */
+	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 (Negociated 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,
@@ -642,6 +718,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..3963b7097a91 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		(5000 * 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.46.2

Re: [PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:16PM +0900, Damien Le Moal wrote:
> The Rockchip rk339 technical reference manual describe the endpoint mode

RK3399

Please include the full reference: TRM name followed by the section.

> 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 | 79 ++++++++++++++++++++++-
>  drivers/pci/controller/pcie-rockchip.h    | 11 ++++
>  2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index a801e040bcad..af50432525b4 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -16,6 +16,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/pci-epf.h>
>  #include <linux/sizes.h>
> +#include <linux/workqueue.h>
> +#include <linux/iopoll.h>

Please keep the includes sorted.

>  
>  #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,
> @@ -465,6 +468,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;
>  }
>  
> @@ -473,6 +478,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 |
> @@ -480,8 +487,77 @@ 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 */
> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {

PCIE_LINK_IS_GEN2()?

> +		/* 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;

TRM doesn't mention this check. Is this really necessary?

> +
> +	val = rockchip_pcie_read(rockchip, PCIE_CLIENT_BASIC_STATUS0);
> +	dev_info(dev,
> +		 "Link UP (Negociated speed: %sGT/s, width: x%lu)\n",


Negotiated

> +		 (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,
> @@ -642,6 +718,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..3963b7097a91 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		(5000 * USEC_PER_MSEC)

pcie-rockchip-host has only 500ms timeout.

- Mani

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

Re: [PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Damien Le Moal]

On 10/10/24 19:35, Manivannan Sadhasivam wrote:
>> +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 */
>> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
>> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
> 
> PCIE_LINK_IS_GEN2()?

This is defined in drivers/pci/controller/pcie-rockchip.h. What is it exactly
you would like to know about this ?

> 
>> +		/* 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;
> 
> TRM doesn't mention this check. Is this really necessary?

I think so, to check the result of the second training for gen2.
Even though the TRM does not say so, I prefer checking that the result is what
we expect: the link is up.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 05:55:25PM +0900, Damien Le Moal wrote:
> On 10/10/24 19:35, Manivannan Sadhasivam wrote:
> >> +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 */
> >> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
> >> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
> > 
> > PCIE_LINK_IS_GEN2()?
> 
> This is defined in drivers/pci/controller/pcie-rockchip.h. What is it exactly
> you would like to know about this ?
> 

!PCIE_LINK_IS_GEN2 means check is for non-Gen2 mode, isn't it? I guess the check
should be 'if (PCIE_LINK_IS_GEN2...)

> > 
> >> +		/* 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;
> > 
> > TRM doesn't mention this check. Is this really necessary?
> 
> I think so, to check the result of the second training for gen2.
> Even though the TRM does not say so, I prefer checking that the result is what
> we expect: the link is up.
> 

Ok.

- Mani

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

Re: [PATCH v3 10/12] PCI: rockchip-ep: Improve link training

[Damien Le Moal]

On 10/12/24 21:16, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 05:55:25PM +0900, Damien Le Moal wrote:
>> On 10/10/24 19:35, Manivannan Sadhasivam wrote:
>>>> +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 */
>>>> +	val = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL);
>>>> +	if (!PCIE_LINK_IS_GEN2(val) && rockchip->link_gen == 2) {
>>>
>>> PCIE_LINK_IS_GEN2()?
>>
>> This is defined in drivers/pci/controller/pcie-rockchip.h. What is it exactly
>> you would like to know about this ?
>>
> 
> !PCIE_LINK_IS_GEN2 means check is for non-Gen2 mode, isn't it? I guess the check
> should be 'if (PCIE_LINK_IS_GEN2...)

Nope, the negative test is correct. The condition means: if we are not at GEN2
speed yet AND gen2 was requested, then initiate training again to get gen2.
So !PCIE_LINK_IS_GEN2() is correct.

-- 
Damien Le Moal
Western Digital Research

[PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Damien Le Moal]

From: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Describe the `ep-gpios` property which is used to map the PERST# input
signal for endpoint mode.

Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml      | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
index 6b62f6f58efe..a8970bda7174 100644
--- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
@@ -30,6 +30,10 @@ properties:
     maximum: 32
     default: 32
 
+  ep-gpios:
+    description: Input GPIO configured for the PERST# signal
+    maxItems: 1
+
 required:
   - rockchip,max-outbound-regions
 
-- 
2.46.2

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Krzysztof Kozlowski]

On Mon, Oct 07, 2024 at 01:12:17PM +0900, Damien Le Moal wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> Describe the `ep-gpios` property which is used to map the PERST# input
> signal for endpoint mode.

Why "ep" for PERST signal? Looks totally unrelated name. There is
already reset-gpios exactly for PERST, so you are duplicating it. Why?

Best regards,
Krzysztof

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Damien Le Moal]

On 10/7/24 15:12, Krzysztof Kozlowski wrote:
> On Mon, Oct 07, 2024 at 01:12:17PM +0900, Damien Le Moal wrote:
>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>
>> Describe the `ep-gpios` property which is used to map the PERST# input
>> signal for endpoint mode.
> 
> Why "ep" for PERST signal? Looks totally unrelated name. There is
> already reset-gpios exactly for PERST, so you are duplicating it. Why?

Because the host side controller already has the same "ep-gpios" property.

Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml

So naming that property the same allows common code to initialize that gpio in
rockchip_pcie_parse_dt().

Also, I do not see reset-gpios being defined/used by this driver (host and ep
sides).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Krzysztof Kozlowski]

On 07/10/2024 08:50, Damien Le Moal wrote:
> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, Damien Le Moal wrote:
>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>
>>> Describe the `ep-gpios` property which is used to map the PERST# input
>>> signal for endpoint mode.
>>
>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
> 
> Because the host side controller already has the same "ep-gpios" property.
> 
> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml

If host has it, then it is a common property so goes to common schema
for these devices.

> 
> So naming that property the same allows common code to initialize that gpio in
> rockchip_pcie_parse_dt().
> 
> Also, I do not see reset-gpios being defined/used by this driver (host and ep
> sides).

I am talking about bindings, not driver.

Best regards,
Krzysztof

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Damien Le Moal]

On 10/7/24 15:54, Krzysztof Kozlowski wrote:
> On 07/10/2024 08:50, Damien Le Moal wrote:
>> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, Damien Le Moal wrote:
>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>>
>>>> Describe the `ep-gpios` property which is used to map the PERST# input
>>>> signal for endpoint mode.
>>>
>>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
>>
>> Because the host side controller already has the same "ep-gpios" property.
>>
>> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> 
> If host has it, then it is a common property so goes to common schema
> for these devices.

Ah. OK. I will move it to
Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-common.yaml then.

>> So naming that property the same allows common code to initialize that gpio in
>> rockchip_pcie_parse_dt().
>>
>> Also, I do not see reset-gpios being defined/used by this driver (host and ep
>> sides).
> 
> I am talking about bindings, not driver.

I do not see reset-gpios being defined in the bindings (common, host and ep).
resets and reset-names are defined though but these have nothing to do with
#PERST control.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Krzysztof Kozlowski]

On 07/10/2024 08:58, Damien Le Moal wrote:
> On 10/7/24 15:54, Krzysztof Kozlowski wrote:
>> On 07/10/2024 08:50, Damien Le Moal wrote:
>>> On 10/7/24 15:12, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 07, 2024 at 01:12:17PM +0900, Damien Le Moal wrote:
>>>>> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>>>>>
>>>>> Describe the `ep-gpios` property which is used to map the PERST# input
>>>>> signal for endpoint mode.
>>>>
>>>> Why "ep" for PERST signal? Looks totally unrelated name. There is
>>>> already reset-gpios exactly for PERST, so you are duplicating it. Why?
>>>
>>> Because the host side controller already has the same "ep-gpios" property.
>>>
>>> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
>>
>> If host has it, then it is a common property so goes to common schema
>> for these devices.
> 
> Ah. OK. I will move it to
> Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-common.yaml then.
> 
>>> So naming that property the same allows common code to initialize that gpio in
>>> rockchip_pcie_parse_dt().
>>>
>>> Also, I do not see reset-gpios being defined/used by this driver (host and ep
>>> sides).
>>
>> I am talking about bindings, not driver.
> 
> I do not see reset-gpios being defined in the bindings (common, host and ep).
> resets and reset-names are defined though but these have nothing to do with
> #PERST control.

Bindings for all PCI devices. See pci-bus-common.yaml

Best regards,
Krzysztof

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Damien Le Moal]

On 10/7/24 16:00, Krzysztof Kozlowski wrote:
>> I do not see reset-gpios being defined in the bindings (common, host and ep).
>> resets and reset-names are defined though but these have nothing to do with
>> #PERST control.
> 
> Bindings for all PCI devices. See pci-bus-common.yaml

Got it. But in this case, since ep-gpios is already defined for the RC host mode
controller, isn't it simpler to simply move that property to
rockchip,rk3399-pcie-common.yaml ?

I can of course instead re-use the reset-gpios property for the endpoint mode,
but that will need a bit more code in the driver.

Which way do you recommend ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 11/12] dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 04:22:17PM +0900, Damien Le Moal wrote:
> On 10/7/24 16:00, Krzysztof Kozlowski wrote:
> >> I do not see reset-gpios being defined in the bindings (common, host and ep).
> >> resets and reset-names are defined though but these have nothing to do with
> >> #PERST control.
> > 
> > Bindings for all PCI devices. See pci-bus-common.yaml
> 
> Got it. But in this case, since ep-gpios is already defined for the RC host mode
> controller, isn't it simpler to simply move that property to
> rockchip,rk3399-pcie-common.yaml ?
> 
> I can of course instead re-use the reset-gpios property for the endpoint mode,
> but that will need a bit more code in the driver.
> 
> Which way do you recommend ?
> 

Please use 'reset-gpios' instead. Using 'ep-gpios' for the endpoint controller
doesn't convey the actual use.

- Mani

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

[PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Damien Le Moal]

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

Modify the endpoint rockchip driver to get the ep_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#.

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

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index af50432525b4..c70a64c37a56 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -18,6 +18,7 @@
 #include <linux/sizes.h>
 #include <linux/workqueue.h>
 #include <linux/iopoll.h>
+#include <linux/gpio/consumer.h>
 
 #include "pcie-rockchip.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;
 };
 
@@ -462,13 +466,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
 
 	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
 
+	if (rockchip->ep_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->ep_gpio)
+		schedule_delayed_work(&ep->link_training, 0);
 
 	return 0;
 }
@@ -478,6 +486,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->ep_gpio) {
+		ep->perst_asserted = true;
+		disable_irq(ep->perst_irq);
+	}
+
 	cancel_delayed_work_sync(&ep->link_training);
 
 	/* Stop link training and disable configuration */
@@ -540,6 +553,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 (Negociated speed: %sGT/s, width: x%lu)\n",
@@ -549,6 +569,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;
 
@@ -556,6 +577,94 @@ 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;
+	struct device *dev = rockchip->dev;
+
+	dev_dbg(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;
+	struct device *dev = rockchip->dev;
+
+	dev_dbg(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->ep_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->ep_gpio)
+		return 0;
+
+	/* PCIe reset interrupt */
+	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
+	if (ep->perst_irq < 0) {
+		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
+		return ep->perst_irq;
+	}
+
+	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,
@@ -719,6 +828,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
 	rockchip->is_rc = false;
 	rockchip->dev = dev;
 	INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training);
+	ep->link_up = false;
 
 	epc = devm_pci_epc_create(dev, &rockchip_pcie_epc_ops);
 	if (IS_ERR(epc)) {
@@ -751,7 +861,13 @@ 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_uninit_port:
+	rockchip_pcie_deinit_phys(rockchip);
 err_release_resources:
 	rockchip_pcie_ep_release_resources(ep);
 err_disable_clocks:
diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 154e78819e6e..bcb1c9266c56 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -119,13 +119,11 @@ 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");
-	}
+	rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep",
+				rockchip->is_rc ? GPIOD_OUT_LOW : GPIOD_IN);
+	if (IS_ERR(rockchip->ep_gpio))
+		return dev_err_probe(dev, PTR_ERR(rockchip->ep_gpio),
+				     "failed to get ep GPIO\n");
 
 	rockchip->aclk_pcie = devm_clk_get(dev, "aclk");
 	if (IS_ERR(rockchip->aclk_pcie)) {
-- 
2.46.2

Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[kernel test robot]

Hi Damien,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus mani-mhi/mhi-next linus/master v6.12-rc2 next-20241009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damien-Le-Moal/PCI-rockchip-ep-Use-a-macro-to-define-EP-controller-align-feature/20241007-131224
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241007041218.157516-13-dlemoal%40kernel.org
patch subject: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20241010/202410101236.xqI8ZWNd-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241010/202410101236.xqI8ZWNd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410101236.xqI8ZWNd-lkp@intel.com/

All errors (new ones prefixed by >>):

         |                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:255:44: error: invalid use of undefined type 'struct pci_epc_map'
     255 |         map->map_ofst = map->pci_addr - map->map_pci_addr;
         |                                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:257:16: error: invalid use of undefined type 'struct pci_epc_map'
     257 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |                ^~
   drivers/pci/controller/pcie-rockchip-ep.c:257:32: error: invalid use of undefined type 'struct pci_epc_map'
     257 |         if (map->map_ofst + map->pci_size > SZ_1M)
         |                                ^~
   drivers/pci/controller/pcie-rockchip-ep.c:258:20: error: invalid use of undefined type 'struct pci_epc_map'
     258 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                    ^~
   drivers/pci/controller/pcie-rockchip-ep.c:258:44: error: invalid use of undefined type 'struct pci_epc_map'
     258 |                 map->pci_size = SZ_1M - map->map_ofst;
         |                                            ^~
   drivers/pci/controller/pcie-rockchip-ep.c:260:12: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |            ^~
   In file included from include/vdso/const.h:5,
                    from include/linux/const.h:4,
                    from include/uapi/linux/kernel.h:6,
                    from include/linux/cache.h:5,
                    from include/linux/time.h:5,
                    from include/linux/stat.h:19,
                    from include/linux/configfs.h:22,
                    from drivers/pci/controller/pcie-rockchip-ep.c:11:
   drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:44: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                            ^
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                  ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:50: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                  ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:34: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                  ^~
   include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                             ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:50: error: invalid use of undefined type 'struct pci_epc_map'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                                                  ^~
   include/uapi/linux/const.h:49:61: note: in definition of macro '__ALIGN_KERNEL_MASK'
      49 | #define __ALIGN_KERNEL_MASK(x, mask)    (((x) + (mask)) & ~(mask))
         |                                                             ^~~~
   include/linux/align.h:8:33: note: in expansion of macro '__ALIGN_KERNEL'
       8 | #define ALIGN(x, a)             __ALIGN_KERNEL((x), (a))
         |                                 ^~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:260:25: note: in expansion of macro 'ALIGN'
     260 |         map->map_size = ALIGN(map->map_ofst + map->pci_size,
         |                         ^~~~~
   drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_perst_irq_thread':
>> drivers/pci/controller/pcie-rockchip-ep.c:631:9: error: implicit declaration of function 'irq_set_irq_type'; did you mean 'irq_set_irq_wake'? [-Wimplicit-function-declaration]
     631 |         irq_set_irq_type(ep->perst_irq,
         |         ^~~~~~~~~~~~~~~~
         |         irq_set_irq_wake
   drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_setup_irq':
>> drivers/pci/controller/pcie-rockchip-ep.c:655:9: error: implicit declaration of function 'irq_set_status_flags' [-Wimplicit-function-declaration]
     655 |         irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
         |         ^~~~~~~~~~~~~~~~~~~~
>> drivers/pci/controller/pcie-rockchip-ep.c:655:45: error: 'IRQ_NOAUTOEN' undeclared (first use in this function); did you mean 'IRQF_NO_AUTOEN'?
     655 |         irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
         |                                             ^~~~~~~~~~~~
         |                                             IRQF_NO_AUTOEN
   drivers/pci/controller/pcie-rockchip-ep.c:655:45: note: each undeclared identifier is reported only once for each function it appears in
   drivers/pci/controller/pcie-rockchip-ep.c: At top level:
   drivers/pci/controller/pcie-rockchip-ep.c:685:10: error: 'const struct pci_epc_ops' has no member named 'map_align'
     685 |         .map_align      = rockchip_pcie_ep_map_align,
         |          ^~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:685:27: error: initialization of 'int (*)(struct pci_epc *, u8,  u8,  phys_addr_t,  u64,  size_t)' {aka 'int (*)(struct pci_epc *, unsigned char,  unsigned char,  long long unsigned int,  long long unsigned int,  long unsigned int)'} from incompatible pointer type 'int (*)(struct pci_epc *, u8,  u8,  struct pci_epc_map *)' {aka 'int (*)(struct pci_epc *, unsigned char,  unsigned char,  struct pci_epc_map *)'} [-Wincompatible-pointer-types]
     685 |         .map_align      = rockchip_pcie_ep_map_align,
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:685:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')
   drivers/pci/controller/pcie-rockchip-ep.c:686:27: warning: initialized field overwritten [-Woverride-init]
     686 |         .map_addr       = rockchip_pcie_ep_map_addr,
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-rockchip-ep.c:686:27: note: (near initialization for 'rockchip_pcie_epc_ops.map_addr')


vim +631 drivers/pci/controller/pcie-rockchip-ep.c

   618	
   619	static irqreturn_t rockchip_pcie_ep_perst_irq_thread(int irq, void *data)
   620	{
   621		struct pci_epc *epc = data;
   622		struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
   623		struct rockchip_pcie *rockchip = &ep->rockchip;
   624		u32 perst = gpiod_get_value(rockchip->ep_gpio);
   625	
   626		if (perst)
   627			rockchip_pcie_ep_perst_assert(ep);
   628		else
   629			rockchip_pcie_ep_perst_deassert(ep);
   630	
 > 631		irq_set_irq_type(ep->perst_irq,
   632				 (perst ? IRQF_TRIGGER_HIGH : IRQF_TRIGGER_LOW));
   633	
   634		return IRQ_HANDLED;
   635	}
   636	
   637	static int rockchip_pcie_ep_setup_irq(struct pci_epc *epc)
   638	{
   639		struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
   640		struct rockchip_pcie *rockchip = &ep->rockchip;
   641		struct device *dev = rockchip->dev;
   642		int ret;
   643	
   644		if (!rockchip->ep_gpio)
   645			return 0;
   646	
   647		/* PCIe reset interrupt */
   648		ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
   649		if (ep->perst_irq < 0) {
   650			dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
   651			return ep->perst_irq;
   652		}
   653	
   654		ep->perst_asserted = true;
 > 655		irq_set_status_flags(ep->perst_irq, IRQ_NOAUTOEN);
   656		ret = devm_request_threaded_irq(dev, ep->perst_irq, NULL,
   657						rockchip_pcie_ep_perst_irq_thread,
   658						IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
   659						"pcie-ep-perst", epc);
   660		if (ret) {
   661			dev_err(dev, "Request PERST GPIO IRQ failed %d\n", ret);
   662			return ret;
   663		}
   664	
   665		return 0;
   666	}
   667	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Manivannan Sadhasivam]

On Mon, Oct 07, 2024 at 01:12:18PM +0900, Damien Le Moal wrote:
> Currently, the Rockchip PCIe endpoint controller driver does not handle
> PERST# signal, which prevents detecting when link training should
> actually be started or if the host reset the device. This however can
> be supported using the controller ep_gpio, set as an input GPIO for
> endpoint mode.
> 
> Modify the endpoint rockchip driver to get the ep_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#.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Just minor nits below. Overall LGTM.

> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 118 +++++++++++++++++++++-
>  drivers/pci/controller/pcie-rockchip.c    |  12 +--
>  2 files changed, 122 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index af50432525b4..c70a64c37a56 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -18,6 +18,7 @@
>  #include <linux/sizes.h>
>  #include <linux/workqueue.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio/consumer.h>

Please sort the includes.

>  
>  #include "pcie-rockchip.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;
>  };
>  
> @@ -462,13 +466,17 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
>  
>  	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
>  
> +	if (rockchip->ep_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->ep_gpio)
> +		schedule_delayed_work(&ep->link_training, 0);
>  
>  	return 0;
>  }
> @@ -478,6 +486,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->ep_gpio) {
> +		ep->perst_asserted = true;
> +		disable_irq(ep->perst_irq);
> +	}
> +
>  	cancel_delayed_work_sync(&ep->link_training);
>  
>  	/* Stop link training and disable configuration */
> @@ -540,6 +553,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 (Negociated speed: %sGT/s, width: x%lu)\n",
> @@ -549,6 +569,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;
>  
> @@ -556,6 +577,94 @@ 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;
> +	struct device *dev = rockchip->dev;
> +
> +	dev_dbg(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;
> +	struct device *dev = rockchip->dev;
> +
> +	dev_dbg(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);
> +

I hope that no registers are getting reset post PERST# assert.

> +	/* 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->ep_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->ep_gpio)
> +		return 0;
> +
> +	/* PCIe reset interrupt */
> +	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
> +	if (ep->perst_irq < 0) {
> +		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
> +		return ep->perst_irq;
> +	}
> +
> +	ep->perst_asserted = true;

How come?

> +	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,
> @@ -719,6 +828,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  	rockchip->is_rc = false;
>  	rockchip->dev = dev;
>  	INIT_DELAYED_WORK(&ep->link_training, rockchip_pcie_ep_link_training);
> +	ep->link_up = false;

'false' is the default state, isn't it?

- Mani

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

Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Damien Le Moal]

On 10/10/24 19:49, Manivannan Sadhasivam wrote:
>> +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->ep_gpio)
>> +		return 0;
>> +
>> +	/* PCIe reset interrupt */
>> +	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
>> +	if (ep->perst_irq < 0) {
>> +		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
>> +		return ep->perst_irq;
>> +	}
>> +
>> +	ep->perst_asserted = true;
> 
> How come?

Yeah, a bit confusing. This is because the gpio active low / inactive high, so
as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio
signal has not changed yet. So to be consistent with this, perst_asserted is
initialized to true so that on the first shot of
rockchip_pcie_ep_perst_irq_thread() we end up calling
rockchip_pcie_ep_perst_assert() as if we got an assert from the host (we have
not yet). I will add a comment clarifying that.



-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Manivannan Sadhasivam]

On Fri, Oct 11, 2024 at 06:30:31PM +0900, Damien Le Moal wrote:
> On 10/10/24 19:49, Manivannan Sadhasivam wrote:
> >> +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->ep_gpio)
> >> +		return 0;
> >> +
> >> +	/* PCIe reset interrupt */
> >> +	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
> >> +	if (ep->perst_irq < 0) {
> >> +		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
> >> +		return ep->perst_irq;
> >> +	}
> >> +
> >> +	ep->perst_asserted = true;
> > 
> > How come?
> 
> Yeah, a bit confusing. This is because the gpio active low / inactive high, so
> as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio
> signal has not changed yet.

Which means you are looking for a wrong level! What is the polarity of the
PERST# gpio in DT?

- Mani

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

Re: [PATCH v3 12/12] PCI: rockchip-ep: Handle PERST# signal in endpoint mode

[Damien Le Moal]

On 10/12/24 21:31, Manivannan Sadhasivam wrote:
> On Fri, Oct 11, 2024 at 06:30:31PM +0900, Damien Le Moal wrote:
>> On 10/10/24 19:49, Manivannan Sadhasivam wrote:
>>>> +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->ep_gpio)
>>>> +		return 0;
>>>> +
>>>> +	/* PCIe reset interrupt */
>>>> +	ep->perst_irq = gpiod_to_irq(rockchip->ep_gpio);
>>>> +	if (ep->perst_irq < 0) {
>>>> +		dev_err(dev, "No corresponding IRQ for PERST GPIO\n");
>>>> +		return ep->perst_irq;
>>>> +	}
>>>> +
>>>> +	ep->perst_asserted = true;
>>>
>>> How come?
>>
>> Yeah, a bit confusing. This is because the gpio active low / inactive high, so
>> as soon as we enable the IRQ, we are going to get one IRQ even though perst gpio
>> signal has not changed yet.
> 
> Which means you are looking for a wrong level! What is the polarity of the
> PERST# gpio in DT?

It is not defined in the default DT with the kernel. I added an overlay file to
define it together with the EP mode. And as I said above, the gpio is active
low. If I reverse that to active high, it does not work.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 00/12]

[Damien Le Moal]

On 10/7/24 13:12, Damien Le Moal wrote:
> This patch series is the second part of the former version 2 of the
> patch series "Improve PCI memory mapping API". This second part is split
> out as it deals solely with the rockchip/rk3399 PCI endpoint controller
> driver.

Missing series title. It should be "Improve rk3399 PCI endpoint controller driver"

Sorry about that.

> 
> 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 .map_align() operation to make the rk3399
>    endpoint controller driver fully functional
>  - Patch 6, 7 and 8 refactor the driver code to make it more readable
>  - Patch 9 introduces the .stop() endpoint controller operation to
>    correctly disable the endpopint controller after use
>  - Patch 10 improves link training
>  - Patch 11 and 12 implement handling of the #PERST signal
> 
> 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 (11):
>   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 .map_align() controller operation
>   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: Introduce rockchip_pcie_ep_stop()
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
> 
> Wilfred Mallawa (1):
>   dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property
> 
>  .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   4 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 392 +++++++++++++++---
>  drivers/pci/controller/pcie-rockchip.c        |  17 +-
>  drivers/pci/controller/pcie-rockchip.h        |  22 +
>  4 files changed, 358 insertions(+), 77 deletions(-)
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 00/12]

[Niklas Cassel]

On Mon, Oct 07, 2024 at 01:12:06PM +0900, Damien Le Moal wrote:
> This patch series is the second part of the former version 2 of the
> patch series "Improve PCI memory mapping API". This second part is split
> out as it deals solely with the rockchip/rk3399 PCI endpoint controller
> driver.

Since this series depends on:
[1] https://lore.kernel.org/linux-pci/20241007040319.157412-1-dlemoal@kernel.org/

I think that you should have added a link to that series in this cover letter,
and more clearly state that this series depends on [1].

(Right now, someone might think that this series could be applied standalone.)


Kind regards,
Niklas


> 
> 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 .map_align() operation to make the rk3399
>    endpoint controller driver fully functional
>  - Patch 6, 7 and 8 refactor the driver code to make it more readable
>  - Patch 9 introduces the .stop() endpoint controller operation to
>    correctly disable the endpopint controller after use
>  - Patch 10 improves link training
>  - Patch 11 and 12 implement handling of the #PERST signal
> 
> 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 (11):
>   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 .map_align() controller operation
>   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: Introduce rockchip_pcie_ep_stop()
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
> 
> Wilfred Mallawa (1):
>   dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property
> 
>  .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   4 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 392 +++++++++++++++---
>  drivers/pci/controller/pcie-rockchip.c        |  17 +-
>  drivers/pci/controller/pcie-rockchip.h        |  22 +
>  4 files changed, 358 insertions(+), 77 deletions(-)
> 
> -- 
> 2.46.2
> 

Re: [PATCH v3 00/12]

[Damien Le Moal]

On 10/7/24 19:02, Niklas Cassel wrote:
> On Mon, Oct 07, 2024 at 01:12:06PM +0900, Damien Le Moal wrote:
>> This patch series is the second part of the former version 2 of the
>> patch series "Improve PCI memory mapping API". This second part is split
>> out as it deals solely with the rockchip/rk3399 PCI endpoint controller
>> driver.
> 
> Since this series depends on:
> [1] https://lore.kernel.org/linux-pci/20241007040319.157412-1-dlemoal@kernel.org/
> 
> I think that you should have added a link to that series in this cover letter,
> and more clearly state that this series depends on [1].

Indeed. I botched this cover letter :)
Will fix it in v2.

-- 
Damien Le Moal
Western Digital Research