| Message ID | 20200511212014.2359225-10-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Renoir ACP driver | expand |
> @@ -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? [...] > 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? > pci_disable_msi(pci); > pci_release_regions(pci); > pci_disable_device(pci); > @@ -278,6 +323,9 @@ static struct pci_driver rn_acp_driver = { > .id_table = snd_rn_acp_ids, > .probe = snd_rn_acp_probe, > .remove = snd_rn_acp_remove, > + .driver = { > + .pm = &rn_acp_pm, > + } > }; > > module_pci_driver(rn_acp_driver); > diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h > index a4f654cf2df0..6e1888167fb3 100644 > --- a/sound/soc/amd/renoir/rn_acp3x.h > +++ b/sound/soc/amd/renoir/rn_acp3x.h > @@ -40,6 +40,8 @@ > #define TWO_CH 0x02 > #define DELAY_US 5 > #define ACP_COUNTER 20000 > +/* time in ms for runtime suspend delay */ > +#define ACP_SUSPEND_DELAY_MS 2000 > > #define ACP_SRAM_PTE_OFFSET 0x02050000 > #define PAGE_SIZE_4K_ENABLE 0x2 >
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. Alex > > > > pci_disable_msi(pci); > > pci_release_regions(pci); > > pci_disable_device(pci); > > @@ -278,6 +323,9 @@ static struct pci_driver rn_acp_driver = { > > .id_table = snd_rn_acp_ids, > > .probe = snd_rn_acp_probe, > > .remove = snd_rn_acp_remove, > > + .driver = { > > + .pm = &rn_acp_pm, > > + } > > }; > > > > module_pci_driver(rn_acp_driver); > > diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h > > index a4f654cf2df0..6e1888167fb3 100644 > > --- a/sound/soc/amd/renoir/rn_acp3x.h > > +++ b/sound/soc/amd/renoir/rn_acp3x.h > > @@ -40,6 +40,8 @@ > > #define TWO_CH 0x02 > > #define DELAY_US 5 > > #define ACP_COUNTER 20000 > > +/* time in ms for runtime suspend delay */ > > +#define ACP_SUSPEND_DELAY_MS 2000 > > > > #define ACP_SRAM_PTE_OFFSET 0x02050000 > > #define PAGE_SIZE_4K_ENABLE 0x2 > >
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. */
> -----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 ?
>>>>> + 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().
[AMD Official Use Only - Internal Distribution Only] > -----Original Message----- > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Sent: Wednesday, May 13, 2020 2:06 AM > To: Mukunda, Vijendar <Vijendar.Mukunda@amd.com>; 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> > 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 .
diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c index 362409ef0d85..14651df9b5c8 100644 --- a/sound/soc/amd/renoir/rn-pci-acp3x.c +++ b/sound/soc/amd/renoir/rn-pci-acp3x.c @@ -10,6 +10,7 @@ #include <linux/delay.h> #include <linux/platform_device.h> #include <linux/interrupt.h> +#include <linux/pm_runtime.h> #include "rn_acp3x.h" @@ -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); return 0; unregister_devs: @@ -250,6 +256,42 @@ static int snd_rn_acp_probe(struct pci_dev *pci, return ret; } +static int snd_rn_acp_suspend(struct device *dev) +{ + int ret; + struct acp_dev_data *adata; + + adata = dev_get_drvdata(dev); + ret = rn_acp_deinit(adata->acp_base); + if (ret) + dev_err(dev, "ACP de-init failed\n"); + else + dev_dbg(dev, "ACP de-initialized\n"); + + return 0; +} + +static int snd_rn_acp_resume(struct device *dev) +{ + int ret; + struct acp_dev_data *adata; + + adata = dev_get_drvdata(dev); + ret = rn_acp_init(adata->acp_base); + if (ret) { + dev_err(dev, "ACP init failed\n"); + return ret; + } + return 0; +} + +static const struct dev_pm_ops rn_acp_pm = { + .runtime_suspend = snd_rn_acp_suspend, + .runtime_resume = snd_rn_acp_resume, + .suspend = snd_rn_acp_suspend, + .resume = snd_rn_acp_resume, +}; + 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); pci_disable_msi(pci); pci_release_regions(pci); pci_disable_device(pci); @@ -278,6 +323,9 @@ static struct pci_driver rn_acp_driver = { .id_table = snd_rn_acp_ids, .probe = snd_rn_acp_probe, .remove = snd_rn_acp_remove, + .driver = { + .pm = &rn_acp_pm, + } }; module_pci_driver(rn_acp_driver); diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h index a4f654cf2df0..6e1888167fb3 100644 --- a/sound/soc/amd/renoir/rn_acp3x.h +++ b/sound/soc/amd/renoir/rn_acp3x.h @@ -40,6 +40,8 @@ #define TWO_CH 0x02 #define DELAY_US 5 #define ACP_COUNTER 20000 +/* time in ms for runtime suspend delay */ +#define ACP_SUSPEND_DELAY_MS 2000 #define ACP_SRAM_PTE_OFFSET 0x02050000 #define PAGE_SIZE_4K_ENABLE 0x2