[PATCH V3 00/12] Enable Tegra root port features and apply SW fixups

[PATCH V3 00/12] Enable Tegra root port features and apply SW fixups

[Manikanta Maddireddy]

These series of patches does the following things,
- Deasserting pcie_xrst after programming root port to make sure that
  register programming is reflected during LTSSM
- Apply REFCLK pad settings to make sure P2P amplitude requirement is met
- Enable Gen2 link speed
- Advertise AER capability
- Program UPHY electrical settings for meeting eye diagram requirements
- Bunch of SW fixups explained in their respective commit log

Testing done on Tegra124, 210 and 186:
- PCIe link up, config read, BAR read and basic functionality of Ethernet
  card
- Link speed switch to Gen2 after link retrain
- Link speed stays in Gen1 after retrain if end point is only Gen1 capable
- Simulated AER errors and verified dmesg logs for them
- Rest of the programming is verified by dumping the registers after PCIe
link up

Manikanta Maddireddy (12):
  PCI: tegra: Start LTSSM after programming root port
  PCI: tegra: Move REFCLK pad settings out of phy_power_on()
  PCI: tegra: Retrain link for Gen2 speed
  PCI: tegra: Advertise AER capability
  PCI: tegra: Program UPHY electrical settings in Tegra210
  PCI: tegra: Enable opportunistic update FC and ACK
  PCI: tegra: Disable AFI dynamic clock gating
  PCI: tegra: Wait for DLLP to finish before entering L1 or L2
  PCI: tegra: Enable PCIe xclk clock clamping
  PCI: tegra: Add SW fixup for RAW violations
  PCI: tegra: Increase the deskew retry time
  PCI: tegra: Update flow control threshold in Tegra210

 drivers/pci/host/pci-tegra.c | 306 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 288 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Manikanta Maddireddy]

This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
after all the required root port register programming is completed.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* no change in this patch
V2:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 96e8038c3019..b41c60c7414c 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 		}
 	}
 
-	/* take the PCIe interface module out of reset */
-	reset_control_deassert(pcie->pcie_xrst);
-
 	/* finally enable PCIe */
 	value = afi_readl(pcie, AFI_CONFIGURATION);
 	value |= AFI_CONFIGURATION_EN_FPCI;
@@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 			dev_err(dev, "failed to power off PHY(s): %d\n", err);
 	}
 
-	reset_control_assert(pcie->pcie_xrst);
 	reset_control_assert(pcie->afi_rst);
 	reset_control_assert(pcie->pex_rst);
 
@@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 			 port->index, port->lanes);
 
 		tegra_pcie_port_enable(port);
+	}
 
+	/* take the PCIe interface module out of reset */
+	reset_control_deassert(pcie->pcie_xrst);
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
 		if (tegra_pcie_port_check_link(port))
 			continue;
 
-- 
2.1.4

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Lorenzo Pieralisi]

On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
> after all the required root port register programming is completed.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * no change in this patch
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 96e8038c3019..b41c60c7414c 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  		}
>  	}
>  
> -	/* take the PCIe interface module out of reset */
> -	reset_control_deassert(pcie->pcie_xrst);
> -
>  	/* finally enable PCIe */
>  	value = afi_readl(pcie, AFI_CONFIGURATION);
>  	value |= AFI_CONFIGURATION_EN_FPCI;
> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
>  	}
>  
> -	reset_control_assert(pcie->pcie_xrst);

This does not look like it is part of the reset de-assertion code move.

tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()

in other words, why are you removing it ?

Lorenzo

>  	reset_control_assert(pcie->afi_rst);
>  	reset_control_assert(pcie->pex_rst);
>  
> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  			 port->index, port->lanes);
>  
>  		tegra_pcie_port_enable(port);
> +	}
>  
> +	/* take the PCIe interface module out of reset */
> +	reset_control_deassert(pcie->pcie_xrst);
> +
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>  		if (tegra_pcie_port_check_link(port))
>  			continue;
>  
> -- 
> 2.1.4
> 

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Manikanta Maddireddy]

On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
>> after all the required root port register programming is completed.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>> V3:
>> * no change in this patch
>> V2:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index 96e8038c3019..b41c60c7414c 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>>  		}
>>  	}
>>  
>> -	/* take the PCIe interface module out of reset */
>> -	reset_control_deassert(pcie->pcie_xrst);
>> -
>>  	/* finally enable PCIe */
>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
>>  	value |= AFI_CONFIGURATION_EN_FPCI;
>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
>>  	}
>>  
>> -	reset_control_assert(pcie->pcie_xrst);
> 
> This does not look like it is part of the reset de-assertion code move.
> 
> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
> 
> in other words, why are you removing it ?
> 
> Lorenzo

Hi Lorenzo,

Host driver should start LTSSM after programming all controller registers.
In tegra_pcie_enable_controller() bunch of AFI module programming is done and
I am adding PCIe register programming in this series.
So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.

> 
>>  	reset_control_assert(pcie->afi_rst);
>>  	reset_control_assert(pcie->pex_rst);
>>  
>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>  			 port->index, port->lanes);
>>  
>>  		tegra_pcie_port_enable(port);
>> +	}
>>  
>> +	/* take the PCIe interface module out of reset */
>> +	reset_control_deassert(pcie->pcie_xrst);
>> +
>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>>  		if (tegra_pcie_port_check_link(port))
>>  			continue;
>>  
>> -- 
>> 2.1.4
>>

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Lorenzo Pieralisi]

On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
> > On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
> >> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
> >> after all the required root port register programming is completed.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V3:
> >> * no change in this patch
> >> V2:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index 96e8038c3019..b41c60c7414c 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >>  		}
> >>  	}
> >>  
> >> -	/* take the PCIe interface module out of reset */
> >> -	reset_control_deassert(pcie->pcie_xrst);
> >> -
> >>  	/* finally enable PCIe */
> >>  	value = afi_readl(pcie, AFI_CONFIGURATION);
> >>  	value |= AFI_CONFIGURATION_EN_FPCI;
> >> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> >>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
> >>  	}
> >>  
> >> -	reset_control_assert(pcie->pcie_xrst);
> > 
> > This does not look like it is part of the reset de-assertion code move.
> > 
> > tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
> > 
> > in other words, why are you removing it ?
> > 
> > Lorenzo
> 
> Hi Lorenzo,
> 
> Host driver should start LTSSM after programming all controller registers.
> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
> I am adding PCIe register programming in this series.
> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.

I asked why you removed the reset assertion in tegra_pcie_power_off(),
it is not clear to me. You still call tegra_pcie_power_off() in
the tegra_pcie_probe() error path and I see no reason why the reset
assertion - called through:

tegra_pcie_put_resources()
	-> tegra_pcie_power_of()
	
is removed, if it was needed previously.

Lorenzo

> >>  	reset_control_assert(pcie->afi_rst);
> >>  	reset_control_assert(pcie->pex_rst);
> >>  
> >> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
> >>  			 port->index, port->lanes);
> >>  
> >>  		tegra_pcie_port_enable(port);
> >> +	}
> >>  
> >> +	/* take the PCIe interface module out of reset */
> >> +	reset_control_deassert(pcie->pcie_xrst);
> >> +
> >> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> >>  		if (tegra_pcie_port_check_link(port))
> >>  			continue;
> >>  
> >> -- 
> >> 2.1.4
> >>

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Manikanta Maddireddy]

On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
>>>> after all the required root port register programming is completed.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>> V3:
>>>> * no change in this patch
>>>> V2:
>>>> * no change in this patch
>>>>
>>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>>>> index 96e8038c3019..b41c60c7414c 100644
>>>> --- a/drivers/pci/host/pci-tegra.c
>>>> +++ b/drivers/pci/host/pci-tegra.c
>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>>>>  		}
>>>>  	}
>>>>  
>>>> -	/* take the PCIe interface module out of reset */
>>>> -	reset_control_deassert(pcie->pcie_xrst);
>>>> -
>>>>  	/* finally enable PCIe */
>>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
>>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
>>>>  	}
>>>>  
>>>> -	reset_control_assert(pcie->pcie_xrst);
>>>
>>> This does not look like it is part of the reset de-assertion code move.
>>>
>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
>>>
>>> in other words, why are you removing it ?
>>>
>>> Lorenzo
>>
>> Hi Lorenzo,
>>
>> Host driver should start LTSSM after programming all controller registers.
>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
>> I am adding PCIe register programming in this series.
>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
> 
> I asked why you removed the reset assertion in tegra_pcie_power_off(),
> it is not clear to me. You still call tegra_pcie_power_off() in
> the tegra_pcie_probe() error path and I see no reason why the reset
> assertion - called through:
> 
> tegra_pcie_put_resources()
> 	-> tegra_pcie_power_of()
> 	
> is removed, if it was needed previously.
> 
> Lorenzo
> 

New sequence with this patch will be
tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
                                   ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);

Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.

>>>>  	reset_control_assert(pcie->afi_rst);
>>>>  	reset_control_assert(pcie->pex_rst);
>>>>  
>>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>>>  			 port->index, port->lanes);
>>>>  
>>>>  		tegra_pcie_port_enable(port);
>>>> +	}
>>>>  
>>>> +	/* take the PCIe interface module out of reset */
>>>> +	reset_control_deassert(pcie->pcie_xrst);
>>>> +
>>>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>>>>  		if (tegra_pcie_port_check_link(port))
>>>>  			continue;
>>>>  
>>>> -- 
>>>> 2.1.4
>>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Lorenzo Pieralisi]

On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
> > On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
> >>
> >>
> >> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
> >>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
> >>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
> >>>> after all the required root port register programming is completed.
> >>>>
> >>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >>>> ---
> >>>> V3:
> >>>> * no change in this patch
> >>>> V2:
> >>>> * no change in this patch
> >>>>
> >>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
> >>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >>>> index 96e8038c3019..b41c60c7414c 100644
> >>>> --- a/drivers/pci/host/pci-tegra.c
> >>>> +++ b/drivers/pci/host/pci-tegra.c
> >>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >>>>  		}
> >>>>  	}
> >>>>  
> >>>> -	/* take the PCIe interface module out of reset */
> >>>> -	reset_control_deassert(pcie->pcie_xrst);
> >>>> -
> >>>>  	/* finally enable PCIe */
> >>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
> >>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
> >>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> >>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
> >>>>  	}
> >>>>  
> >>>> -	reset_control_assert(pcie->pcie_xrst);
> >>>
> >>> This does not look like it is part of the reset de-assertion code move.
> >>>
> >>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
> >>>
> >>> in other words, why are you removing it ?
> >>>
> >>> Lorenzo
> >>
> >> Hi Lorenzo,
> >>
> >> Host driver should start LTSSM after programming all controller registers.
> >> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
> >> I am adding PCIe register programming in this series.
> >> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
> >> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
> > 
> > I asked why you removed the reset assertion in tegra_pcie_power_off(),
> > it is not clear to me. You still call tegra_pcie_power_off() in
> > the tegra_pcie_probe() error path and I see no reason why the reset
> > assertion - called through:
> > 
> > tegra_pcie_put_resources()
> > 	-> tegra_pcie_power_of()
> > 	
> > is removed, if it was needed previously.
> > 
> > Lorenzo
> > 
> 
> New sequence with this patch will be
> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
>                                    ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);
> 
> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.

I do not understand you, sorry for being blunt.

What has tegra_pcie_request_resources() to do with the reset
assertion/deassertion ?

This patch moves:

reset_control_deassert(pcie->pcie_xrst);

from:

tegra_pcie_enable_controller()

to

tegra_pcie_enable_ports()

if:

reset_control_assert(pcie->pcie_xrst);

was needed before this patch in tegra_pcie_power_off()

why it is not needed there after this patch is applied ?

Lorenzo

> >>>>  	reset_control_assert(pcie->afi_rst);
> >>>>  	reset_control_assert(pcie->pex_rst);
> >>>>  
> >>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
> >>>>  			 port->index, port->lanes);
> >>>>  
> >>>>  		tegra_pcie_port_enable(port);
> >>>> +	}
> >>>>  
> >>>> +	/* take the PCIe interface module out of reset */
> >>>> +	reset_control_deassert(pcie->pcie_xrst);
> >>>> +
> >>>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> >>>>  		if (tegra_pcie_port_check_link(port))
> >>>>  			continue;
> >>>>  
> >>>> -- 
> >>>> 2.1.4
> >>>>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Manikanta Maddireddy]

