On Thu, 31 Jan 2019 12:05:30 +0100, Thierry Reding wrote:
On Wed, Jan 30, 2019 at 05:40:42PM +0100, Takashi Iwai wrote:
On Wed, 30 Jan 2019 13:45:49 +0100, Jon Hunter wrote:
On 25/01/2019 11:06, Sameer Pujar wrote:
If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks will not be ON. This could cause issue during probe, where hda init setup is done. This patch enables clocks unconditionally during probe.
Along with above, follwoing changes are done.
- enable runtime PM before exiting from probe work. This helps to avoid usage of pm_runtime_get_sync/pm_runtime_put() in probe work.
- hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check.
- runtime PM callbacks moved out of CONFIG_PM check
Signed-off-by: Sameer Pujar spujar@nvidia.com Reviewed-by: Ravindra Lokhande rlokhande@nvidia.com Reviewed-by: Jon Hunter jonathanh@nvidia.com
sound/pci/hda/hda_tegra.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index c8d18dc..ba6175f 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -219,7 +219,6 @@ static int hda_tegra_enable_clocks(struct hda_tegra *data) return rc; }
-#ifdef CONFIG_PM_SLEEP static void hda_tegra_disable_clocks(struct hda_tegra *data) { clk_disable_unprepare(data->hda2hdmi_clk); @@ -227,6 +226,7 @@ static void hda_tegra_disable_clocks(struct hda_tegra *data) clk_disable_unprepare(data->hda_clk); }
+#ifdef CONFIG_PM_SLEEP /*
- power management
*/ @@ -257,7 +257,6 @@ static int hda_tegra_resume(struct device *dev) } #endif /* CONFIG_PM_SLEEP */
-#ifdef CONFIG_PM static int hda_tegra_runtime_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); @@ -283,7 +282,7 @@ static int hda_tegra_runtime_resume(struct device *dev) int rc;
rc = hda_tegra_enable_clocks(hda);
- if (rc != 0)
- if (rc) return rc; if (chip && chip->running) { hda_tegra_init(hda);
@@ -292,7 +291,6 @@ static int hda_tegra_runtime_resume(struct device *dev)
return 0; } -#endif /* CONFIG_PM */
static const struct dev_pm_ops hda_tegra_pm = { SET_SYSTEM_SLEEP_PM_OPS(hda_tegra_suspend, hda_tegra_resume) @@ -551,9 +549,9 @@ static int hda_tegra_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, card);
- pm_runtime_enable(hda->dev);
- if (!azx_has_pm_runtime(chip))
pm_runtime_forbid(hda->dev);
- err = hda_tegra_enable_clocks(hda);
- if (err)
goto out_free;We also need to think about power-domains here. Enabling the clocks might not be enough as the appropriate power-domain needs to be enabled. For 64-bit Tegra runtime-pm will handle the power-domains (assuming they are populated in device-tree). So I still think it is better we call pm_runtime_get_sync() at some point rather than just replying on enabling the clocks.
If I understand correctly the code, the pm domain is already activated at calling driver's probe callback.
As far as I can tell, the domain will also be powered off again after probe finished, unless the device grabs a runtime PM reference. This is what happens via the dev->pm_domain->sync() call after successful probe of a driver.
Ah, a good point. This can be a problem with a probe work like this case.
It seems to me like it's not a very well defined case what to do when a device needs to be powered up but runtime PM is not enabled.
Adding Rafael and linux-pm, maybe they can provide some guidance on what to do in these situations.
To summarize, what we're debating here is how to handle powering up a device if the pm_runtime infrastructure doesn't take care of it. Jon's proposal here was, and we use this elsewhere, to do something like this:
pm_runtime_enable(dev); if (!pm_runtime_enabled(dev)) { err = foo_runtime_resume(dev); if (err < 0) goto fail; }
So basically when runtime PM is not available, we explicitly "resume" the device to power it up.
It seems to me like that's a fairly common problem, so I'm wondering if there's something that the runtime PM core could do to help with this. Or perhaps there's already a way to achieve this that we're all overlooking?
Rafael, any suggestions?
If any, a common helper would be appreciated, indeed.
thanks,
Takashi