Re: [PATCH 04/13] PCI: cadence: Add support to start link and verify link status

[Andrew Murray <andrew.murray@arm.com>]

On Mon, Dec 09, 2019 at 02:51:38PM +0530, Kishon Vijay Abraham I wrote:
> Add cdns_pcie_ops to start link and verify link status. The registers
> to start link and to check link status is in Platform specific PCIe
> wrapper. Add support for platform specific drivers to add callback
> functions for the PCIe Cadence core to start link and verify link status.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  .../pci/controller/cadence/pcie-cadence-ep.c  |  8 ++++++
>  .../controller/cadence/pcie-cadence-host.c    | 28 +++++++++++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h | 23 +++++++++++++++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 560f22b4d165..088394b6be04 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -355,8 +355,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> +	struct device *dev = pcie->dev;
>  	struct pci_epf *epf;
>  	u32 cfg;
> +	int ret;
>  
>  	/*
>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
> @@ -367,6 +369,12 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  		cfg |= BIT(epf->func_no);
>  	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, cfg);
>  
> +	ret = cdns_pcie_start_link(pcie, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index ccf55e143e1d..0929554f5a81 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -3,6 +3,7 @@
>  // Cadence PCIe host controller driver.
>  // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>  
> +#include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
> @@ -201,6 +202,23 @@ static int cdns_pcie_host_init(struct device *dev,
>  	return err;
>  }
>  
> +static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	int retries;
> +
> +	/* Check if the link is up or not */
> +	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> +		if (cdns_pcie_is_link_up(pcie)) {
> +			dev_info(dev, "Link up\n");
> +			return 0;
> +		}
> +		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> +	}
> +
> +	return -ETIMEDOUT;
> +}

This patch looks fine, except this function (above) is identical to
dw_pcie_wait_for_link, advk_pcie_wait_for_link and nwl_wait_for_link. Even
the definitions of LINK_WAIT_USLEEP_xx are the same.

I don't see any justification to duplicating this again - can you consolidate
these functions to something that all controller drivers can use?

Thanks,

Andrew Murray

> +
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  {
>  	struct device *dev = rc->pcie.dev;
> @@ -254,6 +272,16 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
>  
>  	pcie->mem_res = res;
>  
> +	ret = cdns_pcie_start_link(pcie, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
> +	ret = cdns_pcie_host_wait_for_link(pcie);
> +	if (ret)
> +		dev_dbg(dev, "PCIe link never came up\n");
> +
>  	ret = cdns_pcie_host_init(dev, &resources, rc);
>  	if (ret)
>  		goto err_init;
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index d0d91c69fa1d..f0395eaf9df5 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -10,6 +10,11 @@
>  #include <linux/pci.h>
>  #include <linux/phy/phy.h>
>  
> +/* Parameters for the waiting for link up routine */
> +#define LINK_WAIT_MAX_RETRIES	10
> +#define LINK_WAIT_USLEEP_MIN	90000
> +#define LINK_WAIT_USLEEP_MAX	100000
> +
>  /*
>   * Local Management Registers
>   */
> @@ -226,6 +231,8 @@ enum cdns_pcie_msg_routing {
>  struct cdns_pcie_ops {
>  	u32	(*read)(void __iomem *addr, int size);
>  	void	(*write)(void __iomem *addr, int size, u32 value);
> +	int	(*start_link)(struct cdns_pcie *pcie, bool start);
> +	bool	(*is_link_up)(struct cdns_pcie *pcie);
>  };
>  
>  /**
> @@ -447,6 +454,22 @@ static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg)
>  	return readl(addr);
>  }
>  
> +static inline int cdns_pcie_start_link(struct cdns_pcie *pcie, bool start)
> +{
> +	if (pcie->ops->start_link)
> +		return pcie->ops->start_link(pcie, start);
> +
> +	return 0;
> +}
> +
> +static inline bool cdns_pcie_is_link_up(struct cdns_pcie *pcie)
> +{
> +	if (pcie->ops->is_link_up)
> +		return pcie->ops->is_link_up(pcie);
> +
> +	return true;
> +}
> +
>  #ifdef CONFIG_PCIE_CADENCE_HOST
>  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
>  #else
> -- 
> 2.17.1
>