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

["Mukunda, Vijendar" <Vijendar.Mukunda@amd.com>]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Tuesday, May 12, 2020 8:46 PM
> To: Alex Deucher <alexdeucher@gmail.com>
> Cc: Takashi Iwai <tiwai@suse.de>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; alsa-devel@alsa-project.org; Mark Brown
> <broonie@kernel.org>; Mukunda, Vijendar <Vijendar.Mukunda@amd.com>
> Subject: Re: [PATCH v2 09/14] ASoC: amd: add Renoir ACP PCI driver PM ops
> 
> 
> 
> On 5/12/20 8:46 AM, Alex Deucher wrote:
> > On Mon, May 11, 2020 at 6:37 PM Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com> wrote:
> >>
> >>
> >>> @@ -233,6 +234,11 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
> >>>                ret = PTR_ERR(adata->pdev);
> >>>                goto unregister_devs;
> >>>        }
> >>> +     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);	
	
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 ?