[PATCH] clk: add flags to distinguish xtal clocks

[PATCH] clk: add flags to distinguish xtal clocks

[Felipe Balbi]

From: Luciano Coelho <luca@coelho.fi>

Add a flag that indicate whether the clock is a crystal or not.

Additionally, parse a new device tree binding in clk-fixed-rate to set
this flag.

If clock-xtal isn't set, the clock framework will assume clock to be
generated by an oscillator.  There's only one user for this binding
right now which is Texas Instruments' WiLink devices which need to know
details about the clock in order to initialize the underlying WiFi HW
correctly.

Signed-off-by: Luciano Coelho <luca@coelho.fi>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---

Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag
isn't there, default behavior will be taken.

 Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 +
 drivers/clk/clk-fixed-rate.c                            | 6 +++++-
 include/linux/clk-provider.h                            | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
index 0b1fe78..3036dfe 100644
--- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
+++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
@@ -12,6 +12,7 @@ Required properties:
 Optional properties:
 - gpios : From common gpio binding; gpio connection to clock enable pin.
 - clock-output-names : From common clock binding.
+- clock-xtal: true when a clock is provided by a crystal
 
 Example:
 	clock {
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 1ed591a..5db9bf0 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node)
 	struct clk *clk;
 	const char *clk_name = node->name;
 	u32 rate;
+	unsigned long flags = CLK_IS_ROOT;
 
 	if (of_property_read_u32(node, "clock-frequency", &rate))
 		return;
 
+	if (of_property_read_bool(node, "clock-xtal"))
+		flags |= CLK_IS_TYPE_XTAL;
+
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-	clk = clk_register_fixed_rate(NULL, clk_name, NULL, CLK_IS_ROOT, rate);
+	clk = clk_register_fixed_rate(NULL, clk_name, NULL, flags, rate);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
 }
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 73bdb69..30c0c37 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -29,6 +29,7 @@
 #define CLK_IS_BASIC		BIT(5) /* Basic clk, can't do a to_clk_foo() */
 #define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
 #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
+#define CLK_IS_TYPE_XTAL	BIT(8) /* this is a crystal clock */
 
 struct clk_hw;
 
-- 
1.8.4.1.559.gdb9bdfb

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Luca Coelho]

Hi Felipe,

On Fri, 2013-11-08 at 12:00 -0600, Felipe Balbi wrote:
> From: Luciano Coelho <luca@coelho.fi>
> 
> Add a flag that indicate whether the clock is a crystal or not.
> 
> Additionally, parse a new device tree binding in clk-fixed-rate to set
> this flag.
> 
> If clock-xtal isn't set, the clock framework will assume clock to be
> generated by an oscillator.  There's only one user for this binding
> right now which is Texas Instruments' WiLink devices which need to know
> details about the clock in order to initialize the underlying WiFi HW
> correctly.
> 
> Signed-off-by: Luciano Coelho <luca@coelho.fi>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag
> isn't there, default behavior will be taken.

I don't think this is a good idea, because how would code that relies on
the CLK_IS_TYPE_XTAL flag know whether 0 means that the clock is *not*
crystal or if the flag is simply not defined?

There are many clocks which *are* crystal but don't define this flag
(obviously, because it didn't exist).  That's the reason why I added the
CLK_IS_TYPE_DEFINED flag...

If you want to get rid of this flag, you should traverse all clocks and
set the CLK_IS_TYPE_XTAL on all those that are crystals.

--
Cheers,
Luca.

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

Hi Felipe,

On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote:
> From: Luciano Coelho <luca@coelho.fi>
> 
> Add a flag that indicate whether the clock is a crystal or not.
> 
> Additionally, parse a new device tree binding in clk-fixed-rate to set
> this flag.
> 
> If clock-xtal isn't set, the clock framework will assume clock to be
> generated by an oscillator.  There's only one user for this binding
> right now which is Texas Instruments' WiLink devices which need to know
> details about the clock in order to initialize the underlying WiFi HW
> correctly.
> 
> Signed-off-by: Luciano Coelho <luca@coelho.fi>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag
> isn't there, default behavior will be taken.
> 
>  Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 +
>  drivers/clk/clk-fixed-rate.c                            | 6 +++++-
>  include/linux/clk-provider.h                            | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> index 0b1fe78..3036dfe 100644
> --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> @@ -12,6 +12,7 @@ Required properties:
>  Optional properties:
>  - gpios : From common gpio binding; gpio connection to clock enable pin.
>  - clock-output-names : From common clock binding.
> +- clock-xtal: true when a clock is provided by a crystal
>  
>  Example:
>  	clock {
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index 1ed591a..5db9bf0 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node)
>  	struct clk *clk;
>  	const char *clk_name = node->name;
>  	u32 rate;
> +	unsigned long flags = CLK_IS_ROOT;
>  
>  	if (of_property_read_u32(node, "clock-frequency", &rate))
>  		return;
>  
> +	if (of_property_read_bool(node, "clock-xtal"))
> +		flags |= CLK_IS_TYPE_XTAL;
> +

Introducing a new compatible instead of a property would make more
sense here I think.

Do you have a reason not to do so?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2824 bytes --]

