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

[Alex Deucher <alexdeucher@gmail.com>]

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