On 14-Dec-17 12:04 AM, Lorenzo Pieralisi wrote:
> On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
>>> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
>>>>
>>>>
>>>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
>>>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
>>>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
>>>>>> after all the required root port register programming is completed.
>>>>>>
>>>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>>>> ---
>>>>>> V3:
>>>>>> * no change in this patch
>>>>>> V2:
>>>>>> * no change in this patch
>>>>>>
>>>>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
>>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>>>>>> index 96e8038c3019..b41c60c7414c 100644
>>>>>> --- a/drivers/pci/host/pci-tegra.c
>>>>>> +++ b/drivers/pci/host/pci-tegra.c
>>>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>>>>>>  		}
>>>>>>  	}
>>>>>>  
>>>>>> -	/* take the PCIe interface module out of reset */
>>>>>> -	reset_control_deassert(pcie->pcie_xrst);
>>>>>> -
>>>>>>  	/* finally enable PCIe */
>>>>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
>>>>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
>>>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>>>>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
>>>>>>  	}
>>>>>>  
>>>>>> -	reset_control_assert(pcie->pcie_xrst);
>>>>>
>>>>> This does not look like it is part of the reset de-assertion code move.
>>>>>
>>>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
>>>>>
>>>>> in other words, why are you removing it ?
>>>>>
>>>>> Lorenzo
>>>>
>>>> Hi Lorenzo,
>>>>
>>>> Host driver should start LTSSM after programming all controller registers.
>>>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
>>>> I am adding PCIe register programming in this series.
>>>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
>>>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
>>>
>>> I asked why you removed the reset assertion in tegra_pcie_power_off(),
>>> it is not clear to me. You still call tegra_pcie_power_off() in
>>> the tegra_pcie_probe() error path and I see no reason why the reset
>>> assertion - called through:
>>>
>>> tegra_pcie_put_resources()
>>> 	-> tegra_pcie_power_of()
>>> 	
>>> is removed, if it was needed previously.
>>>
>>> Lorenzo
>>>
>>
>> New sequence with this patch will be
>> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
>>                                    ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);
>>
>> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.
> 
> I do not understand you, sorry for being blunt.
> 
> What has tegra_pcie_request_resources() to do with the reset
> assertion/deassertion ?
> 
> This patch moves:
> 
> reset_control_deassert(pcie->pcie_xrst);
> 
> from:
> 
> tegra_pcie_enable_controller()
> 
> to
> 
> tegra_pcie_enable_ports()
> 
> if:
> 
> reset_control_assert(pcie->pcie_xrst);
> 
> was needed before this patch in tegra_pcie_power_off()
> 
> why it is not needed there after this patch is applied ?
> 
> Lorenzo
> 
Opposite of tegra_pcie_enable_ports() is missing on disable_msi
failure in current driver. So I took care of assert pcie_xrst
in https://patchwork.ozlabs.org/patch/846043/ along with other
resources.

>>>>>>  	reset_control_assert(pcie->afi_rst);
>>>>>>  	reset_control_assert(pcie->pex_rst);
>>>>>>  
>>>>>> @@ -2116,7 +2112,12 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>>>>>  			 port->index, port->lanes);
>>>>>>  
>>>>>>  		tegra_pcie_port_enable(port);
>>>>>> +	}
>>>>>>  
>>>>>> +	/* take the PCIe interface module out of reset */
>>>>>> +	reset_control_deassert(pcie->pcie_xrst);
>>>>>> +
>>>>>> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>>>>>>  		if (tegra_pcie_port_check_link(port))
>>>>>>  			continue;
>>>>>>  
>>>>>> -- 
>>>>>> 2.1.4
>>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Lorenzo Pieralisi]

On Thu, Dec 14, 2017 at 12:57:28AM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 14-Dec-17 12:04 AM, Lorenzo Pieralisi wrote:
> > On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote:
> >>
> >>
> >> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
> >>> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
> >>>>
> >>>>
> >>>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
> >>>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
> >>>>>> after all the required root port register programming is completed.
> >>>>>>
> >>>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >>>>>> ---
> >>>>>> V3:
> >>>>>> * no change in this patch
> >>>>>> V2:
> >>>>>> * no change in this patch
> >>>>>>
> >>>>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
> >>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >>>>>> index 96e8038c3019..b41c60c7414c 100644
> >>>>>> --- a/drivers/pci/host/pci-tegra.c
> >>>>>> +++ b/drivers/pci/host/pci-tegra.c
> >>>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> >>>>>>  		}
> >>>>>>  	}
> >>>>>>  
> >>>>>> -	/* take the PCIe interface module out of reset */
> >>>>>> -	reset_control_deassert(pcie->pcie_xrst);
> >>>>>> -
> >>>>>>  	/* finally enable PCIe */
> >>>>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
> >>>>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
> >>>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> >>>>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
> >>>>>>  	}
> >>>>>>  
> >>>>>> -	reset_control_assert(pcie->pcie_xrst);
> >>>>>
> >>>>> This does not look like it is part of the reset de-assertion code move.
> >>>>>
> >>>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
> >>>>>
> >>>>> in other words, why are you removing it ?
> >>>>>
> >>>>> Lorenzo
> >>>>
> >>>> Hi Lorenzo,
> >>>>
> >>>> Host driver should start LTSSM after programming all controller registers.
> >>>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
> >>>> I am adding PCIe register programming in this series.
> >>>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
> >>>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
> >>>
> >>> I asked why you removed the reset assertion in tegra_pcie_power_off(),
> >>> it is not clear to me. You still call tegra_pcie_power_off() in
> >>> the tegra_pcie_probe() error path and I see no reason why the reset
> >>> assertion - called through:
> >>>
> >>> tegra_pcie_put_resources()
> >>> 	-> tegra_pcie_power_of()
> >>> 	
> >>> is removed, if it was needed previously.
> >>>
> >>> Lorenzo
> >>>
> >>
> >> New sequence with this patch will be
> >> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
> >>                                    ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);
> >>
> >> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.
> > 
> > I do not understand you, sorry for being blunt.
> > 
> > What has tegra_pcie_request_resources() to do with the reset
> > assertion/deassertion ?
> > 
> > This patch moves:
> > 
> > reset_control_deassert(pcie->pcie_xrst);
> > 
> > from:
> > 
> > tegra_pcie_enable_controller()
> > 
> > to
> > 
> > tegra_pcie_enable_ports()
> > 
> > if:
> > 
> > reset_control_assert(pcie->pcie_xrst);
> > 
> > was needed before this patch in tegra_pcie_power_off()
> > 
> > why it is not needed there after this patch is applied ?
> > 
> > Lorenzo
> > 
> Opposite of tegra_pcie_enable_ports() is missing on disable_msi
> failure in current driver. So I took care of assert pcie_xrst
> in https://patchwork.ozlabs.org/patch/846043/ along with other
> resources.

Which is in another series. This patch has to be self-contained with a
single logical change that is applicable on its own. Even if we end up
merging the series above every patch has to make sense on its own. This
does not.

So I would start with re-posting this series as a self contained one,
addressing all review comments; we will get to other series later when
this is sorted.

Thanks,
Lorenzo

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Lorenzo Pieralisi]

On Thu, Dec 14, 2017 at 09:57:24AM +0000, Lorenzo Pieralisi wrote:
> On Thu, Dec 14, 2017 at 12:57:28AM +0530, Manikanta Maddireddy wrote:
> > 
> > 
> > On 14-Dec-17 12:04 AM, Lorenzo Pieralisi wrote:
> > > On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote:
> > >>
> > >>
> > >> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
> > >>> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
> > >>>>
> > >>>>
> > >>>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
> > >>>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
> > >>>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
> > >>>>>> after all the required root port register programming is completed.
> > >>>>>>
> > >>>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > >>>>>> ---
> > >>>>>> V3:
> > >>>>>> * no change in this patch
> > >>>>>> V2:
> > >>>>>> * no change in this patch
> > >>>>>>
> > >>>>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
> > >>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > >>>>>> index 96e8038c3019..b41c60c7414c 100644
> > >>>>>> --- a/drivers/pci/host/pci-tegra.c
> > >>>>>> +++ b/drivers/pci/host/pci-tegra.c
> > >>>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
> > >>>>>>  		}
> > >>>>>>  	}
> > >>>>>>  
> > >>>>>> -	/* take the PCIe interface module out of reset */
> > >>>>>> -	reset_control_deassert(pcie->pcie_xrst);
> > >>>>>> -
> > >>>>>>  	/* finally enable PCIe */
> > >>>>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
> > >>>>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
> > >>>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
> > >>>>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
> > >>>>>>  	}
> > >>>>>>  
> > >>>>>> -	reset_control_assert(pcie->pcie_xrst);
> > >>>>>
> > >>>>> This does not look like it is part of the reset de-assertion code move.
> > >>>>>
> > >>>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
> > >>>>>
> > >>>>> in other words, why are you removing it ?
> > >>>>>
> > >>>>> Lorenzo
> > >>>>
> > >>>> Hi Lorenzo,
> > >>>>
> > >>>> Host driver should start LTSSM after programming all controller registers.
> > >>>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
> > >>>> I am adding PCIe register programming in this series.
> > >>>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
> > >>>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
> > >>>
> > >>> I asked why you removed the reset assertion in tegra_pcie_power_off(),
> > >>> it is not clear to me. You still call tegra_pcie_power_off() in
> > >>> the tegra_pcie_probe() error path and I see no reason why the reset
> > >>> assertion - called through:
> > >>>
> > >>> tegra_pcie_put_resources()
> > >>> 	-> tegra_pcie_power_of()
> > >>> 	
> > >>> is removed, if it was needed previously.
> > >>>
> > >>> Lorenzo
> > >>>
> > >>
> > >> New sequence with this patch will be
> > >> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
> > >>                                    ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);
> > >>
> > >> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.
> > > 
> > > I do not understand you, sorry for being blunt.
> > > 
> > > What has tegra_pcie_request_resources() to do with the reset
> > > assertion/deassertion ?
> > > 
> > > This patch moves:
> > > 
> > > reset_control_deassert(pcie->pcie_xrst);
> > > 
> > > from:
> > > 
> > > tegra_pcie_enable_controller()
> > > 
> > > to
> > > 
> > > tegra_pcie_enable_ports()
> > > 
> > > if:
> > > 
> > > reset_control_assert(pcie->pcie_xrst);
> > > 
> > > was needed before this patch in tegra_pcie_power_off()
> > > 
> > > why it is not needed there after this patch is applied ?
> > > 
> > > Lorenzo
> > > 
> > Opposite of tegra_pcie_enable_ports() is missing on disable_msi
> > failure in current driver. So I took care of assert pcie_xrst
> > in https://patchwork.ozlabs.org/patch/846043/ along with other
> > resources.
> 
> Which is in another series. This patch has to be self-contained with a
> single logical change that is applicable on its own. Even if we end up
> merging the series above every patch has to make sense on its own. This
> does not.
> 
> So I would start with re-posting this series as a self contained one,
> addressing all review comments; we will get to other series later when
> this is sorted.

Hi Manikanta,

I wanted to check with you where are we with this series. I would mark
it with "Changes requested", waiting for you to rebase it and update it
according to review comments.

Please let me know if that's OK with you.

Thanks,
Lorenzo

Re: [PATCH V3 01/12] PCI: tegra: Start LTSSM after programming root port

[Manikanta Maddireddy]

