2023-04-04 08:26:43

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 00/11] PCI: rockchip: Fix RK3399 PCIe endpoint controller driver

This is a series of patches that fixes the PCIe endpoint controller driver
for the Rockchip RK3399 SoC. The driver was introduced in commit
cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
The original driver had issues and would not allow for the RK3399 to
operate in PCIe endpoint mode correctly. This patch series fixes that so
that the PCIe core controller of the RK3399 SoC can now act as a PCIe
endpoint. This is v3 of the patch series and addresses the concerns that
were raised during the review of the V2.

Thank you in advance for reviewing these changes and hopefully
getting this merged. Having a functional PCIe endpoint controller
driver for the RK3399 would allow to develop further PCIe endpoint
functions through the Linux PCIe endpoint framework using this SoC.

Summary of changes to V2 :

* Fix issue with memory mapping from PCIe space to physical space
There was a small mistake with the number of bits passed from the AXI
physical address to the PCIe space address.
* Disable the advertisement of MSI-X capabilities by the endpoint
According to the technical reference manual the controller cannot
generate MSI-X, so the controller should not advertise this capability.
* Add the alignment value to the endpoint attributes.
* [minor] Clean code (line length, variable names, small refactorings).
As pointed out by reviews on the V2.
* [minor] Fix error in variable name.
* [minor] Remove a patch that introduced unnecessary late parameter checks.

General problem statement and overview of the patch series :

Problem: The Rockchip RK3399 PCIe endpoint controller driver introduced in
commit cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe
controller") did not work.

Summary of problems with the driver :

* Missing dtsi entry
* Could not update Device ID (DID)
* The endpoint could not be configured by a host computer because the
endpoint kept sending Configuration Request Retry Status (CRS) messages
* The kernel would sometimes hang on probe due to access to registers in
a clock domain of which the PLLs were not locked
* The memory window mapping and address translation mechanism had
conflicting mappings and did not follow the technical reference manual
as to how the address translation should be done
* Legacy IRQs were not generated by the endpoint
* Message Signaled interrupts (MSI) were not generated by the endpoint
* MSI-X capabilities were advertised but the controller cannot generate
them according to the technical reference manual

The problems have been addressed and validated through tests (see below).

Summary of patches :

This patch series is composed of 11 patches that do the following :
* Remove writes to unused registers in the PCIe core register space.
The registers that were written to is marked "unused" and read
only in the technical reference manual of the RK3399 SoC.
* Write PCI Device ID (DID) to correct register, the DID was written to
a read only register and therefore would not update the DID.
* Assert PCI Configuration Enable bit after probe so that it would stop
sending Configuration Request Retry Status (CRS) messages to the
host once configured, without this the host would retry until
timeout and cancel the PCI configuration.
* Add poll and timeout to wait for PHY PLLs to be locked, this
is the only patch that also applies to the root complex function
of the PCIe core controller, without this the kernel would
sometimes access registers in the PHY PLL clock domain when the PLLs
were not yet locked and the system would hang. This was hackily solved
in other non mainline patches (e.g., in armbian) with a "msleep()"
that was added after PHY PLL configuration but without realizing
why it was needed. A poll with timeout seems like a sane approach.
* Add dtsi entry for RK3399 PCIe endpoint core. The new entry is
in "disabled" status by default, so unless it is explicitly enabled
it will not conflict with the PCIe root complex controller entry.
Developers that will enable it would know that the root complex function
then must be disabled, this can be done in the board level DTS.
* Update the RK3399 example in the documentation to a valid one.
* Fix legacy IRQ generation for RK3399 PCIe endpoint core, the legacy IRQs
were not sent by the device because their generation did not follow the
instructions in the technical reference manual. They now work.
* Fix window mapping and address translation for endpoint. The window
mapping and address translation did not follow the technical reference
manual and a single memory region was used which resulted in conflicting
address translations for memory allocated in that region. The current
patch allows to allocate up to 32 memory windows with 1MB pages.
* Use u32 variable to access 32-bit registers, u16 variables were used to
access and manipulate data of 32-bit registers, this would lead to
overflows e.g., when left shifting more than 16 bits.
* Don't advertise MSI-X in PCIe capabilities because according to the TRM
the controller is not capable of generating them.
* Set address alignment for the endpoint mode.

Validation on real hardware:

This patch series has been tested by me with kernels 6.0.19, 6.1.21,
and 5.19 on real hardware, a FriendlyElec NanoPC-T4 RK3399 based single
computer board connected to a host computer through PCIe x1 and x4. The
driver was also tested by Damien Le Moal <[email protected]>
on a Pine Rockpro64 board [1].

[1] https://lore.kernel.org/linux-pci/[email protected]/

The PCIe endpoint test function driver was loaded on the SoC and the PCIe
endpoint test driver was loaded on the host computer. The following tests were
executed through this setup :

* enumeration of the PCIe endpoint device (lspci)
lspci -vvv
* validation of PCI header and capabilities
setpci and lspci -xxxx
* device was recognized by host computer dans PCIe endpoint test driver
was loaded
lspci -v states "Kernel modules: pci_endpoint_test"
* tested the BARs 0-5
sudo /usr/bin/pcitest -b 0
...
sudo /usr/bin/pcitest -b 5
* tested legacy interrupt through the test driver
sudo /usr/bin/pcitest -i 0
sudo /usr/bin/pcitest -l
* tested MSI interrupt through the test driver
sudo /usr/bin/pcitest -i 1
sudo /usr/bin/pcitest -m 1
* tested read/write to and from host through the test driver with checksum
sudo /usr/bin/pcitest -r -s 1024
sudo /usr/bin/pcitest -w -s 1024
* tested read/write with DMA enabled (all read/write tests also did IRQ)
sudo /usr/bin/pcitest -r -d -s 8192
sudo /usr/bin/pcitest -w -d -s 8192
* tested larged transfers e.g., 100kB with and without DMA

Commands used on the SoC to launch the endpoint function (configfs) :

modprobe -i pci-epf-test
mkdir -p /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0
echo 0xb500 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/deviceid
echo 0x104c > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/vendorid
echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/msi_interrupts
ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0 \
/sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/
echo 1 > /sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/start

Note: to enable the endpoint controller on the board the file :
arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
Was edited to set the status of &pcie0 to "disabled" and &pcie0_ep
to "okay". This is not submitted as a patch because most users
will use the PCIe core controller in host (root complex) mode
rather than endpoint mode.

I have tested and confirmed all functionality required for the
endpoint with the test driver and tools. With the initial driver commit
cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
the device would not even be enumerated by the host computer (mainly because
of CRS messages being sent back to the root complex) and tests would not pass
(driver would not even be loaded because DID was not set correctly)
and then only the BAR test would pass. Now all tests pass as stated above.

Best regards
Rick

Damien Le Moal (1):
PCI: rockchip: Set address alignment for endpoint mode

Rick Wertenbroek (10):
PCI: rockchip: Remove writes to unused registers
PCI: rockchip: Write PCI Device ID to correct register
PCI: rockchip: Assert PCI Configuration Enable bit after probe
PCI: rockchip: Add poll and timeout to wait for PHY PLLs to be locked
arm64: dts: rockchip: Add dtsi entry for RK3399 PCIe endpoint core
dt-bindings: PCI: Update the RK3399 example to a valid one
PCI: rockchip: Fix legacy IRQ generation for RK3399 PCIe endpoint core
PCI: rockchip: Fix window mapping and address translation for endpoint
PCI: rockchip: Use u32 variable to access 32-bit registers
PCI: rockchip: Don't advertise MSI-X in PCIe capabilities

.../bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +-
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 27 +++
drivers/pci/controller/pcie-rockchip-ep.c | 193 ++++++++----------
drivers/pci/controller/pcie-rockchip.c | 17 ++
drivers/pci/controller/pcie-rockchip.h | 44 ++--
5 files changed, 162 insertions(+), 127 deletions(-)

--
2.25.1


2023-04-04 08:26:58

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 04/11] PCI: rockchip: Add poll and timeout to wait for PHY PLLs to be locked

