diff mbox series

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

Message ID 20210108160501.7638-3-rf@opensource.cirrus.com
State New
Headers show
Series Add support for Rpi4b + Cirrus Lochnagar2 and CS47L15 | expand

Commit Message

Richard Fitzgerald Jan. 8, 2021, 4:04 p.m. UTC
The audio-graph-card driver has properties for configuring the clocking
for DAIs within a component, but is missing properties for setting
up the PLLs and sysclks of the component.

This patch adds the two new properties 'plls' and 'sysclks' so that the
audio-graph-driver can fully configure the component clocking.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 .../bindings/sound/audio-graph.yaml           | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Rob Herring (Arm) Jan. 13, 2021, 3:22 p.m. UTC | #1
On Fri, Jan 08, 2021 at 04:04:57PM +0000, Richard Fitzgerald wrote:
> The audio-graph-card driver has properties for configuring the clocking
> for DAIs within a component, but is missing properties for setting
> up the PLLs and sysclks of the component.
> 
> This patch adds the two new properties 'plls' and 'sysclks' so that the
> audio-graph-driver can fully configure the component clocking.

I'm not sure this makes sense to be generic, but if so, we already have 
the clock binding and should use (and possibly extend) that.

This appears to all be configuration of clocks within the codec, so 
these properties belong in the codec or cpu nodes.

> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  .../bindings/sound/audio-graph.yaml           | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph.yaml b/Documentation/devicetree/bindings/sound/audio-graph.yaml
> index 4b46794e5153..9e0819205a17 100644
> --- a/Documentation/devicetree/bindings/sound/audio-graph.yaml
> +++ b/Documentation/devicetree/bindings/sound/audio-graph.yaml
> @@ -39,6 +39,52 @@ properties:
>    mic-det-gpio:
>      maxItems: 1
>  
> +  plls:
> +    description: |
> +      A list of component pll settings. There are 4 cells per PLL setting:
> +        - phandle to the node of the codec or cpu component,
> +        - component PLL id,
> +        - component clock source id,
> +        - frequency (in Hz) of the PLL output clock.

assigned-clocks binding can set frequencies and parent clocks.

'pll' is too specific to the implementation. You may want to configure 
the freq and parent of something that's not a pll.

> +      The PLL id and clock source id are specific to the particular component
> +      so see the relevant component driver for the ids. Typically the
> +      clock source id indicates the pin the source clock is connected to.
> +      The same phandle can appear in multiple entries so that several plls
> +      can be set in the same component.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +  plls-clocks:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description: |
> +      A list of clock names giving the source clock for each setting
> +      in the plls property.
> +
> +  sysclks:
> +    description: |
> +      A list of component sysclk settings. There are 4 cells per sysclk
> +      setting:
> +        - phandle to the node of the codec or cpu component,
> +        - component sysclk id,
> +        - component clock source id,
> +        - direction of the clock: 0 if the clock is an input to the component,
> +          1 if it is an output.

A clock provider and consumer would provide the direction.

> +      The sysclk id and clock source id are specific to the particular
> +      component so see the relevant component driver for the ids. Typically
> +      the clock source id indicates the pin the source clock is connected to.
> +      The same phandle can appear in multiple entries so that several sysclks
> +      can be set in the same component.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +  sysclks-clocks:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description: |
> +      A list of clock names giving the source clock for each setting
> +      in the sysclks property.
> +
> +dependencies:
> +  plls: [ plls-clocks ]
> +  sysclks: [ sysclks-clocks ]
> +
>  required:
>    - dais
>  
> -- 
> 2.20.1
>
Mark Brown Jan. 13, 2021, 4:09 p.m. UTC | #2
On Wed, Jan 13, 2021 at 09:22:25AM -0600, Rob Herring wrote:

> I'm not sure this makes sense to be generic, but if so, we already have 
> the clock binding and should use (and possibly extend) that.

