| Message ID | 20230721074452.65545-9-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
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 <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 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/20230611192005.25636-6-Sergey.Semin@baikalelectronics.ru/ 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 >
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/TYBPR01MB5341BE7E22A0721672A0FFAFD834A@TYBPR01MB5341.jpnprd01.prod.outlook.com/ > 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 <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 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 > >
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/TYBPR01MB5341BE7E22A0721672A0FFAFD834A@TYBPR01MB5341.jpnprd01.prod.outlook.com/ > > > 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 <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 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 > > >
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@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. 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. > > > 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
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@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? 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)
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. Bjorn
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
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); }