On 07-Mar-18 5:30 PM, Lorenzo Pieralisi wrote:
> On Thu, Dec 14, 2017 at 09:57:24AM +0000, Lorenzo Pieralisi wrote:
>> On Thu, Dec 14, 2017 at 12:57:28AM +0530, Manikanta Maddireddy wrote:
>>>
>>>
>>> On 14-Dec-17 12:04 AM, Lorenzo Pieralisi wrote:
>>>> On Wed, Dec 13, 2017 at 10:02:02PM +0530, Manikanta Maddireddy wrote:
>>>>>
>>>>>
>>>>> On 13-Dec-17 7:38 PM, Lorenzo Pieralisi wrote:
>>>>>> On Wed, Dec 13, 2017 at 05:20:39PM +0530, Manikanta Maddireddy wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 12-Dec-17 5:02 PM, Lorenzo Pieralisi wrote:
>>>>>>>> On Mon, Oct 30, 2017 at 07:27:12PM +0530, Manikanta Maddireddy wrote:
>>>>>>>>> This patch ensures that LTSSM is started (by deasserting pcie_xrst) only
>>>>>>>>> after all the required root port register programming is completed.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>>>>>>> ---
>>>>>>>>> V3:
>>>>>>>>> * no change in this patch
>>>>>>>>> V2:
>>>>>>>>> * no change in this patch
>>>>>>>>>
>>>>>>>>>  drivers/pci/host/pci-tegra.c | 9 +++++----
>>>>>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>>>>>>>>> index 96e8038c3019..b41c60c7414c 100644
>>>>>>>>> --- a/drivers/pci/host/pci-tegra.c
>>>>>>>>> +++ b/drivers/pci/host/pci-tegra.c
>>>>>>>>> @@ -1024,9 +1024,6 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>>>>>>>>>  		}
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>> -	/* take the PCIe interface module out of reset */
>>>>>>>>> -	reset_control_deassert(pcie->pcie_xrst);
>>>>>>>>> -
>>>>>>>>>  	/* finally enable PCIe */
>>>>>>>>>  	value = afi_readl(pcie, AFI_CONFIGURATION);
>>>>>>>>>  	value |= AFI_CONFIGURATION_EN_FPCI;
>>>>>>>>> @@ -1065,7 +1062,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
>>>>>>>>>  			dev_err(dev, "failed to power off PHY(s): %d\n", err);
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>> -	reset_control_assert(pcie->pcie_xrst);
>>>>>>>>
>>>>>>>> This does not look like it is part of the reset de-assertion code move.
>>>>>>>>
>>>>>>>> tegra_pcie_enable_controller() -> tegra_pcie_enable_ports()
>>>>>>>>
>>>>>>>> in other words, why are you removing it ?
>>>>>>>>
>>>>>>>> Lorenzo
>>>>>>>
>>>>>>> Hi Lorenzo,
>>>>>>>
>>>>>>> Host driver should start LTSSM after programming all controller registers.
>>>>>>> In tegra_pcie_enable_controller() bunch of AFI module programming is done and
>>>>>>> I am adding PCIe register programming in this series.
>>>>>>> So I moved deasserting of pcie_xrst(which starts LTSSM) to tegra_pcie_enable_ports(),
>>>>>>> which is right after sending RST pulse(tegra_pcie_port_reset()) to endpoint.
>>>>>>
>>>>>> I asked why you removed the reset assertion in tegra_pcie_power_off(),
>>>>>> it is not clear to me. You still call tegra_pcie_power_off() in
>>>>>> the tegra_pcie_probe() error path and I see no reason why the reset
>>>>>> assertion - called through:
>>>>>>
>>>>>> tegra_pcie_put_resources()
>>>>>> 	-> tegra_pcie_power_of()
>>>>>> 	
>>>>>> is removed, if it was needed previously.
>>>>>>
>>>>>> Lorenzo
>>>>>>
>>>>>
>>>>> New sequence with this patch will be
>>>>> tegra_pcie_enable_controller() -> tegra_pcie_request_resources() -> tegra_pcie_enable_ports()
>>>>>                                    ->goto put_resources on fail        -> reset_control_deassert(pcie->pcie_xrst);
>>>>>
>>>>> Since pcie_xrst deassert happens after tegra_pcie_request_resources(), there is no need to assert pcie_xrst on put_resource failure.
>>>>
>>>> I do not understand you, sorry for being blunt.
>>>>
>>>> What has tegra_pcie_request_resources() to do with the reset
>>>> assertion/deassertion ?
>>>>
>>>> This patch moves:
>>>>
>>>> reset_control_deassert(pcie->pcie_xrst);
>>>>
>>>> from:
>>>>
>>>> tegra_pcie_enable_controller()
>>>>
>>>> to
>>>>
>>>> tegra_pcie_enable_ports()
>>>>
>>>> if:
>>>>
>>>> reset_control_assert(pcie->pcie_xrst);
>>>>
>>>> was needed before this patch in tegra_pcie_power_off()
>>>>
>>>> why it is not needed there after this patch is applied ?
>>>>
>>>> Lorenzo
>>>>
>>> Opposite of tegra_pcie_enable_ports() is missing on disable_msi
>>> failure in current driver. So I took care of assert pcie_xrst
>>> in https://patchwork.ozlabs.org/patch/846043/ along with other
>>> resources.
>>
>> Which is in another series. This patch has to be self-contained with a
>> single logical change that is applicable on its own. Even if we end up
>> merging the series above every patch has to make sense on its own. This
>> does not.
>>
>> So I would start with re-posting this series as a self contained one,
>> addressing all review comments; we will get to other series later when
>> this is sorted.
> 
> Hi Manikanta,
> 
> I wanted to check with you where are we with this series. I would mark
> it with "Changes requested", waiting for you to rebase it and update it
> according to review comments.
> 
> Please let me know if that's OK with you.
> 
> Thanks,
> Lorenzo
> 

Hi Lorenzo,

Sure, I will address the review comments and publish rebased series soon.

Thanks,
Manikanta

[PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Manikanta Maddireddy]

In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
in driver. REFCLK pad settings are independent of PHY and should be
programmed by driver. So move REFCLK pad settings out of phy_power_on().
These pad settings improve REFCLK peak to peak amplitude.

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

 drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index b41c60c7414c..068510b40c1a 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
 static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	const struct tegra_pcie_soc *soc = pcie->soc;
 	struct tegra_pcie_port *port;
 	int err;
 
@@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
 		}
 	}
 
-	/* Configure the reference clock driver */
-	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
-
-	if (soc->num_ports > 2)
-		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
-
 	return 0;
 }
 
@@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 	return 0;
 }
 
+static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
+{
+	const struct tegra_pcie_soc *soc = pcie->soc;
+
+	/* Configure the reference clock driver */
+	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
+
+	if (soc->num_ports > 2)
+		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
+}
+
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
  * can result in the increase of the bootup time as there are big timeout
@@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 	struct device *dev = pcie->dev;
 	struct tegra_pcie_port *port, *tmp;
 
+	tegra_pcie_apply_pad_settings(pcie);
+
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
 		dev_info(dev, "probing port %u, using %u lanes\n",
 			 port->index, port->lanes);
-- 
2.1.4

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Lorenzo Pieralisi]

On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped

It is time you defined what FW does and what the kernel does and stick
to that SW initialization sequence for all Tegra PCI host bridges from
now onwards. Those tegra_pcie_soc structures in the driver will end up
with a gazillion hardcoded parameters if you do not take a stance from
that perspective and I do not like that - at all.

You may want to use DT bindings to describe the FW<->OS handover - or
just enforce firmware guidelines for Tegra systems - something has to be
done.

This patch makes sense stand-alone but I wanted to get the message
across.

Thanks,
Lorenzo

