[PATCH v2 09/14] ASoC: amd: add Renoir ACP PCI driver PM ops

[Mukunda, Vijendar]

[AMD Official Use Only - Internal Distribution Only]



> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Sent: Wednesday, May 13, 2020 2:06 AM
> To: Mukunda, Vijendar <Vijendar.Mukunda at amd.com>; Alex Deucher
> <alexdeucher at gmail.com>
> Cc: Takashi Iwai <tiwai at suse.de>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; alsa-devel at alsa-project.org; Mark Brown
> <broonie at kernel.org>
> Subject: Re: [PATCH v2 09/14] ASoC: amd: add Renoir ACP PCI driver PM ops
> 
> 
> 
> >>>>> +     pm_runtime_set_autosuspend_delay(&pci->dev,
> >> ACP_SUSPEND_DELAY_MS);
> >>>>> +     pm_runtime_use_autosuspend(&pci->dev);
> >>>>> +     pm_runtime_allow(&pci->dev);
> >>>>> +     pm_runtime_mark_last_busy(&pci->dev);
> >>>>> +     pm_runtime_put_autosuspend(&pci->dev);
> >>>>
> >>>> usually there is a pm_runtime_put_noidle() here?
> >>>
> >>> I'm not sure.
> >>>
> >>>>
> >>>> [...]
> >>>>
> >>>>>     static void snd_rn_acp_remove(struct pci_dev *pci)
> >>>>>     {
> >>>>>         struct acp_dev_data *adata;
> >>>>> @@ -260,6 +302,9 @@ static void snd_rn_acp_remove(struct pci_dev
> >> *pci)
> >>>>>         ret = rn_acp_deinit(adata->acp_base);
> >>>>>         if (ret)
> >>>>>                 dev_err(&pci->dev, "ACP de-init failed\n");
> >>>>> +     pm_runtime_put_noidle(&pci->dev);
> >>>>> +     pm_runtime_get_sync(&pci->dev);
> >>>>> +     pm_runtime_forbid(&pci->dev);
> >>>>
> >>>> doing a put_noidle() followed by a get_sync() and immediately forbid()
> >>>> is very odd at best.
> >>>> Isn't the recommendation to call get_noresume() here?
> >>>>
> >>>
> >>> I'm not sure here either.  Is there some definitive documentation on
> >>> what exact sequences are supposed to be used in drivers?  A quick
> >>> browse through drivers that implement runtime pm seems to show a lot
> >>> of variation.  This sequence works.  I'm not sure if it's optimal or
> >>> not.
> >>
> >> We based our sequence on the comments in drivers/pci/pci-driver.c
> >>
> >> /*
> >>    * Unbound PCI devices are always put in D0, regardless of
> >>    * runtime PM status.  During probe, the device is set to
> >>    * active and the usage count is incremented.  If the driver
> >>    * supports runtime PM, it should call pm_runtime_put_noidle(),
> >>    * or any other runtime PM helper function decrementing the usage
> >>    * count, in its probe routine and pm_runtime_get_noresume() in
> >>    * its remove routine.
> >>    */
> >
> > If I understood correctly, below should be  the correct sequence rite ?
> >
> > acp pci driver probe sequence:
> >
> > pm_runtime_set_autosuspend_delay(&pci->dev, ACP_SUSPEND_DELAY_MS);
> > pm_runtime_use_autosuspend(&pci->dev);
> > pm_runtime_put_noidle(&pci->dev);
> > pm_runtime_allow(&pci->dev);
> 
> sounds about right. We added an extra pm_runtime_mark_last_busy() to
> make sure the information is updated, but that's probably on the
> paranoid side.
> >
> > acp pci driver remove sequence:
> >
> > pm_runtime_get_noresume(&pci->dev);
> > pm_runtime_disable(&pci->dev);
> >
> > I have still have a doubt.
> > Do we need to call pm_runtime_disable() explicitly  in this case ?
> 
> we don't call pm_runtime_disable().

I see in one of the PCI driver implementation, in driver remove sequence
pm_runtime_forbid( ) API is called before pm_runtime_get_noresume () call.

I believe , It's safe to call pm_runtime_forbid() which will prevent device to 
be power managed  at runtime in remove sequence.

Correct me, if my understanding is wrong .