[AMD Official Use Only - Internal Distribution Only]
> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 13, 2020 2:06 AM
> To: Mukunda, Vijendar <Vijendar.Mukunda@xxxxxxx>; Alex Deucher
> <alexdeucher@xxxxxxxxx>
> Cc: Takashi Iwai <tiwai@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx; Mark Brown
> <broonie@xxxxxxxxxx>
> 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 .