> in driver. REFCLK pad settings are independent of PHY and should be
> programmed by driver. So move REFCLK pad settings out of phy_power_on().
> These pad settings improve REFCLK peak to peak amplitude.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * Corrected commit log
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index b41c60c7414c..068510b40c1a 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
>  static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
> -	const struct tegra_pcie_soc *soc = pcie->soc;
>  	struct tegra_pcie_port *port;
>  	int err;
>  
> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>  		}
>  	}
>  
> -	/* Configure the reference clock driver */
> -	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
> -
> -	if (soc->num_ports > 2)
> -		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
> -
>  	return 0;
>  }
>  
> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  	return 0;
>  }
>  
> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
> +{
> +	const struct tegra_pcie_soc *soc = pcie->soc;
> +
> +	/* Configure the reference clock driver */
> +	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
> +
> +	if (soc->num_ports > 2)
> +		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
> +}
> +
>  /*
>   * FIXME: If there are no PCIe cards attached, then calling this function
>   * can result in the increase of the bootup time as there are big timeout
> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  	struct device *dev = pcie->dev;
>  	struct tegra_pcie_port *port, *tmp;
>  
> +	tegra_pcie_apply_pad_settings(pcie);
> +
>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>  		dev_info(dev, "probing port %u, using %u lanes\n",
>  			 port->index, port->lanes);
> -- 
> 2.1.4
> 

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Manikanta Maddireddy]

On 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote:
> On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
>> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
> 
> It is time you defined what FW does and what the kernel does and stick
> to that SW initialization sequence for all Tegra PCI host bridges from
> now onwards. Those tegra_pcie_soc structures in the driver will end up
> with a gazillion hardcoded parameters if you do not take a stance from
> that perspective and I do not like that - at all.
> 
> You may want to use DT bindings to describe the FW<->OS handover - or
> just enforce firmware guidelines for Tegra systems - something has to be
> done.
> 
> This patch makes sense stand-alone but I wanted to get the message
> across.
> 
> Thanks,
> Lorenzo
> 

In Tegra186 BPMP-FW will take care of programming the UPHY, this is
updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")".
I didn't put it in DT bindings because there is no DT property for this purpose.
Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable?

In current host driver, REFCLK pads settings are done in tegra_pcie_port_phy_power_off()
which is not apt. REFCLK pad settings tunes the PCIe sideband signal REFCLK, it has
nothing to do with UPHY. So I am moving it out of tegra_pcie_port_phy_power_off().

Thanks,
Manikanta


>> in driver. REFCLK pad settings are independent of PHY and should be
>> programmed by driver. So move REFCLK pad settings out of phy_power_on().
>> These pad settings improve REFCLK peak to peak amplitude.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>> V3:
>> * Corrected commit log
>> V2:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>> index b41c60c7414c..068510b40c1a 100644
>> --- a/drivers/pci/host/pci-tegra.c
>> +++ b/drivers/pci/host/pci-tegra.c
>> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
>>  static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>>  {
>>  	struct device *dev = pcie->dev;
>> -	const struct tegra_pcie_soc *soc = pcie->soc;
>>  	struct tegra_pcie_port *port;
>>  	int err;
>>  
>> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>>  		}
>>  	}
>>  
>> -	/* Configure the reference clock driver */
>> -	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
>> -
>> -	if (soc->num_ports > 2)
>> -		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>> -
>>  	return 0;
>>  }
>>  
>> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>  	return 0;
>>  }
>>  
>> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>> +{
>> +	const struct tegra_pcie_soc *soc = pcie->soc;
>> +
>> +	/* Configure the reference clock driver */
>> +	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
>> +
>> +	if (soc->num_ports > 2)
>> +		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>> +}
>> +
>>  /*
>>   * FIXME: If there are no PCIe cards attached, then calling this function
>>   * can result in the increase of the bootup time as there are big timeout
>> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>  	struct device *dev = pcie->dev;
>>  	struct tegra_pcie_port *port, *tmp;
>>  
>> +	tegra_pcie_apply_pad_settings(pcie);
>> +
>>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>>  		dev_info(dev, "probing port %u, using %u lanes\n",
>>  			 port->index, port->lanes);
>> -- 
>> 2.1.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Lorenzo Pieralisi]

On Wed, Dec 13, 2017 at 05:32:32PM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote:
> > On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
> >> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
> > 
> > It is time you defined what FW does and what the kernel does and stick
> > to that SW initialization sequence for all Tegra PCI host bridges from
> > now onwards. Those tegra_pcie_soc structures in the driver will end up
> > with a gazillion hardcoded parameters if you do not take a stance from
> > that perspective and I do not like that - at all.
> > 
> > You may want to use DT bindings to describe the FW<->OS handover - or
> > just enforce firmware guidelines for Tegra systems - something has to be
> > done.
> > 
> > This patch makes sense stand-alone but I wanted to get the message
> > across.
> > 
> > Thanks,
> > Lorenzo
> > 
> 
> In Tegra186 BPMP-FW will take care of programming the UPHY, this is
> updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")".
> I didn't put it in DT bindings because there is no DT property for this purpose.
> Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable?

I would like to see some consistency from now onwards on what's done
in BPMP (or host) FW and what's done in the driver and that's something
you should drive because, as I have already said, the current approch
based on boolean flags per-SoC hardcoded in the kernel does not scale -
I may tolerate it for now but that has to change.

Is it that hard to define a software programming model (to describe
what's done in what piece of software/firmware) for tegra PCI host
bridges ?

> In current host driver, REFCLK pads settings are done in
> tegra_pcie_port_phy_power_off() which is not apt. REFCLK pad settings
> tunes the PCIe sideband signal REFCLK, it has nothing to do with UPHY.
> So I am moving it out of tegra_pcie_port_phy_power_off().

I gathered that, I was not complaining on this patch on his own.

Thanks,
Lorenzo

> Thanks,
> Manikanta
> 
> 
> >> in driver. REFCLK pad settings are independent of PHY and should be
> >> programmed by driver. So move REFCLK pad settings out of phy_power_on().
> >> These pad settings improve REFCLK peak to peak amplitude.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V3:
> >> * Corrected commit log
> >> V2:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
> >>  1 file changed, 13 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index b41c60c7414c..068510b40c1a 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
> >>  static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
> >>  {
> >>  	struct device *dev = pcie->dev;
> >> -	const struct tegra_pcie_soc *soc = pcie->soc;
> >>  	struct tegra_pcie_port *port;
> >>  	int err;
> >>  
> >> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
> >>  		}
> >>  	}
> >>  
> >> -	/* Configure the reference clock driver */
> >> -	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
> >> -
> >> -	if (soc->num_ports > 2)
> >> -		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
> >> -
> >>  	return 0;
> >>  }
> >>  
> >> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
> >>  	return 0;
> >>  }
> >>  
> >> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
> >> +{
> >> +	const struct tegra_pcie_soc *soc = pcie->soc;
> >> +
> >> +	/* Configure the reference clock driver */
> >> +	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
> >> +
> >> +	if (soc->num_ports > 2)
> >> +		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
> >> +}
> >> +
> >>  /*
> >>   * FIXME: If there are no PCIe cards attached, then calling this function
> >>   * can result in the increase of the bootup time as there are big timeout
> >> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
> >>  	struct device *dev = pcie->dev;
> >>  	struct tegra_pcie_port *port, *tmp;
> >>  
> >> +	tegra_pcie_apply_pad_settings(pcie);
> >> +
> >>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> >>  		dev_info(dev, "probing port %u, using %u lanes\n",
> >>  			 port->index, port->lanes);
> >> -- 
> >> 2.1.4
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Mikko Perttunen]

On 13.12.2017 16:23, Lorenzo Pieralisi wrote:
> On Wed, Dec 13, 2017 at 05:32:32PM +0530, Manikanta Maddireddy wrote:
>>
>>
>> On 12-Dec-17 5:15 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
>>>> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
>>>
>>> It is time you defined what FW does and what the kernel does and stick
>>> to that SW initialization sequence for all Tegra PCI host bridges from
>>> now onwards. Those tegra_pcie_soc structures in the driver will end up
>>> with a gazillion hardcoded parameters if you do not take a stance from
>>> that perspective and I do not like that - at all.
>>>
>>> You may want to use DT bindings to describe the FW<->OS handover - or
>>> just enforce firmware guidelines for Tegra systems - something has to be
>>> done.
>>>
>>> This patch makes sense stand-alone but I wanted to get the message
>>> across.
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>
>> In Tegra186 BPMP-FW will take care of programming the UPHY, this is
>> updated in commit log of "9cea513d8cbc ("PCI: tegra: Add Tegra186 PCIe support")".
>> I didn't put it in DT bindings because there is no DT property for this purpose.
>> Do you want me to add a comment in PCIe tegra driver, perhaps near program_uphy variable?
> 
> I would like to see some consistency from now onwards on what's done
> in BPMP (or host) FW and what's done in the driver and that's something
> you should drive because, as I have already said, the current approch
> based on boolean flags per-SoC hardcoded in the kernel does not scale -
> I may tolerate it for now but that has to change.
>

The model has changed once, when BPMP was introduced. So pre-BPMP SoCs 
(Tegra20/30/114/124/210) program the UPHY in kernel and post-BPMP SoCs 
(Tegra186+) have the BPMP program it. So there should not be further 
changes to this.

Mikko

> Is it that hard to define a software programming model (to describe
> what's done in what piece of software/firmware) for tegra PCI host
> bridges >
>> In current host driver, REFCLK pads settings are done in
>> tegra_pcie_port_phy_power_off() which is not apt. REFCLK pad settings
>> tunes the PCIe sideband signal REFCLK, it has nothing to do with UPHY.
>> So I am moving it out of tegra_pcie_port_phy_power_off().
> 
> I gathered that, I was not complaining on this patch on his own.
> 
> Thanks,
> Lorenzo
> 
>> Thanks,
>> Manikanta
>>
>>
>>>> in driver. REFCLK pad settings are independent of PHY and should be
>>>> programmed by driver. So move REFCLK pad settings out of phy_power_on().
>>>> These pad settings improve REFCLK peak to peak amplitude.
>>>>
>>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>>> ---
>>>> V3:
>>>> * Corrected commit log
>>>> V2:
>>>> * no change in this patch
>>>>
>>>>   drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
>>>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
>>>> index b41c60c7414c..068510b40c1a 100644
>>>> --- a/drivers/pci/host/pci-tegra.c
>>>> +++ b/drivers/pci/host/pci-tegra.c
>>>> @@ -910,7 +910,6 @@ static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
>>>>   static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>>>>   {
>>>>   	struct device *dev = pcie->dev;
>>>> -	const struct tegra_pcie_soc *soc = pcie->soc;
>>>>   	struct tegra_pcie_port *port;
>>>>   	int err;
>>>>   
>>>> @@ -936,12 +935,6 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>>>>   		}
>>>>   	}
>>>>   
>>>> -	/* Configure the reference clock driver */
>>>> -	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
>>>> -
>>>> -	if (soc->num_ports > 2)
>>>> -		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>>>> -
>>>>   	return 0;
>>>>   }
>>>>   
>>>> @@ -2049,6 +2042,17 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>>>   	return 0;
>>>>   }
>>>>   
>>>> +static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>>>> +{
>>>> +	const struct tegra_pcie_soc *soc = pcie->soc;
>>>> +
>>>> +	/* Configure the reference clock driver */
>>>> +	pads_writel(pcie, soc->pads_refclk_cfg0, PADS_REFCLK_CFG0);
>>>> +
>>>> +	if (soc->num_ports > 2)
>>>> +		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>>>> +}
>>>> +
>>>>   /*
>>>>    * FIXME: If there are no PCIe cards attached, then calling this function
>>>>    * can result in the increase of the bootup time as there are big timeout
>>>> @@ -2107,6 +2111,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>>>>   	struct device *dev = pcie->dev;
>>>>   	struct tegra_pcie_port *port, *tmp;
>>>>   
>>>> +	tegra_pcie_apply_pad_settings(pcie);
>>>> +
>>>>   	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>>>>   		dev_info(dev, "probing port %u, using %u lanes\n",
>>>>   			 port->index, port->lanes);
>>>> -- 
>>>> 2.1.4
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
> In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
> in driver. REFCLK pad settings are independent of PHY and should be
> programmed by driver. So move REFCLK pad settings out of phy_power_on().
> These pad settings improve REFCLK peak to peak amplitude.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * Corrected commit log
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)

As far as I can tell, this is a bugfix that can and should be applied
independently of the rest of the series.

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V3 02/12] PCI: tegra: Move REFCLK pad settings out of phy_power_on()

[Lorenzo Pieralisi]

On Thu, Dec 14, 2017 at 04:14:52PM +0100, Thierry Reding wrote:
> On Mon, Oct 30, 2017 at 07:27:13PM +0530, Manikanta Maddireddy wrote:
> > In Tegra186 PHY programming is done by BPMP-FW, so PHY calls are skipped
> > in driver. REFCLK pad settings are independent of PHY and should be
> > programmed by driver. So move REFCLK pad settings out of phy_power_on().
> > These pad settings improve REFCLK peak to peak amplitude.
> > 
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > ---
> > V3:
> > * Corrected commit log
> > V2:
> > * no change in this patch
> > 
> >  drivers/pci/host/pci-tegra.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> As far as I can tell, this is a bugfix that can and should be applied
> independently of the rest of the series.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>

Ok, I will apply - I would add a Fixes: tag though.

Is:

Fixes: cf5d31801278 ("PCI: tegra: Program PADS_REFCLK_CFG* always, not
just on legacy SoCs")

the correct commit ?

Thanks,
Lorenzo

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

[Manikanta Maddireddy]

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;
+
+	/* 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)
-- 
2.1.4

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

[Lorenzo Pieralisi]

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

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

[Manikanta Maddireddy]

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

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

[Lorenzo Pieralisi]

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

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

[Bjorn Helgaas]

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

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

[Ley Foon Tan]

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 V3 04/12] PCI: tegra: Advertise PCIe Advanced Error Reporting (AER) capability

[Manikanta Maddireddy]

Default root port settings hide AER capability. This patch enables the
advertisement of AER capability by root port.

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

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index ed5e8acfdc32..46896aaab81d 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -187,6 +187,9 @@
 #define RP_VEND_XP	0x00000f00
 #define  RP_VEND_XP_DL_UP	(1 << 30)
 
+#define RP_VEND_CTL1	0xf48
+#define  RP_VEND_CTL1_ERPT	(1 << 13)
+
 #define RP_VEND_CTL2 0x00000fa8
 #define  RP_VEND_CTL2_PCA_ENABLE (1 << 7)
 
@@ -2055,6 +2058,16 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
 		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
 }
 
+static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
+{
+	unsigned long value;
+
+	/* Enable AER capability */
+	value = readl(port->base + RP_VEND_CTL1);
+	value |= RP_VEND_CTL1_ERPT;
+	writel(value, port->base + RP_VEND_CTL1);
+}
+
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
  * can result in the increase of the bootup time as there are big timeout
@@ -2120,6 +2133,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 			 port->index, port->lanes);
 
 		tegra_pcie_port_enable(port);
+		tegra_pcie_enable_rp_features(port);
 	}
 
 	/* take the PCIe interface module out of reset */
-- 
2.1.4

Re: [PATCH V3 04/12] PCI: tegra: Advertise PCIe Advanced Error Reporting (AER) capability

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

