2014-10-29 03:55:22

by Alison Wang

[permalink] [raw]
Subject: [PATCH] audio: sai: Add Power Management support

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 <[email protected]>
---
sound/soc/fsl/fsl_sai.c | 33 ++++++++++++++++++++++++++++++++-
1 file changed, 32 insertions(+), 1 deletion(-)

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);
--
2.1.0.27.g96db324


2014-10-29 11:37:17

by Mark Brown

[permalink] [raw]
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.

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?


Attachments:
(No filename) (651.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-29 12:33:57

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH] audio: sai: Add Power Management support

Hi Alison,

Please always add the driver maintainers (Xiubo Li and Nicolin Chen).

On Wed, Oct 29, 2014 at 1:21 AM, Alison Wang <[email protected]> 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?

2014-10-30 03:30:44

by Xiubo Li

[permalink] [raw]
Subject: RE: [PATCH] audio: sai: Add Power Management support

Hi,


> -----Original Message-----
> From: linux-arm-kernel [mailto:[email protected]]
> On Behalf Of Mark Brown
> Sent: Wednesday, October 29, 2014 7:37 PM
> To: Wang Huan-B18965
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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?

2014-10-30 11:23:22

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] audio: sai: Add Power Management support

On Thu, Oct 30, 2014 at 03:30:40AM +0000, [email protected] 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.


Attachments:
(No filename) (633.00 B)
signature.asc (473.00 B)
Digital signature
Download all attachments

2014-10-30 14:55:52

by Huan Wang

[permalink] [raw]
Subject: Re: [PATCH] audio: sai: Add Power Management support

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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2014-10-30 14:56:45

by Huan Wang

[permalink] [raw]
Subject: Re: [PATCH] audio: sai: Add Power Management support

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 <[email protected]> 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????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?