The RK3399 PCIe controller should wait until the PHY PLLs are locked.
Add poll and timeout to wait for PHY PLLs to be locked. If they cannot
be locked generate error message and jump to error handler. Accessing
registers in the PHY clock domain when PLLs are not locked causes hang
The PHY PLLs status is checked through a side channel register.
This is documented in the TRM section 17.5.8.1 "PCIe Initialization
Sequence".

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
Tested-by: Damien Le Moal <[email protected]>
---
drivers/pci/controller/pcie-rockchip.c | 17 +++++++++++++++++
drivers/pci/controller/pcie-rockchip.h | 2 ++
2 files changed, 19 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 990a00e08bc5..1aa84035a8bc 100644
--- a/drivers/pci/controller/pcie-rockchip.c
+++ b/drivers/pci/controller/pcie-rockchip.c
@@ -14,6 +14,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
+#include <linux/iopoll.h>
#include <linux/of_pci.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
@@ -153,6 +154,12 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
}
EXPORT_SYMBOL_GPL(rockchip_pcie_parse_dt);

+#define rockchip_pcie_read_addr(addr) rockchip_pcie_read(rockchip, addr)
+/* 100 ms max wait time for PHY PLLs to lock */
+#define RK_PHY_PLL_LOCK_TIMEOUT_US 100000
+/* Sleep should be less than 20ms */
+#define RK_PHY_PLL_LOCK_SLEEP_US 1000
+
int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
{
struct device *dev = rockchip->dev;
@@ -254,6 +261,16 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
}
}

+ err = readx_poll_timeout(rockchip_pcie_read_addr,
+ PCIE_CLIENT_SIDE_BAND_STATUS,
+ regs, !(regs & PCIE_CLIENT_PHY_ST),
+ RK_PHY_PLL_LOCK_SLEEP_US,
+ RK_PHY_PLL_LOCK_TIMEOUT_US);
+ if (err) {
+ dev_err(dev, "PHY PLLs could not lock, %d\n", err);
+ goto err_power_off_phy;
+ }
+
/*
* Please don't reorder the deassert sequence of the following
* four reset pins.
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 51a123e5c0cf..f3a5ff1cf7f4 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -38,6 +38,8 @@
#define PCIE_CLIENT_MODE_EP HIWORD_UPDATE(0x0040, 0)
#define PCIE_CLIENT_GEN_SEL_1 HIWORD_UPDATE(0x0080, 0)
#define PCIE_CLIENT_GEN_SEL_2 HIWORD_UPDATE_BIT(0x0080)
+#define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20)
+#define PCIE_CLIENT_PHY_ST BIT(12)
#define PCIE_CLIENT_DEBUG_OUT_0 (PCIE_CLIENT_BASE + 0x3c)
#define PCIE_CLIENT_DEBUG_LTSSM_MASK GENMASK(5, 0)
#define PCIE_CLIENT_DEBUG_LTSSM_L1 0x18
--
2.25.1

2023-04-04 08:26:59

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 05/11] arm64: dts: rockchip: Add dtsi entry for RK3399 PCIe endpoint core

Add dtsi entry for RK3399 PCIe endpoint core in the device tree.
The status is "disabled" by default, so it will not be loaded unless
explicitly chosen to. The RK3399 PCIe endpoit core should be enabled
with the RK3399 PCIe root complex disabled because the RK3399 PCIe
controller can only work one mode at the time, either in "root complex"
mode or in "endpoint" mode.

Signed-off-by: Rick Wertenbroek <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 27 ++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 928948e7c7bb..c16c6176cffc 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -265,6 +265,33 @@ pcie0_intc: interrupt-controller {
};
};

+ pcie0_ep: pcie-ep@f8000000 {
+ compatible = "rockchip,rk3399-pcie-ep";
+ rockchip,max-outbound-regions = <32>;
+ clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
+ <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
+ clock-names = "aclk", "aclk-perf",
+ "hclk", "pm";
+ max-functions = /bits/ 8 <8>;
+ num-lanes = <4>;
+ reg = <0x0 0xfd000000 0x0 0x1000000>,
+ <0x0 0xfa000000 0x0 0x2000000>;
+ reg-names = "apb-base", "mem-base";
+ resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
+ <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>,
+ <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>,
+ <&cru SRST_A_PCIE>;
+ reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
+ "pm", "pclk", "aclk";
+ phys = <&pcie_phy 0>, <&pcie_phy 1>,
+ <&pcie_phy 2>, <&pcie_phy 3>;
+ phy-names = "pcie-phy-0", "pcie-phy-1",
+ "pcie-phy-2", "pcie-phy-3";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie_clkreqnb_cpm>;
+ status = "disabled";
+ };
+
gmac: ethernet@fe300000 {
compatible = "rockchip,rk3399-gmac";
reg = <0x0 0xfe300000 0x0 0x10000>;
--
2.25.1

2023-04-04 08:27:17

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 02/11] PCI: rockchip: Write PCI Device ID to correct register

Write PCI Device ID (DID) to the correct register. The Device ID was not
updated through the correct register. Device ID was written to a read-only
register and therefore did not work. The Device ID is now set through the
correct register. This is documented in the RK3399 TRM section 17.6.6.1.1

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
Tested-by: Damien Le Moal <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 6 ++++--
drivers/pci/controller/pcie-rockchip.h | 2 ++
2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index d5c477020417..9b835377bd9e 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -115,6 +115,7 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
struct pci_epf_header *hdr)
{
+ u32 reg;
struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
struct rockchip_pcie *rockchip = &ep->rockchip;

@@ -127,8 +128,9 @@ static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
PCIE_CORE_CONFIG_VENDOR);
}

- rockchip_pcie_write(rockchip, hdr->deviceid << 16,
- ROCKCHIP_PCIE_EP_FUNC_BASE(fn) + PCI_VENDOR_ID);
+ reg = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_DID_VID);
+ reg = (reg & 0xFFFF) | (hdr->deviceid << 16);
+ rockchip_pcie_write(rockchip, reg, PCIE_EP_CONFIG_DID_VID);

rockchip_pcie_write(rockchip,
hdr->revid |
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 32c3a859c26b..51a123e5c0cf 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -133,6 +133,8 @@
#define PCIE_RC_RP_ATS_BASE 0x400000
#define PCIE_RC_CONFIG_NORMAL_BASE 0x800000
#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_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
--
2.25.1

2023-04-04 08:27:21

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 08/11] PCI: rockchip: Fix window mapping and address translation for endpoint

The RK3399 PCI endpoint core has 33 windows for PCIe space, now in the
driver up to 32 fixed size (1M) windows are used and pages are allocated
and mapped accordingly. The driver first used a single window and allocated
space inside which caused translation issues (between CPU space and PCI
space) because a window can only have a single translation at a given
time, which if multiple pages are allocated inside will cause conflicts.
Now each window is a single region of 1M which will always guarantee that
the translation is not in conflict.

Set the translation register addresses for physical function. As documented
in the technical reference manual (TRM) section 17.5.5 "PCIe Address
Translation" and section 17.6.8 "Address Translation Registers Description"

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 110 +++++++++-------------
drivers/pci/controller/pcie-rockchip.h | 30 +++---
2 files changed, 64 insertions(+), 76 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 7591a7be78e0..f366846ad77c 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -64,52 +64,30 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
}

static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
- u32 r, u32 type, u64 cpu_addr,
- u64 pci_addr, size_t size)
+ u32 r, u64 cpu_addr, u64 pci_addr,
+ size_t size)
{
u64 sz = 1ULL << fls64(size - 1);
int num_pass_bits = ilog2(sz);
- u32 addr0, addr1, desc0, desc1;
- bool is_nor_msg = (type == AXI_WRAPPER_NOR_MSG);
+ u32 addr0, addr1, desc0;

- /* The minimal region size is 1MB */
if (num_pass_bits < 8)
num_pass_bits = 8;

