On Wed, Jul 18, 2018 at 02:02:41PM +0800, Honghui Zhang wrote:

<snip>

quoted

quoted

+static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)
+{
+	struct mtk_pcie *pcie = dev_get_drvdata(dev);
+	const struct mtk_pcie_soc *soc = pcie->soc;
+	struct mtk_pcie_port *port, *tmp;
+
+	if (!soc->pm_support)
+		return 0;
+
+	if (list_empty(&pcie->ports))
+		return 0;
+
+	if (dev->pm_domain) {
+		pm_runtime_enable(dev);
+		pm_runtime_get_sync(dev);
+	}

Are these runtime PM calls needed/abused here ?

Mind explaining the logic ?

There is certainly an asymmetry with the suspend callback which made me
suspicious, I am pretty certain Rafael/Kevin/Ulf can help me clarify so
that we can make progress with this patch.

Lorenzo

Hi Lorenzo, thanks for your comments.
Sorry I don't get you.
I believe that in suspend callbacks the pm_runtime_put_sync and
pm_runtime_disable should be called to gated the CMOS for this module,
while the pm_rumtime_enable and pm_rumtime_get_sync should be called
in resume callback.

That's why I CC'ed Rafael, Kevin and Ulf, to answer this question
thoroughly, I am not sure it is needed and that's the right way
of doing it in system suspend callbacks.

That's exactly this patch doing.
But the pm_rumtime_put_sync and pm_runtime_disable functions was wrapped
in the mtk_pcie_subsys_powerdown.

Ah, sorry, I missed that.

I did not call mtk_pcie_subsys_powerup since it does not just wrapped
pm_rumtime related functions but also do the platform_resource_get,
devm_ioremap, and free_ck clock get which I do not needed in resume
callback.

Do you think it will be much clear if I abstract the
platform_resource_get, devm_ioremap functions from
mtk_pcie_subsys_powerup and put it to a new functions like
mtk_pcie_subsys_resource_get, and then we may call the
mtk_pcie_subsys_powerup in the resume function?

I think so but let's wait first for feedback on whether those
runtime PM calls are needed in the first place.

Lorenzo