lkml.org 
[lkml]   [2019]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] ALSA: hda/tegra: enable clock during probe
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2019-01-31 12:06    [W:2.247 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site