[RFC] clk: add flags to distinguish xtal clocks

[RFC] clk: add flags to distinguish xtal clocks

[Luciano Coelho]

Add a flag that indicate whether the clock is a crystal or not.  Since
no clocks set this flag right now, include an additional flag that
indicates whether the type is set or not.  If the CLK_IS_TYPE_DEFINED
flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined.
This ensures backwards compatibility.

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

Signed-off-by: Luciano Coelho <coelho@ti.com>
---

I'm not  familiar with the common clock framework and I'm not
entirely sure the flags can be used in such a way, but to me it looks
reasonable, since some clock consumers may need to know what type of
clock is being provided.

Specifically, the wl12xx firmware needs to know if the clock is XTAL
or not to handle the stabilization and boosts properly.

My main idea is that I need to pass this information in the device
tree definition of the clocks, so that the driver can pass this
information on to the firmware.

Please let me know if this looks ok or not.  If not, please let me
know if you have any other ideas on how to solve my problem (of
knowing whether the clock attached to the WiLink chip is XTAL or not).




 drivers/clk/clk-fixed-rate.c |    6 +++++-
 include/linux/clk-provider.h |    2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index dc58fbd..4003a82 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -90,13 +90,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_DEFINED | 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 1186098..034320b 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -27,6 +27,8 @@
 #define CLK_IS_ROOT		BIT(4) /* root clk, has no parent */
 #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_IS_TYPE_DEFINED	BIT(7) /* the clock type is defined */
+#define CLK_IS_TYPE_XTAL	BIT(8) /* this is a crystal clock */
 
 struct clk_hw;
 
-- 
1.7.10.4

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

[Luciano Coelho]

On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> Quoting Luciano Coelho (2013-07-04 14:05:12)
> > Add a flag that indicate whether the clock is a crystal or not.  Since
> > no clocks set this flag right now, include an additional flag that
> > indicates whether the type is set or not.  If the CLK_IS_TYPE_DEFINED
> > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined.
> > This ensures backwards compatibility.
> > 
> > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > this flag.
> > 
> > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > ---
> > 
> > I'm not  familiar with the common clock framework and I'm not
> > entirely sure the flags can be used in such a way, but to me it looks
> > reasonable, since some clock consumers may need to know what type of
> > clock is being provided.
> > 
> > Specifically, the wl12xx firmware needs to know if the clock is XTAL
> > or not to handle the stabilization and boosts properly.
> 
> Hi Luciano,

Hi Mike,

> I'd like clarification on one point. Is the same clock input signal used
> on the wifi chip? What I mean is, are there multiple clock inputs and
> XTAL is one, and not-XTAL is another?

This wifi chip can work with a few different clocks and some of them are
XTAL and others are not.  What the chip's firmware can use is one of
these:

19.2MHz (not XTAL)
26.0MHz (not XTAL)
26.0MHz (XTAL)
38.4MHz (not XTAL)
38.4MHz (XTAL)
52.0MHz (not XTAL)

It treats the XTAL clocks differently (but I don't really understand
enough of the details), so the driver needs to configure the firmware
according to these values.


> Or is it the same clock input and basically the problem is that you need
> to know what kind of waveform to expect (e.g. square versus sine)?

It's the same clock input in the chip's perspective.  One clock input
that can be any of the combinations I mentioned above.  Again, I'm not
familiar with clocks, so I guess the square vs. sine explanation is
plausible.  What I could see in the firmware is that it handles the
clocks differently if they're xtal or not.

--
Luca.

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

[Luciano Coelho]

