Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] dt-bindings: clk: Make example a bit clearer
@ 2019-08-23 10:03 Alexander Dahl
  2019-09-11 16:06 ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Dahl @ 2019-08-23 10:03 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd; +Cc: Uwe Kleine-König, kernel, linux-clk

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Previously the example used <&pll 2> in two places which made it harder
than necessary to understand why this clock gets the parent of
<&clkcon 0>. Also describe why <&pll 2> isn't reparented and <&clkcon 0>
gets no rate assigned.

Co-authored-by: Alexander Dahl <ada@thorsis.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Alexander Dahl <ada@thorsis.com>
---

Notes:
    v2:
        Add additional explaining text to following paragraph and strip mail
        headers from commit message.

 .../devicetree/bindings/clock/clock-bindings.txt     | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index b646bbcf7f92..1d4942380918 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -150,16 +150,18 @@ set to 0, or can be omitted if it is not followed by any non-zero entry.
         compatible = "fsl,imx-uart";
         reg = <0xa000 0x1000>;
         ...
-        clocks = <&osc 0>, <&pll 1>;
-        clock-names = "baud", "register";
+        clocks = ...
+        clock-names = ...
 
         assigned-clocks = <&clkcon 0>, <&pll 2>;
-        assigned-clock-parents = <&pll 2>;
+        assigned-clock-parents = <&pll 1>;
         assigned-clock-rates = <0>, <460800>;
     };
 
-In this example the <&pll 2> clock is set as parent of clock <&clkcon 0> and
-the <&pll 2> clock is assigned a frequency value of 460800 Hz.
+In this example the <&pll 1> clock is set as parent of clock <&clkcon 0> and
+the <&pll 2> clock is assigned a frequency value of 460800 Hz.  A parent
+setting for <&pll 2> is omitted (end of list) and rate setting for <&clkcon 0>
+is skipped because set to <0>.
 
 Configuring a clock's parent and rate through the device node that consumes
 the clock can be done only for clocks that have a single user. Specifying
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] dt-bindings: clk: Make example a bit clearer
  2019-08-23 10:03 [PATCH v2] dt-bindings: clk: Make example a bit clearer Alexander Dahl
@ 2019-09-11 16:06 ` Stephen Boyd
  2019-09-11 17:49   ` Uwe Kleine-König
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-09-11 16:06 UTC (permalink / raw)
  To: Alexander Dahl, Michael Turquette, Rob Herring
  Cc: u.kleine-koenig, kernel, linux-clk, devicetree

Quoting Alexander Dahl (2019-08-23 03:03:15)
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---

You need to Cc Rob and devicetree list on binding changes.

> 
> Notes:
>     v2:
>         Add additional explaining text to following paragraph and strip mail
>         headers from commit message.
> 
>  .../devicetree/bindings/clock/clock-bindings.txt     | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index b646bbcf7f92..1d4942380918 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -150,16 +150,18 @@ set to 0, or can be omitted if it is not followed by any non-zero entry.
>          compatible = "fsl,imx-uart";
>          reg = <0xa000 0x1000>;
>          ...
> -        clocks = <&osc 0>, <&pll 1>;
> -        clock-names = "baud", "register";
> +        clocks = ...
> +        clock-names = ...

I don't see the need for this change.

>  
>          assigned-clocks = <&clkcon 0>, <&pll 2>;
> -        assigned-clock-parents = <&pll 2>;
> +        assigned-clock-parents = <&pll 1>;
>          assigned-clock-rates = <0>, <460800>;
>      };
>  
> -In this example the <&pll 2> clock is set as parent of clock <&clkcon 0> and
> -the <&pll 2> clock is assigned a frequency value of 460800 Hz.
> +In this example the <&pll 1> clock is set as parent of clock <&clkcon 0> and
> +the <&pll 2> clock is assigned a frequency value of 460800 Hz.  A parent
> +setting for <&pll 2> is omitted (end of list) and rate setting for <&clkcon 0>
> +is skipped because set to <0>.

Maybe you can comment that the "clocks" and "clock-names" properties
don't matter for assigned clk rates and parents.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] dt-bindings: clk: Make example a bit clearer
  2019-09-11 16:06 ` Stephen Boyd
@ 2019-09-11 17:49   ` Uwe Kleine-König
  2019-09-16 20:13     ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2019-09-11 17:49 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Alexander Dahl, Michael Turquette, Rob Herring, devicetree,
	linux-clk, kernel

Hello,

On Wed, Sep 11, 2019 at 09:06:47AM -0700, Stephen Boyd wrote:
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index b646bbcf7f92..1d4942380918 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -150,16 +150,18 @@ set to 0, or can be omitted if it is not followed by any non-zero entry.
> >          compatible = "fsl,imx-uart";
> >          reg = <0xa000 0x1000>;
> >          ...
> > -        clocks = <&osc 0>, <&pll 1>;
> > -        clock-names = "baud", "register";
> > +        clocks = ...
> > +        clock-names = ...
> 
> I don't see the need for this change.

