| Message ID | b00bf38b4351831b97892b747d48c9206fbab9c7.1454935205.git.jpinto@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Joao > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Joao Pinto > Sent: 08 February 2016 12:44 > To: helgaas@kernel.org > Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com; Joao Pinto > Subject: [PATCH] link up validation moved to pcie-designware > > This patch goal is to centralize in pcie-designware the link up > validation. A new function was added to pci-designware that is > responsible for doing such a task. This was implemented in a form that > permits flexibility for all SoCs. Looking at the patch I don't think this is really needed, it seems to me like it makes more confusion in reading the drivers code. It is clear that each HW IP has got his own transient time in setting the link up, this is clear looking at the code of the single drivers now. With the modification you are proposing we are seeing drivers calls with magic numbers and the need for the reader to go and look into the designware framework to understand a function that is not even designware specific (in fact all these drivers need to wait different times according to their HW IP and some other drivers do not need to wait at all...) BTW I am quite new to PCIe so let's see what the other guys think... Cheers Gab > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> > --- > drivers/pci/host/pci-dra7xx.c | 11 +++-------- > drivers/pci/host/pci-exynos.c | 11 ++--------- > drivers/pci/host/pci-imx6.c | 11 +++-------- > drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ > drivers/pci/host/pcie-designware.h | 2 ++ > drivers/pci/host/pcie-spear13xx.c | 12 ++---------- > 6 files changed, 32 insertions(+), 35 deletions(-) > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > dra7xx.c > index 8c36880..e425944 100644 > --- a/drivers/pci/host/pci-dra7xx.c > +++ b/drivers/pci/host/pci-dra7xx.c > @@ -10,7 +10,6 @@ > * published by the Free Software Foundation. > */ > > -#include <linux/delay.h> > #include <linux/err.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct > pcie_port *pp) > { > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); > u32 reg; > - unsigned int retries; > > if (dw_pcie_link_up(pp)) { > dev_err(pp->dev, "link is already up\n"); > @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct > pcie_port *pp) > reg |= LTSSM_EN; > dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); > > - for (retries = 0; retries < 1000; retries++) { > - if (dw_pcie_link_up(pp)) > - return 0; > - usleep_range(10, 20); > - } > + /* check if the link is up or not */ > + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) > + return 0; > > - dev_err(pp->dev, "link is not up\n"); > return -EINVAL; > } > > diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci- > exynos.c > index 01095e1..770c45a 100644 > --- a/drivers/pci/host/pci-exynos.c > +++ b/drivers/pci/host/pci-exynos.c > @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct > pcie_port *pp) > { > struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); > u32 val; > - unsigned int retries; > > if (dw_pcie_link_up(pp)) { > dev_err(pp->dev, "Link already up\n"); > @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct > pcie_port *pp) > PCIE_APP_LTSSM_ENABLE); > > /* check if the link is up or not */ > - for (retries = 0; retries < 10; retries++) { > - if (dw_pcie_link_up(pp)) { > - dev_info(pp->dev, "Link up\n"); > - return 0; > - } > - mdelay(100); > - } > + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > + return 0; > > while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { > val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); > @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct > pcie_port *pp) > /* power off phy */ > exynos_pcie_power_off_phy(pp); > > - dev_err(pp->dev, "PCIe Link Fail\n"); > return -EINVAL; > } > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 22e8224..9e323be 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct pcie_port > *pp) > > static int imx6_pcie_wait_for_link(struct pcie_port *pp) > { > - unsigned int retries; > - > - for (retries = 0; retries < 200; retries++) { > - if (dw_pcie_link_up(pp)) > - return 0; > - usleep_range(100, 1000); > - } > + /* check if the link is up or not */ > + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) > + return 0; > > - dev_err(pp->dev, "phy link never came up\n"); > dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", > readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), > readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); > diff --git a/drivers/pci/host/pcie-designware.c > b/drivers/pci/host/pcie-designware.c > index 540f077..6725231 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -22,6 +22,7 @@ > #include <linux/pci_regs.h> > #include <linux/platform_device.h> > #include <linux/types.h> > +#include <linux/delay.h> > > #include "pcie-designware.h" > > @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = { > .teardown_irq = dw_msi_teardown_irq, > }; > > +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > sleep_min, > + int sleep_max) > +{ > + int retries; > + > + /* check if the link is up or not */ > + for (retries = 0; retries < max_ret; retries++) { > + if (dw_pcie_link_up(pp)) { > + dev_info(pp->dev, "link up\n"); > + return 0; > + } > + usleep_range(sleep_min, sleep_max); > + } > + > + dev_err(pp->dev, "phy link never came up\n"); > + > + return 1; > +} > + > int dw_pcie_link_up(struct pcie_port *pp) > { > if (pp->ops->link_up) > diff --git a/drivers/pci/host/pcie-designware.h > b/drivers/pci/host/pcie-designware.h > index 2356d29..b077fe0 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, > u32 *val); > int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); > irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); > void dw_pcie_msi_init(struct pcie_port *pp); > +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > sleep_min, > + int sleep_max); > int dw_pcie_link_up(struct pcie_port *pp); > void dw_pcie_setup_rc(struct pcie_port *pp); > int dw_pcie_host_init(struct pcie_port *pp); > diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie- > spear13xx.c > index b95b756..5c24595 100644 > --- a/drivers/pci/host/pcie-spear13xx.c > +++ b/drivers/pci/host/pcie-spear13xx.c > @@ -13,7 +13,6 @@ > */ > > #include <linux/clk.h> > -#include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > #include <linux/module.h> > @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct > pcie_port *pp) > struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > u32 exp_cap_off = EXP_CAP_ID_OFFSET; > - unsigned int retries; > > if (dw_pcie_link_up(pp)) { > dev_err(pp->dev, "link already up\n"); > @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct > pcie_port *pp) > &app_reg->app_ctrl_0); > > /* check if the link is up or not */ > - for (retries = 0; retries < 10; retries++) { > - if (dw_pcie_link_up(pp)) { > - dev_info(pp->dev, "link up\n"); > - return 0; > - } > - mdelay(100); > - } > + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > + return 0; > > - dev_err(pp->dev, "link Fail\n"); > return -EINVAL; > } > > -- > 1.8.1.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Gab, This patch is part of the on going work of integrating a designware platform driver that is SoC agnostic. This code replication issue was raised during discussions. Thanks Joao On 2/8/2016 1:03 PM, Gabriele Paoloni wrote: > Hi Joao > >> -----Original Message----- >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >> owner@vger.kernel.org] On Behalf Of Joao Pinto >> Sent: 08 February 2016 12:44 >> To: helgaas@kernel.org >> Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- >> kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com; Joao Pinto >> Subject: [PATCH] link up validation moved to pcie-designware >> >> This patch goal is to centralize in pcie-designware the link up >> validation. A new function was added to pci-designware that is >> responsible for doing such a task. This was implemented in a form that >> permits flexibility for all SoCs. > > Looking at the patch I don't think this is really needed, > it seems to me like it makes more confusion in reading the > drivers code. > > It is clear that each HW IP has got his own transient time > in setting the link up, this is clear looking at the code > of the single drivers now. > > With the modification you are proposing we are seeing > drivers calls with magic numbers and the need for the > reader to go and look into the designware framework to > understand a function that is not even designware > specific (in fact all these drivers need to wait different > times according to their HW IP and some other drivers do > not need to wait at all...) > > BTW I am quite new to PCIe so let's see what the other guys > think... > > Cheers > > Gab > >> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >> --- >> drivers/pci/host/pci-dra7xx.c | 11 +++-------- >> drivers/pci/host/pci-exynos.c | 11 ++--------- >> drivers/pci/host/pci-imx6.c | 11 +++-------- >> drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ >> drivers/pci/host/pcie-designware.h | 2 ++ >> drivers/pci/host/pcie-spear13xx.c | 12 ++---------- >> 6 files changed, 32 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- >> dra7xx.c >> index 8c36880..e425944 100644 >> --- a/drivers/pci/host/pci-dra7xx.c >> +++ b/drivers/pci/host/pci-dra7xx.c >> @@ -10,7 +10,6 @@ >> * published by the Free Software Foundation. >> */ >> >> -#include <linux/delay.h> >> #include <linux/err.h> >> #include <linux/interrupt.h> >> #include <linux/irq.h> >> @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct >> pcie_port *pp) >> { >> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); >> u32 reg; >> - unsigned int retries; >> >> if (dw_pcie_link_up(pp)) { >> dev_err(pp->dev, "link is already up\n"); >> @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct >> pcie_port *pp) >> reg |= LTSSM_EN; >> dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); >> >> - for (retries = 0; retries < 1000; retries++) { >> - if (dw_pcie_link_up(pp)) >> - return 0; >> - usleep_range(10, 20); >> - } >> + /* check if the link is up or not */ >> + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) >> + return 0; >> >> - dev_err(pp->dev, "link is not up\n"); >> return -EINVAL; >> } >> >> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci- >> exynos.c >> index 01095e1..770c45a 100644 >> --- a/drivers/pci/host/pci-exynos.c >> +++ b/drivers/pci/host/pci-exynos.c >> @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct >> pcie_port *pp) >> { >> struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); >> u32 val; >> - unsigned int retries; >> >> if (dw_pcie_link_up(pp)) { >> dev_err(pp->dev, "Link already up\n"); >> @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct >> pcie_port *pp) >> PCIE_APP_LTSSM_ENABLE); >> >> /* check if the link is up or not */ >> - for (retries = 0; retries < 10; retries++) { >> - if (dw_pcie_link_up(pp)) { >> - dev_info(pp->dev, "Link up\n"); >> - return 0; >> - } >> - mdelay(100); >> - } >> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) >> + return 0; >> >> while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { >> val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); >> @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct >> pcie_port *pp) >> /* power off phy */ >> exynos_pcie_power_off_phy(pp); >> >> - dev_err(pp->dev, "PCIe Link Fail\n"); >> return -EINVAL; >> } >> >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >> index 22e8224..9e323be 100644 >> --- a/drivers/pci/host/pci-imx6.c >> +++ b/drivers/pci/host/pci-imx6.c >> @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct pcie_port >> *pp) >> >> static int imx6_pcie_wait_for_link(struct pcie_port *pp) >> { >> - unsigned int retries; >> - >> - for (retries = 0; retries < 200; retries++) { >> - if (dw_pcie_link_up(pp)) >> - return 0; >> - usleep_range(100, 1000); >> - } >> + /* check if the link is up or not */ >> + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) >> + return 0; >> >> - dev_err(pp->dev, "phy link never came up\n"); >> dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", >> readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), >> readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); >> diff --git a/drivers/pci/host/pcie-designware.c >> b/drivers/pci/host/pcie-designware.c >> index 540f077..6725231 100644 >> --- a/drivers/pci/host/pcie-designware.c >> +++ b/drivers/pci/host/pcie-designware.c >> @@ -22,6 +22,7 @@ >> #include <linux/pci_regs.h> >> #include <linux/platform_device.h> >> #include <linux/types.h> >> +#include <linux/delay.h> >> >> #include "pcie-designware.h" >> >> @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = { >> .teardown_irq = dw_msi_teardown_irq, >> }; >> >> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int >> sleep_min, >> + int sleep_max) >> +{ >> + int retries; >> + >> + /* check if the link is up or not */ >> + for (retries = 0; retries < max_ret; retries++) { >> + if (dw_pcie_link_up(pp)) { >> + dev_info(pp->dev, "link up\n"); >> + return 0; >> + } >> + usleep_range(sleep_min, sleep_max); >> + } >> + >> + dev_err(pp->dev, "phy link never came up\n"); >> + >> + return 1; >> +} >> + >> int dw_pcie_link_up(struct pcie_port *pp) >> { >> if (pp->ops->link_up) >> diff --git a/drivers/pci/host/pcie-designware.h >> b/drivers/pci/host/pcie-designware.h >> index 2356d29..b077fe0 100644 >> --- a/drivers/pci/host/pcie-designware.h >> +++ b/drivers/pci/host/pcie-designware.h >> @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, >> u32 *val); >> int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); >> irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); >> void dw_pcie_msi_init(struct pcie_port *pp); >> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int >> sleep_min, >> + int sleep_max); >> int dw_pcie_link_up(struct pcie_port *pp); >> void dw_pcie_setup_rc(struct pcie_port *pp); >> int dw_pcie_host_init(struct pcie_port *pp); >> diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie- >> spear13xx.c >> index b95b756..5c24595 100644 >> --- a/drivers/pci/host/pcie-spear13xx.c >> +++ b/drivers/pci/host/pcie-spear13xx.c >> @@ -13,7 +13,6 @@ >> */ >> >> #include <linux/clk.h> >> -#include <linux/delay.h> >> #include <linux/interrupt.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct >> pcie_port *pp) >> struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); >> struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; >> u32 exp_cap_off = EXP_CAP_ID_OFFSET; >> - unsigned int retries; >> >> if (dw_pcie_link_up(pp)) { >> dev_err(pp->dev, "link already up\n"); >> @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct >> pcie_port *pp) >> &app_reg->app_ctrl_0); >> >> /* check if the link is up or not */ >> - for (retries = 0; retries < 10; retries++) { >> - if (dw_pcie_link_up(pp)) { >> - dev_info(pp->dev, "link up\n"); >> - return 0; >> - } >> - mdelay(100); >> - } >> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) >> + return 0; >> >> - dev_err(pp->dev, "link Fail\n"); >> return -EINVAL; >> } >> >> -- >> 1.8.1.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pci" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > Sent: 08 February 2016 15:12 > To: Gabriele Paoloni; Joao Pinto; helgaas@kernel.org > Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com > Subject: Re: [PATCH] link up validation moved to pcie-designware > > > Hi Gab, > This patch is part of the on going work of integrating a designware > platform > driver that is SoC agnostic. This code replication issue was raised > during > discussions. Ok sorry, I missed it then Cheers Gab > > Thanks > Joao > > On 2/8/2016 1:03 PM, Gabriele Paoloni wrote: > > Hi Joao > > > >> -----Original Message----- > >> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > >> owner@vger.kernel.org] On Behalf Of Joao Pinto > >> Sent: 08 February 2016 12:44 > >> To: helgaas@kernel.org > >> Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- > >> kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com; Joao Pinto > >> Subject: [PATCH] link up validation moved to pcie-designware > >> > >> This patch goal is to centralize in pcie-designware the link up > >> validation. A new function was added to pci-designware that is > >> responsible for doing such a task. This was implemented in a form > that > >> permits flexibility for all SoCs. > > > > Looking at the patch I don't think this is really needed, > > it seems to me like it makes more confusion in reading the > > drivers code. > > > > It is clear that each HW IP has got his own transient time > > in setting the link up, this is clear looking at the code > > of the single drivers now. > > > > With the modification you are proposing we are seeing > > drivers calls with magic numbers and the need for the > > reader to go and look into the designware framework to > > understand a function that is not even designware > > specific (in fact all these drivers need to wait different > > times according to their HW IP and some other drivers do > > not need to wait at all...) > > > > BTW I am quite new to PCIe so let's see what the other guys > > think... > > > > Cheers > > > > Gab > > > >> > >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > >> --- > >> drivers/pci/host/pci-dra7xx.c | 11 +++-------- > >> drivers/pci/host/pci-exynos.c | 11 ++--------- > >> drivers/pci/host/pci-imx6.c | 11 +++-------- > >> drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ > >> drivers/pci/host/pcie-designware.h | 2 ++ > >> drivers/pci/host/pcie-spear13xx.c | 12 ++---------- > >> 6 files changed, 32 insertions(+), 35 deletions(-) > >> > >> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > >> dra7xx.c > >> index 8c36880..e425944 100644 > >> --- a/drivers/pci/host/pci-dra7xx.c > >> +++ b/drivers/pci/host/pci-dra7xx.c > >> @@ -10,7 +10,6 @@ > >> * published by the Free Software Foundation. > >> */ > >> > >> -#include <linux/delay.h> > >> #include <linux/err.h> > >> #include <linux/interrupt.h> > >> #include <linux/irq.h> > >> @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct > >> pcie_port *pp) > >> { > >> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); > >> u32 reg; > >> - unsigned int retries; > >> > >> if (dw_pcie_link_up(pp)) { > >> dev_err(pp->dev, "link is already up\n"); > >> @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct > >> pcie_port *pp) > >> reg |= LTSSM_EN; > >> dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); > >> > >> - for (retries = 0; retries < 1000; retries++) { > >> - if (dw_pcie_link_up(pp)) > >> - return 0; > >> - usleep_range(10, 20); > >> - } > >> + /* check if the link is up or not */ > >> + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) > >> + return 0; > >> > >> - dev_err(pp->dev, "link is not up\n"); > >> return -EINVAL; > >> } > >> > >> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci- > >> exynos.c > >> index 01095e1..770c45a 100644 > >> --- a/drivers/pci/host/pci-exynos.c > >> +++ b/drivers/pci/host/pci-exynos.c > >> @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct > >> pcie_port *pp) > >> { > >> struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); > >> u32 val; > >> - unsigned int retries; > >> > >> if (dw_pcie_link_up(pp)) { > >> dev_err(pp->dev, "Link already up\n"); > >> @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct > >> pcie_port *pp) > >> PCIE_APP_LTSSM_ENABLE); > >> > >> /* check if the link is up or not */ > >> - for (retries = 0; retries < 10; retries++) { > >> - if (dw_pcie_link_up(pp)) { > >> - dev_info(pp->dev, "Link up\n"); > >> - return 0; > >> - } > >> - mdelay(100); > >> - } > >> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > >> + return 0; > >> > >> while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { > >> val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); > >> @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct > >> pcie_port *pp) > >> /* power off phy */ > >> exynos_pcie_power_off_phy(pp); > >> > >> - dev_err(pp->dev, "PCIe Link Fail\n"); > >> return -EINVAL; > >> } > >> > >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci- > imx6.c > >> index 22e8224..9e323be 100644 > >> --- a/drivers/pci/host/pci-imx6.c > >> +++ b/drivers/pci/host/pci-imx6.c > >> @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct > pcie_port > >> *pp) > >> > >> static int imx6_pcie_wait_for_link(struct pcie_port *pp) > >> { > >> - unsigned int retries; > >> - > >> - for (retries = 0; retries < 200; retries++) { > >> - if (dw_pcie_link_up(pp)) > >> - return 0; > >> - usleep_range(100, 1000); > >> - } > >> + /* check if the link is up or not */ > >> + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) > >> + return 0; > >> > >> - dev_err(pp->dev, "phy link never came up\n"); > >> dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", > >> readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), > >> readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); > >> diff --git a/drivers/pci/host/pcie-designware.c > >> b/drivers/pci/host/pcie-designware.c > >> index 540f077..6725231 100644 > >> --- a/drivers/pci/host/pcie-designware.c > >> +++ b/drivers/pci/host/pcie-designware.c > >> @@ -22,6 +22,7 @@ > >> #include <linux/pci_regs.h> > >> #include <linux/platform_device.h> > >> #include <linux/types.h> > >> +#include <linux/delay.h> > >> > >> #include "pcie-designware.h" > >> > >> @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = > { > >> .teardown_irq = dw_msi_teardown_irq, > >> }; > >> > >> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > >> sleep_min, > >> + int sleep_max) > >> +{ > >> + int retries; > >> + > >> + /* check if the link is up or not */ > >> + for (retries = 0; retries < max_ret; retries++) { > >> + if (dw_pcie_link_up(pp)) { > >> + dev_info(pp->dev, "link up\n"); > >> + return 0; > >> + } > >> + usleep_range(sleep_min, sleep_max); > >> + } > >> + > >> + dev_err(pp->dev, "phy link never came up\n"); > >> + > >> + return 1; > >> +} > >> + > >> int dw_pcie_link_up(struct pcie_port *pp) > >> { > >> if (pp->ops->link_up) > >> diff --git a/drivers/pci/host/pcie-designware.h > >> b/drivers/pci/host/pcie-designware.h > >> index 2356d29..b077fe0 100644 > >> --- a/drivers/pci/host/pcie-designware.h > >> +++ b/drivers/pci/host/pcie-designware.h > >> @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, > >> u32 *val); > >> int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); > >> irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); > >> void dw_pcie_msi_init(struct pcie_port *pp); > >> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > >> sleep_min, > >> + int sleep_max); > >> int dw_pcie_link_up(struct pcie_port *pp); > >> void dw_pcie_setup_rc(struct pcie_port *pp); > >> int dw_pcie_host_init(struct pcie_port *pp); > >> diff --git a/drivers/pci/host/pcie-spear13xx.c > b/drivers/pci/host/pcie- > >> spear13xx.c > >> index b95b756..5c24595 100644 > >> --- a/drivers/pci/host/pcie-spear13xx.c > >> +++ b/drivers/pci/host/pcie-spear13xx.c > >> @@ -13,7 +13,6 @@ > >> */ > >> > >> #include <linux/clk.h> > >> -#include <linux/delay.h> > >> #include <linux/interrupt.h> > >> #include <linux/kernel.h> > >> #include <linux/module.h> > >> @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct > >> pcie_port *pp) > >> struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > >> struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > >> u32 exp_cap_off = EXP_CAP_ID_OFFSET; > >> - unsigned int retries; > >> > >> if (dw_pcie_link_up(pp)) { > >> dev_err(pp->dev, "link already up\n"); > >> @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct > >> pcie_port *pp) > >> &app_reg->app_ctrl_0); > >> > >> /* check if the link is up or not */ > >> - for (retries = 0; retries < 10; retries++) { > >> - if (dw_pcie_link_up(pp)) { > >> - dev_info(pp->dev, "link up\n"); > >> - return 0; > >> - } > >> - mdelay(100); > >> - } > >> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > >> + return 0; > >> > >> - dev_err(pp->dev, "link Fail\n"); > >> return -EINVAL; > >> } > >> > >> -- > >> 1.8.1.5 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-pci" > in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
No problem! Thanks for your comments! Joao On 2/8/2016 3:29 PM, Gabriele Paoloni wrote: >> -----Original Message----- >> From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] >> Sent: 08 February 2016 15:12 >> To: Gabriele Paoloni; Joao Pinto; helgaas@kernel.org >> Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- >> kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com >> Subject: Re: [PATCH] link up validation moved to pcie-designware >> >> >> Hi Gab, >> This patch is part of the on going work of integrating a designware >> platform >> driver that is SoC agnostic. This code replication issue was raised >> during >> discussions. > > Ok sorry, I missed it then > > Cheers > > Gab > > >> >> Thanks >> Joao >> >> On 2/8/2016 1:03 PM, Gabriele Paoloni wrote: >>> Hi Joao >>> >>>> -----Original Message----- >>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- >>>> owner@vger.kernel.org] On Behalf Of Joao Pinto >>>> Sent: 08 February 2016 12:44 >>>> To: helgaas@kernel.org >>>> Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- >>>> kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com; Joao Pinto >>>> Subject: [PATCH] link up validation moved to pcie-designware >>>> >>>> This patch goal is to centralize in pcie-designware the link up >>>> validation. A new function was added to pci-designware that is >>>> responsible for doing such a task. This was implemented in a form >> that >>>> permits flexibility for all SoCs. >>> >>> Looking at the patch I don't think this is really needed, >>> it seems to me like it makes more confusion in reading the >>> drivers code. >>> >>> It is clear that each HW IP has got his own transient time >>> in setting the link up, this is clear looking at the code >>> of the single drivers now. >>> >>> With the modification you are proposing we are seeing >>> drivers calls with magic numbers and the need for the >>> reader to go and look into the designware framework to >>> understand a function that is not even designware >>> specific (in fact all these drivers need to wait different >>> times according to their HW IP and some other drivers do >>> not need to wait at all...) >>> >>> BTW I am quite new to PCIe so let's see what the other guys >>> think... >>> >>> Cheers >>> >>> Gab >>> >>>> >>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>>> --- >>>> drivers/pci/host/pci-dra7xx.c | 11 +++-------- >>>> drivers/pci/host/pci-exynos.c | 11 ++--------- >>>> drivers/pci/host/pci-imx6.c | 11 +++-------- >>>> drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ >>>> drivers/pci/host/pcie-designware.h | 2 ++ >>>> drivers/pci/host/pcie-spear13xx.c | 12 ++---------- >>>> 6 files changed, 32 insertions(+), 35 deletions(-) >>>> >>>> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- >>>> dra7xx.c >>>> index 8c36880..e425944 100644 >>>> --- a/drivers/pci/host/pci-dra7xx.c >>>> +++ b/drivers/pci/host/pci-dra7xx.c >>>> @@ -10,7 +10,6 @@ >>>> * published by the Free Software Foundation. >>>> */ >>>> >>>> -#include <linux/delay.h> >>>> #include <linux/err.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/irq.h> >>>> @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct >>>> pcie_port *pp) >>>> { >>>> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); >>>> u32 reg; >>>> - unsigned int retries; >>>> >>>> if (dw_pcie_link_up(pp)) { >>>> dev_err(pp->dev, "link is already up\n"); >>>> @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct >>>> pcie_port *pp) >>>> reg |= LTSSM_EN; >>>> dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); >>>> >>>> - for (retries = 0; retries < 1000; retries++) { >>>> - if (dw_pcie_link_up(pp)) >>>> - return 0; >>>> - usleep_range(10, 20); >>>> - } >>>> + /* check if the link is up or not */ >>>> + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) >>>> + return 0; >>>> >>>> - dev_err(pp->dev, "link is not up\n"); >>>> return -EINVAL; >>>> } >>>> >>>> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci- >>>> exynos.c >>>> index 01095e1..770c45a 100644 >>>> --- a/drivers/pci/host/pci-exynos.c >>>> +++ b/drivers/pci/host/pci-exynos.c >>>> @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct >>>> pcie_port *pp) >>>> { >>>> struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); >>>> u32 val; >>>> - unsigned int retries; >>>> >>>> if (dw_pcie_link_up(pp)) { >>>> dev_err(pp->dev, "Link already up\n"); >>>> @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct >>>> pcie_port *pp) >>>> PCIE_APP_LTSSM_ENABLE); >>>> >>>> /* check if the link is up or not */ >>>> - for (retries = 0; retries < 10; retries++) { >>>> - if (dw_pcie_link_up(pp)) { >>>> - dev_info(pp->dev, "Link up\n"); >>>> - return 0; >>>> - } >>>> - mdelay(100); >>>> - } >>>> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) >>>> + return 0; >>>> >>>> while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { >>>> val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); >>>> @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct >>>> pcie_port *pp) >>>> /* power off phy */ >>>> exynos_pcie_power_off_phy(pp); >>>> >>>> - dev_err(pp->dev, "PCIe Link Fail\n"); >>>> return -EINVAL; >>>> } >>>> >>>> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci- >> imx6.c >>>> index 22e8224..9e323be 100644 >>>> --- a/drivers/pci/host/pci-imx6.c >>>> +++ b/drivers/pci/host/pci-imx6.c >>>> @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct >> pcie_port >>>> *pp) >>>> >>>> static int imx6_pcie_wait_for_link(struct pcie_port *pp) >>>> { >>>> - unsigned int retries; >>>> - >>>> - for (retries = 0; retries < 200; retries++) { >>>> - if (dw_pcie_link_up(pp)) >>>> - return 0; >>>> - usleep_range(100, 1000); >>>> - } >>>> + /* check if the link is up or not */ >>>> + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) >>>> + return 0; >>>> >>>> - dev_err(pp->dev, "phy link never came up\n"); >>>> dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", >>>> readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), >>>> readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); >>>> diff --git a/drivers/pci/host/pcie-designware.c >>>> b/drivers/pci/host/pcie-designware.c >>>> index 540f077..6725231 100644 >>>> --- a/drivers/pci/host/pcie-designware.c >>>> +++ b/drivers/pci/host/pcie-designware.c >>>> @@ -22,6 +22,7 @@ >>>> #include <linux/pci_regs.h> >>>> #include <linux/platform_device.h> >>>> #include <linux/types.h> >>>> +#include <linux/delay.h> >>>> >>>> #include "pcie-designware.h" >>>> >>>> @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = >> { >>>> .teardown_irq = dw_msi_teardown_irq, >>>> }; >>>> >>>> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int >>>> sleep_min, >>>> + int sleep_max) >>>> +{ >>>> + int retries; >>>> + >>>> + /* check if the link is up or not */ >>>> + for (retries = 0; retries < max_ret; retries++) { >>>> + if (dw_pcie_link_up(pp)) { >>>> + dev_info(pp->dev, "link up\n"); >>>> + return 0; >>>> + } >>>> + usleep_range(sleep_min, sleep_max); >>>> + } >>>> + >>>> + dev_err(pp->dev, "phy link never came up\n"); >>>> + >>>> + return 1; >>>> +} >>>> + >>>> int dw_pcie_link_up(struct pcie_port *pp) >>>> { >>>> if (pp->ops->link_up) >>>> diff --git a/drivers/pci/host/pcie-designware.h >>>> b/drivers/pci/host/pcie-designware.h >>>> index 2356d29..b077fe0 100644 >>>> --- a/drivers/pci/host/pcie-designware.h >>>> +++ b/drivers/pci/host/pcie-designware.h >>>> @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, >>>> u32 *val); >>>> int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); >>>> irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); >>>> void dw_pcie_msi_init(struct pcie_port *pp); >>>> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int >>>> sleep_min, >>>> + int sleep_max); >>>> int dw_pcie_link_up(struct pcie_port *pp); >>>> void dw_pcie_setup_rc(struct pcie_port *pp); >>>> int dw_pcie_host_init(struct pcie_port *pp); >>>> diff --git a/drivers/pci/host/pcie-spear13xx.c >> b/drivers/pci/host/pcie- >>>> spear13xx.c >>>> index b95b756..5c24595 100644 >>>> --- a/drivers/pci/host/pcie-spear13xx.c >>>> +++ b/drivers/pci/host/pcie-spear13xx.c >>>> @@ -13,7 +13,6 @@ >>>> */ >>>> >>>> #include <linux/clk.h> >>>> -#include <linux/delay.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct >>>> pcie_port *pp) >>>> struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); >>>> struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; >>>> u32 exp_cap_off = EXP_CAP_ID_OFFSET; >>>> - unsigned int retries; >>>> >>>> if (dw_pcie_link_up(pp)) { >>>> dev_err(pp->dev, "link already up\n"); >>>> @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct >>>> pcie_port *pp) >>>> &app_reg->app_ctrl_0); >>>> >>>> /* check if the link is up or not */ >>>> - for (retries = 0; retries < 10; retries++) { >>>> - if (dw_pcie_link_up(pp)) { >>>> - dev_info(pp->dev, "link up\n"); >>>> - return 0; >>>> - } >>>> - mdelay(100); >>>> - } >>>> + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) >>>> + return 0; >>>> >>>> - dev_err(pp->dev, "link Fail\n"); >>>> return -EINVAL; >>>> } >>>> >>>> -- >>>> 1.8.1.5 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" >> in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 February 2016 12:43:58 Joao Pinto wrote: > This patch goal is to centralize in pcie-designware the link up > validation. A new function was added to pci-designware that is > responsible for doing such a task. This was implemented in a form that > permits flexibility for all SoCs. > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> I believe the different timeouts in each driver are just coincidence and partly based on who reviewed which driver. I'm pretty sure it doesn't really matter at all, and we can just use the same loop for every one. I was also hoping that a little more than just the loop could be unified between the various establish_link() functions. I have a feeling that each one just implements a subset of what is actually required, so they are buggy in different ways because nobody has the datasheet. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 08, 2016 at 12:43:58PM +0000, Joao Pinto wrote: > This patch goal is to centralize in pcie-designware the link up > validation. A new function was added to pci-designware that is > responsible for doing such a task. This was implemented in a form that > permits flexibility for all SoCs. > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> > --- > drivers/pci/host/pci-dra7xx.c | 11 +++-------- > drivers/pci/host/pci-exynos.c | 11 ++--------- > drivers/pci/host/pci-imx6.c | 11 +++-------- > drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ > drivers/pci/host/pcie-designware.h | 2 ++ > drivers/pci/host/pcie-spear13xx.c | 12 ++---------- > 6 files changed, 32 insertions(+), 35 deletions(-) > +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int sleep_min, > + int sleep_max) I think "dw_pcie_wait_for_link()" would be a more descriptive name. I doubt that the variations between drivers in number of retries and amount of time to wait are meaningful. I suspect most of those numbers are made up or copied from other drivers. So we might not need the max_ret, sleep_min, and sleep_max parameters at all. Even if there really are important differences, I suspect the only important thing is the total time we're prepared to wait, and we can leave it up to dw_pcie_wait_for_link() to decide how to split that up into sleep ranges and retries. > +{ > + int retries; > + > + /* check if the link is up or not */ > + for (retries = 0; retries < max_ret; retries++) { > + if (dw_pcie_link_up(pp)) { > + dev_info(pp->dev, "link up\n"); > + return 0; > + } > + usleep_range(sleep_min, sleep_max); > + } > + > + dev_err(pp->dev, "phy link never came up\n"); > + > + return 1; > +} -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Ok, so what should be the retries and waiting time in your opinion? The most typical is: retries: 10 delay: 100ms (usleep_range (90000, 100000)) These values should be ok? I am already testing a full pcie-designware platform driver. Joao On 2/8/2016 4:41 PM, Bjorn Helgaas wrote: > On Mon, Feb 08, 2016 at 12:43:58PM +0000, Joao Pinto wrote: >> This patch goal is to centralize in pcie-designware the link up >> validation. A new function was added to pci-designware that is >> responsible for doing such a task. This was implemented in a form that >> permits flexibility for all SoCs. >> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >> --- >> drivers/pci/host/pci-dra7xx.c | 11 +++-------- >> drivers/pci/host/pci-exynos.c | 11 ++--------- >> drivers/pci/host/pci-imx6.c | 11 +++-------- >> drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ >> drivers/pci/host/pcie-designware.h | 2 ++ >> drivers/pci/host/pcie-spear13xx.c | 12 ++---------- >> 6 files changed, 32 insertions(+), 35 deletions(-) > >> +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int sleep_min, >> + int sleep_max) > > I think "dw_pcie_wait_for_link()" would be a more descriptive name. > > I doubt that the variations between drivers in number of retries and > amount of time to wait are meaningful. I suspect most of those > numbers are made up or copied from other drivers. So we might not > need the max_ret, sleep_min, and sleep_max parameters at all. > > Even if there really are important differences, I suspect the only > important thing is the total time we're prepared to wait, and we can > leave it up to dw_pcie_wait_for_link() to decide how to split that up > into sleep ranges and retries. > >> +{ >> + int retries; >> + >> + /* check if the link is up or not */ >> + for (retries = 0; retries < max_ret; retries++) { >> + if (dw_pcie_link_up(pp)) { >> + dev_info(pp->dev, "link up\n"); >> + return 0; >> + } >> + usleep_range(sleep_min, sleep_max); >> + } >> + >> + dev_err(pp->dev, "phy link never came up\n"); >> + >> + return 1; >> +} -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 February 2016 16:43:33 Joao Pinto wrote: > Hi, > Ok, so what should be the retries and waiting time in your opinion? > The most typical is: > > retries: 10 > delay: 100ms (usleep_range (90000, 100000)) > > These values should be ok? > > I am already testing a full pcie-designware platform driver. > > You are the one with the datasheet, not me. ;-) Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Our On 2/8/2016 4:46 PM, Arnd Bergmann wrote: > On Monday 08 February 2016 16:43:33 Joao Pinto wrote: >> Hi, >> Ok, so what should be the retries and waiting time in your opinion? >> The most typical is: >> >> retries: 10 >> delay: 100ms (usleep_range (90000, 100000)) >> >> These values should be ok? >> >> I am already testing a full pcie-designware platform driver. >> >> > You are the one with the datasheet, not me. ;-) Our reference driver follows the 10x with 100ms delay between retries, so lets follow that value. Agree? > > Arnd > Joao -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 February 2016 16:48:07 Joao Pinto wrote: > On 2/8/2016 4:46 PM, Arnd Bergmann wrote: > > On Monday 08 February 2016 16:43:33 Joao Pinto wrote: > >> Hi, > >> Ok, so what should be the retries and waiting time in your opinion? > >> The most typical is: > >> > >> retries: 10 > >> delay: 100ms (usleep_range (90000, 100000)) > >> > >> These values should be ok? > >> > >> I am already testing a full pcie-designware platform driver. > >> > >> > > You are the one with the datasheet, not me. > > Our reference driver follows the 10x with 100ms delay between retries, so lets > follow that value. Agree? > I wouldn't trust the reference driver too much, better follow whatever the hardware specifications says. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Gabriele, On Mon, Feb 08, 2016 at 01:03:16PM +0000, Gabriele Paoloni wrote: > > -----Original Message----- > > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > > owner@vger.kernel.org] On Behalf Of Joao Pinto > > Sent: 08 February 2016 12:44 > > To: helgaas@kernel.org > > Cc: arnd@arndb.de; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org; CARLOS.PALMINHA@synopsys.com; Joao Pinto > > Subject: [PATCH] link up validation moved to pcie-designware > > > > This patch goal is to centralize in pcie-designware the link up > > validation. A new function was added to pci-designware that is > > responsible for doing such a task. This was implemented in a form that > > permits flexibility for all SoCs. > > Looking at the patch I don't think this is really needed, > it seems to me like it makes more confusion in reading the > drivers code. > > It is clear that each HW IP has got his own transient time > in setting the link up, this is clear looking at the code > of the single drivers now. > > With the modification you are proposing we are seeing > drivers calls with magic numbers and the need for the > reader to go and look into the designware framework to > understand a function that is not even designware > specific (in fact all these drivers need to wait different > times according to their HW IP and some other drivers do > not need to wait at all...) Thanks for taking a look at this, Gabriele. If it's feasible, my preference is to have a single shared timeout loop rather than several copies that are implemented slightly differently. I don't know the hardware variants, so it's quite likely that they may actually have different timeout requirements as you suggest. For example, one board might be able to give up after 0.1 second, while another might want to continue waiting for up to 1.0 second. But I'm not sure there's real value in tightening this up per-board. It seems like using the maximum timeout, e.g., 1.0 second, for everybody would be sufficient, even if it makes the first board wait longer than it needs to. After all, something is already wrong if the link didn't come up, so waiting an extra 0.9 second in a failure case doesn't seem like a problem. > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> > > --- > > drivers/pci/host/pci-dra7xx.c | 11 +++-------- > > drivers/pci/host/pci-exynos.c | 11 ++--------- > > drivers/pci/host/pci-imx6.c | 11 +++-------- > > drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ > > drivers/pci/host/pcie-designware.h | 2 ++ > > drivers/pci/host/pcie-spear13xx.c | 12 ++---------- > > 6 files changed, 32 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci- > > dra7xx.c > > index 8c36880..e425944 100644 > > --- a/drivers/pci/host/pci-dra7xx.c > > +++ b/drivers/pci/host/pci-dra7xx.c > > @@ -10,7 +10,6 @@ > > * published by the Free Software Foundation. > > */ > > > > -#include <linux/delay.h> > > #include <linux/err.h> > > #include <linux/interrupt.h> > > #include <linux/irq.h> > > @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct > > pcie_port *pp) > > { > > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); > > u32 reg; > > - unsigned int retries; > > > > if (dw_pcie_link_up(pp)) { > > dev_err(pp->dev, "link is already up\n"); > > @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct > > pcie_port *pp) > > reg |= LTSSM_EN; > > dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); > > > > - for (retries = 0; retries < 1000; retries++) { > > - if (dw_pcie_link_up(pp)) > > - return 0; > > - usleep_range(10, 20); > > - } > > + /* check if the link is up or not */ > > + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) > > + return 0; > > > > - dev_err(pp->dev, "link is not up\n"); > > return -EINVAL; > > } > > > > diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci- > > exynos.c > > index 01095e1..770c45a 100644 > > --- a/drivers/pci/host/pci-exynos.c > > +++ b/drivers/pci/host/pci-exynos.c > > @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct > > pcie_port *pp) > > { > > struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); > > u32 val; > > - unsigned int retries; > > > > if (dw_pcie_link_up(pp)) { > > dev_err(pp->dev, "Link already up\n"); > > @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct > > pcie_port *pp) > > PCIE_APP_LTSSM_ENABLE); > > > > /* check if the link is up or not */ > > - for (retries = 0; retries < 10; retries++) { > > - if (dw_pcie_link_up(pp)) { > > - dev_info(pp->dev, "Link up\n"); > > - return 0; > > - } > > - mdelay(100); > > - } > > + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > > + return 0; > > > > while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { > > val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); > > @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct > > pcie_port *pp) > > /* power off phy */ > > exynos_pcie_power_off_phy(pp); > > > > - dev_err(pp->dev, "PCIe Link Fail\n"); > > return -EINVAL; > > } > > > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > > index 22e8224..9e323be 100644 > > --- a/drivers/pci/host/pci-imx6.c > > +++ b/drivers/pci/host/pci-imx6.c > > @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct pcie_port > > *pp) > > > > static int imx6_pcie_wait_for_link(struct pcie_port *pp) > > { > > - unsigned int retries; > > - > > - for (retries = 0; retries < 200; retries++) { > > - if (dw_pcie_link_up(pp)) > > - return 0; > > - usleep_range(100, 1000); > > - } > > + /* check if the link is up or not */ > > + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) > > + return 0; > > > > - dev_err(pp->dev, "phy link never came up\n"); > > dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", > > readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), > > readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); > > diff --git a/drivers/pci/host/pcie-designware.c > > b/drivers/pci/host/pcie-designware.c > > index 540f077..6725231 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -22,6 +22,7 @@ > > #include <linux/pci_regs.h> > > #include <linux/platform_device.h> > > #include <linux/types.h> > > +#include <linux/delay.h> > > > > #include "pcie-designware.h" > > > > @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = { > > .teardown_irq = dw_msi_teardown_irq, > > }; > > > > +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > > sleep_min, > > + int sleep_max) > > +{ > > + int retries; > > + > > + /* check if the link is up or not */ > > + for (retries = 0; retries < max_ret; retries++) { > > + if (dw_pcie_link_up(pp)) { > > + dev_info(pp->dev, "link up\n"); > > + return 0; > > + } > > + usleep_range(sleep_min, sleep_max); > > + } > > + > > + dev_err(pp->dev, "phy link never came up\n"); > > + > > + return 1; > > +} > > + > > int dw_pcie_link_up(struct pcie_port *pp) > > { > > if (pp->ops->link_up) > > diff --git a/drivers/pci/host/pcie-designware.h > > b/drivers/pci/host/pcie-designware.h > > index 2356d29..b077fe0 100644 > > --- a/drivers/pci/host/pcie-designware.h > > +++ b/drivers/pci/host/pcie-designware.h > > @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, > > u32 *val); > > int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); > > irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); > > void dw_pcie_msi_init(struct pcie_port *pp); > > +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int > > sleep_min, > > + int sleep_max); > > int dw_pcie_link_up(struct pcie_port *pp); > > void dw_pcie_setup_rc(struct pcie_port *pp); > > int dw_pcie_host_init(struct pcie_port *pp); > > diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie- > > spear13xx.c > > index b95b756..5c24595 100644 > > --- a/drivers/pci/host/pcie-spear13xx.c > > +++ b/drivers/pci/host/pcie-spear13xx.c > > @@ -13,7 +13,6 @@ > > */ > > > > #include <linux/clk.h> > > -#include <linux/delay.h> > > #include <linux/interrupt.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct > > pcie_port *pp) > > struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > > struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > > u32 exp_cap_off = EXP_CAP_ID_OFFSET; > > - unsigned int retries; > > > > if (dw_pcie_link_up(pp)) { > > dev_err(pp->dev, "link already up\n"); > > @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct > > pcie_port *pp) > > &app_reg->app_ctrl_0); > > > > /* check if the link is up or not */ > > - for (retries = 0; retries < 10; retries++) { > > - if (dw_pcie_link_up(pp)) { > > - dev_info(pp->dev, "link up\n"); > > - return 0; > > - } > > - mdelay(100); > > - } > > + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) > > + return 0; > > > > - dev_err(pp->dev, "link Fail\n"); > > return -EINVAL; > > } > > > > -- > > 1.8.1.5 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c index 8c36880..e425944 100644 --- a/drivers/pci/host/pci-dra7xx.c +++ b/drivers/pci/host/pci-dra7xx.c @@ -10,7 +10,6 @@ * published by the Free Software Foundation. */ -#include <linux/delay.h> #include <linux/err.h> #include <linux/interrupt.h> #include <linux/irq.h> @@ -108,7 +107,6 @@ static int dra7xx_pcie_establish_link(struct pcie_port *pp) { struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp); u32 reg; - unsigned int retries; if (dw_pcie_link_up(pp)) { dev_err(pp->dev, "link is already up\n"); @@ -119,13 +117,10 @@ static int dra7xx_pcie_establish_link(struct pcie_port *pp) reg |= LTSSM_EN; dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); - for (retries = 0; retries < 1000; retries++) { - if (dw_pcie_link_up(pp)) - return 0; - usleep_range(10, 20); - } + /* check if the link is up or not */ + if (!dw_pcie_check_link_is_up(pp, 1000, 10, 20)) + return 0; - dev_err(pp->dev, "link is not up\n"); return -EINVAL; } diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c index 01095e1..770c45a 100644 --- a/drivers/pci/host/pci-exynos.c +++ b/drivers/pci/host/pci-exynos.c @@ -318,7 +318,6 @@ static int exynos_pcie_establish_link(struct pcie_port *pp) { struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp); u32 val; - unsigned int retries; if (dw_pcie_link_up(pp)) { dev_err(pp->dev, "Link already up\n"); @@ -357,13 +356,8 @@ static int exynos_pcie_establish_link(struct pcie_port *pp) PCIE_APP_LTSSM_ENABLE); /* check if the link is up or not */ - for (retries = 0; retries < 10; retries++) { - if (dw_pcie_link_up(pp)) { - dev_info(pp->dev, "Link up\n"); - return 0; - } - mdelay(100); - } + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) + return 0; while (exynos_phy_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED) == 0) { val = exynos_blk_readl(exynos_pcie, PCIE_PHY_PLL_LOCKED); @@ -372,7 +366,6 @@ static int exynos_pcie_establish_link(struct pcie_port *pp) /* power off phy */ exynos_pcie_power_off_phy(pp); - dev_err(pp->dev, "PCIe Link Fail\n"); return -EINVAL; } diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index 22e8224..9e323be 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -330,15 +330,10 @@ static void imx6_pcie_init_phy(struct pcie_port *pp) static int imx6_pcie_wait_for_link(struct pcie_port *pp) { - unsigned int retries; - - for (retries = 0; retries < 200; retries++) { - if (dw_pcie_link_up(pp)) - return 0; - usleep_range(100, 1000); - } + /* check if the link is up or not */ + if (!dw_pcie_check_link_is_up(pp, 200, 100, 1000)) + return 0; - dev_err(pp->dev, "phy link never came up\n"); dev_dbg(pp->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n", readl(pp->dbi_base + PCIE_PHY_DEBUG_R0), readl(pp->dbi_base + PCIE_PHY_DEBUG_R1)); diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 540f077..6725231 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -22,6 +22,7 @@ #include <linux/pci_regs.h> #include <linux/platform_device.h> #include <linux/types.h> +#include <linux/delay.h> #include "pcie-designware.h" @@ -380,6 +381,25 @@ static struct msi_controller dw_pcie_msi_chip = { .teardown_irq = dw_msi_teardown_irq, }; +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int sleep_min, + int sleep_max) +{ + int retries; + + /* check if the link is up or not */ + for (retries = 0; retries < max_ret; retries++) { + if (dw_pcie_link_up(pp)) { + dev_info(pp->dev, "link up\n"); + return 0; + } + usleep_range(sleep_min, sleep_max); + } + + dev_err(pp->dev, "phy link never came up\n"); + + return 1; +} + int dw_pcie_link_up(struct pcie_port *pp) { if (pp->ops->link_up) diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index 2356d29..b077fe0 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -76,6 +76,8 @@ int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val); int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val); irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); void dw_pcie_msi_init(struct pcie_port *pp); +int dw_pcie_check_link_is_up(struct pcie_port *pp, int max_ret, int sleep_min, + int sleep_max); int dw_pcie_link_up(struct pcie_port *pp); void dw_pcie_setup_rc(struct pcie_port *pp); int dw_pcie_host_init(struct pcie_port *pp); diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c index b95b756..5c24595 100644 --- a/drivers/pci/host/pcie-spear13xx.c +++ b/drivers/pci/host/pcie-spear13xx.c @@ -13,7 +13,6 @@ */ #include <linux/clk.h> -#include <linux/delay.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/module.h> @@ -149,7 +148,6 @@ static int spear13xx_pcie_establish_link(struct pcie_port *pp) struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; u32 exp_cap_off = EXP_CAP_ID_OFFSET; - unsigned int retries; if (dw_pcie_link_up(pp)) { dev_err(pp->dev, "link already up\n"); @@ -201,15 +199,9 @@ static int spear13xx_pcie_establish_link(struct pcie_port *pp) &app_reg->app_ctrl_0); /* check if the link is up or not */ - for (retries = 0; retries < 10; retries++) { - if (dw_pcie_link_up(pp)) { - dev_info(pp->dev, "link up\n"); - return 0; - } - mdelay(100); - } + if (!dw_pcie_check_link_is_up(pp, 10, 100000, 110000)) + return 0; - dev_err(pp->dev, "link Fail\n"); return -EINVAL; }
This patch goal is to centralize in pcie-designware the link up validation. A new function was added to pci-designware that is responsible for doing such a task. This was implemented in a form that permits flexibility for all SoCs. Signed-off-by: Joao Pinto <jpinto@synopsys.com> --- drivers/pci/host/pci-dra7xx.c | 11 +++-------- drivers/pci/host/pci-exynos.c | 11 ++--------- drivers/pci/host/pci-imx6.c | 11 +++-------- drivers/pci/host/pcie-designware.c | 20 ++++++++++++++++++++ drivers/pci/host/pcie-designware.h | 2 ++ drivers/pci/host/pcie-spear13xx.c | 12 ++---------- 6 files changed, 32 insertions(+), 35 deletions(-)