Re: [PATCH v2 11/14] PCI: dw-rockchip: Add endpoint mode support

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

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