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/20230721074452.65545-9-yoshihiro.shimoda.uh@xxxxxxxxxxx)
> 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@xxxxxxxxxxx/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@xxxxxxxxxxxxxxxxxxxx/
> 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@xxxxxxxxxxxxxxxxxxxx/
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)
