Re: [PATCH v4 2/6] dt-bindings: audio-graph-card: Add plls and sysclks properties

From: Richard Fitzgerald <rf@opensource.cirrus.com>
To: Mark Brown <broonie@kernel.org>
Cc: Rob Herring <robh@kernel.org>, <kuninori.morimoto.gx@renesas.com>,
	<nsaenzjulienne@suse.de>, <f.fainelli@gmail.com>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
	<bcm-kernel-feedback-list@broadcom.com>,
	<linux-rpi-kernel@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 2/6] dt-bindings: audio-graph-card: Add plls and sysclks properties
Date: Fri, 15 Jan 2021 16:15:21 +0000	[thread overview]
Message-ID: <d67f805f-2813-14e9-0c4f-5948ec73f7b0@opensource.cirrus.com> (raw)
In-Reply-To: <20210115152004.GD4384@sirena.org.uk>

On 15/01/2021 15:20, Mark Brown wrote:
> On Fri, Jan 15, 2021 at 02:42:12PM +0000, Richard Fitzgerald wrote:
>> On 15/01/2021 13:11, Mark Brown wrote:
>>> On Fri, Jan 15, 2021 at 10:35:23AM +0000, Richard Fitzgerald wrote:
>>>> On 13/01/2021 16:09, Mark Brown wrote:
>>>>> On Wed, Jan 13, 2021 at 09:22:25AM -0600, Rob Herring wrote:
> 
>>>> some_codec {
>>>> 	pll: pll {
>>>> 		compatible = "fixed-clock";
>>>> 		clocks = <&audio_mclk>;
>>>> 		clock-frequency = <98304000>;
>>>> 	}
> 
>>> A PLL is not a fixed clock, why would you define a fixed clock here?
> 
>> It's a fixed clock if you are only setting one configuration. Call it
>> compatible="any-other-dummy-clock-type" if you like, it doesn't matter
>> what it is for the purposes of what I was describing.
> 
>> This isn't a clk driver for a pll, it's just a setting to be passed to
>> snd_soc_component_set_pll() using a clock binding to specify it.
> 
> So you're trying to describe a crystal on the board?  Why would this be
> a subnode of the CODEC then?  Surely it's just a standard fixed clock
> which provides some input to the CODEC in the same way you'd describe
> any other input to the CODEC.  The above doesn't look anything like the
> hardware.  But if that's what you're doing how is that related to
> configuring the FLL except possibly as the input clock you'd reference?
> 
>>> Are you confusing the selection of rates on existing clocks with the use
>>> of the assigned-* properties that the clock binding provides?
> 
>> I'm not at all sure what you and Rob have in mind here. Perhaps you
>> could give an example of what you are thinking the .dts would look like
>> to define some pll/sysclk settings for audio-graph-card to apply. An
>> example is worth a thousand emails.
> 
> As far as I can tell you are trying to configure the FLL in the CODEC,
> telling it to take an input clock and produce a fixed output clock rate
> from that.  The FLL is a fairly basic clock, there are examples for both
> that and choosing a configuration for a clock in the clock bindings.
> 
>>> That seems like a *very* surprising requirement - why would the clock
>>> binding have that requirement?  It would seem to create issues for a
>>> single device providing multiple clocks which should be a pretty common
>>> coase.
> 
>> You misunderstand me. What I'm saying is that to do this:
> 
>> 	sound {
>> 		clocks = <&pll>;
>> 	}
> 
>> The node 'pll' must correspond to a clock provider driver. It can't be
>> just a bare node with some properties pick-n-mixed from the clock
>> binding, like this:
> 
> I'm pretty sure I understand you perfectly; again, what makes you say
> that a description of a clock in the device tree has any requirement
> for a separate compatible string?
> 

If I do:
  	sound {
  		clocks = <&clock>;
  	};

	clock: clock {
		compatible = "fixed-clock";
		clock-frequency = <98304000>;
	};

I can clk_bulk_get_all().
But if I remove the 'compatible' from the clock node, clk_bulk_get_all()
will return -EPROBE_DEFER and log:

  /sound: Failed to get clk index: 0 ret: -517

from the error case in _clk_bulk_get() in clk/clk-bulk.c.

>> So the question I'm trying to ask is: when you and Rob said use
>> the clock binding, did you mean pointing to that binding from
>> clocks=<...>, or from a custom property like my audio-graph-card,plls
>> example above.
> 
> When we say to use the clock binding what we are saying is to use the
> actual clock bindings to describe the clocks, not make a custom binding
> that looks kind of like them - making a custom binding doesn't address
> the problem.
> 

But I don't know what you mean by "use the actual clock bindings to
describe the clocks".

What is not clear to me is how you want me to use a clock binding to
describe something that isn't a clk-framework clk. If you know what you
want, then please.. an example would help explain.