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

Commit Message

Yoshihiro Shimoda July 21, 2023, 7:44 a.m. UTC

To improve code readability, add dw_pcie_link_set_max_link_width().

Signed-off-by: Yoshihiro Shimoda <[e-mail]>
Reviewed-by: Manivannan Sadhasivam <[e-mail]>
---
 drivers/pci/controller/dwc/pcie-designware.c | 86 ++++++++++----------
 1 file changed, 41 insertions(+), 45 deletions(-)

Comments

Serge Semin July 31, 2023, 11:53 p.m. UTC | #1

On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> To improve code readability, add dw_pcie_link_set_max_link_width().

You completely ignored all my comments regarding this patch again.
It's getting to be annoying really.

Once again: "This patch is a preparation before adding the
Max-Link-width capability setup which would in its turn complete the
max-link-width setup procedure defined by Synopsys in the HW-manual.
Seeing there is a max-link-speed setup method defined in the DW PCIe
core driver it would be good to have a similar function for the link
width setup. That's why we need to define a dedicated function first
from already implemented but incomplete link-width setting up
code." This is what should have been described in the commit log.
If you were a side-reader of the patch could you guess that from your
commit log and the patch content? I bet you couldn't. That's why a
very thorough description is important.

> 
> Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> ---
>  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 2d0f816fa0ab..5cca34140d2a 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;

Once again: this masking is unrelated to the link width setup.
Moreover it's completely redundant in here and in the original code.
See further for details.

> +	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;

My series contains the patch which drops this line:
https://patchwork.kernel.org/project/linux-pci/patch/[e-mail]/
So either pick my patch up and add it to your series or still pick it up
but with changing the authorship and adding me under the Suggested-by
tag with the email-address I am using to review your series. Bjorn,
what approach would you prefer? Perhaps alternative?

Note the patch I am talking about doesn't contain anything what
couldn't be merged in. The problem with my series is in completely
another dimension.

Bjorn

> -	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 Aug. 1, 2023, 1:50 a.m. UTC | #2

Hi Serge,

> From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> 
> On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > To improve code readability, add dw_pcie_link_set_max_link_width().
> 
> You completely ignored all my comments regarding this patch again.
> It's getting to be annoying really.

I'm sorry for that. I completely forgot to add description even though
I said so on the v17 [1].

[1] https://lore.kernel.org/linux-pci/[e-mail]/

> Once again: "This patch is a preparation before adding the
> Max-Link-width capability setup which would in its turn complete the
> max-link-width setup procedure defined by Synopsys in the HW-manual.
> Seeing there is a max-link-speed setup method defined in the DW PCIe
> core driver it would be good to have a similar function for the link
> width setup. That's why we need to define a dedicated function first
> from already implemented but incomplete link-width setting up
> code." This is what should have been described in the commit log.
> If you were a side-reader of the patch could you guess that from your
> commit log and the patch content? I bet you couldn't. That's why a
> very thorough description is important.

Thank you for your suggestion. I have never read the description before.
About the [1] above, you said just "This patch is a preparation".
So, perhaps, some trouble happened when I sent an email?
Anyway, I will replace the commit description to your suggestion and
add your Suggested-by tag.

> >
> > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > ---
> >  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 2d0f816fa0ab..5cca34140d2a 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;
> 
> Once again: this masking is unrelated to the link width setup.
> Moreover it's completely redundant in here and in the original code.
> See further for details.

I got it.

> > +	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;
> 
> My series contains the patch which drops this line:
<snip URL>
> So either pick my patch up and add it to your series or still pick it up
> but with changing the authorship and adding me under the Suggested-by
> tag with the email-address I am using to review your series. Bjorn,
> what approach would you prefer? Perhaps alternative?

I'll wait for Bjorn's opinion.

Best regards,
Yoshihiro Shimoda

> Note the patch I am talking about doesn't contain anything what
> couldn't be merged in. The problem with my series is in completely
> another dimension.
> 
> Bjorn
> 
> > -	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 Aug. 7, 2023, 10:53 p.m. UTC | #3

Hi Bjorn,

Your attention is required in this thread. Could you please give us
your resolution regarding the issue denoted in my last comment?

-Serge(y)