On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> Quoting Luciano Coelho (2013-07-04 15:37:45)
> > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > Or is it the same clock input and basically the problem is that you need
> > > to know what kind of waveform to expect (e.g. square versus sine)?
> > 
> > It's the same clock input in the chip's perspective.  One clock input
> > that can be any of the combinations I mentioned above.  Again, I'm not
> > familiar with clocks, so I guess the square vs. sine explanation is
> > plausible.  What I could see in the firmware is that it handles the
> > clocks differently if they're xtal or not.
> 
> OMAP has a similar thing where sys_clkin (the fast reference clock for
> the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> the rates matter.

Right, this part is easy and I already have the code for that.  What I'm
missing is a way to pass this XTAL flag to the chip.


> In your case you need some extra metadata to know what to do. I'm really
> not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> take. It would be best to know if the waveform is what you really need
> to know, or perhaps something else. For instance you might be affected
> by some clock signal stabilization time. Can you talk to your hardware
> guys and figure it out? I'd rather model the actual needs instead of
> just tossing a flag in there.

I get your point.  I have tried to investigate how this flag is used by
the firmware and I could see that it is used to set different "buffer
gains" and "delays" when waking up (I guess this means when the clock is
starting, so probably related to stabilization time).  They specify two
"modes", "boost" and "normal" and use different delay values for each.

As I said, I don't know almost anything about clocks, so all this
doesn't make much sense to me.  But maybe you can get an idea?

--
Cheers,
Luca.

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

[James Hogan]

On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote:
> Add a flag that indicate whether the clock is a crystal or not.  Since
> no clocks set this flag right now, include an additional flag that
> indicates whether the type is set or not.  If the CLK_IS_TYPE_DEFINED
> flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined.
> This ensures backwards compatibility.
>
> Additionally, parse a new device tree binding in clk-fixed-rate to set
> this flag.
>
> Signed-off-by: Luciano Coelho <coelho@ti.com>
> ---
>
> I'm not  familiar with the common clock framework and I'm not
> entirely sure the flags can be used in such a way, but to me it looks
> reasonable, since some clock consumers may need to know what type of
> clock is being provided.
>
> Specifically, the wl12xx firmware needs to know if the clock is XTAL
> or not to handle the stabilization and boosts properly.
>
> My main idea is that I need to pass this information in the device
> tree definition of the clocks, so that the driver can pass this
> information on to the firmware.
>
> Please let me know if this looks ok or not.  If not, please let me
> know if you have any other ideas on how to solve my problem (of
> knowing whether the clock attached to the WiLink chip is XTAL or not).

The TZ1090 SoC has something that sounds possibly similar, where some
of the XTAL pads have a bypass bit, which according to the hardware
engineer I asked should be enabled when you want to use the
corresponding XTAL pads as a clock input pad rather than an
oscillator. I was considering extending clk-fixed-rate (via a wrapper
driver) to parse a custom DT property and a register address / bit
number and set the bypass bit as appropriate itself.

So I was wondering, is there a particular reason you don't have a DT
property on the node for the device that needs to know what type of
clock it is, rather than the clock node itself? That way you're not
depending directly on the generic common clock framework to be able to
tell you such electrical details.

Cheers
James

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

[Felipe Balbi]

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

On Fri, Jul 05, 2013 at 02:12:08PM +0100, James Hogan wrote:
> On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote:
> > Add a flag that indicate whether the clock is a crystal or not.  Since
> > no clocks set this flag right now, include an additional flag that
> > indicates whether the type is set or not.  If the CLK_IS_TYPE_DEFINED
> > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined.
> > This ensures backwards compatibility.
> >
> > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > this flag.
> >
> > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > ---
> >
> > I'm not  familiar with the common clock framework and I'm not
> > entirely sure the flags can be used in such a way, but to me it looks
> > reasonable, since some clock consumers may need to know what type of
> > clock is being provided.
> >
> > Specifically, the wl12xx firmware needs to know if the clock is XTAL
> > or not to handle the stabilization and boosts properly.
> >
> > My main idea is that I need to pass this information in the device
> > tree definition of the clocks, so that the driver can pass this
> > information on to the firmware.
> >
> > Please let me know if this looks ok or not.  If not, please let me
> > know if you have any other ideas on how to solve my problem (of
> > knowing whether the clock attached to the WiLink chip is XTAL or not).
> 
> The TZ1090 SoC has something that sounds possibly similar, where some
> of the XTAL pads have a bypass bit, which according to the hardware
> engineer I asked should be enabled when you want to use the
> corresponding XTAL pads as a clock input pad rather than an
> oscillator. I was considering extending clk-fixed-rate (via a wrapper
> driver) to parse a custom DT property and a register address / bit
> number and set the bypass bit as appropriate itself.
> 
> So I was wondering, is there a particular reason you don't have a DT
> property on the node for the device that needs to know what type of
> clock it is, rather than the clock node itself? That way you're not
> depending directly on the generic common clock framework to be able to
> tell you such electrical details.