- cpu_addr -= rockchip->mem_res->start;
- addr0 = ((is_nor_msg ? 0x10 : (num_pass_bits - 1)) &
- PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
- (lower_32_bits(cpu_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
- addr1 = upper_32_bits(is_nor_msg ? cpu_addr : pci_addr);
- desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | type;
- desc1 = 0;
-
- if (is_nor_msg) {
- rockchip_pcie_write(rockchip, 0,
- ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
- rockchip_pcie_write(rockchip, 0,
- ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
- rockchip_pcie_write(rockchip, desc0,
- ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
- rockchip_pcie_write(rockchip, desc1,
- ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
- } else {
- /* PCI bus address region */
- rockchip_pcie_write(rockchip, addr0,
- ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
- rockchip_pcie_write(rockchip, addr1,
- ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
- rockchip_pcie_write(rockchip, desc0,
- ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
- rockchip_pcie_write(rockchip, desc1,
- ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
-
- addr0 =
- ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
- (lower_32_bits(cpu_addr) &
- PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
- addr1 = upper_32_bits(cpu_addr);
- }
+ 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);
+ desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | AXI_WRAPPER_MEM_WRITE;
+
+ /* PCI bus address region */
+ rockchip_pcie_write(rockchip, addr0,
+ ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
+ rockchip_pcie_write(rockchip, addr1,
+ ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
+ rockchip_pcie_write(rockchip, desc0,
+ ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
+ rockchip_pcie_write(rockchip, 0,
+ ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
}

static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
@@ -248,6 +226,11 @@ static void rockchip_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn, u8 vfn,
ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar));
}

+static inline u32 rockchip_ob_region(phys_addr_t addr)
+{
+ return (addr >> ilog2(SZ_1M)) & 0x1f;
+}
+
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)
@@ -256,18 +239,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
struct rockchip_pcie *pcie = &ep->rockchip;
u32 r;

- r = find_first_zero_bit(&ep->ob_region_map, BITS_PER_LONG);
- /*
- * Region 0 is reserved for configuration space and shouldn't
- * be used elsewhere per TRM, so leave it out.
- */
- if (r >= ep->max_regions - 1) {
- dev_err(&epc->dev, "no free outbound region\n");
- return -EINVAL;
- }
+ r = rockchip_ob_region(addr);

- rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, AXI_WRAPPER_MEM_WRITE, addr,
- pci_addr, size);
+ rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size);

set_bit(r, &ep->ob_region_map);
ep->ob_addr[r] = addr;
@@ -282,15 +256,11 @@ static void rockchip_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
struct rockchip_pcie *rockchip = &ep->rockchip;
u32 r;

- for (r = 0; r < ep->max_regions - 1; r++)
+ for (r = 0; r < ep->max_regions; r++)
if (ep->ob_addr[r] == addr)
break;

- /*
- * Region 0 is reserved for configuration space and shouldn't
- * be used elsewhere per TRM, so leave it out.
- */
- if (r == ep->max_regions - 1)
+ if (r == ep->max_regions)
return;

rockchip_pcie_clear_ep_ob_atu(rockchip, r);
@@ -388,6 +358,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
u16 flags, mme, data, data_mask;
u8 msi_count;
u64 pci_addr, pci_addr_mask = 0xff;
+ u32 r;

/* Check MSI enable bit */
flags = rockchip_pcie_read(&ep->rockchip,
@@ -421,13 +392,12 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
PCI_MSI_ADDRESS_LO);
- pci_addr &= GENMASK_ULL(63, 2);

/* Set the outbound region if needed. */
if (unlikely(ep->irq_pci_addr != (pci_addr & ~pci_addr_mask) ||
ep->irq_pci_fn != fn)) {
- rockchip_pcie_prog_ep_ob_atu(rockchip, fn, ep->max_regions - 1,
- AXI_WRAPPER_MEM_WRITE,
+ r = rockchip_ob_region(ep->irq_phys_addr);
+ rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
ep->irq_phys_addr,
pci_addr & ~pci_addr_mask,
pci_addr_mask + 1);
@@ -516,6 +486,8 @@ static int rockchip_pcie_parse_ep_dt(struct rockchip_pcie *rockchip,
if (err < 0 || ep->max_regions > MAX_REGION_LIMIT)
ep->max_regions = MAX_REGION_LIMIT;

+ ep->ob_region_map = 0;
+
err = of_property_read_u8(dev->of_node, "max-functions",
&ep->epc->max_functions);
if (err < 0)
@@ -536,7 +508,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
struct rockchip_pcie *rockchip;
struct pci_epc *epc;
size_t max_regions;
- int err;
+ struct pci_epc_mem_window *windows = NULL;
+ int err, i;

ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
if (!ep)
@@ -583,15 +556,26 @@ 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);

- err = pci_epc_mem_init(epc, rockchip->mem_res->start,
- resource_size(rockchip->mem_res), PAGE_SIZE);
+ 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_128K);
+ SZ_1M);
if (!ep->irq_cpu_addr) {
dev_err(dev, "failed to reserve memory space for MSI\n");
err = -ENOMEM;
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index ffc68a3a5fee..5797ba73bb6b 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -139,6 +139,7 @@

#define PCIE_RC_RP_ATS_BASE 0x400000
#define PCIE_RC_CONFIG_NORMAL_BASE 0x800000
+#define PCIE_EP_PF_CONFIG_REGS_BASE 0x800000
#define PCIE_RC_CONFIG_BASE 0xa00000
#define PCIE_EP_CONFIG_BASE 0xa00000
#define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)
@@ -232,13 +233,15 @@
#define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
#define ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR 0x1
-#define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
+#define ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR 0x3
+#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) \
+ (PCIE_EP_PF_CONFIG_REGS_BASE + 0x10000 + (((fn) << 12) & GENMASK(19, 12)))
#define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
- (PCIE_RC_RP_ATS_BASE + 0x0840 + (fn) * 0x0040 + (bar) * 0x0008)
+ (PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
#define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \
- (PCIE_RC_RP_ATS_BASE + 0x0844 + (fn) * 0x0040 + (bar) * 0x0008)
-#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
- (PCIE_RC_RP_ATS_BASE + 0x0000 + ((r) & 0x1f) * 0x0020)
+ (PCIE_CORE_AXI_CONF_BASE + 0x082c + (fn) * 0x0040 + (bar) * 0x0008)
#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN_MASK GENMASK(19, 12)
#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) \
(((devfn) << 12) & \
@@ -246,20 +249,21 @@
#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK GENMASK(27, 20)
#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(bus) \
(((bus) << 20) & ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK)
+#define PCIE_RC_EP_ATR_OB_REGIONS_1_32 (PCIE_CORE_AXI_CONF_BASE + 0x0020)
+#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0000 + ((r) & 0x1f) * 0x0020)
#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r) \
- (PCIE_RC_RP_ATS_BASE + 0x0004 + ((r) & 0x1f) * 0x0020)
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0004 + ((r) & 0x1f) * 0x0020)
#define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID BIT(23)
#define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK GENMASK(31, 24)
#define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(devfn) \
(((devfn) << 24) & ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK)
#define ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r) \
- (PCIE_RC_RP_ATS_BASE + 0x0008 + ((r) & 0x1f) * 0x0020)
-#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
- (PCIE_RC_RP_ATS_BASE + 0x000c + ((r) & 0x1f) * 0x0020)
-#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r) \
- (PCIE_RC_RP_ATS_BASE + 0x0018 + ((r) & 0x1f) * 0x0020)
-#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r) \
- (PCIE_RC_RP_ATS_BASE + 0x001c + ((r) & 0x1f) * 0x0020)
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0008 + ((r) & 0x1f) * 0x0020)
+#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x000c + ((r) & 0x1f) * 0x0020)
+#define ROCKCHIP_PCIE_AT_OB_REGION_DESC2(r) \
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0010 + ((r) & 0x1f) * 0x0020)

#define ROCKCHIP_PCIE_CORE_EP_FUNC_BAR_CFG0(fn) \
(PCIE_CORE_CTRL_MGMT_BASE + 0x0240 + (fn) * 0x0008)
--
2.25.1

2023-04-04 08:27:26

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 09/11] PCI: rockchip: Use u32 variable to access 32-bit registers

Previously u16 variables were used to access 32-bit registers, this
resulted in not all of the data being read from the registers. Also
the left shift of more than 16-bits would result in moving data out
of the variable. Use u32 variables to access 32-bit registers

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 10 +++++-----
drivers/pci/controller/pcie-rockchip.h | 1 +
2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index f366846ad77c..924b95bd736c 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -274,15 +274,15 @@ static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn,
{
struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
struct rockchip_pcie *rockchip = &ep->rockchip;
- u16 flags;
+ u32 flags;

flags = rockchip_pcie_read(rockchip,
ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
ROCKCHIP_PCIE_EP_MSI_CTRL_REG);
flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK;
flags |=
- ((multi_msg_cap << 1) << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) |
- PCI_MSI_FLAGS_64BIT;
+ (multi_msg_cap << ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET) |
+ (PCI_MSI_FLAGS_64BIT << ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET);
flags &= ~ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP;
rockchip_pcie_write(rockchip, flags,
ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
@@ -294,7 +294,7 @@ static int rockchip_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
{
struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
struct rockchip_pcie *rockchip = &ep->rockchip;
- u16 flags;
+ u32 flags;

flags = rockchip_pcie_read(rockchip,
ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
@@ -355,7 +355,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
u8 interrupt_num)
{
struct rockchip_pcie *rockchip = &ep->rockchip;
- u16 flags, mme, data, data_mask;
+ u32 flags, mme, data, data_mask;
u8 msi_count;
u64 pci_addr, pci_addr_mask = 0xff;
u32 r;
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 5797ba73bb6b..1558eae298ae 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -226,6 +226,7 @@
#define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4
#define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90
+#define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_OFFSET 20
--
2.25.1

2023-04-04 08:27:47

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 10/11] PCI: rockchip: Don't advertise MSI-X in PCIe capabilities

The RK3399 PCIe endpoint controller cannot generate MSI-X IRQs.
This is documented in the RK3399 technical reference manual (TRM)
section 17.5.9 "Interrupt Support".

MSI-X capability should therefore not be advertised. Remove the
MSI-X capability by editing the capability linked-list. The
previous entry is the MSI capability, therefore get the next
entry from the MSI-X capability entry and set it as next entry
for the MSI capability. This in effect removes MSI-X from the list.

Linked list before : MSI cap -> MSI-X cap -> PCIe Device cap -> ...
Linked list now : MSI cap -> PCIe Device cap -> ...

Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 15 +++++++++++++++
drivers/pci/controller/pcie-rockchip.h | 5 +++++
2 files changed, 20 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 924b95bd736c..20c768287870 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -510,6 +510,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
size_t max_regions;
struct pci_epc_mem_window *windows = NULL;
int err, i;
+ u32 cfg_msi, cfg_msix_cp;

ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
if (!ep)
@@ -584,6 +585,20 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)

ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;

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

return 0;
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 1558eae298ae..a21070ea7166 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -226,6 +226,8 @@
#define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4
#define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90
+#define ROCKCHIP_PCIE_EP_MSI_CP1_OFFSET 8
+#define ROCKCHIP_PCIE_EP_MSI_CP1_MASK GENMASK(15, 8)
#define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17)
@@ -233,6 +235,9 @@
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_MASK GENMASK(22, 20)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
+#define ROCKCHIP_PCIE_EP_MSIX_CAP_REG 0xb0
+#define ROCKCHIP_PCIE_EP_MSIX_CAP_CP_OFFSET 8
+#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_EP_FUNC_BASE(fn) \
--
2.25.1

2023-04-04 08:28:01

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 11/11] PCI: rockchip: Set address alignment for endpoint mode

From: Damien Le Moal <[email protected]>

The address translation unit of the rockchip EP controller does not use
the lower 8 bits of a PCIe-space address to map local memory. Thus we
must set the align feature field to 256 to let the user know about this
constraint.

Signed-off-by: Damien Le Moal <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 20c768287870..b79382c1938a 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -445,6 +445,7 @@ static const struct pci_epc_features rockchip_pcie_epc_features = {
.linkup_notifier = false,
.msi_capable = true,
.msix_capable = false,
+ .align = 256,
};

static const struct pci_epc_features*
--
2.25.1

2023-04-04 08:28:05

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 03/11] PCI: rockchip: Assert PCI Configuration Enable bit after probe

Assert PCI Configuration Enable bit after probe. When this bit is left to
0 in the endpoint mode, the RK3399 PCIe endpoint core will generate
configuration request retry status (CRS) messages back to the root complex.
Assert this bit after probe to allow the RK3399 PCIe endpoint core to reply
to configuration requests from the root complex.
This is documented in section 17.5.8.1.2 of the RK3399 TRM.

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
Tested-by: Damien Le Moal <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 9b835377bd9e..4c84e403e155 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -623,6 +623,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)

ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;

+ rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_CONFIG);
+
return 0;
err_epc_mem_exit:
pci_epc_mem_exit(epc);
--
2.25.1

2023-04-04 08:28:10

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 06/11] dt-bindings: PCI: Update the RK3399 example to a valid one

Update the example in the documentation a valid example.
The default max-outbound-regions is 32 but the example showed 16.
Address for mem-base was invalid. Added pinctrl.

Signed-off-by: Rick Wertenbroek <[email protected]>
---
.../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
index 88386a6d7011..0c67e96096eb 100644
--- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
@@ -47,14 +47,15 @@ examples:

pcie-ep@f8000000 {
compatible = "rockchip,rk3399-pcie-ep";
- reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>;
- reg-names = "apb-base", "mem-base";
+ rockchip,max-outbound-regions = <32>;
clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
<&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
clock-names = "aclk", "aclk-perf",
"hclk", "pm";
max-functions = /bits/ 8 <8>;
num-lanes = <4>;
+ reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>;
+ reg-names = "apb-base", "mem-base";
resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
<&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> ,
<&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
@@ -62,7 +63,8 @@ examples:
"pm", "pclk", "aclk";
phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
- rockchip,max-outbound-regions = <16>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie_clkreqnb_cpm>;
};
};
...
--
2.25.1

2023-04-04 08:28:19

by Rick Wertenbroek

[permalink] [raw]
Subject: [PATCH v3 07/11] PCI: rockchip: Fix legacy IRQ generation for RK3399 PCIe endpoint core

Fix legacy IRQ generation for RK3399 PCIe endpoint core according to
the technical reference manual (TRM). Assert and deassert legacy
interrupt (INTx) through the legacy interrupt control register
("PCIE_CLIENT_LEGACY_INT_CTRL") instead of manually generating a PCIe
message. The generation of the legacy interrupt was tested and validated
with the PCIe endpoint test driver.

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
Cc: [email protected]
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 45 ++++++-----------------
drivers/pci/controller/pcie-rockchip.h | 6 ++-
2 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 4c84e403e155..7591a7be78e0 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -337,48 +337,25 @@ static int rockchip_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
}

static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn,
- u8 intx, bool is_asserted)
+ u8 intx, bool do_assert)
{
struct rockchip_pcie *rockchip = &ep->rockchip;
- u32 r = ep->max_regions - 1;
- u32 offset;
- u32 status;
- u8 msg_code;
-
- if (unlikely(ep->irq_pci_addr != ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR ||
- ep->irq_pci_fn != fn)) {
- rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
- AXI_WRAPPER_NOR_MSG,
- ep->irq_phys_addr, 0, 0);
- ep->irq_pci_addr = ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR;
- ep->irq_pci_fn = fn;
- }

