Message ID | 20240529-rockchip-pcie-ep-v1-v4-0-3dc00fe21a78@kernel.org |
---|---|
Headers | show |
Series | PCI: dw-rockchip: Add endpoint mode support | expand |
Hi Niklas, On 2024/5/29 16:28, 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 can also be found in git: > https://github.com/floatious/linux/commits/rockchip-pcie-ep-v4 > > 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. I'm interesting how you test in Intel x86 host, PC scan the PCIe device in BIOS, do you power on the EP before PC power on? Thanks, - Kever > > BAR4 exposes the ATU Port Logic Structure and the DMA Port Logic Structure > 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 > > Kind regards, > Niklas > > --- > Changes in v4: > - Rebased on pci/next > - Link to v3: https://lore.kernel.org/r/20240508-rockchip-pcie-ep-v1-v3-0-1748e202b084@kernel.org > > Changes in v3: > - Renamed rockchip_pcie_ltssm() to rockchip_pcie_get_ltssm() > - Reworded some commit messages to avoid the term "patches". > - Dropped patch that added explicit rockchip,rk3588-pcie compatible > - Moved !IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST) check to proper patch. > - Added comment in front of rk3588 epc_features describing why BAR4 is > reserved. > - Added another struct epc_features for rk3568 as it does not have the > ATU regs mapped to BAR4 (like rk3588 does). > - Picked up tags from Rob and Mani. Thank you! > - Link to v2: https://lore.kernel.org/r/20240430-rockchip-pcie-ep-v1-v2-0-a0f5ee2a77b6@kernel.org > > 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 (13): > 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_get_ltssm() helper > PCI: dw-rockchip: Refactor the driver to prepare for EP mode > 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 | 308 +++++++++++++++++++-- > 11 files changed, 620 insertions(+), 124 deletions(-) > --- > base-commit: e3fca37312892122d73f8c5293c0d1cc8c34500b > change-id: 20240424-rockchip-pcie-ep-v1-87c78b16d53c > > Best regards,
On 6/4/24 10:45, Kever Yang wrote: > Hi Niklas, > > On 2024/5/29 16:28, 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 can also be found in git: >> https://github.com/floatious/linux/commits/rockchip-pcie-ep-v4 >> >> 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. > > I'm interesting how you test in Intel x86 host, PC scan the PCIe device > in BIOS, do you > > power on the EP before PC power on? Yes. Same with any host, not just x86.
On Wed, May 29, 2024 at 10:29:02AM +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 commits. > > 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, 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 3dfed08ef456..1380e3a5284b 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 u32 rockchip_pcie_get_ltssm(struct rockchip_pcie *rockchip) > +{ > + 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_get_ltssm(rockchip); > > if ((val & PCIE_LINKUP) == PCIE_LINKUP && > (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY) > > -- > 2.45.1 >
On Wed, May 29, 2024 at 10:29:03AM +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 > commit in a much less intrusive way, which makes the follup-up commit much > easier to review. > > No functional change intended. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Few nitpicks below. With those addressed, Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 84 +++++++++++++++++++-------- > 1 file changed, 60 insertions(+), 24 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index 1380e3a5284b..e133511692af 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -49,15 +49,20 @@ > #define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0) > > struct rockchip_pcie { > - struct dw_pcie pci; > - void __iomem *apb_base; > - struct phy *phy; > - struct clk_bulk_data *clks; > - unsigned int clk_cnt; > - struct reset_control *rst; > - struct gpio_desc *rst_gpio; > - struct regulator *vpcie3v3; > - struct irq_domain *irq_domain; > + struct dw_pcie pci; > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_control *rst; > + struct gpio_desc *rst_gpio; > + struct regulator *vpcie3v3; > + struct irq_domain *irq_domain; > + const struct rockchip_pcie_of_data *data; I prefer to avoid aligning the struct members just for this reason. Once you add a member with a bigger name, then you need to align others also. Please just use space. > +}; > + > +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; > } > > @@ -294,13 +292,35 @@ 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; > + > + /* LTSSM enable control mode */ > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > + > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > + PCIE_CLIENT_GENERAL_CONTROL); > + > + pp = &rockchip->pci.pp; > + pp->ops = &rockchip_pcie_host_ops; > + > + return dw_pcie_host_init(pp); > +} > + > static int rockchip_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct rockchip_pcie *rockchip; > - struct dw_pcie_rp *pp; > + const struct rockchip_pcie_of_data *data; > int ret; > > + data = of_device_get_match_data(dev); > + if (!data) > + return -EINVAL; -ENODATA? > + > rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); > if (!rockchip) > return -ENOMEM; > @@ -309,9 +329,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > > rockchip->pci.dev = dev; > rockchip->pci.ops = &dw_pcie_ops; > - > - pp = &rockchip->pci.pp; > - pp->ops = &rockchip_pcie_host_ops; > + rockchip->data = data; > > ret = rockchip_pcie_resource_get(pdev, rockchip); > if (ret) > @@ -347,10 +365,21 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > if (ret) > goto deinit_phy; > > - ret = dw_pcie_host_init(pp); > - if (!ret) > - return 0; Thanks a lot for getting rid of this ugly piece of code! - Mani
On Wed, May 29, 2024 at 10:29:04AM +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> Couple of comments below. With those addressed, Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/Kconfig | 17 ++- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++ > 2 files changed, 224 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 Where is this symbol used? > + > 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. > + > +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 e133511692af..347721207161 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c [...] > +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_get_ltssm(rockchip)); > + > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > + dev_dbg(dev, "hot reset or link-down reset\n"); > + dw_pcie_ep_linkdown(&pci->ep); > + } > + > + if (reg & PCIE_RDLH_LINK_UP_CHGED) { > + val = rockchip_pcie_get_ltssm(rockchip); > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) { > + dev_dbg(dev, "link up\n"); > + dw_pcie_ep_linkup(&pci->ep); > + } > + } > + > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC); It is recommended to clear the IRQs at the start of the handler (after status read). - Mani
On Wed, May 29, 2024 at 10:29:06AM +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> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> One nitpick below. > --- > 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 5984016b5f96..6b5bf1055143 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi > @@ -186,6 +186,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"; As suggested in the bindings patch, please move these reg properties below compatible. - Mani > + resets = <&cru SRST_PCIE0_POWER_UP>, <&cru SRST_P_PCIE0>; > + reset-names = "pwr", "pipe"; > + status = "disabled"; > + }; > + > pcie3x2: pcie@fe160000 { > compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie"; > #address-cells = <3>; > > -- > 2.45.1 >
On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote: > On Wed, May 29, 2024 at 10:29:03AM +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 > > commit in a much less intrusive way, which makes the follup-up commit much > > easier to review. > > > > No functional change intended. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > Few nitpicks below. With those addressed, > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- (snip) > > @@ -294,13 +292,35 @@ 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; > > + > > + /* LTSSM enable control mode */ > > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > > + > > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > > + PCIE_CLIENT_GENERAL_CONTROL); > > + > > + pp = &rockchip->pci.pp; > > + pp->ops = &rockchip_pcie_host_ops; > > + > > + return dw_pcie_host_init(pp); > > +} > > + > > static int rockchip_pcie_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct rockchip_pcie *rockchip; > > - struct dw_pcie_rp *pp; > > + const struct rockchip_pcie_of_data *data; > > int ret; > > > > + data = of_device_get_match_data(dev); > > + if (!data) > > + return -EINVAL; > > -ENODATA? -EINVAL seems to be most common: $ git grep -A 5 of_device_get_match_data drivers/pci/ Kind regards, Niklas
On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote: > On Wed, May 29, 2024 at 10:29:04AM +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> > > Couple of comments below. With those addressed, > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > drivers/pci/controller/dwc/Kconfig | 17 ++- > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++ > > 2 files changed, 224 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 > > Where is this symbol used? It is supposed to be used by drivers/pci/controller/dwc/Makefile such that the driver is compiled if either _EP or _HOST is selected, just like how it is done for other drivers that support both in the same driver. Looks like I missed to update Makefile... Good catch, thank you! > > +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_get_ltssm(rockchip)); > > + > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > > + dev_dbg(dev, "hot reset or link-down reset\n"); > > + dw_pcie_ep_linkdown(&pci->ep); > > + } > > + > > + if (reg & PCIE_RDLH_LINK_UP_CHGED) { > > + val = rockchip_pcie_get_ltssm(rockchip); > > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) { > > + dev_dbg(dev, "link up\n"); > > + dw_pcie_ep_linkup(&pci->ep); > > + } > > + } > > + > > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC); > > It is recommended to clear the IRQs at the start of the handler (after status > read). Can you quote a reference in the databook to back this recommendation? Otherwise I would lean towards keeping it like it is, since this is how it looks in the downstream driver (that *should* be well proven), and it also matches how it's done in dra7xx. (And since you ack only the events you read, you can not accidentally clear another type of event.) Kind regards, Niklas
On Wed, Jun 05, 2024 at 07:57:12PM +0200, Niklas Cassel wrote: > On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote: > > On Wed, May 29, 2024 at 10:29:03AM +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 > > > commit in a much less intrusive way, which makes the follup-up commit much > > > easier to review. > > > > > > No functional change intended. > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > > Few nitpicks below. With those addressed, > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > (snip) > > > > @@ -294,13 +292,35 @@ 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; > > > + > > > + /* LTSSM enable control mode */ > > > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > > > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > > > + > > > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > > > + PCIE_CLIENT_GENERAL_CONTROL); > > > + > > > + pp = &rockchip->pci.pp; > > > + pp->ops = &rockchip_pcie_host_ops; > > > + > > > + return dw_pcie_host_init(pp); > > > +} > > > + > > > static int rockchip_pcie_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > struct rockchip_pcie *rockchip; > > > - struct dw_pcie_rp *pp; > > > + const struct rockchip_pcie_of_data *data; > > > int ret; > > > > > > + data = of_device_get_match_data(dev); > > > + if (!data) > > > + return -EINVAL; > > > > -ENODATA? > > -EINVAL seems to be most common: > $ git grep -A 5 of_device_get_match_data drivers/pci/ > Yeah, but we abused -EINVAL a lot ;) Nowadays, I prefer to use more apt error codes. - Mani
On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote: > On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote: > > On Wed, May 29, 2024 at 10:29:04AM +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> > > > > Couple of comments below. With those addressed, > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > > drivers/pci/controller/dwc/Kconfig | 17 ++- > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++ > > > 2 files changed, 224 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 > > > > Where is this symbol used? > > It is supposed to be used by > drivers/pci/controller/dwc/Makefile > > such that the driver is compiled if either _EP or _HOST is selected, just > like how it is done for other drivers that support both in the same driver. > Looks like I missed to update Makefile... > Good catch, thank you! > > > > > +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_get_ltssm(rockchip)); > > > + > > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > > > + dev_dbg(dev, "hot reset or link-down reset\n"); > > > + dw_pcie_ep_linkdown(&pci->ep); > > > + } > > > + > > > + if (reg & PCIE_RDLH_LINK_UP_CHGED) { > > > + val = rockchip_pcie_get_ltssm(rockchip); > > > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) { > > > + dev_dbg(dev, "link up\n"); > > > + dw_pcie_ep_linkup(&pci->ep); > > > + } > > > + } > > > + > > > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC); > > > > It is recommended to clear the IRQs at the start of the handler (after status > > read). > > Can you quote a reference in the databook to back this recommendation? > It is just a general recommendation. > Otherwise I would lean towards keeping it like it is, since this is how > it looks in the downstream driver (that *should* be well proven), and it > also matches how it's done in dra7xx. > > (And since you ack only the events you read, you can not accidentally > clear another type of event.) > I haven't read the TRM, but if the IRQ line is level triggered, then if you do not clear the IRQs immediately, you will miss some events. So I always suggest to clear the IRQs at the start of the handler for all the platforms. - Mani
On Thu, Jun 06, 2024 at 12:01:28PM +0530, Manivannan Sadhasivam wrote: > On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote: > > On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote: > > > On Wed, May 29, 2024 at 10:29:04AM +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> > > > > > > Couple of comments below. With those addressed, > > > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > > --- > > > > drivers/pci/controller/dwc/Kconfig | 17 ++- > > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++ > > > > 2 files changed, 224 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 > > > > > > Where is this symbol used? > > > > It is supposed to be used by > > drivers/pci/controller/dwc/Makefile > > > > such that the driver is compiled if either _EP or _HOST is selected, just > > like how it is done for other drivers that support both in the same driver. > > Looks like I missed to update Makefile... > > Good catch, thank you! > > > > > > > > +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_get_ltssm(rockchip)); > > > > + > > > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > > > > + dev_dbg(dev, "hot reset or link-down reset\n"); > > > > + dw_pcie_ep_linkdown(&pci->ep); > > > > + } > > > > + > > > > + if (reg & PCIE_RDLH_LINK_UP_CHGED) { > > > > + val = rockchip_pcie_get_ltssm(rockchip); > > > > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) { > > > > + dev_dbg(dev, "link up\n"); > > > > + dw_pcie_ep_linkup(&pci->ep); > > > > + } > > > > + } > > > > + > > > > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC); > > > > > > It is recommended to clear the IRQs at the start of the handler (after status > > > read). > > > > Can you quote a reference in the databook to back this recommendation? > > > > It is just a general recommendation. > > > Otherwise I would lean towards keeping it like it is, since this is how > > it looks in the downstream driver (that *should* be well proven), and it > > also matches how it's done in dra7xx. > > > > (And since you ack only the events you read, you can not accidentally > > clear another type of event.) > > > > I haven't read the TRM, but if the IRQ line is level triggered, then if you do > not clear the IRQs immediately, you will miss some events. So I always suggest > to clear the IRQs at the start of the handler for all the platforms. They are level triggered. In this specific case, what could happen is that we fail to trigger an IRQ if we get two succeeding hot/link-down resets, or two succeding link ups. In neither case is this a serious case (compared to e.g. a host driver missing a MSI irq), but I have incorporated your suggestion for v5, thank you! Kind regards, Niklas