[V3,03/12] PCI: tegra: Retrain link for Gen2 speed

Message ID 1509371843-22931-4-git-send-email-mmaddireddy@nvidia.com
State Changes Requested
Headers show
Series
  • Enable Tegra root port features and apply SW fixups
Related show

Commit Message

Manikanta Maddireddy Oct. 30, 2017, 1:57 p.m.
Tegra124, 132, 210 and 186 support Gen2 link speed. After the link is up
in Gen1, set target link speed as Gen2 and retrain link. Link switches to
Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* Corrected commit log
* Replaced jiffies with ktime
V2:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Lorenzo Pieralisi Dec. 12, 2017, 2:32 p.m. | #1
[+Ley Foon Tan]

On Mon, Oct 30, 2017 at 07:27:14PM +0530, Manikanta Maddireddy wrote:
> Tegra124, 132, 210 and 186 support Gen2 link speed. After the link is up
> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * Corrected commit log
> * Replaced jiffies with ktime
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 068510b40c1a..ed5e8acfdc32 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -232,6 +232,8 @@
>  #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
>  
> +#define LINK_RETRAIN_TIMEOUT 100000
> +
>  struct tegra_msi {
>  	struct msi_controller chip;
>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> @@ -2134,6 +2136,42 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  	}
>  }
>  
> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
> +					 struct pci_dev *pci_dev)
> +{
> +	struct device *dev = pcie->dev;
> +	ktime_t deadline;
> +	unsigned short val;

u16

> +	/* Skip if the current device is not a root port */
> +	if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
> +		return;
> +
> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
> +	val &= ~PCI_EXP_LNKSTA_CLS;
> +	val |= PCI_EXP_LNKSTA_CLS_5_0GB;
> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);