On Mon, Oct 30, 2017 at 07:27:15PM +0530, Manikanta Maddireddy wrote:
> Default root port settings hide AER capability. This patch enables the
> advertisement of AER capability by root port.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * updated commit log
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index ed5e8acfdc32..46896aaab81d 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -187,6 +187,9 @@
>  #define RP_VEND_XP	0x00000f00
>  #define  RP_VEND_XP_DL_UP	(1 << 30)
>  
> +#define RP_VEND_CTL1	0xf48
> +#define  RP_VEND_CTL1_ERPT	(1 << 13)
> +
>  #define RP_VEND_CTL2 0x00000fa8
>  #define  RP_VEND_CTL2_PCA_ENABLE (1 << 7)
>  
> @@ -2055,6 +2058,16 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>  		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>  }
>  
> +static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
> +{
> +	unsigned long value;
> +
> +	/* Enable AER capability */
> +	value = readl(port->base + RP_VEND_CTL1);
> +	value |= RP_VEND_CTL1_ERPT;
> +	writel(value, port->base + RP_VEND_CTL1);
> +}
> +
>  /*
>   * FIXME: If there are no PCIe cards attached, then calling this function
>   * can result in the increase of the bootup time as there are big timeout
> @@ -2120,6 +2133,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  			 port->index, port->lanes);
>  
>  		tegra_pcie_port_enable(port);
> +		tegra_pcie_enable_rp_features(port);

Same as for patch 5: move this into tegra_pcie_port_enable()?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 05/12] PCI: tegra: Program UPHY electrical settings in Tegra210

[Manikanta Maddireddy]

UPHY electrical programming guidelines are documented in Tegra210 TRM.
Program these electrical settings for proper eye diagram in all link
speeds.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* Added shift operation for readability
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 46896aaab81d..c862facfd6e9 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -184,6 +184,32 @@
 
 #define AFI_PEXBIAS_CTRL_0		0x168
 
+#define RP_ECTL_2_R1	0xe84
+#define  RP_ECTL_2_R1_RX_CTLE_1C_MASK		0xffff
+
+#define RP_ECTL_4_R1	0xe8c
+#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
+#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT	16
+
+#define RP_ECTL_5_R1	0xe90
+#define  RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK	0xffffffff
+
+#define RP_ECTL_6_R1	0xe94
+#define  RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK	0xffffffff
+
+#define RP_ECTL_2_R2	0xea4
+#define  RP_ECTL_2_R2_RX_CTLE_1C_MASK		0xffff
+
+#define RP_ECTL_4_R2	0xeac
+#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
+#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT	16
+
+#define RP_ECTL_5_R2	0xeb0
+#define  RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK	0xffffffff
+
+#define RP_ECTL_6_R2	0xeb4
+#define  RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK	0xffffffff
+
 #define RP_VEND_XP	0x00000f00
 #define  RP_VEND_XP_DL_UP	(1 << 30)
 
@@ -254,6 +280,14 @@ struct tegra_pcie_soc {
 	u32 tx_ref_sel;
 	u32 pads_refclk_cfg0;
 	u32 pads_refclk_cfg1;
+	u32 rp_ectl_2_r1;
+	u32 rp_ectl_4_r1;
+	u32 rp_ectl_5_r1;
+	u32 rp_ectl_6_r1;
+	u32 rp_ectl_2_r2;
+	u32 rp_ectl_4_r2;
+	u32 rp_ectl_5_r2;
+	u32 rp_ectl_6_r2;
 	bool has_pex_clkreq_en;
 	bool has_pex_bias_ctrl;
 	bool has_intr_prsnt_sense;
@@ -261,6 +295,7 @@ struct tegra_pcie_soc {
 	bool has_gen2;
 	bool force_pca_enable;
 	bool program_uphy;
+	bool program_ectl_settings;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2058,6 +2093,52 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
 		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
 }
 
+static void tegra_pcie_program_ectl_settings(struct tegra_pcie_port *port)
+{
+	unsigned long value;
+	const struct tegra_pcie_soc *soc = port->pcie->soc;
+
+	value = readl(port->base + RP_ECTL_2_R1);
+	value &= ~RP_ECTL_2_R1_RX_CTLE_1C_MASK;
+	value |= soc->rp_ectl_2_r1;
+	writel(value, port->base + RP_ECTL_2_R1);
+
+	value = readl(port->base + RP_ECTL_4_R1);
+	value &= ~RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK;
+	value |= (soc->rp_ectl_4_r1 << RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT);
+	writel(value, port->base + RP_ECTL_4_R1);
+
+	value = readl(port->base + RP_ECTL_5_R1);
+	value &= ~RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK;
+	value |= soc->rp_ectl_5_r1;
+	writel(value, port->base + RP_ECTL_5_R1);
+
+	value = readl(port->base + RP_ECTL_6_R1);
+	value &= ~RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK;
+	value |= soc->rp_ectl_6_r1;
+	writel(value, port->base + RP_ECTL_6_R1);
+
+	value = readl(port->base + RP_ECTL_2_R2);
+	value &= ~RP_ECTL_2_R2_RX_CTLE_1C_MASK;
+	value |= soc->rp_ectl_2_r2;
+	writel(value, port->base + RP_ECTL_2_R2);
+
+	value = readl(port->base + RP_ECTL_4_R2);
+	value &= ~RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK;
+	value |= (soc->rp_ectl_4_r2 << RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT);
+	writel(value, port->base + RP_ECTL_4_R2);
+
+	value = readl(port->base + RP_ECTL_5_R2);
+	value &= ~RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK;
+	value |= soc->rp_ectl_5_r2;
+	writel(value, port->base + RP_ECTL_5_R2);
+
+	value = readl(port->base + RP_ECTL_6_R2);
+	value &= ~RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK;
+	value |= soc->rp_ectl_6_r2;
+	writel(value, port->base + RP_ECTL_6_R2);
+}
+
 static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
 {
 	unsigned long value;
@@ -2125,6 +2206,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct tegra_pcie_port *port, *tmp;
+	const struct tegra_pcie_soc *soc = pcie->soc;
 
 	tegra_pcie_apply_pad_settings(pcie);
 
@@ -2133,6 +2215,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 			 port->index, port->lanes);
 
 		tegra_pcie_port_enable(port);
+		if (soc->program_ectl_settings)
+			tegra_pcie_program_ectl_settings(port);
 		tegra_pcie_enable_rp_features(port);
 	}
 
@@ -2199,6 +2283,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
+	.program_ectl_settings = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2215,6 +2300,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.has_gen2 = false,
 	.force_pca_enable = false,
 	.program_uphy = true,
+	.program_ectl_settings = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2230,6 +2316,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = true,
+	.program_ectl_settings = false,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2238,6 +2325,14 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.pads_pll_ctl = PADS_PLL_CTL_TEGRA30,
 	.tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN,
 	.pads_refclk_cfg0 = 0x90b890b8,
+	.rp_ectl_2_r1 = 0x0000000f,
+	.rp_ectl_4_r1 = 0x00000067,
+	.rp_ectl_5_r1 = 0x55010000,
+	.rp_ectl_6_r1 = 0x00000001,
+	.rp_ectl_2_r2 = 0x0000008f,
+	.rp_ectl_4_r2 = 0x000000c7,
+	.rp_ectl_5_r2 = 0x55010000,
+	.rp_ectl_6_r2 = 0x00000001,
 	.has_pex_clkreq_en = true,
 	.has_pex_bias_ctrl = true,
 	.has_intr_prsnt_sense = true,
@@ -2245,6 +2340,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = true,
 	.program_uphy = true,
+	.program_ectl_settings = true,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2261,6 +2357,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.has_gen2 = true,
 	.force_pca_enable = false,
 	.program_uphy = false,
+	.program_ectl_settings = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.1.4

Re: [PATCH V3 05/12] PCI: tegra: Program UPHY electrical settings in Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5338 bytes --]

On Mon, Oct 30, 2017 at 07:27:16PM +0530, Manikanta Maddireddy wrote:
> UPHY electrical programming guidelines are documented in Tegra210 TRM.
> Program these electrical settings for proper eye diagram in all link
> speeds.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * Added shift operation for readability
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 46896aaab81d..c862facfd6e9 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -184,6 +184,32 @@
>  
>  #define AFI_PEXBIAS_CTRL_0		0x168
>  
> +#define RP_ECTL_2_R1	0xe84
> +#define  RP_ECTL_2_R1_RX_CTLE_1C_MASK		0xffff
> +
> +#define RP_ECTL_4_R1	0xe8c
> +#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
> +#define  RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT	16
> +
> +#define RP_ECTL_5_R1	0xe90
> +#define  RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_6_R1	0xe94
> +#define  RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_2_R2	0xea4
> +#define  RP_ECTL_2_R2_RX_CTLE_1C_MASK		0xffff
> +
> +#define RP_ECTL_4_R2	0xeac
> +#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK	(0xffff << 16)
> +#define  RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT	16
> +
> +#define RP_ECTL_5_R2	0xeb0
> +#define  RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK	0xffffffff
> +
> +#define RP_ECTL_6_R2	0xeb4
> +#define  RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK	0xffffffff
> +
>  #define RP_VEND_XP	0x00000f00
>  #define  RP_VEND_XP_DL_UP	(1 << 30)
>  
> @@ -254,6 +280,14 @@ struct tegra_pcie_soc {
>  	u32 tx_ref_sel;
>  	u32 pads_refclk_cfg0;
>  	u32 pads_refclk_cfg1;
> +	u32 rp_ectl_2_r1;
> +	u32 rp_ectl_4_r1;
> +	u32 rp_ectl_5_r1;
> +	u32 rp_ectl_6_r1;
> +	u32 rp_ectl_2_r2;
> +	u32 rp_ectl_4_r2;
> +	u32 rp_ectl_5_r2;
> +	u32 rp_ectl_6_r2;
>  	bool has_pex_clkreq_en;
>  	bool has_pex_bias_ctrl;
>  	bool has_intr_prsnt_sense;
> @@ -261,6 +295,7 @@ struct tegra_pcie_soc {
>  	bool has_gen2;
>  	bool force_pca_enable;
>  	bool program_uphy;
> +	bool program_ectl_settings;

Could you move all of the ECTL related fields under a substructure for
better readability? Something like:

	struct {
		struct {
			u32 ...;
			...
		} regs;

		bool enable;
	} ectl;

>  };
>  
>  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> @@ -2058,6 +2093,52 @@ static void tegra_pcie_apply_pad_settings(struct tegra_pcie *pcie)
>  		pads_writel(pcie, soc->pads_refclk_cfg1, PADS_REFCLK_CFG1);
>  }
>  
> +static void tegra_pcie_program_ectl_settings(struct tegra_pcie_port *port)
> +{
> +	unsigned long value;

u32, please.

> +	const struct tegra_pcie_soc *soc = port->pcie->soc;
> +
> +	value = readl(port->base + RP_ECTL_2_R1);
> +	value &= ~RP_ECTL_2_R1_RX_CTLE_1C_MASK;
> +	value |= soc->rp_ectl_2_r1;
> +	writel(value, port->base + RP_ECTL_2_R1);
> +
> +	value = readl(port->base + RP_ECTL_4_R1);
> +	value &= ~RP_ECTL_4_R1_RX_CDR_CTRL_1C_MASK;
> +	value |= (soc->rp_ectl_4_r1 << RP_ECTL_4_R1_RX_CDR_CTRL_1C_SHIFT);
> +	writel(value, port->base + RP_ECTL_4_R1);
> +
> +	value = readl(port->base + RP_ECTL_5_R1);
> +	value &= ~RP_ECTL_5_R1_RX_EQ_CTRL_L_1C_MASK;
> +	value |= soc->rp_ectl_5_r1;
> +	writel(value, port->base + RP_ECTL_5_R1);
> +
> +	value = readl(port->base + RP_ECTL_6_R1);
> +	value &= ~RP_ECTL_6_R1_RX_EQ_CTRL_H_1C_MASK;
> +	value |= soc->rp_ectl_6_r1;
> +	writel(value, port->base + RP_ECTL_6_R1);
> +
> +	value = readl(port->base + RP_ECTL_2_R2);
> +	value &= ~RP_ECTL_2_R2_RX_CTLE_1C_MASK;
> +	value |= soc->rp_ectl_2_r2;
> +	writel(value, port->base + RP_ECTL_2_R2);
> +
> +	value = readl(port->base + RP_ECTL_4_R2);
> +	value &= ~RP_ECTL_4_R2_RX_CDR_CTRL_1C_MASK;
> +	value |= (soc->rp_ectl_4_r2 << RP_ECTL_4_R2_RX_CDR_CTRL_1C_SHIFT);
> +	writel(value, port->base + RP_ECTL_4_R2);
> +
> +	value = readl(port->base + RP_ECTL_5_R2);
> +	value &= ~RP_ECTL_5_R2_RX_EQ_CTRL_L_1C_MASK;
> +	value |= soc->rp_ectl_5_r2;
> +	writel(value, port->base + RP_ECTL_5_R2);
> +
> +	value = readl(port->base + RP_ECTL_6_R2);
> +	value &= ~RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK;
> +	value |= soc->rp_ectl_6_r2;
> +	writel(value, port->base + RP_ECTL_6_R2);
> +}
> +
>  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>  {
>  	unsigned long value;
> @@ -2125,6 +2206,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  {
>  	struct device *dev = pcie->dev;
>  	struct tegra_pcie_port *port, *tmp;
> +	const struct tegra_pcie_soc *soc = pcie->soc;
>  
>  	tegra_pcie_apply_pad_settings(pcie);
>  
> @@ -2133,6 +2215,8 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  			 port->index, port->lanes);
>  
>  		tegra_pcie_port_enable(port);
> +		if (soc->program_ectl_settings)
> +			tegra_pcie_program_ectl_settings(port);

Perhaps this should be moved into tegra_pcie_port_enable()? That way the
programming is done in one central location. If ever we need to enable
the ports from somewhere else we could simply call it instead of having
to duplicate enabling all the extra features.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 06/12] PCI: tegra: Enable opportunistic update FC and ACK

[Manikanta Maddireddy]

This patch ensures that DL sends pending ACKs and update FC packets when
link is idle instead of waiting for timers to expire which improves PCIe
bandwidth.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* no change in this patch
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c862facfd6e9..4562b0c113a8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -211,7 +211,9 @@
 #define  RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK	0xffffffff
 
 #define RP_VEND_XP	0x00000f00
-#define  RP_VEND_XP_DL_UP	(1 << 30)
+#define  RP_VEND_XP_DL_UP			(1 << 30)
+#define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
+#define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
 
 #define RP_VEND_CTL1	0xf48
 #define  RP_VEND_CTL1_ERPT	(1 << 13)
@@ -2149,6 +2151,16 @@ static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
 	writel(value, port->base + RP_VEND_CTL1);
 }
 
+static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
+{
+	unsigned long value;
+
+	/* Optimal settings to enhance bandwidth */
+	value = readl(port->base + RP_VEND_XP);
+	value |= RP_VEND_XP_OPPORTUNISTIC_ACK;
+	value |= RP_VEND_XP_OPPORTUNISTIC_UPDATEFC;
+	writel(value, port->base + RP_VEND_XP);
+}
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
  * can result in the increase of the bootup time as there are big timeout
