| Message ID | 20230426045557.3613826-11-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car Gen4 PCIe support | expand |
On Wed, Apr 26, 2023 at 01:55:46PM +0900, Yoshihiro Shimoda wrote: > To improve code readability, add dw_pcie_link_set_max_width(). > The original code writes the PCIE_PORT_LINK_CONTROL register twice > if the pci->num_lanes is not zero. But, it should avoid to write > the register twice. So, refactor it. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 66 +++++++++++--------- > 1 file changed, 35 insertions(+), 31 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 69358dc202f0..f8926d5ec422 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -737,6 +737,40 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed); > } > > +static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes) Why defining a separate method? Just move it's content to the already defined dw_pcie_link_set_max_link_width(). This could have been done in the framework of the previous patch. > +{ > + u32 val; > + > + /* Set the number of lanes */ > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); > + val &= ~PORT_LINK_FAST_LINK_MODE; > + val |= PORT_LINK_DLL_LINK_EN; > + > + /* Mask LINK_MODE if num_lanes is not zero */ > + if (num_lanes) > + val &= ~PORT_LINK_MODE_MASK; this and... > + > + switch (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_dbg(pci->dev, "Using h/w default number of lanes\n"); ...this change the link-width setup semantic in case if the invalid number of lanes is specified. Your method now causes the PORT_LINK_MODE_MASK field clearance in case if a not permitted link width is passed. > + break; > + } > + > + dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val); > +} > + > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > { > u32 val; > @@ -1040,36 +1074,6 @@ void dw_pcie_setup(struct dw_pcie *pci) > dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val); > } > > - val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); > - val &= ~PORT_LINK_FAST_LINK_MODE; > - val |= PORT_LINK_DLL_LINK_EN; > - dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val); Please leave the setups above here. Link-training mode has nothing to do with the max link width. -Serge(y) > - > - 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_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); > - > + dw_pcie_link_set_max_width(pci, pci->num_lanes); > dw_pcie_link_set_max_link_width(pci, pci->num_lanes); > } > -- > 2.25.1 >
Hi Serge, > From: Serge Semin, Sent: Tuesday, May 2, 2023 4:48 AM > > On Wed, Apr 26, 2023 at 01:55:46PM +0900, Yoshihiro Shimoda wrote: > > To improve code readability, add dw_pcie_link_set_max_width(). > > The original code writes the PCIE_PORT_LINK_CONTROL register twice > > if the pci->num_lanes is not zero. But, it should avoid to write > > the register twice. So, refactor it. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 66 +++++++++++--------- > > 1 file changed, 35 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 69358dc202f0..f8926d5ec422 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -737,6 +737,40 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed); > > } > > > > > +static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes) > > Why defining a separate method? I thought that a separated patch could improve code/patch readability. > Just move it's content to the already > defined dw_pcie_link_set_max_link_width(). This could have been done in > the framework of the previous patch. I got it. > > +{ > > + u32 val; > > + > > + /* Set the number of lanes */ > > + val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); > > + val &= ~PORT_LINK_FAST_LINK_MODE; > > + val |= PORT_LINK_DLL_LINK_EN; > > + > > + /* Mask LINK_MODE if num_lanes is not zero */ > > > + if (num_lanes) > > + val &= ~PORT_LINK_MODE_MASK; > > this and... > > > + > > + switch (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_dbg(pci->dev, "Using h/w default number of lanes\n"); > > ...this change the link-width setup semantic in case if the > invalid number of lanes is specified. Your method now causes the > PORT_LINK_MODE_MASK field clearance in case if a not permitted link > width is passed. Thank you for pointed it out. I'll fix it. > > + break; > > + } > > + > > + dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val); > > +} > > + > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > { > > u32 val; > > @@ -1040,36 +1074,6 @@ void dw_pcie_setup(struct dw_pcie *pci) > > dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val); > > } > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); > > - val &= ~PORT_LINK_FAST_LINK_MODE; > > - val |= PORT_LINK_DLL_LINK_EN; > > - dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val); > > Please leave the setups above here. Link-training mode has nothing to > do with the max link width. I completely overlooked Bjorn's comment [1]. I'll keep the code on v15. [1] https://lore.kernel.org/linux-pci/20230217112101.GA3387033@bhelgaas/ > -Serge(y) > > > - > > - 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_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); > > - > > + dw_pcie_link_set_max_width(pci, pci->num_lanes); > > dw_pcie_link_set_max_link_width(pci, pci->num_lanes); > > } > > -- > > 2.25.1 > >
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 69358dc202f0..f8926d5ec422 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -737,6 +737,40 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, cap | link_speed); } +static void dw_pcie_link_set_max_width(struct dw_pcie *pci, u32 num_lanes) +{ + u32 val; + + /* Set the number of lanes */ + val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); + val &= ~PORT_LINK_FAST_LINK_MODE; + val |= PORT_LINK_DLL_LINK_EN; + + /* Mask LINK_MODE if num_lanes is not zero */ + if (num_lanes) + val &= ~PORT_LINK_MODE_MASK; + + switch (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_dbg(pci->dev, "Using h/w default number of lanes\n"); + break; + } + + dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val); +} + static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) { u32 val; @@ -1040,36 +1074,6 @@ void dw_pcie_setup(struct dw_pcie *pci) dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val); } - val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL); - val &= ~PORT_LINK_FAST_LINK_MODE; - 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_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); - + dw_pcie_link_set_max_width(pci, pci->num_lanes); dw_pcie_link_set_max_link_width(pci, pci->num_lanes); }
To improve code readability, add dw_pcie_link_set_max_width(). The original code writes the PCIE_PORT_LINK_CONTROL register twice if the pci->num_lanes is not zero. But, it should avoid to write the register twice. So, refactor it. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pci/controller/dwc/pcie-designware.c | 66 +++++++++++--------- 1 file changed, 35 insertions(+), 31 deletions(-)