* [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation @ 2017-01-15 13:19 Lukasz Majewski 2017-01-16 6:37 ` Kishon Vijay Abraham I 2017-01-28 20:54 ` Bjorn Helgaas 0 siblings, 2 replies; 17+ messages in thread From: Lukasz Majewski @ 2017-01-15 13:19 UTC (permalink / raw) To: Kishon Vijay Abraham I, Bjorn Helgaas Cc: Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel, Lukasz Majewski Some devices (due to e.g. bad PCIe signal integrity) require to run with forced GEN1 speed on PCIe bus. This patch changes the speed explicitly on dra7 based devices when proper device tree attribute is defined for the PCIe controller. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- Patch applies on newest origin/master SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 Tested at AM5728 --- Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + drivers/pci/host/pci-dra7xx.c | 23 +++++++++++++++++++++++ drivers/pci/host/pcie-designware.h | 1 + 3 files changed, 25 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt index 60e2516..9f97409 100644 --- a/Documentation/devicetree/bindings/pci/ti-pci.txt +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt @@ -25,6 +25,7 @@ PCIe Designware Controller Optional Property: - gpios : Should be added if a gpio line is required to drive PERST# line + - to,pcie-is-gen1: Indicates that forced gen1 port operation is needed. Example: axi { diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c index 9595fad..eec5fae 100644 --- a/drivers/pci/host/pci-dra7xx.c +++ b/drivers/pci/host/pci-dra7xx.c @@ -63,6 +63,13 @@ #define LINK_UP BIT(16) #define DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF +#define PCIECTRL_EP_DBICS_LNK_CAP 0x007C +#define MAX_LINK_SPEEDS_MASK GENMASK(3, 0) +#define MAX_LINK_SPEEDS_GEN1 BIT(0) + +#define PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C +#define CFG_DIRECTED_SPEED_CHANGE BIT(17) + struct dra7xx_pcie { struct pcie_port pp; void __iomem *base; /* DT ti_conf */ @@ -270,6 +277,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, struct pcie_port *pp = &dra7xx->pp; struct device *dev = pp->dev; struct resource *res; + u32 val; pp->irq = platform_get_irq(pdev, 1); if (pp->irq < 0) { @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, if (!pp->dbi_base) return -ENOMEM; + if (pp->is_gen1) { + dev_info(dev, "GEN1 forced\n"); + + val = readl(pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, MAX_LINK_SPEEDS_GEN1); + writel(val, pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); + + val = readl(pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); + val &= ~CFG_DIRECTED_SPEED_CHANGE; + writel(val, pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); + } + ret = dw_pcie_host_init(pp); if (ret) { dev_err(dev, "failed to initialize host\n"); @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) goto err_gpio; } + if (of_property_read_bool(np, "ti,pcie-is-gen1")) + pp->is_gen1 = true; + reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD); reg &= ~LTSSM_EN; dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index a567ea2..2fb0b18 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -50,6 +50,7 @@ struct pcie_port { struct irq_domain *irq_domain; unsigned long msi_data; u8 iatu_unroll_enabled; + u8 is_gen1; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); }; -- 2.1.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-15 13:19 [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation Lukasz Majewski @ 2017-01-16 6:37 ` Kishon Vijay Abraham I 2017-01-16 6:49 ` Lukasz Majewski 2017-01-28 20:54 ` Bjorn Helgaas 1 sibling, 1 reply; 17+ messages in thread From: Kishon Vijay Abraham I @ 2017-01-16 6:37 UTC (permalink / raw) To: Lukasz Majewski, Bjorn Helgaas Cc: Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel Hi, On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > Some devices (due to e.g. bad PCIe signal integrity) require to run > with forced GEN1 speed on PCIe bus. > > This patch changes the speed explicitly on dra7 based devices when > proper device tree attribute is defined for the PCIe controller. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> Bjorn has already queued a patch to do the same thing https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx Thanks Kishon > --- > > Patch applies on newest origin/master > SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 > > Tested at AM5728 > > --- > Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + > drivers/pci/host/pci-dra7xx.c | 23 +++++++++++++++++++++++ > drivers/pci/host/pcie-designware.h | 1 + > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt > index 60e2516..9f97409 100644 > --- a/Documentation/devicetree/bindings/pci/ti-pci.txt > +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt > @@ -25,6 +25,7 @@ PCIe Designware Controller > > Optional Property: > - gpios : Should be added if a gpio line is required to drive PERST# line > + - to,pcie-is-gen1: Indicates that forced gen1 port operation is needed. > > Example: > axi { > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c > index 9595fad..eec5fae 100644 > --- a/drivers/pci/host/pci-dra7xx.c > +++ b/drivers/pci/host/pci-dra7xx.c > @@ -63,6 +63,13 @@ > #define LINK_UP BIT(16) > #define DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF > > +#define PCIECTRL_EP_DBICS_LNK_CAP 0x007C > +#define MAX_LINK_SPEEDS_MASK GENMASK(3, 0) > +#define MAX_LINK_SPEEDS_GEN1 BIT(0) > + > +#define PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C > +#define CFG_DIRECTED_SPEED_CHANGE BIT(17) > + > struct dra7xx_pcie { > struct pcie_port pp; > void __iomem *base; /* DT ti_conf */ > @@ -270,6 +277,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > struct pcie_port *pp = &dra7xx->pp; > struct device *dev = pp->dev; > struct resource *res; > + u32 val; > > pp->irq = platform_get_irq(pdev, 1); > if (pp->irq < 0) { > @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > if (!pp->dbi_base) > return -ENOMEM; > > + if (pp->is_gen1) { > + dev_info(dev, "GEN1 forced\n"); > + > + val = readl(pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); > + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, MAX_LINK_SPEEDS_GEN1); > + writel(val, pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); > + > + val = readl(pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); > + val &= ~CFG_DIRECTED_SPEED_CHANGE; > + writel(val, pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); > + } > + > ret = dw_pcie_host_init(pp); > if (ret) { > dev_err(dev, "failed to initialize host\n"); > @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) > goto err_gpio; > } > > + if (of_property_read_bool(np, "ti,pcie-is-gen1")) > + pp->is_gen1 = true; > + > reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > reg &= ~LTSSM_EN; > dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index a567ea2..2fb0b18 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -50,6 +50,7 @@ struct pcie_port { > struct irq_domain *irq_domain; > unsigned long msi_data; > u8 iatu_unroll_enabled; > + u8 is_gen1; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 6:37 ` Kishon Vijay Abraham I @ 2017-01-16 6:49 ` Lukasz Majewski 2017-01-16 8:12 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 17+ messages in thread From: Lukasz Majewski @ 2017-01-16 6:49 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel, Bjorn Helgaas Hi Kishon, > Hi, > > On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > > Some devices (due to e.g. bad PCIe signal integrity) require to run > > with forced GEN1 speed on PCIe bus. > > > > This patch changes the speed explicitly on dra7 based devices when > > proper device tree attribute is defined for the PCIe controller. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Bjorn has already queued a patch to do the same thing > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx It seems like Bjorn only modifies CAP registers. He also needs to change register with 0x080C offset to actually ( PCIECTRL_PL_WIDTH_SPEED_CTL ) Best regards, Łukasz > > Thanks > Kishon > > > --- > > > > Patch applies on newest origin/master > > SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 > > > > Tested at AM5728 > > > > --- > > Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + > > drivers/pci/host/pci-dra7xx.c | 23 > > +++++++++++++++++++++++ > > drivers/pci/host/pcie-designware.h | 1 + 3 files > > changed, 25 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt > > b/Documentation/devicetree/bindings/pci/ti-pci.txt index > > 60e2516..9f97409 100644 --- > > a/Documentation/devicetree/bindings/pci/ti-pci.txt +++ > > b/Documentation/devicetree/bindings/pci/ti-pci.txt @@ -25,6 +25,7 > > @@ PCIe Designware Controller > > Optional Property: > > - gpios : Should be added if a gpio line is required to drive > > PERST# line > > + - to,pcie-is-gen1: Indicates that forced gen1 port operation is > > needed. > > Example: > > axi { > > diff --git a/drivers/pci/host/pci-dra7xx.c > > b/drivers/pci/host/pci-dra7xx.c index 9595fad..eec5fae 100644 > > --- a/drivers/pci/host/pci-dra7xx.c > > +++ > > b/drivers/pci/host/pci-https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xxdra7xx.c > > @@ -63,6 +63,13 @@ #define > > LINK_UP BIT(16) > > #define > > DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF > > +#define PCIECTRL_EP_DBICS_LNK_CAP > > 0x007C +#define > > MAX_LINK_SPEEDS_MASK GENMASK(3, 0) > > +#define MAX_LINK_SPEEDS_GEN1 > > BIT(0) + +#define > > PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C > > +#define CFG_DIRECTED_SPEED_CHANGE > > BIT(17) + struct dra7xx_pcie { struct pcie_port pp; > > void __iomem *base; /* DT > > ti_conf */ @@ -270,6 +277,7 @@ static int __init > > dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, struct pcie_port > > *pp = &dra7xx->pp; struct device *dev = pp->dev; > > struct resource *res; > > + u32 val; > > > > pp->irq = platform_get_irq(pdev, 1); > > if (pp->irq < 0) { > > @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct > > dra7xx_pcie *dra7xx, if (!pp->dbi_base) > > return -ENOMEM; > > > > + if (pp->is_gen1) { > > + dev_info(dev, "GEN1 forced\n"); > > + > > + val = readl(pp->dbi_base + > > PCIECTRL_EP_DBICS_LNK_CAP); > > + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, > > MAX_LINK_SPEEDS_GEN1); > > + writel(val, pp->dbi_base + > > PCIECTRL_EP_DBICS_LNK_CAP); + > > + val = readl(pp->dbi_base + > > PCIECTRL_PL_WIDTH_SPEED_CTL); > > + val &= ~CFG_DIRECTED_SPEED_CHANGE; > > + writel(val, pp->dbi_base + > > PCIECTRL_PL_WIDTH_SPEED_CTL); > > + } > > + > > ret = dw_pcie_host_init(pp); > > if (ret) { > > dev_err(dev, "failed to initialize host\n"); > > @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct > > platform_device *pdev) goto err_gpio; > > } > > > > + if (of_property_read_bool(np, "ti,pcie-is-gen1")) > > + pp->is_gen1 = true; > > + > > reg = dra7xx_pcie_readl(dra7xx, > > PCIECTRL_DRA7XX_CONF_DEVICE_CMD); reg &= ~LTSSM_EN; > > dra7xx_pcie_writel(dra7xx, > > PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); diff --git > > a/drivers/pci/host/pcie-designware.h > > b/drivers/pci/host/pcie-designware.h index a567ea2..2fb0b18 100644 > > --- a/drivers/pci/host/pcie-designware.h +++ > > b/drivers/pci/host/pcie-designware.h @@ -50,6 +50,7 @@ struct > > pcie_port { struct irq_domain *irq_domain; > > unsigned long msi_data; > > u8 iatu_unroll_enabled; > > + u8 is_gen1; > > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > > }; > > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 6:49 ` Lukasz Majewski @ 2017-01-16 8:12 ` Kishon Vijay Abraham I 2017-01-16 9:31 ` Lukasz Majewski 0 siblings, 1 reply; 17+ messages in thread From: Kishon Vijay Abraham I @ 2017-01-16 8:12 UTC (permalink / raw) To: Lukasz Majewski Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel Hi Łukasz, On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: > Hi Kishon, > >> Hi, >> >> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>> Some devices (due to e.g. bad PCIe signal integrity) require to run >>> with forced GEN1 speed on PCIe bus. >>> >>> This patch changes the speed explicitly on dra7 based devices when >>> proper device tree attribute is defined for the PCIe controller. >>> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >> >> Bjorn has already queued a patch to do the same thing >> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx > > It seems like Bjorn only modifies CAP registers. The patch also modifies the LNKCTL2 register. > > He also needs to change register with 0x080C offset to actually > ( PCIECTRL_PL_WIDTH_SPEED_CTL ) This bit is used to initiate speed change (after the link is initialized in GEN1). Resetting the bit (like what you have done here) prevents speed change. IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking the IP register. Thanks Kishon > > Best regards, > Łukasz > >> >> Thanks >> Kishon >> >>> --- >>> >>> Patch applies on newest origin/master >>> SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 >>> >>> Tested at AM5728 >>> >>> --- >>> Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + >>> drivers/pci/host/pci-dra7xx.c | 23 >>> +++++++++++++++++++++++ >>> drivers/pci/host/pcie-designware.h | 1 + 3 files >>> changed, 25 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt >>> b/Documentation/devicetree/bindings/pci/ti-pci.txt index >>> 60e2516..9f97409 100644 --- >>> a/Documentation/devicetree/bindings/pci/ti-pci.txt +++ >>> b/Documentation/devicetree/bindings/pci/ti-pci.txt @@ -25,6 +25,7 >>> @@ PCIe Designware Controller >>> Optional Property: >>> - gpios : Should be added if a gpio line is required to drive >>> PERST# line >>> + - to,pcie-is-gen1: Indicates that forced gen1 port operation is >>> needed. >>> Example: >>> axi { >>> diff --git a/drivers/pci/host/pci-dra7xx.c >>> b/drivers/pci/host/pci-dra7xx.c index 9595fad..eec5fae 100644 >>> --- a/drivers/pci/host/pci-dra7xx.c >>> +++ >>> b/drivers/pci/host/pci-https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xxdra7xx.c >>> @@ -63,6 +63,13 @@ #define >>> LINK_UP BIT(16) >>> #define >>> DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF >>> +#define PCIECTRL_EP_DBICS_LNK_CAP >>> 0x007C +#define >>> MAX_LINK_SPEEDS_MASK GENMASK(3, 0) >>> +#define MAX_LINK_SPEEDS_GEN1 >>> BIT(0) + +#define >>> PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C >>> +#define CFG_DIRECTED_SPEED_CHANGE >>> BIT(17) + struct dra7xx_pcie { struct pcie_port pp; >>> void __iomem *base; /* DT >>> ti_conf */ @@ -270,6 +277,7 @@ static int __init >>> dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, struct pcie_port >>> *pp = &dra7xx->pp; struct device *dev = pp->dev; >>> struct resource *res; >>> + u32 val; >>> >>> pp->irq = platform_get_irq(pdev, 1); >>> if (pp->irq < 0) { >>> @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct >>> dra7xx_pcie *dra7xx, if (!pp->dbi_base) >>> return -ENOMEM; >>> >>> + if (pp->is_gen1) { >>> + dev_info(dev, "GEN1 forced\n"); >>> + >>> + val = readl(pp->dbi_base + >>> PCIECTRL_EP_DBICS_LNK_CAP); >>> + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, >>> MAX_LINK_SPEEDS_GEN1); >>> + writel(val, pp->dbi_base + >>> PCIECTRL_EP_DBICS_LNK_CAP); + >>> + val = readl(pp->dbi_base + >>> PCIECTRL_PL_WIDTH_SPEED_CTL); >>> + val &= ~CFG_DIRECTED_SPEED_CHANGE; >>> + writel(val, pp->dbi_base + >>> PCIECTRL_PL_WIDTH_SPEED_CTL); >>> + } >>> + >>> ret = dw_pcie_host_init(pp); >>> if (ret) { >>> dev_err(dev, "failed to initialize host\n"); >>> @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct >>> platform_device *pdev) goto err_gpio; >>> } >>> >>> + if (of_property_read_bool(np, "ti,pcie-is-gen1")) >>> + pp->is_gen1 = true; >>> + >>> reg = dra7xx_pcie_readl(dra7xx, >>> PCIECTRL_DRA7XX_CONF_DEVICE_CMD); reg &= ~LTSSM_EN; >>> dra7xx_pcie_writel(dra7xx, >>> PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); diff --git >>> a/drivers/pci/host/pcie-designware.h >>> b/drivers/pci/host/pcie-designware.h index a567ea2..2fb0b18 100644 >>> --- a/drivers/pci/host/pcie-designware.h +++ >>> b/drivers/pci/host/pcie-designware.h @@ -50,6 +50,7 @@ struct >>> pcie_port { struct irq_domain *irq_domain; >>> unsigned long msi_data; >>> u8 iatu_unroll_enabled; >>> + u8 is_gen1; >>> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >>> }; >>> >>> > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 8:12 ` Kishon Vijay Abraham I @ 2017-01-16 9:31 ` Lukasz Majewski 2017-01-16 10:13 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 17+ messages in thread From: Lukasz Majewski @ 2017-01-16 9:31 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel Hi Kishon, > Hi Łukasz, > > On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: > > Hi Kishon, > > > >> Hi, > >> > >> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > >>> Some devices (due to e.g. bad PCIe signal integrity) require to > >>> run with forced GEN1 speed on PCIe bus. > >>> > >>> This patch changes the speed explicitly on dra7 based devices when > >>> proper device tree attribute is defined for the PCIe controller. > >>> > >>> Signed-off-by: Lukasz Majewski <lukma@denx.de> > >> > >> Bjorn has already queued a patch to do the same thing > >> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx > > > > It seems like Bjorn only modifies CAP registers. > > The patch also modifies the LNKCTL2 register. > > > > He also needs to change register with 0x080C offset to actually > > ( PCIECTRL_PL_WIDTH_SPEED_CTL ) > > This bit is used to initiate speed change (after the link is > initialized in GEN1). Resetting the bit (like what you have done > here) prevents speed change. This is strange, but e2e advised me to do things as I did in the patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) Link: [1] https://e2e.ti.com/support/arm/sitara_arm/f/791/t/566421 Both patches modify 0x5180 007C register to set GEN1 capability (PCI_EXP_LNKCAP_SLS_2_5GB) The problem is with second register (in your patch): >From SPRUHZ6G TRM: PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more description in TRM It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as default /reset value. Could you clarify which way to _force_ PCIe GEN1 operation is correct? Mine shows differences in lspci output (as posted in [1]). > > IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking > the IP register. >From the original patch description: "Add support to force Root Complex to work in GEN1 mode if so desired, but don't force GEN1 mode on any board just yet." Are there any (floating around) patches allowing forcing GEN1 operation on any board (I would like to reuse/port them to my current solution)? Thanks in advance, Łukasz Majewski > > Thanks > Kishon > > > > > Best regards, > > Łukasz > > > >> > >> Thanks > >> Kishon > >> > >>> --- > >>> > >>> Patch applies on newest origin/master > >>> SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 > >>> > >>> Tested at AM5728 > >>> > >>> --- > >>> Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + > >>> drivers/pci/host/pci-dra7xx.c | 23 > >>> +++++++++++++++++++++++ > >>> drivers/pci/host/pcie-designware.h | 1 + 3 files > >>> changed, 25 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt > >>> b/Documentation/devicetree/bindings/pci/ti-pci.txt index > >>> 60e2516..9f97409 100644 --- > >>> a/Documentation/devicetree/bindings/pci/ti-pci.txt +++ > >>> b/Documentation/devicetree/bindings/pci/ti-pci.txt @@ -25,6 +25,7 > >>> @@ PCIe Designware Controller > >>> Optional Property: > >>> - gpios : Should be added if a gpio line is required to drive > >>> PERST# line > >>> + - to,pcie-is-gen1: Indicates that forced gen1 port operation is > >>> needed. > >>> Example: > >>> axi { > >>> diff --git a/drivers/pci/host/pci-dra7xx.c > >>> b/drivers/pci/host/pci-dra7xx.c index 9595fad..eec5fae 100644 > >>> --- a/drivers/pci/host/pci-dra7xx.c > >>> +++ > >>> b/drivers/pci/host/pci-https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xxdra7xx.c > >>> @@ -63,6 +63,13 @@ #define > >>> LINK_UP BIT(16) > >>> #define > >>> DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF > >>> +#define PCIECTRL_EP_DBICS_LNK_CAP > >>> 0x007C +#define > >>> MAX_LINK_SPEEDS_MASK GENMASK(3, 0) > >>> +#define MAX_LINK_SPEEDS_GEN1 > >>> BIT(0) + +#define > >>> PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C > >>> +#define CFG_DIRECTED_SPEED_CHANGE > >>> BIT(17) + struct dra7xx_pcie { struct pcie_port pp; > >>> void __iomem *base; /* DT > >>> ti_conf */ @@ -270,6 +277,7 @@ static int __init > >>> dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, struct pcie_port > >>> *pp = &dra7xx->pp; struct device *dev = pp->dev; > >>> struct resource *res; > >>> + u32 val; > >>> > >>> pp->irq = platform_get_irq(pdev, 1); > >>> if (pp->irq < 0) { > >>> @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct > >>> dra7xx_pcie *dra7xx, if (!pp->dbi_base) > >>> return -ENOMEM; > >>> > >>> + if (pp->is_gen1) { > >>> + dev_info(dev, "GEN1 forced\n"); > >>> + > >>> + val = readl(pp->dbi_base + > >>> PCIECTRL_EP_DBICS_LNK_CAP); > >>> + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, > >>> MAX_LINK_SPEEDS_GEN1); > >>> + writel(val, pp->dbi_base + > >>> PCIECTRL_EP_DBICS_LNK_CAP); + > >>> + val = readl(pp->dbi_base + > >>> PCIECTRL_PL_WIDTH_SPEED_CTL); > >>> + val &= ~CFG_DIRECTED_SPEED_CHANGE; > >>> + writel(val, pp->dbi_base + > >>> PCIECTRL_PL_WIDTH_SPEED_CTL); > >>> + } > >>> + > >>> ret = dw_pcie_host_init(pp); > >>> if (ret) { > >>> dev_err(dev, "failed to initialize host\n"); > >>> @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct > >>> platform_device *pdev) goto err_gpio; > >>> } > >>> > >>> + if (of_property_read_bool(np, "ti,pcie-is-gen1")) > >>> + pp->is_gen1 = true; > >>> + > >>> reg = dra7xx_pcie_readl(dra7xx, > >>> PCIECTRL_DRA7XX_CONF_DEVICE_CMD); reg &= ~LTSSM_EN; > >>> dra7xx_pcie_writel(dra7xx, > >>> PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); diff --git > >>> a/drivers/pci/host/pcie-designware.h > >>> b/drivers/pci/host/pcie-designware.h index a567ea2..2fb0b18 100644 > >>> --- a/drivers/pci/host/pcie-designware.h +++ > >>> b/drivers/pci/host/pcie-designware.h @@ -50,6 +50,7 @@ struct > >>> pcie_port { struct irq_domain *irq_domain; > >>> unsigned long msi_data; > >>> u8 iatu_unroll_enabled; > >>> + u8 is_gen1; > >>> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > >>> }; > >>> > >>> > > > > > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Wolfgang > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: > > wd@denx.de > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 9:31 ` Lukasz Majewski @ 2017-01-16 10:13 ` Kishon Vijay Abraham I 2017-01-16 10:34 ` Lukasz Majewski ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Kishon Vijay Abraham I @ 2017-01-16 10:13 UTC (permalink / raw) To: Lukasz Majewski, Joao Pinto, jingoohan1 Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel + Joao, Jingoo Hi, On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: > Hi Kishon, > >> Hi Łukasz, >> >> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>> Hi Kishon, >>> >>>> Hi, >>>> >>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>> run with forced GEN1 speed on PCIe bus. >>>>> >>>>> This patch changes the speed explicitly on dra7 based devices when >>>>> proper device tree attribute is defined for the PCIe controller. >>>>> >>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>> >>>> Bjorn has already queued a patch to do the same thing >>>> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx >>> >>> It seems like Bjorn only modifies CAP registers. >> >> The patch also modifies the LNKCTL2 register. >>> >>> He also needs to change register with 0x080C offset to actually >>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >> >> This bit is used to initiate speed change (after the link is >> initialized in GEN1). Resetting the bit (like what you have done >> here) prevents speed change. > > This is strange, but e2e advised me to do things as I did in the patch > to _force_ GEN1 operation on PCIe2 port [1] (AM5728) > > Link: > [1] https://e2e.ti.com/support/arm/sitara_arm/f/791/t/566421 > > Both patches modify 0x5180 007C register to set GEN1 capability > (PCI_EXP_LNKCAP_SLS_2_5GB) > > The problem is with second register (in your patch): > > From SPRUHZ6G TRM: > > PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) > - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more > description in TRM > > It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as > default /reset value. The default value is 0x2 (or else none of the cards would have enumerated in GEN2) > > > Could you clarify which way to _force_ PCIe GEN1 operation is correct? > Mine shows differences in lspci output (as posted in [1]). You'll see the difference even with the patch in Bjorn's tree ;-) I think these are 2 different approaches to keep the link at GEN1. Joao or Jingoo, do you have any suggestion here? > >> >> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking >> the IP register. > > From the original patch description: > > "Add support to force Root Complex to work in GEN1 mode if so desired, > but don't force GEN1 mode on any board just yet." > > Are there any (floating around) patches allowing forcing GEN1 operation > on any board (I would like to reuse/port them to my current solution)? For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt with the patch in Bjorn's tree. Thanks Kishon ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 10:13 ` Kishon Vijay Abraham I @ 2017-01-16 10:34 ` Lukasz Majewski 2017-01-16 13:40 ` Joao Pinto 2017-01-16 17:01 ` Joao Pinto 2 siblings, 0 replies; 17+ messages in thread From: Lukasz Majewski @ 2017-01-16 10:34 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Joao Pinto, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Dear All, > + Joao, Jingoo > > Hi, > > On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: > > Hi Kishon, > > > >> Hi Łukasz, > >> > >> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: > >>> Hi Kishon, > >>> > >>>> Hi, > >>>> > >>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > >>>>> Some devices (due to e.g. bad PCIe signal integrity) require to > >>>>> run with forced GEN1 speed on PCIe bus. > >>>>> > >>>>> This patch changes the speed explicitly on dra7 based devices > >>>>> when proper device tree attribute is defined for the PCIe > >>>>> controller. > >>>>> > >>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> > >>>> > >>>> Bjorn has already queued a patch to do the same thing > >>>> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-dra7xx > >>> > >>> It seems like Bjorn only modifies CAP registers. > >> > >> The patch also modifies the LNKCTL2 register. > >>> > >>> He also needs to change register with 0x080C offset to actually > >>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) > >> > >> This bit is used to initiate speed change (after the link is > >> initialized in GEN1). Resetting the bit (like what you have done > >> here) prevents speed change. > > > > This is strange, but e2e advised me to do things as I did in the > > patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) > > > > Link: > > [1] https://e2e.ti.com/support/arm/sitara_arm/f/791/t/566421 > > > > Both patches modify 0x5180 007C register to set GEN1 capability > > (PCI_EXP_LNKCAP_SLS_2_5GB) > > > > The problem is with second register (in your patch): > > > > From SPRUHZ6G TRM: > > > > PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) > > - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more > > description in TRM > > > > It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as > > default /reset value. > > The default value is 0x2 (or else none of the cards would have > enumerated in GEN2) SPRUHZ6G – October 2014 – Revised June 2016 - page 6313. Maybe TRM is not up to date? > > > > > > Could you clarify which way to _force_ PCIe GEN1 operation is > > correct? Mine shows differences in lspci output (as posted in [1]). > > You'll see the difference even with the patch in Bjorn's tree ;-) :-) The details of my test cases and output are in the post [1]. > > I think these are 2 different approaches to keep the link at GEN1. > Joao or Jingoo, do you have any suggestion here? Please read through thread [1] [1] https://e2e.ti.com/support/arm/sitara_arm/f/791/t/566421 > > > > >> > >> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking > >> the IP register. > > > > From the original patch description: > > > > "Add support to force Root Complex to work in GEN1 mode if so > > desired, but don't force GEN1 mode on any board just yet." > > > > Are there any (floating around) patches allowing forcing GEN1 > > operation on any board (I would like to reuse/port them to my > > current solution)? > > For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt > with the patch in Bjorn's tree. Ah.... ok. > > Thanks > Kishon Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 10:13 ` Kishon Vijay Abraham I 2017-01-16 10:34 ` Lukasz Majewski @ 2017-01-16 13:40 ` Joao Pinto 2017-01-16 17:01 ` Joao Pinto 2 siblings, 0 replies; 17+ messages in thread From: Joao Pinto @ 2017-01-16 13:40 UTC (permalink / raw) To: Kishon Vijay Abraham I, Lukasz Majewski, Joao Pinto, jingoohan1 Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi, Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: > + Joao, Jingoo > > Hi, > > On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >> Hi Kishon, >> >>> Hi Łukasz, >>> >>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>> Hi Kishon, >>>> >>>>> Hi, >>>>> >>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>> run with forced GEN1 speed on PCIe bus. >>>>>> >>>>>> This patch changes the speed explicitly on dra7 based devices when >>>>>> proper device tree attribute is defined for the PCIe controller. >>>>>> >>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>> >>>>> Bjorn has already queued a patch to do the same thing >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>> >>>> It seems like Bjorn only modifies CAP registers. >>> >>> The patch also modifies the LNKCTL2 register. >>>> >>>> He also needs to change register with 0x080C offset to actually >>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>> >>> This bit is used to initiate speed change (after the link is >>> initialized in GEN1). Resetting the bit (like what you have done >>> here) prevents speed change. >> >> This is strange, but e2e advised me to do things as I did in the patch >> to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >> >> Link: >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >> >> Both patches modify 0x5180 007C register to set GEN1 capability >> (PCI_EXP_LNKCAP_SLS_2_5GB) >> >> The problem is with second register (in your patch): >> >> From SPRUHZ6G TRM: >> >> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >> description in TRM >> >> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >> default /reset value. > > The default value is 0x2 (or else none of the cards would have enumerated in GEN2) >> >> >> Could you clarify which way to _force_ PCIe GEN1 operation is correct? >> Mine shows differences in lspci output (as posted in [1]). > > You'll see the difference even with the patch in Bjorn's tree ;-) > > I think these are 2 different approaches to keep the link at GEN1. Joao or > Jingoo, do you have any suggestion here? I am going to check this and come back soon with a fundamented opinion. Thanks. > >> >>> >>> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking >>> the IP register. >> >> From the original patch description: >> >> "Add support to force Root Complex to work in GEN1 mode if so desired, >> but don't force GEN1 mode on any board just yet." >> >> Are there any (floating around) patches allowing forcing GEN1 operation >> on any board (I would like to reuse/port them to my current solution)? > > For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt with the > patch in Bjorn's tree. > > Thanks > Kishon > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 10:13 ` Kishon Vijay Abraham I 2017-01-16 10:34 ` Lukasz Majewski 2017-01-16 13:40 ` Joao Pinto @ 2017-01-16 17:01 ` Joao Pinto 2017-01-16 21:44 ` Lukasz Majewski 2017-01-17 5:35 ` Kishon Vijay Abraham I 2 siblings, 2 replies; 17+ messages in thread From: Joao Pinto @ 2017-01-16 17:01 UTC (permalink / raw) To: Kishon Vijay Abraham I, Lukasz Majewski, Joao Pinto, jingoohan1 Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi, Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: > + Joao, Jingoo > > Hi, > > On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >> Hi Kishon, >> >>> Hi Łukasz, >>> >>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>> Hi Kishon, >>>> >>>>> Hi, >>>>> >>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>> run with forced GEN1 speed on PCIe bus. >>>>>> >>>>>> This patch changes the speed explicitly on dra7 based devices when >>>>>> proper device tree attribute is defined for the PCIe controller. >>>>>> >>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>> >>>>> Bjorn has already queued a patch to do the same thing >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>> >>>> It seems like Bjorn only modifies CAP registers. >>> >>> The patch also modifies the LNKCTL2 register. >>>> >>>> He also needs to change register with 0x080C offset to actually >>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>> >>> This bit is used to initiate speed change (after the link is >>> initialized in GEN1). Resetting the bit (like what you have done >>> here) prevents speed change. >> >> This is strange, but e2e advised me to do things as I did in the patch >> to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >> >> Link: >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >> >> Both patches modify 0x5180 007C register to set GEN1 capability >> (PCI_EXP_LNKCAP_SLS_2_5GB) >> >> The problem is with second register (in your patch): >> >> From SPRUHZ6G TRM: >> >> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >> description in TRM >> >> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >> default /reset value. > > The default value is 0x2 (or else none of the cards would have enumerated in GEN2) >> >> >> Could you clarify which way to _force_ PCIe GEN1 operation is correct? >> Mine shows differences in lspci output (as posted in [1]). > > You'll see the difference even with the patch in Bjorn's tree ;-) > > I think these are 2 different approaches to keep the link at GEN1. Joao or > Jingoo, do you have any suggestion here? I studied the Databook, and both approaches seem to be right, dependently of the Core configuration and setup. The standard manual speed change sequence is: a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) b) Clear "Directed Speed Change" c) Set "Directed Speed Change" If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the default value), it will execute LTSSM to initiate speed change to Gen2 or Gen3, after link is started in Gen1, and then the bit is automatically cleared. Lukasz is reseting this bit, in order to avoid the LTSSM to be executed, which is correct. There is another way to prevent this automatic speed change, which is to set GEN1 speed before link up which might be difficult in some setups, so Kishon's also right. In my opinion Lukasz approach would be the one that might be more universal and more "secure". Joao > >> >>> >>> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking >>> the IP register. >> >> From the original patch description: >> >> "Add support to force Root Complex to work in GEN1 mode if so desired, >> but don't force GEN1 mode on any board just yet." >> >> Are there any (floating around) patches allowing forcing GEN1 operation >> on any board (I would like to reuse/port them to my current solution)? > > For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt with the > patch in Bjorn's tree. > > Thanks > Kishon > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 17:01 ` Joao Pinto @ 2017-01-16 21:44 ` Lukasz Majewski 2017-01-17 10:43 ` Joao Pinto 2017-01-17 5:35 ` Kishon Vijay Abraham I 1 sibling, 1 reply; 17+ messages in thread From: Lukasz Majewski @ 2017-01-16 21:44 UTC (permalink / raw) To: Joao Pinto Cc: Kishon Vijay Abraham I, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi Joao, > > Hi, > > Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: > > + Joao, Jingoo > > > > Hi, > > > > On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: > >> Hi Kishon, > >> > >>> Hi Łukasz, > >>> > >>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: > >>>> Hi Kishon, > >>>> > >>>>> Hi, > >>>>> > >>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > >>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to > >>>>>> run with forced GEN1 speed on PCIe bus. > >>>>>> > >>>>>> This patch changes the speed explicitly on dra7 based devices > >>>>>> when proper device tree attribute is defined for the PCIe > >>>>>> controller. > >>>>>> > >>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> > >>>>> > >>>>> Bjorn has already queued a patch to do the same thing > >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= > >>>> > >>>> It seems like Bjorn only modifies CAP registers. > >>> > >>> The patch also modifies the LNKCTL2 register. > >>>> > >>>> He also needs to change register with 0x080C offset to actually > >>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) > >>> > >>> This bit is used to initiate speed change (after the link is > >>> initialized in GEN1). Resetting the bit (like what you have done > >>> here) prevents speed change. > >> > >> This is strange, but e2e advised me to do things as I did in the > >> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) > >> > >> Link: > >> [1] > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= > >> > >> Both patches modify 0x5180 007C register to set GEN1 capability > >> (PCI_EXP_LNKCAP_SLS_2_5GB) > >> > >> The problem is with second register (in your patch): > >> > >> From SPRUHZ6G TRM: > >> > >> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) > >> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more > >> description in TRM > >> > >> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as > >> default /reset value. > > > > The default value is 0x2 (or else none of the cards would have > > enumerated in GEN2) > >> > >> > >> Could you clarify which way to _force_ PCIe GEN1 operation is > >> correct? Mine shows differences in lspci output (as posted in [1]). > > > > You'll see the difference even with the patch in Bjorn's tree ;-) > > > > I think these are 2 different approaches to keep the link at GEN1. > > Joao or Jingoo, do you have any suggestion here? > > I studied the Databook, Could you reveal which databook do you have in mind? Is that the TRM for AM5728? > and both approaches seem to be right, > dependently of the Core configuration and setup. > > The standard manual speed change sequence is: > a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ? > b) Clear "Directed Speed Change" CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C > c) Set "Directed Speed Change" > > If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the > default value), it will execute LTSSM to initiate speed change to > Gen2 or Gen3, after link is started in Gen1, and then the bit is > automatically cleared. Ok, so with default settings (after reset) we do have Gen1 speed link and when we enable LTSSM (with LTSSM_EN bit setting) we negotiate to Gen2/Gen3. > > Lukasz is reseting this bit, in order to avoid the LTSSM to be > executed, which is correct. So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with LTSSM_EN) the state machine returns immediately and leaves link in the Gen1? The pci-dra7 driver always sets LTSSM_EN bit to start link negotiation. > There is another way to prevent this > automatic speed change, which is to set GEN1 speed before link up > which might be difficult in some setups, so Kishon's also right. > > In my opinion Lukasz approach would be the one that might be more > universal and more "secure". The robustness is the key here since there are some devices, which on particular HW must only work with Gen1 speed. When we start LTSSM state machine and hence start negotiation to Gen2, not always the result of LTSSM is correct and device is properly recognized. > > Joao > > > > > >> > >>> > >>> IMO the better way is to set the LNKCTL2 to GEN1 instead of > >>> hacking the IP register. > >> > >> From the original patch description: > >> > >> "Add support to force Root Complex to work in GEN1 mode if so > >> desired, but don't force GEN1 mode on any board just yet." > >> > >> Are there any (floating around) patches allowing forcing GEN1 > >> operation on any board (I would like to reuse/port them to my > >> current solution)? > > > > For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt > > with the patch in Bjorn's tree. > > > > Thanks > > Kishon > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 21:44 ` Lukasz Majewski @ 2017-01-17 10:43 ` Joao Pinto 2017-01-17 11:23 ` Joao Pinto 0 siblings, 1 reply; 17+ messages in thread From: Joao Pinto @ 2017-01-17 10:43 UTC (permalink / raw) To: Lukasz Majewski, Joao Pinto Cc: Kishon Vijay Abraham I, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi Lukasz, Às 9:44 PM de 1/16/2017, Lukasz Majewski escreveu: > Hi Joao, > >> >> Hi, >> >> Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: >>> + Joao, Jingoo >>> >>> Hi, >>> >>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >>>> Hi Kishon, >>>> >>>>> Hi Łukasz, >>>>> >>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>>>> Hi Kishon, >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>>>> run with forced GEN1 speed on PCIe bus. >>>>>>>> >>>>>>>> This patch changes the speed explicitly on dra7 based devices >>>>>>>> when proper device tree attribute is defined for the PCIe >>>>>>>> controller. >>>>>>>> >>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>>>> >>>>>>> Bjorn has already queued a patch to do the same thing >>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>>>> >>>>>> It seems like Bjorn only modifies CAP registers. >>>>> >>>>> The patch also modifies the LNKCTL2 register. >>>>>> >>>>>> He also needs to change register with 0x080C offset to actually >>>>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>>>> >>>>> This bit is used to initiate speed change (after the link is >>>>> initialized in GEN1). Resetting the bit (like what you have done >>>>> here) prevents speed change. >>>> >>>> This is strange, but e2e advised me to do things as I did in the >>>> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >>>> >>>> Link: >>>> [1] >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >>>> >>>> Both patches modify 0x5180 007C register to set GEN1 capability >>>> (PCI_EXP_LNKCAP_SLS_2_5GB) >>>> >>>> The problem is with second register (in your patch): >>>> >>>> From SPRUHZ6G TRM: >>>> >>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >>>> description in TRM >>>> >>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >>>> default /reset value. >>> >>> The default value is 0x2 (or else none of the cards would have >>> enumerated in GEN2) >>>> >>>> >>>> Could you clarify which way to _force_ PCIe GEN1 operation is >>>> correct? Mine shows differences in lspci output (as posted in [1]). >>> >>> You'll see the difference even with the patch in Bjorn's tree ;-) >>> >>> I think these are 2 different approaches to keep the link at GEN1. >>> Joao or Jingoo, do you have any suggestion here? >> >> I studied the Databook, > > Could you reveal which databook do you have in mind? Is that the TRM for > AM5728? I checked the Designware PCIe Databook, since it is based on this IP. > >> and both approaches seem to be right, >> dependently of the Core configuration and setup. >> >> The standard manual speed change sequence is: >> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) > > Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ? Correct. > >> b) Clear "Directed Speed Change" > > CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C Correct. > >> c) Set "Directed Speed Change" >> >> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the >> default value), it will execute LTSSM to initiate speed change to >> Gen2 or Gen3, after link is started in Gen1, and then the bit is >> automatically cleared. > > Ok, so with default settings (after reset) we do have Gen1 speed link > and when we enable LTSSM (with LTSSM_EN bit setting) we negotiate to > Gen2/Gen3. Yes, that's the expected behavior. I submited this direct question to R&D and will have your doubt answered soon. > >> >> Lukasz is reseting this bit, in order to avoid the LTSSM to be >> executed, which is correct. > > So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with > LTSSM_EN) the state machine returns immediately and leaves link in the > Gen1? > > The pci-dra7 driver always sets LTSSM_EN bit to start link negotiation. > >> There is another way to prevent this >> automatic speed change, which is to set GEN1 speed before link up >> which might be difficult in some setups, so Kishon's also right. >> >> In my opinion Lukasz approach would be the one that might be more >> universal and more "secure". > > The robustness is the key here since there are some devices, which on > particular HW must only work with Gen1 speed. When we start LTSSM state > machine and hence start negotiation to Gen2, not always the result of > LTSSM is correct and device is properly recognized. > >> >> Joao >> >> >>> >>>> >>>>> >>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of >>>>> hacking the IP register. >>>> >>>> From the original patch description: >>>> >>>> "Add support to force Root Complex to work in GEN1 mode if so >>>> desired, but don't force GEN1 mode on any board just yet." >>>> >>>> Are there any (floating around) patches allowing forcing GEN1 >>>> operation on any board (I would like to reuse/port them to my >>>> current solution)? >>> >>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt >>> with the patch in Bjorn's tree. >>> >>> Thanks >>> Kishon >>> >> > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-17 10:43 ` Joao Pinto @ 2017-01-17 11:23 ` Joao Pinto 2017-01-17 11:38 ` Lukasz Majewski 0 siblings, 1 reply; 17+ messages in thread From: Joao Pinto @ 2017-01-17 11:23 UTC (permalink / raw) To: Lukasz Majewski, Joao Pinto Cc: Kishon Vijay Abraham I, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Às 10:43 AM de 1/17/2017, Joao Pinto escreveu: > > Hi Lukasz, > > Às 9:44 PM de 1/16/2017, Lukasz Majewski escreveu: >> Hi Joao, >> >>> >>> Hi, >>> >>> Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: >>>> + Joao, Jingoo >>>> >>>> Hi, >>>> >>>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >>>>> Hi Kishon, >>>>> >>>>>> Hi Łukasz, >>>>>> >>>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>>>>> Hi Kishon, >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>>>>> run with forced GEN1 speed on PCIe bus. >>>>>>>>> >>>>>>>>> This patch changes the speed explicitly on dra7 based devices >>>>>>>>> when proper device tree attribute is defined for the PCIe >>>>>>>>> controller. >>>>>>>>> >>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>>>>> >>>>>>>> Bjorn has already queued a patch to do the same thing >>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>>>>> >>>>>>> It seems like Bjorn only modifies CAP registers. >>>>>> >>>>>> The patch also modifies the LNKCTL2 register. >>>>>>> >>>>>>> He also needs to change register with 0x080C offset to actually >>>>>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>>>>> >>>>>> This bit is used to initiate speed change (after the link is >>>>>> initialized in GEN1). Resetting the bit (like what you have done >>>>>> here) prevents speed change. >>>>> >>>>> This is strange, but e2e advised me to do things as I did in the >>>>> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >>>>> >>>>> Link: >>>>> [1] >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >>>>> >>>>> Both patches modify 0x5180 007C register to set GEN1 capability >>>>> (PCI_EXP_LNKCAP_SLS_2_5GB) >>>>> >>>>> The problem is with second register (in your patch): >>>>> >>>>> From SPRUHZ6G TRM: >>>>> >>>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >>>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >>>>> description in TRM >>>>> >>>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >>>>> default /reset value. >>>> >>>> The default value is 0x2 (or else none of the cards would have >>>> enumerated in GEN2) >>>>> >>>>> >>>>> Could you clarify which way to _force_ PCIe GEN1 operation is >>>>> correct? Mine shows differences in lspci output (as posted in [1]). >>>> >>>> You'll see the difference even with the patch in Bjorn's tree ;-) >>>> >>>> I think these are 2 different approaches to keep the link at GEN1. >>>> Joao or Jingoo, do you have any suggestion here? >>> >>> I studied the Databook, >> >> Could you reveal which databook do you have in mind? Is that the TRM for >> AM5728? > > I checked the Designware PCIe Databook, since it is based on this IP. > >> >>> and both approaches seem to be right, >>> dependently of the Core configuration and setup. >>> >>> The standard manual speed change sequence is: >>> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) >> >> Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ? > > Correct. > >> >>> b) Clear "Directed Speed Change" >> >> CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C > > Correct. > >> >>> c) Set "Directed Speed Change" >>> >>> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the >>> default value), it will execute LTSSM to initiate speed change to >>> Gen2 or Gen3, after link is started in Gen1, and then the bit is >>> automatically cleared. >> >> Ok, so with default settings (after reset) we do have Gen1 speed link >> and when we enable LTSSM (with LTSSM_EN bit setting) we negotiate to >> Gen2/Gen3. > > Yes, that's the expected behavior. I submited this direct question to R&D and > will have your doubt answered soon. According to R&D if you set "Target Link Speed" to Gen1 before setting LTSSM_EN bit, the controller should stay in GEN1. > >> >>> >>> Lukasz is reseting this bit, in order to avoid the LTSSM to be >>> executed, which is correct. >> >> So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with >> LTSSM_EN) the state machine returns immediately and leaves link in the >> Gen1? >> >> The pci-dra7 driver always sets LTSSM_EN bit to start link negotiation. >> >>> There is another way to prevent this >>> automatic speed change, which is to set GEN1 speed before link up >>> which might be difficult in some setups, so Kishon's also right. >>> >>> In my opinion Lukasz approach would be the one that might be more >>> universal and more "secure". >> >> The robustness is the key here since there are some devices, which on >> particular HW must only work with Gen1 speed. When we start LTSSM state >> machine and hence start negotiation to Gen2, not always the result of >> LTSSM is correct and device is properly recognized. >> >>> >>> Joao >>> >>> >>>> >>>>> >>>>>> >>>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of >>>>>> hacking the IP register. >>>>> >>>>> From the original patch description: >>>>> >>>>> "Add support to force Root Complex to work in GEN1 mode if so >>>>> desired, but don't force GEN1 mode on any board just yet." >>>>> >>>>> Are there any (floating around) patches allowing forcing GEN1 >>>>> operation on any board (I would like to reuse/port them to my >>>>> current solution)? >>>> >>>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt >>>> with the patch in Bjorn's tree. >>>> >>>> Thanks >>>> Kishon >>>> >>> >> >> Best regards, >> >> Lukasz Majewski >> >> -- >> >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-17 11:23 ` Joao Pinto @ 2017-01-17 11:38 ` Lukasz Majewski 2017-01-17 11:48 ` Joao Pinto 0 siblings, 1 reply; 17+ messages in thread From: Lukasz Majewski @ 2017-01-17 11:38 UTC (permalink / raw) To: Joao Pinto Cc: Kishon Vijay Abraham I, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel, Finger, Robert Hi Joao, Thank you for your reply. > Às 10:43 AM de 1/17/2017, Joao Pinto escreveu: > > > > Hi Lukasz, > > > > Às 9:44 PM de 1/16/2017, Lukasz Majewski escreveu: > >> Hi Joao, > >> > >>> > >>> Hi, > >>> > >>> Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: > >>>> + Joao, Jingoo > >>>> > >>>> Hi, > >>>> > >>>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: > >>>>> Hi Kishon, > >>>>> > >>>>>> Hi Łukasz, > >>>>>> > >>>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: > >>>>>>> Hi Kishon, > >>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: > >>>>>>>>> Some devices (due to e.g. bad PCIe signal integrity) > >>>>>>>>> require to run with forced GEN1 speed on PCIe bus. > >>>>>>>>> > >>>>>>>>> This patch changes the speed explicitly on dra7 based > >>>>>>>>> devices when proper device tree attribute is defined for > >>>>>>>>> the PCIe controller. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> > >>>>>>>> > >>>>>>>> Bjorn has already queued a patch to do the same thing > >>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= > >>>>>>> > >>>>>>> It seems like Bjorn only modifies CAP registers. > >>>>>> > >>>>>> The patch also modifies the LNKCTL2 register. > >>>>>>> > >>>>>>> He also needs to change register with 0x080C offset to > >>>>>>> actually ( PCIECTRL_PL_WIDTH_SPEED_CTL ) > >>>>>> > >>>>>> This bit is used to initiate speed change (after the link is > >>>>>> initialized in GEN1). Resetting the bit (like what you have > >>>>>> done here) prevents speed change. > >>>>> > >>>>> This is strange, but e2e advised me to do things as I did in the > >>>>> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) > >>>>> > >>>>> Link: > >>>>> [1] > >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= > >>>>> > >>>>> Both patches modify 0x5180 007C register to set GEN1 capability > >>>>> (PCI_EXP_LNKCAP_SLS_2_5GB) > >>>>> > >>>>> The problem is with second register (in your patch): > >>>>> > >>>>> From SPRUHZ6G TRM: > >>>>> > >>>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) > >>>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more > >>>>> description in TRM > >>>>> > >>>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same > >>>>> as default /reset value. > >>>> > >>>> The default value is 0x2 (or else none of the cards would have > >>>> enumerated in GEN2) > >>>>> > >>>>> > >>>>> Could you clarify which way to _force_ PCIe GEN1 operation is > >>>>> correct? Mine shows differences in lspci output (as posted in > >>>>> [1]). > >>>> > >>>> You'll see the difference even with the patch in Bjorn's tree ;-) > >>>> > >>>> I think these are 2 different approaches to keep the link at > >>>> GEN1. Joao or Jingoo, do you have any suggestion here? > >>> > >>> I studied the Databook, > >> > >> Could you reveal which databook do you have in mind? Is that the > >> TRM for AM5728? > > > > I checked the Designware PCIe Databook, since it is based on this > > IP. > > > >> > >>> and both approaches seem to be right, > >>> dependently of the Core configuration and setup. > >>> > >>> The standard manual speed change sequence is: > >>> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) > >> > >> Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ? > > > > Correct. > > > >> > >>> b) Clear "Directed Speed Change" > >> > >> CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C > > > > Correct. > > > >> > >>> c) Set "Directed Speed Change" > >>> > >>> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is > >>> the default value), it will execute LTSSM to initiate speed > >>> change to Gen2 or Gen3, after link is started in Gen1, and then > >>> the bit is automatically cleared. > >> > >> Ok, so with default settings (after reset) we do have Gen1 speed > >> link and when we enable LTSSM (with LTSSM_EN bit setting) we > >> negotiate to Gen2/Gen3. > > > > Yes, that's the expected behavior. I submited this direct question > > to R&D and will have your doubt answered soon. > > According to R&D if you set "Target Link Speed" to Gen1 before > setting LTSSM_EN bit, the controller should stay in GEN1. I assume that this is the "recommended" and most robust possible approach? And the patch already submitted to ML is 100% correct (so I don't need to clear PCIECTRL_PL_WIDTH_SPEED_CTL) ? Our problem has been described here: https://e2e.ti.com/support/arm/sitara_arm/f/791/p/567936/2081573#2081573 Best regards, Łukasz Majewski > > > > >> > >>> > >>> Lukasz is reseting this bit, in order to avoid the LTSSM to be > >>> executed, which is correct. > >> > >> So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with > >> LTSSM_EN) the state machine returns immediately and leaves link in > >> the Gen1? > >> > >> The pci-dra7 driver always sets LTSSM_EN bit to start link > >> negotiation. > >> > >>> There is another way to prevent this > >>> automatic speed change, which is to set GEN1 speed before link up > >>> which might be difficult in some setups, so Kishon's also right. > >>> > >>> In my opinion Lukasz approach would be the one that might be more > >>> universal and more "secure". > >> > >> The robustness is the key here since there are some devices, which > >> on particular HW must only work with Gen1 speed. When we start > >> LTSSM state machine and hence start negotiation to Gen2, not > >> always the result of LTSSM is correct and device is properly > >> recognized. > >> > >>> > >>> Joao > >>> > >>> > >>>> > >>>>> > >>>>>> > >>>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of > >>>>>> hacking the IP register. > >>>>> > >>>>> From the original patch description: > >>>>> > >>>>> "Add support to force Root Complex to work in GEN1 mode if so > >>>>> desired, but don't force GEN1 mode on any board just yet." > >>>>> > >>>>> Are there any (floating around) patches allowing forcing GEN1 > >>>>> operation on any board (I would like to reuse/port them to my > >>>>> current solution)? > >>>> > >>>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in > >>>> dt with the patch in Bjorn's tree. > >>>> > >>>> Thanks > >>>> Kishon > >>>> > >>> > >> > >> Best regards, > >> > >> Lukasz Majewski > >> > >> -- > >> > >> DENX Software Engineering GmbH, Managing Director: Wolfgang > >> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > >> Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: > >> wd@denx.de > >> > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-17 11:38 ` Lukasz Majewski @ 2017-01-17 11:48 ` Joao Pinto 0 siblings, 0 replies; 17+ messages in thread From: Joao Pinto @ 2017-01-17 11:48 UTC (permalink / raw) To: Lukasz Majewski, Joao Pinto Cc: Kishon Vijay Abraham I, jingoohan1, Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel, Finger, Robert Hello, Às 11:38 AM de 1/17/2017, Lukasz Majewski escreveu: > Hi Joao, > > Thank you for your reply. > >> Às 10:43 AM de 1/17/2017, Joao Pinto escreveu: >>> >>> Hi Lukasz, >>> >>> Às 9:44 PM de 1/16/2017, Lukasz Majewski escreveu: >>>> Hi Joao, >>>> >>>>> >>>>> Hi, >>>>> >>>>> Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: >>>>>> + Joao, Jingoo >>>>>> >>>>>> Hi, >>>>>> >>>>>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >>>>>>> Hi Kishon, >>>>>>> >>>>>>>> Hi Łukasz, >>>>>>>> >>>>>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>>>>>>> Hi Kishon, >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>>>>>>> Some devices (due to e.g. bad PCIe signal integrity) >>>>>>>>>>> require to run with forced GEN1 speed on PCIe bus. >>>>>>>>>>> >>>>>>>>>>> This patch changes the speed explicitly on dra7 based >>>>>>>>>>> devices when proper device tree attribute is defined for >>>>>>>>>>> the PCIe controller. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>>>>>>> >>>>>>>>>> Bjorn has already queued a patch to do the same thing >>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>>>>>>> >>>>>>>>> It seems like Bjorn only modifies CAP registers. >>>>>>>> >>>>>>>> The patch also modifies the LNKCTL2 register. >>>>>>>>> >>>>>>>>> He also needs to change register with 0x080C offset to >>>>>>>>> actually ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>>>>>>> >>>>>>>> This bit is used to initiate speed change (after the link is >>>>>>>> initialized in GEN1). Resetting the bit (like what you have >>>>>>>> done here) prevents speed change. >>>>>>> >>>>>>> This is strange, but e2e advised me to do things as I did in the >>>>>>> patch to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >>>>>>> >>>>>>> Link: >>>>>>> [1] >>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >>>>>>> >>>>>>> Both patches modify 0x5180 007C register to set GEN1 capability >>>>>>> (PCI_EXP_LNKCAP_SLS_2_5GB) >>>>>>> >>>>>>> The problem is with second register (in your patch): >>>>>>> >>>>>>> From SPRUHZ6G TRM: >>>>>>> >>>>>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >>>>>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >>>>>>> description in TRM >>>>>>> >>>>>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same >>>>>>> as default /reset value. >>>>>> >>>>>> The default value is 0x2 (or else none of the cards would have >>>>>> enumerated in GEN2) >>>>>>> >>>>>>> >>>>>>> Could you clarify which way to _force_ PCIe GEN1 operation is >>>>>>> correct? Mine shows differences in lspci output (as posted in >>>>>>> [1]). >>>>>> >>>>>> You'll see the difference even with the patch in Bjorn's tree ;-) >>>>>> >>>>>> I think these are 2 different approaches to keep the link at >>>>>> GEN1. Joao or Jingoo, do you have any suggestion here? >>>>> >>>>> I studied the Databook, >>>> >>>> Could you reveal which databook do you have in mind? Is that the >>>> TRM for AM5728? >>> >>> I checked the Designware PCIe Databook, since it is based on this >>> IP. >>> >>>> >>>>> and both approaches seem to be right, >>>>> dependently of the Core configuration and setup. >>>>> >>>>> The standard manual speed change sequence is: >>>>> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) >>>> >>>> Do you mean TRGT_LINK_SPEED @ 0x5180 00A0 ? >>> >>> Correct. >>> >>>> >>>>> b) Clear "Directed Speed Change" >>>> >>>> CFG_DIRECTED_SPEED_CHANGE @ 0x5180 080C >>> >>> Correct. >>> >>>> >>>>> c) Set "Directed Speed Change" >>>>> >>>>> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is >>>>> the default value), it will execute LTSSM to initiate speed >>>>> change to Gen2 or Gen3, after link is started in Gen1, and then >>>>> the bit is automatically cleared. >>>> >>>> Ok, so with default settings (after reset) we do have Gen1 speed >>>> link and when we enable LTSSM (with LTSSM_EN bit setting) we >>>> negotiate to Gen2/Gen3. >>> >>> Yes, that's the expected behavior. I submited this direct question >>> to R&D and will have your doubt answered soon. >> >> According to R&D if you set "Target Link Speed" to Gen1 before >> setting LTSSM_EN bit, the controller should stay in GEN1. > > I assume that this is the "recommended" and most robust possible > approach? > > And the patch already submitted to ML is 100% correct (so I don't need > to clear PCIECTRL_PL_WIDTH_SPEED_CTL) ? > Yes, according to R&D the approach available in Bjorn' tree should be ok, since it sets GEN1 before enabling LTSSM. Of course this is from a standard designware PCie RC ocre perspective. Joao > Our problem has been described here: > https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_p_567936_2081573-232081573&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=0i9CSBYPpoLydd2AtYg7xapHfGpCvlyir1etY0ZbIwY&s=6f24idB3Pa7aqJEpfsuMpGxkyFUwWwyOFwQtnDztWLA&e= > > Best regards, > Łukasz Majewski > >> >>> >>>> >>>>> >>>>> Lukasz is reseting this bit, in order to avoid the LTSSM to be >>>>> executed, which is correct. >>>> >>>> So with CFG_DIRECTED_SPEED_CHANGE = 0, when I start LTSSM (with >>>> LTSSM_EN) the state machine returns immediately and leaves link in >>>> the Gen1? >>>> >>>> The pci-dra7 driver always sets LTSSM_EN bit to start link >>>> negotiation. >>>> >>>>> There is another way to prevent this >>>>> automatic speed change, which is to set GEN1 speed before link up >>>>> which might be difficult in some setups, so Kishon's also right. >>>>> >>>>> In my opinion Lukasz approach would be the one that might be more >>>>> universal and more "secure". >>>> >>>> The robustness is the key here since there are some devices, which >>>> on particular HW must only work with Gen1 speed. When we start >>>> LTSSM state machine and hence start negotiation to Gen2, not >>>> always the result of LTSSM is correct and device is properly >>>> recognized. >>>> >>>>> >>>>> Joao >>>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of >>>>>>>> hacking the IP register. >>>>>>> >>>>>>> From the original patch description: >>>>>>> >>>>>>> "Add support to force Root Complex to work in GEN1 mode if so >>>>>>> desired, but don't force GEN1 mode on any board just yet." >>>>>>> >>>>>>> Are there any (floating around) patches allowing forcing GEN1 >>>>>>> operation on any board (I would like to reuse/port them to my >>>>>>> current solution)? >>>>>> >>>>>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in >>>>>> dt with the patch in Bjorn's tree. >>>>>> >>>>>> Thanks >>>>>> Kishon >>>>>> >>>>> >>>> >>>> Best regards, >>>> >>>> Lukasz Majewski >>>> >>>> -- >>>> >>>> DENX Software Engineering GmbH, Managing Director: Wolfgang >>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, >>>> Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: >>>> wd@denx.de >>>> >>> >> > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-16 17:01 ` Joao Pinto 2017-01-16 21:44 ` Lukasz Majewski @ 2017-01-17 5:35 ` Kishon Vijay Abraham I 2017-01-17 10:19 ` Joao Pinto 1 sibling, 1 reply; 17+ messages in thread From: Kishon Vijay Abraham I @ 2017-01-17 5:35 UTC (permalink / raw) To: Joao Pinto, Lukasz Majewski, jingoohan1 Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi Joao, On Monday 16 January 2017 10:31 PM, Joao Pinto wrote: > > Hi, > > Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: >> + Joao, Jingoo >> >> Hi, >> >> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >>> Hi Kishon, >>> >>>> Hi Łukasz, >>>> >>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>>> Hi Kishon, >>>>> >>>>>> Hi, >>>>>> >>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>>> run with forced GEN1 speed on PCIe bus. >>>>>>> >>>>>>> This patch changes the speed explicitly on dra7 based devices when >>>>>>> proper device tree attribute is defined for the PCIe controller. >>>>>>> >>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>>> >>>>>> Bjorn has already queued a patch to do the same thing >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>>> >>>>> It seems like Bjorn only modifies CAP registers. >>>> >>>> The patch also modifies the LNKCTL2 register. >>>>> >>>>> He also needs to change register with 0x080C offset to actually >>>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>>> >>>> This bit is used to initiate speed change (after the link is >>>> initialized in GEN1). Resetting the bit (like what you have done >>>> here) prevents speed change. >>> >>> This is strange, but e2e advised me to do things as I did in the patch >>> to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >>> >>> Link: >>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >>> >>> Both patches modify 0x5180 007C register to set GEN1 capability >>> (PCI_EXP_LNKCAP_SLS_2_5GB) >>> >>> The problem is with second register (in your patch): >>> >>> From SPRUHZ6G TRM: >>> >>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >>> description in TRM >>> >>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >>> default /reset value. >> >> The default value is 0x2 (or else none of the cards would have enumerated in GEN2) >>> >>> >>> Could you clarify which way to _force_ PCIe GEN1 operation is correct? >>> Mine shows differences in lspci output (as posted in [1]). >> >> You'll see the difference even with the patch in Bjorn's tree ;-) >> >> I think these are 2 different approaches to keep the link at GEN1. Joao or >> Jingoo, do you have any suggestion here? > > I studied the Databook, and both approaches seem to be right, dependently of the > Core configuration and setup. > > The standard manual speed change sequence is: > a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) > b) Clear "Directed Speed Change" > c) Set "Directed Speed Change" > > If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the default > value), it will execute LTSSM to initiate speed change to Gen2 or Gen3, after > link is started in Gen1, and then the bit is automatically cleared. > > Lukasz is reseting this bit, in order to avoid the LTSSM to be executed, which > is correct. There is another way to prevent this automatic speed change, which > is to set GEN1 speed before link up which might be difficult in some setups, so > Kishon's also right. Just for my understanding, why do you think this will be difficult in some setups? > > In my opinion Lukasz approach would be the one that might be more universal and > more "secure". IMHO setting link control in the standard PCIe header space should be more universal. I'm not sure about the secure part though. Thanks Kishon > > Joao > > >> >>> >>>> >>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking >>>> the IP register. >>> >>> From the original patch description: >>> >>> "Add support to force Root Complex to work in GEN1 mode if so desired, >>> but don't force GEN1 mode on any board just yet." >>> >>> Are there any (floating around) patches allowing forcing GEN1 operation >>> on any board (I would like to reuse/port them to my current solution)? >> >> For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt with the >> patch in Bjorn's tree. >> >> Thanks >> Kishon >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-17 5:35 ` Kishon Vijay Abraham I @ 2017-01-17 10:19 ` Joao Pinto 0 siblings, 0 replies; 17+ messages in thread From: Joao Pinto @ 2017-01-17 10:19 UTC (permalink / raw) To: Kishon Vijay Abraham I, Joao Pinto, Lukasz Majewski, jingoohan1 Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, linux-omap, linux-pci, devicetree, linux-kernel Hi, Às 5:35 AM de 1/17/2017, Kishon Vijay Abraham I escreveu: > Hi Joao, > > On Monday 16 January 2017 10:31 PM, Joao Pinto wrote: >> >> Hi, >> >> Às 10:13 AM de 1/16/2017, Kishon Vijay Abraham I escreveu: >>> + Joao, Jingoo >>> >>> Hi, >>> >>> On Monday 16 January 2017 03:01 PM, Lukasz Majewski wrote: >>>> Hi Kishon, >>>> >>>>> Hi Łukasz, >>>>> >>>>> On Monday 16 January 2017 12:19 PM, Lukasz Majewski wrote: >>>>>> Hi Kishon, >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Sunday 15 January 2017 06:49 PM, Lukasz Majewski wrote: >>>>>>>> Some devices (due to e.g. bad PCIe signal integrity) require to >>>>>>>> run with forced GEN1 speed on PCIe bus. >>>>>>>> >>>>>>>> This patch changes the speed explicitly on dra7 based devices when >>>>>>>> proper device tree attribute is defined for the PCIe controller. >>>>>>>> >>>>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de> >>>>>>> >>>>>>> Bjorn has already queued a patch to do the same thing >>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_cgit_linux_kernel_git_helgaas_pci.git_log_-3Fh-3Dpci_host-2Ddra7xx&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=E8zk1CbKxGH-f3fw_WpXxFU-A8BLkgA8NusCaxk1SvA&e= >>>>>> >>>>>> It seems like Bjorn only modifies CAP registers. >>>>> >>>>> The patch also modifies the LNKCTL2 register. >>>>>> >>>>>> He also needs to change register with 0x080C offset to actually >>>>>> ( PCIECTRL_PL_WIDTH_SPEED_CTL ) >>>>> >>>>> This bit is used to initiate speed change (after the link is >>>>> initialized in GEN1). Resetting the bit (like what you have done >>>>> here) prevents speed change. >>>> >>>> This is strange, but e2e advised me to do things as I did in the patch >>>> to _force_ GEN1 operation on PCIe2 port [1] (AM5728) >>>> >>>> Link: >>>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__e2e.ti.com_support_arm_sitara-5Farm_f_791_t_566421&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=zD82T5n4WcL7Ga-NSY2NI7KE75xQ99hN-mW2yX46wQk&s=uXLwglyRYqKpwp1JSxkOWmKpQ2wjfhgofpm8DCfquNw&e= >>>> >>>> Both patches modify 0x5180 007C register to set GEN1 capability >>>> (PCI_EXP_LNKCAP_SLS_2_5GB) >>>> >>>> The problem is with second register (in your patch): >>>> >>>> From SPRUHZ6G TRM: >>>> >>>> PCIECTRL_EP_DBICS_LNK_CAS_2 (0x5180 00A0) >>>> - TRGT_LINK_SPEED (Reset 0x1) - "Target Link Speed" - no more >>>> description in TRM >>>> >>>> It is set to PCI_EXP_LNKCAP_SLS_2_5GB = 0x1, which is the same as >>>> default /reset value. >>> >>> The default value is 0x2 (or else none of the cards would have enumerated in GEN2) >>>> >>>> >>>> Could you clarify which way to _force_ PCIe GEN1 operation is correct? >>>> Mine shows differences in lspci output (as posted in [1]). >>> >>> You'll see the difference even with the patch in Bjorn's tree ;-) >>> >>> I think these are 2 different approaches to keep the link at GEN1. Joao or >>> Jingoo, do you have any suggestion here? >> >> I studied the Databook, and both approaches seem to be right, dependently of the >> Core configuration and setup. >> >> The standard manual speed change sequence is: >> a) Write to PCIE_CAP_TARGET_LINK_SPEED (indicating desired speed) >> b) Clear "Directed Speed Change" >> c) Set "Directed Speed Change" >> >> If "Directed Speed Change" is set (DEFAULT_GEN2_SPEED_CHANGE is the default >> value), it will execute LTSSM to initiate speed change to Gen2 or Gen3, after >> link is started in Gen1, and then the bit is automatically cleared. >> >> Lukasz is reseting this bit, in order to avoid the LTSSM to be executed, which >> is correct. There is another way to prevent this automatic speed change, which >> is to set GEN1 speed before link up which might be difficult in some setups, so >> Kishon's also right. > > Just for my understanding, why do you think this will be difficult in some setups? It depends on the way some PCI Cores are configured. >> >> In my opinion Lukasz approach would be the one that might be more universal and >> more "secure". > > IMHO setting link control in the standard PCIe header space should be more > universal. I'm not sure about the secure part though. When I said secure, I misleaded you, sorry. I meant more robust. > > Thanks > Kishon >> >> Joao >> >> >>> >>>> >>>>> >>>>> IMO the better way is to set the LNKCTL2 to GEN1 instead of hacking >>>>> the IP register. >>>> >>>> From the original patch description: >>>> >>>> "Add support to force Root Complex to work in GEN1 mode if so desired, >>>> but don't force GEN1 mode on any board just yet." >>>> >>>> Are there any (floating around) patches allowing forcing GEN1 operation >>>> on any board (I would like to reuse/port them to my current solution)? >>> >>> For setting to GEN1 mode, "max-link-speed" should be set to 1 in dt with the >>> patch in Bjorn's tree. >>> >>> Thanks >>> Kishon >>> >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation 2017-01-15 13:19 [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation Lukasz Majewski 2017-01-16 6:37 ` Kishon Vijay Abraham I @ 2017-01-28 20:54 ` Bjorn Helgaas 1 sibling, 0 replies; 17+ messages in thread From: Bjorn Helgaas @ 2017-01-28 20:54 UTC (permalink / raw) To: Lukasz Majewski Cc: Kishon Vijay Abraham I, Bjorn Helgaas, Rob Herring, Mark Rutland, Jingoo Han, Joao Pinto, linux-omap, linux-pci, devicetree, linux-kernel On Sun, Jan 15, 2017 at 02:19:14PM +0100, Lukasz Majewski wrote: > Some devices (due to e.g. bad PCIe signal integrity) require to run > with forced GEN1 speed on PCIe bus. > > This patch changes the speed explicitly on dra7 based devices when > proper device tree attribute is defined for the PCIe controller. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> Hi Lukasz, there was a lot of discussion after this, so I'm going to drop this version and wait for a new one (if one is still needed). I think I saw a URL with more scenario details; please include that if appropriate (ideally the details would be in a permanent public place like the kernel.org bugzilla). > --- > > Patch applies on newest origin/master > SHA1: f4d3935e4f4884ba80561db5549394afb8eef8f7 > > Tested at AM5728 > > --- > Documentation/devicetree/bindings/pci/ti-pci.txt | 1 + > drivers/pci/host/pci-dra7xx.c | 23 +++++++++++++++++++++++ > drivers/pci/host/pcie-designware.h | 1 + > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt > index 60e2516..9f97409 100644 > --- a/Documentation/devicetree/bindings/pci/ti-pci.txt > +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt > @@ -25,6 +25,7 @@ PCIe Designware Controller > > Optional Property: > - gpios : Should be added if a gpio line is required to drive PERST# line > + - to,pcie-is-gen1: Indicates that forced gen1 port operation is needed. > > Example: > axi { > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c > index 9595fad..eec5fae 100644 > --- a/drivers/pci/host/pci-dra7xx.c > +++ b/drivers/pci/host/pci-dra7xx.c > @@ -63,6 +63,13 @@ > #define LINK_UP BIT(16) > #define DRA7XX_CPU_TO_BUS_ADDR 0x0FFFFFFF > > +#define PCIECTRL_EP_DBICS_LNK_CAP 0x007C > +#define MAX_LINK_SPEEDS_MASK GENMASK(3, 0) > +#define MAX_LINK_SPEEDS_GEN1 BIT(0) > + > +#define PCIECTRL_PL_WIDTH_SPEED_CTL 0x080C > +#define CFG_DIRECTED_SPEED_CHANGE BIT(17) > + > struct dra7xx_pcie { > struct pcie_port pp; > void __iomem *base; /* DT ti_conf */ > @@ -270,6 +277,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > struct pcie_port *pp = &dra7xx->pp; > struct device *dev = pp->dev; > struct resource *res; > + u32 val; > > pp->irq = platform_get_irq(pdev, 1); > if (pp->irq < 0) { > @@ -296,6 +304,18 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > if (!pp->dbi_base) > return -ENOMEM; > > + if (pp->is_gen1) { > + dev_info(dev, "GEN1 forced\n"); > + > + val = readl(pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); > + set_mask_bits(&val, MAX_LINK_SPEEDS_MASK, MAX_LINK_SPEEDS_GEN1); > + writel(val, pp->dbi_base + PCIECTRL_EP_DBICS_LNK_CAP); > + > + val = readl(pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); > + val &= ~CFG_DIRECTED_SPEED_CHANGE; > + writel(val, pp->dbi_base + PCIECTRL_PL_WIDTH_SPEED_CTL); > + } > + > ret = dw_pcie_host_init(pp); > if (ret) { > dev_err(dev, "failed to initialize host\n"); > @@ -404,6 +424,9 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) > goto err_gpio; > } > > + if (of_property_read_bool(np, "ti,pcie-is-gen1")) > + pp->is_gen1 = true; > + > reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > reg &= ~LTSSM_EN; > dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_DEVICE_CMD, reg); > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index a567ea2..2fb0b18 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -50,6 +50,7 @@ struct pcie_port { > struct irq_domain *irq_domain; > unsigned long msi_data; > u8 iatu_unroll_enabled; > + u8 is_gen1; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; > > -- > 2.1.4 > > -- > 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] 17+ messages in thread
end of thread, other threads:[~2017-01-28 20:55 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-15 13:19 [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation Lukasz Majewski 2017-01-16 6:37 ` Kishon Vijay Abraham I 2017-01-16 6:49 ` Lukasz Majewski 2017-01-16 8:12 ` Kishon Vijay Abraham I 2017-01-16 9:31 ` Lukasz Majewski 2017-01-16 10:13 ` Kishon Vijay Abraham I 2017-01-16 10:34 ` Lukasz Majewski 2017-01-16 13:40 ` Joao Pinto 2017-01-16 17:01 ` Joao Pinto 2017-01-16 21:44 ` Lukasz Majewski 2017-01-17 10:43 ` Joao Pinto 2017-01-17 11:23 ` Joao Pinto 2017-01-17 11:38 ` Lukasz Majewski 2017-01-17 11:48 ` Joao Pinto 2017-01-17 5:35 ` Kishon Vijay Abraham I 2017-01-17 10:19 ` Joao Pinto 2017-01-28 20:54 ` 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).