intx &= 3;
- if (is_asserted) {
+
+ if (do_assert) {
ep->irq_pending |= BIT(intx);
- msg_code = ROCKCHIP_PCIE_MSG_CODE_ASSERT_INTA + intx;
+ rockchip_pcie_write(rockchip,
+ PCIE_CLIENT_INT_IN_ASSERT |
+ PCIE_CLIENT_INT_PEND_ST_PEND,
+ PCIE_CLIENT_LEGACY_INT_CTRL);
} else {
ep->irq_pending &= ~BIT(intx);
- msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx;
+ rockchip_pcie_write(rockchip,
+ PCIE_CLIENT_INT_IN_DEASSERT |
+ PCIE_CLIENT_INT_PEND_ST_NORMAL,
+ PCIE_CLIENT_LEGACY_INT_CTRL);
}
-
- status = rockchip_pcie_read(rockchip,
- ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
- ROCKCHIP_PCIE_EP_CMD_STATUS);
- status &= ROCKCHIP_PCIE_EP_CMD_STATUS_IS;
-
- if ((status != 0) ^ (ep->irq_pending != 0)) {
- status ^= ROCKCHIP_PCIE_EP_CMD_STATUS_IS;
- rockchip_pcie_write(rockchip, status,
- ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
- ROCKCHIP_PCIE_EP_CMD_STATUS);
- }
-
- offset =
- ROCKCHIP_PCIE_MSG_ROUTING(ROCKCHIP_PCIE_MSG_ROUTING_LOCAL_INTX) |
- ROCKCHIP_PCIE_MSG_CODE(msg_code) | ROCKCHIP_PCIE_MSG_NO_DATA;
- writel(0, ep->irq_cpu_addr + offset);
}

