Patchwork [14/30] PCI: tegra: Set target speed as Gen1 before link up

login
register
mail settings
Submitter Manikanta Maddireddy
Date April 11, 2019, 5:03 p.m.
Message ID <20190411170355.6882-15-mmaddireddy@nvidia.com>
Download mbox | patch
Permalink /patch/770839/
State New
Headers show

Comments

Manikanta Maddireddy - April 11, 2019, 5:03 p.m.
Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
initially advertise only Gen-1 and after link is up, retrain link to Gen-2
speed.

Following two cards display this behaviour,
  - Fusion HDTV 5 Express card
  - IOGear SIL - PCIE - SATA card

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
 drivers/pci/controller/pci-tegra.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
Bjorn Helgaas - April 11, 2019, 8:04 p.m.
On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
> speed.
> 
> Following two cards display this behaviour,
>   - Fusion HDTV 5 Express card
>   - IOGear SIL - PCIE - SATA card

This sounds like a Tegra erratum.  If you think this is an issue with
the endpoints above, not with Tegra, we should see issues with these
cards in non-Tegra systems.

If that's the case, we might need a more far-reaching solution that
would fix issues with these cards in all systems.

If it really is a Tegra erratum, that's fine; just own up to it in the
commit log and comment so it's not misleading.

> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 7dc728cc5f51..7e24eac12668 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  		value |= soc->update_fc_val;
>  		writel(value, port->base + RP_VEND_XP);
>  	}
> +
> +	/*
> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
> +	 * Hence, the strategy followed here is to initially advertise
> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
> +	 */
> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
> +	value &= ~PCI_EXP_LNKSTA_CLS;
> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
>  }
>  
>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
> -- 
> 2.17.1
>
Manikanta Maddireddy - April 12, 2019, 6:44 a.m.
On 12-Apr-19 1:34 AM, Bjorn Helgaas wrote:
> On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
>> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
>> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
>> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
>> speed.
>>
>> Following two cards display this behaviour,
>>   - Fusion HDTV 5 Express card
>>   - IOGear SIL - PCIE - SATA card
> This sounds like a Tegra erratum.  If you think this is an issue with
> the endpoints above, not with Tegra, we should see issues with these
> cards in non-Tegra systems.
>
> If that's the case, we might need a more far-reaching solution that
> would fix issues with these cards in all systems.
>
> If it really is a Tegra erratum, that's fine; just own up to it in the
> commit log and comment so it's not misleading.

Based on PCIe LA traces with x86 platform:
 1) x86 initially advertises Gen1, Gen2 & Gen3 speeds. Link number negotiation does not happen
 as end point does not lock down to the advertised link number from RP.
 2) There are multiple entries to detect<->compliance from the both the sides.
 3) After some time (or some counts of detect entry), x86 only advertises Gen1 speed in TS1
 4) This time end point responds to the link number in the TS1 and link comes up

Tegra PCIe IP fails after step-2, it doesn't retry with only Gen1. This is the reason for

setting link speed to Gen1 and then start LTSSM.

I don't see anything mentioned about advertising lower link speed and retrying

link up in "Configuration Substate Machine" figure in PCIe spec. I am not sure

if it is mentioned anywhere in implementation notes or left for implementer to

decide in PCIe spec.

I will update this information in next version of this patch to justify why this is required

only for Tegra.