Hi,

On Sun, Nov 10, 2013 at 12:37:16PM +0100, Maxime Ripard wrote:
> Hi Felipe,
> 
> On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote:
> > From: Luciano Coelho <luca@coelho.fi>
> > 
> > Add a flag that indicate whether the clock is a crystal or not.
> > 
> > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > this flag.
> > 
> > If clock-xtal isn't set, the clock framework will assume clock to be
> > generated by an oscillator.  There's only one user for this binding
> > right now which is Texas Instruments' WiLink devices which need to know
> > details about the clock in order to initialize the underlying WiFi HW
> > correctly.
> > 
> > Signed-off-by: Luciano Coelho <luca@coelho.fi>
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag
> > isn't there, default behavior will be taken.
> > 
> >  Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 +
> >  drivers/clk/clk-fixed-rate.c                            | 6 +++++-
> >  include/linux/clk-provider.h                            | 1 +
> >  3 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > index 0b1fe78..3036dfe 100644
> > --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > @@ -12,6 +12,7 @@ Required properties:
> >  Optional properties:
> >  - gpios : From common gpio binding; gpio connection to clock enable pin.
> >  - clock-output-names : From common clock binding.
> > +- clock-xtal: true when a clock is provided by a crystal
> >  
> >  Example:
> >  	clock {
> > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> > index 1ed591a..5db9bf0 100644
> > --- a/drivers/clk/clk-fixed-rate.c
> > +++ b/drivers/clk/clk-fixed-rate.c
> > @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node)
> >  	struct clk *clk;
> >  	const char *clk_name = node->name;
> >  	u32 rate;
> > +	unsigned long flags = CLK_IS_ROOT;
> >  
> >  	if (of_property_read_u32(node, "clock-frequency", &rate))
> >  		return;
> >  
> > +	if (of_property_read_bool(node, "clock-xtal"))
> > +		flags |= CLK_IS_TYPE_XTAL;
> > +
> 
> Introducing a new compatible instead of a property would make more
> sense here I think.
> 
> Do you have a reason not to do so?

As you can see, this is original work from Luca but I disagree that
adding a new compatible makes more sense. This still related to a fixed
rate clock, we're just giving it one extra metadata which will
differentiate between crystal and oscilator fixed rate clocks.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Luca Coelho]