static int rockchip_pcie_ep_send_legacy_irq(struct rockchip_pcie_ep *ep, u8 fn,
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index f3a5ff1cf7f4..ffc68a3a5fee 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -38,6 +38,11 @@
#define PCIE_CLIENT_MODE_EP HIWORD_UPDATE(0x0040, 0)
#define PCIE_CLIENT_GEN_SEL_1 HIWORD_UPDATE(0x0080, 0)
#define PCIE_CLIENT_GEN_SEL_2 HIWORD_UPDATE_BIT(0x0080)
+#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c)
+#define PCIE_CLIENT_INT_IN_ASSERT HIWORD_UPDATE_BIT(0x0002)
+#define PCIE_CLIENT_INT_IN_DEASSERT HIWORD_UPDATE(0x0002, 0)
+#define PCIE_CLIENT_INT_PEND_ST_PEND HIWORD_UPDATE_BIT(0x0001)
+#define PCIE_CLIENT_INT_PEND_ST_NORMAL HIWORD_UPDATE(0x0001, 0)
#define PCIE_CLIENT_SIDE_BAND_STATUS (PCIE_CLIENT_BASE + 0x20)
#define PCIE_CLIENT_PHY_ST BIT(12)
#define PCIE_CLIENT_DEBUG_OUT_0 (PCIE_CLIENT_BASE + 0x3c)
@@ -227,7 +232,6 @@
#define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
#define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
#define ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR 0x1
-#define ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR 0x3
#define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
#define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
(PCIE_RC_RP_ATS_BASE + 0x0840 + (fn) * 0x0040 + (bar) * 0x0008)
--
2.25.1

2023-04-04 08:47:53

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 06/11] dt-bindings: PCI: Update the RK3399 example to a valid one

On 04/04/2023 10:24, Rick Wertenbroek wrote:
> Update the example in the documentation a valid example.
> The default max-outbound-regions is 32 but the example showed 16.

This is not reason to be invalid. It is perfectly fine to change default
values to desired ones. What is not actually obvious is to change some
value to a default one, instead of removing it...

> Address for mem-base was invalid. Added pinctrl.
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> index 88386a6d7011..0c67e96096eb 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> @@ -47,14 +47,15 @@ examples:
>
> pcie-ep@f8000000 {
> compatible = "rockchip,rk3399-pcie-ep";
> - reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>;
> - reg-names = "apb-base", "mem-base";

Reg (and reg-names) is usually second property, why moving it? What is
incorrect in the placement?

> + rockchip,max-outbound-regions = <32>;
> clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
> <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
> clock-names = "aclk", "aclk-perf",
> "hclk", "pm";
> max-functions = /bits/ 8 <8>;
> num-lanes = <4>;
> + reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>;
> + reg-names = "apb-base", "mem-base";
> resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
> <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> ,
> <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
> @@ -62,7 +63,8 @@ examples:
> "pm", "pclk", "aclk";
> phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
> - rockchip,max-outbound-regions = <16>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie_clkreqnb_cpm>;
> };
> };
> ...

Best regards,
Krzysztof

2023-04-04 09:01:43

by Rick Wertenbroek

[permalink] [raw]
Subject: Re: [PATCH v3 06/11] dt-bindings: PCI: Update the RK3399 example to a valid one

On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 04/04/2023 10:24, Rick Wertenbroek wrote:
> > Update the example in the documentation a valid example.
> > The default max-outbound-regions is 32 but the example showed 16.
>
> This is not reason to be invalid. It is perfectly fine to change default
> values to desired ones. What is not actually obvious is to change some
> value to a default one, instead of removing it...

Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong
and will crash the kernel. This is a value that point to an address that falls
in the DDR RAM region but depending on the amount of RAM on the
board this address may not even exist (e.g., board with 2GB or less).

Also this address requires pointing to where the PCIe controller has the
windows from AXI Physical space to PCIe space. This address is
allocated when the SoC address map is created so it can only be that
one unless rockchip refabs the SoC with another address map.

The example never worked with the values given as reported by e.g.,
https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint
and here they set it to 0 (base of the DDR, which is a "valid" address
as to it exists even on boards with less than 2GB) but it is still wrong
to do so.

>
> > Address for mem-base was invalid. Added pinctrl.
> >
> > Signed-off-by: Rick Wertenbroek <[email protected]>
> > ---
> > .../devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> > index 88386a6d7011..0c67e96096eb 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie-ep.yaml
> > @@ -47,14 +47,15 @@ examples:
> >
> > pcie-ep@f8000000 {
> > compatible = "rockchip,rk3399-pcie-ep";
> > - reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0x80000000 0x0 0x20000>;
> > - reg-names = "apb-base", "mem-base";
>
> Reg (and reg-names) is usually second property, why moving it? What is
> incorrect in the placement?

Sorry, I was not aware there was a standard ordering, the reason I moved
so that it follows the ordering I had in the entry I added to the .dtsi file
(which therefore also is in the non standard order).
Could you be kind enough to share with me the link to the documentation
for the order, so that I can both update the .dtsi and this file, this
way it will
be in order and coherent for both. Thank you.

>
> > + rockchip,max-outbound-regions = <32>;
> > clocks = <&cru ACLK_PCIE>, <&cru ACLK_PERF_PCIE>,
> > <&cru PCLK_PCIE>, <&cru SCLK_PCIE_PM>;
> > clock-names = "aclk", "aclk-perf",
> > "hclk", "pm";
> > max-functions = /bits/ 8 <8>;
> > num-lanes = <4>;
> > + reg = <0x0 0xfd000000 0x0 0x1000000>, <0x0 0xfa000000 0x0 0x2000000>;
> > + reg-names = "apb-base", "mem-base";
> > resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
> > <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> ,
> > <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
> > @@ -62,7 +63,8 @@ examples:
> > "pm", "pclk", "aclk";
> > phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> > phy-names = "pcie-phy-0", "pcie-phy-1", "pcie-phy-2", "pcie-phy-3";
> > - rockchip,max-outbound-regions = <16>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pcie_clkreqnb_cpm>;
> > };
> > };
> > ...
>
> Best regards,
> Krzysztof
>

Thank you for you comments,
Sincerely
Rick

2023-04-04 13:33:57

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v3 06/11] dt-bindings: PCI: Update the RK3399 example to a valid one

On 04/04/2023 10:58, Rick Wertenbroek wrote:
> On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 04/04/2023 10:24, Rick Wertenbroek wrote:
>>> Update the example in the documentation a valid example.
>>> The default max-outbound-regions is 32 but the example showed 16.
>>
>> This is not reason to be invalid. It is perfectly fine to change default
>> values to desired ones. What is not actually obvious is to change some
>> value to a default one, instead of removing it...
>
> Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong
> and will crash the kernel. This is a value that point to an address that falls
> in the DDR RAM region but depending on the amount of RAM on the
> board this address may not even exist (e.g., board with 2GB or less).

We talk about max-outbound-regions.

>
> Also this address requires pointing to where the PCIe controller has the
> windows from AXI Physical space to PCIe space. This address is
> allocated when the SoC address map is created so it can only be that
> one unless rockchip refabs the SoC with another address map.
>
> The example never worked with the values given as reported by e.g.,
> https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint
> and here they set it to 0 (base of the DDR, which is a "valid" address
> as to it exists even on boards with less than 2GB) but it is still wrong
> to do so.

Again, my comment was under max-outbound-regions, not under some other
pieces. Does this all apply?

Best regards,
Krzysztof

2023-04-04 14:51:57

