| Message ID | 20241007041218.157516-9-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/12] PCI: rockchip-ep: Fix address translation unit programming | expand |
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 >
On 10/10/24 17:22, Manivannan Sadhasivam wrote: > 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... Yes, I think that was a mistake, likely when the EP mode support was added. >> 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. Indeed. Removed. It does not seem to hurt though.
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); - 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;
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. 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(). 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(-)