[PATCH] PCI: dwc: Fix writing wrong value if snps,enable-cdm-check

[PATCH] PCI: dwc: Fix writing wrong value if snps,enable-cdm-check

[Yoshihiro Shimoda]

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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 6d5d619ab2e9..3bb9ca14fb9c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	}
 
 	/* Set the number of lanes */
+	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_FAST_LINK_MODE;
 	val &= ~PORT_LINK_MODE_MASK;
 	switch (pci->num_lanes) {
-- 
2.25.1

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

[Bjorn Helgaas]

On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d5d619ab2e9..3bb9ca14fb9c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  	}
>  
>  	/* Set the number of lanes */
> +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);

Definitely a bug, thanks for the fix and the Fixes: tag.

But I would like the whole function better if it could be structured
so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
same for PCIE_LINK_WIDTH_SPEED_CONTROL.

Maybe there's a reason PCIE_PL_CHK_REG_CONTROL_STATUS must be written
between the two PCIE_PORT_LINK_CONTROL writes or the two
PCIE_LINK_WIDTH_SPEED_CONTROL writes, I dunno.  If so, a comment there
about why that is would be helpful.

>  	val &= ~PORT_LINK_FAST_LINK_MODE;
>  	val &= ~PORT_LINK_MODE_MASK;
>  	switch (pci->num_lanes) {
> -- 
> 2.25.1
> 

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

[Serge Semin]

On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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>

You turned to be a one day ahead of me submitting the fix. Thanks
anyway. My mistake.

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

-Serge(y)

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d5d619ab2e9..3bb9ca14fb9c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  	}
>  
>  	/* Set the number of lanes */
> +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
>  	val &= ~PORT_LINK_FAST_LINK_MODE;
>  	val &= ~PORT_LINK_MODE_MASK;
>  	switch (pci->num_lanes) {
> -- 
> 2.25.1
> 
> 

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

[Serge Semin]

On Thu, Feb 16, 2023 at 11:58:22AM -0600, Bjorn Helgaas wrote:
> On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 6d5d619ab2e9..3bb9ca14fb9c 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  	}
> >  
> >  	/* Set the number of lanes */
> > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> 
> Definitely a bug, thanks for the fix and the Fixes: tag.
> 

> But I would like the whole function better if it could be structured
> so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
> same for PCIE_LINK_WIDTH_SPEED_CONTROL.
>

I don't see a good looking solution for what you suggest. We'd need to
use additional temporary vars and gotos to implement that.

> Maybe there's a reason PCIE_PL_CHK_REG_CONTROL_STATUS must be written
> between the two PCIE_PORT_LINK_CONTROL writes or the two
> PCIE_LINK_WIDTH_SPEED_CONTROL writes, I dunno.  If so, a comment there
> about why that is would be helpful.

There were no sign of dependencies between the CDM-check enabling and
the rest of the setting performed in the dw_pcie_setup() function.
Originally the CDM-check was placed at the tail of the function:
07f123def73e ("PCI: dwc: Add support to enable CDM register check")
with no comments why it was placed there exactly. Moreover I got the
Rb-tag for my fix from Vidya Sagar, the original patch author. So he
was ok with the suggested solution.

-Serge(y)

> 
> >  	val &= ~PORT_LINK_FAST_LINK_MODE;
> >  	val &= ~PORT_LINK_MODE_MASK;
> >  	switch (pci->num_lanes) {
> > -- 
> > 2.25.1
> > 
> 

RE: [PATCH] PCI: dwc: Fix writing wrong value if snps,enable-cdm-check

[Yoshihiro Shimoda]

Hi Bjorn, Serge,

> From: Serge Semin, Sent: Friday, February 17, 2023 5:50 AM
> 
> On Thu, Feb 16, 2023 at 11:58:22AM -0600, Bjorn Helgaas wrote:
> > On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 6d5d619ab2e9..3bb9ca14fb9c 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > >  	}
> > >
> > >  	/* Set the number of lanes */
> > > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> >
> > Definitely a bug, thanks for the fix and the Fixes: tag.
> >
> 
> > But I would like the whole function better if it could be structured
> > so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
> > same for PCIE_LINK_WIDTH_SPEED_CONTROL.
> >
> 
> I don't see a good looking solution for what you suggest. We'd need to
> use additional temporary vars and gotos to implement that.
> 
> > Maybe there's a reason PCIE_PL_CHK_REG_CONTROL_STATUS must be written
> > between the two PCIE_PORT_LINK_CONTROL writes or the two
> > PCIE_LINK_WIDTH_SPEED_CONTROL writes, I dunno.  If so, a comment there
> > about why that is would be helpful.
> 
> There were no sign of dependencies between the CDM-check enabling and
> the rest of the setting performed in the dw_pcie_setup() function.
> Originally the CDM-check was placed at the tail of the function:
> 07f123def73e ("PCI: dwc: Add support to enable CDM register check")
> with no comments why it was placed there exactly. Moreover I got the
> Rb-tag for my fix from Vidya Sagar, the original patch author. So he
> was ok with the suggested solution.