Should you not read the Link Capabilities 2 register ("Supported Speed
Vector") before programming the Link control 2 register Target Link
Speed value ?

> +
> +	/* Retrain the link */
> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
> +	val |= PCI_EXP_LNKCTL_RL;
> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
> +
> +	deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> +	for (;;) {
> +		pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &val);
> +		if (!(val & PCI_EXP_LNKSTA_LT))
> +			break;
> +		if (ktime_after(ktime_get(), deadline))
> +			break;
> +		usleep_range(2000, 3000);

Ok - I hope we won't end up with every host bridge re-writing its own
link training loop because at that point in time we should think about
consolidating this.

CC'ing Ley Foon Tan since I would like to understand why the Altera
driver link retraining can't be written with the same code as this
driver - I suspect it has to do with the retraining sequence and when
the retraining is actually carried out in the host bridge probe
sequence.

> +	}
> +
> +	if (val & PCI_EXP_LNKSTA_LT)
> +		dev_err(dev, "link retrain of PCIe slot %u failed\n",
> +			PCI_SLOT(pci_dev->devfn));
> +}
> +
>  static const struct tegra_pcie_soc tegra20_pcie = {
>  	.num_ports = 2,
>  	.msi_base_shift = 0,
> @@ -2335,6 +2373,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	struct pci_host_bridge *host;
>  	struct tegra_pcie *pcie;
>  	struct pci_bus *child;
> +	struct pci_dev *pci_dev = NULL;
>  	int err;
>  
>  	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> @@ -2400,6 +2439,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  
>  	pci_bus_add_devices(host->bus);
>  
> +	for_each_pci_dev(pci_dev)
> +		tegra_pcie_change_link_speed(pcie, pci_dev);
> +

Are you sure it is safe to change link speed after adding devices ?

Lorenzo

>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>  		err = tegra_pcie_debugfs_init(pcie);
>  		if (err < 0)
> -- 
> 2.1.4
>
Manikanta Maddireddy Dec. 13, 2017, 5:54 p.m. | #2
On 12-Dec-17 8:02 PM, Lorenzo Pieralisi wrote:
> [+Ley Foon Tan]
> 
> On Mon, Oct 30, 2017 at 07:27:14PM +0530, Manikanta Maddireddy wrote:
>> Tegra124, 132, 210 and 186 support Gen2 link speed. After the link is up
>> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
>> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>> V3:
>> * Corrected commit log
>> * Replaced jiffies with ktime
>> V2:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 068510b40c1a..ed5e8acfdc32 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -232,6 +232,8 @@
>>  #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
>>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
>>  
>> +#define LINK_RETRAIN_TIMEOUT 100000
>> +
>>  struct tegra_msi {
>>  	struct msi_controller chip;
>>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
>> @@ -2134,6 +2136,42 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>  	}
>>  }
>>  
>> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
>> +					 struct pci_dev *pci_dev)
>> +{
>> +	struct device *dev = pcie->dev;
>> +	ktime_t deadline;
>> +	unsigned short val;
> 
> u16
> 
>> +	/* Skip if the current device is not a root port */
>> +	if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
>> +		return;
>> +
>> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
>> +	val &= ~PCI_EXP_LNKSTA_CLS;
>> +	val |= PCI_EXP_LNKSTA_CLS_5_0GB;
>> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);
> 
> Should you not read the Link Capabilities 2 register ("Supported Speed
> Vector") before programming the Link control 2 register Target Link
> Speed value ?
> 
Link Capabilities 2 register is hardwired to 0 and not used in Tegra.
This information is documented in Tegra TRM.

>> +
>> +	/* Retrain the link */
>> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
>> +	val |= PCI_EXP_LNKCTL_RL;
>> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
>> +
>> +	deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
>> +	for (;;) {
>> +		pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &val);
>> +		if (!(val & PCI_EXP_LNKSTA_LT))
>> +			break;
>> +		if (ktime_after(ktime_get(), deadline))
>> +			break;
>> +		usleep_range(2000, 3000);
> 
> Ok - I hope we won't end up with every host bridge re-writing its own
> link training loop because at that point in time we should think about
> consolidating this.
> 
Are you saying that we need to add common link retrain function in
pci core driver and reuse it in all host drivers?

> CC'ing Ley Foon Tan since I would like to understand why the Altera
> driver link retraining can't be written with the same code as this
> driver - I suspect it has to do with the retraining sequence and when
> the retraining is actually carried out in the host bridge probe
> sequence.
>
>> +	}
>> +
>> +	if (val & PCI_EXP_LNKSTA_LT)
>> +		dev_err(dev, "link retrain of PCIe slot %u failed\n",
>> +			PCI_SLOT(pci_dev->devfn));
>> +}
>> +
>>  static const struct tegra_pcie_soc tegra20_pcie = {
>>  	.num_ports = 2,
>>  	.msi_base_shift = 0,
>> @@ -2335,6 +2373,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>  	struct pci_host_bridge *host;
>>  	struct tegra_pcie *pcie;
>>  	struct pci_bus *child;
>> +	struct pci_dev *pci_dev = NULL;
>>  	int err;
>>  
>>  	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
>> @@ -2400,6 +2439,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>>  
>>  	pci_bus_add_devices(host->bus);
>>  
>> +	for_each_pci_dev(pci_dev)
>> +		tegra_pcie_change_link_speed(pcie, pci_dev);
>> +
> 
> Are you sure it is safe to change link speed after adding devices ?
> 
> Lorenzo
I tried to do link retrain right after 'linkup in Gen1' i.e before pci_bus_add_devices(),
but it taking more time than timeout(100 msec) I added in tegra_pcie_change_link_speed().
So I moved it here to have minimum delay for retraining link. I didn't see any issue
here, link speed is moving to Gen2 without any issue. Do you want me look into anything
particular here?
> 
>>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>>  		err = tegra_pcie_debugfs_init(pcie);
>>  		if (err < 0)
>> -- 
>> 2.1.4
>>
Lorenzo Pieralisi Dec. 13, 2017, 6:51 p.m. | #3
On Wed, Dec 13, 2017 at 11:24:03PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 12-Dec-17 8:02 PM, Lorenzo Pieralisi wrote:
> > [+Ley Foon Tan]
> > 
> > On Mon, Oct 30, 2017 at 07:27:14PM +0530, Manikanta Maddireddy wrote:
> >> Tegra124, 132, 210 and 186 support Gen2 link speed. After the link is up
> >> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
> >> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V3:
> >> * Corrected commit log
> >> * Replaced jiffies with ktime
> >> V2:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index 068510b40c1a..ed5e8acfdc32 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -232,6 +232,8 @@
> >>  #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
> >>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
> >>  
> >> +#define LINK_RETRAIN_TIMEOUT 100000
> >> +
> >>  struct tegra_msi {
> >>  	struct msi_controller chip;
> >>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> >> @@ -2134,6 +2136,42 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
> >>  	}
> >>  }
> >>  
> >> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
> >> +					 struct pci_dev *pci_dev)
> >> +{
> >> +	struct device *dev = pcie->dev;
> >> +	ktime_t deadline;
> >> +	unsigned short val;
> > 
> > u16
> > 
> >> +	/* Skip if the current device is not a root port */
> >> +	if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
> >> +		return;
> >> +
> >> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
> >> +	val &= ~PCI_EXP_LNKSTA_CLS;
> >> +	val |= PCI_EXP_LNKSTA_CLS_5_0GB;
> >> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);
> > 
> > Should you not read the Link Capabilities 2 register ("Supported Speed
> > Vector") before programming the Link control 2 register Target Link
> > Speed value ?
> > 
> Link Capabilities 2 register is hardwired to 0 and not used in Tegra.
> This information is documented in Tegra TRM.

You should add a comment to explain that then.

> >> +	/* Retrain the link */
> >> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
> >> +	val |= PCI_EXP_LNKCTL_RL;
> >> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
> >> +
> >> +	deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> >> +	for (;;) {
> >> +		pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &val);
> >> +		if (!(val & PCI_EXP_LNKSTA_LT))
> >> +			break;
> >> +		if (ktime_after(ktime_get(), deadline))
> >> +			break;
> >> +		usleep_range(2000, 3000);
> > 
> > Ok - I hope we won't end up with every host bridge re-writing its own
> > link training loop because at that point in time we should think about
> > consolidating this.
> > 
> Are you saying that we need to add common link retrain function in
> pci core driver and reuse it in all host drivers?

We do not need to, we should aim for it though. There is nothing
(well - I wish) platform specific in what you are doing.

> > CC'ing Ley Foon Tan since I would like to understand why the Altera
> > driver link retraining can't be written with the same code as this
> > driver - I suspect it has to do with the retraining sequence and when
> > the retraining is actually carried out in the host bridge probe
> > sequence.
> >
> >> +	}
> >> +
> >> +	if (val & PCI_EXP_LNKSTA_LT)
> >> +		dev_err(dev, "link retrain of PCIe slot %u failed\n",
> >> +			PCI_SLOT(pci_dev->devfn));
> >> +}
> >> +
> >>  static const struct tegra_pcie_soc tegra20_pcie = {
> >>  	.num_ports = 2,
> >>  	.msi_base_shift = 0,
> >> @@ -2335,6 +2373,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
> >>  	struct pci_host_bridge *host;
> >>  	struct tegra_pcie *pcie;
> >>  	struct pci_bus *child;
> >> +	struct pci_dev *pci_dev = NULL;
> >>  	int err;
> >>  
> >>  	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> >> @@ -2400,6 +2439,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
> >>  
> >>  	pci_bus_add_devices(host->bus);
> >>  
> >> +	for_each_pci_dev(pci_dev)
> >> +		tegra_pcie_change_link_speed(pcie, pci_dev);
> >> +
> > 
> > Are you sure it is safe to change link speed after adding devices ?
> > 
> > Lorenzo
> I tried to do link retrain right after 'linkup in Gen1' i.e before pci_bus_add_devices(),
> but it taking more time than timeout(100 msec) I added in tegra_pcie_change_link_speed().

You should not try, you should understand the reason behind it.

> So I moved it here to have minimum delay for retraining link. I didn't see any issue
> here, link speed is moving to Gen2 without any issue. Do you want me look into anything
> particular here?

At that point you can have devices matched to drivers - I do not think
that's correct to carry out the retraining after devices are added even
if it may work for you.

Yes, I want you please to look into the reasons that bring to the timeout
and the consequent retraining code placement in the probe path.

Thanks,
Lorenzo
Bjorn Helgaas Dec. 13, 2017, 7:10 p.m. | #4
On Wed, Dec 13, 2017 at 11:24:03PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 12-Dec-17 8:02 PM, Lorenzo Pieralisi wrote:
> > [+Ley Foon Tan]
> > 
> > On Mon, Oct 30, 2017 at 07:27:14PM +0530, Manikanta Maddireddy wrote:
> >> Tegra124, 132, 210 and 186 support Gen2 link speed. After the link is up
> >> in Gen1, set target link speed as Gen2 and retrain link. Link switches to
> >> Gen2 speed if Gen2 capable end point is connected, else link stays in Gen1.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V3:
> >> * Corrected commit log
> >> * Replaced jiffies with ktime
> >> V2:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index 068510b40c1a..ed5e8acfdc32 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -232,6 +232,8 @@
> >>  #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
> >>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
> >>  
> >> +#define LINK_RETRAIN_TIMEOUT 100000
> >> +
> >>  struct tegra_msi {
> >>  	struct msi_controller chip;
> >>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> >> @@ -2134,6 +2136,42 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
> >>  	}
> >>  }
> >>  
> >> +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
> >> +					 struct pci_dev *pci_dev)
> >> +{
> >> +	struct device *dev = pcie->dev;
> >> +	ktime_t deadline;
> >> +	unsigned short val;
> > 
> > u16
> > 
> >> +	/* Skip if the current device is not a root port */
> >> +	if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
> >> +		return;
> >> +
> >> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
> >> +	val &= ~PCI_EXP_LNKSTA_CLS;
> >> +	val |= PCI_EXP_LNKSTA_CLS_5_0GB;
> >> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);
> > 
> > Should you not read the Link Capabilities 2 register ("Supported Speed
> > Vector") before programming the Link control 2 register Target Link
> > Speed value ?
> > 
> Link Capabilities 2 register is hardwired to 0 and not used in Tegra.
> This information is documented in Tegra TRM.
> 
> >> +
> >> +	/* Retrain the link */
> >> +	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
> >> +	val |= PCI_EXP_LNKCTL_RL;
> >> +	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
> >> +
> >> +	deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> >> +	for (;;) {
> >> +		pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &val);
> >> +		if (!(val & PCI_EXP_LNKSTA_LT))
> >> +			break;
> >> +		if (ktime_after(ktime_get(), deadline))
> >> +			break;
> >> +		usleep_range(2000, 3000);
> > 
> > Ok - I hope we won't end up with every host bridge re-writing its own
> > link training loop because at that point in time we should think about
> > consolidating this.
> > 
> Are you saying that we need to add common link retrain function in
> pci core driver and reuse it in all host drivers?
> 
> > CC'ing Ley Foon Tan since I would like to understand why the Altera
> > driver link retraining can't be written with the same code as this
> > driver - I suspect it has to do with the retraining sequence and when
> > the retraining is actually carried out in the host bridge probe
> > sequence.
> >
> >> +	}
> >> +
> >> +	if (val & PCI_EXP_LNKSTA_LT)
> >> +		dev_err(dev, "link retrain of PCIe slot %u failed\n",
> >> +			PCI_SLOT(pci_dev->devfn));
> >> +}
> >> +
> >>  static const struct tegra_pcie_soc tegra20_pcie = {
> >>  	.num_ports = 2,
> >>  	.msi_base_shift = 0,
> >> @@ -2335,6 +2373,7 @@ static int tegra_pcie_probe(struct platform_device *pdev)
> >>  	struct pci_host_bridge *host;
> >>  	struct tegra_pcie *pcie;
> >>  	struct pci_bus *child;
> >> +	struct pci_dev *pci_dev = NULL;
> >>  	int err;
> >>  
> >>  	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> >> @@ -2400,6 +2439,9 @@ static int tegra_pcie_probe(struct platform_device *pdev)
> >>  
> >>  	pci_bus_add_devices(host->bus);
> >>  
> >> +	for_each_pci_dev(pci_dev)
> >> +		tegra_pcie_change_link_speed(pcie, pci_dev);
> >> +
> > 
> > Are you sure it is safe to change link speed after adding devices ?

> I tried to do link retrain right after 'linkup in Gen1' i.e before
> pci_bus_add_devices(), but it taking more time than timeout(100
> msec) I added in tegra_pcie_change_link_speed().  So I moved it here
> to have minimum delay for retraining link. I didn't see any issue
> here, link speed is moving to Gen2 without any issue. Do you want me
> look into anything particular here?

pci_bus_add_devices() is basically the end of the line as far as the
PCI core is concerned because pci_bus_add_devices() is the point at
which drivers can claim the device.  After that, the core must not do
anything that might interfere with the driver.

You might be able to change the link speed while the driver is
operating the device, but I think it's bad form.

Plus this call doesn't do anything for hot-added devices.  I can see
that tegra_pcie_change_link_speed() only touches Root Ports, and maybe
you don't support hot-add for them, but it just doesn't fit the model.

Bjorn
Ley Foon Tan Dec. 21, 2017, 7:48 p.m. | #5
On Tue, 2017-12-12 at 14:32 +0000, Lorenzo Pieralisi wrote:
> [+Ley Foon Tan]
> 
> On Mon, Oct 30, 2017 at 07:27:14PM +0530, Manikanta Maddireddy wrote:
> > 
> > Tegra124, 132, 210 and 186 support Gen2 link speed. After the link
> > is up
> > in Gen1, set target link speed as Gen2 and retrain link. Link
> > switches to
> > Gen2 speed if Gen2 capable end point is connected, else link stays
> > in Gen1.
> > 
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > ---
> > V3:
> > * Corrected commit log
> > * Replaced jiffies with ktime
> > V2:
> > * no change in this patch
> > 
> >  drivers/pci/host/pci-tegra.c | 42
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-
> > tegra.c
> > index 068510b40c1a..ed5e8acfdc32 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -232,6 +232,8 @@
> >  #define PADS_REFCLK_CFG_PREDI_SHIFT          8  /* 11:8 */
> >  #define PADS_REFCLK_CFG_DRVI_SHIFT           12 /* 15:12 */
> > 
> > +#define LINK_RETRAIN_TIMEOUT 100000
> > +
> >  struct tegra_msi {
> >       struct msi_controller chip;
> >       DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> > @@ -2134,6 +2136,42 @@ static void tegra_pcie_enable_ports(struct
> > tegra_pcie *pcie)
> >       }
> >  }
> > 
> > +static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
> > +                                      struct pci_dev *pci_dev)
> > +{
> > +     struct device *dev = pcie->dev;
> > +     ktime_t deadline;
> > +     unsigned short val;
> u16
> 
> > 
> > +     /* Skip if the current device is not a root port */
> > +     if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
> > +             return;
> > +
> > +     pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
> > +     val &= ~PCI_EXP_LNKSTA_CLS;
> > +     val |= PCI_EXP_LNKSTA_CLS_5_0GB;
> > +     pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);
> Should you not read the Link Capabilities 2 register ("Supported
> Speed
> Vector") before programming the Link control 2 register Target Link
> Speed value ?
> 
> > 
> > +
> > +     /* Retrain the link */
> > +     pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
> > +     val |= PCI_EXP_LNKCTL_RL;
> > +     pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
> > +
> > +     deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
> > +     for (;;) {
> > +             pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA,
> > &val);
> > +             if (!(val & PCI_EXP_LNKSTA_LT))
> > +                     break;
> > +             if (ktime_after(ktime_get(), deadline))
> > +                     break;
> > +             usleep_range(2000, 3000);
> Ok - I hope we won't end up with every host bridge re-writing its own
> link training loop because at that point in time we should think
> about
> consolidating this.
> 
> CC'ing Ley Foon Tan since I would like to understand why the Altera
> driver link retraining can't be written with the same code as this
> driver - I suspect it has to do with the retraining sequence and when
> the retraining is actually carried out in the host bridge probe
> sequence.
Yes, our hardware requires to poll the LT bit in link status register
&and polling LTSSM status after setting retrain bit link control
register.
> 
> > 
> > +     }
> > +
> > +     if (val & PCI_EXP_LNKSTA_LT)
> > +             dev_err(dev, "link retrain of PCIe slot %u failed\n",
> > +                     PCI_SLOT(pci_dev->devfn));
> > +}
> > +
> >  static const struct tegra_pcie_soc tegra20_pcie = {
> >       .num_ports = 2,
> >       .msi_base_shift = 0,
> > @@ -2335,6 +2373,7 @@ static int tegra_pcie_probe(struct
> > platform_device *pdev)
> >       struct pci_host_bridge *host;
> >       struct tegra_pcie *pcie;
> >       struct pci_bus *child;
> > +     struct pci_dev *pci_dev = NULL;
> >       int err;
> > 
> >       host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> > @@ -2400,6 +2439,9 @@ static int tegra_pcie_probe(struct
> > platform_device *pdev)
> > 
> >       pci_bus_add_devices(host->bus);
> > 
> > +     for_each_pci_dev(pci_dev)
> > +             tegra_pcie_change_link_speed(pcie, pci_dev);
> > +
> Are you sure it is safe to change link speed after adding devices ?
> 
> Lorenzo

"for_each_pci_dev(pci_dev)" will lookup all the PCIe devices in system.
What happen if your system have more than one Tegra PCIe Rootport? Will
it retrains all rootport? And same rootport will retrain for 2 times if
there are 2 rootports?
> 
> > 
> >       if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> >               err = tegra_pcie_debugfs_init(pcie);
> >               if (err < 0)
> > --
> > 2.1.4
> >

Patch

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 068510b40c1a..ed5e8acfdc32 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -232,6 +232,8 @@ 
 #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
 #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
 
+#define LINK_RETRAIN_TIMEOUT 100000
+
 struct tegra_msi {
 	struct msi_controller chip;
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
@@ -2134,6 +2136,42 @@  static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 	}
 }
 
