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

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

Commit Message

Manikanta Maddireddy Oct. 30, 2017, 1:57 p.m.
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(-)

Comments

Lorenzo Pieralisi Dec. 12, 2017, 11:32 a.m. | #1
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
>
Manikanta Maddireddy Dec. 13, 2017, 11:50 a.m. | #2
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
>>
Lorenzo Pieralisi Dec. 13, 2017, 2:08 p.m. | #3
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
> >>
Manikanta Maddireddy Dec. 13, 2017, 4:32 p.m. | #4
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
>
Lorenzo Pieralisi Dec. 13, 2017, 6:34 p.m. | #5
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
> >
Manikanta Maddireddy Dec. 13, 2017, 7:27 p.m. | #6
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
>>>
Lorenzo Pieralisi Dec. 14, 2017, 9:57 a.m. | #7
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

Patch

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;