Re: [PATCH V6 7/7] PCI: tegra: Add power management support

[Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 25-Jan-18 8:18 PM, Thierry Reding wrote:
> On Thu, Jan 11, 2018 at 11:38:08AM +0530, Manikanta Maddireddy wrote:
>> Tegra186 powergate driver is implemented as power domain driver, power
>> partition ungate/gate are registered as power_on/power_off callback
>> functions. There are no direct functions to power gate/ungate host
>> controller in Tegra186. Host controller driver should add "power-domains"
>> property in device tree and implement runtime suspend and resume
>> callback functons. Power gate and ungate is taken care by power domain
>> driver when host controller driver calls pm_runtime_put_sync and
>> pm_runtime_get_sync respectively.
>>
>> Register suspend_noirq & resume_noirq callback functions to allow PCIe to
>> come up after resume from RAM. Both runtime and noirq pm ops share same
>> callback functions.
>>
>> Signed-off-by: Manikanta Maddireddy <mmaddireddy-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> V2:
>> * no change in this patch
>> V3:
>> * no change in this patch
>> V4:
>> * no change in this patch
>> V5:
>> * Decoupled from https://patchwork.ozlabs.org/patch/832053/ and
>> rebased on linux-next
>> V6:
>> * no change in this patch
>>
>>  drivers/pci/host/pci-tegra.c | 181 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 110 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> [...]
>> @@ -1536,37 +1526,41 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>>  	int err;
>>  	u32 reg;
>>  
>> -	mutex_init(&msi->lock);
>> +	if (!msi->phys) {
>> +		mutex_init(&msi->lock);
>>  
>> -	msi->chip.dev = dev;
>> -	msi->chip.setup_irq = tegra_msi_setup_irq;
>> -	msi->chip.teardown_irq = tegra_msi_teardown_irq;
>> +		msi->chip.dev = dev;
>> +		msi->chip.setup_irq = tegra_msi_setup_irq;
>> +		msi->chip.teardown_irq = tegra_msi_teardown_irq;
>>  
>> -	msi->domain = irq_domain_add_linear(dev->of_node, INT_PCI_MSI_NR,
>> -					    &msi_domain_ops, &msi->chip);
>> -	if (!msi->domain) {
>> -		dev_err(dev, "failed to create IRQ domain\n");
>> -		return -ENOMEM;
>> -	}
>> +		msi->domain = irq_domain_add_linear(dev->of_node,
>> +						    INT_PCI_MSI_NR,
>> +						    &msi_domain_ops,
>> +						    &msi->chip);
>> +		if (!msi->domain) {
>> +			dev_err(dev, "failed to create IRQ domain\n");
>> +			return -ENOMEM;
>> +		}
>>  
>> -	err = platform_get_irq_byname(pdev, "msi");
>> -	if (err < 0) {
>> -		dev_err(dev, "failed to get IRQ: %d\n", err);
>> -		goto err;
>> -	}
>> +		err = platform_get_irq_byname(pdev, "msi");
>> +		if (err < 0) {
>> +			dev_err(dev, "failed to get IRQ: %d\n", err);
>> +			goto err;
>> +		}
>>  
>> -	msi->irq = err;
>> +		msi->irq = err;
>>  
>> -	err = request_irq(msi->irq, tegra_pcie_msi_irq, IRQF_NO_THREAD,
>> -			  tegra_msi_irq_chip.name, pcie);
>> -	if (err < 0) {
>> -		dev_err(dev, "failed to request IRQ: %d\n", err);
>> -		goto err;
>> -	}
>> +		err = request_irq(msi->irq, tegra_pcie_msi_irq, IRQF_NO_THREAD,
>> +				  tegra_msi_irq_chip.name, pcie);
>> +		if (err < 0) {
>> +			dev_err(dev, "failed to request IRQ: %d\n", err);
>> +			goto err;
>> +		}
>>  
>> -	/* setup AFI/FPCI range */
>> -	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> -	msi->phys = virt_to_phys((void *)msi->pages);
>> +		/* setup AFI/FPCI range */
>> +		msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> +		msi->phys = virt_to_phys((void *)msi->pages);
>> +	}
> 
> I think it'd be better to split this off into a separate function so
> that we can get rid of the confusing if (!msi->phys) conditional. It
> looks as though this would work, but it's confusing to have that code
> in a resume handler.
> 
> It also becomes very asymmetric when you set up the MSIs in the resume
> handler, but then don't call tegra_pcie_msi_disable() from the suspend
> handler.
> 
> I'd suggest just pulling out all of the allocations and such into a
> separate function, perhaps tegra_pcie_msi_setup(), and similarily split
> off the resource part of tegra_pcie_disable_msi() into a separate
> function, perhaps tegra_pcie_msi_teardown().
> 
> That way, suspend/resume will do only what they need (write registers)
> and everything will look more symmetric again. That in turn makes the
> code a lot easier to follow and maintain.
I'll do this change in next version.
> 
> Thierry
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html