[alsa-devel] [PATCH] ASoC: omap-mcbsp: Do not attempt to change DAI sysclk if stream is active
Attempt to change McBSP CLKS source while another stream is active is not safe after commit d135865 ("OMAP: McBSP: implement functional clock switching via clock framework") in 2.6.37.
CLKS parent clock switching using clock framework have to idle the McBSP before switching and then activate it again. This short break can cause a DMA transaction error to already running stream which halts and recovers only by closing and restarting the stream.
This goes more fatal after commit e2fa61d ("OMAP3: l3: Introduce l3-interconnect error handling driver") in 2.6.39 where l3 driver detects a severe timeout error and does BUG_ON().
Fix this by checking is the McBSP active in omap_mcbsp_dai_set_dai_sysclk before attempting to change any clocking configuration. This test should have been here just from the beginning anyway.
Signed-off-by: Jarkko Nikula jhnikula@gmail.com Cc: stable@kernel.org --- For 2.6.37 and onward.
This issue is valid to those machine drivers that use the McBSP as a DAI link master and are configuring the CLKS source (like sound/soc/omap/omap3pandora.c in mainline). --- sound/soc/omap/omap-mcbsp.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c index 07b7723..fc896c1 100644 --- a/sound/soc/omap/omap-mcbsp.c +++ b/sound/soc/omap/omap-mcbsp.c @@ -516,6 +516,9 @@ static int omap_mcbsp_dai_set_dai_sysclk(struct snd_soc_dai *cpu_dai, struct omap_mcbsp_reg_cfg *regs = &mcbsp_data->regs; int err = 0;
+ if (mcbsp_data->active) + return 0; + /* The McBSP signal muxing functions are only available on McBSP1 */ if (clk_id == OMAP_MCBSP_CLKR_SRC_CLKR || clk_id == OMAP_MCBSP_CLKR_SRC_CLKX ||
On Fri, Jun 10, 2011 at 02:28:55PM +0300, Jarkko Nikula wrote:
- if (mcbsp_data->active)
return 0;
Shouldn't we be returning -EBUSY or something here so the caller knows we didn't do what it asked (after checking to make sure it's not just trying to set the existing configuration)? Otherwise it might get surprised, assume the new configuration and just move the bug elsewhere.
On Fri, 10 Jun 2011 12:35:30 +0100 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Fri, Jun 10, 2011 at 02:28:55PM +0300, Jarkko Nikula wrote:
- if (mcbsp_data->active)
return 0;Shouldn't we be returning -EBUSY or something here so the caller knows we didn't do what it asked (after checking to make sure it's not just trying to set the existing configuration)? Otherwise it might get surprised, assume the new configuration and just move the bug elsewhere.
I was thinking to return -EBUSY here but went thinking is it really worth of trouble? Machine drivers are calling this from their hw_params callback with the same configuration always (I suppose) for both playback and capture and it sounded kind of overkill to check what is existing configuration or skip the -EBUSY error.
On Fri, Jun 10, 2011 at 02:56:10PM +0300, Jarkko Nikula wrote:
I was thinking to return -EBUSY here but went thinking is it really worth of trouble? Machine drivers are calling this from their hw_params callback with the same configuration always (I suppose) for both playback and capture and it sounded kind of overkill to check what is existing configuration or skip the -EBUSY error.
One fairly common case is selecting the system clock rate to a fixed multiple of the sample rate so applications that try to set up different playback and record rates trigger issues.
On Fri, 10 Jun 2011 12:59:54 +0100 Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Fri, Jun 10, 2011 at 02:56:10PM +0300, Jarkko Nikula wrote:
I was thinking to return -EBUSY here but went thinking is it really worth of trouble? Machine drivers are calling this from their hw_params callback with the same configuration always (I suppose) for both playback and capture and it sounded kind of overkill to check what is existing configuration or skip the -EBUSY error.
One fairly common case is selecting the system clock rate to a fixed multiple of the sample rate so applications that try to set up different playback and record rates trigger issues.
Umm, yeah. I'll cook another version which returns -EBUSY but still trying to keep this regression fix minimal. Anyway it's only pandora that can trigger the regression in mainline.
On Friday 10 June 2011 14:28:55 Jarkko Nikula wrote:
Attempt to change McBSP CLKS source while another stream is active is not safe after commit d135865 ("OMAP: McBSP: implement functional clock switching via clock framework") in 2.6.37.
CLKS parent clock switching using clock framework have to idle the McBSP before switching and then activate it again. This short break can cause a DMA transaction error to already running stream which halts and recovers only by closing and restarting the stream.
This goes more fatal after commit e2fa61d ("OMAP3: l3: Introduce l3-interconnect error handling driver") in 2.6.39 where l3 driver detects a severe timeout error and does BUG_ON().
Fix this by checking is the McBSP active in omap_mcbsp_dai_set_dai_sysclk before attempting to change any clocking configuration. This test should have been here just from the beginning anyway.
Signed-off-by: Jarkko Nikula jhnikula@gmail.com Cc: stable@kernel.org
Acked-by: Peter Ujfalusi peter.ujfalusi@ti.com
participants (3)
-
Jarkko Nikula -
Mark Brown -
Péter Ujfalusi