RE: [PATCH] link up validation moved to pcie-designware

From: Gabriele Paoloni
Date: Mon Feb 08 2016 - 10:29:26 EST

Next message: Marc Zyngier: "Re: [PATCH 2/3] arm64: dts: add the Alpine v2 EVP"
Previous message: codekipper: "[PATCH v5 2/2] ASoC: sunxi: Add support for the SPDIF block"
In reply to: Joao Pinto: "Re: [PATCH] link up validation moved to pcie-designware"
Next in thread: Joao Pinto: "Re: [PATCH] link up validation moved to pcie-designware"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Marc Zyngier: "Re: [PATCH 2/3] arm64: dts: add the Alpine v2 EVP"
Previous message: codekipper: "[PATCH v5 2/2] ASoC: sunxi: Add support for the SPDIF block"
In reply to: Joao Pinto: "Re: [PATCH] link up validation moved to pcie-designware"
Next in thread: Joao Pinto: "Re: [PATCH] link up validation moved to pcie-designware"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]