three things here:

1) you end up with several devices implementing the clock type
attribute.

2) the type is a property of the clock itself

3) At least WiLink, can work with both types of clocks.

considering those, I really think this should belong to the clock node.
Otherwise Imagine how your DT would look like:

clock {
	compatible = "fixed-rate";
	...
};

wilink {
	clocks = &clock;
	wilink,btw-this-time-we-are-using-xtal;
	...
};

where you could:

clock {
	compatible = "fixed-rate";
	clock-type = "xtal";
	...
};

wilink {
	clocks = &clock;
	...
};

second option looks a lot cleaner to me.

-- 
balbi

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

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

[Luciano Coelho]

On Fri, 2013-07-05 at 14:12 +0100, James Hogan wrote:
> On 4 July 2013 22:05, Luciano Coelho <coelho@ti.com> wrote:
> > Add a flag that indicate whether the clock is a crystal or not.  Since
> > no clocks set this flag right now, include an additional flag that
> > indicates whether the type is set or not.  If the CLK_IS_TYPE_DEFINED
> > flag is not set, the value of the CLK_IS_TYPE_XTAL flag is undefined.
> > This ensures backwards compatibility.
> >
> > Additionally, parse a new device tree binding in clk-fixed-rate to set
> > this flag.
> >
> > Signed-off-by: Luciano Coelho <coelho@ti.com>
> > ---
> >
> > I'm not  familiar with the common clock framework and I'm not
> > entirely sure the flags can be used in such a way, but to me it looks
> > reasonable, since some clock consumers may need to know what type of
> > clock is being provided.
> >
> > Specifically, the wl12xx firmware needs to know if the clock is XTAL
> > or not to handle the stabilization and boosts properly.
> >
> > My main idea is that I need to pass this information in the device
> > tree definition of the clocks, so that the driver can pass this
> > information on to the firmware.
> >
> > Please let me know if this looks ok or not.  If not, please let me
> > know if you have any other ideas on how to solve my problem (of
> > knowing whether the clock attached to the WiLink chip is XTAL or not).
> 
> The TZ1090 SoC has something that sounds possibly similar, where some
> of the XTAL pads have a bypass bit, which according to the hardware
> engineer I asked should be enabled when you want to use the
> corresponding XTAL pads as a clock input pad rather than an
> oscillator.

Cool, good to know that I'm not alone here. ;)


>  I was considering extending clk-fixed-rate (via a wrapper
> driver) to parse a custom DT property and a register address / bit
> number and set the bypass bit as appropriate itself.

I thought about this too.  I actually have a "driver" for my clocks,
because normally they're not part of the main board, but part of the
module that contains the WiLink chip.  In those cases, I don't want my
clocks to be used as a generic clk-fixed-rate by the clock framework.
But the only difference in my "wrapper" is that it matches
"ti,wilink-clock" instead of "fixed-clock".


> So I was wondering, is there a particular reason you don't have a DT
> property on the node for the device that needs to know what type of
> clock it is, rather than the clock node itself? That way you're not
> depending directly on the generic common clock framework to be able to
> tell you such electrical details.

