This is a series of patches that fixes the PCIe endpoint controller driver
for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
The original driver in mainline had issues and would not allow for the
RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
that the PCIe core controller of the RK3399 SoC can now act as a PCIe
endpoint.
This patch series has been tested on kernel 6.0.19 (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 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 /sur/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
Summary of changes :
This patch series is composed of 8 patches that do the following :
* Removed 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.
* Fixed setup to the PCI Device ID (DID), this was written to a read
only register and therefore would not update the DID.
* Fixed setup of the PCIe endpoint controller 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.
* Added a poll with timeout to check the PHY PLL lock status, 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.
* Added a dtsi entry for the PCIe endpoint controller. 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.
* Fixed the window translation between CPU space and PCI space.
Allows up to 32 memory windows, with (1MB) page allocation and mapping.
* Fixed the legacy IRQ (INTx) generation of the PCIe core in
endpoint mode.
* Fixed the generation of message signalled interrupts (MSI) of the
PCIe core in endpoint mode.
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.
I have tested and confirmed all basic functionality required for the
endpoint with the test driver and tools. With the previous state of
the driver 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
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.
Rick Wertenbroek (8):
PCI: rockchip: Removed writes to unused registers
PCI: rockchip: Fixed setup of Device ID
PCI: rockchip: Fixed endpoint controller Configuration Request Retry
Status
PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be
locked
PCI: rockchip: Added dtsi entry for PCIe endpoint controller
PCI: rockchip: Fixed window mapping and address translation for
endpoint
PCI: rockchip: Fixed legacy IRQ generation for endpoint
PCI: rockchip: Fixed MSI generation from PCIe endpoint core
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++
drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++-----------
drivers/pci/controller/pcie-rockchip.c | 16 +++
drivers/pci/controller/pcie-rockchip.h | 36 ++++--
4 files changed, 137 insertions(+), 89 deletions(-)
--
2.25.1
Removed write accesses to registers that are marked "unused" in the
technical reference manual (TRM) (see RK3399 TRM 17.6.8.1)
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index d1a200b93..d5c477020 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -61,10 +61,6 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
ROCKCHIP_PCIE_AT_OB_REGION_DESC0(region));
rockchip_pcie_write(rockchip, 0,
ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
- rockchip_pcie_write(rockchip, 0,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(region));
- rockchip_pcie_write(rockchip, 0,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(region));
}
static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
@@ -114,12 +110,6 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
addr1 = upper_32_bits(cpu_addr);
}
-
- /* CPU bus address region */
- rockchip_pcie_write(rockchip, addr0,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r));
- rockchip_pcie_write(rockchip, addr1,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r));
}
static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
--
2.25.1
Device ID was written to a read-only register and therefore did not work.
The Device ID is now updated through the correct register.
This is documented in the RK3399 TRM section 17.6.6.1.1
Signed-off-by: Rick Wertenbroek <[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 d5c477020..9b835377b 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 32c3a859c..51a123e5c 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
The endpoint was left in the config mode after probe, this would mean the
core would send configuration requestion retry status (CRS) messages back
to the root complex. The conf_en bit should be asserted after probe. This
is documented in section 17.5.8.1.2 of the RK3399 TRM.
Signed-off-by: Rick Wertenbroek <[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 9b835377b..4c84e403e 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
The Rockchip PCIe controller did not wait until the PHY PLLs were locked.
This could cause hangs. Now the PHY PLLs status is checked through a side
channel bit with a poll and timeout. If the PHY PLLs cannot lock an error
is generated. This is documented in the TRM section 17.5.8.1 PCIe
Initalization Sequence.
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip.c | 16 ++++++++++++++++
drivers/pci/controller/pcie-rockchip.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
index 990a00e08..5f2e2dd5d 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,15 @@ 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 51a123e5c..f3a5ff1cf 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
Added missing PCIe endpoint controller entry in the device tree. This
entry is documented in :
Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
The status is disabled by default, so it will not be loaded unless
explicitly chosen to.
Signed-off-by: Rick Wertenbroek <[email protected]>
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 9d5b0e8c9..5f7251118 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -265,6 +265,31 @@ pcie0_intc: interrupt-controller {
};
};
+ pcie0_ep: pcie-ep@f8000000 {
+ compatible = "rockchip,rk3399-pcie-ep";
+ #address-cells = <3>;
+ #size-cells = <2>;
+ 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
The RK3399 PCI EP core has 33 windows for PCIe space, here in the driver
up to 32 fixed size (1M) windows are used and pages are allocated /
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 regions 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.
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 62 ++++++++++++-----------
drivers/pci/controller/pcie-rockchip.h | 25 ++++-----
2 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 4c84e403e..a682a941d 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -76,11 +76,17 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
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);
+ if (is_nor_msg) {
+ dev_warn(rockchip->dev, "NOR MSG\n");
+ cpu_addr -= rockchip->mem_res->start;
+ addr0 = (0x10 & 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);
+ } else {
+ addr0 = (num_pass_bits & 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) | type;
desc1 = 0;
@@ -103,12 +109,6 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
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);
}
}
@@ -256,15 +256,7 @@ 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 = (addr >> ilog2(SZ_1M)) & 0x1f;
rockchip_pcie_prog_ep_ob_atu(pcie, fn, r, AXI_WRAPPER_MEM_WRITE, addr,
pci_addr, size);
@@ -282,15 +274,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);
@@ -539,6 +527,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)
@@ -559,7 +549,10 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
struct rockchip_pcie *rockchip;
struct pci_epc *epc;
size_t max_regions;
+ struct pci_epc_mem_window *windows = NULL;
int err;
+ u32 cfg;
+ int i;
ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
if (!ep)
@@ -606,15 +599,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 f3a5ff1cf..72e427a0f 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -134,6 +134,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)
@@ -228,13 +229,14 @@
#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_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) & \
@@ -242,20 +244,19 @@
#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)
+ (PCIE_RC_EP_ATR_OB_REGIONS_1_32 + 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 + 0x000c + ((r) & 0x1f) * 0x0020)
#define ROCKCHIP_PCIE_CORE_EP_FUNC_BAR_CFG0(fn) \
(PCIE_CORE_CTRL_MGMT_BASE + 0x0240 + (fn) * 0x0008)
--
2.25.1
Added generation of legacy IRQ (INTx) for the RK3399 SoC PCIe EP core.
The generation of the legacy interrupt was validated with the PCIe EP
test driver. Generation of IRQ through the core is documented in the
TRM and is done through the PCIE_CLIENT_LEGACY_INT_CTRL register of
the core.
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 32 ++++++-----------------
drivers/pci/controller/pcie-rockchip.h | 6 +++++
2 files changed, 14 insertions(+), 24 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index a682a941d..a58c9d56b 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -333,15 +333,6 @@ static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn,
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) {
ep->irq_pending |= BIT(intx);
@@ -351,22 +342,15 @@ static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn,
msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx;
}
- 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);
+ if (is_asserted) {
+ rockchip_pcie_write(&ep->rockchip,
+ PCIE_CLIENT_INT_IN_ASSERT | PCIE_CLIENT_INT_PEND_ST_PEND,
+ PCIE_CLIENT_LEGACY_INT_CTRL);
+ } else {
+ rockchip_pcie_write(&ep->rockchip,
+ PCIE_CLIENT_INT_IN_DEASSERT | PCIE_CLIENT_INT_PEND_ST_NORMAL,
+ PCIE_CLIENT_LEGACY_INT_CTRL);
}
-
- 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 72e427a0f..e90c2a2b8 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -39,6 +39,12 @@
#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_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)
#define PCIE_CLIENT_DEBUG_LTSSM_MASK GENMASK(5, 0)
--
2.25.1
The generation of MSI interrupts from the RK3399 PCIe endpoint core was
broken. The main issue came from u16 variables to be used to access u32
registers, this would lead to shifts of more than 16 bits moving data
out of the variable etc. Address translation for sending the MSI messages
over PCIe has also been fixed.
Signed-off-by: Rick Wertenbroek <[email protected]>
---
drivers/pci/controller/pcie-rockchip-ep.c | 37 +++++++++++++++--------
drivers/pci/controller/pcie-rockchip.h | 1 +
2 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index a58c9d56b..b26f16bed 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -288,19 +288,31 @@ static void rockchip_pcie_ep_unmap_addr(struct pci_epc *epc, u8 fn, u8 vfn,
}
static int rockchip_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn,
- u8 multi_msg_cap)
+ u8 mmc)
{
struct rockchip_pcie_ep *ep = epc_get_drvdata(epc);
struct rockchip_pcie *rockchip = &ep->rockchip;
- u16 flags;
+ u32 flags;
+ u32 multi_msg_cap;
+
+ if (fn) {
+ dev_err(&epc->dev, "This endpoint controller only supports a single physical function\n");
+ return -EINVAL;
+ }
+
+ if (mmc > 0x5) {
+ dev_err(&epc->dev, "Number of MSI IRQs cannot be more than 32\n");
+ return -EINVAL;
+ }
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;
+ multi_msg_cap = mmc;
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) +
@@ -312,7 +324,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) +
@@ -380,9 +392,10 @@ 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;
/* Check MSI enable bit */
flags = rockchip_pcie_read(&ep->rockchip,
@@ -416,16 +429,16 @@ 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,
- ep->irq_phys_addr,
- pci_addr & ~pci_addr_mask,
- pci_addr_mask + 1);
+ r = (ep->irq_phys_addr >> ilog2(SZ_1M)) & 0x1f;
+ rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
+ AXI_WRAPPER_MEM_WRITE,
+ ep->irq_phys_addr,
+ pci_addr & ~pci_addr_mask,
+ pci_addr_mask + 1);
ep->irq_pci_addr = (pci_addr & ~pci_addr_mask);
ep->irq_pci_fn = fn;
}
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index e90c2a2b8..11dbf53cd 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -227,6 +227,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
On Thu, Jan 26, 2023 at 02:50:44PM +0100, Rick Wertenbroek wrote:
> The Rockchip PCIe controller did not wait until the PHY PLLs were locked.
> This could cause hangs. Now the PHY PLLs status is checked through a side
> channel bit with a poll and timeout. If the PHY PLLs cannot lock an error
> is generated. This is documented in the TRM section 17.5.8.1 PCIe
> Initalization Sequence.
s/Initalization/Initialization/
Hi Rick,
Thanks very much for your work.
On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> This is a series of patches that fixes the PCIe endpoint controller driver
> for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
>
> The original driver in mainline had issues and would not allow for the
> RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> endpoint.
So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
PCIe controller") when it actually didn't work? Ouch. Thanks for
fixing it and testing it.
> Rick Wertenbroek (8):
> PCI: rockchip: Removed writes to unused registers
> PCI: rockchip: Fixed setup of Device ID
> PCI: rockchip: Fixed endpoint controller Configuration Request Retry
> Status
> PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be
> locked
> PCI: rockchip: Added dtsi entry for PCIe endpoint controller
> PCI: rockchip: Fixed window mapping and address translation for
> endpoint
> PCI: rockchip: Fixed legacy IRQ generation for endpoint
> PCI: rockchip: Fixed MSI generation from PCIe endpoint core
For the next iteration, can you please update these subject lines and
commit logs to:
- Use imperative mood, i.e., read like a command, instead of a past
tense description of what was done. For example, say "Remove
writes to unused registers" instead of "Removed writes ..."
- Be more specific when possible. "Fix" conveys no information
about the actual code change. For example, "Fixed endpoint
controller Configuration Request Retry Status" gives a general
idea, but it would be more useful if it said something about
clearing config mode after probe.
- Say what the patch does in the commit log. The current ones often
describe a *problem*, but do not explicitly say what the patch
does. The commit log should be complete in itself even without
the subject line, so it usually contains a slightly expanded
version of the subject line.
- Split patches that do more than one logical thing. The commit log
for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
but the patch also adds an unrelated check for multi-function
devices.
- If a patch is a fix for an existing issue and may need to be
backported, identify the commit that introduced the issue and add
"Fixes: " lines. This helps distros figure out whether and how
far to backport patches.
- Refer to the device consistently. I see:
RK3399 PCI EP core
RK3399 SoC PCIe EP core
RK3399 PCIe endpoint core
I vote for "RK3399 PCIe Endpoint core".
Notes about imperative mood:
https://chris.beams.io/posts/git-commit/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++
> drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++-----------
> drivers/pci/controller/pcie-rockchip.c | 16 +++
> drivers/pci/controller/pcie-rockchip.h | 36 ++++--
> 4 files changed, 137 insertions(+), 89 deletions(-)
>
> --
> 2.25.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 26/01/2023 14:50, Rick Wertenbroek wrote:
> Added missing PCIe endpoint controller entry in the device tree. This
> entry is documented in :
> Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
There is no such file
> The status is disabled by default, so it will not be loaded unless
> explicitly chosen to.
Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 9d5b0e8c9..5f7251118 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -265,6 +265,31 @@ pcie0_intc: interrupt-controller {
> };
> };
>
> + pcie0_ep: pcie-ep@f8000000 {
> + compatible = "rockchip,rk3399-pcie-ep";
reg is usually second property...
> + #address-cells = <3>;
> + #size-cells = <2>;
Best regards,
Krzysztof
Hello, Bjorn
Thank you for your prompt reply and helpful comments.
Le jeu. 26 janv. 2023 à 15:52, Bjorn Helgaas <[email protected]> a écrit :
>
> Hi Rick,
>
> Thanks very much for your work.
>
> On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> > This is a series of patches that fixes the PCIe endpoint controller driver
> > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> >
> > The original driver in mainline had issues and would not allow for the
> > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> > that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> > endpoint.
>
> So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
> PCIe controller") when it actually didn't work? Ouch. Thanks for
> fixing it and testing it.
It seems it wasn't fully tested, the code compiles and kernel module loads,
but further functionality didn't seem to have been tested
(e.g., lspci, and with the pcitest tool and pci_endpoit_test_driver).
>
> For the next iteration, can you please update these subject lines and
> commit logs to:
Thank you, I will prepare the changes and add them to the next iteration
with changes from other comments that may arise.
>
> - Use imperative mood, i.e., read like a command, instead of a past
> tense description of what was done. For example, say "Remove
> writes to unused registers" instead of "Removed writes ..."
>
> - Be more specific when possible. "Fix" conveys no information
> about the actual code change. For example, "Fixed endpoint
> controller Configuration Request Retry Status" gives a general
> idea, but it would be more useful if it said something about
> clearing config mode after probe.
>
> - Say what the patch does in the commit log. The current ones often
> describe a *problem*, but do not explicitly say what the patch
> does. The commit log should be complete in itself even without
> the subject line, so it usually contains a slightly expanded
> version of the subject line.
>
> - Split patches that do more than one logical thing. The commit log
> for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
> but the patch also adds an unrelated check for multi-function
> devices.
I will. I tried to split everything into the function it was related to, but I
now understand I should split even more so that the commit message
and changes are more tightly linked.
>
> - If a patch is a fix for an existing issue and may need to be
> backported, identify the commit that introduced the issue and add
> "Fixes: " lines. This helps distros figure out whether and how
> far to backport patches.
Does this mean I should refer to the commit cf590b078391
("PCI: rockchip: Add EP driver for Rockchip PCIe controller") ?
Because it wasn't working in the first place ?
>
> - Refer to the device consistently. I see:
> RK3399 PCI EP core
> RK3399 SoC PCIe EP core
> RK3399 PCIe endpoint core
> I vote for "RK3399 PCIe Endpoint core".
I agree.
>
> Notes about imperative mood:
> https://chris.beams.io/posts/git-commit/
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94
Thank you for all the pointers, I'll take them into account for the
next iteration. This is the first time I actually submitted a series of
patches to the LKML so it's all relatively new to me.
On 26/01/2023 14:50, Rick Wertenbroek wrote:
> Added generation of legacy IRQ (INTx) for the RK3399 SoC PCIe EP core.
Here and in all other patches and subjects: Use imperative, not past tense.
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
"Fix legacy IRQ", not "Fixed legacy IRQ".
> The generation of the legacy interrupt was validated with the PCIe EP
> test driver. Generation of IRQ through the core is documented in the
> TRM and is done through the PCIE_CLIENT_LEGACY_INT_CTRL register of
> the core.
>
If this is a fix, you need fixes tag and maybe cc-stable. Also the bug
should be described - it's effect, impact. Then the patch should be
first in the series (or even entirely separate).
Best regards,
Krzysztof
On 26/01/2023 14:50, Rick Wertenbroek wrote:
> The generation of MSI interrupts from the RK3399 PCIe endpoint core was
> broken. The main issue came from u16 variables to be used to access u32
> registers, this would lead to shifts of more than 16 bits moving data
> out of the variable etc. Address translation for sending the MSI messages
> over PCIe has also been fixed.
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
Same comments as other fix. Missing Fixes tag, cc-stable, move to the
beginning of patchset.
Best regards,
Krzysztof
On Thu, Jan 26, 2023 at 4:23 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 26/01/2023 14:50, Rick Wertenbroek wrote:
> > Added missing PCIe endpoint controller entry in the device tree. This
> > entry is documented in :
> > Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
>
> There is no such file
Sorry but the file exists see :
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt?h=v6.0.19
It also exists in 6.1 :
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt?h=linux-6.1.y
>
> > The status is disabled by default, so it will not be loaded unless
> > explicitly chosen to.
>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>
Sorry about this, you are right, I'll fix it for the next iteration.
Thank you
> >
> > Signed-off-by: Rick Wertenbroek <[email protected]>
> > ---
> > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > index 9d5b0e8c9..5f7251118 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > @@ -265,6 +265,31 @@ pcie0_intc: interrupt-controller {
> > };
> > };
> >
> > + pcie0_ep: pcie-ep@f8000000 {
> > + compatible = "rockchip,rk3399-pcie-ep";
>
> reg is usually second property...
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> Best regards,
> Krzysztof
>
On 26/01/2023 16:30, Rick Wertenbroek wrote:
> On Thu, Jan 26, 2023 at 4:23 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 26/01/2023 14:50, Rick Wertenbroek wrote:
>>> Added missing PCIe endpoint controller entry in the device tree. This
>>> entry is documented in :
>>> Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
>>
>> There is no such file
>
> Sorry but the file exists see :
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt?h=v6.0.19
> It also exists in 6.1 :
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt?h=linux-6.1.y
It's some old tree. It could have existed in the past. But it does not
exist. Don't refer to non-existing paths.
Your code needs then rebasing if you refer to some old trees. Please
always work on latest maintainer's tree. Optionally on linux-next.
Best regards,
Krzysztof
On Thu, Jan 26, 2023 at 04:23:57PM +0100, Rick Wertenbroek wrote:
> Le jeu. 26 janv. 2023 ? 15:52, Bjorn Helgaas <[email protected]> a ?crit :
> > Thanks very much for your work.
> >
> > On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> > > This is a series of patches that fixes the PCIe endpoint controller driver
> > > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> > >
> > > The original driver in mainline had issues and would not allow for the
> > > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> > > that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> > > endpoint.
> >
> > So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
> > PCIe controller") when it actually didn't work? Ouch. Thanks for
> > fixing it and testing it.
>
> It seems it wasn't fully tested, the code compiles and kernel module loads,
> but further functionality didn't seem to have been tested
> (e.g., lspci, and with the pcitest tool and pci_endpoit_test_driver).
OK, I guess that happens sometimes. Glad you're getting it into
shape!
> Does this mean I should refer to the commit cf590b078391
> ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") ?
> Because it wasn't working in the first place ?
Yes, I think so.
> Thank you for all the pointers, I'll take them into account for the
> next iteration. This is the first time I actually submitted a series of
> patches to the LKML so it's all relatively new to me.
Welcome to Linux, and great start!
Bjorn
DTC arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm016-dc2.dtb
../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
(pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
configuration space
DTC arch/arm64/boot/dts/amlogic/meson-gxm-s912-libretech-pc.dtb
../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
(pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
configuration space
HDRINST usr/include/linux/aio_abi.h
HDRINST usr/include/linux/am437x-vpfe.h
../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
(pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
configuration space
DTC arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm017-dc3.dtb
Thanks,
Alok
On 1/26/2023 7:20 PM, Rick Wertenbroek wrote:
> Added missing PCIe endpoint controller entry in the device tree. This
> entry is documented in :
> Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
> The status is disabled by default, so it will not be loaded unless
> explicitly chosen to.
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 9d5b0e8c9..5f7251118 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -265,6 +265,31 @@ pcie0_intc: interrupt-controller {
> };
> };
>
> + pcie0_ep: pcie-ep@f8000000 {
> + compatible = "rockchip,rk3399-pcie-ep";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + 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>;
Hi Rick,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rockchip/for-next]
[also build test WARNING on linus/master v6.2-rc5 next-20230127]
[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/Rick-Wertenbroek/PCI-rockchip-Removed-writes-to-unused-registers/20230128-155300
base: https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
patch link: https://lore.kernel.org/r/20230126135049.708524-8-rick.wertenbroek%40gmail.com
patch subject: [PATCH 7/8] PCI: rockchip: Fixed legacy IRQ generation for endpoint
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230128/[email protected]/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/53e861f3393ad4ebae5f2e133f4783b919036e19
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Rick-Wertenbroek/PCI-rockchip-Removed-writes-to-unused-registers/20230128-155300
git checkout 53e861f3393ad4ebae5f2e133f4783b919036e19
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/pci/controller/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_assert_intx':
>> drivers/pci/controller/pcie-rockchip-ep.c:334:12: warning: variable 'msg_code' set but not used [-Wunused-but-set-variable]
334 | u8 msg_code;
| ^~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:333:13: warning: unused variable 'status' [-Wunused-variable]
333 | u32 status;
| ^~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:332:13: warning: unused variable 'offset' [-Wunused-variable]
332 | u32 offset;
| ^~~~~~
drivers/pci/controller/pcie-rockchip-ep.c:331:13: warning: unused variable 'r' [-Wunused-variable]
331 | u32 r = ep->max_regions - 1;
| ^
drivers/pci/controller/pcie-rockchip-ep.c:330:31: warning: unused variable 'rockchip' [-Wunused-variable]
330 | struct rockchip_pcie *rockchip = &ep->rockchip;
| ^~~~~~~~
drivers/pci/controller/pcie-rockchip-ep.c: In function 'rockchip_pcie_ep_probe':
drivers/pci/controller/pcie-rockchip-ep.c:538:13: warning: unused variable 'cfg' [-Wunused-variable]
538 | u32 cfg;
| ^~~
vim +/msg_code +334 drivers/pci/controller/pcie-rockchip-ep.c
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 326
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 327 static void rockchip_pcie_ep_assert_intx(struct rockchip_pcie_ep *ep, u8 fn,
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 328 u8 intx, bool is_asserted)
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 329 {
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 330 struct rockchip_pcie *rockchip = &ep->rockchip;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 331 u32 r = ep->max_regions - 1;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 332 u32 offset;
c577f4a5a08bb9 drivers/pci/controller/pcie-rockchip-ep.c Colin Ian King 2019-03-30 333 u32 status;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 @334 u8 msg_code;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 335
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 336 intx &= 3;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 337 if (is_asserted) {
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 338 ep->irq_pending |= BIT(intx);
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 339 msg_code = ROCKCHIP_PCIE_MSG_CODE_ASSERT_INTA + intx;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 340 } else {
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 341 ep->irq_pending &= ~BIT(intx);
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 342 msg_code = ROCKCHIP_PCIE_MSG_CODE_DEASSERT_INTA + intx;
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 343 }
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 344
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 345 if (is_asserted) {
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 346 rockchip_pcie_write(&ep->rockchip,
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 347 PCIE_CLIENT_INT_IN_ASSERT | PCIE_CLIENT_INT_PEND_ST_PEND,
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 348 PCIE_CLIENT_LEGACY_INT_CTRL);
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 349 } else {
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 350 rockchip_pcie_write(&ep->rockchip,
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 351 PCIE_CLIENT_INT_IN_DEASSERT | PCIE_CLIENT_INT_PEND_ST_NORMAL,
53e861f3393ad4 drivers/pci/controller/pcie-rockchip-ep.c Rick Wertenbroek 2023-01-26 352 PCIE_CLIENT_LEGACY_INT_CTRL);
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 353 }
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 354 }
cf590b07839133 drivers/pci/host/pcie-rockchip-ep.c Shawn Lin 2018-05-09 355
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Fri, Jan 27, 2023 at 9:43 AM ALOK TIWARI <[email protected]> wrote:
>
> DTC arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm016-dc2.dtb
> ../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
> (pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
> configuration space
> DTC arch/arm64/boot/dts/amlogic/meson-gxm-s912-libretech-pc.dtb
> ../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
> (pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
> configuration space
> HDRINST usr/include/linux/aio_abi.h
> HDRINST usr/include/linux/am437x-vpfe.h
> ../arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi:460.3-52: Warning
> (pci_device_reg): /pcie@f8000000/pcie@0,0:reg: PCI reg address is not
> configuration space
> DTC arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm017-dc3.dtb
>
>
> Thanks,
>
> Alok
These warnings are for the pcie (host controller), I did not touch that entry.
Plus they are for another file (arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi)
They are related to "pci_device_reg" warnings (can be suppressed with
"-Wno-pci_device_reg").
Sorry, but this does not seem to be related to my change and I don't know
how to fix this.
Rick
On Thu, Jan 26, 2023 at 7:52 AM Rick Wertenbroek
<[email protected]> wrote:
>
> Added missing PCIe endpoint controller entry in the device tree. This
> entry is documented in :
> Documentation/devicetree/bindings/pci/rockchip-pcie-ep.txt
> The status is disabled by default, so it will not be loaded unless
> explicitly chosen to.
>
> Signed-off-by: Rick Wertenbroek <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 9d5b0e8c9..5f7251118 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -265,6 +265,31 @@ pcie0_intc: interrupt-controller {
> };
> };
>
> + pcie0_ep: pcie-ep@f8000000 {
> + compatible = "rockchip,rk3399-pcie-ep";
> + #address-cells = <3>;
> + #size-cells = <2>;
These are only needed when you have child nodes. Additionally, it
would not be a PCI bus which is the only case that has 3 address
cells.
There's a schema for this in linux-next now. Please test this change
with that. It should point out the above issue and maybe others.
> + 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
>