[alsa-devel] [PATCH v2] ALSA: hda/tegra: enable clock during probe
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; schedule_work(&hda->probe_work); @@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work) struct platform_device *pdev = to_platform_device(hda->dev); int err; - pm_runtime_get_sync(hda->dev); err = hda_tegra_first_init(chip, pdev); if (err < 0) goto out_free; @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000); + /* set device state as active */ + if (pm_runtime_set_active(hda->dev) < 0) + goto out_free; + /* enable runtime PM */ + pm_runtime_enable(hda->dev); + if (!azx_has_pm_runtime(chip)) + pm_runtime_forbid(hda->dev); + out_free: - pm_runtime_put(hda->dev); return; /* no error return from async probe */ } @@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev) ret = snd_card_free(dev_get_drvdata(&pdev->dev)); pm_runtime_disable(&pdev->dev); + if (!pm_runtime_status_suspended(&pdev->dev)) { + hda_tegra_runtime_suspend(&pdev->dev); + pm_runtime_set_suspended(&pdev->dev); + } return ret; } -- 2.7.4
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;
schedule_work(&hda->probe_work);
@@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work) struct platform_device *pdev = to_platform_device(hda->dev); int err;
- pm_runtime_get_sync(hda->dev); err = hda_tegra_first_init(chip, pdev); if (err < 0) goto out_free; @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000);
+ /* set device state as active */ + if (pm_runtime_set_active(hda->dev) < 0) + goto out_free; + /* enable runtime PM */ + pm_runtime_enable(hda->dev); + if (!azx_has_pm_runtime(chip)) + pm_runtime_forbid(hda->dev); + out_free: - pm_runtime_put(hda->dev); return; /* no error return from async probe */ }
@@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev)
ret = snd_card_free(dev_get_drvdata(&pdev->dev)); pm_runtime_disable(&pdev->dev); + if (!pm_runtime_status_suspended(&pdev->dev)) { + hda_tegra_runtime_suspend(&pdev->dev); + pm_runtime_set_suspended(&pdev->dev); + }
I think that we need to be consistent in the above with what is done in the probe. If in the probe we call hda_tegra_enable_clocks(), then here we should call hda_tegra_disable_clocks(). However, my preference is still to all hda_tegra_runtime_resume() in probe and then leave the above as-is. Let's see what everyone else thinks. Cheers Jon -- nvpublic
On 1/25/2019 5:12 PM, Jon Hunter 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;
schedule_work(&hda->probe_work);
@@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work) struct platform_device *pdev = to_platform_device(hda->dev); int err;
- pm_runtime_get_sync(hda->dev); err = hda_tegra_first_init(chip, pdev); if (err < 0) goto out_free; @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000);
+ /* set device state as active */ + if (pm_runtime_set_active(hda->dev) < 0) + goto out_free; + /* enable runtime PM */ + pm_runtime_enable(hda->dev); + if (!azx_has_pm_runtime(chip)) + pm_runtime_forbid(hda->dev); + out_free: - pm_runtime_put(hda->dev); return; /* no error return from async probe */ }
@@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev)
ret = snd_card_free(dev_get_drvdata(&pdev->dev)); pm_runtime_disable(&pdev->dev); + if (!pm_runtime_status_suspended(&pdev->dev)) { + hda_tegra_runtime_suspend(&pdev->dev); + pm_runtime_set_suspended(&pdev->dev); + } I think that we need to be consistent in the above with what is done in
On 25/01/2019 11:06, Sameer Pujar wrote: the probe. If in the probe we call hda_tegra_enable_clocks(), then here we should call hda_tegra_disable_clocks(). However, my preference is still to all hda_tegra_runtime_resume() in probe and then leave the above as-is. Let's see what everyone else thinks. Though it is good to have the handling at a single place, it is slightly confusing to call runtime_resume() in probe. When runtime PM is not yet enabled, why call something that is related to it(not logically but intuitively)!
What we can possibly follow is, 1. Till probe() completes, handle things explicitly. 2. Before returning from successful probe, handle control to runtime PM. Nevertheless, shall go with the majority of the opinion. Thanks, Sameer.
Cheers Jon
On 25/01/2019 12:19, Sameer Pujar wrote:
On 1/25/2019 5:12 PM, Jon Hunter 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; schedule_work(&hda->probe_work); @@ -571,7 +569,6 @@ static void hda_tegra_probe_work(struct work_struct *work) struct platform_device *pdev = to_platform_device(hda->dev); int err; - pm_runtime_get_sync(hda->dev); err = hda_tegra_first_init(chip, pdev); if (err < 0) goto out_free; @@ -592,8 +589,15 @@ static void hda_tegra_probe_work(struct work_struct *work) chip->running = 1; snd_hda_set_power_save(&chip->bus, power_save * 1000); + /* set device state as active */ + if (pm_runtime_set_active(hda->dev) < 0) + goto out_free; + /* enable runtime PM */ + pm_runtime_enable(hda->dev); + if (!azx_has_pm_runtime(chip)) + pm_runtime_forbid(hda->dev); + out_free: - pm_runtime_put(hda->dev); return; /* no error return from async probe */ } @@ -603,6 +607,10 @@ static int hda_tegra_remove(struct platform_device *pdev) ret = snd_card_free(dev_get_drvdata(&pdev->dev)); pm_runtime_disable(&pdev->dev); + if (!pm_runtime_status_suspended(&pdev->dev)) { + hda_tegra_runtime_suspend(&pdev->dev); + pm_runtime_set_suspended(&pdev->dev); + } I think that we need to be consistent in the above with what is done in
On 25/01/2019 11:06, Sameer Pujar wrote: the probe. If in the probe we call hda_tegra_enable_clocks(), then here we should call hda_tegra_disable_clocks(). However, my preference is still to all hda_tegra_runtime_resume() in probe and then leave the above as-is. Let's see what everyone else thinks. Though it is good to have the handling at a single place, it is slightly confusing to call runtime_resume() in probe. When runtime PM is not yet enabled, why call something that is related to it(not logically but intuitively)!
I still don't think that it is confusing, the name of the function is called hda_tegra_runtime_resume and does what is says on the tin (ie. resumes the device at runtime). Furthermore, I think it is completely logical that if pm-runtime is not enabled, we manually call it. The benefit is that we have single function for resuming the device for all circumstances. Cheers Jon -- nvpublic
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. Cheers Jon -- nvpublic
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. thanks, Takashi
On 1/30/2019 10:10 PM, Takashi Iwai wrote:
On Wed, 30 Jan 2019 13:45:49 +0100, Jon Hunter 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
On 25/01/2019 11:06, Sameer Pujar wrote: 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.
If there are no further concerns, can we consider this for approval? Thanks, Sameer.
thanks,
Takashi
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. 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? Thanks, Thierry
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
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it. What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally pm_runtime_enable(dev); ret = pm_runtime_resume(dev); should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
On Thu, Jan 31, 2019 at 12:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret = pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset?
Well, we got rid of CONFIG_PM_RUNTIME, so that would be CONFIG_PM unset rather.
Or something else?
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret = pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly. hda_tegra_probe() { .... pm_runtime_enable(); schedule_work(); return; } hda_tegra_probe_work() { pm_runtime_get_sync(); .... pm_runtime_put_sync(); } Then it truned outhis code lacks of the clock initialization when runtime PM isn't enabled. Normally it's done via runtime resume hda_tegra_runtime_resume() { hda_tegra_enable_clocks(); .... } And now the question is what is the standard idiom in such a case. IMO, calling pm_runtime_resume() inside the probe function looks weird, and my preference was to initialize the clocks explicitly, then enable runtime PM. But if using pm_runtime_resume() in the proc should be seen as a standard procedure, I'm fine with that. thanks, Takashi
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret = pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME).
This basically is about setup, because after that point all should just work in both cases. Personally, I would do if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup }
Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { ....
So why don't you do if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup } here?
pm_runtime_enable(); schedule_work(); return; }
hda_tegra_probe_work() { pm_runtime_get_sync(); .... pm_runtime_put_sync(); }
Then it truned outhis code lacks of the clock initialization when runtime PM isn't enabled. Normally it's done via runtime resume
hda_tegra_runtime_resume() { hda_tegra_enable_clocks(); .... }
And now the question is what is the standard idiom in such a case.
IMO, calling pm_runtime_resume() inside the probe function looks weird, and my preference was to initialize the clocks explicitly, then enable runtime PM. But if using pm_runtime_resume() in the proc should be seen as a standard procedure, I'm fine with that.
Well, people do pm_runtime_resume() in ->probe() too, but pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give you what you want anyway. :-) Cheers, Rafael
On 1/31/2019 5:40 PM, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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: [cut]
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.
Are you suggesting, whether runtime PM is enabled/disabled, after successful probe the domain would be powered off? For CONFIG_PM enabled case, probe() can call get_sync() and put_sync() can be in probe_work. How this needs to be handled for CONFG_PM disabled case? (just calling clock_enable() may not be sufficient as per previous comments)
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. I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret = pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else? Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). This basically is about setup, because after that point all should just work in both cases.
Personally, I would do
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup }
do we really need config check here? The debate was, whether to call hda_tegra_runtime_resume() or hda_tegra_enable_clocks() unconditionally here. It would take care of both CONFIG_PM enabled/disabled cases. Then enable runtime PM.
Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { .... So why don't you do
if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup }
here?
pm_runtime_enable(); schedule_work(); return; }
hda_tegra_probe_work() { pm_runtime_get_sync(); .... pm_runtime_put_sync(); }
Then it truned outhis code lacks of the clock initialization when runtime PM isn't enabled. Normally it's done via runtime resume
hda_tegra_runtime_resume() { hda_tegra_enable_clocks(); .... }
And now the question is what is the standard idiom in such a case.
IMO, calling pm_runtime_resume() inside the probe function looks weird, and my preference was to initialize the clocks explicitly, then enable runtime PM. But if using pm_runtime_resume() in the proc should be seen as a standard procedure, I'm fine with that.
I think reference here is, whether calling hda_tegra_runtime_resume() in probe() is a standard procedure or not.
Well, people do pm_runtime_resume() in ->probe() too, but pm_runtime_resume() returns 1 for CONFIG_PM unset, so that won't give you what you want anyway. :-)
Cheers, Rafael
On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret = pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME).
This basically is about setup, because after that point all should just work in both cases.
Personally, I would do
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup }
Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { ....
So why don't you do
if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup }
here?
I think that's exactly what Jon and Sameer were proposing, although the discussion started primarily because of the way it was done. So basically the idea was to do: pm_runtime_enable() if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */ hda_runtime_resume() So we're not calling pm_runtime_resume() but rather the driver's implementation of it. This is to avoid duplicating the code, which under some circumstances can be fairly long. Duplicating is also error prone because both instances may not always be in sync. My understanding is that Takashi had reservations about using this kind of construct because, well, frankly, it looks a little weird. We'd also likely want to have a similar construct again in the ->remove() callback to make sure we properly power off the device when it is no longer needed. I'm just wondering if perhaps there should be a mechanism in the core to take care of this, because this is basically something that we'd need to do for every single driver. For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be modified to do the above? This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated. Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way. Thierry
On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
--Pk/CTwBz1VvfPIDp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
> If I understand correctly the code, the pm domain is already ac=
tivated
> at calling driver's probe callback.
As far as I can tell, the domain will also be powered off again a= fter probe finished, unless the device grabs a runtime PM reference. T= his is what happens via the dev->pm_domain->sync() call after successful=
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 lik= e this:
pm_runtime_enable(dev); if (!pm_runtime_enabled(dev)) { err =3D foo_runtime_resume(dev); if (err < 0) goto fail; }
So basically when runtime PM is not available, we explicitly "res= ume" the device to power it up.
It seems to me like that's a fairly common problem, so I'm wonder= ing if there's something that the runtime PM core could do to help with =
On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote: probe 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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret =3D pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). =20 This basically is about setup, because after that point all should just work in both cases. =20 Personally, I would do =20 if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } =20 Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { .... =20 So why don't you do =20 if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup } =20 here?
I think that's exactly what Jon and Sameer were proposing, although the discussion started primarily because of the way it was done.
So basically the idea was to do:
pm_runtime_enable() if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */
But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
hda_runtime_resume()
So we're not calling pm_runtime_resume() but rather the driver's implementation of it. This is to avoid duplicating the code, which under some circumstances can be fairly long. Duplicating is also error prone because both instances may not always be in sync.
My understanding is that Takashi had reservations about using this kind of construct because, well, frankly, it looks a little weird.
Yes, the way it was originally written above was weird, but is checking IS_ENABLED(CONFIG_PM) directly really so weird?
We'd also likely want to have a similar construct again in the ->remove() callback to make sure we properly power off the device when it is no longer needed.
Sure. Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
I'm just wondering if perhaps there should be a mechanism in the core to take care of this,
How exactly? How's the core going to know what to do when CONFIG_PM is disabled?
because this is basically something that we'd need to do for every single driver.
That is not true. If the device is alwyas "on" to start with, you don't need to do anything. That's the case on many systems.
For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be modified to do the above?
But you'd need to pass a pointer to your hda_runtime_resume() to it at least and how's that simpler than using a simple conditional directly?
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there. Alternatively, you can make the driver depend on PM. Cheers, Rafael
On 2/1/2019 4:54 AM, Rafael J. Wysocki wrote:
On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
--Pk/CTwBz1VvfPIDp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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: [cut]
>> If I understand correctly the code, the pm domain is already ac= tivated >> at calling driver's probe callback. > As far as I can tell, the domain will also be powered off again a= fter > probe finished, unless the device grabs a runtime PM reference. T= his is > what happens via the dev->pm_domain->sync() call after successful=
> 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 lik= e this: > pm_runtime_enable(dev); > if (!pm_runtime_enabled(dev)) { > err =3D foo_runtime_resume(dev); > if (err < 0) > goto fail; > } > > So basically when runtime PM is not available, we explicitly "res= ume" > the device to power it up. > > It seems to me like that's a fairly common problem, so I'm wonder= ing if > there's something that the runtime PM core could do to help with =
On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote: probe 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. I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret =3D pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else? Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). =20 This basically is about setup, because after that point all should just work in both cases. =20 Personally, I would do =20 if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } =20 Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { .... =20 So why don't you do =20 if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup } =20 here? I think that's exactly what Jon and Sameer were proposing, although the discussion started primarily because of the way it was done.
So basically the idea was to do:
pm_runtime_enable() if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */ But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
hda_runtime_resume()
So we're not calling pm_runtime_resume() but rather the driver's implementation of it. This is to avoid duplicating the code, which under some circumstances can be fairly long. Duplicating is also error prone because both instances may not always be in sync.
My understanding is that Takashi had reservations about using this kind of construct because, well, frankly, it looks a little weird.
Yes, the way it was originally written above was weird, but is checking IS_ENABLED(CONFIG_PM) directly really so weird?
We'd also likely want to have a similar construct again in the ->remove() callback to make sure we properly power off the device when it is no longer needed. Sure. Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
I'm just wondering if perhaps there should be a mechanism in the core to take care of this, How exactly? How's the core going to know what to do when CONFIG_PM is disabled?
because this is basically something that we'd need to do for every single driver. That is not true. If the device is alwyas "on" to start with, you don't need to do anything. That's the case on many systems.
For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be modified to do the above? But you'd need to pass a pointer to your hda_runtime_resume() to it at least and how's that simpler than using a simple conditional directly?
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way. If you select PM, then all of the requisite code should be there.
Alternatively, you can make the driver depend on PM. Objective is to have things working with or without CONFIG_PM enabled. From previous comments and discussions it appears that there is mixed response for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need to have consensus regarding the best practice to be followed, which would eventually can be used in other drivers too.
Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe, which would bring back the earlier above mentioned concern. if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } Both if/else might end up doing the same here. Do we really need CONFIG_PM check here? Instead does below proposal appear fine? probe() { hda_tegra_enable_clock(); } probe_work() { /* hda setup */ . . . pm_runtime_set_active(); /* initial state as active */ pm_runtime_enable(); return; } remove() { pm_runtime_disable(); if (!pm_runtime_status_suspended()) hda_tegra_runtime_suspend(); /* takes care of both CONFIG_PM enable/disable case */ } One of the other concern was, remove() and probe() do not appear to be in sync, because in probe() hda_tegra_enable_clock() is called and in remove() there is hda_tegra_runtime_suspend() to effectively disable clock. IMO, this should be ok since it can avoid duplication and proper comment can be added here for clarity. Alternatively we can call hda_tegra_runtime_resume() in probe() unconditionally to avoid confusion. Another point Thierry mentioned was, after successful probe() power-domain would be turned OFF. It seems Rafael had a different view. I am little confused here. Kindly clarify if above proposal seems fine. Thanks, Sameer.
Cheers, Rafael
On Mon, Feb 04, 2019 at 01:46:20PM +0530, Sameer Pujar wrote:
On 2/1/2019 4:54 AM, Rafael J. Wysocki wrote:
On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
--Pk/CTwBz1VvfPIDp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote: > 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: [cut]
> > > If I understand correctly the code, the pm domain is already ac= tivated > > > at calling driver's probe callback. > > As far as I can tell, the domain will also be powered off again a= fter > > probe finished, unless the device grabs a runtime PM reference. T= his is > > what happens via the dev->pm_domain->sync() call after successful=
> > 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 lik= e this: > > pm_runtime_enable(dev); > > if (!pm_runtime_enabled(dev)) { > > err =3D foo_runtime_resume(dev); > > if (err < 0) > > goto fail; > > } > > > > So basically when runtime PM is not available, we explicitly "res= ume" > > the device to power it up. > > > > It seems to me like that's a fairly common problem, so I'm wonder= ing if > > there's something that the runtime PM core could do to help with =
On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote: probe 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. I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret =3D pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else? Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). =20 This basically is about setup, because after that point all should just work in both cases. =20 Personally, I would do =20 if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } =20 Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { .... =20 So why don't you do =20 if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup } =20 here? I think that's exactly what Jon and Sameer were proposing, although the discussion started primarily because of the way it was done.
So basically the idea was to do:
pm_runtime_enable() if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */ But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
hda_runtime_resume()
So we're not calling pm_runtime_resume() but rather the driver's implementation of it. This is to avoid duplicating the code, which under some circumstances can be fairly long. Duplicating is also error prone because both instances may not always be in sync.
My understanding is that Takashi had reservations about using this kind of construct because, well, frankly, it looks a little weird.
Yes, the way it was originally written above was weird, but is checking IS_ENABLED(CONFIG_PM) directly really so weird?
We'd also likely want to have a similar construct again in the ->remove() callback to make sure we properly power off the device when it is no longer needed. Sure. Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
I'm just wondering if perhaps there should be a mechanism in the core to take care of this, How exactly? How's the core going to know what to do when CONFIG_PM is disabled?
because this is basically something that we'd need to do for every single driver. That is not true. If the device is alwyas "on" to start with, you don't need to do anything. That's the case on many systems.
For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be modified to do the above? But you'd need to pass a pointer to your hda_runtime_resume() to it at least and how's that simpler than using a simple conditional directly?
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way. If you select PM, then all of the requisite code should be there.
Alternatively, you can make the driver depend on PM. Objective is to have things working with or without CONFIG_PM enabled. From previous comments and discussions it appears that there is mixed response for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need to have consensus regarding the best practice to be followed, which would eventually can be used in other drivers too.
Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe, which would bring back the earlier above mentioned concern.
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } Both if/else might end up doing the same here. Do we really need CONFIG_PM check here?
Instead does below proposal appear fine?
probe() { hda_tegra_enable_clock(); }
probe_work() { /* hda setup */ . . . pm_runtime_set_active(); /* initial state as active */ pm_runtime_enable(); return; }
remove() { pm_runtime_disable(); if (!pm_runtime_status_suspended()) hda_tegra_runtime_suspend(); /* takes care of both CONFIG_PM enable/disable case */ }
One of the other concern was, remove() and probe() do not appear to be in sync, because in probe() hda_tegra_enable_clock() is called and in remove() there is hda_tegra_runtime_suspend() to effectively disable clock. IMO, this should be ok since it can avoid duplication and proper comment can be added here for clarity. Alternatively we can call hda_tegra_runtime_resume() in probe() unconditionally to avoid confusion.
Another point Thierry mentioned was, after successful probe() power-domain would be turned OFF. It seems Rafael had a different view. I am little confused here. Kindly clarify if above proposal seems fine.
We're wasting an awful lot of time over the discussion of something that I think is of questionable usefulness. I propose we do the below and can forget about this once and for all. Thierry --- >8 --- diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig index 7f3b83e0d324..51a8fa3566ef 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig @@ -10,6 +10,7 @@ menuconfig ARCH_TEGRA select HAVE_ARM_SCU if SMP select HAVE_ARM_TWD if SMP select PINCTRL + select PM select PM_OPP select ARCH_HAS_RESET_CONTROLLER select RESET_CONTROLLER
On 04/02/2019 08:16, Sameer Pujar wrote: ...
Objective is to have things working with or without CONFIG_PM enabled. From previous comments and discussions it appears that there is mixed response for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need to have consensus regarding the best practice to be followed, which would eventually can be used in other drivers too.
Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe, which would bring back the earlier above mentioned concern.
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } Both if/else might end up doing the same here. Do we really need CONFIG_PM check here?
Instead does below proposal appear fine?
probe() { hda_tegra_enable_clock(); }
probe_work() { /* hda setup */ . . . pm_runtime_set_active(); /* initial state as active */ pm_runtime_enable(); return; }
I believe that this still does not work, because if there is a power-domain that needs to be turned on, this does not guarantee this. So I think that you need to call pm_runtime_get ... probe() { if (!IS_ENABLED(CONFIG_PM)) hda_tegra_enable_clock(); } probe_work() { /* hda setup */ . . . pm_runtime_enable(); pm_runtime_get_sync(); return; } The alternative here could just be ... probe() { pm_runtime_enable(); if (!pm_runtime_enabled()) ret = hda_tegra_enable_clock(); else ret = pm_runtime_get_sync(); if (ret < 0) { ... } } Very similar to what I was saying to begin with but not call the pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike. Cheers Jon -- nvpublic
On Mon, 04 Feb 2019 11:04:50 +0100, Jon Hunter wrote:
On 04/02/2019 08:16, Sameer Pujar wrote:
...
Objective is to have things working with or without CONFIG_PM enabled. From previous comments and discussions it appears that there is mixed response for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need to have consensus regarding the best practice to be followed, which would eventually can be used in other drivers too.
Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe, which would bring back the earlier above mentioned concern.
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } Both if/else might end up doing the same here. Do we really need CONFIG_PM check here?
Instead does below proposal appear fine?
probe() { hda_tegra_enable_clock(); }
probe_work() { /* hda setup */ . . . pm_runtime_set_active(); /* initial state as active */ pm_runtime_enable(); return; }
I believe that this still does not work, because if there is a power-domain that needs to be turned on, this does not guarantee this. So I think that you need to call pm_runtime_get ...
probe() { if (!IS_ENABLED(CONFIG_PM)) hda_tegra_enable_clock(); }
probe_work() { /* hda setup */ . . . pm_runtime_enable(); pm_runtime_get_sync(); return; }
The alternative here could just be ...
probe() { pm_runtime_enable(); if (!pm_runtime_enabled()) ret = hda_tegra_enable_clock(); else ret = pm_runtime_get_sync();
if (ret < 0) { ... } }
Very similar to what I was saying to begin with but not call the pm_runtime_resume handler directly. Which I believe was Iwai-san's dislike.
Yes, exactly, what bothered me was really a nuance: calling hda_tegra_runtime_resume() there makes the code misleading (or confusing) as if the runtime PM were mandatory. I hoped there could be some standard idiom for this expression, but apparently there isn't any, so far... Obviously the easiest option is to enforce the dependency on CONFIG_PM. Would there be any platform that needs to run without PM, practically seen...? But, now after lengthy discussions and the clarification of the current situation, I have no strong opinion on this any longer. So I leave the decision to you guys, and I'll apply it as-is. thanks, Takashi
On Monday, February 4, 2019 11:04:50 AM CET Jon Hunter wrote:
On 04/02/2019 08:16, Sameer Pujar wrote:
...
Objective is to have things working with or without CONFIG_PM enabled. From previous comments and discussions it appears that there is mixed response for calling hda_tegra_runtime_resume() or runtime PM APIs in probe() call. Need to have consensus regarding the best practice to be followed, which would eventually can be used in other drivers too.
Rafael is suggesting to use CONFIG_PM check to do manual setup or runtime PM setup in probe, which would bring back the earlier above mentioned concern.
if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } Both if/else might end up doing the same here. Do we really need CONFIG_PM check here?
Instead does below proposal appear fine?
probe() { hda_tegra_enable_clock(); }
probe_work() { /* hda setup */ . . . pm_runtime_set_active(); /* initial state as active */ pm_runtime_enable(); return; }
I believe that this still does not work, because if there is a power-domain that needs to be turned on, this does not guarantee this. So I think that you need to call pm_runtime_get ...
probe() { if (!IS_ENABLED(CONFIG_PM)) hda_tegra_enable_clock(); }
But alas, there are no PM domains with CONFIG_PM unset. CONFIG_PM unset means that the *driver* has to know how to turn on the device and that has to be sufficient. Which basically is my point when I'm saying that this information is not available to the core and it cannot do anything to handle this case without the extra knowledge. Thanks, Rafael
On Fri, Feb 01, 2019 at 12:24:31AM +0100, Rafael J. Wysocki wrote:
On Thursday, January 31, 2019 3:30:24 PM CET Thierry Reding wrote:
--Pk/CTwBz1VvfPIDp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
On Thu, Jan 31, 2019 at 12:59 PM Takashi Iwai <tiwai@suse.de> wrote:
On Thu, 31 Jan 2019 12:46:54 +0100, Rafael J. Wysocki wrote:
On Thu, Jan 31, 2019 at 12:21 PM Takashi Iwai <tiwai@suse.de> wrote:
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:
[cut]
> > If I understand correctly the code, the pm domain is already ac=
tivated
> > at calling driver's probe callback. > > As far as I can tell, the domain will also be powered off again a= fter > probe finished, unless the device grabs a runtime PM reference. T= his is > what happens via the dev->pm_domain->sync() call after successful=
> 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 lik= e this: > > pm_runtime_enable(dev); > if (!pm_runtime_enabled(dev)) { > err =3D foo_runtime_resume(dev); > if (err < 0) > goto fail; > } > > So basically when runtime PM is not available, we explicitly "res= ume" > the device to power it up. > > It seems to me like that's a fairly common problem, so I'm wonder= ing if > there's something that the runtime PM core could do to help with =
On Thu, Jan 31, 2019 at 01:10:01PM +0100, Rafael J. Wysocki wrote: probe 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.
I'm not sure that I understand the problem correctly, so let me restate it the way I understand it.
What we're talking about is a driver ->probe() callback. Runtime PM is disabled initially and the device is off. It needs to be powered up, but the way to do that depends on some configuration of the board etc., so ideally
pm_runtime_enable(dev); ret =3D pm_runtime_resume(dev);
should just work, but the question is what to do if runtime PM doesn't work as expected. That is, CONFIG_PM_RUNTIME is unset? Or something else?
Yes, the question is how to write the code for both with and without CONFIG_PM (or CONFIG_PM_RUNTIME). =20 This basically is about setup, because after that point all should just work in both cases. =20 Personally, I would do =20 if (IS_ENABLED(CONFIG_PM)) { do setup based on pm-runtime } else { do manual setup } =20 Right now, we have a code like below, pushing the initialization in an async work and let the probe returning quickly.
hda_tegra_probe() { .... =20 So why don't you do =20 if (!IS_ENABLED(CONFIG_PM)) { do manual clock setup } =20 here?
I think that's exactly what Jon and Sameer were proposing, although the discussion started primarily because of the way it was done.
So basically the idea was to do:
pm_runtime_enable() if (!pm_runtime_enabled()) /* basically !IS_ENABLED(CONFIG_PM) */
But why is it any better than checking !IS_ENABLED(CONFIG_PM) directly?
I guess the intention here was to avoid the conditional. This may also have been cargo-culted from a time when we didn't have IS_ENABLED, and the ifdefery required would've made this a lot worse.
hda_runtime_resume()
So we're not calling pm_runtime_resume() but rather the driver's implementation of it. This is to avoid duplicating the code, which under some circumstances can be fairly long. Duplicating is also error prone because both instances may not always be in sync.
My understanding is that Takashi had reservations about using this kind of construct because, well, frankly, it looks a little weird.
Yes, the way it was originally written above was weird, but is checking IS_ENABLED(CONFIG_PM) directly really so weird?
We'd also likely want to have a similar construct again in the ->remove() callback to make sure we properly power off the device when it is no longer needed.
Sure. Again, why don't you make it conditional on IS_ENABLED(CONFIG_PM)?
I'm just wondering if perhaps there should be a mechanism in the core to take care of this,
How exactly? How's the core going to know what to do when CONFIG_PM is disabled?
because this is basically something that we'd need to do for every single driver.
That is not true. If the device is alwyas "on" to start with, you don't need to do anything. That's the case on many systems.
For example, if !CONFIG_PM couldn't the pm_runtime_enable() function be modified to do the above?
But you'd need to pass a pointer to your hda_runtime_resume() to it at least and how's that simpler than using a simple conditional directly?
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this: pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev); ... } But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say: pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev); return 0; } Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns: 1) select PM would force the setting for all platforms on multi- platforms builds 2) prevents anyone from disabling PM for debugging purposes 1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken. Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple. Jon, any objections? Thierry
On 04/02/2019 08:45, Thierry Reding wrote: ...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ... pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync) ... and in these APIs we add ... pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev); pm_runtime_enable(dev); return pm_runtime_get(dev); }
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case. Cheers Jon -- nvpublic
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM. Thierry
04.02.2019 14:05, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.
On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
04.02.2019 14:05, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
This would be somewhat tricky because drivers usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and that would result in an empty structure if !CONFIG_PM, but we could probably work around that by adding a __SET_RUNTIME_PM_OPS that would never be compiled out for this kind of case. Or such drivers could even manually set .runtime_suspend and .runtime_resume to make sure they're always populated.
Another way out of this would be to make sure we never run into the case where runtime PM is disabled. If we always "select PM" on Tegra, then PM should always be available. But is it guaranteed that runtime PM for the devices is functional in that case? From a cursory look at the code it would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.
What is it about disabling PM that you consider useful? I can understand why you'd want that option if power management is broken, but as far as I can tell, power management on Tegra is in pretty good state, and it's more likely that !PM would actually be broken (though I haven't built a configuration like that in a couple of years, so I'm speculating). We already can't disable PM on 64-bit ARM, so I don't understand why 32-bit ARM should be treated any differently. Thierry
04.02.2019 17:00, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
04.02.2019 14:05, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
> This would be somewhat tricky because drivers > usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and > that would result in an empty structure if !CONFIG_PM, but we could > probably work around that by adding a __SET_RUNTIME_PM_OPS that would > never be compiled out for this kind of case. Or such drivers could even > manually set .runtime_suspend and .runtime_resume to make sure they're > always populated. > > Another way out of this would be to make sure we never run into the case > where runtime PM is disabled. If we always "select PM" on Tegra, then PM > should always be available. But is it guaranteed that runtime PM for the > devices is functional in that case? From a cursory look at the code it > would seem that way.
If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.
What is it about disabling PM that you consider useful? I can understand why you'd want that option if power management is broken, but as far as I can tell, power management on Tegra is in pretty good state, and it's more likely that !PM would actually be broken (though I haven't built a configuration like that in a couple of years, so I'm speculating). We already can't disable PM on 64-bit ARM, so I don't understand why 32-bit ARM should be treated any differently.
Yes, I want an option for debugging of a broken PM. It doesn't matter that there are no known PM-related issues on Tegra today, it could become a problem tomorrow. Probably a kernel boot parameter like pm_debug_always_on would suit even better here. Or maybe PM API already provides such debug options and I'm just not aware about them? Do you know what's the exact reason for ARM64 to not support !PM?
On Mon, Feb 04, 2019 at 05:28:23PM +0300, Dmitry Osipenko wrote:
04.02.2019 17:00, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
04.02.2019 14:05, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this:
pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev);
... }
But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say:
pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
return 0; }
Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
>> This would be somewhat tricky because drivers >> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and >> that would result in an empty structure if !CONFIG_PM, but we could >> probably work around that by adding a __SET_RUNTIME_PM_OPS that would >> never be compiled out for this kind of case. Or such drivers could even >> manually set .runtime_suspend and .runtime_resume to make sure they're >> always populated. >> >> Another way out of this would be to make sure we never run into the case >> where runtime PM is disabled. If we always "select PM" on Tegra, then PM >> should always be available. But is it guaranteed that runtime PM for the >> devices is functional in that case? From a cursory look at the code it >> would seem that way. > > If you select PM, then all of the requisite code should be there.
We do this on 64-bit ARM, but there had been some pushback when we had proposed to do the same thing on 32-bit ARM. I think there were two concerns:
1) select PM would force the setting for all platforms on multi- platforms builds
2) prevents anyone from disabling PM for debugging purposes
1) no longer seems to be valid because Rockchip already selects PM unconditionally. I'm not sure if 2) is valid anymore either. I haven't run a build with !PM in a very long time and I wouldn't be surprised if that was completely broken.
Maybe we need to try this again since a couple of years have elapsed and runtime PM support on Tegra is much more mature at this point.
> Alternatively, you can make the driver depend on PM.
That's probably the easiest way out, but to be honest I think I'd prefer to just enforce PM and keep things simple.
Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.
What is it about disabling PM that you consider useful? I can understand why you'd want that option if power management is broken, but as far as I can tell, power management on Tegra is in pretty good state, and it's more likely that !PM would actually be broken (though I haven't built a configuration like that in a couple of years, so I'm speculating). We already can't disable PM on 64-bit ARM, so I don't understand why 32-bit ARM should be treated any differently.
Yes, I want an option for debugging of a broken PM. It doesn't matter that there are no known PM-related issues on Tegra today, it could become a problem tomorrow. Probably a kernel boot parameter like pm_debug_always_on would suit even better here.
Or maybe PM API already provides such debug options and I'm just not aware about them?
I'm not sure I understand what you're trying to do. If you want to use !PM as a way to debug broken PM, how would that help? Presumably if you disable !PM then the problem would be gone? In other words, in order to reproduce a PM related issue don't you have to enable PM first? I'm not aware of an alternate mechanism to force PM to off once built into the kernel.
Do you know what's the exact reason for ARM64 to not support !PM?
There's no reason per se not to support PM. It's just that at the time we made the decision that we didn't want to. I vaguely remember that we tried to do the same thing for 32-bit ARM, but at the time we would've been the only platform to select PM, and that would've meant enabling PM for all subarchitectures and therefore forcing PM in multi-platform builds. Today if you build multi-platform kernels, at least Rockchip will also forcibly enable PM for everyone. I think you may also have objected at the time for similar reasons. But this was a long time ago (at least 2 to 3 years I would guess) and much has changed since then. Power management has gotten a lot better on Tegra, to the point where we actually need it in order to properly function. For example, Tegra DRM is not going to work with !PM because we rely on runtime PM while disabling and enabling display pipelines. Thierry
04.02.2019 19:17, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 05:28:23PM +0300, Dmitry Osipenko wrote:
04.02.2019 17:00, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 03:03:49PM +0300, Dmitry Osipenko wrote:
04.02.2019 14:05, Thierry Reding пишет:
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
On 04/02/2019 08:45, Thierry Reding wrote:
...
> The idea was, as I was saying below, to reuse dev_pm_ops even if > !CONFIG_PM. So pm_runtime_enable() could be something like this: > > pm_runtime_enable(dev) > { > if (!CONFIG_PM) > if (dev->pm_ops->resume) > dev->pm_ops->resume(dev); > > ... > } > > But that's admittedly somewhat of a stretch. This could of course be > made somewhat nicer by adding an explicit variant, say: > > pm_runtime_enable_foo(dev) > { > if (!CONFIG_PM && dev->pm_ops->resume) > return dev->pm_ops->resume(dev); > > return 0; > } > > Maybe the fact that I couldn't come up with a good name is a good > indication that this is a bad idea...
How about some new APIs called ...
pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync)
... and in these APIs we add ...
pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
pm_runtime_enable(dev); return pm_runtime_get(dev); }
Yeah, those sound sensible. I'm still a bit torn between this and just enforcing PM. At least on the display side I think we already require PM and with all the power domain work that you did we'd be much better off if we could just get rid of the !PM workarounds.
>>> This would be somewhat tricky because drivers >>> usually use SET_RUNTIME_PM_OPS to populate the struct dev_pm_ops and >>> that would result in an empty structure if !CONFIG_PM, but we could >>> probably work around that by adding a __SET_RUNTIME_PM_OPS that would >>> never be compiled out for this kind of case. Or such drivers could even >>> manually set .runtime_suspend and .runtime_resume to make sure they're >>> always populated. >>> >>> Another way out of this would be to make sure we never run into the case >>> where runtime PM is disabled. If we always "select PM" on Tegra, then PM >>> should always be available. But is it guaranteed that runtime PM for the >>> devices is functional in that case? From a cursory look at the code it >>> would seem that way. >> >> If you select PM, then all of the requisite code should be there. > > We do this on 64-bit ARM, but there had been some pushback when we had > proposed to do the same thing on 32-bit ARM. I think there were two > concerns: > > 1) select PM would force the setting for all platforms on multi- > platforms builds > > 2) prevents anyone from disabling PM for debugging purposes > > 1) no longer seems to be valid because Rockchip already selects PM > unconditionally. I'm not sure if 2) is valid anymore either. I haven't > run a build with !PM in a very long time and I wouldn't be surprised if > that was completely broken. > > Maybe we need to try this again since a couple of years have elapsed and > runtime PM support on Tegra is much more mature at this point. > >> Alternatively, you can make the driver depend on PM. > > That's probably the easiest way out, but to be honest I think I'd prefer > to just enforce PM and keep things simple. > > Jon, any objections?
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
I'm requesting PM_DEBUG_ALWAYS_ON option then! Disabling PM is a useful debug feature, it can't just go away.
What is it about disabling PM that you consider useful? I can understand why you'd want that option if power management is broken, but as far as I can tell, power management on Tegra is in pretty good state, and it's more likely that !PM would actually be broken (though I haven't built a configuration like that in a couple of years, so I'm speculating). We already can't disable PM on 64-bit ARM, so I don't understand why 32-bit ARM should be treated any differently.
Yes, I want an option for debugging of a broken PM. It doesn't matter that there are no known PM-related issues on Tegra today, it could become a problem tomorrow. Probably a kernel boot parameter like pm_debug_always_on would suit even better here.
Or maybe PM API already provides such debug options and I'm just not aware about them?
I'm not sure I understand what you're trying to do. If you want to use !PM as a way to debug broken PM, how would that help? Presumably if you disable !PM then the problem would be gone? In other words, in order to reproduce a PM related issue don't you have to enable PM first?
Disabling PM globally is one of first diagnostics steps, at least it gives an idea about source of a problem.
I'm not aware of an alternate mechanism to force PM to off once built into the kernel.
I think runtime PM could be disabled per-device. That's probably what tools like powertop use to tune power management.
Do you know what's the exact reason for ARM64 to not support !PM?
There's no reason per se not to support PM. It's just that at the time we made the decision that we didn't want to. I vaguely remember that we tried to do the same thing for 32-bit ARM, but at the time we would've been the only platform to select PM, and that would've meant enabling PM for all subarchitectures and therefore forcing PM in multi-platform builds. Today if you build multi-platform kernels, at least Rockchip will also forcibly enable PM for everyone.
I think you may also have objected at the time for similar reasons. But this was a long time ago (at least 2 to 3 years I would guess) and much has changed since then. Power management has gotten a lot better on Tegra, to the point where we actually need it in order to properly function. For example, Tegra DRM is not going to work with !PM because we rely on runtime PM while disabling and enabling display pipelines.
I also know that people are disabling PM for performance reasons, sometime it's more important to reduce system's response latency than to care about power savings. But that probably not very relevant to Tegra usages. Okay, I guess in general it should be fine to enforce PM requirement and improve debug-ability later on by as-needed basis.
On Monday, February 4, 2019 12:05:10 PM CET Thierry Reding wrote:
--FCuugMFkClbJLl1L Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable
On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
The idea was, as I was saying below, to reuse dev_pm_ops even if !CONFIG_PM. So pm_runtime_enable() could be something like this: =20 pm_runtime_enable(dev) { if (!CONFIG_PM) if (dev->pm_ops->resume) dev->pm_ops->resume(dev); =20 ... } =20 But that's admittedly somewhat of a stretch. This could of course be made somewhat nicer by adding an explicit variant, say: =20 pm_runtime_enable_foo(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev); =20 return 0; } =20 Maybe the fact that I couldn't come up with a good name is a good indication that this is a bad idea... =20 How about some new APIs called ... =20
=20 =20 On 04/02/2019 08:45, Thierry Reding wrote: =20 ... =20 pm_runtime_enable_get() pm_runtime_enable_get_sync() pm_runtime_put_disable() (implies a put_sync) =20 ... and in these APIs we add ... =20 pm_runtime_enable_get(dev) { if (!CONFIG_PM && dev->pm_ops->resume) return dev->pm_ops->resume(dev);
No, we're not adding this to the core. While in principle you could provide a set of pointers to the routines you want to be called when CONFIG_PM is unset, IMO it is just cleaner and more straightforward (and fewer lines of code for that matter) to add a simple conditional to each ->probe() and ->remove(). [cut]
None, but seems overkill just for this case.
But that's precisely the point. It's not just about this case. We've already got some drivers where we do a similar dance just to be able to support the, let's admit it, exotic case where somebody turns off PM. I think supporting !PM might have made sense at a point where we had no support for power management at all. But I think we've come a long way since then, and while we may still have some ways to go in some areas, we do fairly decent runtime PM most of the time, to the point where I no longer see any benefits in !PM.
This IMO is an excellent point. Trying to handle the !CONFIG_PM case only really makes sense if you know what to do to turn the device on without relying on things like PM domains etc which are not even available if CONFIG_PM is not set. IOW, if any of the platforms your driver is expected to work with require something like genpd to even make the device functional, I wouldn't bother. Thanks, Rafael
participants (7)
-
Dmitry Osipenko -
Jon Hunter -
Rafael J. Wysocki -
Rafael J. Wysocki -
Sameer Pujar -
Takashi Iwai -
Thierry Reding