@@ -2218,6 +2230,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 		if (soc->program_ectl_settings)
 			tegra_pcie_program_ectl_settings(port);
 		tegra_pcie_enable_rp_features(port);
+		tegra_pcie_apply_sw_fixup(port);
 	}
 
 	/* take the PCIe interface module out of reset */
-- 
2.1.4

Re: [PATCH V3 06/12] PCI: tegra: Enable opportunistic update FC and ACK

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On Mon, Oct 30, 2017 at 07:27:17PM +0530, Manikanta Maddireddy wrote:
> This patch ensures that DL sends pending ACKs and update FC packets when
> link is idle instead of waiting for timers to expire which improves PCIe
> bandwidth.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * no change in this patch
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index c862facfd6e9..4562b0c113a8 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -211,7 +211,9 @@
>  #define  RP_ECTL_6_R2_RX_EQ_CTRL_H_1C_MASK	0xffffffff
>  
>  #define RP_VEND_XP	0x00000f00
> -#define  RP_VEND_XP_DL_UP	(1 << 30)
> +#define  RP_VEND_XP_DL_UP			(1 << 30)
> +#define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
> +#define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  
>  #define RP_VEND_CTL1	0xf48
>  #define  RP_VEND_CTL1_ERPT	(1 << 13)
> @@ -2149,6 +2151,16 @@ static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>  	writel(value, port->base + RP_VEND_CTL1);
>  }
>  
> +static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)

Why not make the name reflect what the function does?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 07/12] PCI: tegra: Disable AFI dynamic clock gating

[Manikanta Maddireddy]

When there are 32 outstanding writes from AFI to memory, the outstanding
write counter overflows and indicates that there are "0" outstanding write
transactions. This outstanding write counter is used to generate IDLE
signal to dynamically gate the AFI clock.

When memory controller is under heavy load, its possible that write
completions will come back to AFI after long delay and AFI write counter
overflows. AFI clock gets gated even when there are outstanding
transactions towards memory controller resutling in system hang.

Disable dynamic clock gating of AFI clock to avoid system hang.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* no change in this patch
V2:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 4562b0c113a8..c264037112cb 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -103,8 +103,9 @@
 #define AFI_MSI_EN_VEC6		0xa4
 #define AFI_MSI_EN_VEC7		0xa8
 
-#define AFI_CONFIGURATION		0xac
-#define  AFI_CONFIGURATION_EN_FPCI	(1 << 0)
+#define AFI_CONFIGURATION			0xac
+#define  AFI_CONFIGURATION_EN_FPCI		(1 << 0)
+#define  AFI_CONFIGURATION_CLKEN_OVERRIDE	(1 << 31)
 
 #define AFI_FPCI_ERROR_MASKS	0xb0
 
@@ -1059,9 +1060,10 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
 		}
 	}
 
-	/* finally enable PCIe */
+	/* Disable AFI dynamic clock gating and enable PCIe */
 	value = afi_readl(pcie, AFI_CONFIGURATION);
-	value |= AFI_CONFIGURATION_EN_FPCI;
+	value |= (AFI_CONFIGURATION_EN_FPCI |
+			AFI_CONFIGURATION_CLKEN_OVERRIDE);
 	afi_writel(pcie, value, AFI_CONFIGURATION);
 
 	value = AFI_INTR_EN_INI_SLVERR | AFI_INTR_EN_INI_DECERR |
-- 
2.1.4

Re: [PATCH V3 07/12] PCI: tegra: Disable AFI dynamic clock gating

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On Mon, Oct 30, 2017 at 07:27:18PM +0530, Manikanta Maddireddy wrote:
> When there are 32 outstanding writes from AFI to memory, the outstanding
> write counter overflows and indicates that there are "0" outstanding write
> transactions. This outstanding write counter is used to generate IDLE
> signal to dynamically gate the AFI clock.
> 
> When memory controller is under heavy load, its possible that write
> completions will come back to AFI after long delay and AFI write counter
> overflows. AFI clock gets gated even when there are outstanding
> transactions towards memory controller resutling in system hang.
> 
> Disable dynamic clock gating of AFI clock to avoid system hang.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * no change in this patch
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 4562b0c113a8..c264037112cb 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -103,8 +103,9 @@
>  #define AFI_MSI_EN_VEC6		0xa4
>  #define AFI_MSI_EN_VEC7		0xa8
>  
> -#define AFI_CONFIGURATION		0xac
> -#define  AFI_CONFIGURATION_EN_FPCI	(1 << 0)
> +#define AFI_CONFIGURATION			0xac
> +#define  AFI_CONFIGURATION_EN_FPCI		(1 << 0)
> +#define  AFI_CONFIGURATION_CLKEN_OVERRIDE	(1 << 31)
>  
>  #define AFI_FPCI_ERROR_MASKS	0xb0
>  
> @@ -1059,9 +1060,10 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie)
>  		}
>  	}
>  
> -	/* finally enable PCIe */
> +	/* Disable AFI dynamic clock gating and enable PCIe */
>  	value = afi_readl(pcie, AFI_CONFIGURATION);
> -	value |= AFI_CONFIGURATION_EN_FPCI;
> +	value |= (AFI_CONFIGURATION_EN_FPCI |
> +			AFI_CONFIGURATION_CLKEN_OVERRIDE);

I think this is cleaner (and results in a smaller diff) if you simply
put this on a new line:

	value |= AFI_CONFIGURATION_CLKEN_OVERRIDE;

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 08/12] PCI: tegra: Wait for DLLP to finish before entering L1 or L2

[Manikanta Maddireddy]

Set required bit to have LTSSM wait for DLLP to finish before entering L1
or L2. This avoids truncation of PM messages which results in receiver
errors.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* no change in this patch
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c264037112cb..34740a7033f7 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -219,6 +219,9 @@
 #define RP_VEND_CTL1	0xf48
 #define  RP_VEND_CTL1_ERPT	(1 << 13)
 
+#define RP_VEND_XP_BIST	0xf4c
+#define  RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE	(1 << 28)
+
 #define RP_VEND_CTL2 0x00000fa8
 #define  RP_VEND_CTL2_PCA_ENABLE (1 << 7)
 
@@ -2162,6 +2165,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 	value |= RP_VEND_XP_OPPORTUNISTIC_ACK;
 	value |= RP_VEND_XP_OPPORTUNISTIC_UPDATEFC;
 	writel(value, port->base + RP_VEND_XP);
+
+	/* LTSSM will wait for DLLP to finish before entering L1 or L2,
+	 * to avoid truncation of PM messages which results in receiver errors
+	 */
+	value = readl(port->base + RP_VEND_XP_BIST);
+	value |= RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE;
+	writel(value, port->base + RP_VEND_XP_BIST);
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
-- 
2.1.4

Re: [PATCH V3 08/12] PCI: tegra: Wait for DLLP to finish before entering L1 or L2

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Mon, Oct 30, 2017 at 07:27:19PM +0530, Manikanta Maddireddy wrote:
> Set required bit to have LTSSM wait for DLLP to finish before entering L1
> or L2. This avoids truncation of PM messages which results in receiver
> errors.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * no change in this patch
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index c264037112cb..34740a7033f7 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -219,6 +219,9 @@
>  #define RP_VEND_CTL1	0xf48
>  #define  RP_VEND_CTL1_ERPT	(1 << 13)
>  
> +#define RP_VEND_XP_BIST	0xf4c
> +#define  RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE	(1 << 28)
> +
>  #define RP_VEND_CTL2 0x00000fa8
>  #define  RP_VEND_CTL2_PCA_ENABLE (1 << 7)
>  
> @@ -2162,6 +2165,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  	value |= RP_VEND_XP_OPPORTUNISTIC_ACK;
>  	value |= RP_VEND_XP_OPPORTUNISTIC_UPDATEFC;
>  	writel(value, port->base + RP_VEND_XP);
> +
> +	/* LTSSM will wait for DLLP to finish before entering L1 or L2,
> +	 * to avoid truncation of PM messages which results in receiver errors
> +	 */

Block comment style, please.

I'm still not sure if tegra_pcie_apply_sw_fixup() is a good name for
this, even it's now becoming clear why you have a separate function.

These aren't really SW fixups, are they? Why not stash these into the
tegra_pcie_enable_rp_features() function?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 09/12] PCI: tegra: Enable PCIe xclk clock clamping

[Manikanta Maddireddy]

This patch enables PCIe xlck clock clamping by pad control. Pad control
asserts UPHY lane sleep signal when L1 entry signal received from PCIe.
UPHY sleep signal assertion is done per lane. Default clamp threshold
margin is not enough to assert all UPHY lane sleep signals. Increase
the clamp threshold in Tegra124, 132, 210 and 186.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* Parentheses removed for OR operation
V2:
* no change in this patch

 drivers/pci/host/pci-tegra.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 34740a7033f7..9f13e6fcc64e 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -226,8 +226,14 @@
 #define  RP_VEND_CTL2_PCA_ENABLE (1 << 7)
 
 #define RP_PRIV_MISC	0x00000fe0
-#define  RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT (0xe << 0)
-#define  RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT (0xf << 0)
+#define  RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT		(0xe << 0)
+#define  RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT		(0xf << 0)
+#define  RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD_MASK	(0x7f << 16)
+#define  RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD		(0xf << 16)
+#define  RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE		(1 << 23)
+#define  RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD_MASK	(0x7f << 24)
+#define  RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD		(0xf << 24)
+#define  RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE		(1 << 31)
 
 #define RP_LINK_CONTROL_STATUS			0x00000090
 #define  RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE	0x20000000