I think so.

And, I think the commit 07f123def73e and commit ec7b952f453c are not
related to PCIE_PORT_LINK_CONTROL. So, PCIE_PL_CHK_REG_CONTROL_STATUS
handling can be moved everywhere in the function, IIUC. So, I think
we can have a solution with two patches like below:
1) Move PCIE_PL_CHK_REG_CONTROL_STATUS handling before reading
   PCIE_PORT_LINK_CONTROL (as a bug fix patch).
2) Refactor PCIE_PORT_LINK_CONTROL handling to avoid writing
   the register twice (as a patch for next).

I made patches for it like below. But, what do you think?
--------------- for 1) ---------------
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -806,11 +806,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 |
@@ -818,6 +813,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;
---
--------------- for 2) ---------------
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -813,19 +813,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
 		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
 	}
 
+	/* Set the number of lanes */
 	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;
-	}
-
-	/* Set the number of lanes */
-	val &= ~PORT_LINK_FAST_LINK_MODE;
-	val &= ~PORT_LINK_MODE_MASK;
+	/* Mask LINK_MODE if num_lanes is not zero */
+	if (pci->num_lanes)
+		val &= ~PORT_LINK_MODE_MASK;
 	switch (pci->num_lanes) {
 	case 1:
 		val |= PORT_LINK_MODE_1_LANES;
@@ -840,10 +834,12 @@ void dw_pcie_setup(struct dw_pcie *pci)
 		val |= PORT_LINK_MODE_8_LANES;
 		break;
 	default:
-		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->num_lanes);
-		return;
+		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
+		break;
 	}
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
+	if (!pci->num_lanes)
+		return;
 
 	/* Set link width speed control register */
 	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
--------------------------------------------

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> >
> > >  	val &= ~PORT_LINK_FAST_LINK_MODE;
> > >  	val &= ~PORT_LINK_MODE_MASK;
> > >  	switch (pci->num_lanes) {
> > > --
> > > 2.25.1
> > >
> >

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

[Serge Semin]

On Fri, Feb 17, 2023 at 12:46:03AM +0000, Yoshihiro Shimoda wrote:
> Hi Bjorn, Serge,
> 
> > From: Serge Semin, Sent: Friday, February 17, 2023 5:50 AM
> > 
> > On Thu, Feb 16, 2023 at 11:58:22AM -0600, Bjorn Helgaas wrote:
> > > On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 6d5d619ab2e9..3bb9ca14fb9c 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > > >  	}
> > > >
> > > >  	/* Set the number of lanes */
> > > > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> > >
> > > Definitely a bug, thanks for the fix and the Fixes: tag.
> > >
> > 
> > > But I would like the whole function better if it could be structured
> > > so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
> > > same for PCIE_LINK_WIDTH_SPEED_CONTROL.
> > >
> > 
> > I don't see a good looking solution for what you suggest. We'd need to
> > use additional temporary vars and gotos to implement that.
> > 
> > > Maybe there's a reason PCIE_PL_CHK_REG_CONTROL_STATUS must be written
> > > between the two PCIE_PORT_LINK_CONTROL writes or the two
> > > PCIE_LINK_WIDTH_SPEED_CONTROL writes, I dunno.  If so, a comment there
> > > about why that is would be helpful.
> > 
> > There were no sign of dependencies between the CDM-check enabling and
> > the rest of the setting performed in the dw_pcie_setup() function.
> > Originally the CDM-check was placed at the tail of the function:
> > 07f123def73e ("PCI: dwc: Add support to enable CDM register check")
> > with no comments why it was placed there exactly. Moreover I got the
> > Rb-tag for my fix from Vidya Sagar, the original patch author. So he
> > was ok with the suggested solution.
> 
> I think so.
> 
> And, I think the commit 07f123def73e and commit ec7b952f453c are not
> related to PCIE_PORT_LINK_CONTROL. So, PCIE_PL_CHK_REG_CONTROL_STATUS
> handling can be moved everywhere in the function, IIUC. So, I think
> we can have a solution with two patches like below:
> 1) Move PCIE_PL_CHK_REG_CONTROL_STATUS handling before reading
>    PCIE_PORT_LINK_CONTROL (as a bug fix patch).

