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

Manivannan Sadhasivam
2024-10-10T08:22:30Z

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
>

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