diff mbox series

[v13,11/22] PCI: dwc: Add dw_pcie_link_set_max_width()

Message ID 20230418122403.3178462-12-yoshihiro.shimoda.uh@renesas.com
State New
Headers show
Series PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand

Commit Message

Yoshihiro Shimoda April 18, 2023, 12:23 p.m. UTC 
To improve code readability, add dw_pcie_link_set_max_width().
The original code writes the PCIE_PORT_LINK_CONTROL register twice
if the pci->num_lanes is not zero. But, it should avoid to write
the register twice. So, refactor it.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 65 ++++++++++----------
 1 file changed, 34 insertions(+), 31 deletions(-)

Comments

Manivannan Sadhasivam April 22, 2023, 11:50 a.m. UTC | #1 
On Tue, Apr 18, 2023 at 09:23:52PM +0900, Yoshihiro Shimoda wrote:
> To improve code readability, add dw_pcie_link_set_max_width().
> The original code writes the PCIE_PORT_LINK_CONTROL register twice
> if the pci->num_lanes is not zero. But, it should avoid to write
> the register twice. So, refactor it.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 65 ++++++++++----------
>  1 file changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 69358dc202f0..c76fa78c6468 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -737,6 +737,39 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed);
>  }
>  
> +static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes)
> +{
> +	u32 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;
> +
> +	/* Mask LINK_MODE if num_lanes is not zero */
> +	if (num_lanes)
> +		val &= ~PORT_LINK_MODE_MASK;
> +
> +	switch (num_lanes) {
> +	case 1:
> +		val |= PORT_LINK_MODE_1_LANES;
> +		break;
> +	case 2:
> +		val |= PORT_LINK_MODE_2_LANES;
> +		break;
> +	case 4:
> +		val |= PORT_LINK_MODE_4_LANES;
> +		break;
> +	case 8:
> +		val |= PORT_LINK_MODE_8_LANES;
> +		break;
> +	default:
> +		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> +		return;

Here you are not updating the LINK_CONTROL register. You should break instead of
returning.

> +	}

Newline here.

- Mani

> +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> +}
> +
>  static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  {
>  	u32 val;
> @@ -1040,36 +1073,6 @@ 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;
> -	}
> -
> -	/* Set the number of lanes */
> -	val &= ~PORT_LINK_MODE_MASK;
> -	switch (pci->num_lanes) {
> -	case 1:
> -		val |= PORT_LINK_MODE_1_LANES;
> -		break;
> -	case 2:
> -		val |= PORT_LINK_MODE_2_LANES;
> -		break;
> -	case 4:
> -		val |= PORT_LINK_MODE_4_LANES;
> -		break;
> -	case 8:
> -		val |= PORT_LINK_MODE_8_LANES;
> -		break;
> -	default:
> -		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->num_lanes);
> -		return;
> -	}
> -	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> -
> +	dw_pcie_link_set_max_width(pci, pci->num_lanes);
>  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
>  }
> -- 
> 2.25.1
>
Yoshihiro Shimoda April 24, 2023, 5:27 a.m. UTC | #2 
Hi Manivannan,

> From: Manivannan Sadhasivam, Sent: Saturday, April 22, 2023 8:51 PM
> 
> On Tue, Apr 18, 2023 at 09:23:52PM +0900, Yoshihiro Shimoda wrote:
> > To improve code readability, add dw_pcie_link_set_max_width().
> > The original code writes the PCIE_PORT_LINK_CONTROL register twice
> > if the pci->num_lanes is not zero. But, it should avoid to write
> > the register twice. So, refactor it.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 65 ++++++++++----------
> >  1 file changed, 34 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 69358dc202f0..c76fa78c6468 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -737,6 +737,39 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> >  	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed);
> >  }
> >
> > +static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes)
> > +{
> > +	u32 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;
> > +
> > +	/* Mask LINK_MODE if num_lanes is not zero */
> > +	if (num_lanes)
> > +		val &= ~PORT_LINK_MODE_MASK;
> > +
> > +	switch (num_lanes) {
> > +	case 1:
> > +		val |= PORT_LINK_MODE_1_LANES;
> > +		break;
> > +	case 2:
> > +		val |= PORT_LINK_MODE_2_LANES;
> > +		break;
> > +	case 4:
> > +		val |= PORT_LINK_MODE_4_LANES;
> > +		break;
> > +	case 8:
> > +		val |= PORT_LINK_MODE_8_LANES;
> > +		break;
> > +	default:
> > +		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
> > +		return;
> 
> Here you are not updating the LINK_CONTROL register. You should break instead of
> returning.

Oops. I'll use "break;" here, not return; on v13.

> > +	}
> 
> Newline here.

I got it.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> > +}
> > +
> >  static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
> >  {
> >  	u32 val;
> > @@ -1040,36 +1073,6 @@ 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;
> > -	}
> > -
> > -	/* Set the number of lanes */
> > -	val &= ~PORT_LINK_MODE_MASK;
> > -	switch (pci->num_lanes) {
> > -	case 1:
> > -		val |= PORT_LINK_MODE_1_LANES;
> > -		break;
> > -	case 2:
> > -		val |= PORT_LINK_MODE_2_LANES;
> > -		break;
> > -	case 4:
> > -		val |= PORT_LINK_MODE_4_LANES;
> > -		break;
> > -	case 8:
> > -		val |= PORT_LINK_MODE_8_LANES;
> > -		break;
> > -	default:
> > -		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->num_lanes);
> > -		return;
> > -	}
> > -	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> > -
> > +	dw_pcie_link_set_max_width(pci, pci->num_lanes);
> >  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
> >  }
> > --
> > 2.25.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 69358dc202f0..c76fa78c6468 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -737,6 +737,39 @@  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
 	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed);
 }
 
+static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes)
+{
+	u32 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;
+
+	/* Mask LINK_MODE if num_lanes is not zero */
+	if (num_lanes)
+		val &= ~PORT_LINK_MODE_MASK;
+
+	switch (num_lanes) {
+	case 1:
+		val |= PORT_LINK_MODE_1_LANES;
+		break;
+	case 2:
+		val |= PORT_LINK_MODE_2_LANES;
+		break;
+	case 4:
+		val |= PORT_LINK_MODE_4_LANES;
+		break;
+	case 8:
+		val |= PORT_LINK_MODE_8_LANES;
+		break;
+	default:
+		dev_dbg(pci->dev, "Using h/w default number of lanes\n");
+		return;
+	}
+	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
+}
+
 static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
 {
 	u32 val;
@@ -1040,36 +1073,6 @@  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;
-	}
-
-	/* Set the number of lanes */
-	val &= ~PORT_LINK_MODE_MASK;
-	switch (pci->num_lanes) {
-	case 1:
-		val |= PORT_LINK_MODE_1_LANES;
-		break;
-	case 2:
-		val |= PORT_LINK_MODE_2_LANES;
-		break;
-	case 4:
-		val |= PORT_LINK_MODE_4_LANES;
-		break;
-	case 8:
-		val |= PORT_LINK_MODE_8_LANES;
-		break;
-	default:
-		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->num_lanes);
-		return;
-	}
-	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
-
+	dw_pcie_link_set_max_width(pci, pci->num_lanes);
 	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
 }