Re: [PATCH v3 08/12] PCI: rockchip-ep: Refactor endpoint link training enable

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

On Mon, Oct 07, 2024 at 01:12:14PM +0900, Damien Le Moal wrote:
> The function rockchip_pcie_init_port() enables link training for a
> controller configured in EP mode. Enabling link training is again done
> in rockchip_pcie_ep_probe() after that function executed
> rockchip_pcie_init_port(). Enabling link training only needs to be done
> once, and doing so at the probe stage before the controller is actually
> started by the user serves no purpose.
> 

I hope that the dual enablement is done as a mistake and not on purpose...

> Refactor this by removing the link training enablement from both
> rockchip_pcie_init_port() and rockchip_pcie_ep_probe() and moving it to
> the endpoint start operation defined with rockchip_pcie_ep_start().
> Enabling the controller configuration using the PCIE_CLIENT_CONF_ENABLE
> bit in the same PCIE_CLIENT_CONFIG register is also move to
> rockchip_pcie_ep_start() and both the controller configuration and link
> training enable bits are set with a single call to
> rockchip_pcie_write().
> 

But you didn't remove the existing code in probe() that sets
PCIE_CLIENT_CONF_ENABLE.

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 11 ++++++-----
>  drivers/pci/controller/pcie-rockchip.c    |  5 +++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index 7a1798fcc2ad..99f26f4a485b 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -459,6 +459,12 @@ static int rockchip_pcie_ep_start(struct pci_epc *epc)
>  
>  	rockchip_pcie_write(rockchip, cfg, PCIE_CORE_PHY_FUNC_CFG);
>  
> +	/* Enable configuration and start link training */
> +	rockchip_pcie_write(rockchip,
> +			    PCIE_CLIENT_LINK_TRAIN_ENABLE |
> +			    PCIE_CLIENT_CONF_ENABLE,
> +			    PCIE_CLIENT_CONFIG);
> +
>  	return 0;
>  }
>  
> @@ -537,7 +543,6 @@ static int rockchip_pcie_ep_get_resources(struct rockchip_pcie_ep *ep)
>  
>  	ep->ob_addr = devm_kcalloc(dev, ep->max_regions, sizeof(*ep->ob_addr),
>  				   GFP_KERNEL);
> -

Spurious change.

- Mani

>  	if (!ep->ob_addr)
>  		return -ENOMEM;
>  
> @@ -648,10 +653,6 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
>  
>  	rockchip_pcie_ep_hide_msix_cap(rockchip);
>  
> -	/* Establish the link automatically */
> -	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> -			    PCIE_CLIENT_CONFIG);
> -
>  	/* Only enable function 0 by default */
>  	rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
>  
> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index c07d7129f1c7..154e78819e6e 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -244,11 +244,12 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
>  				    PCIE_CLIENT_CONFIG);
>  
> -	regs = PCIE_CLIENT_LINK_TRAIN_ENABLE | PCIE_CLIENT_ARI_ENABLE |
> +	regs = PCIE_CLIENT_ARI_ENABLE |
>  	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);
>  
>  	if (rockchip->is_rc)
> -		regs |= PCIE_CLIENT_CONF_ENABLE | PCIE_CLIENT_MODE_RC;
> +		regs |= PCIE_CLIENT_LINK_TRAIN_ENABLE |
> +			PCIE_CLIENT_CONF_ENABLE | PCIE_CLIENT_MODE_RC;
>  	else
>  		regs |= PCIE_CLIENT_CONF_DISABLE | PCIE_CLIENT_MODE_EP;
>  
> -- 
> 2.46.2
> 

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