On Fri, Mar 10, 2023 at 09:34:58PM +0900, Yoshihiro Shimoda wrote:
> The "val" of PCIE_PORT_LINK_CONTROL will be reused on the
> "Set the number of lanes". But, if snps,enable-cdm-check" exists,
> the "val" will be set to PCIE_PL_CHK_REG_CONTROL_STATUS.
> Therefore, unexpected register value is possible to be used
> to PCIE_PORT_LINK_CONTROL register if snps,enable-cdm-check" exists.
> So, change reading timing of PCIE_PORT_LINK_CONTROL register to fix
> the issue.
My version of the commit log:
< If CDM_CHECK capability is set then the local variable 'val' will be
< overwritten in the dw_pcie_setup() method in the PL_CHK register
< initialization procedure. Thus further variable usage in the framework of
< the PCIE_PORT_LINK_CONTROL register initialization at the very least must
< imply the variable re-initialization. Alas it hasn't been taken into
< account in the commit ec7b952f453c ("PCI: dwc: Always enable CDM check if
< "snps,enable-cdm-check" exists"). Due to that the PCIE_PORT_LINK_CONTROL
< register will be written with an improper value in case if the CDM-check
< is enabled. Let's fix this by moving the PCIE_PORT_LINK_CONTROL CSR
< updated to be fully performed after the PL_CHK register
< initialization.
>
> Fixes: ec7b952f453c ("PCI: dwc: Always enable CDM check if "snps,enable-cdm-check" exists")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
Looks good. Thanks.
Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
@Bjorn, if it's possible could you please take this patch to a
fixes(-ish) branch of your tree and merge it in the next rc-cycle?
It definitely fixes a bug in the DW PCIe core driver.
-Serge(y)
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 53a16b8b6ac2..8e33e6e59e68 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1001,11 +1001,6 @@ void dw_pcie_setup(struct dw_pcie *pci)
> dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> }
>
> - val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> - val &= ~PORT_LINK_FAST_LINK_MODE;
> - val |= PORT_LINK_DLL_LINK_EN;
> - dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> -
> if (dw_pcie_cap_is(pci, CDM_CHECK)) {
> val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> val |= PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS |
> @@ -1013,6 +1008,11 @@ void dw_pcie_setup(struct dw_pcie *pci)
> dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> }
>
> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> + val &= ~PORT_LINK_FAST_LINK_MODE;
> + val |= PORT_LINK_DLL_LINK_EN;
> + dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> +
> if (!pci->num_lanes) {
> dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> return;
> --
> 2.25.1
>
>