I think this is a detail of the clock itself, not of the user of the
clock.  Of course, the user of the clock needs to know what to do if the
clock is XTAL.  But the information of whether it is a XTAL or not
should be in the clock data.

I tried to make a generic solution that everyone could use.  For
instance, you. :) You could use the same bit that I implemented and, in
your driver, convert that bit into the action you need to take.


--
Luca.

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

[Luciano Coelho]

Hi Mike,

On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote:
> On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> > Quoting Luciano Coelho (2013-07-04 15:37:45)
> > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > > Or is it the same clock input and basically the problem is that you need
> > > > to know what kind of waveform to expect (e.g. square versus sine)?
> > > 
> > > It's the same clock input in the chip's perspective.  One clock input
> > > that can be any of the combinations I mentioned above.  Again, I'm not
> > > familiar with clocks, so I guess the square vs. sine explanation is
> > > plausible.  What I could see in the firmware is that it handles the
> > > clocks differently if they're xtal or not.
> > 
> > OMAP has a similar thing where sys_clkin (the fast reference clock for
> > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> > the rates matter.
> 
> Right, this part is easy and I already have the code for that.  What I'm
> missing is a way to pass this XTAL flag to the chip.
> 
> 
> > In your case you need some extra metadata to know what to do. I'm really
> > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> > take. It would be best to know if the waveform is what you really need
> > to know, or perhaps something else. For instance you might be affected
> > by some clock signal stabilization time. Can you talk to your hardware
> > guys and figure it out? I'd rather model the actual needs instead of
> > just tossing a flag in there.
> 
> I get your point.  I have tried to investigate how this flag is used by
> the firmware and I could see that it is used to set different "buffer
> gains" and "delays" when waking up (I guess this means when the clock is
> starting, so probably related to stabilization time).  They specify two
> "modes", "boost" and "normal" and use different delay values for each.

I tried but I couldn't find any more information on how exactly this
works.  But since this change is really simple and there seems to be
other people who need the same information, couldn't we add it as is and
try to figure out more specific information about the clocks later on?

Even if XTAL is not that useful if we know the other details, at least
it wouldn't hurt to have the flag there anyway.

--
Cheers,
Luca.

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

[Felipe Balbi]

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

Hi,

Fixing Luca's address since he left TI

On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote:
> Quoting Luciano Coelho (2013-07-29 06:50:42)
> > Hi Mike,
> > 
> > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote:
> > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> > > > Quoting Luciano Coelho (2013-07-04 15:37:45)
> > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > > > > Or is it the same clock input and basically the problem is that you need
> > > > > > to know what kind of waveform to expect (e.g. square versus sine)?
> > > > > 
> > > > > It's the same clock input in the chip's perspective.  One clock input
> > > > > that can be any of the combinations I mentioned above.  Again, I'm not
> > > > > familiar with clocks, so I guess the square vs. sine explanation is
> > > > > plausible.  What I could see in the firmware is that it handles the
> > > > > clocks differently if they're xtal or not.
> > > > 
> > > > OMAP has a similar thing where sys_clkin (the fast reference clock for
> > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> > > > the rates matter.
> > > 
> > > Right, this part is easy and I already have the code for that.  What I'm
> > > missing is a way to pass this XTAL flag to the chip.
> > > 
> > > 
> > > > In your case you need some extra metadata to know what to do. I'm really
> > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> > > > take. It would be best to know if the waveform is what you really need
> > > > to know, or perhaps something else. For instance you might be affected
> > > > by some clock signal stabilization time. Can you talk to your hardware
> > > > guys and figure it out? I'd rather model the actual needs instead of
> > > > just tossing a flag in there.
> > > 
> > > I get your point.  I have tried to investigate how this flag is used by
> > > the firmware and I could see that it is used to set different "buffer
> > > gains" and "delays" when waking up (I guess this means when the clock is
> > > starting, so probably related to stabilization time).  They specify two
> > > "modes", "boost" and "normal" and use different delay values for each.
> > 
> > I tried but I couldn't find any more information on how exactly this
> > works.  But since this change is really simple and there seems to be
> > other people who need the same information, couldn't we add it as is and
> > try to figure out more specific information about the clocks later on?
> > 
> > Even if XTAL is not that useful if we know the other details, at least
> > it wouldn't hurt to have the flag there anyway.
> 
> Luca,
> 
> By any chance did you come to a different solution for this problem? I
> can take the patch, but I do not feel like we're solving the right
> problem the right way.
> 
> If not I will take it for 3.13.
> 
> Regards,
> Mike
> 
> > 
> > --
> > Cheers,
> > Luca.

