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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <587C8088.80905@ti.com>
Date:   Mon, 16 Jan 2017 13:42:56 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Lukasz Majewski <lukma@...x.de>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jingoo Han <jingoohan1@...il.com>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        <linux-omap@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pcie: ti: Provide patch to force GEN1 PCIe operation

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@...x.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@...x.de
>