<&pll 1> is mentioned below. But it is not this instance that is
relevant. So I suggested to get rid of it to not be a source of
confusion. (But I guess you understood that.)

> >          assigned-clocks = <&clkcon 0>, <&pll 2>;
> > -        assigned-clock-parents = <&pll 2>;
> > +        assigned-clock-parents = <&pll 1>;
> >          assigned-clock-rates = <0>, <460800>;
> >      };
> >  
> > -In this example the <&pll 2> clock is set as parent of clock <&clkcon 0> and
> > -the <&pll 2> clock is assigned a frequency value of 460800 Hz.
> > +In this example the <&pll 1> clock is set as parent of clock <&clkcon 0> and
> > +the <&pll 2> clock is assigned a frequency value of 460800 Hz.  A parent
> > +setting for <&pll 2> is omitted (end of list) and rate setting for <&clkcon 0>
> > +is skipped because set to <0>.
> 
> Maybe you can comment that the "clocks" and "clock-names" properties
> don't matter for assigned clk rates and parents.

Sure, a long text can explain this, but a maximal simple example is
very beneficial, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] dt-bindings: clk: Make example a bit clearer
  2019-09-11 17:49   ` Uwe Kleine-König
@ 2019-09-16 20:13     ` Stephen Boyd
  2019-10-10 14:16       ` Alexander Dahl
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-09-16 20:13 UTC (permalink / raw)
  To: u.kleine-koenig
  Cc: Alexander Dahl, Michael Turquette, Rob Herring, devicetree,
	linux-clk, kernel

(Sorry my MUA fails at utf8 encoding emails)

> On Wed, Sep 11, 2019 at 09:06:47AM -0700, Stephen Boyd wrote:
> > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > index b646bbcf7f92..1d4942380918 100644
> > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > @@ -150,16 +150,18 @@ set to 0, or can be omitted if it is not followed by any non-zero entry.
> > >          compatible = "fsl,imx-uart";
> > >          reg = <0xa000 0x1000>;
> > >          ...
> > > -        clocks = <&osc 0>, <&pll 1>;
> > > -        clock-names = "baud", "register";
> > > +        clocks = ...
> > > +        clock-names = ...
> > 
> > I don't see the need for this change.
> 
> <&pll 1> is mentioned below. But it is not this instance that is
> relevant. So I suggested to get rid of it to not be a source of
> confusion. (But I guess you understood that.)

Yes.

> 
> > >          assigned-clocks = <&clkcon 0>, <&pll 2>;
> > > -        assigned-clock-parents = <&pll 2>;
> > > +        assigned-clock-parents = <&pll 1>;
> > >          assigned-clock-rates = <0>, <460800>;
> > >      };
> > >  
> > > -In this example the <&pll 2> clock is set as parent of clock <&clkcon 0> and
> > > -the <&pll 2> clock is assigned a frequency value of 460800 Hz.
> > > +In this example the <&pll 1> clock is set as parent of clock <&clkcon 0> and
> > > +the <&pll 2> clock is assigned a frequency value of 460800 Hz.  A parent
> > > +setting for <&pll 2> is omitted (end of list) and rate setting for <&clkcon 0>
> > > +is skipped because set to <0>.
> > 
> > Maybe you can comment that the "clocks" and "clock-names" properties
> > don't matter for assigned clk rates and parents.
> 
> Sure, a long text can explain this, but a maximal simple example is
> very beneficial, too.
> 

The paragraph at the start of "Assigned clock parents and rates" already
explains how these properties work. This patch isn't going to help if
the reader hasn't read that paragraph, so reordering things isn't
providing more clarity. If anything, I would clarify the description of
the example by indicating what parts of the example we're talking about
when explaining, i.e.

	In this example the <&pll 2> clock is set as parent of the
	<&clkcon 0> clock because the first clock specifier in the
	assigned-clocks property is <&clkcon 0> and that matches the
	first clock specifier in the assigned-clock-parents property.
	Similarly the <&pll 2> clock is assigned a frequency value of
	460800 Hz because the second frequency in the
	assigned-clock-rates property is 460800 Hz and that matches the
	second clock specifier in the assigned-clocks property <&pll 2>.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] dt-bindings: clk: Make example a bit clearer
  2019-09-16 20:13     ` Stephen Boyd