-- 
balbi

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

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

[Luca Coelho]

Hi,

Sorry for the delayed response.

On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote:
> Fixing Luca's address since he left TI

Thanks, Felipe! I wouldn't have seen this otherwise.


> On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote:
> > Quoting Luciano Coelho (2013-07-29 06:50:42)
> > > Hi Mike,
> > > 
> > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote:
> > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> > > > > Quoting Luciano Coelho (2013-07-04 15:37:45)
> > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > > > > > Or is it the same clock input and basically the problem is that you need
> > > > > > > to know what kind of waveform to expect (e.g. square versus sine)?
> > > > > > 
> > > > > > It's the same clock input in the chip's perspective.  One clock input
> > > > > > that can be any of the combinations I mentioned above.  Again, I'm not
> > > > > > familiar with clocks, so I guess the square vs. sine explanation is
> > > > > > plausible.  What I could see in the firmware is that it handles the
> > > > > > clocks differently if they're xtal or not.
> > > > > 
> > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for
> > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> > > > > the rates matter.
> > > > 
> > > > Right, this part is easy and I already have the code for that.  What I'm
> > > > missing is a way to pass this XTAL flag to the chip.
> > > > 
> > > > 
> > > > > In your case you need some extra metadata to know what to do. I'm really
> > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> > > > > take. It would be best to know if the waveform is what you really need
> > > > > to know, or perhaps something else. For instance you might be affected
> > > > > by some clock signal stabilization time. Can you talk to your hardware
> > > > > guys and figure it out? I'd rather model the actual needs instead of
> > > > > just tossing a flag in there.
> > > > 
> > > > I get your point.  I have tried to investigate how this flag is used by
> > > > the firmware and I could see that it is used to set different "buffer
> > > > gains" and "delays" when waking up (I guess this means when the clock is
> > > > starting, so probably related to stabilization time).  They specify two
> > > > "modes", "boost" and "normal" and use different delay values for each.
> > > 
> > > I tried but I couldn't find any more information on how exactly this
> > > works.  But since this change is really simple and there seems to be
> > > other people who need the same information, couldn't we add it as is and
> > > try to figure out more specific information about the clocks later on?
> > > 
> > > Even if XTAL is not that useful if we know the other details, at least
> > > it wouldn't hurt to have the flag there anyway.
> > 
> > Luca,
> > 
> > By any chance did you come to a different solution for this problem? I
> > can take the patch, but I do not feel like we're solving the right
> > problem the right way.

Unfortunately I didn't find any better solution for this.  From the
WiLink firmware's point of view, there was not much information about
the details of stabilization time requirements and such.

 
> > If not I will take it for 3.13.

That would be great! As I mentioned above, the XTAL/non-XTAL flag
wouldn't hurt, even if in most cases it may not provide the necessary
details for the clock code.  But at least for the WiLink hardware it is
very useful. ;)

--
Cheers,
Luca.

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

[Mike Turquette]

