| Message ID | 1414552896-20797-1-git-send-email-b18965@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 29, 2014 at 11:21:36AM +0800, Alison Wang wrote: > This patch adds Power Management support for SAI. > Activate regmap cache with REGCACHE_RBTREE, and use Are you sure that REGCACHE_RBTREE is the best option here? For MMIO devices the cost tradeoff for the rbtree is usually higher than people like so flat caches are preferred. But if it works for you that's fine, this shouldn't be *that* performance critical. I'm also a bit surprised that this works without register defaults being provided since we need to make sure we allocate the rbtree nodes outside of the spinlock we use to lock MMIO access - was this tested with mainline?
Hi Alison, Please always add the driver maintainers (Xiubo Li and Nicolin Chen). On Wed, Oct 29, 2014 at 1:21 AM, Alison Wang <b18965@freescale.com> wrote: > +#ifdef CONFIG_PM_SLEEP > +static int fsl_sai_suspend(struct device *dev) > +{ > + struct fsl_sai *sai = dev_get_drvdata(dev); > + > + regcache_cache_only(sai->regmap, true); > + regcache_mark_dirty(sai->regmap); > + > + return 0; > +} > + > +static int fsl_sai_resume(struct device *dev) > +{ > + struct fsl_sai *sai = dev_get_drvdata(dev); > + > + /* Restore all registers */ > + regcache_cache_only(sai->regmap, false); > + regcache_sync(sai->regmap); > + > + return 0; > +}; > +#endif /* CONFIG_PM_SLEEP */ > + > +static const struct dev_pm_ops fsl_sai_pm = { > + SET_SYSTEM_SLEEP_PM_OPS(fsl_sai_suspend, fsl_sai_resume) This could be simplified to: static SIMPLE_DEV_PM_OPS(fsl_sai_pm, fsl_sai_suspend, fsl_sai_resume); I am also curious as to how you tested it, as I noticed that suspend/resume is broken on 3.18-rc for mx6sx. Are you able to do suspend/resume on 3.18-rc on a mx6sx sdb board?
Hi, > -----Original Message----- > From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org] > On Behalf Of Mark Brown > Sent: Wednesday, October 29, 2014 7:37 PM > To: Wang Huan-B18965 > Cc: alsa-devel@alsa-project.org; tiwai@suse.de; linux-kernel@vger.kernel.org; > lgirdwood@gmail.com; perex@perex.cz; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] audio: sai: Add Power Management support > > On Wed, Oct 29, 2014 at 11:21:36AM +0800, Alison Wang wrote: > > This patch adds Power Management support for SAI. > > Activate regmap cache with REGCACHE_RBTREE, and use > > Are you sure that REGCACHE_RBTREE is the best option here? For MMIO > devices the cost tradeoff for the rbtree is usually higher than people > like so flat caches are preferred. But if it works for you that's fine, > this shouldn't be *that* performance critical. > Yes, the flat caches will have higher performance and can also fix the Register defaults and spinlock issue here. One more thing, if the device is not performance critical, then shouldn't we Take care of the cache memory consumption to determine using flat or rbtree Type ? Thanks, BRs Xiubo > I'm also a bit surprised that this works without register defaults being > provided since we need to make sure we allocate the rbtree nodes outside > of the spinlock we use to lock MMIO access - was this tested with > mainline?
On Thu, Oct 30, 2014 at 03:30:40AM +0000, Li.Xiubo@freescale.com wrote: > One more thing, if the device is not performance critical, then shouldn't we > Take care of the cache memory consumption to determine using flat or rbtree > Type ? Yes, it's always fine to use a rbtree if it makes sense - it was just an unusual choice for a device like this that didn't seem to be discussed. Depending on the register map a flat cache can actually be more memory efficient sometimes since there's some overhead for the rbtree data structures, if you've just got one block of registers with no gaps a flat cache is going to be a win there.
Hi, On Wed, Oct 29, 2014 at 11:21:36AM +0800, Alison Wang wrote: > This patch adds Power Management support for SAI. > Activate regmap cache with REGCACHE_RBTREE, and use Are you sure that REGCACHE_RBTREE is the best option here? For MMIO devices the cost tradeoff for the rbtree is usually higher than people like so flat caches are preferred. But if it works for you that's fine, this shouldn't be *that* performance critical. I'm also a bit surprised that this works without register defaults being provided since we need to make sure we allocate the rbtree nodes outside of the spinlock we use to lock MMIO access - was this tested with mainline? [Alison Wang] I tested rbtree and flat caches, they both work. But I didn't pay attention to the cost tradeoff and register defaults before, so I think flat caches are preferred now. Thanks. Best Regards, Alison Wang
Hi, Fabio, Please always add the driver maintainers (Xiubo Li and Nicolin Chen). [Alison Wang] ok, thanks for your reminder. On Wed, Oct 29, 2014 at 1:21 AM, Alison Wang <b18965@freescale.com> wrote: > +#ifdef CONFIG_PM_SLEEP > +static int fsl_sai_suspend(struct device *dev) > +{ > + struct fsl_sai *sai = dev_get_drvdata(dev); > + > + regcache_cache_only(sai->regmap, true); > + regcache_mark_dirty(sai->regmap); > + > + return 0; > +} > + > +static int fsl_sai_resume(struct device *dev) > +{ > + struct fsl_sai *sai = dev_get_drvdata(dev); > + > + /* Restore all registers */ > + regcache_cache_only(sai->regmap, false); > + regcache_sync(sai->regmap); > + > + return 0; > +}; > +#endif /* CONFIG_PM_SLEEP */ > + > +static const struct dev_pm_ops fsl_sai_pm = { > + SET_SYSTEM_SLEEP_PM_OPS(fsl_sai_suspend, fsl_sai_resume) This could be simplified to: static SIMPLE_DEV_PM_OPS(fsl_sai_pm, fsl_sai_suspend, fsl_sai_resume); [Alison Wang] ok. I am also curious as to how you tested it, as I noticed that suspend/resume is broken on 3.18-rc for mx6sx. Are you able to do suspend/resume on 3.18-rc on a mx6sx sdb board? [Alison Wang] No, I don't have that board. I tested on LS1021A QDS board which supports deep sleep. Best Regards, Alison Wang
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 7eeb1dd..c7dd953 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c @@ -509,9 +509,11 @@ static bool fsl_sai_readable_reg(struct device *dev, unsigned int reg) static bool fsl_sai_volatile_reg(struct device *dev, unsigned int reg) { switch (reg) { + case FSL_SAI_TCSR: case FSL_SAI_TFR: - case FSL_SAI_RFR: case FSL_SAI_TDR: + case FSL_SAI_RCSR: + case FSL_SAI_RFR: case FSL_SAI_RDR: return true; default: @@ -553,6 +555,7 @@ static const struct regmap_config fsl_sai_regmap_config = { .readable_reg = fsl_sai_readable_reg, .volatile_reg = fsl_sai_volatile_reg, .writeable_reg = fsl_sai_writeable_reg, + .cache_type = REGCACHE_RBTREE, }; static int fsl_sai_probe(struct platform_device *pdev) @@ -668,6 +671,33 @@ static int fsl_sai_probe(struct platform_device *pdev) SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); } +#ifdef CONFIG_PM_SLEEP +static int fsl_sai_suspend(struct device *dev) +{ + struct fsl_sai *sai = dev_get_drvdata(dev); + + regcache_cache_only(sai->regmap, true); + regcache_mark_dirty(sai->regmap); + + return 0; +} + +static int fsl_sai_resume(struct device *dev) +{ + struct fsl_sai *sai = dev_get_drvdata(dev); + + /* Restore all registers */ + regcache_cache_only(sai->regmap, false); + regcache_sync(sai->regmap); + + return 0; +}; +#endif /* CONFIG_PM_SLEEP */ + +static const struct dev_pm_ops fsl_sai_pm = { + SET_SYSTEM_SLEEP_PM_OPS(fsl_sai_suspend, fsl_sai_resume) +}; + static const struct of_device_id fsl_sai_ids[] = { { .compatible = "fsl,vf610-sai", }, { .compatible = "fsl,imx6sx-sai", }, @@ -680,6 +710,7 @@ static struct platform_driver fsl_sai_driver = { .name = "fsl-sai", .owner = THIS_MODULE, .of_match_table = fsl_sai_ids, + .pm = &fsl_sai_pm, }, }; module_platform_driver(fsl_sai_driver);
This patch adds Power Management support for SAI. Activate regmap cache with REGCACHE_RBTREE, and use regmap cache code to save and restore registers in suspend and resume. The Transmit Control Register (TCSR) and Receive Control Register(RCSR) should be volatile registers. Signed-off-by: Alison Wang <alison.wang@freescale.com> --- sound/soc/fsl/fsl_sai.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)