by Rick Wertenbroek

[permalink] [raw]
Subject: Re: [PATCH v3 06/11] dt-bindings: PCI: Update the RK3399 example to a valid one

On Tue, Apr 4, 2023 at 3:29 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 04/04/2023 10:58, Rick Wertenbroek wrote:
> > On Tue, Apr 4, 2023 at 10:45 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 04/04/2023 10:24, Rick Wertenbroek wrote:
> >>> Update the example in the documentation a valid example.
> >>> The default max-outbound-regions is 32 but the example showed 16.
> >>
> >> This is not reason to be invalid. It is perfectly fine to change default
> >> values to desired ones. What is not actually obvious is to change some
> >> value to a default one, instead of removing it...
> >
> > Hello, the example value <0x0 0x80000000 0x0 0x20000>; is plain wrong
> > and will crash the kernel. This is a value that point to an address that falls
> > in the DDR RAM region but depending on the amount of RAM on the
> > board this address may not even exist (e.g., board with 2GB or less).
>
> We talk about max-outbound-regions.

Okay, sorry, I didn't get that, you are right, there is nothing wrong with 16.
I'll remove that change and leave it be.

>
> >
> > Also this address requires pointing to where the PCIe controller has the
> > windows from AXI Physical space to PCIe space. This address is
> > allocated when the SoC address map is created so it can only be that
> > one unless rockchip refabs the SoC with another address map.
> >
> > The example never worked with the values given as reported by e.g.,
> > https://stackoverflow.com/questions/73586703/device-tree-issues-with-rockpro64-pcie-endpoint
> > and here they set it to 0 (base of the DDR, which is a "valid" address
> > as to it exists even on boards with less than 2GB) but it is still wrong
> > to do so.
>
> Again, my comment was under max-outbound-regions, not under some other
> pieces. Does this all apply?
>
> Best regards,
> Krzysztof
>

I'll remove the change to the max-outbound-regions, it is not needed.
I'll place the registers as second parameter, both on the dtsi entry and here.
I'll keep the change to the register value because it is necessary along
with the added pinctrl.

This will simplify the patch, avoid unnecessary changes, and make
things clearer.

Sorry for the misunderstanding.
Regards,
Rick

2023-04-05 09:27:28

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH v3 00/11] PCI: rockchip: Fix RK3399 PCIe endpoint controller driver

On 4/4/23 17:24, Rick Wertenbroek wrote:
> This is a series of patches that fixes the PCIe endpoint controller driver
> for the Rockchip RK3399 SoC. The driver was introduced in commit
> cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> The original driver had issues and would not allow for the RK3399 to
> operate in PCIe endpoint mode correctly. This patch series fixes that so
> that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> endpoint. This is v3 of the patch series and addresses the concerns that
> were raised during the review of the V2.
>
> Thank you in advance for reviewing these changes and hopefully
> getting this merged. Having a functional PCIe endpoint controller
> driver for the RK3399 would allow to develop further PCIe endpoint
> functions through the Linux PCIe endpoint framework using this SoC.
>
> Summary of changes to V2 :
>
> * Fix issue with memory mapping from PCIe space to physical space
> There was a small mistake with the number of bits passed from the AXI
> physical address to the PCIe space address.
> * Disable the advertisement of MSI-X capabilities by the endpoint
> According to the technical reference manual the controller cannot
> generate MSI-X, so the controller should not advertise this capability.
> * Add the alignment value to the endpoint attributes.
> * [minor] Clean code (line length, variable names, small refactorings).
> As pointed out by reviews on the V2.
> * [minor] Fix error in variable name.
> * [minor] Remove a patch that introduced unnecessary late parameter checks.
>
> General problem statement and overview of the patch series :
>
> Problem: The Rockchip RK3399 PCIe endpoint controller driver introduced in
> commit cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe
> controller") did not work.
>
> Summary of problems with the driver :
>
> * Missing dtsi entry
> * Could not update Device ID (DID)
> * The endpoint could not be configured by a host computer because the
> endpoint kept sending Configuration Request Retry Status (CRS) messages
> * The kernel would sometimes hang on probe due to access to registers in
> a clock domain of which the PLLs were not locked
> * The memory window mapping and address translation mechanism had
> conflicting mappings and did not follow the technical reference manual
> as to how the address translation should be done
> * Legacy IRQs were not generated by the endpoint
> * Message Signaled interrupts (MSI) were not generated by the endpoint
> * MSI-X capabilities were advertised but the controller cannot generate
> them according to the technical reference manual
>
> The problems have been addressed and validated through tests (see below).
>
> Summary of patches :
>
> This patch series is composed of 11 patches that do the following :
> * Remove writes to unused registers in the PCIe core register space.
> The registers that were written to is marked "unused" and read
> only in the technical reference manual of the RK3399 SoC.
> * Write PCI Device ID (DID) to correct register, the DID was written to
> a read only register and therefore would not update the DID.
> * Assert PCI Configuration Enable bit after probe so that it would stop
> sending Configuration Request Retry Status (CRS) messages to the
> host once configured, without this the host would retry until
> timeout and cancel the PCI configuration.
> * Add poll and timeout to wait for PHY PLLs to be locked, this
> is the only patch that also applies to the root complex function
> of the PCIe core controller, without this the kernel would
> sometimes access registers in the PHY PLL clock domain when the PLLs
> were not yet locked and the system would hang. This was hackily solved
> in other non mainline patches (e.g., in armbian) with a "msleep()"
> that was added after PHY PLL configuration but without realizing
> why it was needed. A poll with timeout seems like a sane approach.
> * Add dtsi entry for RK3399 PCIe endpoint core. The new entry is
> in "disabled" status by default, so unless it is explicitly enabled
> it will not conflict with the PCIe root complex controller entry.
> Developers that will enable it would know that the root complex function
> then must be disabled, this can be done in the board level DTS.
> * Update the RK3399 example in the documentation to a valid one.
> * Fix legacy IRQ generation for RK3399 PCIe endpoint core, the legacy IRQs
> were not sent by the device because their generation did not follow the
> instructions in the technical reference manual. They now work.
> * Fix window mapping and address translation for endpoint. The window
> mapping and address translation did not follow the technical reference
> manual and a single memory region was used which resulted in conflicting
> address translations for memory allocated in that region. The current
> patch allows to allocate up to 32 memory windows with 1MB pages.
> * Use u32 variable to access 32-bit registers, u16 variables were used to
> access and manipulate data of 32-bit registers, this would lead to
> overflows e.g., when left shifting more than 16 bits.
> * Don't advertise MSI-X in PCIe capabilities because according to the TRM
> the controller is not capable of generating them.
> * Set address alignment for the endpoint mode.
>
> Validation on real hardware:
>
> This patch series has been tested by me with kernels 6.0.19, 6.1.21,
> and 5.19 on real hardware, a FriendlyElec NanoPC-T4 RK3399 based single
> computer board connected to a host computer through PCIe x1 and x4. The
> driver was also tested by Damien Le Moal <[email protected]>
> on a Pine Rockpro64 board [1].

I retested this series on top of rc5 with the patched epf-test RC & EP drivers
with a Pine Rockpro64 board. All good, no issues detected.

Feel free to add:

Tested-by: Damien Le Moal <[email protected]>