>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>> index 7dc728cc5f51..7e24eac12668 100644
>> --- a/drivers/pci/controller/pci-tegra.c
>> +++ b/drivers/pci/controller/pci-tegra.c
>> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>>  		value |= soc->update_fc_val;
>>  		writel(value, port->base + RP_VEND_XP);
>>  	}
>> +
>> +	/*
>> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
>> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
>> +	 * Hence, the strategy followed here is to initially advertise
>> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
>> +	 */
>> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
>> +	value &= ~PCI_EXP_LNKSTA_CLS;
>> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
>> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
>>  }
>>  
>>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
>> -- 
>> 2.17.1
>>
Bjorn Helgaas - April 12, 2019, 2:35 p.m.
On Fri, Apr 12, 2019 at 12:14:20PM +0530, Manikanta Maddireddy wrote:
> On 12-Apr-19 1:34 AM, Bjorn Helgaas wrote:
> > On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
> >> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
> >> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
> >> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
> >> speed.
> >>
> >> Following two cards display this behaviour,
> >>   - Fusion HDTV 5 Express card
> >>   - IOGear SIL - PCIE - SATA card
> > This sounds like a Tegra erratum.  If you think this is an issue with
> > the endpoints above, not with Tegra, we should see issues with these
> > cards in non-Tegra systems.
> >
> > If that's the case, we might need a more far-reaching solution that
> > would fix issues with these cards in all systems.
> >
> > If it really is a Tegra erratum, that's fine; just own up to it in the
> > commit log and comment so it's not misleading.
> 
> Based on PCIe LA traces with x86 platform:
>  1) x86 initially advertises Gen1, Gen2 & Gen3 speeds. Link number negotiation does not happen
>  as end point does not lock down to the advertised link number from RP.
>  2) There are multiple entries to detect<->compliance from the both the sides.
>  3) After some time (or some counts of detect entry), x86 only advertises Gen1 speed in TS1
>  4) This time end point responds to the link number in the TS1 and link comes up
> 
> Tegra PCIe IP fails after step-2, it doesn't retry with only Gen1.
> This is the reason for setting link speed to Gen1 and then start
> LTSSM.
> 
> I don't see anything mentioned about advertising lower link speed
> and retrying link up in "Configuration Substate Machine" figure in
> PCIe spec. I am not sure if it is mentioned anywhere in
> implementation notes or left for implementer to decide in PCIe spec.

There's a note in PCIe r4.0, sec 1.2 that says

  Initialization – During hardware initialization, each PCI Express
  Link is set up following a negotiation of Lane widths and frequency
  of operation by the two agents at each end of the Link. No firmware
  or operating system software is involved.

I interpret that as meaning the hardware at each end of the link
should be able to initialize the link without any help from software.

