[alsa-devel] [PATCH 1/2] ASoC: rt5670: Revert Keep sysclk on patch
The "Keep sysclk on if JD func is used" patch force enable/disable pin in rt5670_set_dai_sysclk. But some machine driver call it in dapm widget event. It will cause kernel crash.
Signed-off-by: Bard Liao bardliao@realtek.com --- sound/soc/codecs/rt5670.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c index 592f961..32cd266 100644 --- a/sound/soc/codecs/rt5670.c +++ b/sound/soc/codecs/rt5670.c @@ -2273,13 +2273,6 @@ static int rt5670_set_dai_sysclk(struct snd_soc_dai *dai, if (freq == rt5670->sysclk && clk_id == rt5670->sysclk_src) return 0;
- if (rt5670->pdata.jd_mode) { - if (clk_id == RT5670_SCLK_S_PLL1) - snd_soc_dapm_force_enable_pin(&codec->dapm, "PLL1"); - else - snd_soc_dapm_disable_pin(&codec->dapm, "PLL1"); - snd_soc_dapm_sync(&codec->dapm); - } switch (clk_id) { case RT5670_SCLK_S_MCLK: reg_val |= RT5670_SCLK_SRC_MCLK; @@ -2724,10 +2717,6 @@ static int rt5670_i2c_probe(struct i2c_client *i2c, }
if (rt5670->pdata.jd_mode) { - regmap_update_bits(rt5670->regmap, RT5670_GLB_CLK, - RT5670_SCLK_SRC_MASK, RT5670_SCLK_SRC_RCCLK); - rt5670->sysclk = 0; - rt5670->sysclk_src = RT5670_SCLK_S_RCCLK; regmap_update_bits(rt5670->regmap, RT5670_PWR_ANLG1, RT5670_PWR_MB, RT5670_PWR_MB); regmap_update_bits(rt5670->regmap, RT5670_PWR_ANLG2,
Currently, is_sys_clk_from_pll check sysclk source by reading codec register value. And it will be invoked before updating dapm widget power. In some machine driver, snd_soc_dai_set_sysclk is called in dapm event to switch codec sysclk to RC clock in idle mode. And in some use cases (such as syspend/resume) hw_params will not be called when the dapm widget is powered up. As a result, is_sys_clk_from_pll will return 0 although it is supposed to be 1. To solve this, we let is_sys_clk_from_pll check sysclk sysclk_src which is stored in private data and don't change the value of sysclk_src when codec sysclk is switched to internal clock. The internal clock can only be used in idle mode, so it sould be fine if we don't set sysclk_src to internal clock.
Signed-off-by: Bard Liao bardliao@realtek.com --- sound/soc/codecs/rt5670.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c index 32cd266..9e3bc43 100644 --- a/sound/soc/codecs/rt5670.c +++ b/sound/soc/codecs/rt5670.c @@ -517,11 +517,10 @@ static int set_dmic_clk(struct snd_soc_dapm_widget *w, static int is_sys_clk_from_pll(struct snd_soc_dapm_widget *source, struct snd_soc_dapm_widget *sink) { - unsigned int val; + struct snd_soc_codec *codec = source->codec; + struct rt5670_priv *rt5670 = snd_soc_codec_get_drvdata(codec);
- val = snd_soc_read(source->codec, RT5670_GLB_CLK); - val &= RT5670_SCLK_SRC_MASK; - if (val == RT5670_SCLK_SRC_PLL1) + if (rt5670->sysclk_src == RT5670_SCLK_S_PLL1) return 1; else return 0; @@ -2270,9 +2269,6 @@ static int rt5670_set_dai_sysclk(struct snd_soc_dai *dai, struct rt5670_priv *rt5670 = snd_soc_codec_get_drvdata(codec); unsigned int reg_val = 0;
- if (freq == rt5670->sysclk && clk_id == rt5670->sysclk_src) - return 0; - switch (clk_id) { case RT5670_SCLK_S_MCLK: reg_val |= RT5670_SCLK_SRC_MCLK; @@ -2290,7 +2286,8 @@ static int rt5670_set_dai_sysclk(struct snd_soc_dai *dai, snd_soc_update_bits(codec, RT5670_GLB_CLK, RT5670_SCLK_SRC_MASK, reg_val); rt5670->sysclk = freq; - rt5670->sysclk_src = clk_id; + if (clk_id != RT5670_SCLK_S_RCCLK) + rt5670->sysclk_src = clk_id;
dev_dbg(dai->dev, "Sysclk is %dHz and clock id is %d\n", freq, clk_id);
On Mon, Mar 09, 2015 at 04:55:22PM +0800, Bard Liao wrote:
The "Keep sysclk on if JD func is used" patch force enable/disable pin in rt5670_set_dai_sysclk. But some machine driver call it in dapm widget event. It will cause kernel crash.
Could you go into more detail on what's going wrong here please - this sounds like the machine drivers are buggy, not the jack detection code? Configuring jack detection in response to a DAPM event would be very unusual.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Monday, March 09, 2015 6:50 PM To: Bard Liao Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; lars@metafoo.de; Flove; Oder Chiou; John Lin; mengdong.lin@intel.com; yao.jin@intel.com Subject: Re: [PATCH 1/2] ASoC: rt5670: Revert Keep sysclk on patch
On Mon, Mar 09, 2015 at 04:55:22PM +0800, Bard Liao wrote:
The "Keep sysclk on if JD func is used" patch force enable/disable pin in rt5670_set_dai_sysclk. But some machine driver call it in dapm widget event. It will cause kernel crash.
Could you go into more detail on what's going wrong here please - this sounds like the machine drivers are buggy, not the jack detection code? Configuring jack detection in response to a DAPM event would be very unusual.
System clock is needed for rt5670 jack detection function. Usually, the system clock source is from MCLK. But some platform such as Intel BSW will turn off MCLK in idle mode. So rt5670 need to switch the system clock source to its internal clock. The action is done by machine driver.
There is a PLL power in rt5670. It should be on if we want to use PLL as the system clock source. So we call force enable/disable pin in rt5670_set_dai_sysclk. However, cht_bsw_rt5672.c call snd_soc_dai_set_sysclk in platform_clock_control which is the dapm event of "Platform Clock" widget. In that case, snd_soc_dapm_disable_pin and snd_soc_dapm_sync are called within a dapm event. I think it is not valid.
Thanks.
------Please consider the environment before printing this e-mail.
On Mon, Mar 09, 2015 at 12:13:04PM +0000, Bard Liao wrote:
System clock is needed for rt5670 jack detection function. Usually, the system clock source is from MCLK. But some platform such as Intel BSW will turn off MCLK in idle mode. So rt5670 need to switch the system clock source to its internal clock. The action is done by machine driver.
OK... but if this is the case why does your patch not update any machine drivers?
There is a PLL power in rt5670. It should be on if we want to use PLL as the system clock source. So we call force enable/disable pin in rt5670_set_dai_sysclk. However, cht_bsw_rt5672.c call snd_soc_dai_set_sysclk in platform_clock_control which is the dapm event of "Platform Clock" widget. In that case, snd_soc_dapm_disable_pin and snd_soc_dapm_sync are called within a dapm event. I think it is not valid.
This is very unusual, normally such system clock reconfiguration would be done in the set_bias_level() callback not in a DAPM event. Doing it in a DAPM event sounds unreliable as there are only weak guarantees about the state of the system.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Monday, March 09, 2015 8:20 PM To: Bard Liao Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; lars@metafoo.de; Flove; Oder Chiou; John Lin; mengdong.lin@intel.com; yao.jin@intel.com Subject: Re: [PATCH 1/2] ASoC: rt5670: Revert Keep sysclk on patch
On Mon, Mar 09, 2015 at 12:13:04PM +0000, Bard Liao wrote:
System clock is needed for rt5670 jack detection function. Usually, the system clock source is from MCLK. But some platform such as Intel BSW will turn off MCLK in idle mode. So rt5670 need to switch the system clock source to its internal clock. The action is done by machine driver.
OK... but if this is the case why does your patch not update any machine drivers?
The sysclk source switching is already in cht_bsw_rt5672.c. But it switches to internal clock only but doesn't switch back to PLL. Jin Yao will send a patch to fix it.
There is a PLL power in rt5670. It should be on if we want to use PLL as the system clock source. So we call force enable/disable pin in rt5670_set_dai_sysclk. However, cht_bsw_rt5672.c call snd_soc_dai_set_sysclk in platform_clock_control which is the dapm event of "Platform Clock" widget. In that case, snd_soc_dapm_disable_pin and snd_soc_dapm_sync are called within a dapm event. I think it is not valid.
This is very unusual, normally such system clock reconfiguration would be done in the set_bias_level() callback not in a DAPM event. Doing it in a DAPM event sounds unreliable as there are only weak guarantees about the state of the system.
------Please consider the environment before printing this e-mail.
On Mon, Mar 09, 2015 at 04:55:22PM +0800, Bard Liao wrote:
The "Keep sysclk on if JD func is used" patch force enable/disable pin in rt5670_set_dai_sysclk. But some machine driver call it in dapm widget event. It will cause kernel crash.
Applied both, thanks.
participants (2)
-
Bard Liao -
Mark Brown