> This appears to all be configuration of clocks within the codec, so 
> these properties belong in the codec or cpu nodes.

Right, I think this should just be the clock binding. 

> > +      The PLL id and clock source id are specific to the particular component
> > +      so see the relevant component driver for the ids. Typically the

This should refer to the bindings for components, not to their drivers.

> > +      clock source id indicates the pin the source clock is connected to.
> > +      The same phandle can appear in multiple entries so that several plls
> > +      can be set in the same component.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> > +  plls-clocks:
> > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > +    description: |
> > +      A list of clock names giving the source clock for each setting
> > +      in the plls property.
> > +
> > +  sysclks:
> > +    description: |
> > +      A list of component sysclk settings. There are 4 cells per sysclk
> > +      setting:
> > +        - phandle to the node of the codec or cpu component,
> > +        - component sysclk id,
> > +        - component clock source id,
> > +        - direction of the clock: 0 if the clock is an input to the component,
> > +          1 if it is an output.
> 
> A clock provider and consumer would provide the direction.
> 
> > +      The sysclk id and clock source id are specific to the particular
> > +      component so see the relevant component driver for the ids. Typically
> > +      the clock source id indicates the pin the source clock is connected to.
> > +      The same phandle can appear in multiple entries so that several sysclks
> > +      can be set in the same component.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> > +  sysclks-clocks:
> > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > +    description: |
> > +      A list of clock names giving the source clock for each setting
> > +      in the sysclks property.
> > +
> > +dependencies:
> > +  plls: [ plls-clocks ]
> > +  sysclks: [ sysclks-clocks ]
> > +
> >  required:
> >    - dais
> >  
> > -- 
> > 2.20.1
> >
Richard Fitzgerald Jan. 14, 2021, 10:31 a.m. UTC | #3
On 13/01/2021 15:22, Rob Herring wrote:
> On Fri, Jan 08, 2021 at 04:04:57PM +0000, Richard Fitzgerald wrote:
>> The audio-graph-card driver has properties for configuring the clocking
>> for DAIs within a component, but is missing properties for setting
>> up the PLLs and sysclks of the component.
>>
>> This patch adds the two new properties 'plls' and 'sysclks' so that the
>> audio-graph-driver can fully configure the component clocking.
> 
> I'm not sure this makes sense to be generic, but if so, we already have
> the clock binding and should use (and possibly extend) that.
> 
> This appears to all be configuration of clocks within the codec, so
> these properties belong in the codec or cpu nodes.
>

audio-graph-card doesn't have codec or cpu nodes. Those were in
simple-card but are replaced in audio-graph-card by a simple phandle
array forming a graph.

I could assume that all clock settings apply to the codec and that there
is only ever one codec in an audio-graph-card configuration.