@@ -302,6 +308,7 @@ struct tegra_pcie_soc {
 	bool force_pca_enable;
 	bool program_uphy;
 	bool program_ectl_settings;
+	bool update_clamp_threshold;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2158,6 +2165,7 @@ static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
 
 static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 {
+	const struct tegra_pcie_soc *soc = port->pcie->soc;
 	unsigned long value;
 
 	/* Optimal settings to enhance bandwidth */
@@ -2172,6 +2180,17 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 	value = readl(port->base + RP_VEND_XP_BIST);
 	value |= RP_VEND_XP_BIST_GOTO_L1_L2_AFTER_DLLP_DONE;
 	writel(value, port->base + RP_VEND_XP_BIST);
+
+	value = readl(port->base + RP_PRIV_MISC);
+	value |= RP_PRIV_MISC_CTLR_CLK_CLAMP_ENABLE |
+			RP_PRIV_MISC_TMS_CLK_CLAMP_ENABLE;
+	if (soc->update_clamp_threshold) {
+		value &= ~(RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD_MASK |
+				RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD_MASK);
+		value |= RP_PRIV_MISC_CTLR_CLK_CLAMP_THRESHOLD |
+				RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD;
+	}
+	writel(value, port->base + RP_PRIV_MISC);
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
@@ -2309,6 +2328,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.force_pca_enable = false,
 	.program_uphy = true,
 	.program_ectl_settings = false,
+	.update_clamp_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2326,6 +2346,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.force_pca_enable = false,
 	.program_uphy = true,
 	.program_ectl_settings = false,
+	.update_clamp_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2342,6 +2363,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.force_pca_enable = false,
 	.program_uphy = true,
 	.program_ectl_settings = false,
+	.update_clamp_threshold = true,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2366,6 +2388,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.force_pca_enable = true,
 	.program_uphy = true,
 	.program_ectl_settings = true,
+	.update_clamp_threshold = true,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2383,6 +2406,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.force_pca_enable = false,
 	.program_uphy = false,
 	.program_ectl_settings = false,
+	.update_clamp_threshold = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.1.4

Re: [PATCH V3 09/12] PCI: tegra: Enable PCIe xclk clock clamping

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 167 bytes --]

On Mon, Oct 30, 2017 at 07:27:20PM +0530, Manikanta Maddireddy wrote:
> This patch enables PCIe xlck clock clamping by pad control. Pad control

xlck -> xclk

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 10/12] PCI: tegra: Add SW fixup for RAW violations

[Manikanta Maddireddy]

The logic which blocks read requests till AFI gets ACK for all outstanding
MC writes does not behave correctly when number of outstanding write
becomes more than 32.

SW fixup to prevent this issue is to limit outstanding posted writes and
tweak updateFC timer threshold.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* changed soc parameter name
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 9f13e6fcc64e..3993e9221c96 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -185,6 +185,13 @@
 
 #define AFI_PEXBIAS_CTRL_0		0x168
 
+#define RP_PRIV_XP_DL	0x494
+#define  RP_PRIV_XP_DL_GEN2_UPD_FC_TSHOLD	(0x1ff << 1)
+
+#define RP_RX_HDR_LIMIT	0xe00
+#define  RP_RX_HDR_LIMIT_PW_MASK	(0xff << 8)
+#define  RP_RX_HDR_LIMIT_PW		(0x0e << 8)
+
 #define RP_ECTL_2_R1	0xe84
 #define  RP_ECTL_2_R1_RX_CTLE_1C_MASK		0xffff
 
@@ -215,6 +222,7 @@
 #define  RP_VEND_XP_DL_UP			(1 << 30)
 #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
 #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
+#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
 
 #define RP_VEND_CTL1	0xf48
 #define  RP_VEND_CTL1_ERPT	(1 << 13)
@@ -309,6 +317,7 @@ struct tegra_pcie_soc {
 	bool program_uphy;
 	bool program_ectl_settings;
 	bool update_clamp_threshold;
+	bool raw_violation_fixup;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2191,6 +2200,22 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 				RP_PRIV_MISC_TMS_CLK_CLAMP_THRESHOLD;
 	}
 	writel(value, port->base + RP_PRIV_MISC);
+
+	/* Fixup for read after write violation in T124 & T132 platforms */
+	if (soc->raw_violation_fixup) {
+		value = readl(port->base + RP_RX_HDR_LIMIT);
+		value &= ~RP_RX_HDR_LIMIT_PW_MASK;
+		value |= RP_RX_HDR_LIMIT_PW;
+		writel(value, port->base + RP_RX_HDR_LIMIT);
+
+		value = readl(port->base + RP_PRIV_XP_DL);
+		value |= RP_PRIV_XP_DL_GEN2_UPD_FC_TSHOLD;
+		writel(value, port->base + RP_PRIV_XP_DL);
+
+		value = readl(port->base + RP_VEND_XP);
+		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
+		writel(value, port->base + RP_VEND_XP);
+	}
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
@@ -2329,6 +2354,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.program_uphy = true,
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
+	.raw_violation_fixup = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2347,6 +2373,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.program_uphy = true,
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
+	.raw_violation_fixup = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2364,6 +2391,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.program_uphy = true,
 	.program_ectl_settings = false,
 	.update_clamp_threshold = true,
+	.raw_violation_fixup = true,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2389,6 +2417,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.program_uphy = true,
 	.program_ectl_settings = true,
 	.update_clamp_threshold = true,
+	.raw_violation_fixup = false,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2407,6 +2436,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.program_uphy = false,
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
+	.raw_violation_fixup = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.1.4

Re: [PATCH V3 10/12] PCI: tegra: Add SW fixup for RAW violations

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Mon, Oct 30, 2017 at 07:27:21PM +0530, Manikanta Maddireddy wrote:
> The logic which blocks read requests till AFI gets ACK for all outstanding
> MC writes does not behave correctly when number of outstanding write
> becomes more than 32.
> 
> SW fixup to prevent this issue is to limit outstanding posted writes and
> tweak updateFC timer threshold.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * changed soc parameter name
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 11/12] PCI: tegra: Increase the deskew retry time

[Manikanta Maddireddy]

Some times Gen2 to Gen1 link speed switching fails due to instability in
deskew logic on lane0 in Tegra210. Increase the deskew retry time to
resolve this issue.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* no change in this patch
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 3993e9221c96..b29329226e3d 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -224,6 +224,10 @@
 #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
 #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
 
+#define RP_VEND_CTL0	0xf44
+#define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
+#define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH	(0x9 << 12)
+
 #define RP_VEND_CTL1	0xf48
 #define  RP_VEND_CTL1_ERPT	(1 << 13)
 
@@ -318,6 +322,7 @@ struct tegra_pcie_soc {
 	bool program_ectl_settings;
 	bool update_clamp_threshold;
 	bool raw_violation_fixup;
+	bool program_deskew_time;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2216,6 +2221,16 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
 		writel(value, port->base + RP_VEND_XP);
 	}
+
+	/* Tune deskew retry time to take care of Gen2 -> Gen1
+	 * link speed change error in corner cases
+	 */
+	if (soc->program_deskew_time) {
+		value = readl(port->base + RP_VEND_CTL0);
+		value &= ~RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK;
+		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
+		writel(value, port->base + RP_VEND_CTL0);
+	}
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
@@ -2355,6 +2370,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
+	.program_deskew_time = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2374,6 +2390,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
+	.program_deskew_time = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2392,6 +2409,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.program_ectl_settings = false,
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = true,
+	.program_deskew_time = false,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2418,6 +2436,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.program_ectl_settings = true,
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = false,
+	.program_deskew_time = true,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2437,6 +2456,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.program_ectl_settings = false,
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
+	.program_deskew_time = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.1.4

Re: [PATCH V3 11/12] PCI: tegra: Increase the deskew retry time

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Mon, Oct 30, 2017 at 07:27:22PM +0530, Manikanta Maddireddy wrote:
> Some times Gen2 to Gen1 link speed switching fails due to instability in
> deskew logic on lane0 in Tegra210. Increase the deskew retry time to
> resolve this issue.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * no change in this patch
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 3993e9221c96..b29329226e3d 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -224,6 +224,10 @@
>  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
>  
> +#define RP_VEND_CTL0	0xf44
> +#define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> +#define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH	(0x9 << 12)
> +
>  #define RP_VEND_CTL1	0xf48
>  #define  RP_VEND_CTL1_ERPT	(1 << 13)
>  
> @@ -318,6 +322,7 @@ struct tegra_pcie_soc {
>  	bool program_ectl_settings;
>  	bool update_clamp_threshold;
>  	bool raw_violation_fixup;
> +	bool program_deskew_time;
>  };
>  
>  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> @@ -2216,6 +2221,16 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
>  		writel(value, port->base + RP_VEND_XP);
>  	}
> +
> +	/* Tune deskew retry time to take care of Gen2 -> Gen1
> +	 * link speed change error in corner cases
> +	 */

Wrong block comment style. With that fixed:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 12/12] PCI: tegra: Update flow control threshold in Tegra210

[Manikanta Maddireddy]

Recommended update FC threshold in Tegra210 is 0x60 for best performance
of x1 link. Setting this to 0x60 provides the best balance between number
of UpdateFC and read data sent over the link.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
---
V3:
* changed soc parameter name
V2:
* no change in this patch

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

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index b29329226e3d..812d32cfdd0e 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -223,6 +223,7 @@
 #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
 #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
 #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
+#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
 
 #define RP_VEND_CTL0	0xf44
 #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
@@ -323,6 +324,7 @@ struct tegra_pcie_soc {
 	bool update_clamp_threshold;
 	bool raw_violation_fixup;
 	bool program_deskew_time;
+	bool update_fc_threshold;
 };
 
 static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
@@ -2231,6 +2233,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
 		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
 		writel(value, port->base + RP_VEND_CTL0);
 	}
+
+	if (soc->update_fc_threshold) {
+		value = readl(port->base + RP_VEND_XP);
+		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
+		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
+		writel(value, port->base + RP_VEND_XP);
+	}
 }
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
@@ -2371,6 +2380,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra30_pcie = {
@@ -2391,6 +2401,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra124_pcie = {
@@ -2410,6 +2421,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = true,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct tegra_pcie_soc tegra210_pcie = {
@@ -2437,6 +2449,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
 	.update_clamp_threshold = true,
 	.raw_violation_fixup = false,
 	.program_deskew_time = true,
+	.update_fc_threshold = true,
 };
 
 static const struct tegra_pcie_soc tegra186_pcie = {
@@ -2457,6 +2470,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
 	.update_clamp_threshold = false,
 	.raw_violation_fixup = false,
 	.program_deskew_time = false,
+	.update_fc_threshold = false,
 };
 
 static const struct of_device_id tegra_pcie_of_match[] = {
-- 
2.1.4

Re: [PATCH V3 12/12] PCI: tegra: Update flow control threshold in Tegra210

[Lorenzo Pieralisi]

On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> Recommended update FC threshold in Tegra210 is 0x60 for best performance
> of x1 link. Setting this to 0x60 provides the best balance between number
> of UpdateFC and read data sent over the link.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * changed soc parameter name
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index b29329226e3d..812d32cfdd0e 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -223,6 +223,7 @@
>  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
>  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)

You define a SOC specific threshold and a update_fc_threshold bool
variable to update it ? And what are you going to do if that's needed
on something that it is not a T210 ? Should not this be a(nother)
struct tegra_pcie_soc parameter instead than a macro ?

Not that I am happy about it but this deviates from the current
approach.

>  #define RP_VEND_CTL0	0xf44
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
>  	bool update_clamp_threshold;
>  	bool raw_violation_fixup;
>  	bool program_deskew_time;
> +	bool update_fc_threshold;
>  };
>  
>  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> @@ -2231,6 +2233,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
>  		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
>  		writel(value, port->base + RP_VEND_CTL0);
>  	}
> +
> +	if (soc->update_fc_threshold) {
> +		value = readl(port->base + RP_VEND_XP);
> +		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
> +		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
> +		writel(value, port->base + RP_VEND_XP);
> +	}

If, say, a platform requires update_fc_threshold and raw_violation_fixup
what takes precedence (ie they required programming the _same_
registers) ? update_fc_threshold takes precedence, since it is applied
last - but I would like you to think about this and realize that this
per-SoC mechanism does not scale anymore.

You should a) enforce some firmware initialization - most of the
parameters in struct tegra_pcie_soc could have been pre-programmed
by FW and b) think about adding some DT properties to handle the PCI
host bridge set-up.

Lorenzo