I think the hardware pretty much *has* to be able to do that.
Consider the native PCIe hotplug case: the OS has the generic pciehp
driver, but no knowledge of any device-specific behavior of the
Downstream Port leading to a hot-added device.  I don't *think*
there's anything in the spec about software having to be involved in
initializing the link in that case (correct me if I'm wrong), so the
link has to come up correctly without any OS or firmware intervention.

I'd argue that the same applies to this Tegra root port situation.  If
some issue requires a workaround to bring the link up, you do have the
opportunity to apply the fixup at probe-time.  But if you ever hope to
support hotplug for the root port, I think you'll have trouble because
I don't think there's a hook that lets you apply this fixup at hot-add
time.

> I will update this information in next version of this patch to
> justify why this is required only for Tegra.

> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> >> index 7dc728cc5f51..7e24eac12668 100644
> >> --- a/drivers/pci/controller/pci-tegra.c
> >> +++ b/drivers/pci/controller/pci-tegra.c
> >> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
> >>  		value |= soc->update_fc_val;
> >>  		writel(value, port->base + RP_VEND_XP);
> >>  	}
> >> +
> >> +	/*
> >> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
> >> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
> >> +	 * Hence, the strategy followed here is to initially advertise
> >> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
> >> +	 */
> >> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
> >> +	value &= ~PCI_EXP_LNKSTA_CLS;
> >> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
> >> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
> >>  }
> >>  
> >>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
> >> -- 
> >> 2.17.1
> >>
Manikanta Maddireddy - April 15, 2019, 10:43 a.m.
On 12-Apr-19 8:05 PM, Bjorn Helgaas wrote:
> On Fri, Apr 12, 2019 at 12:14:20PM +0530, Manikanta Maddireddy wrote:
>> On 12-Apr-19 1:34 AM, Bjorn Helgaas wrote:
>>> On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
>>>> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
>>>> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
>>>> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
>>>> speed.
>>>>
>>>> Following two cards display this behaviour,
>>>>   - Fusion HDTV 5 Express card
>>>>   - IOGear SIL - PCIE - SATA card
>>> This sounds like a Tegra erratum.  If you think this is an issue with
>>> the endpoints above, not with Tegra, we should see issues with these
>>> cards in non-Tegra systems.
>>>
>>> If that's the case, we might need a more far-reaching solution that
>>> would fix issues with these cards in all systems.
>>>
>>> If it really is a Tegra erratum, that's fine; just own up to it in the
>>> commit log and comment so it's not misleading.
>> Based on PCIe LA traces with x86 platform:
>>  1) x86 initially advertises Gen1, Gen2 & Gen3 speeds. Link number negotiation does not happen
>>  as end point does not lock down to the advertised link number from RP.
>>  2) There are multiple entries to detect<->compliance from the both the sides.
>>  3) After some time (or some counts of detect entry), x86 only advertises Gen1 speed in TS1
>>  4) This time end point responds to the link number in the TS1 and link comes up
>>
>> Tegra PCIe IP fails after step-2, it doesn't retry with only Gen1.
>> This is the reason for setting link speed to Gen1 and then start
>> LTSSM.
>>
>> I don't see anything mentioned about advertising lower link speed
>> and retrying link up in "Configuration Substate Machine" figure in
>> PCIe spec. I am not sure if it is mentioned anywhere in
>> implementation notes or left for implementer to decide in PCIe spec.
> There's a note in PCIe r4.0, sec 1.2 that says
>
>   Initialization – During hardware initialization, each PCI Express
>   Link is set up following a negotiation of Lane widths and frequency
>   of operation by the two agents at each end of the Link. No firmware
>   or operating system software is involved.
>
> I interpret that as meaning the hardware at each end of the link
> should be able to initialize the link without any help from software.
>
> I think the hardware pretty much *has* to be able to do that.
> Consider the native PCIe hotplug case: the OS has the generic pciehp
> driver, but no knowledge of any device-specific behavior of the
> Downstream Port leading to a hot-added device.  I don't *think*
> there's anything in the spec about software having to be involved in
> initializing the link in that case (correct me if I'm wrong), so the
> link has to come up correctly without any OS or firmware intervention.
>
> I'd argue that the same applies to this Tegra root port situation.  If
> some issue requires a workaround to bring the link up, you do have the
> opportunity to apply the fixup at probe-time.  But if you ever hope to
> support hotplug for the root port, I think you'll have trouble because
> I don't think there's a hook that lets you apply this fixup at hot-add
> time.

When you said "link initialization should happen without SW/firmware

intervention", it is after LTSSM is started, right? This patch programs

the target link speed before LTSSM is started.

As mentioned above these cards works on x86 because PCIe IP tires

link up with only Gen1 advertisement after "link number" negotiation fails.

Where as Tegra PCIe IP doesn't try with only Gen1 advertisement. i.e

 1) After LTSSM is initiated by driver, Tegra advertises Gen1 & Gen2
speeds. Link number negotiation does not happen.
 as end point does not lock down to the advertised link number from RP.
 2) LTSSM state moves to disabled.

LTSSM sequence with current patch,
 1) After LTSSM is initiated by driver, Tegra advertises Gen1 speeds.
 2) This time end point responds to the link number in the TS1 and link
comes up

Setting target speed Gen1 is OK because HW autonomous speed change support

is not available in Tegra. After link up driver retrains the link to Gen2 after setting

Target speed.


Tegra doesn't support hot-plug, so no need to worry about this scenario

in hot-plug case.

>
>> I will update this information in next version of this patch to
>> justify why this is required only for Tegra.
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>>>> index 7dc728cc5f51..7e24eac12668 100644
>>>> --- a/drivers/pci/controller/pci-tegra.c
>>>> +++ b/drivers/pci/controller/pci-tegra.c
>>>> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>>>>  		value |= soc->update_fc_val;
>>>>  		writel(value, port->base + RP_VEND_XP);
>>>>  	}
>>>> +
>>>> +	/*
>>>> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
>>>> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
>>>> +	 * Hence, the strategy followed here is to initially advertise
>>>> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
>>>> +	 */
>>>> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
>>>> +	value &= ~PCI_EXP_LNKSTA_CLS;
>>>> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
>>>> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
>>>>  }
>>>>  
>>>>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
>>>> -- 
>>>> 2.17.1
>>>>
Thierry Reding - April 15, 2019, 11:52 a.m.
On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
> speed.
> 
> Following two cards display this behaviour,
>   - Fusion HDTV 5 Express card
>   - IOGear SIL - PCIE - SATA card
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 7dc728cc5f51..7e24eac12668 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  		value |= soc->update_fc_val;
>  		writel(value, port->base + RP_VEND_XP);
>  	}
> +
> +	/*
> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
> +	 * Hence, the strategy followed here is to initially advertise
> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
> +	 */
> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
> +	value &= ~PCI_EXP_LNKSTA_CLS;
> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
>  }
>  
>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)