>
> [1] https://lore.kernel.org/linux-pci/[email protected]/
>
> The PCIe endpoint test function driver was loaded on the SoC and the PCIe
> endpoint test driver was loaded on the host computer. The following tests were
> executed through this setup :
>
> * enumeration of the PCIe endpoint device (lspci)
> lspci -vvv
> * validation of PCI header and capabilities
> setpci and lspci -xxxx
> * device was recognized by host computer dans PCIe endpoint test driver
> was loaded
> lspci -v states "Kernel modules: pci_endpoint_test"
> * tested the BARs 0-5
> sudo /usr/bin/pcitest -b 0
> ...
> sudo /usr/bin/pcitest -b 5
> * tested legacy interrupt through the test driver
> sudo /usr/bin/pcitest -i 0
> sudo /usr/bin/pcitest -l
> * tested MSI interrupt through the test driver
> sudo /usr/bin/pcitest -i 1
> sudo /usr/bin/pcitest -m 1
> * tested read/write to and from host through the test driver with checksum
> sudo /usr/bin/pcitest -r -s 1024
> sudo /usr/bin/pcitest -w -s 1024
> * tested read/write with DMA enabled (all read/write tests also did IRQ)
> sudo /usr/bin/pcitest -r -d -s 8192
> sudo /usr/bin/pcitest -w -d -s 8192
> * tested larged transfers e.g., 100kB with and without DMA
>
> Commands used on the SoC to launch the endpoint function (configfs) :
>
> modprobe -i pci-epf-test
> mkdir -p /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0
> echo 0xb500 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/deviceid
> echo 0x104c > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/vendorid
> echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/msi_interrupts
> ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0 \
> /sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/
> echo 1 > /sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/start
>
> Note: to enable the endpoint controller on the board the file :
> arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
> Was edited to set the status of &pcie0 to "disabled" and &pcie0_ep
> to "okay". This is not submitted as a patch because most users
> will use the PCIe core controller in host (root complex) mode
> rather than endpoint mode.
>
> I have tested and confirmed all functionality required for the
> endpoint with the test driver and tools. With the initial driver commit
> cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> the device would not even be enumerated by the host computer (mainly because
> of CRS messages being sent back to the root complex) and tests would not pass
> (driver would not even be loaded because DID was not set correctly)
> and then only the BAR test would pass. Now all tests pass as stated above.
>
> Best regards
> Rick
>
> Damien Le Moal (1):
> PCI: rockchip: Set address alignment for endpoint mode
>
> Rick Wertenbroek (10):
> PCI: rockchip: Remove writes to unused registers
> PCI: rockchip: Write PCI Device ID to correct register
> PCI: rockchip: Assert PCI Configuration Enable bit after probe
> PCI: rockchip: Add poll and timeout to wait for PHY PLLs to be locked
> arm64: dts: rockchip: Add dtsi entry for RK3399 PCIe endpoint core
> dt-bindings: PCI: Update the RK3399 example to a valid one
> PCI: rockchip: Fix legacy IRQ generation for RK3399 PCIe endpoint core
> PCI: rockchip: Fix window mapping and address translation for endpoint
> PCI: rockchip: Use u32 variable to access 32-bit registers
> PCI: rockchip: Don't advertise MSI-X in PCIe capabilities
>
> .../bindings/pci/rockchip,rk3399-pcie-ep.yaml | 8 +-
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 27 +++
> drivers/pci/controller/pcie-rockchip-ep.c | 193 ++++++++----------
> drivers/pci/controller/pcie-rockchip.c | 17 ++
> drivers/pci/controller/pcie-rockchip.h | 44 ++--
> 5 files changed, 162 insertions(+), 127 deletions(-)
>

2023-04-05 11:46:51

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH v3 08/11] PCI: rockchip: Fix window mapping and address translation for endpoint