On Tue, Aug 01, 2023 at 01:50:59AM +0000, Yoshihiro Shimoda wrote:
> Hi Serge,
> 
> > From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> > 
> > On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > 
> > You completely ignored all my comments regarding this patch again.
> > It's getting to be annoying really.
> 
> I'm sorry for that. I completely forgot to add description even though
> I said so on the v17 [1].
> 
> [1] https://lore.kernel.org/linux-pci/[e-mail]/
> 
> > Once again: "This patch is a preparation before adding the
> > Max-Link-width capability setup which would in its turn complete the
> > max-link-width setup procedure defined by Synopsys in the HW-manual.
> > Seeing there is a max-link-speed setup method defined in the DW PCIe
> > core driver it would be good to have a similar function for the link
> > width setup. That's why we need to define a dedicated function first
> > from already implemented but incomplete link-width setting up
> > code." This is what should have been described in the commit log.
> > If you were a side-reader of the patch could you guess that from your
> > commit log and the patch content? I bet you couldn't. That's why a
> > very thorough description is important.
> 
> Thank you for your suggestion. I have never read the description before.
> About the [1] above, you said just "This patch is a preparation".
> So, perhaps, some trouble happened when I sent an email?
> Anyway, I will replace the commit description to your suggestion and
> add your Suggested-by tag.
> 
> > >
> > > Signed-off-by: Yoshihiro Shimoda <[e-mail]>
> > > Reviewed-by: Manivannan Sadhasivam <[e-mail]>
> > > ---
> > >  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 2d0f816fa0ab..5cca34140d2a 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;
> > 
> > Once again: this masking is unrelated to the link width setup.
> > Moreover it's completely redundant in here and in the original code.
> > See further for details.
> 
> I got it.
> 
> > > +	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;
> > 
> > My series contains the patch which drops this line:
> <snip URL>
> > So either pick my patch up and add it to your series or still pick it up
> > but with changing the authorship and adding me under the Suggested-by
> > tag with the email-address I am using to review your series. Bjorn,
> > what approach would you prefer? Perhaps alternative?
> 
> I'll wait for Bjorn's opinion.
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > Note the patch I am talking about doesn't contain anything what
> > couldn't be merged in. The problem with my series is in completely
> > another dimension.
> > 
> > Bjorn
> > 
> > > -	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
> > >

Bjorn Helgaas Aug. 7, 2023, 11:40 p.m. UTC | #4

On Tue, Aug 08, 2023 at 01:53:11AM +0300, Serge Semin wrote:
> Your attention is required in this thread. Could you please give us
> your resolution regarding the issue denoted in my last comment?

Sorry I missed this and thanks for pinging me.  Lorenzo and Krzysztof
take care of the native controller drivers so I don't pay close
attention.

> On Tue, Aug 01, 2023 at 01:50:59AM +0000, Yoshihiro Shimoda wrote:
> > > From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> > > On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > > > ...

> > > > @@ -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;
> > > 
> > > My series contains the patch which drops this line:
> > <snip URL>
> > > So either pick my patch up and add it to your series or still pick it up
> > > but with changing the authorship and adding me under the Suggested-by
> > > tag with the email-address I am using to review your series. Bjorn,
> > > what approach would you prefer? Perhaps alternative?

