Am 2021-03-15 16:19, schrieb Sameer Pujar:
On 3/15/2021 5:35 PM, Michael Walle wrote:
External email: Use caution opening links or attachments
Am 2021-03-12 14:46, schrieb Mark Brown:
On Fri, Mar 12, 2021 at 01:30:02PM +0100, Michael Walle wrote:
The card calls set_sysclk(), which eventually ends up in the codec.
The codec therefore, could figure out if it needs to configure the
clock or if it can use its internal FLL.
Is that what you mean?
Yes.
But the set_sysclk() of the codec isn't even called, because the
card itself already tries to call clk_set_rate() on the Codec's
MCLK,
which returns with an error [0].
OK, so I think we need to push this down a level so that the clock
setting is implemented by the core/CODEC rather than by simple-card,
with the helpers being something the CODEC can opt out of.
Sameer, it looks like the proper fix should be to add the clock
support to your codec.
I agree that complicated clock relationships should be handled within
the codec itself, however MCLK rate setting depends on "mclk-fs"
factor and this property is specified as part of
simple-card/audio-graph-card codec subnode. Right now codec, in
general, does not have a way to know this. The set_sysclk() callback
takes rate argument and not the factor.
Why would you need the factor?
Moreover the same codec is
used by other platform vendors too and unless a new DT property is
added for codec, runtime MCLK update based on the scaling factor
cannot be supported.
Could you test the following:
diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
index 67f0ab817135..7fd41f51f856 100644
--- a/sound/soc/codecs/rt5659.c
+++ b/sound/soc/codecs/rt5659.c
@@ -3426,12 +3426,18 @@ static int rt5659_set_component_sysclk(struct
snd_soc_component *component, int
{
struct rt5659_priv *rt5659 =
snd_soc_component_get_drvdata(component);
unsigned int reg_val = 0;
+ int ret;
if (freq == rt5659->sysclk && clk_id == rt5659->sysclk_src)
return 0;
switch (clk_id) {
case RT5659_SCLK_S_MCLK:
+ ret = clk_set_rate(rt5659->mclk, freq);
+ if (ret)
+ return ret;
reg_val |= RT5659_SCLK_SRC_MCLK;
break;
case RT5659_SCLK_S_PLL1:
-michael
This would mean that we will be having two
methods to specify "mclk-fs" factor, one from
simple-card/audio-graph-card and one from respective codec nodes,
which does not seem ideal.
Also it does not seem consistent with the way we handle MCLK clock
based on where it is specified.
a) If specified in simple-card/audio-graph-card, MCLK clock
rate/enable/disable updates are allowed.
b) If specified in codec device node, it is not expected to touch
the MCLK clock. This patch tried to treat it the same way as (a) does.
Advantage of this is, all codec drivers need not explicitly handle
MCLK, instead it is done at a central place. The platforms which use
specific machine drivers do the same and that is why probably the
codec driver patch was never required. It is about just setting MCLK
clock as a factor of sample rate, whenever the factor is available. I
understand that it is breaking your use case, but I am not sure if the
usage of set_sysclk() is consistent. I mean, it takes the "freq"
argument. Does it refer to MCLK rate or system-clock (sysclk) rate?
MCLK and sysclk are not really the same when codec PLL is involved. So
I would like to understand clearly about what "freq" argument means.
Also when "mclk-fs" factor is specified, is it related to MCLK or
sysclk? My understanding is that it should be strictly viewed as
related to MCLK.
Does it help if a separate helper is used for audio-graph-card with
current change and reverting the simple-card to its previous state?
Morimoto-san, does it affect any other users of audio-graph-card?
I've also looked at other users of "simple-audio-card" and
it looks like they will break too. For example,
- arch/arm64/boot/dts/rockchip/rk3399.dtsi
If I'm not mistaken, this will try to set the first clock
of hdmi@ff940000 there, which is "iahb".
- arch/arm/boot/dts/sun8i-a33.dtsi
There "&ccu CLK_BUS_CODEC" of codec@1c22e00 will be changed
And it doesn't stop there, it also sets the first clock
of the CPU endpoint, which I guess just works because .set_rate
is a noop for the most clocks which are used there.
Yes this is a problem, unfortunately I missed checking some of the
simple-card examples. I wonder we should be specifically looking for
"mclk" clock here.