> 2) Refactor PCIE_PORT_LINK_CONTROL handling to avoid writing
>    the register twice (as a patch for next).

IMO I would leave the procedure as is for now seeing you are going to
move the rcar_gen4_pcie_set_max_link_width() code to the generic part
of the driver in the framework of this patch:
https://lore.kernel.org/linux-pci/20230210134917.2909314-7-yoshihiro.shimoda.uh@renesas.com/
per Rob and my requests.

Thus you'll be able to combine all the bus-width updates into a single
method, like dw_pcie_link_set_max_link_width(). The function will look
as coherent as possible meanwhile the dw_pcie_setup() method body will
turn to be smaller and easier to comprehend. Alas that will imply
updating the PCIE_PORT_LINK_CONTROL and PCIE_LINK_WIDTH_SPEED_CONTROL
registers twice.

@Bjorn, are you ok with that?

-Serge(y)

> 
> I made patches for it like below. But, what do you think?
> --------------- for 1) ---------------
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -806,11 +806,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 |
> @@ -818,6 +813,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;
> ---
> --------------- for 2) ---------------
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -813,19 +813,13 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
>  
> +	/* Set the number of lanes */
>  	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;
> -	}
> -
> -	/* Set the number of lanes */
> -	val &= ~PORT_LINK_FAST_LINK_MODE;
> -	val &= ~PORT_LINK_MODE_MASK;
> +	/* Mask LINK_MODE if num_lanes is not zero */
> +	if (pci->num_lanes)
> +		val &= ~PORT_LINK_MODE_MASK;
>  	switch (pci->num_lanes) {
>  	case 1:
>  		val |= PORT_LINK_MODE_1_LANES;
> @@ -840,10 +834,12 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		val |= PORT_LINK_MODE_8_LANES;
>  		break;
>  	default:
> -		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->num_lanes);
> -		return;
> +		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> +		break;
>  	}
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> +	if (!pci->num_lanes)
> +		return;
>  
>  	/* Set link width speed control register */
>  	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> --------------------------------------------
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > -Serge(y)
> > 
> > >
> > > >  	val &= ~PORT_LINK_FAST_LINK_MODE;
> > > >  	val &= ~PORT_LINK_MODE_MASK;
> > > >  	switch (pci->num_lanes) {
> > > > --
> > > > 2.25.1
> > > >
> > >
> 

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

[Bjorn Helgaas]

On Fri, Feb 17, 2023 at 11:06:56AM +0300, Serge Semin wrote:
> On Fri, Feb 17, 2023 at 12:46:03AM +0000, Yoshihiro Shimoda wrote:
> > > From: Serge Semin, Sent: Friday, February 17, 2023 5:50 AM
> > > On Thu, Feb 16, 2023 at 11:58:22AM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Feb 16, 2023 at 06:20:12PM +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, read PCIE_PORT_LINK_CONTROL register again 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 | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 6d5d619ab2e9..3bb9ca14fb9c 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > > > >  	}
> > > > >
> > > > >  	/* Set the number of lanes */
> > > > > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> > > >
> > > > Definitely a bug, thanks for the fix and the Fixes: tag.
> > > >
> > > > But I would like the whole function better if it could be structured
> > > > so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
> > > > same for PCIE_LINK_WIDTH_SPEED_CONTROL.

> ...
> IMO I would leave the procedure as is for now seeing you are going to
> move the rcar_gen4_pcie_set_max_link_width() code to the generic part
> of the driver in the framework of this patch:
> https://lore.kernel.org/linux-pci/20230210134917.2909314-7-yoshihiro.shimoda.uh@renesas.com/
> per Rob and my requests.
> 
> Thus you'll be able to combine all the bus-width updates into a single
> method, like dw_pcie_link_set_max_link_width(). The function will look
> as coherent as possible meanwhile the dw_pcie_setup() method body will
> turn to be smaller and easier to comprehend. Alas that will imply
> updating the PCIE_PORT_LINK_CONTROL and PCIE_LINK_WIDTH_SPEED_CONTROL
> registers twice.
> 
> @Bjorn, are you ok with that?

I haven't looked at the rcar_gen4_pcie_set_max_link_width()
restructuring so don't have an opinion on that.

We read PCIE_PORT_LINK_CONTROL once, we lost the value by using "val"
for something else, and then we need it again.  My point was only that
re-reading PCIE_PORT_LINK_CONTROL seems like overkill compared to
adding another local variable to remember it.

If the updates are in different parts of the framework, maybe two
updates make sense.  I'm not suggesting we need to contort things to
achieve a single update at all costs.  It's just that two updates in
a single function looks like it should be avoidable.

Bjorn