| Message ID | 20200505205327.642282-10-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Renoir ACP driver | expand |
> diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c > index 362409ef0d85..6d013a1bffa6 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,12 @@ 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_set_active(&pci->dev); is the set_active() needed? I haven't seen this in the other PCI audio drivers? > + pm_runtime_put_noidle(&pci->dev); > + pm_runtime_enable(&pci->dev); same, is the _enable() needed()? > + pm_runtime_allow(&pci->dev); > return 0; > > unregister_devs: > @@ -250,6 +257,42 @@ static int snd_rn_acp_probe(struct pci_dev *pci, > return ret; > } >
> -----Original Message----- > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Sent: Wednesday, May 6, 2020 3:19 AM > To: Alex Deucher <alexdeucher@gmail.com>; alsa-devel@alsa-project.org; > broonie@kernel.org; Mukunda, Vijendar <Vijendar.Mukunda@amd.com>; > tiwai@suse.de > Cc: Deucher, Alexander <Alexander.Deucher@amd.com> > Subject: Re: [PATCH 09/14] ASoC: amd: add Renoir ACP PCI driver PM ops > > > > > diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c > b/sound/soc/amd/renoir/rn-pci-acp3x.c > > index 362409ef0d85..6d013a1bffa6 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,12 @@ 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_set_active(&pci->dev); > > is the set_active() needed? I haven't seen this in the other PCI audio > drivers? We have similar implementation in our Raven ACP PCI driver as well which got up streamed. I will give a try by modifying this sequence. Could you please point me , what's exactly wrong with this code? > > > + pm_runtime_put_noidle(&pci->dev); > > + pm_runtime_enable(&pci->dev); > > same, is the _enable() needed()? We have similar implementation in Raven ACP PCI driver as well. > > > + pm_runtime_allow(&pci->dev); > > return 0; > > > > unregister_devs: > > @@ -250,6 +257,42 @@ static int snd_rn_acp_probe(struct pci_dev *pci, > > return ret; > > } > >
>>> @@ -233,6 +234,12 @@ 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_set_active(&pci->dev); >> >> is the set_active() needed? I haven't seen this in the other PCI audio >> drivers? > > We have similar implementation in our Raven ACP PCI driver as well > which got up streamed. > I will give a try by modifying this sequence. > Could you please point me , what's exactly wrong with this code? you would use pm_runtime_set_active() if the device was suspended. I don't think this can possibly happen since there is a _get done by the PCI core, which you compensate for in the line below. Also look at drivers/pci/pci.c, the core already does this set_active() and _enable(). void pci_pm_init(struct pci_dev *dev) { ... pm_runtime_forbid(&dev->dev); pm_runtime_set_active(&dev->dev); pm_runtime_enable(&dev->dev); >>> + pm_runtime_put_noidle(&pci->dev); >>> + pm_runtime_enable(&pci->dev); >> >> same, is the _enable() needed()? > > We have similar implementation in Raven ACP PCI driver as well. It's quite common unfortunately that extended pm_runtime sequences are used without checking what's necessary - it took Intel some time to clearly define what we needed and what was redundant/noop. >>> + pm_runtime_allow(&pci->dev); >>> return 0; >>> >>> unregister_devs: >>> @@ -250,6 +257,42 @@ static int snd_rn_acp_probe(struct pci_dev *pci, >>> return ret; >>> } >>> >
diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c index 362409ef0d85..6d013a1bffa6 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,12 @@ 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_set_active(&pci->dev); + pm_runtime_put_noidle(&pci->dev); + pm_runtime_enable(&pci->dev); + pm_runtime_allow(&pci->dev); return 0; unregister_devs: @@ -250,6 +257,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 +303,8 @@ 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_disable(&pci->dev); + pm_runtime_get_noresume(&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 f63e232565af..f24a4da8c721 100644 --- a/sound/soc/amd/renoir/rn_acp3x.h +++ b/sound/soc/amd/renoir/rn_acp3x.h @@ -38,6 +38,8 @@ #define ACP_PDM_DISABLE 0x00 #define ACP_PDM_DMA_EN_STATUS 0x02 #define TWO_CH 0x02 +/* 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