Quoting Luca Coelho (2013-10-16 03:24:27)
> Hi,
> 
> Sorry for the delayed response.
> 
> On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote:
> > Fixing Luca's address since he left TI
> 
> Thanks, Felipe! I wouldn't have seen this otherwise.
> 
> 
> > On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote:
> > > Quoting Luciano Coelho (2013-07-29 06:50:42)
> > > > Hi Mike,
> > > > 
> > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote:
> > > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> > > > > > Quoting Luciano Coelho (2013-07-04 15:37:45)
> > > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > > > > > > Or is it the same clock input and basically the problem is that you need
> > > > > > > > to know what kind of waveform to expect (e.g. square versus sine)?
> > > > > > > 
> > > > > > > It's the same clock input in the chip's perspective.  One clock input
> > > > > > > that can be any of the combinations I mentioned above.  Again, I'm not
> > > > > > > familiar with clocks, so I guess the square vs. sine explanation is
> > > > > > > plausible.  What I could see in the firmware is that it handles the
> > > > > > > clocks differently if they're xtal or not.
> > > > > > 
> > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for
> > > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> > > > > > the rates matter.
> > > > > 
> > > > > Right, this part is easy and I already have the code for that.  What I'm
> > > > > missing is a way to pass this XTAL flag to the chip.
> > > > > 
> > > > > 
> > > > > > In your case you need some extra metadata to know what to do. I'm really
> > > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> > > > > > take. It would be best to know if the waveform is what you really need
> > > > > > to know, or perhaps something else. For instance you might be affected
> > > > > > by some clock signal stabilization time. Can you talk to your hardware
> > > > > > guys and figure it out? I'd rather model the actual needs instead of
> > > > > > just tossing a flag in there.
> > > > > 
> > > > > I get your point.  I have tried to investigate how this flag is used by
> > > > > the firmware and I could see that it is used to set different "buffer
> > > > > gains" and "delays" when waking up (I guess this means when the clock is
> > > > > starting, so probably related to stabilization time).  They specify two
> > > > > "modes", "boost" and "normal" and use different delay values for each.
> > > > 
> > > > I tried but I couldn't find any more information on how exactly this
> > > > works.  But since this change is really simple and there seems to be
> > > > other people who need the same information, couldn't we add it as is and
> > > > try to figure out more specific information about the clocks later on?
> > > > 
> > > > Even if XTAL is not that useful if we know the other details, at least
> > > > it wouldn't hurt to have the flag there anyway.
> > > 
> > > Luca,
> > > 
> > > By any chance did you come to a different solution for this problem? I
> > > can take the patch, but I do not feel like we're solving the right
> > > problem the right way.
> 
> Unfortunately I didn't find any better solution for this.  From the
> WiLink firmware's point of view, there was not much information about
> the details of stabilization time requirements and such.
> 
>  
> > > If not I will take it for 3.13.
> 
> That would be great! As I mentioned above, the XTAL/non-XTAL flag
> wouldn't hurt, even if in most cases it may not provide the necessary
> details for the clock code.  But at least for the WiLink hardware it is
> very useful. ;)

Why is the CLK_IS_TYPE_DEFINED necessary?

Also we're adding a property to the fixed-rate binding so
Documentation/devicetree/bindings/clock/fixed-clock.txt needs to be
updated.

Regards,
Mike

> 
> --
> Cheers,
> Luca.

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

[Luca Coelho]

