-----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 ?