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