Re: [PATCH v11 01/13] PCI: dwc: Fix writing wrong value if snps,enable-cdm-check

[Serge Semin <fancer.lancer@gmail.com>]

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@renesas.com>

Looks good. Thanks.
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

@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
> 
>