From: Andrejs Cainikovs <[email protected]>
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.
Thanks,
Andrejs.
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
Signed-off-by: Andrejs Cainikovs <[email protected]>
---
sound/soc/codecs/wm8904.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
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;
}
--
2.34.1
On Fri, Mar 08, 2024 at 04:58:31PM +0100, Andrejs Cainikovs wrote:
> From: Andrejs Cainikovs <[email protected]>
>
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> 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