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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 11 Nov 2013 21:50:56 +0200
From:	Luca Coelho <luca@...lho.fi>
To:	balbi@...com
Cc:	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	mturquette@...aro.org, james.hogan@...tec.com,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
	Linux ARM Kernel Mailing List 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] clk: add flags to distinguish xtal clocks

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@...lho.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@...lho.fi>
> > > Signed-off-by: Felipe Balbi <balbi@...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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/