Message ID | 20240430-rockchip-pcie-ep-v1-v2-0-a0f5ee2a77b6@kernel.org |
---|---|
Headers | show |
Series | PCI: dw-rockchip: Add endpoint mode support | expand |
On Tue, Apr 30, 2024 at 02:00:57PM +0200, Niklas Cassel wrote: > Hello all, > > This series adds PCIe endpoint mode support for the rockchip rk3588 and > rk3568 SoCs. > > This series is based on: pci/next > (git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git) > > This series has the following dependencies: > 1) https://lore.kernel.org/linux-pci/20240430-pci-epf-rework-v4-0-22832d0d456f@linaro.org/ > The series in 1) has not been merged to pci/next yet. > > 2) https://lore.kernel.org/linux-phy/20240412125818.17052-1-cassel@kernel.org/ > The series in 2) has already been merged to phy/next. > > Even though this series (the series in $subject) has a runtime dependency > on the changes that are currently queued in the PHY tree, there is no need > to coordinate between the PCI tree and the PHY tree (i.e. this series can > be merged via the PCI tree even for the coming merge window (v6.10-rc1)). > > This is because there is no compile time dependency between the changes in > the PHY tree and this series. Likewise, the device tree overlays in this > series passes "make CHECK_DTBS=y" even without the changes in the PHY tree. > > This series (including dependencies) can also be found in git: > https://github.com/floatious/linux/commits/rockchip-pcie-ep-v2 > > Testing done: > This series has been tested with two rock5b:s, one running in RC mode and > one running in EP mode. This series has also been tested with an Intel x86 > host and rock5b running in EP mode. > > BAR4 exposes the ATU Port Logic Structure and the DMA Port Logic Structure Is this for configuring the EP from host? Just curious. > to the host. The EPC controller driver thus disables this BAR as init time, > because if it doesn't, when the host writes the test pattern to this BAR, > all the iATU settings will get wiped, resulting in all further BAR accesses > being non-functional. > > Running pcitest.sh (modified to also perform the READ and WRITE tests with > the -d option, i.e. with DMA enabled) results in the following: > > $ /usr/bin/pcitest.sh > BAR tests > > BAR0: OKAY > BAR1: OKAY > BAR2: OKAY > BAR3: OKAY > BAR4: NOT OKAY > BAR5: OKAY > > Interrupt tests > > SET IRQ TYPE TO LEGACY: OKAY > LEGACY IRQ: NOT OKAY > SET IRQ TYPE TO MSI: OKAY > MSI1: OKAY > MSI2: OKAY > MSI3: OKAY > MSI4: OKAY > MSI5: OKAY > MSI6: OKAY > MSI7: OKAY > MSI8: OKAY > MSI9: OKAY > MSI10: OKAY > MSI11: OKAY > MSI12: OKAY > MSI13: OKAY > MSI14: OKAY > MSI15: OKAY > MSI16: OKAY > MSI17: OKAY > MSI18: OKAY > MSI19: OKAY > MSI20: OKAY > MSI21: OKAY > MSI22: OKAY > MSI23: OKAY > MSI24: OKAY > MSI25: OKAY > MSI26: OKAY > MSI27: OKAY > MSI28: OKAY > MSI29: OKAY > MSI30: OKAY > MSI31: OKAY > MSI32: OKAY > > SET IRQ TYPE TO MSI-X: OKAY > MSI-X1: OKAY > MSI-X2: OKAY > MSI-X3: OKAY > MSI-X4: OKAY > MSI-X5: OKAY > MSI-X6: OKAY > MSI-X7: OKAY > MSI-X8: OKAY > MSI-X9: OKAY > MSI-X10: OKAY > MSI-X11: OKAY > MSI-X12: OKAY > MSI-X13: OKAY > MSI-X14: OKAY > MSI-X15: OKAY > MSI-X16: OKAY > MSI-X17: OKAY > MSI-X18: OKAY > MSI-X19: OKAY > MSI-X20: OKAY > MSI-X21: OKAY > MSI-X22: OKAY > MSI-X23: OKAY > MSI-X24: OKAY > MSI-X25: OKAY > MSI-X26: OKAY > MSI-X27: OKAY > MSI-X28: OKAY > MSI-X29: OKAY > MSI-X30: OKAY > MSI-X31: OKAY > MSI-X32: OKAY > > Read Tests > > SET IRQ TYPE TO MSI: OKAY > READ ( 1 bytes): OKAY > READ ( 1024 bytes): OKAY > READ ( 1025 bytes): OKAY > READ (1024000 bytes): OKAY > READ (1024001 bytes): OKAY > > Write Tests > > WRITE ( 1 bytes): OKAY > WRITE ( 1024 bytes): OKAY > WRITE ( 1025 bytes): OKAY > WRITE (1024000 bytes): OKAY > WRITE (1024001 bytes): OKAY > > Copy Tests > > COPY ( 1 bytes): OKAY > COPY ( 1024 bytes): OKAY > COPY ( 1025 bytes): OKAY > COPY (1024000 bytes): OKAY > COPY (1024001 bytes): OKAY > > Read Tests DMA > > READ ( 1 bytes): OKAY > READ ( 1024 bytes): OKAY > READ ( 1025 bytes): OKAY > READ (1024000 bytes): OKAY > READ (1024001 bytes): OKAY > > Write Tests DMA > > WRITE ( 1 bytes): OKAY > WRITE ( 1024 bytes): OKAY > WRITE ( 1025 bytes): OKAY > WRITE (1024000 bytes): OKAY > WRITE (1024001 bytes): OKAY > > Corresponding output on the EP side: > rockchip-dw-pcie a40000000.pcie-ep: EP cannot raise INTX IRQs > pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: NO, Time: 0.000000292 s, Rate: 3424 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: NO, Time: 0.000007583 s, Rate: 135038 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: NO, Time: 0.000007584 s, Rate: 135152 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: NO, Time: 0.009164167 s, Rate: 111739 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: NO, Time: 0.009164458 s, Rate: 111736 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: NO, Time: 0.000001750 s, Rate: 571 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: NO, Time: 0.000147875 s, Rate: 6924 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: NO, Time: 0.000149041 s, Rate: 6877 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: NO, Time: 0.147537833 s, Rate: 6940 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: NO, Time: 0.147533750 s, Rate: 6940 KB/s > pci_epf_test pci_epf_test.0: COPY => Size: 1 B, DMA: NO, Time: 0.000003208 s, Rate: 311 KB/s > pci_epf_test pci_epf_test.0: COPY => Size: 1024 B, DMA: NO, Time: 0.000156625 s, Rate: 6537 KB/s > pci_epf_test pci_epf_test.0: COPY => Size: 1025 B, DMA: NO, Time: 0.000158375 s, Rate: 6471 KB/s > pci_epf_test pci_epf_test.0: COPY => Size: 1024000 B, DMA: NO, Time: 0.156902666 s, Rate: 6526 KB/s > pci_epf_test pci_epf_test.0: COPY => Size: 1024001 B, DMA: NO, Time: 0.156847833 s, Rate: 6528 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: YES, Time: 0.000185500 s, Rate: 5 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: YES, Time: 0.000177334 s, Rate: 5774 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: YES, Time: 0.000178792 s, Rate: 5732 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: YES, Time: 0.000486209 s, Rate: 2106090 KB/s > pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: YES, Time: 0.000486791 s, Rate: 2103574 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: YES, Time: 0.000177333 s, Rate: 5 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: YES, Time: 0.000177625 s, Rate: 5764 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: YES, Time: 0.000171208 s, Rate: 5986 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: YES, Time: 0.000701167 s, Rate: 1460422 KB/s > pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: YES, Time: 0.000702625 s, Rate: 1457393 KB/s > Thanks a lot for sharing the pcitest results in the cover letter. - Mani > Kind regards, > Niklas > > --- > Changes in v2: > - Rebased on v4 of the pci-epf-rework series that we depend on. > - Picked up tags from Rob. > - Split dw-rockchip DT binding in to common, RC and EP parts. > - Added support for rk3568 in DT binding and driver. > - Added a new patch that fixed "combined legacy IRQ description". > - Link to v1: https://lore.kernel.org/r/20240424-rockchip-pcie-ep-v1-v1-0-b1a02ddad650@kernel.org > > --- > Niklas Cassel (14): > dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific reg-name > dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific interrupt-names > dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs > dt-bindings: PCI: rockchip-dw-pcie: Prepare for Endpoint mode support > dt-bindings: PCI: rockchip-dw-pcie: Fix description of legacy irq > dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller > PCI: dw-rockchip: Fix weird indentation > PCI: dw-rockchip: Add rockchip_pcie_ltssm() helper > PCI: dw-rockchip: Refactor the driver to prepare for EP mode > PCI: dw-rockchip: Add explicit rockchip,rk3588-pcie compatible > PCI: dw-rockchip: Add endpoint mode support > misc: pci_endpoint_test: Add support for rockchip rk3588 > arm64: dts: rockchip: Add PCIe endpoint mode support > arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode > > .../bindings/pci/rockchip-dw-pcie-common.yaml | 126 ++++++++++ > .../bindings/pci/rockchip-dw-pcie-ep.yaml | 95 ++++++++ > .../devicetree/bindings/pci/rockchip-dw-pcie.yaml | 93 +------ > .../devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 13 +- > arch/arm64/boot/dts/rockchip/Makefile | 5 + > .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso | 25 ++ > .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso | 16 ++ > arch/arm64/boot/dts/rockchip/rk3588.dtsi | 35 +++ > drivers/misc/pci_endpoint_test.c | 11 + > drivers/pci/controller/dwc/Kconfig | 17 +- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 267 +++++++++++++++++++-- > 11 files changed, 588 insertions(+), 115 deletions(-) > --- > base-commit: b452acb8fa6fc90851a93300eb0aaf89038a83d5 > change-id: 20240424-rockchip-pcie-ep-v1-87c78b16d53c > > Best regards, > -- > Niklas Cassel <cassel@kernel.org> >
On Tue, Apr 30, 2024 at 02:01:04PM +0200, Niklas Cassel wrote: > Fix the indentation of rockchip_pcie_{readl,writel}_apb() parameters to > match the opening parenthesis. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index d6842141d384..1993c430b90c 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -60,14 +60,13 @@ struct rockchip_pcie { > struct irq_domain *irq_domain; > }; > > -static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > - u32 reg) > +static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg) > { > return readl_relaxed(rockchip->apb_base + reg); > } > > -static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > - u32 val, u32 reg) > +static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, u32 val, > + u32 reg) > { > writel_relaxed(val, rockchip->apb_base + reg); > } > > -- > 2.44.0 >
On Tue, Apr 30, 2024 at 02:01:05PM +0200, Niklas Cassel wrote: > Add a rockchip_pcie_ltssm() helper function that reads the LTSSM status. > This helper will be used in additional places in follow-up patches. > Please don't use 'patches' in commit logs. Once the patches get merged, they become commits. > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index 1993c430b90c..4023fd86176f 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -143,6 +143,11 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip) > return 0; > } > > +static inline u32 rockchip_pcie_ltssm(struct rockchip_pcie *rockchip) rockchip_pcie_get_ltssm()? Also, no inline in C files, please. Compiler will inline functions with or without the keyword anyway. - Mani > +{ > + return rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); > +} > + > static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > { > rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM, > @@ -152,7 +157,7 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > static int rockchip_pcie_link_up(struct dw_pcie *pci) > { > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > - u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); > + u32 val = rockchip_pcie_ltssm(rockchip); > > if ((val & PCIE_LINKUP) == PCIE_LINKUP && > (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY) > > -- > 2.44.0 >
On Tue, Apr 30, 2024 at 02:01:06PM +0200, Niklas Cassel wrote: > This refactors the driver to prepare for EP mode. > Add of-match data to the existing compatible, and explicitly define it as > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up > patch in a much less intrusive way, which makes the follup-up patches > much easier to review. > Same comment as previous patch. > No functional change intended. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 72 +++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index 4023fd86176f..f985539fb00a 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -58,6 +58,11 @@ struct rockchip_pcie { > struct gpio_desc *rst_gpio; > struct regulator *vpcie3v3; > struct irq_domain *irq_domain; > + enum dw_pcie_device_mode mode; > +}; > + > +struct rockchip_pcie_of_data { > + enum dw_pcie_device_mode mode; > }; > > static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg) > @@ -195,7 +200,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > struct device *dev = rockchip->pci.dev; > - u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > int irq, ret; > > irq = of_irq_get_byname(dev->of_node, "legacy"); > @@ -209,12 +213,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler, > rockchip); > > - /* LTSSM enable control mode */ > - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > - > - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > - PCIE_CLIENT_GENERAL_CONTROL); > - > return 0; > } > > @@ -288,13 +286,41 @@ static const struct dw_pcie_ops dw_pcie_ops = { > .start_link = rockchip_pcie_start_link, > }; > > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip) > +{ > + struct dw_pcie_rp *pp; > + u32 val; > + > + if (!IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST)) > + return -ENODEV; Right now this driver is only selected using CONFIG_PCIE_ROCKCHIP_DW_HOST. So this check is not valid in _this_patch. - Mani
On Tue, Apr 30, 2024 at 02:01:07PM +0200, Niklas Cassel wrote: > The rockchip-dw-pcie.yaml device tree binding already defines > rockchip,rk3588-pcie as a supported compatible string. > > Add an explicit rockchip,rk3588-pcie entry to make it easier to find the > driver that implements this compatible string. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index f985539fb00a..f38d267e4e64 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -400,6 +400,10 @@ static const struct of_device_id rockchip_pcie_of_match[] = { > .compatible = "rockchip,rk3568-pcie", > .data = &rockchip_pcie_rc_of_data, > }, > + { > + .compatible = "rockchip,rk3588-pcie", > + .data = &rockchip_pcie_rc_of_data, This is not required. In fact, it is encouraged to just use fallback compatible in DT and not add new entries in driver. - Mani
On Tue, Apr 30, 2024 at 02:01:08PM +0200, Niklas Cassel wrote: > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode. > This endpoint mode support heavily leverages the existing code in > pcie-designware-ep.c. > > Add support for endpoint mode to the existing pcie-dw-rockchip glue > driver. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/pci/controller/dwc/Kconfig | 17 ++- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 177 ++++++++++++++++++++++++++ > 2 files changed, 191 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 8afacc90c63b..9fae0d977271 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP > SoCs. To compile this driver as a module, choose M here: the module > will be called pcie-rcar-gen4.ko. This uses the DesignWare core. > > +config PCIE_ROCKCHIP_DW > + bool > + > config PCIE_ROCKCHIP_DW_HOST > - bool "Rockchip DesignWare PCIe controller" > - select PCIE_DW > + bool "Rockchip DesignWare PCIe controller (host mode)" > select PCIE_DW_HOST > depends on PCI_MSI > depends on ARCH_ROCKCHIP || COMPILE_TEST > depends on OF > help > Enables support for the DesignWare PCIe controller in the > - Rockchip SoC except RK3399. > + Rockchip SoC (except RK3399) to work in host mode. Just curious. RK3399 is an exception because lack of driver support or it doesn't support EP mode at all? > + > +config PCIE_ROCKCHIP_DW_EP > + bool "Rockchip DesignWare PCIe controller (endpoint mode)" > + select PCIE_DW_EP > + depends on ARCH_ROCKCHIP || COMPILE_TEST > + depends on OF > + help > + Enables support for the DesignWare PCIe controller in the > + Rockchip SoC (except RK3399) to work in endpoint mode. > > config PCI_EXYNOS > tristate "Samsung Exynos PCIe controller" > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index f38d267e4e64..7614c20c7112 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -34,10 +34,16 @@ > #define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) > > #define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) > +#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0) > #define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) > +#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8) > +#define PCIE_CLIENT_INTR_STATUS_MISC 0x10 > +#define PCIE_CLIENT_INTR_MASK_MISC 0x24 > #define PCIE_SMLH_LINKUP BIT(16) > #define PCIE_RDLH_LINKUP BIT(17) > #define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP) > +#define PCIE_RDLH_LINK_UP_CHGED BIT(1) > +#define PCIE_LINK_REQ_RST_NOT_INT BIT(2) > #define PCIE_L0S_ENTRY 0x11 > #define PCIE_CLIENT_GENERAL_CONTROL 0x0 > #define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8 > @@ -159,6 +165,12 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > PCIE_CLIENT_GENERAL_CONTROL); > } > > +static void rockchip_pcie_disable_ltssm(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DISABLE_LTSSM, > + PCIE_CLIENT_GENERAL_CONTROL); > +} > + > static int rockchip_pcie_link_up(struct dw_pcie *pci) > { > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > @@ -195,6 +207,13 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci) > return 0; > } > > +static void rockchip_pcie_stop_link(struct dw_pcie *pci) > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + rockchip_pcie_disable_ltssm(rockchip); > +} > + > static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -220,6 +239,59 @@ static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { > .init = rockchip_pcie_host_init, > }; > > +static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + enum pci_barno bar; > + > + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) > + dw_pcie_ep_reset_bar(pci, bar); > +}; > + > +static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no, > + unsigned int type, u16 interrupt_num) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > + > + switch (type) { > + case PCI_IRQ_INTX: > + return dw_pcie_ep_raise_intx_irq(ep, func_no); > + case PCI_IRQ_MSI: > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > + case PCI_IRQ_MSIX: > + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > + default: > + dev_err(pci->dev, "UNKNOWN IRQ type\n"); > + } > + > + return 0; > +} > + > +static const struct pci_epc_features rockchip_pcie_epc_features = { > + .linkup_notifier = true, > + .msi_capable = true, > + .msix_capable = true, > + .align = SZ_64K, > + .bar[BAR_0] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > + .bar[BAR_1] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > + .bar[BAR_2] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > + .bar[BAR_3] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > + .bar[BAR_4] = { .type = BAR_RESERVED, }, You have documented the reason for this in cover letter. But it'd be good if you do the same in commit message also. > + .bar[BAR_5] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > +}; > + > +static const struct pci_epc_features * > +rockchip_pcie_get_features(struct dw_pcie_ep *ep) > +{ > + return &rockchip_pcie_epc_features; > +} > + > +static const struct dw_pcie_ep_ops rockchip_pcie_ep_ops = { > + .init = rockchip_pcie_ep_init, > + .raise_irq = rockchip_pcie_raise_irq, > + .get_features = rockchip_pcie_get_features, > +}; > + > static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > { > struct device *dev = rockchip->pci.dev; > @@ -284,8 +356,39 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) > static const struct dw_pcie_ops dw_pcie_ops = { > .link_up = rockchip_pcie_link_up, > .start_link = rockchip_pcie_start_link, > + .stop_link = rockchip_pcie_stop_link, > }; > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg) > +{ > + struct rockchip_pcie *rockchip = arg; > + struct dw_pcie *pci = &rockchip->pci; > + struct device *dev = pci->dev; > + u32 reg, val; > + > + reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC); > + > + dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg); > + dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_ltssm(rockchip)); > + > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > + dev_dbg(dev, "hot reset or link-down reset\n"); 'hot reset' means the host doing a hot reset? Rest LGTM! - Mani
On Tue, Apr 30, 2024 at 02:01:09PM +0200, Niklas Cassel wrote: > Rockchip rk3588 requires 64k alignment. > While there is an existing device_id:vendor_id in the driver with 64k > alignment, that device_id:vendor_id is am654, which uses BAR2 instead of > BAR0 as the test_reg_bar, and also has special is_am654_pci_dev() checks > in the driver to disallow BAR0. In order to allow testing all BARs, add a > new rk3588 entry in the driver. > > We intentionally do not add the vendor id to pci_ids.h, since the policy > for that file is that the vendor id has to be used by multiple drivers. > > Hopefully, this new entry will be short-lived, as there is a series on the > mailing list which intends to move the address alignment restrictions from > this driver to the endpoint side. > > Add a new entry for rk3588 in order to allow us to test all BARs. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/misc/pci_endpoint_test.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index c38a6083f0a7..a7f593b4e3b3 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -84,6 +84,9 @@ > #define PCI_DEVICE_ID_RENESAS_R8A774E1 0x0025 > #define PCI_DEVICE_ID_RENESAS_R8A779F0 0x0031 > > +#define PCI_VENDOR_ID_ROCKCHIP 0x1d87 > +#define PCI_DEVICE_ID_ROCKCHIP_RK3588 0x3588 > + > static DEFINE_IDA(pci_endpoint_test_ida); > > #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \ > @@ -980,6 +983,11 @@ static const struct pci_endpoint_test_data j721e_data = { > .irq_type = IRQ_TYPE_MSI, > }; > > +static const struct pci_endpoint_test_data rk3588_data = { > + .alignment = SZ_64K, > + .irq_type = IRQ_TYPE_MSI, > +}; > + > static const struct pci_device_id pci_endpoint_test_tbl[] = { > { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x), > .driver_data = (kernel_ulong_t)&default_data, > @@ -1017,6 +1025,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = { > { PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721S2), > .driver_data = (kernel_ulong_t)&j721e_data, > }, > + { PCI_DEVICE(PCI_VENDOR_ID_ROCKCHIP, PCI_DEVICE_ID_ROCKCHIP_RK3588), > + .driver_data = (kernel_ulong_t)&rk3588_data, > + }, > { } > }; > MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl); > > -- > 2.44.0 >
On Tue, Apr 30, 2024 at 02:01:10PM +0200, Niklas Cassel wrote: > Add a device tree node representing PCIe endpoint mode. > > The controller can either be configured to run in Root Complex or Endpoint > node. > > If a user wants to run the controller in endpoint mode, the user has to > disable the pcie3x4 node and enable the pcie3x4_ep node. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > arch/arm64/boot/dts/rockchip/rk3588.dtsi | 35 ++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi > index 5519c1430cb7..09a06e8c43b7 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi > @@ -136,6 +136,41 @@ pcie3x4_intc: legacy-interrupt-controller { > }; > }; > > + pcie3x4_ep: pcie-ep@fe150000 { > + compatible = "rockchip,rk3588-pcie-ep"; > + clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>, > + <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>, > + <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>; > + clock-names = "aclk_mst", "aclk_slv", > + "aclk_dbi", "pclk", > + "aux", "pipe"; > + interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>, > + <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>; > + interrupt-names = "sys", "pmc", "msg", "legacy", "err", > + "dma0", "dma1", "dma2", "dma3"; > + max-link-speed = <3>; > + num-lanes = <4>; > + phys = <&pcie30phy>; > + phy-names = "pcie-phy"; > + power-domains = <&power RK3588_PD_PCIE>; > + reg = <0xa 0x40000000 0x0 0x00100000>, > + <0xa 0x40100000 0x0 0x00100000>, > + <0x0 0xfe150000 0x0 0x00010000>, > + <0x9 0x00000000 0x0 0x40000000>, > + <0xa 0x40300000 0x0 0x00100000>; > + reg-names = "dbi", "dbi2", "apb", "addr_space", "atu"; Isn't it common to define 'reg' property just below 'compatible'? - Mani
On Tue, Apr 30, 2024 at 02:01:11PM +0200, Niklas Cassel wrote: > Add rock5b overlays for PCIe endpoint mode support. > I'm not aware of mainline using overlays. Is this a new one? - Mani > If using the rock5b as an endpoint against a normal PC, only the > rk3588-rock-5b-pcie-ep.dtbo needs to be applied. > > If using two rock5b:s, with one board as EP and the other board as RC, > rk3588-rock-5b-pcie-ep.dtbo and rk3588-rock-5b-pcie-srns.dtbo has to > be applied to the respective boards. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > arch/arm64/boot/dts/rockchip/Makefile | 5 +++++ > .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso | 25 ++++++++++++++++++++++ > .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso | 16 ++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile > index f906a868b71a..d827432d5111 100644 > --- a/arch/arm64/boot/dts/rockchip/Makefile > +++ b/arch/arm64/boot/dts/rockchip/Makefile > @@ -117,6 +117,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-nanopc-t6.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb > @@ -127,3 +129,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6s.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6c.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5.dtb > + > +# Enable support for device-tree overlays > +DTC_FLAGS_rk3588-rock-5b += -@ > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso > new file mode 100644 > index 000000000000..672d748fcc67 > --- /dev/null > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso > @@ -0,0 +1,25 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * DT-overlay to run the PCIe3_4L Dual Mode controller in Endpoint mode > + * in the SRNS (Separate Reference Clock No Spread) configuration. > + * > + * NOTE: If using a setup with two ROCK 5B:s, with one board running in > + * RC mode and the other board running in EP mode, see also the device > + * tree overlay: rk3588-rock-5b-pcie-srns.dtso. > + */ > + > +/dts-v1/; > +/plugin/; > + > +&pcie30phy { > + rockchip,rx-common-refclk-mode = <0 0 0 0>; > +}; > + > +&pcie3x4 { > + status = "disabled"; > +}; > + > +&pcie3x4_ep { > + vpcie3v3-supply = <&vcc3v3_pcie30>; > + status = "okay"; > +}; > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso > new file mode 100644 > index 000000000000..1a0f1af65c43 > --- /dev/null > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * DT-overlay to run the PCIe3_4L Dual Mode controller in Root Complex > + * mode in the SRNS (Separate Reference Clock No Spread) configuration. > + * > + * This device tree overlay is only needed (on the RC side) when running > + * a setup with two ROCK 5B:s, with one board running in RC mode and the > + * other board running in EP mode. > + */ > + > +/dts-v1/; > +/plugin/; > + > +&pcie30phy { > + rockchip,rx-common-refclk-mode = <0 0 0 0>; > +}; > > -- > 2.44.0 >
Am Samstag, 4. Mai 2024, 19:37:30 CEST schrieb Manivannan Sadhasivam: > On Tue, Apr 30, 2024 at 02:01:11PM +0200, Niklas Cassel wrote: > > Add rock5b overlays for PCIe endpoint mode support. > > > > I'm not aware of mainline using overlays. Is this a new one? I guess you could still call it new'ish ;-) But the mainline kernel does carry a number of overlays already [0] . This does of course not handle the actual application of overlays, which I guess bootloaders do only at this point. But I think it's definitely reasonable to carry them in a "central" location especially as for example u-boot uses the kernel as canonical source for most of its devicetrees. Heikoi [0] some random examples ;-) https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-g12a-fbx8am-realtek.dtso https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3-emmc.dtso https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-wifi.dtso > > If using the rock5b as an endpoint against a normal PC, only the > > rk3588-rock-5b-pcie-ep.dtbo needs to be applied. > > > > If using two rock5b:s, with one board as EP and the other board as RC, > > rk3588-rock-5b-pcie-ep.dtbo and rk3588-rock-5b-pcie-srns.dtbo has to > > be applied to the respective boards. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > arch/arm64/boot/dts/rockchip/Makefile | 5 +++++ > > .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso | 25 ++++++++++++++++++++++ > > .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso | 16 ++++++++++++++ > > 3 files changed, 46 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile > > index f906a868b71a..d827432d5111 100644 > > --- a/arch/arm64/boot/dts/rockchip/Makefile > > +++ b/arch/arm64/boot/dts/rockchip/Makefile > > @@ -117,6 +117,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-nanopc-t6.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb > > @@ -127,3 +129,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6s.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6c.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5.dtb > > + > > +# Enable support for device-tree overlays > > +DTC_FLAGS_rk3588-rock-5b += -@ > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso > > new file mode 100644 > > index 000000000000..672d748fcc67 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso > > @@ -0,0 +1,25 @@ > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > > +/* > > + * DT-overlay to run the PCIe3_4L Dual Mode controller in Endpoint mode > > + * in the SRNS (Separate Reference Clock No Spread) configuration. > > + * > > + * NOTE: If using a setup with two ROCK 5B:s, with one board running in > > + * RC mode and the other board running in EP mode, see also the device > > + * tree overlay: rk3588-rock-5b-pcie-srns.dtso. > > + */ > > + > > +/dts-v1/; > > +/plugin/; > > + > > +&pcie30phy { > > + rockchip,rx-common-refclk-mode = <0 0 0 0>; > > +}; > > + > > +&pcie3x4 { > > + status = "disabled"; > > +}; > > + > > +&pcie3x4_ep { > > + vpcie3v3-supply = <&vcc3v3_pcie30>; > > + status = "okay"; > > +}; > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso > > new file mode 100644 > > index 000000000000..1a0f1af65c43 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso > > @@ -0,0 +1,16 @@ > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > > +/* > > + * DT-overlay to run the PCIe3_4L Dual Mode controller in Root Complex > > + * mode in the SRNS (Separate Reference Clock No Spread) configuration. > > + * > > + * This device tree overlay is only needed (on the RC side) when running > > + * a setup with two ROCK 5B:s, with one board running in RC mode and the > > + * other board running in EP mode. > > + */ > > + > > +/dts-v1/; > > +/plugin/; > > + > > +&pcie30phy { > > + rockchip,rx-common-refclk-mode = <0 0 0 0>; > > +}; > > > >
Hello Mani, On Sat, May 04, 2024 at 10:35:37PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 30, 2024 at 02:00:57PM +0200, Niklas Cassel wrote: > > Hello all, > > > > This series adds PCIe endpoint mode support for the rockchip rk3588 and > > rk3568 SoCs. > > > > This series is based on: pci/next > > (git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git) > > > > This series has the following dependencies: > > 1) https://lore.kernel.org/linux-pci/20240430-pci-epf-rework-v4-0-22832d0d456f@linaro.org/ > > The series in 1) has not been merged to pci/next yet. > > > > 2) https://lore.kernel.org/linux-phy/20240412125818.17052-1-cassel@kernel.org/ > > The series in 2) has already been merged to phy/next. > > > > Even though this series (the series in $subject) has a runtime dependency > > on the changes that are currently queued in the PHY tree, there is no need > > to coordinate between the PCI tree and the PHY tree (i.e. this series can > > be merged via the PCI tree even for the coming merge window (v6.10-rc1)). > > > > This is because there is no compile time dependency between the changes in > > the PHY tree and this series. Likewise, the device tree overlays in this > > series passes "make CHECK_DTBS=y" even without the changes in the PHY tree. > > > > This series (including dependencies) can also be found in git: > > https://github.com/floatious/linux/commits/rockchip-pcie-ep-v2 > > > > Testing done: > > This series has been tested with two rock5b:s, one running in RC mode and > > one running in EP mode. This series has also been tested with an Intel x86 > > host and rock5b running in EP mode. > > > > BAR4 exposes the ATU Port Logic Structure and the DMA Port Logic Structure > > Is this for configuring the EP from host? Just curious. That is what I assume as well. (As I cannot come up with any other reason why they have done this.) > > pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: YES, Time: 0.000177625 s, Rate: 5764 KB/s > > pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: YES, Time: 0.000171208 s, Rate: 5986 KB/s > > pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: YES, Time: 0.000701167 s, Rate: 1460422 KB/s > > pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: YES, Time: 0.000702625 s, Rate: 1457393 KB/s > > > > Thanks a lot for sharing the pcitest results in the cover letter. Thank you for the review! Kind regards, Niklas
Hello Mani, On Sat, May 04, 2024 at 11:02:01PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 30, 2024 at 02:01:08PM +0200, Niklas Cassel wrote: > > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode. > > This endpoint mode support heavily leverages the existing code in > > pcie-designware-ep.c. > > > > Add support for endpoint mode to the existing pcie-dw-rockchip glue > > driver. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > drivers/pci/controller/dwc/Kconfig | 17 ++- > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 177 ++++++++++++++++++++++++++ > > 2 files changed, 191 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > > index 8afacc90c63b..9fae0d977271 100644 > > --- a/drivers/pci/controller/dwc/Kconfig > > +++ b/drivers/pci/controller/dwc/Kconfig > > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP > > SoCs. To compile this driver as a module, choose M here: the module > > will be called pcie-rcar-gen4.ko. This uses the DesignWare core. > > > > +config PCIE_ROCKCHIP_DW > > + bool > > + > > config PCIE_ROCKCHIP_DW_HOST > > - bool "Rockchip DesignWare PCIe controller" > > - select PCIE_DW > > + bool "Rockchip DesignWare PCIe controller (host mode)" > > select PCIE_DW_HOST > > depends on PCI_MSI > > depends on ARCH_ROCKCHIP || COMPILE_TEST > > depends on OF > > help > > Enables support for the DesignWare PCIe controller in the > > - Rockchip SoC except RK3399. > > + Rockchip SoC (except RK3399) to work in host mode. > > Just curious. RK3399 is an exception because lack of driver support or it > doesn't support EP mode at all? RK3399 is an exception because it uses a non-DWC PCIe controller. RK3399 has support for both RC and EP mode, see: CONFIG_PCIE_ROCKCHIP_HOST drivers/pci/controller/pcie-rockchip.c and CONFIG_PCIE_ROCKCHIP_EP drivers/pci/controller/pcie-rockchip-ep.c > > > + > > +config PCIE_ROCKCHIP_DW_EP > > + bool "Rockchip DesignWare PCIe controller (endpoint mode)" > > + select PCIE_DW_EP > > + depends on ARCH_ROCKCHIP || COMPILE_TEST > > + depends on OF > > + help > > + Enables support for the DesignWare PCIe controller in the > > + Rockchip SoC (except RK3399) to work in endpoint mode. > > > > config PCI_EXYNOS > > tristate "Samsung Exynos PCIe controller" > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > index f38d267e4e64..7614c20c7112 100644 > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > @@ -34,10 +34,16 @@ > > #define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) > > > > #define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) > > +#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0) > > #define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) > > +#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8) > > +#define PCIE_CLIENT_INTR_STATUS_MISC 0x10 > > +#define PCIE_CLIENT_INTR_MASK_MISC 0x24 > > #define PCIE_SMLH_LINKUP BIT(16) > > #define PCIE_RDLH_LINKUP BIT(17) > > #define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP) > > +#define PCIE_RDLH_LINK_UP_CHGED BIT(1) > > +#define PCIE_LINK_REQ_RST_NOT_INT BIT(2) > > #define PCIE_L0S_ENTRY 0x11 > > #define PCIE_CLIENT_GENERAL_CONTROL 0x0 > > #define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8 > > @@ -159,6 +165,12 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > > PCIE_CLIENT_GENERAL_CONTROL); > > } > > > > +static void rockchip_pcie_disable_ltssm(struct rockchip_pcie *rockchip) > > +{ > > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DISABLE_LTSSM, > > + PCIE_CLIENT_GENERAL_CONTROL); > > +} > > + > > static int rockchip_pcie_link_up(struct dw_pcie *pci) > > { > > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > > @@ -195,6 +207,13 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci) > > return 0; > > } > > > > +static void rockchip_pcie_stop_link(struct dw_pcie *pci) > > +{ > > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > > + > > + rockchip_pcie_disable_ltssm(rockchip); > > +} > > + > > static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > > { > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > @@ -220,6 +239,59 @@ static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { > > .init = rockchip_pcie_host_init, > > }; > > > > +static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + enum pci_barno bar; > > + > > + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) > > + dw_pcie_ep_reset_bar(pci, bar); > > +}; > > + > > +static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no, > > + unsigned int type, u16 interrupt_num) > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > > + > > + switch (type) { > > + case PCI_IRQ_INTX: > > + return dw_pcie_ep_raise_intx_irq(ep, func_no); > > + case PCI_IRQ_MSI: > > + return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > + case PCI_IRQ_MSIX: > > + return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > > + default: > > + dev_err(pci->dev, "UNKNOWN IRQ type\n"); > > + } > > + > > + return 0; > > +} > > + > > +static const struct pci_epc_features rockchip_pcie_epc_features = { > > + .linkup_notifier = true, > > + .msi_capable = true, > > + .msix_capable = true, > > + .align = SZ_64K, > > + .bar[BAR_0] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > > + .bar[BAR_1] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > > + .bar[BAR_2] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > > + .bar[BAR_3] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > > + .bar[BAR_4] = { .type = BAR_RESERVED, }, > > You have documented the reason for this in cover letter. But it'd be good if you > do the same in commit message also. Will do! It appears that rk3568 does not have ATU and DMA regs mapped to BAR4 (or any other BAR). I will create a separate "static const struct pci_epc_features" for rk3568. > > > + .bar[BAR_5] = { .type = BAR_FIXED, .fixed_size = SZ_1M, }, > > +}; > > + > > +static const struct pci_epc_features * > > +rockchip_pcie_get_features(struct dw_pcie_ep *ep) > > +{ > > + return &rockchip_pcie_epc_features; > > +} > > + > > +static const struct dw_pcie_ep_ops rockchip_pcie_ep_ops = { > > + .init = rockchip_pcie_ep_init, > > + .raise_irq = rockchip_pcie_raise_irq, > > + .get_features = rockchip_pcie_get_features, > > +}; > > + > > static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > > { > > struct device *dev = rockchip->pci.dev; > > @@ -284,8 +356,39 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) > > static const struct dw_pcie_ops dw_pcie_ops = { > > .link_up = rockchip_pcie_link_up, > > .start_link = rockchip_pcie_start_link, > > + .stop_link = rockchip_pcie_stop_link, > > }; > > > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg) > > +{ > > + struct rockchip_pcie *rockchip = arg; > > + struct dw_pcie *pci = &rockchip->pci; > > + struct device *dev = pci->dev; > > + u32 reg, val; > > + > > + reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC); > > + > > + dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg); > > + dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_ltssm(rockchip)); > > + > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > > + dev_dbg(dev, "hot reset or link-down reset\n"); > > 'hot reset' means the host doing a hot reset? Hot reset means that LTSSM state machine is in state "Hot Reset". I don't know all the reasons why the state machine can go to this state by heart, but from the Databook: "A Downstream Port (DSP) can hot reset an Upstream Port (USP) by sending two consecutive TS1 ordered sets with the hot reset bit asserted" "There is no difference in the handling of a link-down reset or a hot reset; the controller asserts the link_req_rst_not output requesting the DWC_pcie_clkrst.v module to reset the controller." Kind regards, Niklas
Hello Mani, On Sat, May 04, 2024 at 11:04:20PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 30, 2024 at 02:01:10PM +0200, Niklas Cassel wrote: > > Add a device tree node representing PCIe endpoint mode. > > > > The controller can either be configured to run in Root Complex or Endpoint > > node. > > > > If a user wants to run the controller in endpoint mode, the user has to > > disable the pcie3x4 node and enable the pcie3x4_ep node. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > arch/arm64/boot/dts/rockchip/rk3588.dtsi | 35 ++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi > > index 5519c1430cb7..09a06e8c43b7 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi > > @@ -136,6 +136,41 @@ pcie3x4_intc: legacy-interrupt-controller { > > }; > > }; > > > > + pcie3x4_ep: pcie-ep@fe150000 { > > + compatible = "rockchip,rk3588-pcie-ep"; > > + clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>, > > + <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>, > > + <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>; > > + clock-names = "aclk_mst", "aclk_slv", > > + "aclk_dbi", "pclk", > > + "aux", "pipe"; > > + interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>, > > + <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>; > > + interrupt-names = "sys", "pmc", "msg", "legacy", "err", > > + "dma0", "dma1", "dma2", "dma3"; > > + max-link-speed = <3>; > > + num-lanes = <4>; > > + phys = <&pcie30phy>; > > + phy-names = "pcie-phy"; > > + power-domains = <&power RK3588_PD_PCIE>; > > + reg = <0xa 0x40000000 0x0 0x00100000>, > > + <0xa 0x40100000 0x0 0x00100000>, > > + <0x0 0xfe150000 0x0 0x00010000>, > > + <0x9 0x00000000 0x0 0x40000000>, > > + <0xa 0x40300000 0x0 0x00100000>; > > + reg-names = "dbi", "dbi2", "apb", "addr_space", "atu"; > > Isn't it common to define 'reg' property just below 'compatible'? Looking at the result from: $ git grep -A 4 "compatible = \"" Documentation/devicetree/bindings/pci/ Common, yes, but far from all examples in Documentation/devicetree/bindings/pci/ do it that way. The example in the yaml for this binding passes "make dt_binding_check". If the device tree maintainers had a strong opinion on this, I would have expected "make dt_binding_check" to emit a warning or error for this. Additionally, the "pcie3x4" (RC-node) in rk3588.dtsi already use this same ordering. I do think there is some value in keeping the ordering in "pcie3x4" and "pcie3x4_ep" the same, so I will keep the ordering unless the device tree maintainers start screaming at me :) Kind regards, Niklas
On Sun, May 05, 2024 at 02:14:45PM +0200, Heiko Stübner wrote: > Am Samstag, 4. Mai 2024, 19:37:30 CEST schrieb Manivannan Sadhasivam: > > On Tue, Apr 30, 2024 at 02:01:11PM +0200, Niklas Cassel wrote: > > > Add rock5b overlays for PCIe endpoint mode support. > > > > > > > I'm not aware of mainline using overlays. Is this a new one? > > I guess you could still call it new'ish ;-) > > But the mainline kernel does carry a number of overlays already [0] . > This does of course not handle the actual application of overlays, > which I guess bootloaders do only at this point. Yes, AFAICT, the actual application is handled by bootloaders. (Well, there is a "Overlay in-kernel API", but not a user API.) > > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile > > > index f906a868b71a..d827432d5111 100644 > > > --- a/arch/arm64/boot/dts/rockchip/Makefile > > > +++ b/arch/arm64/boot/dts/rockchip/Makefile > > > @@ -117,6 +117,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-nanopc-t6.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb > > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo > > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb > > > @@ -127,3 +129,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6s.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6c.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb > > > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5.dtb > > > + > > > +# Enable support for device-tree overlays > > > +DTC_FLAGS_rk3588-rock-5b += -@ Note that this DTC compiler flag is needed for the overlay for the bootloader to be able to apply the overlay successfully. (In order to supply the .dtbo compiled by the kernel directly to u-boot.) I'm not sure how the other rockchip overlays compiled by the kernel can work when suppling them to u-boot. I'm guessing that they compile them externally/manually outside the source tree with the -@ flag enabled. If we look at other arm64 device tree overlays in the kernel, e.g. TI, they seem to supply -@ for all their .dtso files: $ git grep -- "-@" arch/arm64/ Kind regards, Niklas
Hello Mani, On Sat, May 04, 2024 at 10:43:46PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 30, 2024 at 02:01:05PM +0200, Niklas Cassel wrote: > > Add a rockchip_pcie_ltssm() helper function that reads the LTSSM status. > > This helper will be used in additional places in follow-up patches. > > > > Please don't use 'patches' in commit logs. Once the patches get merged, they > become commits. Sure, will fix in V2. > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > index 1993c430b90c..4023fd86176f 100644 > > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > > @@ -143,6 +143,11 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip) > > return 0; > > } > > > > +static inline u32 rockchip_pcie_ltssm(struct rockchip_pcie *rockchip) > > rockchip_pcie_get_ltssm()? Sure, will fix in V2. > > Also, no inline in C files, please. Compiler will inline functions with or > without the keyword anyway. Sure, will fix in V2. Kind regards, Niklas