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

[Michael Walle]

Am 2021-03-11 15:29, schrieb Sameer Pujar:
> On 3/11/2021 4:46 PM, Michael Walle wrote:
> > Am 2021-03-11 12:05, schrieb Sameer Pujar:
> >
> > > It would work and initially I had similar patch, see [0] and related
> > > series. Suggestion is to always use "clocks" property with devices
> > > only.
> >
> > I see. But again, I don't think it is correct to change the clock of
> > the codec by default. What happens if this is for example a
> > compatible = "fixed-clock"?
>
> The codec rate won't be changed unless a corresponding "*mclk-fs" is 
> provided.
>
> > As you pointed out in the referred thread [0]. simple-audio-card has
> > that clock and judging from the code it is exactly for this reason:
> > to either change/enable it or not.
>
>
> > With this patch you'll switch that to "always change it". Therefore,
> > shouldn't there be a dt flag to indicate wheter 
> > simple-audio-card/graph
> > should be in charge of the codecs clock input?
>
> As mentioned above, it does not change always. Requires "*mclk-fs" to 
> do so.

As mentioned earlier, this is changing the sysclk, too. And I'd guess
most codecs need a fixed factor for the sysclk, thus if the codec 
supports
generating its own sysclk by a PLL it will need this factor, too.

Which is also defined in the binding:

  system-clock-frequency:
    description: |
      If a clock is specified and a multiplication factor is given with
      mclk-fs, the clock will be set to the calculated mclk frequency
      when the stream starts.


> May be below could be a possible alternative?
> - Re-order if-else of clock parsing.
>
>    if (!of_property_read_u32(node, "system-clock-frequency", &val)) {
>        // Since you are fixing rate already via "assigned-clocks" this
> may be a duplication. OR

exactly. and also need a device tree change (also for older kernels)
on my side.

This could be a last resort, yes. But I'd rather see a flag which
indicates whether the simple-audio-card should control the (first)
clock of the codec or not. Because until now, this wasn't the case.
And I don't know if this was an oversight or on purpose. Kuninori would
need to comment on that. And with the "we change mclk by default", we
break codecs with automatic sysclk generation.

>        // "assigned-clocks" can be parsed to understand if a fixed
> rate is expected.

Sounds like a hack to me. Esp. its doing the same as its already
doing right now. That is a "sysfreq = clk_get_rate(codec)".

>        simple_dai->sysclk = val;
>    } else {
>        // fetch MCLK clock from device and setup sysclk
>        // a. If "*mclk-fs" is given and "clocks" is found, the rate
> would be updated.
>        // b. If "*mclk-fs" is not mentioned and "clocks" is found,
> then simple-card utils won't touch rate. It will just do clock
> enable/disable.

mclk-fs is also a factor for the sysclk, thus it is also needed
if there is no mclk (or a fixed mclk).

I don't think you can deduce whether you can change the codecs
first clock with the current information :(

>    }
>
> > And its fetching just the first clock, doesn't it? What happens if a
> > codec has two clock inputs?
>
> Yes, it would have been more descriptive if it were specifically
> looking for clock "mclk". I think the original assumption was codec
> takes one input clock (MCLK) and uses it for sysclk.

Yeah, I've just noticed that the clk_get_rate() also only works
for the first clock of the codec.

-michael