I don't really see the argument here.  AFAICT, Yoshihiro's patch
(https://lore.kernel.org/r/[e-mail])
is a trivial refactoring to make dw_pcie_link_set_max_link_width(),
which might be reused elsewhere later, which seems perfectly fine.

It'd be fine with me to add a little detail in the commit log to
reference the Synopsys manual, which I don't have.  But doesn't seem
like a big deal to me.

Dropping the PORT_LINK_FAST_LINK_MODE mask seems like a separate
question that should be in a separate patch.
https://lore.kernel.org/linux-pci/[e-mail]/
says it's redundant, so it sounds more like a cleanup than a fix.

> > > Note the patch I am talking about doesn't contain anything what
> > > couldn't be merged in. The problem with my series is in completely
> > > another dimension.
> > > 
> > > Bjorn

Despite the "Bjorn" that looks like a signature, I did not write the
"Note the patch ..." paragraph above.

Bjorn

Serge Semin Aug. 8, 2023, 12:15 a.m. UTC | #5

On Mon, Aug 07, 2023 at 06:40:34PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 08, 2023 at 01:53:11AM +0300, Serge Semin wrote:
> > Your attention is required in this thread. Could you please give us
> > your resolution regarding the issue denoted in my last comment?
> 
> Sorry I missed this and thanks for pinging me.  Lorenzo and Krzysztof
> take care of the native controller drivers so I don't pay close
> attention.
> 
> > On Tue, Aug 01, 2023 at 01:50:59AM +0000, Yoshihiro Shimoda wrote:
> > > > From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> > > > On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > > > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > > > > ...
> 
> > > > > @@ -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;
> > > > 
> > > > My series contains the patch which drops this line:
> > > <snip URL>
> > > > So either pick my patch up and add it to your series or still pick it up
> > > > but with changing the authorship and adding me under the Suggested-by
> > > > tag with the email-address I am using to review your series. Bjorn,
> > > > what approach would you prefer? Perhaps alternative?
> 

> I don't really see the argument here.  AFAICT, Yoshihiro's patch
> (https://lore.kernel.org/r/[e-mail])
> is a trivial refactoring to make dw_pcie_link_set_max_link_width(),
> which might be reused elsewhere later, which seems perfectly fine.
> 
> It'd be fine with me to add a little detail in the commit log to
> reference the Synopsys manual, which I don't have.  But doesn't seem
> like a big deal to me.

More details are in one of my earlier comments to this patch which
Yoshihiro promised to add to the patch log on the next patchset
revision. You can read it here:
https://lore.kernel.org/linux-pci/[e-mail]/T/#m8ac364249f40c726da88316b67f11a6d55068ef0

> 
> Dropping the PORT_LINK_FAST_LINK_MODE mask seems like a separate
> question that should be in a separate patch.
> https://lore.kernel.org/linux-pci/[e-mail]/
> says it's redundant, so it sounds more like a cleanup than a fix.

That's the point of my comment. There is no need in copying that mask
to the dw_pcie_link_set_max_link_width() method because first it's
unrelated to the link-width setting, second it's redundant. There is
my patch dropping the mask with the proper justification:
https://lore.kernel.org/linux-pci/[e-mail]/
It would be good to either merge it in before the Yoshihiro' series or
add my patch to the Yoshihiro' patchset. But it's in the patchwork
limbo now, neither you nor Lorenzo or Krzysztof were willing to merge
it in. That's why I suggested to move the patch here with the denoted
alterations. Could you give your resolution whether the suggested
movement is ok or perhaps you or Lorenzo or Krzysztof consider merge
it in as is?

Note this and the next Yoshihiro' patches aren't considered as fixes
for now.

> 
> > > > Note the patch I am talking about doesn't contain anything what
> > > > couldn't be merged in. The problem with my series is in completely
> > > > another dimension.
> > > > 
> > > > Bjorn
> 

> Despite the "Bjorn" that looks like a signature, I did not write the
> "Note the patch ..." paragraph above.
> 
> Bjorn

Ah, sorry. It was my incomplete text. Part of it somehow was dropped from the
message so it turned out to look as a signature. My message was in
response to the Yoshihiro' worry regarding your email:
https://lore.kernel.org/linux-pci/20230612154127.GA1335023@bhelgaas/
What I was saying is that my patch didn't contain anything which could prevent
it from being merged in. So at least the patch content could be easily
copied to his series.

-Serge(y)

Bjorn Helgaas Aug. 8, 2023, 3:08 p.m. UTC | #6

On Tue, Aug 08, 2023 at 03:15:33AM +0300, Serge Semin wrote:
> On Mon, Aug 07, 2023 at 06:40:34PM -0500, Bjorn Helgaas wrote:
> > On Tue, Aug 08, 2023 at 01:53:11AM +0300, Serge Semin wrote:
> > > On Tue, Aug 01, 2023 at 01:50:59AM +0000, Yoshihiro Shimoda wrote:
> > > > > From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> > > > > On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > > > > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > > > > > ...
> > 
> > > > > > @@ -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;
> > > > > 
> > > > > My series contains the patch which drops this line:
> > > > <snip URL>
> > > > > So either pick my patch up and add it to your series or still pick it up
> > > > > but with changing the authorship and adding me under the Suggested-by
> > > > > tag with the email-address I am using to review your series. Bjorn,
> > > > > what approach would you prefer? Perhaps alternative?
> 
> > I don't really see the argument here.  AFAICT, Yoshihiro's patch
> > (https://lore.kernel.org/r/[e-mail])
> > is a trivial refactoring to make dw_pcie_link_set_max_link_width(),
> > which might be reused elsewhere later, which seems perfectly fine.
> > 
> > It'd be fine with me to add a little detail in the commit log to
> > reference the Synopsys manual, which I don't have.  But doesn't seem
> > like a big deal to me.
> 
> More details are in one of my earlier comments to this patch which
> Yoshihiro promised to add to the patch log on the next patchset
> revision. You can read it here:
> https://lore.kernel.org/linux-pci/[e-mail]/T/#m8ac364249f40c726da88316b67f11a6d55068ef0
> 
> > Dropping the PORT_LINK_FAST_LINK_MODE mask seems like a separate
> > question that should be in a separate patch.
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > says it's redundant, so it sounds more like a cleanup than a fix.
> 
> That's the point of my comment. There is no need in copying that mask
> to the dw_pcie_link_set_max_link_width() method because first it's
> unrelated to the link-width setting, second it's redundant. There is
> my patch dropping the mask with the proper justification:
> https://lore.kernel.org/linux-pci/[e-mail]/
> It would be good to either merge it in before the Yoshihiro' series or
> add my patch to the Yoshihiro' patchset. But it's in the patchwork
> limbo now, neither you nor Lorenzo or Krzysztof were willing to merge
> it in. That's why I suggested to move the patch here with the denoted
> alterations. Could you give your resolution whether the suggested
> movement is ok or perhaps you or Lorenzo or Krzysztof consider merge
> it in as is?

If I understand Yoshihiro's patch, it pulls code out into
dw_pcie_link_set_max_link_width() without changing that code.  That
seems like the best approach to me because it's very easy to review.

If we want to remove a little redundant code later in a separate
patch, that's fine too but doesn't seem urgent.  I don't think they
need to be tied together.

Bjorn

Serge Semin Aug. 8, 2023, 9:16 p.m. UTC | #7

On Tue, Aug 08, 2023 at 10:08:54AM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 08, 2023 at 03:15:33AM +0300, Serge Semin wrote:
> > On Mon, Aug 07, 2023 at 06:40:34PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Aug 08, 2023 at 01:53:11AM +0300, Serge Semin wrote:
> > > > On Tue, Aug 01, 2023 at 01:50:59AM +0000, Yoshihiro Shimoda wrote:
> > > > > > From: Serge Semin, Sent: Tuesday, August 1, 2023 8:54 AM
> > > > > > On Fri, Jul 21, 2023 at 04:44:40PM +0900, Yoshihiro Shimoda wrote:
> > > > > > > To improve code readability, add dw_pcie_link_set_max_link_width().
> > > > > > > ...
> > > 
> > > > > > > @@ -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;
> > > > > > 
> > > > > > My series contains the patch which drops this line:
> > > > > <snip URL>
> > > > > > So either pick my patch up and add it to your series or still pick it up
> > > > > > but with changing the authorship and adding me under the Suggested-by
> > > > > > tag with the email-address I am using to review your series. Bjorn,
> > > > > > what approach would you prefer? Perhaps alternative?
> > 
> > > I don't really see the argument here.  AFAICT, Yoshihiro's patch
> > > (https://lore.kernel.org/r/[e-mail])
> > > is a trivial refactoring to make dw_pcie_link_set_max_link_width(),
> > > which might be reused elsewhere later, which seems perfectly fine.
> > > 
> > > It'd be fine with me to add a little detail in the commit log to
> > > reference the Synopsys manual, which I don't have.  But doesn't seem
> > > like a big deal to me.
> > 
> > More details are in one of my earlier comments to this patch which
> > Yoshihiro promised to add to the patch log on the next patchset
> > revision. You can read it here:
> > https://lore.kernel.org/linux-pci/[e-mail]/T/#m8ac364249f40c726da88316b67f11a6d55068ef0
> > 
> > > Dropping the PORT_LINK_FAST_LINK_MODE mask seems like a separate
> > > question that should be in a separate patch.
> > > https://lore.kernel.org/linux-pci/[e-mail]/
> > > says it's redundant, so it sounds more like a cleanup than a fix.
> > 
> > That's the point of my comment. There is no need in copying that mask
> > to the dw_pcie_link_set_max_link_width() method because first it's
> > unrelated to the link-width setting, second it's redundant. There is
> > my patch dropping the mask with the proper justification:
> > https://lore.kernel.org/linux-pci/[e-mail]/
> > It would be good to either merge it in before the Yoshihiro' series or
> > add my patch to the Yoshihiro' patchset. But it's in the patchwork
> > limbo now, neither you nor Lorenzo or Krzysztof were willing to merge
> > it in. That's why I suggested to move the patch here with the denoted
> > alterations. Could you give your resolution whether the suggested
> > movement is ok or perhaps you or Lorenzo or Krzysztof consider merge
> > it in as is?
> 
> If I understand Yoshihiro's patch, it pulls code out into
> dw_pcie_link_set_max_link_width() without changing that code.  That
> seems like the best approach to me because it's very easy to review.
> 

> If we want to remove a little redundant code later in a separate
> patch, that's fine too but doesn't seem urgent.  I don't think they
> need to be tied together.

Well, my point was the opposite: why would we need to maintain a dead,
redundant code which also unrelated to the function it's being copied
to, meanwhile there is already available patch which drops that code
and Yoshihiro needs to resubmit a new revision of his series anyway?
It would have been much better to just merge in my patch/change
somehow (with another authorship if that's the problem) and forget
about that from now on. If you get to merge in the Yoshohiro patchset
first, my patch won't be cleanly applicable after that.

-Serge(y)

> 
> Bjorn

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 2d0f816fa0ab..5cca34140d2a 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);
 }