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@xxxxxxxxxxx>
> > > Reviewed-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxx/
> > 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
> > >