>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   .../bindings/sound/audio-graph.yaml           | 46 +++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/audio-graph.yaml b/Documentation/devicetree/bindings/sound/audio-graph.yaml
>> index 4b46794e5153..9e0819205a17 100644
>> --- a/Documentation/devicetree/bindings/sound/audio-graph.yaml
>> +++ b/Documentation/devicetree/bindings/sound/audio-graph.yaml
>> @@ -39,6 +39,52 @@ properties:
>>     mic-det-gpio:
>>       maxItems: 1
>>   
>> +  plls:
>> +    description: |
>> +      A list of component pll settings. There are 4 cells per PLL setting:
>> +        - phandle to the node of the codec or cpu component,
>> +        - component PLL id,
>> +        - component clock source id,
>> +        - frequency (in Hz) of the PLL output clock.
> 
> assigned-clocks binding can set frequencies and parent clocks.
> 
> 'pll' is too specific to the implementation. You may want to configure
> the freq and parent of something that's not a pll.
> 
>> +      The PLL id and clock source id are specific to the particular component
>> +      so see the relevant component driver for the ids. Typically the
>> +      clock source id indicates the pin the source clock is connected to.
>> +      The same phandle can appear in multiple entries so that several plls
>> +      can be set in the same component.
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +
>> +  plls-clocks:
>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +    description: |
>> +      A list of clock names giving the source clock for each setting
>> +      in the plls property.
>> +
>> +  sysclks:
>> +    description: |
>> +      A list of component sysclk settings. There are 4 cells per sysclk
>> +      setting:
>> +        - phandle to the node of the codec or cpu component,
>> +        - component sysclk id,
>> +        - component clock source id,
>> +        - direction of the clock: 0 if the clock is an input to the component,
>> +          1 if it is an output.
> 
> A clock provider and consumer would provide the direction.
> 
>> +      The sysclk id and clock source id are specific to the particular
>> +      component so see the relevant component driver for the ids. Typically
>> +      the clock source id indicates the pin the source clock is connected to.
>> +      The same phandle can appear in multiple entries so that several sysclks
>> +      can be set in the same component.
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +
>> +  sysclks-clocks:
>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +    description: |
>> +      A list of clock names giving the source clock for each setting
>> +      in the sysclks property.
>> +
>> +dependencies:
>> +  plls: [ plls-clocks ]
>> +  sysclks: [ sysclks-clocks ]
>> +
>>   required:
>>     - dais
>>   
>> -- 
>> 2.20.1
>>
Mark Brown Jan. 14, 2021, 11:14 a.m. UTC | #4
On Thu, Jan 14, 2021 at 10:31:10AM +0000, Richard Fitzgerald wrote:
> On 13/01/2021 15:22, Rob Herring wrote:

> > This appears to all be configuration of clocks within the codec, so
> > these properties belong in the codec or cpu nodes.

> audio-graph-card doesn't have codec or cpu nodes. Those were in
> simple-card but are replaced in audio-graph-card by a simple phandle
> array forming a graph.

> I could assume that all clock settings apply to the codec and that there
> is only ever one codec in an audio-graph-card configuration.

The suggestion here is to put properties in the node for the relevant
device rather than the card.
Richard Fitzgerald Jan. 14, 2021, 12:31 p.m. UTC | #5
On 14/01/2021 11:14, Mark Brown wrote:
> On Thu, Jan 14, 2021 at 10:31:10AM +0000, Richard Fitzgerald wrote:
>> On 13/01/2021 15:22, Rob Herring wrote:
> 
>>> This appears to all be configuration of clocks within the codec, so
>>> these properties belong in the codec or cpu nodes.
> 
>> audio-graph-card doesn't have codec or cpu nodes. Those were in
>> simple-card but are replaced in audio-graph-card by a simple phandle
>> array forming a graph.
> 
>> I could assume that all clock settings apply to the codec and that there
>> is only ever one codec in an audio-graph-card configuration.
> 
> The suggestion here is to put properties in the node for the relevant
> device rather than the card.
> 

That's seems bad to me - putting the properties for one driver in the
node of another driver. It's also potentially misleading as an example.
As in something like:

   wm5102: wm5102 {
	compatible = "wlf,wm5102";

	sysclks = <&some_clock>;
	plls = <&some_other_clock>;

	...
   };

To me that seems to imply that these are properties of the wm5102 driver
and it is going to setup its own sysclk and pll settings from those
properties. But actually it's the machine driver that does it, and these
properties are specific to audio-graph-card.
Richard Fitzgerald Jan. 15, 2021, 10:35 a.m. UTC | #6
On 13/01/2021 16:09, Mark Brown wrote:
> On Wed, Jan 13, 2021 at 09:22:25AM -0600, Rob Herring wrote:
> 
>> I'm not sure this makes sense to be generic, but if so, we already have
>> the clock binding and should use (and possibly extend) that.
> 
>> This appears to all be configuration of clocks within the codec, so
>> these properties belong in the codec or cpu nodes.
> 
> Right, I think this should just be the clock binding.
> 