On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote:
> Hi,
> 
> On Sun, Nov 10, 2013 at 12:37:16PM +0100, Maxime Ripard wrote:
> > Hi Felipe,
> > 
> > On Fri, Nov 08, 2013 at 12:00:48PM -0600, Felipe Balbi wrote:
> > > From: Luciano Coelho <luca@coelho.fi>
> > > 
> > > Add a flag that indicate whether the clock is a crystal or not.
> > > 
> > > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > > this flag.
> > > 
> > > If clock-xtal isn't set, the clock framework will assume clock to be
> > > generated by an oscillator.  There's only one user for this binding
> > > right now which is Texas Instruments' WiLink devices which need to know
> > > details about the clock in order to initialize the underlying WiFi HW
> > > correctly.
> > > 
> > > Signed-off-by: Luciano Coelho <luca@coelho.fi>
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > > 
> > > Dropped CLK_IS_TYPE_DEFINED flag and just assume that if the flag
> > > isn't there, default behavior will be taken.
> > > 
> > >  Documentation/devicetree/bindings/clock/fixed-clock.txt | 1 +
> > >  drivers/clk/clk-fixed-rate.c                            | 6 +++++-
> > >  include/linux/clk-provider.h                            | 1 +
> > >  3 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/fixed-clock.txt b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > > index 0b1fe78..3036dfe 100644
> > > --- a/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > > +++ b/Documentation/devicetree/bindings/clock/fixed-clock.txt
> > > @@ -12,6 +12,7 @@ Required properties:
> > >  Optional properties:
> > >  - gpios : From common gpio binding; gpio connection to clock enable pin.
> > >  - clock-output-names : From common clock binding.
> > > +- clock-xtal: true when a clock is provided by a crystal
> > >  
> > >  Example:
> > >  	clock {
> > > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> > > index 1ed591a..5db9bf0 100644
> > > --- a/drivers/clk/clk-fixed-rate.c
> > > +++ b/drivers/clk/clk-fixed-rate.c
> > > @@ -91,13 +91,17 @@ void of_fixed_clk_setup(struct device_node *node)
> > >  	struct clk *clk;
> > >  	const char *clk_name = node->name;
> > >  	u32 rate;
> > > +	unsigned long flags = CLK_IS_ROOT;
> > >  
> > >  	if (of_property_read_u32(node, "clock-frequency", &rate))
> > >  		return;
> > >  
> > > +	if (of_property_read_bool(node, "clock-xtal"))
> > > +		flags |= CLK_IS_TYPE_XTAL;
> > > +
> > 
> > Introducing a new compatible instead of a property would make more
> > sense here I think.
> > 
> > Do you have a reason not to do so?
> 
> As you can see, this is original work from Luca but I disagree that
> adding a new compatible makes more sense. This still related to a fixed
> rate clock, we're just giving it one extra metadata which willAnd t
> differentiate between crystal and oscilator fixed rate clocks.

I agree with Felipe.  This was discussed before [1].  While still at TI,
I tried to figure out the exact need for the firmware to know whether it
was an oscillator or not.  It was mostly because the stabilization time
and such things differ with oscillators, but I wasn't able to find out
how exactly this affected things.

In any case, as I concluded earlier (but it's not really my call), being
a crystal or an oscillator *is* a characteristic of the hardware,
regardless of whether that information is useful or not.  In the WiLink
case it is, at least it can differentiate the clocks that are used in
the HW modules it uses.

So IMHO it doesn't really hurt and it's not really against the DT
principles.

[1] https://lkml.org/lkml/2013/7/29/321

--
Cheers,
Luca.

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]

Hi Luca,

On Mon, Nov 11, 2013 at 09:50:56PM +0200, Luca Coelho wrote:
> On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote:
> > > > +	if (of_property_read_bool(node, "clock-xtal"))
> > > > +		flags |= CLK_IS_TYPE_XTAL;
> > > > +
> > > 
> > > Introducing a new compatible instead of a property would make more
> > > sense here I think.
> > > 
> > > Do you have a reason not to do so?
> > 
> > As you can see, this is original work from Luca but I disagree that
> > adding a new compatible makes more sense. This still related to a fixed
> > rate clock, we're just giving it one extra metadata which willAnd t
> > differentiate between crystal and oscilator fixed rate clocks.
> 
> I agree with Felipe.  This was discussed before [1].  While still at TI,
> I tried to figure out the exact need for the firmware to know whether it
> was an oscillator or not.  It was mostly because the stabilization time
> and such things differ with oscillators, but I wasn't able to find out
> how exactly this affected things.
> 
> In any case, as I concluded earlier (but it's not really my call), being
> a crystal or an oscillator *is* a characteristic of the hardware,
> regardless of whether that information is useful or not.  In the WiLink
> case it is, at least it can differentiate the clocks that are used in
> the HW modules it uses.
> 
> So IMHO it doesn't really hurt and it's not really against the DT
> principles.

Just to be clear, I'm not against your patch. If you need this to make
your driver work, then it's fine for me. Mike will probably know
better if we actually need some extra metadata.

What I'm not really convinced about is *how* you carry that metadata
in the DT, that's all, nothing more.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Luca Coelho]

On Mon, 2013-11-11 at 21:59 +0100, Maxime Ripard wrote:
> Hi Luca,
> 
> On Mon, Nov 11, 2013 at 09:50:56PM +0200, Luca Coelho wrote:
> > On Mon, 2013-11-11 at 13:42 -0600, Felipe Balbi wrote:
> > > > > +	if (of_property_read_bool(node, "clock-xtal"))
> > > > > +		flags |= CLK_IS_TYPE_XTAL;
> > > > > +
> > > > 
> > > > Introducing a new compatible instead of a property would make more
> > > > sense here I think.
> > > > 
> > > > Do you have a reason not to do so?
> > > 
> > > As you can see, this is original work from Luca but I disagree that
> > > adding a new compatible makes more sense. This still related to a fixed
> > > rate clock, we're just giving it one extra metadata which willAnd t
> > > differentiate between crystal and oscilator fixed rate clocks.
> > 
> > I agree with Felipe.  This was discussed before [1].  While still at TI,
> > I tried to figure out the exact need for the firmware to know whether it
> > was an oscillator or not.  It was mostly because the stabilization time
> > and such things differ with oscillators, but I wasn't able to find out
> > how exactly this affected things.
> > 
> > In any case, as I concluded earlier (but it's not really my call), being
> > a crystal or an oscillator *is* a characteristic of the hardware,
> > regardless of whether that information is useful or not.  In the WiLink
> > case it is, at least it can differentiate the clocks that are used in
> > the HW modules it uses.
> > 
> > So IMHO it doesn't really hurt and it's not really against the DT
> > principles.
> 
> Just to be clear, I'm not against your patch. If you need this to make
> your driver work, then it's fine for me. Mike will probably know
> better if we actually need some extra metadata.

:)

I understand, we should really try to make this as clean as possible, DT
should really be a good description of the hardware.


> What I'm not really convinced about is *how* you carry that metadata
> in the DT, that's all, nothing more.

Okay, I get you.  My point is that being a crystal or not *is* a
characteristic of the clock, so I think it could be part of the flags
that describe it.

In any case, it's not really my call.  This is about the clock and it's
not even my home turf (wireless). ;)

Thanks for your comments.  And I'm sorry if the tone of my previous
email sounded harsh, it was not supposed to. :)