This looks like it's related to the earlier patch that adds support for
retraining the link at Gen-2. As such, I think the two patches should be
moved closer together to make that more obvious.

Also, perhaps even the order needs to be changed. For example, if the
earlier patch enables advertisement of Gen-2, then there will be a
period of 10 or so patches where the above devices wouldn't work. So if
this fixes an error introduced by an earlier patch, it makes sense to
resort the patches so that we first fix the potential error and then
introduce the code that would cause the error to happen.

Thierry
Manikanta Maddireddy - April 15, 2019, 3:12 p.m.
On 15-Apr-19 5:22 PM, Thierry Reding wrote:
> On Thu, Apr 11, 2019 at 10:33:39PM +0530, Manikanta Maddireddy wrote:
>> Some of the legacy PCIe endpoints doesn't enumerate if root port advertises
>> both Gen-1 and Gen-2 speeds. Hence, the strategy followed here is to
>> initially advertise only Gen-1 and after link is up, retrain link to Gen-2
>> speed.
>>
>> Following two cards display this behaviour,
>>   - Fusion HDTV 5 Express card
>>   - IOGear SIL - PCIE - SATA card
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>>  drivers/pci/controller/pci-tegra.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>> index 7dc728cc5f51..7e24eac12668 100644
>> --- a/drivers/pci/controller/pci-tegra.c
>> +++ b/drivers/pci/controller/pci-tegra.c
>> @@ -670,6 +670,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>>  		value |= soc->update_fc_val;
>>  		writel(value, port->base + RP_VEND_XP);
>>  	}
>> +
>> +	/*
>> +	 * PCIe link doesn't come up with few legacy PCIe endpoints
>> +	 * if root port advertises both Gen-1 and Gen-2 speeds.
>> +	 * Hence, the strategy followed here is to initially advertise
>> +	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
>> +	 */
>> +	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
>> +	value &= ~PCI_EXP_LNKSTA_CLS;
>> +	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
>> +	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
>>  }
>>  
>>  static void tegra_pcie_port_enable(struct tegra_pcie_port *port)
> This looks like it's related to the earlier patch that adds support for
> retraining the link at Gen-2. As such, I think the two patches should be
> moved closer together to make that more obvious.
>
> Also, perhaps even the order needs to be changed. For example, if the
> earlier patch enables advertisement of Gen-2, then there will be a
> period of 10 or so patches where the above devices wouldn't work. So if
> this fixes an error introduced by an earlier patch, it makes sense to
> resort the patches so that we first fix the potential error and then
> introduce the code that would cause the error to happen.
>
> Thierry
Both are independent patches. Even though HW init Target speed is Gen2, Tegra
PCIe gets the link up in Gen1 only because HW autonomous speed change feature
is not available. After link up in Gen1 SW has to retrain the link to Gen2, which is
done in 4/30. Current patch changes the HW init value of Target speed to Gen1,
to support the cards mentioned in commit message.

Patch

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 7dc728cc5f51..7e24eac12668 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -670,6 +670,17 @@  static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 		value |= soc->update_fc_val;
 		writel(value, port->base + RP_VEND_XP);
 	}
+
+	/*
+	 * PCIe link doesn't come up with few legacy PCIe endpoints
+	 * if root port advertises both Gen-1 and Gen-2 speeds.
+	 * Hence, the strategy followed here is to initially advertise
+	 * only Gen-1 and after link is up, retrain link to Gen-2 speed
+	 */
+	value = readl(port->base + RP_LINK_CONTROL_STATUS_2);
+	value &= ~PCI_EXP_LNKSTA_CLS;
+	value |= PCI_EXP_LNKSTA_CLS_2_5GB;
+	writel(value, port->base + RP_LINK_CONTROL_STATUS_2);
 }
 
 static void tegra_pcie_port_enable(struct tegra_pcie_port *port)