Ok, so if the idea is to do this:

sound {
	clocks = <&audio_mclk>, <&pll>;
	clock-names = "mclk", "pll";
}

some_codec {
	pll: pll {
		compatible = "fixed-clock";
		clocks = <&audio_mclk>;
		clock-frequency = <98304000>;
	}
};

For this to work the clock binding must be a real clock object (so needs
a valid compatible=). But I need to somehow specify the PLL ID and
source pin for the PLL configuration. The schema for "fixed-clock" has
"additionalProperties: false" so I can't add extra custom properties to
the clock node.

Of course if we were able to use the clock framework to provide real
clock drivers for the plls and sysclks, the ID would be inherent in
the binding, and it can define a custom property for the source pin.

Some options:

1) Remove "additionalProperties: false" from the "fixed-clock" binding.

2) Add new core clock properties. Well, source-pin might legitimately be
meaningful, but for a real clock provider the clock ID is implicit.

3) Use 'reg' as fixed-clock doesn't use it. This works, but I suspect it
will be seen as an abuse of reg.

4) Put some extra properties in the sound node to define the <id,source>
pair for each clock. But that's clumsy to have some of the config in a
clock binding and a couple of extra elsewhere.

5) Use a bare clock binding that isn't a real clock provider, like:

sound {
	plls = <&pll>;
}

some_codec {
	pll: pll {
		reg = <1>; /* PLL ID */
	 	audio-graph-card,source-pin = <4>;
		clocks = <&audio_mclk>;
		clock-frequency = <98304000>;
		
	}
};


>>> +      The PLL id and clock source id are specific to the particular component
>>> +      so see the relevant component driver for the ids. Typically the
> 
> This should refer to the bindings for components, not to their drivers.
> 
>>> +      clock source id indicates the pin the source clock is connected to.
>>> +      The same phandle can appear in multiple entries so that several plls
>>> +      can be set in the same component.
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +
>>> +  plls-clocks:
>>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>>> +    description: |
>>> +      A list of clock names giving the source clock for each setting
>>> +      in the plls property.
>>> +
>>> +  sysclks:
>>> +    description: |
>>> +      A list of component sysclk settings. There are 4 cells per sysclk
>>> +      setting:
>>> +        - phandle to the node of the codec or cpu component,
>>> +        - component sysclk id,
>>> +        - component clock source id,
>>> +        - direction of the clock: 0 if the clock is an input to the component,
>>> +          1 if it is an output.
>>
>> A clock provider and consumer would provide the direction.
>>
>>> +      The sysclk id and clock source id are specific to the particular
>>> +      component so see the relevant component driver for the ids. Typically
>>> +      the clock source id indicates the pin the source clock is connected to.
>>> +      The same phandle can appear in multiple entries so that several sysclks
>>> +      can be set in the same component.
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +
>>> +  sysclks-clocks:
>>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>>> +    description: |
>>> +      A list of clock names giving the source clock for each setting
>>> +      in the sysclks property.
>>> +
>>> +dependencies:
>>> +  plls: [ plls-clocks ]
>>> +  sysclks: [ sysclks-clocks ]
>>> +
>>>   required:
>>>     - dais
>>>   
>>> -- 
>>> 2.20.1
>>>
Mark Brown Jan. 15, 2021, 1:11 p.m. UTC | #7
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?
Are you confusing the selection of rates on existing clocks with the use
of the assigned-* properties that the clock binding provides?

> For this to work the clock binding must be a real clock object (so needs
> a valid compatible=). But I need to somehow specify the PLL ID and

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.

> > > > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > > > +    description: |
> > > > +      A list of clock names giving the source clock for each setting
> > > > +      in the sysclks property.
> > > > +


Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
Richard Fitzgerald Jan. 15, 2021, 2:42 p.m. UTC | #8
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.

> 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.