On Wed, 2013-10-23 at 02:24 -0700, Mike Turquette wrote:
> Quoting Luca Coelho (2013-10-16 03:24:27)
> > Hi,
> > 
> > Sorry for the delayed response.
> > 
> > On Tue, 2013-10-08 at 10:27 -0500, Felipe Balbi wrote:
> > > Fixing Luca's address since he left TI
> > 
> > Thanks, Felipe! I wouldn't have seen this otherwise.
> > 
> > 
> > > On Mon, Oct 07, 2013 at 12:44:24AM -0700, Mike Turquette wrote:
> > > > Quoting Luciano Coelho (2013-07-29 06:50:42)
> > > > > Hi Mike,
> > > > > 
> > > > > On Fri, 2013-07-05 at 10:54 +0300, Luciano Coelho wrote:
> > > > > > On Thu, 2013-07-04 at 16:19 -0700, Mike Turquette wrote:
> > > > > > > Quoting Luciano Coelho (2013-07-04 15:37:45)
> > > > > > > > On Thu, 2013-07-04 at 15:25 -0700, Mike Turquette wrote:
> > > > > > > > > Or is it the same clock input and basically the problem is that you need
> > > > > > > > > to know what kind of waveform to expect (e.g. square versus sine)?
> > > > > > > > 
> > > > > > > > It's the same clock input in the chip's perspective.  One clock input
> > > > > > > > that can be any of the combinations I mentioned above.  Again, I'm not
> > > > > > > > familiar with clocks, so I guess the square vs. sine explanation is
> > > > > > > > plausible.  What I could see in the firmware is that it handles the
> > > > > > > > clocks differently if they're xtal or not.
> > > > > > > 
> > > > > > > OMAP has a similar thing where sys_clkin (the fast reference clock for
> > > > > > > the chip) can be 19.2, 26, 38.4, etc. This is easy to handle since only
> > > > > > > the rates matter.
> > > > > > 
> > > > > > Right, this part is easy and I already have the code for that.  What I'm
> > > > > > missing is a way to pass this XTAL flag to the chip.
> > > > > > 
> > > > > > 
> > > > > > > In your case you need some extra metadata to know what to do. I'm really
> > > > > > > not sure if CLK_IS_TYPE_XTAL is the most useful form this metadata can
> > > > > > > take. It would be best to know if the waveform is what you really need
> > > > > > > to know, or perhaps something else. For instance you might be affected
> > > > > > > by some clock signal stabilization time. Can you talk to your hardware
> > > > > > > guys and figure it out? I'd rather model the actual needs instead of
> > > > > > > just tossing a flag in there.
> > > > > > 
> > > > > > I get your point.  I have tried to investigate how this flag is used by
> > > > > > the firmware and I could see that it is used to set different "buffer
> > > > > > gains" and "delays" when waking up (I guess this means when the clock is
> > > > > > starting, so probably related to stabilization time).  They specify two
> > > > > > "modes", "boost" and "normal" and use different delay values for each.
> > > > > 
> > > > > I tried but I couldn't find any more information on how exactly this
> > > > > works.  But since this change is really simple and there seems to be
> > > > > other people who need the same information, couldn't we add it as is and
> > > > > try to figure out more specific information about the clocks later on?
> > > > > 
> > > > > Even if XTAL is not that useful if we know the other details, at least
> > > > > it wouldn't hurt to have the flag there anyway.
> > > > 
> > > > Luca,
> > > > 
> > > > By any chance did you come to a different solution for this problem? I
> > > > can take the patch, but I do not feel like we're solving the right
> > > > problem the right way.
> > 
> > Unfortunately I didn't find any better solution for this.  From the
> > WiLink firmware's point of view, there was not much information about
> > the details of stabilization time requirements and such.
> > 
> >  
> > > > If not I will take it for 3.13.
> > 
> > That would be great! As I mentioned above, the XTAL/non-XTAL flag
> > wouldn't hurt, even if in most cases it may not provide the necessary
> > details for the clock code.  But at least for the WiLink hardware it is
> > very useful. ;)
> 
> Why is the CLK_IS_TYPE_DEFINED necessary?

This is just for backwards compatibility.  Currently all the other
clocks don't set the XTAL/non-XTAL flag (even if they *are* XTAL), so if
the CLK_IS_TYPE_DEFINED is not set, we know we can't trust the
XTAL/no-XTAL flag.


> Also we're adding a property to the fixed-rate binding so
> Documentation/devicetree/bindings/clock/fixed-clock.txt needs to be
> updated.

Yeah, you're right.  Unfortunately, I won't have the time to resubmit
this patch any time soon. :(

--
Luca.

[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

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

[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 --]

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

[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 --]