>  }
>  /*
>   * FIXME: If there are no PCIe cards attached, then calling this function
> @@ -2371,6 +2380,7 @@ static const struct tegra_pcie_soc tegra20_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra30_pcie = {
> @@ -2391,6 +2401,7 @@ static const struct tegra_pcie_soc tegra30_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra124_pcie = {
> @@ -2410,6 +2421,7 @@ static const struct tegra_pcie_soc tegra124_pcie = {
>  	.update_clamp_threshold = true,
>  	.raw_violation_fixup = true,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct tegra_pcie_soc tegra210_pcie = {
> @@ -2437,6 +2449,7 @@ static const struct tegra_pcie_soc tegra210_pcie = {
>  	.update_clamp_threshold = true,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = true,
> +	.update_fc_threshold = true,
>  };
>  
>  static const struct tegra_pcie_soc tegra186_pcie = {
> @@ -2457,6 +2470,7 @@ static const struct tegra_pcie_soc tegra186_pcie = {
>  	.update_clamp_threshold = false,
>  	.raw_violation_fixup = false,
>  	.program_deskew_time = false,
> +	.update_fc_threshold = false,
>  };
>  
>  static const struct of_device_id tegra_pcie_of_match[] = {
> -- 
> 2.1.4
> 

Re: [PATCH V3 12/12] PCI: tegra: Update flow control threshold in Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On Tue, Dec 12, 2017 at 05:43:29PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> > Recommended update FC threshold in Tegra210 is 0x60 for best performance
> > of x1 link. Setting this to 0x60 provides the best balance between number
> > of UpdateFC and read data sent over the link.
> > 
> > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> > ---
> > V3:
> > * changed soc parameter name
> > V2:
> > * no change in this patch
> > 
> >  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index b29329226e3d..812d32cfdd0e 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -223,6 +223,7 @@
> >  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
> >  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
> >  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> > +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
> 
> You define a SOC specific threshold and a update_fc_threshold bool
> variable to update it ? And what are you going to do if that's needed
> on something that it is not a T210 ? Should not this be a(nother)
> struct tegra_pcie_soc parameter instead than a macro ?
> 
> Not that I am happy about it but this deviates from the current
> approach.
> 
> >  #define RP_VEND_CTL0	0xf44
> >  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> > @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
> >  	bool update_clamp_threshold;
> >  	bool raw_violation_fixup;
> >  	bool program_deskew_time;
> > +	bool update_fc_threshold;
> >  };
> >  
> >  static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> > @@ -2231,6 +2233,13 @@ static void tegra_pcie_apply_sw_fixup(struct tegra_pcie_port *port)
> >  		value |= RP_VEND_CTL0_DSK_RST_PULSE_WIDTH;
> >  		writel(value, port->base + RP_VEND_CTL0);
> >  	}
> > +
> > +	if (soc->update_fc_threshold) {
> > +		value = readl(port->base + RP_VEND_XP);
> > +		value &= ~RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK;
> > +		value |= RP_VEND_XP_UPDATE_FC_THRESHOLD_T210;
> > +		writel(value, port->base + RP_VEND_XP);
> > +	}
> 
> If, say, a platform requires update_fc_threshold and raw_violation_fixup
> what takes precedence (ie they required programming the _same_
> registers) ? update_fc_threshold takes precedence, since it is applied
> last - but I would like you to think about this and realize that this
> per-SoC mechanism does not scale anymore.
> 
> You should a) enforce some firmware initialization - most of the
> parameters in struct tegra_pcie_soc could have been pre-programmed
> by FW and

I don't think that's going to work. We reset the hardware during probe
(and a subsequent patchset will make it possible for this driver to be
a loadable module, which will power down the unit on driver removal or
even runtime PM suspend), so any firmware initialization will be lost
after that point and the driver has to reprogram all the registers.

> b) think about adding some DT properties to handle the PCI host bridge
> set-up.

That isn't going to solve the problem of precedence that you bring up
above. Where the data is coming from doesn't change that the order of
execution needs to be defined in the code.

Also, all of the parameters that we have in the struct tegra_pcie_soc
are SoC specific and therefore implied by the compatible string, which
is also why the current approach was chosen back at the time. The
reasoning was that if the property defines the programming model, then
it will be implied by the compatible string. If we moved that data out
into the device tree, we essentially wind up with the same problem, in
a different location. We'd still need to specify the data on a per-SoC
basis in device tree.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V3 12/12] PCI: tegra: Update flow control threshold in Tegra210

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Mon, Oct 30, 2017 at 07:27:23PM +0530, Manikanta Maddireddy wrote:
> Recommended update FC threshold in Tegra210 is 0x60 for best performance
> of x1 link. Setting this to 0x60 provides the best balance between number
> of UpdateFC and read data sent over the link.
> 
> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> ---
> V3:
> * changed soc parameter name
> V2:
> * no change in this patch
> 
>  drivers/pci/host/pci-tegra.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index b29329226e3d..812d32cfdd0e 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -223,6 +223,7 @@
>  #define  RP_VEND_XP_OPPORTUNISTIC_ACK		(1 << 27)
>  #define  RP_VEND_XP_OPPORTUNISTIC_UPDATEFC	(1 << 28)
>  #define  RP_VEND_XP_UPDATE_FC_THRESHOLD_MASK	(0xff << 18)
> +#define  RP_VEND_XP_UPDATE_FC_THRESHOLD_T210	(0x60 << 18)
>  
>  #define RP_VEND_CTL0	0xf44
>  #define  RP_VEND_CTL0_DSK_RST_PULSE_WIDTH_MASK	(0xf << 12)
> @@ -323,6 +324,7 @@ struct tegra_pcie_soc {
>  	bool update_clamp_threshold;
>  	bool raw_violation_fixup;
>  	bool program_deskew_time;
> +	bool update_fc_threshold;
>  };

I agree with Lorenzo here that the threshold value should be a separate
field in struct tegra_pcie_soc.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V3 00/12] Enable Tegra root port features and apply SW fixups

[Manikanta Maddireddy]

Hi Bjorn, Thierry,

Could you please review this series of patches?

On 30-Oct-17 7:27 PM, Manikanta Maddireddy wrote:
> These series of patches does the following things,
> - Deasserting pcie_xrst after programming root port to make sure that
>   register programming is reflected during LTSSM
> - Apply REFCLK pad settings to make sure P2P amplitude requirement is met
> - Enable Gen2 link speed
> - Advertise AER capability
> - Program UPHY electrical settings for meeting eye diagram requirements
> - Bunch of SW fixups explained in their respective commit log
> 
> Testing done on Tegra124, 210 and 186:
> - PCIe link up, config read, BAR read and basic functionality of Ethernet
>   card
> - Link speed switch to Gen2 after link retrain
> - Link speed stays in Gen1 after retrain if end point is only Gen1 capable
> - Simulated AER errors and verified dmesg logs for them
> - Rest of the programming is verified by dumping the registers after PCIe
> link up
> 
> Manikanta Maddireddy (12):
>   PCI: tegra: Start LTSSM after programming root port
>   PCI: tegra: Move REFCLK pad settings out of phy_power_on()
>   PCI: tegra: Retrain link for Gen2 speed
>   PCI: tegra: Advertise AER capability
>   PCI: tegra: Program UPHY electrical settings in Tegra210
>   PCI: tegra: Enable opportunistic update FC and ACK
>   PCI: tegra: Disable AFI dynamic clock gating
>   PCI: tegra: Wait for DLLP to finish before entering L1 or L2
>   PCI: tegra: Enable PCIe xclk clock clamping
>   PCI: tegra: Add SW fixup for RAW violations
>   PCI: tegra: Increase the deskew retry time
>   PCI: tegra: Update flow control threshold in Tegra210
> 
>  drivers/pci/host/pci-tegra.c | 306 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 288 insertions(+), 18 deletions(-)
> 

Re: [PATCH V3 00/12] Enable Tegra root port features and apply SW fixups

[Lorenzo Pieralisi]

Hi Manikanta,

On Sun, Nov 26, 2017 at 01:29:26AM +0530, Manikanta Maddireddy wrote:
> Hi Bjorn, Thierry,
> 
> Could you please review this series of patches?

It is new code correct (ie there are not any fixes in the series) ?
I will mark it as code to review for the next kernel cycle on my side.

Thanks,
Lorenzo

> On 30-Oct-17 7:27 PM, Manikanta Maddireddy wrote:
> > These series of patches does the following things,
> > - Deasserting pcie_xrst after programming root port to make sure that
> >   register programming is reflected during LTSSM
> > - Apply REFCLK pad settings to make sure P2P amplitude requirement is met
> > - Enable Gen2 link speed
> > - Advertise AER capability
> > - Program UPHY electrical settings for meeting eye diagram requirements
> > - Bunch of SW fixups explained in their respective commit log
> > 
> > Testing done on Tegra124, 210 and 186:
> > - PCIe link up, config read, BAR read and basic functionality of Ethernet
> >   card
> > - Link speed switch to Gen2 after link retrain
> > - Link speed stays in Gen1 after retrain if end point is only Gen1 capable
> > - Simulated AER errors and verified dmesg logs for them
> > - Rest of the programming is verified by dumping the registers after PCIe
> > link up
> > 
> > Manikanta Maddireddy (12):
> >   PCI: tegra: Start LTSSM after programming root port
> >   PCI: tegra: Move REFCLK pad settings out of phy_power_on()
> >   PCI: tegra: Retrain link for Gen2 speed
> >   PCI: tegra: Advertise AER capability
> >   PCI: tegra: Program UPHY electrical settings in Tegra210
> >   PCI: tegra: Enable opportunistic update FC and ACK
> >   PCI: tegra: Disable AFI dynamic clock gating
> >   PCI: tegra: Wait for DLLP to finish before entering L1 or L2
> >   PCI: tegra: Enable PCIe xclk clock clamping
> >   PCI: tegra: Add SW fixup for RAW violations
> >   PCI: tegra: Increase the deskew retry time
> >   PCI: tegra: Update flow control threshold in Tegra210
> > 
> >  drivers/pci/host/pci-tegra.c | 306 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 288 insertions(+), 18 deletions(-)
> > 

Re: [PATCH V3 00/12] Enable Tegra root port features and apply SW fixups

[Manikanta Maddireddy]

Hi Lorenzo,

Except "PCI: tegra: Move REFCLK pad settings out of phy_power_on()"
all other patches in the series is new code.

Thanks,
Manikanta

On 27-Nov-17 11:39 PM, Lorenzo Pieralisi wrote:
> Hi Manikanta,
> 
> On Sun, Nov 26, 2017 at 01:29:26AM +0530, Manikanta Maddireddy wrote:
>> Hi Bjorn, Thierry,
>>
>> Could you please review this series of patches?
> 
> It is new code correct (ie there are not any fixes in the series) ?
> I will mark it as code to review for the next kernel cycle on my side.
> 
> Thanks,
> Lorenzo
> 
>> On 30-Oct-17 7:27 PM, Manikanta Maddireddy wrote:
>>> These series of patches does the following things,
>>> - Deasserting pcie_xrst after programming root port to make sure that
>>>   register programming is reflected during LTSSM
>>> - Apply REFCLK pad settings to make sure P2P amplitude requirement is met
>>> - Enable Gen2 link speed
>>> - Advertise AER capability
>>> - Program UPHY electrical settings for meeting eye diagram requirements
>>> - Bunch of SW fixups explained in their respective commit log
>>>
>>> Testing done on Tegra124, 210 and 186:
>>> - PCIe link up, config read, BAR read and basic functionality of Ethernet
>>>   card
>>> - Link speed switch to Gen2 after link retrain
>>> - Link speed stays in Gen1 after retrain if end point is only Gen1 capable
>>> - Simulated AER errors and verified dmesg logs for them
>>> - Rest of the programming is verified by dumping the registers after PCIe
>>> link up
>>>
>>> Manikanta Maddireddy (12):
>>>   PCI: tegra: Start LTSSM after programming root port
>>>   PCI: tegra: Move REFCLK pad settings out of phy_power_on()
>>>   PCI: tegra: Retrain link for Gen2 speed
>>>   PCI: tegra: Advertise AER capability
>>>   PCI: tegra: Program UPHY electrical settings in Tegra210
>>>   PCI: tegra: Enable opportunistic update FC and ACK
>>>   PCI: tegra: Disable AFI dynamic clock gating
>>>   PCI: tegra: Wait for DLLP to finish before entering L1 or L2
>>>   PCI: tegra: Enable PCIe xclk clock clamping
>>>   PCI: tegra: Add SW fixup for RAW violations
>>>   PCI: tegra: Increase the deskew retry time
>>>   PCI: tegra: Update flow control threshold in Tegra210
>>>
>>>  drivers/pci/host/pci-tegra.c | 306 ++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 288 insertions(+), 18 deletions(-)
>>>