>> For this to work the clock binding must be a real clock object (so needs
>> a valid compatible=). But I need to somehow specify the PLL ID and
> 
> 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:

	pll1 : pll1 {
		clock-frequency = <98304000>;
	};

which doesn't define a compatible= to match it to a clk driver. An
attempt to bulk_get the machine driver clocks here will fail.

To use a bare node with pick-n-mixed useful clock binding properties,
that doesn't represent a real clk provider driver, it would have to be
pointed to by a custom property that is not treated as a clk framework
object, e.g.:

	sound {
		audio-graph-card,plls = <&pll>;
	}

In this case pll is a node parsed by audio-graph-card that just happens
to use properties from the clock binding.

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.
Mark Brown Jan. 15, 2021, 3:20 p.m. UTC | #9
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?

> 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.
Richard Fitzgerald Jan. 15, 2021, 4:15 p.m. UTC | #10
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.
Mark Brown Jan. 15, 2021, 6:35 p.m. UTC | #11
On Fri, Jan 15, 2021 at 04:15:21PM +0000, Richard Fitzgerald wrote:

> 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:

OK, so if this is only supposed to represent a fixed clock on the board
separate to the CODEC then yes, of course you do need to instantiate a
driver for it like you do for every device on the board.  However it
shouldn't be a subdevice of the CODEC as you had it originally, it
should be a distinct device as the above has it since that is what
physically exists.  This obviously won't configure the FLL at all though
(which was what the binding you were proposing was for, the above is
definitely not a direct substitute for the binding you originally
proposed).

> > 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.

The concept of a clock framework is an implementation detail of Linux
which should not affect how the DT bindings for a device or system are
written, DT bindings should be clear and idiomatic as DT bindings.  The
goal is to represent the system in a clear and standardized fashion
which is useful to OSs in general, not just something convenient for
Linux as it happens to be implemented right now.  Current Linux
internals are not a constraint for DT bindings.

In this case if you can't figure out how to parse clock bindings without
moving the clocks over to standard Linux clock APIs (which seems likely)
then it follows that if you want to describe the clock configuration in
DT then the driver support for these clocks should use the standard
Linux clock framework.  This seems like a good idea in general anyway.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/audio-graph.yaml b/Documentation/devicetree/bindings/sound/audio-graph.yaml
index 4b46794e5153..9e0819205a17 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph.yaml
+++ b/Documentation/devicetree/bindings/sound/audio-graph.yaml
@@ -39,6 +39,52 @@  properties:
   mic-det-gpio:
     maxItems: 1
 
+  plls:
+    description: |
+      A list of component pll settings. There are 4 cells per PLL setting:
+        - phandle to the node of the codec or cpu component,
+        - component PLL id,
+        - component clock source id,
+        - frequency (in Hz) of the PLL output clock.
+      The PLL id and clock source id are specific to the particular component
+      so see the relevant component driver for the ids. Typically the
+      clock source id indicates the pin the source clock is connected to.
+      The same phandle can appear in multiple entries so that several plls
+      can be set in the same component.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  plls-clocks:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    description: |
+      A list of clock names giving the source clock for each setting
+      in the plls property.
+
+  sysclks:
+    description: |
+      A list of component sysclk settings. There are 4 cells per sysclk
+      setting:
+        - phandle to the node of the codec or cpu component,
+        - component sysclk id,
+        - component clock source id,
+        - direction of the clock: 0 if the clock is an input to the component,
+          1 if it is an output.
+      The sysclk id and clock source id are specific to the particular
+      component so see the relevant component driver for the ids. Typically
+      the clock source id indicates the pin the source clock is connected to.
+      The same phandle can appear in multiple entries so that several sysclks
+      can be set in the same component.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  sysclks-clocks:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    description: |
+      A list of clock names giving the source clock for each setting
+      in the sysclks property.
+
+dependencies:
+  plls: [ plls-clocks ]
+  sysclks: [ sysclks-clocks ]
+
 required:
   - dais