--
Cheers,
Luca.

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On Tue, Nov 12, 2013 at 10:05:49AM +0200, Luca Coelho wrote:
> > What I'm not really convinced about is *how* you carry that metadata
> > in the DT, that's all, nothing more.
> 
> Okay, I get you.  My point is that being a crystal or not *is* a
> characteristic of the clock, so I think it could be part of the flags
> that describe it.

Yeah, but then, a crystal has a slightly different behaviour and
property than a oscillator, therefore the two aren't fully
"compatible" with each other.

> In any case, it's not really my call.  This is about the clock and it's
> not even my home turf (wireless). ;)
> 
> Thanks for your comments.  And I'm sorry if the tone of my previous
> email sounded harsh, it was not supposed to. :)

That's fine, don't worry :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On Mon, Nov 11, 2013 at 01:42:47PM -0600, Felipe Balbi wrote:
> > > +	if (of_property_read_bool(node, "clock-xtal"))
> > > +		flags |= CLK_IS_TYPE_XTAL;
> > > +
> > 
> > Introducing a new compatible instead of a property would make more
> > sense here I think.
> > 
> > Do you have a reason not to do so?
> 
> As you can see, this is original work from Luca but I disagree that
> adding a new compatible makes more sense. This still related to a fixed
> rate clock, we're just giving it one extra metadata which will
> differentiate between crystal and oscilator fixed rate clocks.

I don't know, I think it's more a matter of consistency.

If we turn the problem the other way around. Let's say we have a
crystal that for some reason can't be used with clk-fixed-rate. You'd
add a new driver for it, with a compatible of its own, and you'd put
that XTAL flag in there, without any extra metadata in the DT, right?

And I'm pretty sure having a compatible like "clk-xtal" would make it
pretty obvious that it's still a fixed rate clock.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Stephen Warren]

On 11/08/2013 11:00 AM, Felipe Balbi wrote:
> From: Luciano Coelho <luca@coelho.fi>
> 
> Add a flag that indicate whether the clock is a crystal or not.
> 
> Additionally, parse a new device tree binding in clk-fixed-rate to set
> this flag.
> 
> If clock-xtal isn't set, the clock framework will assume clock to be
> generated by an oscillator.  There's only one user for this binding
> right now which is Texas Instruments' WiLink devices which need to know
> details about the clock in order to initialize the underlying WiFi HW
> correctly.

Why on earth does it care? Surely the WiFi HW doesn't care about
crystal-vs-non-crystal, but rather some facet of the clock signal that
the type of source implies. Shouldn't the DT property describe that
facet of the signal, rather than the reason why it has that facet?

Re: [PATCH] clk: add flags to distinguish xtal clocks

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

Hi,

On Mon, Nov 11, 2013 at 09:27:58AM -0700, Stephen Warren wrote:
> On 11/08/2013 11:00 AM, Felipe Balbi wrote:
> > From: Luciano Coelho <luca@coelho.fi>
> > 
> > Add a flag that indicate whether the clock is a crystal or not.
> > 
> > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > this flag.
> > 
> > If clock-xtal isn't set, the clock framework will assume clock to be
> > generated by an oscillator.  There's only one user for this binding
> > right now which is Texas Instruments' WiLink devices which need to know
> > details about the clock in order to initialize the underlying WiFi HW
> > correctly.
> 
> Why on earth does it care? Surely the WiFi HW doesn't care about
> crystal-vs-non-crystal, but rather some facet of the clock signal that
> the type of source implies. Shouldn't the DT property describe that
> facet of the signal, rather than the reason why it has that facet?

well, if you can figure out what that facet is, then _do_ tell. Luca has
tried for months to get that information with no success.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]