* [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup @ 2015-06-02 2:14 Zhou Wang 2015-06-02 19:37 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Zhou Wang @ 2015-06-02 2:14 UTC (permalink / raw) To: Jingoo Han, Pratyush Anand, Bjorn Helgaas Cc: linux-pci, gabriele.paoloni, yuanzhichang, zhangjukuo, liguozhu, Zhou Wang When connected some PCIe3.0 cards(e.g. LSI 2208 PCIe-RAID card, Mellanox IB card), It will appear link unstable which will lead reading/writing fail. Here just move the setting of PORT_LOGIC_SPEED_CHANGE bit before starting building link. After doing this, it will work fine with above PCIe3.0 card. This patch is based on v4.1-rc4 Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> --- drivers/pci/host/pcie-designware.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 2e9f84f..64ebc51 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -498,10 +498,6 @@ int dw_pcie_host_init(struct pcie_port *pp) /* program correct class for RC */ dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); - dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val); - val |= PORT_LOGIC_SPEED_CHANGE; - dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val); - #ifdef CONFIG_PCI_MSI dw_pcie_msi_chip.dev = pp->dev; dw_pci.msi_ctrl = &dw_pcie_msi_chip; @@ -797,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) } dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); + dw_pcie_readl_rc(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, &val); + val |= PORT_LOGIC_SPEED_CHANGE; + dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); + /* setup RC BARs */ dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0); dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1); -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup 2015-06-02 2:14 [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup Zhou Wang @ 2015-06-02 19:37 ` Bjorn Helgaas 2015-06-05 11:05 ` Zhou Wang 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2015-06-02 19:37 UTC (permalink / raw) To: Zhou Wang Cc: Jingoo Han, Pratyush Anand, linux-pci, gabriele.paoloni, yuanzhichang, zhangjukuo, liguozhu, Richard Zhu, Lucas Stach [+cc Richard, Lucas for imx6 questions below] On Tue, Jun 02, 2015 at 10:14:33AM +0800, Zhou Wang wrote: > When connected some PCIe3.0 cards(e.g. LSI 2208 PCIe-RAID card, Mellanox IB card), > It will appear link unstable which will lead reading/writing fail. > > Here just move the setting of PORT_LOGIC_SPEED_CHANGE bit before starting > building link. After doing this, it will work fine with above PCIe3.0 card. > > This patch is based on v4.1-rc4 > > Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> > --- > drivers/pci/host/pcie-designware.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 2e9f84f..64ebc51 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -498,10 +498,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > /* program correct class for RC */ > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > > - dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val); > - val |= PORT_LOGIC_SPEED_CHANGE; > - dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val); > - > #ifdef CONFIG_PCI_MSI > dw_pcie_msi_chip.dev = pp->dev; > dw_pci.msi_ctrl = &dw_pcie_msi_chip; > @@ -797,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > } > dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > > + dw_pcie_readl_rc(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, &val); > + val |= PORT_LOGIC_SPEED_CHANGE; > + dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); It makes sense to me to do this at the same place we do the other write to PCIE_LINK_WIDTH_SPEED_CONTROL. 1) Can this be combined with the previous write, so we only need a single write to PCIE_LINK_WIDTH_SPEED_CONTROL, e.g., something like this? switch (pp->lanes) { case 1: val |= PORT_LOGIC_LINK_WIDTH_1_LANES; ... } val |= PORT_LOGIC_SPEED_CHANGE; dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); 2) imx6_pcie_start_link() also writes PCIE_LINK_WIDTH_SPEED_CONTROL. It has a loop to wait for the speed change to finish. Does dw_pcie_setup_rc() need a similar loop? 3) Since you are moving the PORT_LOGIC_SPEED_CHANGE write from dw_pcie_host_init() to dw_pcie_setup_rc(), it will now happen before imx6_pcie_start_link(). Does that mean we can remove the write from imx6_pcie_start_link() completely? > /* setup RC BARs */ > dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0); > dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1); > -- > 1.9.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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup 2015-06-02 19:37 ` Bjorn Helgaas @ 2015-06-05 11:05 ` Zhou Wang 2015-06-18 16:42 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Zhou Wang @ 2015-06-05 11:05 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jingoo Han, Pratyush Anand, linux-pci, gabriele.paoloni, yuanzhichang, zhangjukuo, liguozhu, Richard Zhu, Lucas Stach On 2015/6/3 3:37, Bjorn Helgaas wrote: > [+cc Richard, Lucas for imx6 questions below] > > On Tue, Jun 02, 2015 at 10:14:33AM +0800, Zhou Wang wrote: >> When connected some PCIe3.0 cards(e.g. LSI 2208 PCIe-RAID card, Mellanox IB card), >> It will appear link unstable which will lead reading/writing fail. >> >> Here just move the setting of PORT_LOGIC_SPEED_CHANGE bit before starting >> building link. After doing this, it will work fine with above PCIe3.0 card. >> >> This patch is based on v4.1-rc4 >> >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> >> --- >> drivers/pci/host/pcie-designware.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >> index 2e9f84f..64ebc51 100644 >> --- a/drivers/pci/host/pcie-designware.c >> +++ b/drivers/pci/host/pcie-designware.c >> @@ -498,10 +498,6 @@ int dw_pcie_host_init(struct pcie_port *pp) >> /* program correct class for RC */ >> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); >> >> - dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val); >> - val |= PORT_LOGIC_SPEED_CHANGE; >> - dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val); >> - >> #ifdef CONFIG_PCI_MSI >> dw_pcie_msi_chip.dev = pp->dev; >> dw_pci.msi_ctrl = &dw_pcie_msi_chip; >> @@ -797,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >> } >> dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); >> >> + dw_pcie_readl_rc(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, &val); >> + val |= PORT_LOGIC_SPEED_CHANGE; >> + dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > > It makes sense to me to do this at the same place we do the other write to > PCIE_LINK_WIDTH_SPEED_CONTROL. > > 1) Can this be combined with the previous write, so we only need a single > write to PCIE_LINK_WIDTH_SPEED_CONTROL, e.g., something like this? > > switch (pp->lanes) { > case 1: > val |= PORT_LOGIC_LINK_WIDTH_1_LANES; > ... > } > val |= PORT_LOGIC_SPEED_CHANGE; > dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > I think it is OK to put them together. > 2) imx6_pcie_start_link() also writes PCIE_LINK_WIDTH_SPEED_CONTROL. It > has a loop to wait for the speed change to finish. Does > dw_pcie_setup_rc() need a similar loop? imx6 has a loop there to make sure link is stable, because enumeration will start soon. Also in other drivers which are based on pcie-designware, we should check if speed change is done before enumeration. In pcie-designware, now it does not make this check. Maybe we may not move PORT_LOGIC_SPEED_CHANGE set, but add a loop check after it to make sure the link is stable. Then related same setting in imx6 can be deleted. I wonder if other PCIe hosts which is based on DW have met the same problem? > > 3) Since you are moving the PORT_LOGIC_SPEED_CHANGE write from > dw_pcie_host_init() to dw_pcie_setup_rc(), it will now happen before > imx6_pcie_start_link(). Does that mean we can remove the write from > imx6_pcie_start_link() completely? > I think so. Best Regards, Zhou >> /* setup RC BARs */ >> dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0); >> dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1); >> -- >> 1.9.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 > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup 2015-06-05 11:05 ` Zhou Wang @ 2015-06-18 16:42 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2015-06-18 16:42 UTC (permalink / raw) To: Zhou Wang Cc: Jingoo Han, Pratyush Anand, linux-pci, gabriele.paoloni, yuanzhichang, zhangjukuo, liguozhu, Richard Zhu, Lucas Stach On Fri, Jun 05, 2015 at 07:05:02PM +0800, Zhou Wang wrote: > On 2015/6/3 3:37, Bjorn Helgaas wrote: > > [+cc Richard, Lucas for imx6 questions below] > > > > On Tue, Jun 02, 2015 at 10:14:33AM +0800, Zhou Wang wrote: > >> When connected some PCIe3.0 cards(e.g. LSI 2208 PCIe-RAID card, Mellanox IB card), > >> It will appear link unstable which will lead reading/writing fail. > >> > >> Here just move the setting of PORT_LOGIC_SPEED_CHANGE bit before starting > >> building link. After doing this, it will work fine with above PCIe3.0 card. > >> > >> This patch is based on v4.1-rc4 > >> > >> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com> > >> --- > >> drivers/pci/host/pcie-designware.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > >> index 2e9f84f..64ebc51 100644 > >> --- a/drivers/pci/host/pcie-designware.c > >> +++ b/drivers/pci/host/pcie-designware.c > >> @@ -498,10 +498,6 @@ int dw_pcie_host_init(struct pcie_port *pp) > >> /* program correct class for RC */ > >> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > >> > >> - dw_pcie_rd_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, &val); > >> - val |= PORT_LOGIC_SPEED_CHANGE; > >> - dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val); > >> - > >> #ifdef CONFIG_PCI_MSI > >> dw_pcie_msi_chip.dev = pp->dev; > >> dw_pci.msi_ctrl = &dw_pcie_msi_chip; > >> @@ -797,6 +793,10 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > >> } > >> dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > >> > >> + dw_pcie_readl_rc(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, &val); > >> + val |= PORT_LOGIC_SPEED_CHANGE; > >> + dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > > > > It makes sense to me to do this at the same place we do the other write to > > PCIE_LINK_WIDTH_SPEED_CONTROL. > > > > 1) Can this be combined with the previous write, so we only need a single > > write to PCIE_LINK_WIDTH_SPEED_CONTROL, e.g., something like this? > > > > switch (pp->lanes) { > > case 1: > > val |= PORT_LOGIC_LINK_WIDTH_1_LANES; > > ... > > } > > val |= PORT_LOGIC_SPEED_CHANGE; > > dw_pcie_writel_rc(pp, val, PCIE_LINK_WIDTH_SPEED_CONTROL); > > > > I think it is OK to put them together. > > > 2) imx6_pcie_start_link() also writes PCIE_LINK_WIDTH_SPEED_CONTROL. It > > has a loop to wait for the speed change to finish. Does > > dw_pcie_setup_rc() need a similar loop? > > imx6 has a loop there to make sure link is stable, because enumeration will start > soon. Also in other drivers which are based on pcie-designware, we should check > if speed change is done before enumeration. In pcie-designware, now it does not > make this check. > > Maybe we may not move PORT_LOGIC_SPEED_CHANGE set, but add a loop check after it > to make sure the link is stable. Then related same setting in imx6 can be deleted. > > I wonder if other PCIe hosts which is based on DW have met the same problem? > > > > > 3) Since you are moving the PORT_LOGIC_SPEED_CHANGE write from > > dw_pcie_host_init() to dw_pcie_setup_rc(), it will now happen before > > imx6_pcie_start_link(). Does that mean we can remove the write from > > imx6_pcie_start_link() completely? > > > > I think so. Ping? I'm dropping this for now since nobody acked it and we never quite resolved the questions. But it seems like something is broken and needs to be fixed, so please post an updated version. > >> /* setup RC BARs */ > >> dw_pcie_writel_rc(pp, 0x00000004, PCI_BASE_ADDRESS_0); > >> dw_pcie_writel_rc(pp, 0x00000000, PCI_BASE_ADDRESS_1); > >> -- > >> 1.9.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 > > > > . > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-06-18 16:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-02 2:14 [PATCH] PCI: designware: set PORT_LOGIC_SPEED_CHANGE before linkup Zhou Wang 2015-06-02 19:37 ` Bjorn Helgaas 2015-06-05 11:05 ` Zhou Wang 2015-06-18 16:42 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).