[PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Kishon Vijay Abraham I]

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);
>  };
>  
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Lukasz Majewski]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Kishon Vijay Abraham I]

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
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Lukasz Majewski]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Kishon Vijay Abraham I]

+ 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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Lukasz Majewski]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Lukasz Majewski]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Kishon Vijay Abraham I]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Lukasz Majewski]

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

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Joao Pinto]

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
> 

Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

[Bjorn Helgaas]

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