@ 2019-10-10 14:16       ` Alexander Dahl
  2019-11-08 22:18         ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Dahl @ 2019-10-10 14:16 UTC (permalink / raw)
  To: linux-clk
  Cc: Stephen Boyd, u.kleine-koenig, Michael Turquette, Rob Herring,
	devicetree, kernel

Hello Stephen,

I took some minutes to look at this again, because I was the one who had 
difficulties understanding the documentation here in the first place.

Note: this patch/discussion is not about changing the dt-binding, but about 
improving the documentation on the properties 'assigned-clocks', 'assigned-
clock-parents', and 'assigned-clock-rates'. See below:

Am Montag, 16. September 2019, 13:13:16 CEST schrieb Stephen Boyd:
> (Sorry my MUA fails at utf8 encoding emails)
> 
> > On Wed, Sep 11, 2019 at 09:06:47AM -0700, Stephen Boyd wrote:
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > b/Documentation/devicetree/bindings/clock/clock-bindings.txt index
> > > > b646bbcf7f92..1d4942380918 100644
> > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > @@ -150,16 +150,18 @@ set to 0, or can be omitted if it is not
> > > > followed by any non-zero entry.> > > 
> > > >          compatible = "fsl,imx-uart";
> > > >          reg = <0xa000 0x1000>;
> > > >          ...
> > > > 
> > > > -        clocks = <&osc 0>, <&pll 1>;
> > > > -        clock-names = "baud", "register";
> > > > +        clocks = ...
> > > > +        clock-names = ...
> > > 
> > > I don't see the need for this change.
> > 
> > <&pll 1> is mentioned below. But it is not this instance that is
> > relevant. So I suggested to get rid of it to not be a source of
> > confusion. (But I guess you understood that.)
> 
> Yes.

This was indeed one source of confusion for me. This obfuscation of Uwe would 
have helped me, because it hides distracting information, which is irrelevant 
for explaining assigned clock parents and might lead to false conclusions on 
how those things work.

> > > >          assigned-clocks = <&clkcon 0>, <&pll 2>;
> > > > 
> > > > -        assigned-clock-parents = <&pll 2>;
> > > > +        assigned-clock-parents = <&pll 1>;
> > > > 
> > > >          assigned-clock-rates = <0>, <460800>;
> > > >      
> > > >      };
> > > > 
> > > > -In this example the <&pll 2> clock is set as parent of clock <&clkcon
> > > > 0> and -the <&pll 2> clock is assigned a frequency value of 460800
> > > > Hz.
> > > > +In this example the <&pll 1> clock is set as parent of clock <&clkcon
> > > > 0> and +the <&pll 2> clock is assigned a frequency value of 460800
> > > > Hz.  A parent +setting for <&pll 2> is omitted (end of list) and rate
> > > > setting for <&clkcon 0> +is skipped because set to <0>.
> > > 
> > > Maybe you can comment that the "clocks" and "clock-names" properties
> > > don't matter for assigned clk rates and parents.
> > 
> > Sure, a long text can explain this, but a maximal simple example is
> > very beneficial, too.
> 
> The paragraph at the start of "Assigned clock parents and rates" already
> explains how these properties work. This patch isn't going to help if
> the reader hasn't read that paragraph, so reordering things isn't
> providing more clarity. 

Reordering might not help. What helps on understanding things is describing 
the same topic in a different way again. This is somewhat redundant, but 
someone who does not fully understand explanation A might understand 
explanation B better.

> If anything, I would clarify the description of
> the example by indicating what parts of the example we're talking about
> when explaining, i.e.
> 
> 	In this example the <&pll 2> clock is set as parent of the
> 	<&clkcon 0> clock because the first clock specifier in the
> 	assigned-clocks property is <&clkcon 0> and that matches the
> 	first clock specifier in the assigned-clock-parents property.
> 	Similarly the <&pll 2> clock is assigned a frequency value of
> 	460800 Hz because the second frequency in the
> 	assigned-clock-rates property is 460800 Hz and that matches the
> 	second clock specifier in the assigned-clocks property <&pll 2>.

I could go with that and send a v3 with the obfuscation from above (I'd like 
to keep that) and this paragraph of yours?

Greets
Alex


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] dt-bindings: clk: Make example a bit clearer
  2019-10-10 14:16       ` Alexander Dahl
@ 2019-11-08 22:18         ` Stephen Boyd
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-11-08 22:18 UTC (permalink / raw)
  To: Alexander Dahl, linux-clk
  Cc: u.kleine-koenig, Michael Turquette, Rob Herring, devicetree, kernel

Quoting Alexander Dahl (2019-10-10 07:16:00)
> 
> > If anything, I would clarify the description of
> > the example by indicating what parts of the example we're talking about
> > when explaining, i.e.
> > 
> >       In this example the <&pll 2> clock is set as parent of the
> >       <&clkcon 0> clock because the first clock specifier in the
> >       assigned-clocks property is <&clkcon 0> and that matches the
> >       first clock specifier in the assigned-clock-parents property.
> >       Similarly the <&pll 2> clock is assigned a frequency value of
> >       460800 Hz because the second frequency in the
> >       assigned-clock-rates property is 460800 Hz and that matches the
> >       second clock specifier in the assigned-clocks property <&pll 2>.
> 
> I could go with that and send a v3 with the obfuscation from above (I'd like 
> to keep that) and this paragraph of yours?
> 

Sure. Please resend.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 10:03 [PATCH v2] dt-bindings: clk: Make example a bit clearer Alexander Dahl
2019-09-11 16:06 ` Stephen Boyd
2019-09-11 17:49   ` Uwe Kleine-König
2019-09-16 20:13     ` Stephen Boyd
2019-10-10 14:16       ` Alexander Dahl
2019-11-08 22:18         ` Stephen Boyd

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git