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

[Serge Semin <fancer.lancer@gmail.com>]

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/20230721074452.65545-9-yoshihiro.shimoda.uh@renesas.com)
> > > 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/20230721074452.65545-1-yoshihiro.shimoda.uh@renesas.com/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/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/
> > > 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/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/
> > 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