[alsa-devel] [PATCH 0/3] ASoC: rsnd: control SSICR::EN correctly
Hi Mark
These are bugfix patches for Renesas sound SSI driver. SSI1 can share clock pin with SSI0, and then, I had assumed that SSI0 clock output needs SSICR::EN bit. But, it wasn't. SSI0 clock output for SSI1 needs other settings which already sets, but EN was not needed. Because of this assumption, and SSICR::EN operation bug, it gets underrun error in some sound route. This patch solves this issue.
Kuninori Morimoto (3): ASoC: rsnd: control SSICR::EN correctly ASoC: rsnd: move rsnd_ssi_config_init() execute condition into it. ASoC: rsnd: rsnd_ssi_can_output_clk() macro
sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 15 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
In case of SSI0 playback, SSI1 capture, SSI0 might be shared for clock output if clock master mode.
Current rsnd driver had been assumed that SSI clock contiguous output which is needed for SSI parent needs SSICR::EN (SSI module enable) bit. But, this bit controls data input/output, not for clock. Clock contiguous output needs SSICR : FORCE, SCKD, SWSD, and SSIWSR : CONT. Not SSICR : EN.
Because of this wrong assumption, and insufficient control, on current code, for example, if it starts SSI0(playback) -> SSI1(capture) order, SSI0 SSICR::EN bit will temporarily be 0. It causes playback side underrun error. This is bug. We can reproduce this issue with SSI+SRC (without DVC), and capture during playback operation.
This patch fixup current (wrong) assumption, and control SSICR::EN bit correctly.
Reported-by: Hiroyuki Yokoyama hiroyuki.yokoyama.vx@renesas.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Tested-by: Hiroyuki Yokoyama hiroyuki.yokoyama.vx@renesas.com --- sound/soc/sh/rcar/ssi.c | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 49cdc5e..498e71d 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -72,6 +72,7 @@ struct rsnd_ssi { u32 cr_own; u32 cr_clk; u32 cr_mode; + u32 cr_en; u32 wsr; int chan; int rate; @@ -291,6 +292,16 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (ret < 0) return ret;
+ /* + * SSI clock will be output contiguously + * by below settings. + * This means, rsnd_ssi_master_clk_start() + * and rsnd_ssi_register_setup() are necessary + * for SSI parent + * + * SSICR : FORCE, SCKD, SWSD + * SSIWSR : CONT + */ ssi->cr_clk = FORCE | SWL_32 | SCKD | SWSD | CKDV(idx); ssi->wsr = CONT; ssi->rate = rate; @@ -393,7 +404,8 @@ static void rsnd_ssi_register_setup(struct rsnd_mod *mod) rsnd_mod_write(mod, SSIWSR, ssi->wsr); rsnd_mod_write(mod, SSICR, ssi->cr_own | ssi->cr_clk | - ssi->cr_mode); /* without EN */ + ssi->cr_mode | + ssi->cr_en); }
static void rsnd_ssi_pointer_init(struct rsnd_mod *mod, @@ -544,6 +556,8 @@ static int rsnd_ssi_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) { + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); + if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
@@ -554,7 +568,19 @@ static int rsnd_ssi_start(struct rsnd_mod *mod, if (rsnd_ssi_multi_slaves_runtime(io)) return 0;
- rsnd_mod_bset(mod, SSICR, EN, EN); + /* + * EN is for data output. + * SSI parent EN is not needed. + */ + if (rsnd_ssi_is_parent(mod, io)) + return 0; + + ssi->cr_en = EN; + + rsnd_mod_write(mod, SSICR, ssi->cr_own | + ssi->cr_clk | + ssi->cr_mode | + ssi->cr_en);
return 0; } @@ -569,13 +595,7 @@ static int rsnd_ssi_stop(struct rsnd_mod *mod, if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
- /* - * don't stop if not last user - * see also - * rsnd_ssi_start - * rsnd_ssi_interrupt - */ - if (ssi->usrcnt > 1) + if (rsnd_ssi_is_parent(mod, io)) return 0;
/* @@ -595,6 +615,8 @@ static int rsnd_ssi_stop(struct rsnd_mod *mod, rsnd_mod_write(mod, SSICR, cr); /* disabled all */ rsnd_ssi_status_check(mod, IIRQ);
+ ssi->cr_en = 0; + return 0; }
The patch
ASoC: rsnd: control SSICR::EN correctly
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 597b046f0d99b0b0123a715f8c8b1ea881d2bec6 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Tue, 8 Aug 2017 01:31:39 +0000 Subject: [PATCH] ASoC: rsnd: control SSICR::EN correctly
In case of SSI0 playback, SSI1 capture, SSI0 might be shared for clock output if clock master mode.
Current rsnd driver had been assumed that SSI clock contiguous output which is needed for SSI parent needs SSICR::EN (SSI module enable) bit. But, this bit controls data input/output, not for clock. Clock contiguous output needs SSICR : FORCE, SCKD, SWSD, and SSIWSR : CONT. Not SSICR : EN.
Because of this wrong assumption, and insufficient control, on current code, for example, if it starts SSI0(playback) -> SSI1(capture) order, SSI0 SSICR::EN bit will temporarily be 0. It causes playback side underrun error. This is bug. We can reproduce this issue with SSI+SRC (without DVC), and capture during playback operation.
This patch fixup current (wrong) assumption, and control SSICR::EN bit correctly.
Reported-by: Hiroyuki Yokoyama hiroyuki.yokoyama.vx@renesas.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Tested-by: Hiroyuki Yokoyama hiroyuki.yokoyama.vx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/ssi.c | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 46feddd78ee2..c5d5c64152e9 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -72,6 +72,7 @@ struct rsnd_ssi { u32 cr_own; u32 cr_clk; u32 cr_mode; + u32 cr_en; u32 wsr; int chan; int rate; @@ -291,6 +292,16 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (ret < 0) return ret;
+ /* + * SSI clock will be output contiguously + * by below settings. + * This means, rsnd_ssi_master_clk_start() + * and rsnd_ssi_register_setup() are necessary + * for SSI parent + * + * SSICR : FORCE, SCKD, SWSD + * SSIWSR : CONT + */ ssi->cr_clk = FORCE | SWL_32 | SCKD | SWSD | CKDV(idx); ssi->wsr = CONT; ssi->rate = rate; @@ -393,7 +404,8 @@ static void rsnd_ssi_register_setup(struct rsnd_mod *mod) rsnd_mod_write(mod, SSIWSR, ssi->wsr); rsnd_mod_write(mod, SSICR, ssi->cr_own | ssi->cr_clk | - ssi->cr_mode); /* without EN */ + ssi->cr_mode | + ssi->cr_en); }
static void rsnd_ssi_pointer_init(struct rsnd_mod *mod, @@ -544,6 +556,8 @@ static int rsnd_ssi_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) { + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); + if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
@@ -554,7 +568,19 @@ static int rsnd_ssi_start(struct rsnd_mod *mod, if (rsnd_ssi_multi_slaves_runtime(io)) return 0;
- rsnd_mod_bset(mod, SSICR, EN, EN); + /* + * EN is for data output. + * SSI parent EN is not needed. + */ + if (rsnd_ssi_is_parent(mod, io)) + return 0; + + ssi->cr_en = EN; + + rsnd_mod_write(mod, SSICR, ssi->cr_own | + ssi->cr_clk | + ssi->cr_mode | + ssi->cr_en);
return 0; } @@ -569,13 +595,7 @@ static int rsnd_ssi_stop(struct rsnd_mod *mod, if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
- /* - * don't stop if not last user - * see also - * rsnd_ssi_start - * rsnd_ssi_interrupt - */ - if (ssi->usrcnt > 1) + if (rsnd_ssi_is_parent(mod, io)) return 0;
/* @@ -595,6 +615,8 @@ static int rsnd_ssi_stop(struct rsnd_mod *mod, rsnd_mod_write(mod, SSICR, cr); /* disabled all */ rsnd_ssi_status_check(mod, IIRQ);
+ ssi->cr_en = 0; + return 0; }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Make it the same style as other functions
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/ssi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 498e71d..cd0d9a8 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -346,6 +346,9 @@ static void rsnd_ssi_config_init(struct rsnd_mod *mod, u32 wsr; int is_tdm;
+ if (rsnd_ssi_is_parent(mod, io)) + return; + is_tdm = rsnd_runtime_is_ssi_tdm(io);
/* @@ -484,8 +487,7 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, if (ret < 0) return ret;
- if (!rsnd_ssi_is_parent(mod, io)) - rsnd_ssi_config_init(mod, io); + rsnd_ssi_config_init(mod, io);
rsnd_ssi_register_setup(mod);
The patch
ASoC: rsnd: move rsnd_ssi_config_init() execute condition into it.
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From bf23c6a3813aac96e0af01565aba012b25c8bae5 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Tue, 8 Aug 2017 01:32:08 +0000 Subject: [PATCH] ASoC: rsnd: move rsnd_ssi_config_init() execute condition into it.
Make it the same style as other functions
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/ssi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 49cdc5e007b3..3ee7b2de7dea 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -335,6 +335,9 @@ static void rsnd_ssi_config_init(struct rsnd_mod *mod, u32 wsr; int is_tdm;
+ if (rsnd_ssi_is_parent(mod, io)) + return; + is_tdm = rsnd_runtime_is_ssi_tdm(io);
/* @@ -472,8 +475,7 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, if (ret < 0) return ret;
- if (!rsnd_ssi_is_parent(mod, io)) - rsnd_ssi_config_init(mod, io); + rsnd_ssi_config_init(mod, io);
rsnd_ssi_register_setup(mod);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
For example SSI0/SSI1 case, SSI1 can share pin with SSI0. And then, SSI1 needs SSI0's clock. This clock controlling is very picky and difficult to understand in current code.
This patch adds new rsnd_ssi_can_output_clk() macro, the code will be more understandable.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/ssi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index cd0d9a8..25c351f 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -107,6 +107,7 @@ struct rsnd_ssi { (rsnd_ssi_multi_slaves(io) & (1 << rsnd_mod_id(mod))) #define rsnd_ssi_is_run_mods(mod, io) \ (rsnd_ssi_run_mods(io) & (1 << rsnd_mod_id(mod))) +#define rsnd_ssi_can_output_clk(mod) (!__rsnd_ssi_is_pin_sharing(mod))
int rsnd_ssi_hdmi_port(struct rsnd_dai_stream *io) { @@ -256,7 +257,6 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, struct device *dev = rsnd_priv_to_dev(priv); struct rsnd_dai *rdai = rsnd_io_to_rdai(io); struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); - struct rsnd_mod *ssi_parent_mod = rsnd_io_to_mod_ssip(io); int chan = rsnd_runtime_channel_for_ssi(io); int idx, ret; unsigned int main_rate; @@ -267,7 +267,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (!rsnd_rdai_is_clk_master(rdai)) return 0;
- if (ssi_parent_mod && !rsnd_ssi_is_parent(mod, io)) + if (!rsnd_ssi_can_output_clk(mod)) return 0;
if (rsnd_ssi_is_multi_slave(mod, io)) @@ -318,12 +318,11 @@ static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, { struct rsnd_dai *rdai = rsnd_io_to_rdai(io); struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); - struct rsnd_mod *ssi_parent_mod = rsnd_io_to_mod_ssip(io);
if (!rsnd_rdai_is_clk_master(rdai)) return;
- if (ssi_parent_mod && !rsnd_ssi_is_parent(mod, io)) + if (!rsnd_ssi_can_output_clk(mod)) return;
if (ssi->usrcnt > 1)
The patch
ASoC: rsnd: rsnd_ssi_can_output_clk() macro
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From bb002f925ae18c0c9d72d87b74b311a5209d7178 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Tue, 8 Aug 2017 01:32:42 +0000 Subject: [PATCH] ASoC: rsnd: rsnd_ssi_can_output_clk() macro
For example SSI0/SSI1 case, SSI1 can share pin with SSI0. And then, SSI1 needs SSI0's clock. This clock controlling is very picky and difficult to understand in current code.
This patch adds new rsnd_ssi_can_output_clk() macro, the code will be more understandable.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/ssi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index 3ee7b2de7dea..434996d4054a 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -106,6 +106,7 @@ struct rsnd_ssi { (rsnd_ssi_multi_slaves(io) & (1 << rsnd_mod_id(mod))) #define rsnd_ssi_is_run_mods(mod, io) \ (rsnd_ssi_run_mods(io) & (1 << rsnd_mod_id(mod))) +#define rsnd_ssi_can_output_clk(mod) (!__rsnd_ssi_is_pin_sharing(mod))
int rsnd_ssi_hdmi_port(struct rsnd_dai_stream *io) { @@ -255,7 +256,6 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, struct device *dev = rsnd_priv_to_dev(priv); struct rsnd_dai *rdai = rsnd_io_to_rdai(io); struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); - struct rsnd_mod *ssi_parent_mod = rsnd_io_to_mod_ssip(io); int chan = rsnd_runtime_channel_for_ssi(io); int idx, ret; unsigned int main_rate; @@ -266,7 +266,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (!rsnd_rdai_is_clk_master(rdai)) return 0;
- if (ssi_parent_mod && !rsnd_ssi_is_parent(mod, io)) + if (!rsnd_ssi_can_output_clk(mod)) return 0;
if (rsnd_ssi_is_multi_slave(mod, io)) @@ -307,12 +307,11 @@ static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, { struct rsnd_dai *rdai = rsnd_io_to_rdai(io); struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); - struct rsnd_mod *ssi_parent_mod = rsnd_io_to_mod_ssip(io);
if (!rsnd_rdai_is_clk_master(rdai)) return;
- if (ssi_parent_mod && !rsnd_ssi_is_parent(mod, io)) + if (!rsnd_ssi_can_output_clk(mod)) return;
if (ssi->usrcnt > 1)
participants (2)
-
Kuninori Morimoto -
Mark Brown