Re: [PATCH 2/2] ASoC: fsl: Move clock operation to PM runtime
On Sat, Apr 20, 2019 at 03:59:05PM +, Daniel Baluta wrote:
> Turn off/on clocks when device enters suspend/resume. This
> helps saving power.
> @@ -934,6 +933,25 @@ static int fsl_sai_runtime_suspend(struct device *dev)
> static int fsl_sai_runtime_resume(struct device *dev)
> {
> struct fsl_sai *sai = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(sai->bus_clk);
> + if (ret) {
> + dev_err(dev, "failed to enable bus clock: %d\n", ret);
> + return ret;
> + }
> +
> + if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK)) {
> + ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[1]]);
> + if (ret)
> + goto disable_bus_clk;
> + }
> +
> + if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE)) {
> + ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[0]]);
> + if (ret)
> + goto disable_tx_clk;
> + }
The driver only enables mclk_clks for I2S master mode. But this
change enables them for I2S slave mode also. It doesn't sound a
right thing to me since we are supposed to save power?
Re: [PATCH 1/2] ASoC: fsl: sai: Add support for runtime pm
On Sat, Apr 20, 2019 at 03:59:04PM +, Daniel Baluta wrote: > @@ -898,6 +899,8 @@ static int fsl_sai_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, sai); > > + pm_runtime_enable(>dev); You will need pm_runtime_disable() somewhere, like remove(). Thanks
Re: [PATCH 0/2] Add runtime PM for SAI digital audio interface
On Sat, Apr 20, 2019 at 03:59:03PM +, Daniel Baluta wrote: > First patch uses system PM handlers to implement runtime PM. While > the second patch moves clock handling from startup/shutdown to runtime > PM handlers. > > ASoC: fsl: sai: Add support for runtime pm > ASoC: fsl: Move clock operation to PM runtime Both patches should use the prefix: ASoC: fsl_sai: X Thanks
Re: [PATCH] ASoC: fsl: sai: Fix clock source for mclk0
By following the pattern of previous Subjects:
ASoC: fsl_sai: Fix clock Source for mclk0
On Sat, Apr 20, 2019 at 03:41:04PM +, Daniel Baluta wrote:
> SAI provide multiple master clock source options selectable
> via bit MSEL of TCR2/RCR2.
>
> All possible master clock sources are stored in sai->mclk_clk
> array. Current implementation assumes that MCLK0 source is always
> busclk, but this is wrong!
>
> For example, on i.MX8QM we have:
>
> 00b - Bus Clock selected.
> 01b - Master Clock (MCLK) 1 option selected.
> 10b - Master Clock (MCLK) 2 option selected.
> 11b - Master Clock (MCLK) 3 option selected.
>
> while on i.MX6SX we have:
>
> 00b - Master Clock (MCLK) 1 option selected.
> 01b - Master Clock (MCLK) 1 option selected.
> 10b - Master Clock (MCLK) 2 option selected.
> 11b - Master Clock (MCLK) 3 option selected.
>
> So, this patch will read mclk0 source clock from device tree.
>
> Signed-off-by: Shengjiu Wang
> Signed-off-by: Daniel Baluta
> ---
> sound/soc/fsl/fsl_sai.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index d2a4dc744fd7..faa8de87ff83 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -829,8 +829,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
> sai->bus_clk = NULL;
> }
>
> - sai->mclk_clk[0] = sai->bus_clk;
> - for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
> + for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
> sprintf(tmp, "mclk%d", i);
Firstly, according to your commit message, neither imx8qm nor
imx6sx has an "mclk0" clock in the clock list. Either of them
starts with "mclk1". So, before you change the driver, I don't
think it's even a right thing to define an "mclk0" in the DT.
> sai->mclk_clk[i] = devm_clk_get(>dev, tmp);
> if (IS_ERR(sai->mclk_clk[i])) {
Secondly, this would break existing DT bindings of imx6sx and
imx7 platforms as they both have clock-names defined in DTB:
clock-names = "bus", "mclk1", "mclk2", "mclk3";
Since there's no "mclk0", the entire probe() would error-out.
And mainline has a DT backward-compatible policy, which means
you can't just rename the "bus" in the DTBs but would have to
support them, not to mention "mclk0" is still questionable.
So the right way to fix it is, in my option, to differentiate
the mclk_clk[0] clock source name with the compatible string.
Then you can get the clock name and simply do:
- sai->mclk_clk[0] = sai->bus_clk;
+ sai->mclk_clk[0] = devm_clk_get(>dev, tmp);
+ if (IS_ERR(sai->mclk_clk[0)) {
+ /* error-out*/
+ }
[PATCH 2/2] ASoC: fsl: Move clock operation to PM runtime
From: Shengjiu Wang
Turn off/on clocks when device enters suspend/resume. This
helps saving power.
Signed-off-by: Shengjiu Wang
Signed-off-by: Daniel Baluta
---
sound/soc/fsl/fsl_sai.c | 54 +
1 file changed, 44 insertions(+), 10 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index aeeb07b74177..a224c07ce31f 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -594,15 +594,8 @@ static int fsl_sai_startup(struct snd_pcm_substream
*substream,
{
struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
- struct device *dev = >pdev->dev;
int ret;
- ret = clk_prepare_enable(sai->bus_clk);
- if (ret) {
- dev_err(dev, "failed to enable bus clock: %d\n", ret);
- return ret;
- }
-
regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE,
FSL_SAI_CR3_TRCE);
@@ -619,8 +612,6 @@ static void fsl_sai_shutdown(struct snd_pcm_substream
*substream,
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
regmap_update_bits(sai->regmap, FSL_SAI_xCR3(tx), FSL_SAI_CR3_TRCE, 0);
-
- clk_disable_unprepare(sai->bus_clk);
}
static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
@@ -925,6 +916,14 @@ static int fsl_sai_runtime_suspend(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[0]]);
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[1]]);
+
+ clk_disable_unprepare(sai->bus_clk);
+
regcache_cache_only(sai->regmap, true);
regcache_mark_dirty(sai->regmap);
@@ -934,6 +933,25 @@ static int fsl_sai_runtime_suspend(struct device *dev)
static int fsl_sai_runtime_resume(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_prepare_enable(sai->bus_clk);
+ if (ret) {
+ dev_err(dev, "failed to enable bus clock: %d\n", ret);
+ return ret;
+ }
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK)) {
+ ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[1]]);
+ if (ret)
+ goto disable_bus_clk;
+ }
+
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE)) {
+ ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id[0]]);
+ if (ret)
+ goto disable_tx_clk;
+ }
regcache_cache_only(sai->regmap, false);
regmap_write(sai->regmap, FSL_SAI_TCSR, FSL_SAI_CSR_SR);
@@ -941,7 +959,23 @@ static int fsl_sai_runtime_resume(struct device *dev)
usleep_range(1000, 2000);
regmap_write(sai->regmap, FSL_SAI_TCSR, 0);
regmap_write(sai->regmap, FSL_SAI_RCSR, 0);
- return regcache_sync(sai->regmap);
+
+ ret = regcache_sync(sai->regmap);
+ if (ret)
+ goto disable_rx_clk;
+
+ return 0;
+
+disable_rx_clk:
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_CAPTURE))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[0]]);
+disable_tx_clk:
+ if (sai->mclk_streams & BIT(SNDRV_PCM_STREAM_PLAYBACK))
+ clk_disable_unprepare(sai->mclk_clk[sai->mclk_id[1]]);
+disable_bus_clk:
+ clk_disable_unprepare(sai->bus_clk);
+
+ return ret;
}
#endif /* CONFIG_PM */
--
2.17.1
[PATCH 1/2] ASoC: fsl: sai: Add support for runtime pm
Basically the same actions as for system PM, so make use
of pm_runtime_force_suspend/pm_runtime_force_resume.
Signed-off-by: Shengjiu Wang
Signed-off-by: Daniel Baluta
---
sound/soc/fsl/fsl_sai.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index b563004fb89f..aeeb07b74177 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -898,6 +899,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sai);
+ pm_runtime_enable(>dev);
+
ret = devm_snd_soc_register_component(>dev, _component,
_sai_dai, 1);
if (ret)
@@ -917,8 +920,8 @@ static const struct of_device_id fsl_sai_ids[] = {
};
MODULE_DEVICE_TABLE(of, fsl_sai_ids);
-#ifdef CONFIG_PM_SLEEP
-static int fsl_sai_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int fsl_sai_runtime_suspend(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
@@ -928,7 +931,7 @@ static int fsl_sai_suspend(struct device *dev)
return 0;
}
-static int fsl_sai_resume(struct device *dev)
+static int fsl_sai_runtime_resume(struct device *dev)
{
struct fsl_sai *sai = dev_get_drvdata(dev);
@@ -940,10 +943,13 @@ static int fsl_sai_resume(struct device *dev)
regmap_write(sai->regmap, FSL_SAI_RCSR, 0);
return regcache_sync(sai->regmap);
}
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
static const struct dev_pm_ops fsl_sai_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(fsl_sai_suspend, fsl_sai_resume)
+ SET_RUNTIME_PM_OPS(fsl_sai_runtime_suspend,
+ fsl_sai_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static struct platform_driver fsl_sai_driver = {
--
2.17.1
[PATCH 0/2] Add runtime PM for SAI digital audio interface
First patch uses system PM handlers to implement runtime PM. While the second patch moves clock handling from startup/shutdown to runtime PM handlers. Daniel Baluta (1): ASoC: fsl: sai: Add support for runtime pm Shengjiu Wang (1): ASoC: fsl: Move clock operation to PM runtime sound/soc/fsl/fsl_sai.c | 70 - 1 file changed, 55 insertions(+), 15 deletions(-) -- 2.17.1
[PATCH] ASoC: fsl: sai: Fix clock source for mclk0
SAI provide multiple master clock source options selectable
via bit MSEL of TCR2/RCR2.
All possible master clock sources are stored in sai->mclk_clk
array. Current implementation assumes that MCLK0 source is always
busclk, but this is wrong!
For example, on i.MX8QM we have:
00b - Bus Clock selected.
01b - Master Clock (MCLK) 1 option selected.
10b - Master Clock (MCLK) 2 option selected.
11b - Master Clock (MCLK) 3 option selected.
while on i.MX6SX we have:
00b - Master Clock (MCLK) 1 option selected.
01b - Master Clock (MCLK) 1 option selected.
10b - Master Clock (MCLK) 2 option selected.
11b - Master Clock (MCLK) 3 option selected.
So, this patch will read mclk0 source clock from device tree.
Signed-off-by: Shengjiu Wang
Signed-off-by: Daniel Baluta
---
sound/soc/fsl/fsl_sai.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index d2a4dc744fd7..faa8de87ff83 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -829,8 +829,7 @@ static int fsl_sai_probe(struct platform_device *pdev)
sai->bus_clk = NULL;
}
- sai->mclk_clk[0] = sai->bus_clk;
- for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
+ for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
sprintf(tmp, "mclk%d", i);
sai->mclk_clk[i] = devm_clk_get(>dev, tmp);
if (IS_ERR(sai->mclk_clk[i])) {
--
2.17.1
Re: [PATCH] [v2] x86/mpx: fix recursive munmap() corruption
Dave Hansen writes: > Changes from v1: > * Fix compile errors on UML and non-x86 arches > * Clarify commit message and Fixes about the origin of the >bug and add the impact to powerpc / uml / unicore32 > > -- > > This is a bit of a mess, to put it mildly. But, it's a bug > that only seems to have showed up in 4.20 but wasn't noticed > until now because nobody uses MPX. > > MPX has the arch_unmap() hook inside of munmap() because MPX > uses bounds tables that protect other areas of memory. When > memory is unmapped, there is also a need to unmap the MPX > bounds tables. Barring this, unused bounds tables can eat 80% > of the address space. > > But, the recursive do_munmap() that gets called vi arch_unmap() > wreaks havoc with __do_munmap()'s state. It can result in > freeing populated page tables, accessing bogus VMA state, > double-freed VMAs and more. > > To fix this, call arch_unmap() before __do_unmap() has a chance > to do anything meaningful. Also, remove the 'vma' argument > and force the MPX code to do its own, independent VMA lookup. > > == UML / unicore32 impact == > > Remove unused 'vma' argument to arch_unmap(). No functional > change. > > I compile tested this on UML but not unicore32. > > == powerpc impact == > > powerpc uses arch_unmap() well to watch for munmap() on the > VDSO and zeroes out 'current->mm->context.vdso_base'. Moving > arch_unmap() makes this happen earlier in __do_munmap(). But, > 'vdso_base' seems to only be used in perf and in the signal > delivery that happens near the return to userspace. I can not > find any likely impact to powerpc, other than the zeroing > happening a little earlier. Yeah I agree. > powerpc does not use the 'vma' argument and is unaffected by > its removal. > > I compile-tested a 64-bit powerpc defconfig. Thanks. cheers
Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
On Sat, Apr 20, 2019 at 07:23:59AM +, S.j. Wang wrote: > > > /* Validate input and output sample rates */ > > > - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++) > > > - if (inrate == supported_input_rate[in]) > > > + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++) > > > + if (inrate == supported_asrc_rate[in]) > > > break; > > > > Not sure if we still need it upon having hw_constraint. Maybe m2m needs it? > Yes. OK. Then we can leave it there. Thanks
Re: [PATCH V4 3/3] ASoC: fsl_asrc: Unify the supported input and output rate
Hi.
>
> On Fri, Apr 19, 2019 at 10:23:56AM +, S.j. Wang wrote:
> > Unify the supported input and output rate, add the 12kHz/24kHz/128kHz
> > to the support list
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 32 +++-
> > 1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 2c4bbc3499db..0d06e738264a 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -27,13 +27,14 @@
> > dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index,
> > ##__VA_ARGS__)
> >
> > /* Corresponding to process_option */ -static int
> > supported_input_rate[] = {
> > - 5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200,
> > - 96000, 176400, 192000,
> > +static unsigned int supported_asrc_rate[] = {
> > + 5512, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
> > + 64000, 88200, 96000, 128000, 176400, 192000,
> > };
> >
> > -static int supported_asrc_rate[] = {
> > - 8000, 11025, 16000, 22050, 32000, 44100, 48000, 64000, 88200, 96000,
> 176400, 192000,
> > +static struct snd_pcm_hw_constraint_list fsl_asrc_rate_constraints = {
> > + .count = ARRAY_SIZE(supported_asrc_rate),
> > + .list = supported_asrc_rate,
> > };
> >
> > /**
> > @@ -293,11 +294,11 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > ideal = config->inclk == INCLK_NONE;
> >
> > /* Validate input and output sample rates */
> > - for (in = 0; in < ARRAY_SIZE(supported_input_rate); in++)
> > - if (inrate == supported_input_rate[in])
> > + for (in = 0; in < ARRAY_SIZE(supported_asrc_rate); in++)
> > + if (inrate == supported_asrc_rate[in])
> > break;
>
> Not sure if we still need it upon having hw_constraint. Maybe m2m needs it?
Yes.
Best regards
Wang shengjiu
Re: [PATCH V4 2/3] ASoC: fsl_asrc: replace the process_option table with function
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:53AM +, S.j. Wang wrote:
>
> > @@ -289,6 +318,12 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > return -EINVAL;
> > }
> >
> > + ret = fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
>
> Since the function always return 0, I am thinking of treating this function as
> a lookup function, and then moving this call right before the register
> settings -- as we have already made sure that both inrate and outrate are
> supported.
Ok.
>
> > + if (ret) {
> > + pair_err("No supported pre-processing options\n");
> > + return ret;
> > + }
>
> And probably no longer need this error-out. If there's a new limitation
> related to this function, I believe we can add it to the rate validation
> section as we are doing now -- better to have rate validation code at one
> place.
>
Ok.
> > @@ -380,8 +415,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> > /* Apply configurations for pre- and post-processing */
>
> Here:
> - /* Apply configurations for pre- and post-processing */
> + /* Select and apply configurations for pre- and post-processing */
> + fsl_asrc_sel_proc(inrate, outrate, _proc, _proc);
> > regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> > ASRCFG_PREMODi_MASK(index) |
> ASRCFG_POSTMODi_MASK(index),
> > -ASRCFG_PREMOD(index, process_option[in][out][0]) |
> > -ASRCFG_POSTMOD(index, process_option[in][out][1]));
> > +ASRCFG_PREMOD(index, pre_proc) |
> > +ASRCFG_POSTMOD(index, post_proc));
RE: [EXT] Re: [PATCH V4 1/3] ASoC: fsl_asrc: Fix the issue about unsupported rate
Hi
>
>
> On Fri, Apr 19, 2019 at 10:23:50AM +, S.j. Wang wrote:
> > When the output sample rate is [8kHz, 30kHz], the limitation of the
> > supported ratio range is (1/24, 8). In the driver we use (8kHz, 30kHz)
> > instead of [8kHz, 30kHz].
> > So this patch is to fix this issue and the potential rounding issue
> > with divider.
> >
> > Fixes: fff6e03c7b65 ("ASoC: fsl_asrc: add support for 8-30kHz output
> > sample rate")
> > Cc:
> > Signed-off-by: Shengjiu Wang
> > ---
> > sound/soc/fsl/fsl_asrc.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 0b937924d2e4..5b8adc7fb117 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -282,10 +282,10 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> > return -EINVAL;
> > }
> >
> > - if ((outrate > 8000 && outrate < 3) &&
> > - (outrate/inrate > 24 || inrate/outrate > 8)) {
> > - pair_err("exceed supported ratio range [1/24, 8] for \
> > - inrate/outrate: %d/%d\n", inrate, outrate);
> > + if ((outrate >= 8000 && outrate <= 3) &&
> > + (outrate > 24 * inrate || inrate > 8 * outrate)) {
> > + pair_err("exceed supported ratio range (1/24, 8) for
> > + inrate/outrate: %d/%d\n",
>
> Using one of the conditions:
> if (inrate > 8 * outrate)
> pair_err();
>
> This means:
> if (inrate <= 8 * outrate)
> /* Everything is fine */
>
> So the supported ratio range is still [1/24, 8] right?
>
Oh, yes, should still [1/24, 8].
> Thanks
