diff mbox series

[v17,08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width()

Message ID 20230705114206.3585188-9-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand

Commit Message

Yoshihiro Shimoda July 5, 2023, 11:41 a.m. UTC 
To improve code readability, add dw_pcie_link_set_max_link_width().

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/controller/dwc/pcie-designware.c | 86 ++++++++++----------
 1 file changed, 41 insertions(+), 45 deletions(-)

Comments

Serge Semin July 11, 2023, 8:45 p.m. UTC | #1 
On Wed, Jul 05, 2023 at 08:41:54PM +0900, Yoshihiro Shimoda wrote:
> To improve code readability, add dw_pcie_link_set_max_link_width().

This is a preparation 

> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 86 ++++++++++----------
>  1 file changed, 41 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index a531dc50abea..7b720bad7656 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -728,6 +728,46 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  
>  }
>  
> +static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
> +{
> +	u32 lwsc, plc;
> +
> +	if (!num_lanes)
> +		return;
> +
> +	/* Set the number of lanes */
> +	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);

> +	plc &= ~PORT_LINK_FAST_LINK_MODE;

This is redundant and unrelated to the link width setup. See my next
comment for solution.

> +	plc &= ~PORT_LINK_MODE_MASK;
> +
> +	/* Set link width speed control register */
> +	lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> +	lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> +	switch (num_lanes) {
> +	case 1:
> +		plc |= PORT_LINK_MODE_1_LANES;
> +		lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> +		break;
> +	case 2:
> +		plc |= PORT_LINK_MODE_2_LANES;
> +		lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> +		break;
> +	case 4:
> +		plc |= PORT_LINK_MODE_4_LANES;
> +		lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> +		break;
> +	case 8:
> +		plc |= PORT_LINK_MODE_8_LANES;
> +		lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> +		break;
> +	default:
> +		dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
> +		return;
> +	}
> +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
> +	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
> +}
> +
>  void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  {
>  	int max_region, ob, ib;
> @@ -1009,49 +1049,5 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  	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;

Please pick up my patch dropping the line above
https://patchwork.kernel.org/project/linux-pci/patch/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/
and add it to your series before this one.
* unless Krzysztof decide to merge it and some another patches from my
* series in...

-Serge(y)

> -	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);
> -
> -	/* Set link width speed control register */
> -	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> -	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> -	switch (pci->num_lanes) {
> -	case 1:
> -		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> -		break;
> -	case 2:
> -		val |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> -		break;
> -	case 4:
> -		val |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> -		break;
> -	case 8:
> -		val |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> -		break;
> -	}
> -	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> +	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
>  }
> -- 
> 2.25.1
>
Yoshihiro Shimoda July 14, 2023, 2:11 a.m. UTC | #2 
He Serge,

> From: Serge Semin, Sent: Wednesday, July 12, 2023 5:45 AM
> 
> On Wed, Jul 05, 2023 at 08:41:54PM +0900, Yoshihiro Shimoda wrote:
> > To improve code readability, add dw_pcie_link_set_max_link_width().
> 
> This is a preparation

Yes, this is a preparation. So, I'll add such information.

> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 86 ++++++++++----------
> >  1 file changed, 41 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index a531dc50abea..7b720bad7656 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -728,6 +728,46 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> >
> >  }
> >
> > +static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
> > +{
> > +	u32 lwsc, plc;
> > +
> > +	if (!num_lanes)
> > +		return;
> > +
> > +	/* Set the number of lanes */
> > +	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> 
> > +	plc &= ~PORT_LINK_FAST_LINK_MODE;
> 
> This is redundant and unrelated to the link width setup. See my next
> comment for solution.
> 
> > +	plc &= ~PORT_LINK_MODE_MASK;
> > +
> > +	/* Set link width speed control register */
> > +	lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > +	lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> > +	switch (num_lanes) {
> > +	case 1:
> > +		plc |= PORT_LINK_MODE_1_LANES;
> > +		lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> > +		break;
> > +	case 2:
> > +		plc |= PORT_LINK_MODE_2_LANES;
> > +		lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> > +		break;
> > +	case 4:
> > +		plc |= PORT_LINK_MODE_4_LANES;
> > +		lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> > +		break;
> > +	case 8:
> > +		plc |= PORT_LINK_MODE_8_LANES;
> > +		lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> > +		break;
> > +	default:
> > +		dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
> > +		return;
> > +	}
> > +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
> > +	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
> > +}
> > +
> >  void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >  {
> >  	int max_region, ob, ib;
> > @@ -1009,49 +1049,5 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  	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;
> 
> Please pick up my patch dropping the line above
> https://patchwork.kernel.org/project/linux-pci/patch/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/
> and add it to your series before this one.

I'm afraid but, I cannot pick up your patch into my series because I have a concern
about the following comment from the PCI maintainer..:

https://lore.kernel.org/linux-pci/20230612154127.GA1335023@bhelgaas/

Best regards,
Yoshihiro Shimoda

> * unless Krzysztof decide to merge it and some another patches from my
> * series in...
> 
> -Serge(y)
> 
> > -	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);
> > -
> > -	/* Set link width speed control register */
> > -	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > -	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> > -	switch (pci->num_lanes) {
> > -	case 1:
> > -		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> > -		break;
> > -	case 2:
> > -		val |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> > -		break;
> > -	case 4:
> > -		val |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> > -		break;
> > -	case 8:
> > -		val |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> > -		break;
> > -	}
> > -	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> > +	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
> >  }
> > --
> > 2.25.1
> >
Serge Semin July 14, 2023, 12:03 p.m. UTC | #3 
On Fri, Jul 14, 2023 at 02:11:33AM +0000, Yoshihiro Shimoda wrote:
> He Serge,
> 
> > From: Serge Semin, Sent: Wednesday, July 12, 2023 5:45 AM
> > 
> > On Wed, Jul 05, 2023 at 08:41:54PM +0900, Yoshihiro Shimoda wrote:
> > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > 
> > This is a preparation
> 
> Yes, this is a preparation. So, I'll add such information.
> 
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware.c | 86 ++++++++++----------
> > >  1 file changed, 41 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index a531dc50abea..7b720bad7656 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -728,6 +728,46 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> > >
> > >  }
> > >
> > > +static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
> > > +{
> > > +	u32 lwsc, plc;
> > > +
> > > +	if (!num_lanes)
> > > +		return;
> > > +
> > > +	/* Set the number of lanes */
> > > +	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> > 
> > > +	plc &= ~PORT_LINK_FAST_LINK_MODE;
> > 
> > This is redundant and unrelated to the link width setup. See my next
> > comment for solution.
> > 
> > > +	plc &= ~PORT_LINK_MODE_MASK;
> > > +
> > > +	/* Set link width speed control register */
> > > +	lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > > +	lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> > > +	switch (num_lanes) {
> > > +	case 1:
> > > +		plc |= PORT_LINK_MODE_1_LANES;
> > > +		lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> > > +		break;
> > > +	case 2:
> > > +		plc |= PORT_LINK_MODE_2_LANES;
> > > +		lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> > > +		break;
> > > +	case 4:
> > > +		plc |= PORT_LINK_MODE_4_LANES;
> > > +		lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> > > +		break;
> > > +	case 8:
> > > +		plc |= PORT_LINK_MODE_8_LANES;
> > > +		lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> > > +		break;
> > > +	default:
> > > +		dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
> > > +		return;
> > > +	}
> > > +	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
> > > +	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
> > > +}
> > > +
> > >  void dw_pcie_iatu_detect(struct dw_pcie *pci)
> > >  {
> > >  	int max_region, ob, ib;
> > > @@ -1009,49 +1049,5 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > >  	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;
> > 
> > Please pick up my patch dropping the line above
> > https://patchwork.kernel.org/project/linux-pci/patch/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/
> > and add it to your series before this one.
> 

> I'm afraid but, I cannot pick up your patch into my series because I have a concern
> about the following comment from the PCI maintainer..:
> 
> https://lore.kernel.org/linux-pci/20230612154127.GA1335023@bhelgaas/

I don't know what particularly caused that Bjorn decision but it
wasn't the patches content for sure. Feel free to pick it up with
changing the authorship but add me under the Suggested-by tag with the
email-address I am using to review your series.

-Serge(y)

> 
> Best regards,
> Yoshihiro Shimoda
> 
> > * unless Krzysztof decide to merge it and some another patches from my
> > * series in...
> > 
> > -Serge(y)
> > 
> > > -	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);
> > > -
> > > -	/* Set link width speed control register */
> > > -	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > > -	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> > > -	switch (pci->num_lanes) {
> > > -	case 1:
> > > -		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
> > > -		break;
> > > -	case 2:
> > > -		val |= PORT_LOGIC_LINK_WIDTH_2_LANES;
> > > -		break;
> > > -	case 4:
> > > -		val |= PORT_LOGIC_LINK_WIDTH_4_LANES;
> > > -		break;
> > > -	case 8:
> > > -		val |= PORT_LOGIC_LINK_WIDTH_8_LANES;
> > > -		break;
> > > -	}
> > > -	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> > > +	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 a531dc50abea..7b720bad7656 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -728,6 +728,46 @@  static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
 
 }
 
+static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
+{
+	u32 lwsc, plc;
+
+	if (!num_lanes)
+		return;
+
+	/* Set the number of lanes */
+	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
+	plc &= ~PORT_LINK_FAST_LINK_MODE;
+	plc &= ~PORT_LINK_MODE_MASK;
+
+	/* Set link width speed control register */
+	lwsc = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
+	lwsc &= ~PORT_LOGIC_LINK_WIDTH_MASK;
+	switch (num_lanes) {
+	case 1:
+		plc |= PORT_LINK_MODE_1_LANES;
+		lwsc |= PORT_LOGIC_LINK_WIDTH_1_LANES;
+		break;
+	case 2:
+		plc |= PORT_LINK_MODE_2_LANES;
+		lwsc |= PORT_LOGIC_LINK_WIDTH_2_LANES;
+		break;
+	case 4:
+		plc |= PORT_LINK_MODE_4_LANES;
+		lwsc |= PORT_LOGIC_LINK_WIDTH_4_LANES;
+		break;
+	case 8:
+		plc |= PORT_LINK_MODE_8_LANES;
+		lwsc |= PORT_LOGIC_LINK_WIDTH_8_LANES;
+		break;
+	default:
+		dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
+		return;
+	}
+	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
+	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
+}
+
 void dw_pcie_iatu_detect(struct dw_pcie *pci)
 {
 	int max_region, ob, ib;
@@ -1009,49 +1049,5 @@  void dw_pcie_setup(struct dw_pcie *pci)
 	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;
-	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);
-
-	/* Set link width speed control register */
-	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
-	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
-	switch (pci->num_lanes) {
-	case 1:
-		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
-		break;
-	case 2:
-		val |= PORT_LOGIC_LINK_WIDTH_2_LANES;
-		break;
-	case 4:
-		val |= PORT_LOGIC_LINK_WIDTH_4_LANES;
-		break;
-	case 8:
-		val |= PORT_LOGIC_LINK_WIDTH_8_LANES;
-		break;
-	}
-	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
+	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
 }