+static void tegra_pcie_change_link_speed(struct tegra_pcie *pcie,
+					 struct pci_dev *pci_dev)
+{
+	struct device *dev = pcie->dev;
+	ktime_t deadline;
+	unsigned short val;
+
+	/* Skip if the current device is not a root port */
+	if (pci_pcie_type(pci_dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return;
+
+	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL2, &val);
+	val &= ~PCI_EXP_LNKSTA_CLS;
+	val |= PCI_EXP_LNKSTA_CLS_5_0GB;
+	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL2, val);
+
+	/* Retrain the link */
+	pcie_capability_read_word(pci_dev, PCI_EXP_LNKCTL, &val);
+	val |= PCI_EXP_LNKCTL_RL;
+	pcie_capability_write_word(pci_dev, PCI_EXP_LNKCTL, val);
+
+	deadline = ktime_add_us(ktime_get(), LINK_RETRAIN_TIMEOUT);
+	for (;;) {
+		pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &val);
+		if (!(val & PCI_EXP_LNKSTA_LT))
+			break;
+		if (ktime_after(ktime_get(), deadline))
+			break;
+		usleep_range(2000, 3000);
+	}
+
+	if (val & PCI_EXP_LNKSTA_LT)
+		dev_err(dev, "link retrain of PCIe slot %u failed\n",
+			PCI_SLOT(pci_dev->devfn));
+}
+
 static const struct tegra_pcie_soc tegra20_pcie = {
 	.num_ports = 2,
 	.msi_base_shift = 0,
@@ -2335,6 +2373,7 @@  static int tegra_pcie_probe(struct platform_device *pdev)
 	struct pci_host_bridge *host;
 	struct tegra_pcie *pcie;
 	struct pci_bus *child;
+	struct pci_dev *pci_dev = NULL;
 	int err;
 
 	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
@@ -2400,6 +2439,9 @@  static int tegra_pcie_probe(struct platform_device *pdev)
 
 	pci_bus_add_devices(host->bus);
 
+	for_each_pci_dev(pci_dev)
+		tegra_pcie_change_link_speed(pcie, pci_dev);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_pcie_debugfs_init(pcie);
 		if (err < 0)