Re: [PATCH v7 03/13] PCI: tegra194: Don't force the device into the D0 state before L2

[Manikanta Maddireddy <mmaddireddy@nvidia.com>]

On 05/03/26 3:10 pm, Manivannan Sadhasivam wrote:
> On Tue, Mar 03, 2026 at 12:24:38PM +0530, Manikanta Maddireddy wrote:
>> From: Vidya Sagar <vidyas@nvidia.com>
>>
>> As per PCIe CEM spec rev 4.0 ver 1.0 sec 2.3, the PCIe Endpoint device
>> should be in D3 state to assert wake# pin. This takes precedence over PCI
> 
> WAKE#
> 
>> Express Base r4.0 v1.0 September 27-2017, 5.2 Link State Power Management
>> which states that the device can be put into D0 state before taking the
>> link to L2 state. To enable the wake functionality for Endpoint devices,
>> do not force the devices to D0 state before taking the link to L2 state.
>> There is no functional issue with the Endpoint devices where the link
>> doesn't go into L2 state (the reason why the earlier change was made in
>> the first place) as the Root Port proceeds with the usual flow post PME
>> timeout.
>>
> 
> So the previous claim in the comments is not true?
> 
> I agree with this patch in principle, but just want to know why the comment
> claimed there is an issue if the devices are not in D0 state.
> 
> - Mani
No, previous claim is true. D0 fix is done to fix L2 timeout with a 
specific Endpoint. However, later realized that it is breaking wake 
functionality with other Endpoints. As I mentioned in the commit message 
reverting D0 fix still causes L2 timeout with that specific Endpoint, 
but it doesn't cause any functional issue.

- Manikanta
> 
>> Fixes: 56e15a238d92 ("PCI: tegra: Add Tegra194 PCIe support")
>> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
>> Tested-by: Jon Hunter <jonathanh@nvidia.com>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> ---
>> Changes V6 -> V7: Fix commit message
>> Changes V1 -> V6: None
>>
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 41 ----------------------
>>   1 file changed, 41 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index afbc0bdd8a93..831986de584e 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -1258,44 +1258,6 @@ static int tegra_pcie_bpmp_set_pll_state(struct tegra_pcie_dw *pcie,
>>   	return 0;
>>   }
>>   
>> -static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
>> -{
>> -	struct dw_pcie_rp *pp = &pcie->pci.pp;
>> -	struct pci_bus *child, *root_port_bus = NULL;
>> -	struct pci_dev *pdev;
>> -
>> -	/*
>> -	 * link doesn't go into L2 state with some of the endpoints with Tegra
>> -	 * if they are not in D0 state. So, need to make sure that immediate
>> -	 * downstream devices are in D0 state before sending PME_TurnOff to put
>> -	 * link into L2 state.
>> -	 * This is as per PCI Express Base r4.0 v1.0 September 27-2017,
>> -	 * 5.2 Link State Power Management (Page #428).
>> -	 */
>> -
>> -	list_for_each_entry(child, &pp->bridge->bus->children, node) {
>> -		if (child->parent == pp->bridge->bus) {
>> -			root_port_bus = child;
>> -			break;
>> -		}
>> -	}
>> -
>> -	if (!root_port_bus) {
>> -		dev_err(pcie->dev, "Failed to find downstream bus of Root Port\n");
>> -		return;
>> -	}
>> -
>> -	/* Bring downstream devices to D0 if they are not already in */
>> -	list_for_each_entry(pdev, &root_port_bus->devices, bus_list) {
>> -		if (PCI_SLOT(pdev->devfn) == 0) {
>> -			if (pci_set_power_state(pdev, PCI_D0))
>> -				dev_err(pcie->dev,
>> -					"Failed to transition %s to D0 state\n",
>> -					dev_name(&pdev->dev));
>> -		}
>> -	}
>> -}
>> -
>>   static int tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
>>   {
>>   	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
>> @@ -1625,7 +1587,6 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
>>   
>>   static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
>>   {
>> -	tegra_pcie_downstream_dev_to_D0(pcie);
>>   	dw_pcie_host_deinit(&pcie->pci.pp);
>>   	tegra_pcie_dw_pme_turnoff(pcie);
>>   	tegra_pcie_unconfig_controller(pcie);
>> @@ -2335,7 +2296,6 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
>>   	if (!pcie->link_state)
>>   		return 0;
>>   
>> -	tegra_pcie_downstream_dev_to_D0(pcie);
>>   	tegra_pcie_dw_pme_turnoff(pcie);
>>   	tegra_pcie_unconfig_controller(pcie);
>>   
>> @@ -2409,7 +2369,6 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
>>   			return;
>>   
>>   		debugfs_remove_recursive(pcie->debugfs);
>> -		tegra_pcie_downstream_dev_to_D0(pcie);
>>   
>>   		disable_irq(pcie->pci.pp.irq);
>>   		if (IS_ENABLED(CONFIG_PCI_MSI))
>> -- 
>> 2.34.1
>>
> 

-- 
nvpublic