Re: [PATCH 1/3] ASoC: simple-card-utils: Fix device module clock

[Michael Walle <michael@walle.cc>]

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.