| Message ID | 1447942365-11662-1-git-send-email-tharvey@gateworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Am Donnerstag, den 19.11.2015, 06:12 -0800 schrieb Tim Harvey: > Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > external clock source is present and supplied back to the IMX6 PCIe core > via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > > Add a dt property to specify gen1 vs gen2 and check this before allowing > a Gen2 link. > > We default to Gen1 if the property is not present because at this time there > are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > > In order to be Gen2 compliant on IMX6 you need to: > - have a Gen2 compliant external clock generator and route that clock back > to either LVDS CLK1 or LVDS CLK2 as an input. > (see IMX6SX-SabreSD reference design) > - specify this clock in the pcie node in the dt > (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > > [1] https://community.freescale.com/message/453209 > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> One nit below, but looks good to me: Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > v2: > - moved dt property to designware core > > .../devicetree/bindings/pci/designware-pcie.txt | 1 + > drivers/pci/host/pci-imx6.c | 16 ++++++++++------ > drivers/pci/host/pcie-designware.c | 4 ++++ > drivers/pci/host/pcie-designware.h | 1 + > 4 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index 9f4faa8..a9a94b9 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -26,3 +26,4 @@ Optional properties: > - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to > specify this property, to keep backwards compatibility a range of 0x00-0xff > is assumed if not present) > +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 8f3a981..6a9f310 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > if (ret) > return ret; > > - /* Allow Gen2 mode after the link is up. */ > - tmp = readl(pp->dbi_base + PCIE_RC_LCR); > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > - writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + if (pp->link_gen == 2) { > + /* Allow Gen2 mode after the link is up. */ > + tmp = readl(pp->dbi_base + PCIE_RC_LCR); > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > + writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + } else { > + dev_info(pp->dev, "Link: Gen2 disabled\n"); > + } > > /* > * Start Directed Speed Change so the best possible speed both link > @@ -420,7 +424,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > } > > tmp = readl(pp->dbi_base + PCIE_RC_LCSR); > - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); > + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); > return 0; > } > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 52aa6e3..7aa81ff 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > return -EINVAL; > } > > + pp->link_gen = -1; > + if (of_property_read_u32(np, "max-link-speed", &ret) == 0) > + pp->link_gen = ret; of_property_read_u32() doesn't change the return parameter if the property isn't found. So using &ret as temporary storage isn't strictly needed. > + > if (IS_ENABLED(CONFIG_PCI_MSI)) { > if (!pp->ops->msi_host_init) { > pp->irq_domain = irq_domain_add_linear(pp->dev->of_node, > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index d0bbd27..5ce821d 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -48,6 +48,7 @@ struct pcie_port { > struct resource busn; > int irq; > u32 lanes; > + int link_gen; > struct pcie_host_ops *ops; > int msi_irq; > struct irq_domain *irq_domain;
Hi Tim, On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: > Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > external clock source is present and supplied back to the IMX6 PCIe core > via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > > Add a dt property to specify gen1 vs gen2 and check this before allowing > a Gen2 link. > > We default to Gen1 if the property is not present because at this time there > are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > > In order to be Gen2 compliant on IMX6 you need to: > - have a Gen2 compliant external clock generator and route that clock back > to either LVDS CLK1 or LVDS CLK2 as an input. > (see IMX6SX-SabreSD reference design) > - specify this clock in the pcie node in the dt > (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > > [1] https://community.freescale.com/message/453209 > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v2: > - moved dt property to designware core > > .../devicetree/bindings/pci/designware-pcie.txt | 1 + > drivers/pci/host/pci-imx6.c | 16 ++++++++++------ > drivers/pci/host/pcie-designware.c | 4 ++++ > drivers/pci/host/pcie-designware.h | 1 + > 4 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > index 9f4faa8..a9a94b9 100644 > --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > @@ -26,3 +26,4 @@ Optional properties: > - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to > specify this property, to keep backwards compatibility a range of 0x00-0xff > is assumed if not present) > +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) Is there some sort of DT or OF spec that lists "max-link-speed" as a generic property? I see Lucas' desire to have this be common across DesignWare PCIe cores. Should it be moved up a level even from there, i.e., to bindings/pci/pci.txt? It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit the link to gen1 unless max-link-speed is present and has the value "2". > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 8f3a981..6a9f310 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > if (ret) > return ret; > > - /* Allow Gen2 mode after the link is up. */ > - tmp = readl(pp->dbi_base + PCIE_RC_LCR); > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > - writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + if (pp->link_gen == 2) { > + /* Allow Gen2 mode after the link is up. */ > + tmp = readl(pp->dbi_base + PCIE_RC_LCR); > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > + writel(tmp, pp->dbi_base + PCIE_RC_LCR); > + } else { > + dev_info(pp->dev, "Link: Gen2 disabled\n"); > + } > > /* > * Start Directed Speed Change so the best possible speed both link > @@ -420,7 +424,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > } > > tmp = readl(pp->dbi_base + PCIE_RC_LCSR); > - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); > + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); > return 0; > } > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 52aa6e3..7aa81ff 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp) > return -EINVAL; > } > > + pp->link_gen = -1; > + if (of_property_read_u32(np, "max-link-speed", &ret) == 0) > + pp->link_gen = ret; > + > if (IS_ENABLED(CONFIG_PCI_MSI)) { > if (!pp->ops->msi_host_init) { > pp->irq_domain = irq_domain_add_linear(pp->dev->of_node, > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index d0bbd27..5ce821d 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -48,6 +48,7 @@ struct pcie_port { > struct resource busn; > int irq; > u32 lanes; > + int link_gen; > struct pcie_host_ops *ops; > int msi_irq; > struct irq_domain *irq_domain; > -- > 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 Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > Hi Tim, > > On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant >> external clock source is present and supplied back to the IMX6 PCIe core >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. >> >> Add a dt property to specify gen1 vs gen2 and check this before allowing >> a Gen2 link. >> >> We default to Gen1 if the property is not present because at this time there >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. >> >> In order to be Gen2 compliant on IMX6 you need to: >> - have a Gen2 compliant external clock generator and route that clock back >> to either LVDS CLK1 or LVDS CLK2 as an input. >> (see IMX6SX-SabreSD reference design) >> - specify this clock in the pcie node in the dt >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) >> >> [1] https://community.freescale.com/message/453209 >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> --- >> v2: >> - moved dt property to designware core >> >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ >> drivers/pci/host/pcie-designware.c | 4 ++++ >> drivers/pci/host/pcie-designware.h | 1 + >> 4 files changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> index 9f4faa8..a9a94b9 100644 >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> @@ -26,3 +26,4 @@ Optional properties: >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to >> specify this property, to keep backwards compatibility a range of 0x00-0xff >> is assumed if not present) >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) > > Is there some sort of DT or OF spec that lists "max-link-speed" as a > generic property? I see Lucas' desire to have this be common across > DesignWare PCIe cores. Should it be moved up a level even from there, > i.e., to bindings/pci/pci.txt? Bjorn, I don't know what the general consensus is here. As your the PCI maintainer I would leave that up to you. Are there other platforms that need to link at a lesser capability than the host controller is capable of? I am only aware of the IMX6 and SPEAr13XX [1] I can make that change and re-submit if you feel that is necessary. > > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit > the link to gen1 unless max-link-speed is present and has the value > "2". I can add that in a v3 patch. Tim [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt#n14 -- 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 Wed, Dec 2, 2015 at 8:35 AM, Tim Harvey <tharvey@gateworks.com> wrote: > On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> Hi Tim, >> >> On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: >>> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass >>> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant >>> external clock source is present and supplied back to the IMX6 PCIe core >>> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. >>> >>> Add a dt property to specify gen1 vs gen2 and check this before allowing >>> a Gen2 link. >>> >>> We default to Gen1 if the property is not present because at this time there >>> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. >>> >>> In order to be Gen2 compliant on IMX6 you need to: >>> - have a Gen2 compliant external clock generator and route that clock back >>> to either LVDS CLK1 or LVDS CLK2 as an input. >>> (see IMX6SX-SabreSD reference design) >>> - specify this clock in the pcie node in the dt >>> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of >>> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) >>> >>> [1] https://community.freescale.com/message/453209 >>> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >>> --- >>> v2: >>> - moved dt property to designware core >>> >>> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ >>> drivers/pci/host/pcie-designware.c | 4 ++++ >>> drivers/pci/host/pcie-designware.h | 1 + >>> 4 files changed, 16 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> index 9f4faa8..a9a94b9 100644 >>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> @@ -26,3 +26,4 @@ Optional properties: >>> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to >>> specify this property, to keep backwards compatibility a range of 0x00-0xff >>> is assumed if not present) >>> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) >> >> Is there some sort of DT or OF spec that lists "max-link-speed" as a >> generic property? I see Lucas' desire to have this be common across >> DesignWare PCIe cores. Should it be moved up a level even from there, >> i.e., to bindings/pci/pci.txt? > > Bjorn, > > I don't know what the general consensus is here. As your the PCI > maintainer I would leave that up to you. Are there other platforms > that need to link at a lesser capability than the host controller is > capable of? I am only aware of the IMX6 and SPEAr13XX [1] > > I can make that change and re-submit if you feel that is necessary. > Bjorn, Any thoughts here? I would really like to see this get picked up sooner rather than later. Regards, Tim -- 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
Tim Harvey <tharvey <at> gateworks.com> writes: > >> > >> Is there some sort of DT or OF spec that lists "max-link-speed" as a > >> generic property? I see Lucas' desire to have this be common across > >> DesignWare PCIe cores. Should it be moved up a level even from there, > >> i.e., to bindings/pci/pci.txt? > > > > Bjorn, > > > > I don't know what the general consensus is here. As your the PCI > > maintainer I would leave that up to you. Are there other platforms > > that need to link at a lesser capability than the host controller is > > capable of? I am only aware of the IMX6 and SPEAr13XX [1] > > > > I can make that change and re-submit if you feel that is necessary. > > > > Bjorn, > > Any thoughts here? I would really like to see this get picked up > sooner rather than later. > > Regards, > > Tim > Bjorn, It looks like there was no further follow-up on this patch. Could you kindly provide feedback to Tim's question? We have multiple i.MX boards that could use this patch to resolve PCIe issues. Thanks, Akshay -- 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
[+cc Tim] On Wed, Feb 24, 2016 at 07:35:55PM +0000, Akshay Bhat wrote: > Tim Harvey <tharvey <at> gateworks.com> writes: > > > >> > > >> Is there some sort of DT or OF spec that lists "max-link-speed" as a > > >> generic property? I see Lucas' desire to have this be common across > > >> DesignWare PCIe cores. Should it be moved up a level even from there, > > >> i.e., to bindings/pci/pci.txt? > > > > > > Bjorn, > > > > > > I don't know what the general consensus is here. As your the PCI > > > maintainer I would leave that up to you. Are there other platforms > > > that need to link at a lesser capability than the host controller is > > > capable of? I am only aware of the IMX6 and SPEAr13XX [1] > > > > > > I can make that change and re-submit if you feel that is necessary. > > > > > > > Bjorn, > > > > Any thoughts here? I would really like to see this get picked up > > sooner rather than later. > > > > Regards, > > > > Tim > > > > Bjorn, > > It looks like there was no further follow-up on this patch. Could you > kindly provide feedback to Tim's question? We have multiple i.MX boards > that could use this patch to resolve PCIe issues. Tim had mentioned doing a v3 with a documentation update, so I dropped the v2 patch in anticipation of v3, which never appeared. Looking at it again, I have another question, so I'll respond to that thread again. Bjorn -- 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
[+cc Rob, devicetree list] On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote: > On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: > >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > >> external clock source is present and supplied back to the IMX6 PCIe core > >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > >> > >> Add a dt property to specify gen1 vs gen2 and check this before allowing > >> a Gen2 link. > >> > >> We default to Gen1 if the property is not present because at this time there > >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > >> > >> In order to be Gen2 compliant on IMX6 you need to: > >> - have a Gen2 compliant external clock generator and route that clock back > >> to either LVDS CLK1 or LVDS CLK2 as an input. > >> (see IMX6SX-SabreSD reference design) > >> - specify this clock in the pcie node in the dt > >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > >> > >> [1] https://community.freescale.com/message/453209 > >> > >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >> --- > >> v2: > >> - moved dt property to designware core > >> > >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + > >> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ > >> drivers/pci/host/pcie-designware.c | 4 ++++ > >> drivers/pci/host/pcie-designware.h | 1 + > >> 4 files changed, 16 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >> index 9f4faa8..a9a94b9 100644 > >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >> @@ -26,3 +26,4 @@ Optional properties: > >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to > >> specify this property, to keep backwards compatibility a range of 0x00-0xff > >> is assumed if not present) > >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) > > > > Is there some sort of DT or OF spec that lists "max-link-speed" as a > > generic property? I see Lucas' desire to have this be common across > > DesignWare PCIe cores. Should it be moved up a level even from there, > > i.e., to bindings/pci/pci.txt? > > I don't know what the general consensus is here. As your the PCI > maintainer I would leave that up to you. Are there other platforms > that need to link at a lesser capability than the host controller is > capable of? I am only aware of the IMX6 and SPEAr13XX [1] This is really a devicetree question, not a PCI one, so I added Rob and the devicetree list in case they have any comments on this. > > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit > > the link to gen1 unless max-link-speed is present and has the value > > "2". This default seems backwards. It seems like we'd want to configure the link to go as fast as possible unless we have a quirk, e.g., "max-link-speed", that imposes a device-specific link. In other words, why don't we penalize the broken board instead of penalizing all the working ones? > [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt#n14 -- 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 Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Rob, devicetree list] > > On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote: >> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: >> >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass >> >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant >> >> external clock source is present and supplied back to the IMX6 PCIe core >> >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. >> >> >> >> Add a dt property to specify gen1 vs gen2 and check this before allowing >> >> a Gen2 link. >> >> >> >> We default to Gen1 if the property is not present because at this time there >> >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. >> >> >> >> In order to be Gen2 compliant on IMX6 you need to: >> >> - have a Gen2 compliant external clock generator and route that clock back >> >> to either LVDS CLK1 or LVDS CLK2 as an input. >> >> (see IMX6SX-SabreSD reference design) >> >> - specify this clock in the pcie node in the dt >> >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of >> >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) >> >> >> >> [1] https://community.freescale.com/message/453209 >> >> >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> >> --- >> >> v2: >> >> - moved dt property to designware core >> >> >> >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >> >> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ >> >> drivers/pci/host/pcie-designware.c | 4 ++++ >> >> drivers/pci/host/pcie-designware.h | 1 + >> >> 4 files changed, 16 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> >> index 9f4faa8..a9a94b9 100644 >> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> >> @@ -26,3 +26,4 @@ Optional properties: >> >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to >> >> specify this property, to keep backwards compatibility a range of 0x00-0xff >> >> is assumed if not present) >> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) >> > >> > Is there some sort of DT or OF spec that lists "max-link-speed" as a >> > generic property? I see Lucas' desire to have this be common across >> > DesignWare PCIe cores. Should it be moved up a level even from there, >> > i.e., to bindings/pci/pci.txt? >> >> I don't know what the general consensus is here. As your the PCI >> maintainer I would leave that up to you. Are there other platforms >> that need to link at a lesser capability than the host controller is >> capable of? I am only aware of the IMX6 and SPEAr13XX [1] > > This is really a devicetree question, not a PCI one, so I added Rob > and the devicetree list in case they have any comments on this. Seems generally useful to me. You could want to limit the speed for a variety of reasons. There's no standard property that I'm aware of. Shouldn't this be a property of the phy though? >> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit >> > the link to gen1 unless max-link-speed is present and has the value >> > "2". > > This default seems backwards. It seems like we'd want to configure > the link to go as fast as possible unless we have a quirk, e.g., > "max-link-speed", that imposes a device-specific link. In other > words, why don't we penalize the broken board instead of penalizing > all the working ones? I agree. It could be argued that this way doesn't require a DT update to fix broken boards. However, this problem should really be found before production and DT updates are normal for enabling new features (such as compliant PCIe). Rob -- 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 Wed, Feb 24, 2016 at 12:35 PM, Rob Herring <robh+dt@kernel.org> wrote: > On Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> [+cc Rob, devicetree list] >> >> On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote: >>> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >>> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: >>> >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass >>> >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant >>> >> external clock source is present and supplied back to the IMX6 PCIe core >>> >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. >>> >> >>> >> Add a dt property to specify gen1 vs gen2 and check this before allowing >>> >> a Gen2 link. >>> >> >>> >> We default to Gen1 if the property is not present because at this time there >>> >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. >>> >> >>> >> In order to be Gen2 compliant on IMX6 you need to: >>> >> - have a Gen2 compliant external clock generator and route that clock back >>> >> to either LVDS CLK1 or LVDS CLK2 as an input. >>> >> (see IMX6SX-SabreSD reference design) >>> >> - specify this clock in the pcie node in the dt >>> >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of >>> >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) >>> >> >>> >> [1] https://community.freescale.com/message/453209 >>> >> >>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >>> >> --- >>> >> v2: >>> >> - moved dt property to designware core >>> >> >>> >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + >>> >> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ >>> >> drivers/pci/host/pcie-designware.c | 4 ++++ >>> >> drivers/pci/host/pcie-designware.h | 1 + >>> >> 4 files changed, 16 insertions(+), 6 deletions(-) >>> >> >>> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> >> index 9f4faa8..a9a94b9 100644 >>> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >>> >> @@ -26,3 +26,4 @@ Optional properties: >>> >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to >>> >> specify this property, to keep backwards compatibility a range of 0x00-0xff >>> >> is assumed if not present) >>> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) >>> > >>> > Is there some sort of DT or OF spec that lists "max-link-speed" as a >>> > generic property? I see Lucas' desire to have this be common across >>> > DesignWare PCIe cores. Should it be moved up a level even from there, >>> > i.e., to bindings/pci/pci.txt? >>> >>> I don't know what the general consensus is here. As your the PCI >>> maintainer I would leave that up to you. Are there other platforms >>> that need to link at a lesser capability than the host controller is >>> capable of? I am only aware of the IMX6 and SPEAr13XX [1] >> >> This is really a devicetree question, not a PCI one, so I added Rob >> and the devicetree list in case they have any comments on this. > > Seems generally useful to me. You could want to limit the speed for a > variety of reasons. There's no standard property that I'm aware of. > > Shouldn't this be a property of the phy though? > >>> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit >>> > the link to gen1 unless max-link-speed is present and has the value >>> > "2". >> >> This default seems backwards. It seems like we'd want to configure >> the link to go as fast as possible unless we have a quirk, e.g., >> "max-link-speed", that imposes a device-specific link. In other >> words, why don't we penalize the broken board instead of penalizing >> all the working ones? > > I agree. It could be argued that this way doesn't require a DT update > to fix broken boards. However, this problem should really be found > before production and DT updates are normal for enabling new features > (such as compliant PCIe). > > Rob Rob, In this case I feel that every IMX6 based board with PCIe likely has this issue because the original reference schematics from Freescale did not mention that the LVDS clock source from the IMX6 did not meet PCIe gen2 requirements so everyone blindly followed the reference design. Only later did they seem to come out with this information and if you did use an external clock you would have to setup the IMX6 pcie clocks differently in the device-tree (configuring the IMX6 to use a clock input instead of output), which I see no current boards doing. There are several SoC's that use the designware core that likely are not in this same boat, so it would be unfair to penalize them by defaulting a gen1 speed tied to the mac. So yes, perhaps a property of the imx6 pcie phy and defaulting it to gen1 because no current device-tree's set the phy's clock as an external input makes the most sense? Note that I never have seen any failure when running an IMX6 at Gen2 while using the LVDS clock generated from the IMX6 for PCIe, I'm just taking Freescale's word for it that it does not meet jitter requirements (they never provided details on the lack of compliance failure mode). I assume most board manufacturers have not been able to run compliance tests on their own designs derived from the reference design. Regards, Tim -- 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 29, 2016 at 06:41:29AM -0800, Tim Harvey wrote: > On Wed, Feb 24, 2016 at 12:35 PM, Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> [+cc Rob, devicetree list] > >> > >> On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote: > >>> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > >>> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, Tim Harvey wrote: > >>> >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > >>> >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > >>> >> external clock source is present and supplied back to the IMX6 PCIe core > >>> >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > >>> >> > >>> >> Add a dt property to specify gen1 vs gen2 and check this before allowing > >>> >> a Gen2 link. > >>> >> > >>> >> We default to Gen1 if the property is not present because at this time there > >>> >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > >>> >> > >>> >> In order to be Gen2 compliant on IMX6 you need to: > >>> >> - have a Gen2 compliant external clock generator and route that clock back > >>> >> to either LVDS CLK1 or LVDS CLK2 as an input. > >>> >> (see IMX6SX-SabreSD reference design) > >>> >> - specify this clock in the pcie node in the dt > >>> >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > >>> >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > >>> >> > >>> >> [1] https://community.freescale.com/message/453209 > >>> >> > >>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >>> >> --- > >>> >> v2: > >>> >> - moved dt property to designware core > >>> >> > >>> >> .../devicetree/bindings/pci/designware-pcie.txt | 1 + > >>> >> drivers/pci/host/pci-imx6.c | 16 ++++++++++------ > >>> >> drivers/pci/host/pcie-designware.c | 4 ++++ > >>> >> drivers/pci/host/pcie-designware.h | 1 + > >>> >> 4 files changed, 16 insertions(+), 6 deletions(-) > >>> >> > >>> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>> >> index 9f4faa8..a9a94b9 100644 > >>> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt > >>> >> @@ -26,3 +26,4 @@ Optional properties: > >>> >> - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to > >>> >> specify this property, to keep backwards compatibility a range of 0x00-0xff > >>> >> is assumed if not present) > >>> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) > >>> > > >>> > Is there some sort of DT or OF spec that lists "max-link-speed" as a > >>> > generic property? I see Lucas' desire to have this be common across > >>> > DesignWare PCIe cores. Should it be moved up a level even from there, > >>> > i.e., to bindings/pci/pci.txt? > >>> > >>> I don't know what the general consensus is here. As your the PCI > >>> maintainer I would leave that up to you. Are there other platforms > >>> that need to link at a lesser capability than the host controller is > >>> capable of? I am only aware of the IMX6 and SPEAr13XX [1] > >> > >> This is really a devicetree question, not a PCI one, so I added Rob > >> and the devicetree list in case they have any comments on this. > > > > Seems generally useful to me. You could want to limit the speed for a > > variety of reasons. There's no standard property that I'm aware of. > > > > Shouldn't this be a property of the phy though? > > > >>> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit > >>> > the link to gen1 unless max-link-speed is present and has the value > >>> > "2". > >> > >> This default seems backwards. It seems like we'd want to configure > >> the link to go as fast as possible unless we have a quirk, e.g., > >> "max-link-speed", that imposes a device-specific link. In other > >> words, why don't we penalize the broken board instead of penalizing > >> all the working ones? > > > > I agree. It could be argued that this way doesn't require a DT update > > to fix broken boards. However, this problem should really be found > > before production and DT updates are normal for enabling new features > > (such as compliant PCIe). > > > > Rob > > Rob, > > In this case I feel that every IMX6 based board with PCIe likely has > this issue because the original reference schematics from Freescale > did not mention that the LVDS clock source from the IMX6 did not meet > PCIe gen2 requirements so everyone blindly followed the reference > design. Only later did they seem to come out with this information and > if you did use an external clock you would have to setup the IMX6 pcie > clocks differently in the device-tree (configuring the IMX6 to use a > clock input instead of output), which I see no current boards doing. > > There are several SoC's that use the designware core that likely are > not in this same boat, so it would be unfair to penalize them by > defaulting a gen1 speed tied to the mac. So yes, perhaps a property of > the imx6 pcie phy and defaulting it to gen1 because no current > device-tree's set the phy's clock as an external input makes the most > sense? I'm not an IMX6 expert, but this sounds good to me, so I'll look for a new rev that does this. Bjorn -- 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/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt index 9f4faa8..a9a94b9 100644 --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt @@ -26,3 +26,4 @@ Optional properties: - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to specify this property, to keep backwards compatibility a range of 0x00-0xff is assumed if not present) +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2) diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index 8f3a981..6a9f310 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) if (ret) return ret; - /* Allow Gen2 mode after the link is up. */ - tmp = readl(pp->dbi_base + PCIE_RC_LCR); - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; - writel(tmp, pp->dbi_base + PCIE_RC_LCR); + if (pp->link_gen == 2) { + /* Allow Gen2 mode after the link is up. */ + tmp = readl(pp->dbi_base + PCIE_RC_LCR); + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; + writel(tmp, pp->dbi_base + PCIE_RC_LCR); + } else { + dev_info(pp->dev, "Link: Gen2 disabled\n"); + } /* * Start Directed Speed Change so the best possible speed both link @@ -420,7 +424,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) } tmp = readl(pp->dbi_base + PCIE_RC_LCSR); - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); return 0; } diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 52aa6e3..7aa81ff 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp) return -EINVAL; } + pp->link_gen = -1; + if (of_property_read_u32(np, "max-link-speed", &ret) == 0) + pp->link_gen = ret; + if (IS_ENABLED(CONFIG_PCI_MSI)) { if (!pp->ops->msi_host_init) { pp->irq_domain = irq_domain_add_linear(pp->dev->of_node, diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index d0bbd27..5ce821d 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -48,6 +48,7 @@ struct pcie_port { struct resource busn; int irq; u32 lanes; + int link_gen; struct pcie_host_ops *ops; int msi_irq; struct irq_domain *irq_domain;
Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant external clock source is present and supplied back to the IMX6 PCIe core via LVDS CLK1/CLK2 you can not claim Gen2 compliance. Add a dt property to specify gen1 vs gen2 and check this before allowing a Gen2 link. We default to Gen1 if the property is not present because at this time there are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. In order to be Gen2 compliant on IMX6 you need to: - have a Gen2 compliant external clock generator and route that clock back to either LVDS CLK1 or LVDS CLK2 as an input. (see IMX6SX-SabreSD reference design) - specify this clock in the pcie node in the dt (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) [1] https://community.freescale.com/message/453209 Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v2: - moved dt property to designware core .../devicetree/bindings/pci/designware-pcie.txt | 1 + drivers/pci/host/pci-imx6.c | 16 ++++++++++------ drivers/pci/host/pcie-designware.c | 4 ++++ drivers/pci/host/pcie-designware.h | 1 + 4 files changed, 16 insertions(+), 6 deletions(-)