On 4/4/23 17:24, Rick Wertenbroek wrote:
> The RK3399 PCI endpoint core has 33 windows for PCIe space, now in the
> driver up to 32 fixed size (1M) windows are used and pages are allocated
> and mapped accordingly. The driver first used a single window and allocated
> space inside which caused translation issues (between CPU space and PCI
> space) because a window can only have a single translation at a given
> time, which if multiple pages are allocated inside will cause conflicts.
> Now each window is a single region of 1M which will always guarantee that
> the translation is not in conflict.
>
> Set the translation register addresses for physical function. As documented
> in the technical reference manual (TRM) section 17.5.5 "PCIe Address
> Translation" and section 17.6.8 "Address Translation Registers Description"
>
> Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> Cc: [email protected]
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 110 +++++++++-------------
> drivers/pci/controller/pcie-rockchip.h | 30 +++---
> 2 files changed, 64 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 7591a7be78e0..f366846ad77c 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -64,52 +64,30 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
> }
>
> static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
> - u32 r, u32 type, u64 cpu_addr,
> - u64 pci_addr, size_t size)
> + u32 r, u64 cpu_addr, u64 pci_addr,
> + size_t size)
> {
> u64 sz = 1ULL << fls64(size - 1);
> int num_pass_bits = ilog2(sz);
> - u32 addr0, addr1, desc0, desc1;
> - bool is_nor_msg = (type == AXI_WRAPPER_NOR_MSG);
> + u32 addr0, addr1, desc0;
>
> - /* The minimal region size is 1MB */
> if (num_pass_bits < 8)
> num_pass_bits = 8;
>
> - cpu_addr -= rockchip->mem_res->start;
> - addr0 = ((is_nor_msg ? 0x10 : (num_pass_bits - 1)) &
> - PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
> - (lower_32_bits(cpu_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> - addr1 = upper_32_bits(is_nor_msg ? cpu_addr : pci_addr);
> - desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | type;
> - desc1 = 0;
> -
> - if (is_nor_msg) {
> - rockchip_pcie_write(rockchip, 0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> - rockchip_pcie_write(rockchip, 0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> - rockchip_pcie_write(rockchip, desc0,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> - rockchip_pcie_write(rockchip, desc1,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> - } else {
> - /* PCI bus address region */
> - rockchip_pcie_write(rockchip, addr0,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> - rockchip_pcie_write(rockchip, addr1,
> - ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> - rockchip_pcie_write(rockchip, desc0,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> - rockchip_pcie_write(rockchip, desc1,
> - ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> -
> - addr0 =
> - ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
> - (lower_32_bits(cpu_addr) &
> - PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
> - addr1 = upper_32_bits(cpu_addr);
> - }
> + 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);
> + desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | AXI_WRAPPER_MEM_WRITE;
> +
> + /* PCI bus address region */
> + rockchip_pcie_write(rockchip, addr0,
> + ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
> + rockchip_pcie_write(rockchip, addr1,
> + ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
> + rockchip_pcie_write(rockchip, desc0,
> + ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
> + rockchip_pcie_write(rockchip, 0,
> + ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
> }
>
> static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
> @@ -248,6 +226,11 @@ static void rockchip_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar));
> }
>
> +static inline u32 rockchip_ob_region(phys_addr_t addr)
> +{
> + return (addr >> ilog2(SZ_1M)) & 0x1f;
> +}
> +
> 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)
> @@ -256,18 +239,9 @@ static int rockchip_pcie_ep_map_addr(struct pci_epc *epc, u8 fn, u8 vfn,
> struct rockchip_pcie *pcie = &ep->rockchip;
> u32 r;
>
> - r = find_first_zero_bit(&ep->ob_region_map, BITS_PER_LONG);
> - /*
> - * Region 0 is reserved for configuration space and shouldn't
> - * be used elsewhere per TRM, so leave it out.
> - */
> - if (r >= ep->max_regions - 1) {
> - dev_err(&epc->dev, "no free outbound region\n");
> - return -EINVAL;
> - }
> + r = rockchip_ob_region(addr);

Nit: you can move this together with the decalration:

u32 r = rockchip_ob_region(addr);

>
> - rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, AXI_WRAPPER_MEM_WRITE, addr,
> - pci_addr, size);
> + rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, addr, pci_addr, size);
>
> set_bit(r, &ep->ob_region_map);
> ep->ob_addr[r] = addr;
> @@ -282,15 +256,11 @@ static void rockchip_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
> struct rockchip_pcie *rockchip = &ep->rockchip;
> u32 r;
>
> - for (r = 0; r < ep->max_regions - 1; r++)
> + for (r = 0; r < ep->max_regions; r++)
> if (ep->ob_addr[r] == addr)
> break;
>
> - /*
> - * Region 0 is reserved for configuration space and shouldn't
> - * be used elsewhere per TRM, so leave it out.
> - */
> - if (r == ep->max_regions - 1)
> + if (r == ep->max_regions)
> return;
>
> rockchip_pcie_clear_ep_ob_atu(rockchip, r);
> @@ -388,6 +358,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> u16 flags, mme, data, data_mask;
> u8 msi_count;
> u64 pci_addr, pci_addr_mask = 0xff;

Nit: pci_addr_mask is constant and never changed, so we could get rid of this
variable and use a macro instead.

> + u32 r;
>
> /* Check MSI enable bit */
> flags = rockchip_pcie_read(&ep->rockchip,
> @@ -421,13 +392,12 @@ static int rockchip_pcie_ep_send_msi_irq(struct rockchip_pcie_ep *ep, u8 fn,
> ROCKCHIP_PCIE_EP_FUNC_BASE(fn) +
> ROCKCHIP_PCIE_EP_MSI_CTRL_REG +
> PCI_MSI_ADDRESS_LO);
> - pci_addr &= GENMASK_ULL(63, 2);
>
> /* Set the outbound region if needed. */
> if (unlikely(ep->irq_pci_addr != (pci_addr & ~pci_addr_mask) ||
> ep->irq_pci_fn != fn)) {
> - rockchip_pcie_prog_ep_ob_atu(rockchip, fn, ep->max_regions - 1,
> - AXI_WRAPPER_MEM_WRITE,
> + r = rockchip_ob_region(ep->irq_phys_addr);
> + rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> ep->irq_phys_addr,
> pci_addr & ~pci_addr_mask,
> pci_addr_mask + 1);
> @@ -516,6 +486,8 @@ static int rockchip_pcie_parse_ep_dt(struct rockchip_pcie *rockchip,
> if (err < 0 || ep->max_regions > MAX_REGION_LIMIT)
> ep->max_regions = MAX_REGION_LIMIT;
>
> + ep->ob_region_map = 0;
> +
> err = of_property_read_u8(dev->of_node, "max-functions",
> &ep->epc->max_functions);
> if (err < 0)
> @@ -536,7 +508,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> struct rockchip_pcie *rockchip;
> struct pci_epc *epc;
> size_t max_regions;
> - int err;
> + struct pci_epc_mem_window *windows = NULL;
> + int err, i;
>
> ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> if (!ep)
> @@ -583,15 +556,26 @@ 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);
>
> - err = pci_epc_mem_init(epc, rockchip->mem_res->start,
> - resource_size(rockchip->mem_res), PAGE_SIZE);
> + windows = devm_kcalloc(dev, ep->max_regions, sizeof(struct pci_epc_mem_window), GFP_KERNEL);

Nit: long line. Please split it at sizeof(...).

> + 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_128K);
> + SZ_1M);
> if (!ep->irq_cpu_addr) {
> dev_err(dev, "failed to reserve memory space for MSI\n");
> err = -ENOMEM;
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index ffc68a3a5fee..5797ba73bb6b 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -139,6 +139,7 @@
>
> #define PCIE_RC_RP_ATS_BASE 0x400000
> #define PCIE_RC_CONFIG_NORMAL_BASE 0x800000
> +#define PCIE_EP_PF_CONFIG_REGS_BASE 0x800000
> #define PCIE_RC_CONFIG_BASE 0xa00000
> #define PCIE_EP_CONFIG_BASE 0xa00000
> #define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)
> @@ -232,13 +233,15 @@
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
> #define ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR 0x1
> -#define ROCKCHIP_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
> +#define ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR 0x3
> +#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) \
> + (PCIE_EP_PF_CONFIG_REGS_BASE + 0x10000 + (((fn) << 12) & GENMASK(19, 12)))
> #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR0(fn, bar) \
> - (PCIE_RC_RP_ATS_BASE + 0x0840 + (fn) * 0x0040 + (bar) * 0x0008)
> + (PCIE_CORE_AXI_CONF_BASE + 0x0828 + (fn) * 0x0040 + (bar) * 0x0008)
> #define ROCKCHIP_PCIE_AT_IB_EP_FUNC_BAR_ADDR1(fn, bar) \
> - (PCIE_RC_RP_ATS_BASE + 0x0844 + (fn) * 0x0040 + (bar) * 0x0008)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0000 + ((r) & 0x1f) * 0x0020)
> + (PCIE_CORE_AXI_CONF_BASE + 0x082c + (fn) * 0x0040 + (bar) * 0x0008)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN_MASK GENMASK(19, 12)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) \
> (((devfn) << 12) & \
> @@ -246,20 +249,21 @@
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK GENMASK(27, 20)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS(bus) \
> (((bus) << 20) & ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0_BUS_MASK)
> +#define PCIE_RC_EP_ATR_OB_REGIONS_1_32 (PCIE_CORE_AXI_CONF_BASE + 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0000 + ((r) & 0x1f) * 0x0020)
> #define ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0004 + ((r) & 0x1f) * 0x0020)
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0004 + ((r) & 0x1f) * 0x0020)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_HARDCODED_RID BIT(23)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK GENMASK(31, 24)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(devfn) \
> (((devfn) << 24) & ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN_MASK)
> #define ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0008 + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x000c + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x0018 + ((r) & 0x1f) * 0x0020)
> -#define ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r) \
> - (PCIE_RC_RP_ATS_BASE + 0x001c + ((r) & 0x1f) * 0x0020)
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0008 + ((r) & 0x1f) * 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x000c + ((r) & 0x1f) * 0x0020)
> +#define ROCKCHIP_PCIE_AT_OB_REGION_DESC2(r) \
> + (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 0x0010 + ((r) & 0x1f) * 0x0020)
>
> #define ROCKCHIP_PCIE_CORE_EP_FUNC_BAR_CFG0(fn) \
> (PCIE_CORE_CTRL_MGMT_BASE + 0x0240 + (fn) * 0x0008)

2023-04-05 11:51:56

by Damien Le Moal

[permalink] [raw]
Subject: Re: [PATCH v3 10/11] PCI: rockchip: Don't advertise MSI-X in PCIe capabilities

On 4/4/23 17:24, Rick Wertenbroek wrote:
> The RK3399 PCIe endpoint controller cannot generate MSI-X IRQs.
> This is documented in the RK3399 technical reference manual (TRM)
> section 17.5.9 "Interrupt Support".
>
> MSI-X capability should therefore not be advertised. Remove the
> MSI-X capability by editing the capability linked-list. The
> previous entry is the MSI capability, therefore get the next
> entry from the MSI-X capability entry and set it as next entry
> for the MSI capability. This in effect removes MSI-X from the list.
>
> Linked list before : MSI cap -> MSI-X cap -> PCIe Device cap -> ...
> Linked list now : MSI cap -> PCIe Device cap -> ...
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 15 +++++++++++++++
> drivers/pci/controller/pcie-rockchip.h | 5 +++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 924b95bd736c..20c768287870 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -510,6 +510,7 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> size_t max_regions;
> struct pci_epc_mem_window *windows = NULL;
> int err, i;
> + u32 cfg_msi, cfg_msix_cp;
>
> ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> if (!ep)
> @@ -584,6 +585,20 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>
> ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;
>

Nit: Adding a comment here about what this is doing and why would be nice. E.g.
something like:

/*
* MSI-X is not supported but the controller still advertises by default
* the MSI-X capability, which can lead to the RC-side attempting to use
* MSI-X. Avoid this by skipping the MSI-X capability entry in the
* chain of PCIe capabilities: get the next pointer from the
* MSI-X entry and set that in the MSI capability entry. This way
* the MSI-X entry is skipped (left out of the linked-list).
*/

> + 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);
>
> return 0;
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index 1558eae298ae..a21070ea7166 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -226,6 +226,8 @@
> #define ROCKCHIP_PCIE_EP_CMD_STATUS 0x4
> #define ROCKCHIP_PCIE_EP_CMD_STATUS_IS BIT(19)
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_REG 0x90
> +#define ROCKCHIP_PCIE_EP_MSI_CP1_OFFSET 8
> +#define ROCKCHIP_PCIE_EP_MSI_CP1_MASK GENMASK(15, 8)
> #define ROCKCHIP_PCIE_EP_MSI_FLAGS_OFFSET 16
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_OFFSET 17
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MMC_MASK GENMASK(19, 17)
> @@ -233,6 +235,9 @@
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MME_MASK GENMASK(22, 20)
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_ME BIT(16)
> #define ROCKCHIP_PCIE_EP_MSI_CTRL_MASK_MSI_CAP BIT(24)
> +#define ROCKCHIP_PCIE_EP_MSIX_CAP_REG 0xb0
> +#define ROCKCHIP_PCIE_EP_MSIX_CAP_CP_OFFSET 8
> +#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_EP_FUNC_BASE(fn) \