| Message ID | 1433211273-226508-1-git-send-email-wangzhou1@hisilicon.com |
|---|---|
| State | Changes Requested |
| Headers | show |
[+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 -- 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 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 > > . > -- 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 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 > > > > . > > > > -- 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/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);
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(-)