| Message ID | 20240308155831.141229-1-andrejs.cainikovs@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,v1] ASoC: wm8904: enable fll with fixed mclk | expand |
On Fri, Mar 08, 2024 at 04:58:31PM +0100, Andrejs Cainikovs wrote: > From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com> > > Dear all, > > This is an attempt to change wm8904 for a scenario when reference clock > supposed to be fixed to a particular frequency, but is not configured as > a fixed-clock. While this change is working fine, I'm struggling to > finalize it, not being able to find a proper solution of adding a check > whether we want to use fixed MCLK with codec's FLL or not. I could, of > course, introduce a new device tree property, but do not feel this would > be a proper way forward. Hence, I'm sending out this RFC patch to gather > your valuable feedback. > > DUT: Dahlia carrier board with Verdin TI AM62 SOM. > Audio card configuration can be found in ti/k3-am62-verdin-dahlia.dtsi. At some point one really starts to question if this is really a "simple card" any more. > On systems with a fixed reference clock output rate it > is impossible to use this clock for all audio frequencies. > > Following is an example of playing a 44100Hz audio on a system > with a fixed 25MHz reference clock applied to wm8904 codec, > in combination with simple-audio-card without mclk-fs: > > [ 27.013564] wm8904 1-001a: Target BCLK is 1411200Hz > [ 27.013601] wm8904 1-001a: Using 25000000Hz MCLK > [ 27.013611] wm8904 1-001a: CLK_SYS is 12500000Hz > [ 27.013654] wm8904 1-001a: Selected CLK_SYS_RATIO of 256 > [ 27.013663] wm8904 1-001a: Selected SAMPLE_RATE of 44100Hz > [ 27.013671] wm8904 1-001a: Selected BCLK_DIV of 80 for 1562500Hz BCLK > [ 27.013680] wm8904 1-001a: LRCLK_RATE is 35 > > This leads to a distorted sound and this configuration is unusable. > > On the other hand, configuring simple-audio-card with mclk-fs will > force the system to change MCLK frequency, which supposed to be fixed. > > This change forces to use wm8904 FLL while keeping SoC's MCLK > frequency intact: > > [ 234.108149] wm8904 1-001a: Target BCLK is 1411200Hz > [ 234.108304] wm8904 1-001a: Using 0Hz FLL clock > [ 234.108722] wm8904 1-001a: FLL configured for 25000000Hz->1411200Hz > [ 234.108794] wm8904 1-001a: CLK_SYS is 1411200Hz > [ 234.108835] wm8904 1-001a: Selected CLK_SYS_RATIO of 64 > [ 234.108875] wm8904 1-001a: Selected SAMPLE_RATE of 44100Hz > [ 234.108913] wm8904 1-001a: Selected BCLK_DIV of 10 for 1411200Hz BCLK > [ 234.108955] wm8904 1-001a: LRCLK_RATE is 32 > Hmm... the driver already provides an option to automatically configure the clock. Is the issue here that in your fail case the machine driver never calls wm8904_set_sysclk? Or if it does call it, when and what parameters is it passing? Thanks, Charles
On Mon, Mar 11, 2024 at 11:35:53AM +0000, Charles Keepax wrote: > On Fri, Mar 08, 2024 at 04:58:31PM +0100, Andrejs Cainikovs wrote: > > From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com> > > > > Dear all, > > > > This is an attempt to change wm8904 for a scenario when reference clock > > supposed to be fixed to a particular frequency, but is not configured as > > a fixed-clock. While this change is working fine, I'm struggling to > > finalize it, not being able to find a proper solution of adding a check > > whether we want to use fixed MCLK with codec's FLL or not. I could, of > > course, introduce a new device tree property, but do not feel this would > > be a proper way forward. Hence, I'm sending out this RFC patch to gather > > your valuable feedback. > > > > DUT: Dahlia carrier board with Verdin TI AM62 SOM. > > Audio card configuration can be found in ti/k3-am62-verdin-dahlia.dtsi. > > At some point one really starts to question if this is really a > "simple card" any more. > > > On systems with a fixed reference clock output rate it > > is impossible to use this clock for all audio frequencies. > > > > Following is an example of playing a 44100Hz audio on a system > > with a fixed 25MHz reference clock applied to wm8904 codec, > > in combination with simple-audio-card without mclk-fs: > > > > [ 27.013564] wm8904 1-001a: Target BCLK is 1411200Hz > > [ 27.013601] wm8904 1-001a: Using 25000000Hz MCLK > > [ 27.013611] wm8904 1-001a: CLK_SYS is 12500000Hz > > [ 27.013654] wm8904 1-001a: Selected CLK_SYS_RATIO of 256 > > [ 27.013663] wm8904 1-001a: Selected SAMPLE_RATE of 44100Hz > > [ 27.013671] wm8904 1-001a: Selected BCLK_DIV of 80 for 1562500Hz BCLK > > [ 27.013680] wm8904 1-001a: LRCLK_RATE is 35 > > > > This leads to a distorted sound and this configuration is unusable. > > > > On the other hand, configuring simple-audio-card with mclk-fs will > > force the system to change MCLK frequency, which supposed to be fixed. > > > > This change forces to use wm8904 FLL while keeping SoC's MCLK > > frequency intact: > > > > [ 234.108149] wm8904 1-001a: Target BCLK is 1411200Hz > > [ 234.108304] wm8904 1-001a: Using 0Hz FLL clock > > [ 234.108722] wm8904 1-001a: FLL configured for 25000000Hz->1411200Hz > > [ 234.108794] wm8904 1-001a: CLK_SYS is 1411200Hz > > [ 234.108835] wm8904 1-001a: Selected CLK_SYS_RATIO of 64 > > [ 234.108875] wm8904 1-001a: Selected SAMPLE_RATE of 44100Hz > > [ 234.108913] wm8904 1-001a: Selected BCLK_DIV of 10 for 1411200Hz BCLK > > [ 234.108955] wm8904 1-001a: LRCLK_RATE is 32 > > Charles, With or without mclk-fs wm8904_set_sysclk() is called always during probe, with following parameters: clk_id = 0 dir = 0 freq = 25000000 mclk_freq = 25000000 When mclk-fs is set, wm8904_set_sysclk() is also called before each playback [1]. In case of 44.1kHz: clk_id = 0 dir = 0 freq = 11289600 mclk_freq = 11235955 In both scenarios, clk_id is always WM8904_CLK_AUTO. [1]: https://elixir.bootlin.com/linux/latest/source/sound/soc/generic/simple-card-utils.c#L441 Best regards, Andrejs > Hmm... the driver already provides an option to automatically > configure the clock. Is the issue here that in your fail case the > machine driver never calls wm8904_set_sysclk? Or if it does call > it, when and what parameters is it passing? > > Thanks, > Charles
On Tue, Mar 12, 2024 at 01:13:40PM +0100, Andrejs Cainikovs wrote: > On Mon, Mar 11, 2024 at 11:35:53AM +0000, Charles Keepax wrote: > > On Fri, Mar 08, 2024 at 04:58:31PM +0100, Andrejs Cainikovs wrote: > > > From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com> > With or without mclk-fs wm8904_set_sysclk() is called always during probe, > with following parameters: > > clk_id = 0 > dir = 0 > freq = 25000000 > mclk_freq = 25000000 > > When mclk-fs is set, wm8904_set_sysclk() is also called before each > playback [1]. In case of 44.1kHz: > > clk_id = 0 > dir = 0 > freq = 11289600 > mclk_freq = 11235955 > > In both scenarios, clk_id is always WM8904_CLK_AUTO. Ah, ok I see what I was missing here. simple_card_utils calls clk_set_rate if you specify mclk_fs. Which you don't want in this case. My gut reaction is that really the problem here is the machine driver doesn't support the clocking setup you have. Having a quick look through the simple card stuff can you remove: clocks = <&audio_refclk1>; From the machine driver DT stuff, and add mclk-fs. I think that should cause the simple card to call the codec dai_set_sysclk but without ever touching the audio_refclk. A small change in simple_util_parse_clk might also be needed to allow it to return without finding a clock. Which feels like a much simpler and less scary change. My only slight reservation is the automatic clocking thing only really exists as a hack to support simple card anyway. But overal I think it might be better to try to move the direction of travel more to adding support for the clocking systems that exist into simple-card rather than tweaking the codec driver to work around it. Thanks, Charles
On Tue, Mar 12, 2024 at 01:51:02PM +0000, Charles Keepax wrote: > Ah, ok I see what I was missing here. simple_card_utils calls > clk_set_rate if you specify mclk_fs. Which you don't want in this > case. My gut reaction is that really the problem here is the machine > driver doesn't support the clocking setup you have. > > Having a quick look through the simple card stuff can you remove: > > clocks = <&audio_refclk1>; > > From the machine driver DT stuff, and add mclk-fs. I think that > should cause the simple card to call the codec dai_set_sysclk > but without ever touching the audio_refclk. A small change in > simple_util_parse_clk might also be needed to allow it to return > without finding a clock. Which feels like a much simpler and less > scary change. Thanks for your help, Charles. Your suggestion works as expected. Btw, there was no need to touch simple_util_parse_clk as it does allow to return without finding a clock. Best regards, Andrejs. > My only slight reservation is the automatic clocking thing only > really exists as a hack to support simple card anyway. But overal > I think it might be better to try to move the direction of travel > more to adding support for the clocking systems that exist into > simple-card rather than tweaking the codec driver to work around > it. > > Thanks, > Charles
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c index 829bf055622a7..033ef1d67ac50 100644 --- a/sound/soc/codecs/wm8904.c +++ b/sound/soc/codecs/wm8904.c @@ -311,10 +311,17 @@ static bool wm8904_readable_register(struct device *dev, unsigned int reg) } } -static int wm8904_configure_clocking(struct snd_soc_component *component) +/* Forward declaration */ +static int wm8904_set_fll(struct snd_soc_dai *dai, int fll_id, int source, + unsigned int Fref, unsigned int Fout); + +static int wm8904_configure_clocking(struct snd_soc_dai *dai) { + struct snd_soc_component *component = dai->component; struct wm8904_priv *wm8904 = snd_soc_component_get_drvdata(component); unsigned int clock0, clock2, rate; + unsigned long mclk_freq; + int ret; /* Gate the clock while we're updating to avoid misclocking */ clock2 = snd_soc_component_read(component, WM8904_CLOCK_RATES_2); @@ -338,6 +345,14 @@ static int wm8904_configure_clocking(struct snd_soc_component *component) dev_dbg(component->dev, "Using %dHz FLL clock\n", wm8904->fll_fout); + mclk_freq = clk_get_rate(wm8904->mclk); + ret = wm8904_set_fll(dai, WM8904_FLL_MCLK, WM8904_FLL_MCLK, + mclk_freq, wm8904->bclk); + if (ret) { + dev_err(component->dev, "Could not configure FLL\n"); + return ret; + } + clock2 |= WM8904_SYSCLK_SRC; rate = wm8904->fll_fout; break; @@ -1334,7 +1349,7 @@ static int wm8904_hw_params(struct snd_pcm_substream *substream, dev_dbg(component->dev, "Target BCLK is %dHz\n", wm8904->bclk); - ret = wm8904_configure_clocking(component); + ret = wm8904_configure_clocking(dai); if (ret != 0) return ret; @@ -1809,7 +1824,6 @@ static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id, struct snd_soc_component *component = dai->component; struct wm8904_priv *priv = snd_soc_component_get_drvdata(component); unsigned long mclk_freq; - int ret; switch (clk_id) { case WM8904_CLK_AUTO: @@ -1820,17 +1834,11 @@ static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id, return 0; mclk_freq = clk_get_rate(priv->mclk); - /* enable FLL if a different sysclk is desired */ - if (mclk_freq != freq) { - priv->sysclk_src = WM8904_CLK_FLL; - ret = wm8904_set_fll(dai, WM8904_FLL_MCLK, - WM8904_FLL_MCLK, - mclk_freq, freq); - if (ret) - return ret; - break; - } - clk_id = WM8904_CLK_MCLK; + /* TODO: add a check to force fll with fixed mclk */ + if (mclk_freq != freq || 1) + clk_id = WM8904_CLK_FLL; + else + clk_id = WM8904_CLK_MCLK; fallthrough; case WM8904_CLK_MCLK: @@ -1848,7 +1856,7 @@ static int wm8904_set_sysclk(struct snd_soc_dai *dai, int clk_id, dev_dbg(dai->dev, "Clock source is %d at %uHz\n", clk_id, freq); - wm8904_configure_clocking(component); + wm8904_configure_clocking(dai); return 0; }