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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > 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@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 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 > >
