Message ID | 20230310123510.675685-2-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
---|---|
State | Superseded |
Commit | cdce67099117ece371582f706c6eff7d3a65326d |
Delegated to: | Krzysztof WilczyĆski |
Headers | show |
Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
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 > >
On Tue, Mar 21, 2023 at 12:02:55PM +0300, Serge Semin wrote: > 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. I applied this patch only to for-linus for v6.3. I adapted the commit message as follows, let me know if you spot a mistake: PCI: dwc: Fix PORT_LINK_CONTROL update when CDM check enabled If CDM_CHECK is enabled (by the DT "snps,enable-cdm-check" property), 'val' is overwritten by PCIE_PL_CHK_REG_CONTROL_STATUS initialization. Commit ec7b952f453c ("PCI: dwc: Always enable CDM check if "snps,enable-cdm-check" exists") did not account for further usage of 'val', so we wrote improper values to PCIE_PORT_LINK_CONTROL when the CDM check is enabled. Move the PCIE_PORT_LINK_CONTROL update to be completely after the PCIE_PL_CHK_REG_CONTROL_STATUS register initialization. [bhelgaas: commit log adapted from Serge's version] > > --- > > 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 > > > >
On Tue, Mar 21, 2023 at 01:52:28PM -0500, Bjorn Helgaas wrote: > On Tue, Mar 21, 2023 at 12:02:55PM +0300, Serge Semin wrote: > > 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. > > I applied this patch only to for-linus for v6.3. I adapted the commit > message as follows, let me know if you spot a mistake: > > PCI: dwc: Fix PORT_LINK_CONTROL update when CDM check enabled > > If CDM_CHECK is enabled (by the DT "snps,enable-cdm-check" property), 'val' > is overwritten by PCIE_PL_CHK_REG_CONTROL_STATUS initialization. Commit > ec7b952f453c ("PCI: dwc: Always enable CDM check if "snps,enable-cdm-check" > exists") did not account for further usage of 'val', so we wrote improper > values to PCIE_PORT_LINK_CONTROL when the CDM check is enabled. > > Move the PCIE_PORT_LINK_CONTROL update to be completely after the > PCIE_PL_CHK_REG_CONTROL_STATUS register initialization. > > [bhelgaas: commit log adapted from Serge's version] Sounds good. Thanks! * I'll keep reviewing the rest of the patches in the series. There are several aspects which I think is possible to improve/optimize. -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 > > > > > >
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;
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. Fixes: ec7b952f453c ("PCI: dwc: Always enable CDM check if "snps,enable-cdm-check" exists") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pci/controller/dwc/pcie-designware.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)