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@xxxxxxxxxxxx] >> Sent: 08 February 2016 15:12 >> To: Gabriele Paoloni; Joao Pinto; helgaas@xxxxxxxxxx >> Cc: arnd@xxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux- >> kernel@xxxxxxxxxxxxxxx; CARLOS.PALMINHA@xxxxxxxxxxxx >> 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@xxxxxxxxxxxxxxx [mailto:linux-pci- >>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Joao Pinto >>>> Sent: 08 February 2016 12:44 >>>> To: helgaas@xxxxxxxxxx >>>> Cc: arnd@xxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux- >>>> kernel@xxxxxxxxxxxxxxx; CARLOS.PALMINHA@xxxxxxxxxxxx; 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@xxxxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxxx >>>> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
