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

[Sameer Pujar <spujar@nvidia.com>]

On 3/10/2021 4:00 AM, Michael Walle wrote:
> Am 2021-03-09 17:27, schrieb Sameer Pujar:
>> On 3/9/2021 8:11 PM, Michael Walle wrote:
>>>> If "clocks = <&xxx>" is specified from the CPU or Codec component
>>>> device node, the clock is not getting enabled. Thus audio playback
>>>> or capture fails.
>>>>
>>>> Fix this by populating "simple_dai->clk" field when clocks property
>>>> is specified from device node as well. Also tidy up by re-organising
>>>> conditional statements of parsing logic.
>>>>
>>>> Fixes: bb6fc620c2ed ("ASoC: simple-card-utils: add
>>>> asoc_simple_card_parse_clk()")
>>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>>>
>>> This actually breaks sound on my board
>>> (arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts).
>>> The codec on this board (wm8904) has a fixed clock input (only
>>> distinct
>>> frequencies are supported) and uses the FLL of the codec to generate
>>> the
>>> desired sample rate.
>>>
>>> It seems that after this patch the clock rate of the codecs clock
>>> (rather
>>> than the FLL) is tried to be changed. Which fails, because it doesn't
>>> support arbitrary frequencies.
>>
>> Yes, after the given change the clock will be updated if "*mclk-fs"
>> property is specified.
>>
>> DT you mentioned has property "simple-audio-card,mclk-fs = <256>",
>> which means you need a clock that is a function of sample rate. But as
>> per above you want a fixed clock for MCLK. I think if you drop this
>> property, the clock updates won't happen. Earlier for your case, this
>> property was not used at all because the clock handle was not
>> populated.
>
> You mean to drop the mclk-fs property? I can't do that because I
> actually need a frequency of 256 * sample rate. But that doesn't
> necessarily need to be the MCLK, because the codec itself has a
> FLL/PLL which can be used to generate any frequency for a given
> MCLK. So that is a valid scenario. See also commit 13409d27cb39
> ("ASoC: wm8904: configure sysclk/FLL automatically").
>

If I read this correctly below is the configuration you need,
SoC -> MCLK(fixed rate) -> PLL(wm8904) -> PLL output (256 * fs) -> sysclk

 From the doc simple-card.txt, "simple-audio-card,mclk-fs" is a scaling 
factor for MCLK and hence I am not sure if it is correct to have 
"*mclk-fs" property when MCLK is fixed. In simple cases, codec sysclk 
direclty depends on MCLK and set_sysclk() callback helps. Your case 
requires PLL configuration and set_pll() may be a better alternative. 
However simple-card does not offer this yet. But even if this is added, 
there should be a way to suggest PLL output requirement as a function of 
sample rate.