[RFC 0/8] Initial DT clock bindings

[RFC 0/8] Initial DT clock bindings

[Grant Likely]

Hey all, here is an initial implementation of the DT clock bindings
with the versatile platform modified to use them.  I'm fairly happy
with how this has turned out, although I'll probably roll the clock
lookup stuff directly into drivers/clk/clkdev.c which will simplify
things a bit.  Please take a look and comment.

You'll also notice that I've started adding selftests to the
devicetree code, which turned out to be pretty useful for debugging.
I think I want to do this for a lot of the dt infrastructure.

The first 5 patches are probably pretty close to ready to be merged,
the last 3 I expect to still need work.

g.

[RFC 7/8] arm/clkdev: lookup clocks from OF clock providers

[Rob Herring]

On 11/08/2011 07:19 PM, Grant Likely wrote:
> From: Jeremy Kerr <jeremy.kerr@canonical.com>
> 
> Hook the OF clock provider infrastructure to clk_get.
> 
> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/clk/clkdev.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..63f81c9 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -19,6 +19,8 @@
>  #include <linux/mutex.h>
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
>  
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
> @@ -78,6 +80,11 @@ EXPORT_SYMBOL(clk_get_sys);
>  struct clk *clk_get(struct device *dev, const char *con_id)
>  {
>  	const char *dev_id = dev ? dev_name(dev) : NULL;
> +	struct clk *clk;
> +

Need NULL check for dev (and then init clk to NULL):

	if (dev && dev->of_node)
> +	clk = of_clk_get_by_name(dev->of_node, con_id);

> +	if (clk && __clk_get(clk))
> +		return clk;
>  
>  	return clk_get_sys(dev_id, con_id);
>  }

[RFC 7/8] arm/clkdev: lookup clocks from OF clock providers

[Grant Likely]

On Tue, Nov 8, 2011 at 7:36 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 11/08/2011 07:19 PM, Grant Likely wrote:
>> From: Jeremy Kerr <jeremy.kerr@canonical.com>
>>
>> Hook the OF clock provider infrastructure to clk_get.
>>
>> Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>> ?drivers/clk/clkdev.c | ? ?7 +++++++
>> ?1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index 6db161f..63f81c9 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -19,6 +19,8 @@
>> ?#include <linux/mutex.h>
>> ?#include <linux/clk.h>
>> ?#include <linux/clkdev.h>
>> +#include <linux/of.h>
>> +#include <linux/of_clk.h>
>>
>> ?static LIST_HEAD(clocks);
>> ?static DEFINE_MUTEX(clocks_mutex);
>> @@ -78,6 +80,11 @@ EXPORT_SYMBOL(clk_get_sys);
>> ?struct clk *clk_get(struct device *dev, const char *con_id)
>> ?{
>> ? ? ? const char *dev_id = dev ? dev_name(dev) : NULL;
>> + ? ? struct clk *clk;
>> +
>
> Need NULL check for dev (and then init clk to NULL):
>
> ? ? ? ?if (dev && dev->of_node)
>> + ? ? clk = of_clk_get_by_name(dev->of_node, con_id);
>
>> + ? ? if (clk && __clk_get(clk))
>> + ? ? ? ? ? ? return clk;

Fixed, thanks.

g.

[RFC 6/8] of: add clock providers

[Rob Herring]

On 11/08/2011 07:19 PM, Grant Likely wrote:
> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> of_clk_get function to allow platforms to retrieve clock data from the
> device tree.
> 
> Platform register a provider through of_clk_add_provider, which will be
> called when a device references the provider's OF node for a clock
> reference.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |  109 +++++++++++++++++
>  drivers/of/Kconfig                                 |    6 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/clock.c                                 |  129 ++++++++++++++++++++
>  include/linux/of_clk.h                             |   37 ++++++
>  5 files changed, 282 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
>  create mode 100644 drivers/of/clock.c
>  create mode 100644 include/linux/of_clk.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> new file mode 100644
> index 0000000..4770c7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -0,0 +1,109 @@
> +This binding is a work-in-progress, and are based on some experimental
> +work by benh[1].
> +
> +Sources of clock signal can be represented by any node in the device
> +tree.  Those nodes are designated as clock providers.  Clock consumer
> +nodes use a phandle and clock specifier pair to connect clock provider
> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> +specifier is an array of one more more cells identifying the clock
> +output on a device.  The length of a clock specifier is defined by the
> +value of a #clock-cells property in the clock provider node.
> +
> +[1] http://patchwork.ozlabs.org/patch/31551/
> +
> +==Clock providers==
> +
> +Required properties:
> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> +		   set to 1
> +
> +Optional properties:
> +clock-output-name: Recommended to be a list of strings of clock output signal
> +		   names indexed by the first cell in the clock specifier.
> +		   However, the meaning of clock-output-names is domain
> +		   specific to the clock provider, and is only provided to
> +		   encourage using the same meaning for the majority of clock
> +		   providers.  This format may not work for clock providers
> +		   using a complex clock specifier format.  In those cases it
> +		   is recommended to omit this property and create a binding
> +		   specific names property.
> +
> +		   Clock consumer nodes must never directly reference
> +		   the provider's clock-output-names property.
> +
> +For example:
> +
> +    oscillator {
> +        #clock-cells = <1>;
> +        clock-output-name = "ckil", "ckih";
> +    };
> +
> +- this node defines a device with two clock outputs, the first named
> +  "ckil" and the second named "ckih".  Consumer nodes always reference
> +  clocks by index. The names should reflect the clock output signal
> +  names for the device.
> +
> +==Clock consumers==
> +
> +Required properties:
> +clock-input:	List of phandle and clock specifier pairs, one pair
> +		for each clock input to the device.
> +clock-input-name: List of clock input name strings sorted in the same
> +		order as the clock-input property.  Consumers drivers
> +		will use clock-input-name to match clock input names
> +		with clock-input specifiers.
> +
> +For example:
> +
> +    uart {
> +        clock-input = <&osc 1> <&ref 0>;
> +        clock-input-name = "baud", "register";
> +    };

This is duplicated below.

> +
> +
> +This represents a device with two clock inputs, named "baud" and "register".
> +The baud clock is connected to output 1 of the &osc device, and the register
> +clock is connected to output 0 of the &ref.
> +
> +==Example==
> +
> +    /* external oscillator */
> +    osc: oscillator {
> +        compatible = "fixed-clock";
> +        #clock-cells = <1>;
> +        frequency  = <32678>;

The code is using "clock-frequency" here.

> +        clock-output-name = "osc";
> +    };
> +
> +    /* phase-locked-loop device, generates a higher frequency clock
> +     * from the external oscillator reference */
> +    pll: pll {
> +        compatible = "some-pll-interface"
> +        #clock-cells = <1>;
> +        clock-input = <&osc 0>;

There's a mismatch in #clock-cells size and this.

> +        clock-input-name = "ref";
> +        reg = <0x4c000 0x1000>;
> +        clock-output-name = "pll", "pll-switched";
> +    };
> +
> +    /* UART, using the low frequency oscillator for the baud clock,
> +     * and the high frequency switched PLL output for register
> +     * clocking */
> +    uart {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +        interrupts = <33>;
> +        clock-input = <&osc 0>, <&pll 1>;
> +        clock-input-name = "baud", "register";
> +    };
> +
> +This DT fragment defines three devices: an external oscillator to provide a
> +low-frequency reference clock, a PLL device to generate a higher frequency
> +clock signal, and a UART.
> +
> +* The oscillator is fixed-frequency, and provides one clock output, named "osc".
> +* The PLL is both a clock provider and a clock consumer. It uses the clock
> +  signal generated by the external oscillator, and provides two output signals
> +  ("pll" and "pll-switched").
> +* The UART has its baud clock connected the external oscillator and its
> +  register clock connected to the PLL clock (the "pll-switched" signal)
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 268163d..b49ab9c 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -47,6 +47,12 @@ config OF_IRQ
>  	def_bool y
>  	depends on !SPARC
>  
> +config OF_CLOCK
> +	def_bool y
> +	depends on HAVE_CLK
> +	help
> +	  OpenFirmware clock accessors
> +
>  config OF_DEVICE
>  	def_bool y
>  
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index a73f5a5..e7ad1e9 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS)  += address.o
>  obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_GPIO)   += gpio.o
> +obj-$(CONFIG_OF_CLOCK)	+= clock.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
>  obj-$(CONFIG_OF_SPI)	+= of_spi.o
> diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> new file mode 100644
> index 0000000..542b9e4
> --- /dev/null
> +++ b/drivers/of/clock.c
> @@ -0,0 +1,129 @@
> +/*
> + * Clock infrastructure for device tree platforms
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_clk.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +
> +/**
> + * struct of_clk_provider - Clock provider registration structure
> + * @link: Entry in global list of clock providers
> + * @node: Pointer to device tree node of clock provider
> + * @get: Get clock callback.  Returns NULL or a struct clk for the
> + *       given clock specifier
> + * @data: context pointer to be passed into @get callback
> + */
> +struct of_clk_provider {
> +	struct list_head link;
> +
> +	struct device_node *node;
> +	struct clk *(*get)(struct of_phandle_args *clkspec, void *data);
> +	void *data;
> +};
> +
> +static LIST_HEAD(of_clk_providers);
> +static DEFINE_MUTEX(of_clk_lock);
> +
> +/**
> + * of_clk_add_provider() - Register a clock provider for a node
> + * @np: Device node pointer associated with clock provider
> + * @clk_src_get: callback for decoding clock
> + * @data: context pointer for @clk_src_get callback.
> + */
> +int of_clk_add_provider(struct device_node *np,
> +			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
> +						   void *data),
> +			void *data)
> +{
> +	struct of_clk_provider *cp;
> +
> +	cp = kzalloc(sizeof(struct of_clk_provider), GFP_KERNEL);
> +	if (!cp)
> +		return -ENOMEM;
> +
> +	cp->node = of_node_get(np);
> +	cp->data = data;
> +	cp->get = clk_src_get;
> +
> +	mutex_lock(&of_clk_lock);
> +	list_add(&cp->link, &of_clk_providers);
> +	mutex_unlock(&of_clk_lock);
> +	pr_debug("Added clock from %s\n", np->full_name);
> +
> +	return 0;
> +}
> +
> +/**
> + * of_clk_del_provider() - Remove a previously registered clock provider
> + * @np: Device node pointer associated with clock provider
> + */
> +void of_clk_del_provider(struct device_node *np)
> +{
> +	struct of_clk_provider *cp;
> +
> +	mutex_lock(&of_clk_lock);
> +	list_for_each_entry(cp, &of_clk_providers, link) {
> +		if (cp->node == np) {
> +			list_del(&cp->link);
> +			of_node_put(cp->node);
> +			kfree(cp);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&of_clk_lock);
> +}
> +
> +static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
> +{
> +	struct of_clk_provider *provider;
> +	struct clk *clk = NULL;
> +
> +	/* Check if we have such a provider in our array */
> +	mutex_lock(&of_clk_lock);
> +	list_for_each_entry(provider, &of_clk_providers, link) {
> +		if (provider->node == clkspec->np)
> +			clk = provider->get(clkspec, provider->data);

How about:

if (provider->get)
	clk = provider->get(clkspec, provider->data);
else
	clk = provider->data;


Or a default get function can do this.

> +		if (clk)
> +			break;
> +	}
> +	mutex_unlock(&of_clk_lock);
> +
> +	return clk;
> +}
> +
> +struct clk *of_clk_get(struct device_node *np, int index)
> +{
> +	struct of_phandle_args clkspec;
> +	struct clk *clk;
> +	int rc;
> +
> +	if (index < 0)
> +		return NULL;
> +
> +	rc = of_parse_phandle_with_args(np, "clock", "#clock-cells", index,
> +					&clkspec);
> +	if (rc)
> +		return NULL;
> +
> +	clk = __of_clk_get_from_provider(&clkspec);
> +	of_node_put(clkspec.np);
> +	return clk;
> +}
> +
> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> +{
> +	int index = 0;
> +
> +	if (name)
> +		index = of_property_match_string(np, "clock-input-names", name);
> +	return of_clk_get(np, index);
> +}
> +
> diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
> new file mode 100644
> index 0000000..e476a5f
> --- /dev/null
> +++ b/include/linux/of_clk.h
> @@ -0,0 +1,37 @@
> +/*
> + * Clock infrastructure for device tree platforms
> + */
> +#ifndef __OF_CLK_H
> +#define __OF_CLK_H
> +
> +struct device;
> +struct clk;
> +
> +#ifdef CONFIG_OF_CLOCK
> +
> +struct device_node;
> +
> +int of_clk_add_provider(struct device_node *np,
> +			struct clk *(*clk_src_get)(struct of_phandle_args *args,
> +						   void *data),
> +			void *data);
> +
> +void of_clk_del_provider(struct device_node *np);
> +
> +struct clk *of_clk_get(struct device_node *np, int index);
> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
> +
> +#else
> +static struct clk *of_clk_get(struct device_node *np, int index);
> +{
> +	return NULL;
> +}
> +
> +static struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +#endif /* __OF_CLK_H */
> +

[RFC 6/8] of: add clock providers

[Richard Zhao]

On Tue, Nov 08, 2011 at 08:49:42PM -0600, Rob Herring wrote:
> On 11/08/2011 07:19 PM, Grant Likely wrote:
> > Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> > of_clk_get function to allow platforms to retrieve clock data from the
> > device tree.
> > 
> > Platform register a provider through of_clk_add_provider, which will be
> > called when a device references the provider's OF node for a clock
> > reference.
> > 
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >  .../devicetree/bindings/clock/clock-bindings.txt   |  109 +++++++++++++++++
> >  drivers/of/Kconfig                                 |    6 +
> >  drivers/of/Makefile                                |    1 +
> >  drivers/of/clock.c                                 |  129 ++++++++++++++++++++
> >  include/linux/of_clk.h                             |   37 ++++++
> >  5 files changed, 282 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
> >  create mode 100644 drivers/of/clock.c
> >  create mode 100644 include/linux/of_clk.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > new file mode 100644
> > index 0000000..4770c7e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -0,0 +1,109 @@
> > +This binding is a work-in-progress, and are based on some experimental
> > +work by benh[1].
> > +
> > +Sources of clock signal can be represented by any node in the device
> > +tree.  Those nodes are designated as clock providers.  Clock consumer
> > +nodes use a phandle and clock specifier pair to connect clock provider
> > +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> > +specifier is an array of one more more cells identifying the clock
> > +output on a device.  The length of a clock specifier is defined by the
> > +value of a #clock-cells property in the clock provider node.
> > +
> > +[1] http://patchwork.ozlabs.org/patch/31551/
> > +
> > +==Clock providers==
> > +
> > +Required properties:
> > +#clock-cells:	   Number of cells in a clock specifier; typically will be
> > +		   set to 1
> > +
> > +Optional properties:
> > +clock-output-name: Recommended to be a list of strings of clock output signal
> > +		   names indexed by the first cell in the clock specifier.
> > +		   However, the meaning of clock-output-names is domain
> > +		   specific to the clock provider, and is only provided to
> > +		   encourage using the same meaning for the majority of clock
> > +		   providers.  This format may not work for clock providers
> > +		   using a complex clock specifier format.  In those cases it
> > +		   is recommended to omit this property and create a binding
> > +		   specific names property.
> > +
> > +		   Clock consumer nodes must never directly reference
> > +		   the provider's clock-output-names property.
> > +
> > +For example:
> > +
> > +    oscillator {
> > +        #clock-cells = <1>;
> > +        clock-output-name = "ckil", "ckih";
> > +    };
> > +
> > +- this node defines a device with two clock outputs, the first named
> > +  "ckil" and the second named "ckih".  Consumer nodes always reference
> > +  clocks by index. The names should reflect the clock output signal
> > +  names for the device.
> > +
> > +==Clock consumers==
> > +
> > +Required properties:
> > +clock-input:	List of phandle and clock specifier pairs, one pair
> > +		for each clock input to the device.
> > +clock-input-name: List of clock input name strings sorted in the same
> > +		order as the clock-input property.  Consumers drivers
> > +		will use clock-input-name to match clock input names
> > +		with clock-input specifiers.
> > +
> > +For example:
> > +
> > +    uart {
> > +        clock-input = <&osc 1> <&ref 0>;
> > +        clock-input-name = "baud", "register";
> > +    };
> 
> This is duplicated below.
> 
> > +
> > +
> > +This represents a device with two clock inputs, named "baud" and "register".
> > +The baud clock is connected to output 1 of the &osc device, and the register
> > +clock is connected to output 0 of the &ref.
> > +
> > +==Example==
> > +
> > +    /* external oscillator */
> > +    osc: oscillator {
> > +        compatible = "fixed-clock";
> > +        #clock-cells = <1>;
> > +        frequency  = <32678>;
> 
> The code is using "clock-frequency" here.
> 
> > +        clock-output-name = "osc";
> > +    };
> > +
> > +    /* phase-locked-loop device, generates a higher frequency clock
> > +     * from the external oscillator reference */
> > +    pll: pll {
> > +        compatible = "some-pll-interface"
> > +        #clock-cells = <1>;
> > +        clock-input = <&osc 0>;
> 
> There's a mismatch in #clock-cells size and this.
osc #clock-cells is 1. "0" is one cell too. Nothing wrong. Correct my understanding?
> 
> > +        clock-input-name = "ref";
> > +        reg = <0x4c000 0x1000>;
> > +        clock-output-name = "pll", "pll-switched";
> > +    };
> > +
> > +    /* UART, using the low frequency oscillator for the baud clock,
> > +     * and the high frequency switched PLL output for register
> > +     * clocking */
> > +    uart {
> > +        compatible = "fsl,imx-uart";
> > +        reg = <0xa000 0x1000>;
> > +        interrupts = <33>;
> > +        clock-input = <&osc 0>, <&pll 1>;
> > +        clock-input-name = "baud", "register";
> > +    };
> > +
> > +This DT fragment defines three devices: an external oscillator to provide a
> > +low-frequency reference clock, a PLL device to generate a higher frequency
> > +clock signal, and a UART.
> > +
> > +* The oscillator is fixed-frequency, and provides one clock output, named "osc".
> > +* The PLL is both a clock provider and a clock consumer. It uses the clock
> > +  signal generated by the external oscillator, and provides two output signals
> > +  ("pll" and "pll-switched").
> > +* The UART has its baud clock connected the external oscillator and its
> > +  register clock connected to the PLL clock (the "pll-switched" signal)
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 268163d..b49ab9c 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -47,6 +47,12 @@ config OF_IRQ
> >  	def_bool y
> >  	depends on !SPARC
> >  
> > +config OF_CLOCK
> > +	def_bool y
> > +	depends on HAVE_CLK
> > +	help
> > +	  OpenFirmware clock accessors
> > +
> >  config OF_DEVICE
> >  	def_bool y
> >  
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index a73f5a5..e7ad1e9 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS)  += address.o
> >  obj-$(CONFIG_OF_IRQ)    += irq.o
> >  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
> >  obj-$(CONFIG_OF_GPIO)   += gpio.o
> > +obj-$(CONFIG_OF_CLOCK)	+= clock.o
> >  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
> >  obj-$(CONFIG_OF_NET)	+= of_net.o
> >  obj-$(CONFIG_OF_SPI)	+= of_spi.o
> > diff --git a/drivers/of/clock.c b/drivers/of/clock.c
> > new file mode 100644
> > index 0000000..542b9e4
> > --- /dev/null
> > +++ b/drivers/of/clock.c
> > @@ -0,0 +1,129 @@
> > +/*
> > + * Clock infrastructure for device tree platforms
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_clk.h>
> > +#include <linux/list.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/device.h>
> > +
> > +/**
> > + * struct of_clk_provider - Clock provider registration structure
> > + * @link: Entry in global list of clock providers
> > + * @node: Pointer to device tree node of clock provider
> > + * @get: Get clock callback.  Returns NULL or a struct clk for the
> > + *       given clock specifier
> > + * @data: context pointer to be passed into @get callback
> > + */
> > +struct of_clk_provider {
> > +	struct list_head link;
> > +
> > +	struct device_node *node;
> > +	struct clk *(*get)(struct of_phandle_args *clkspec, void *data);
> > +	void *data;
> > +};
> > +
> > +static LIST_HEAD(of_clk_providers);
> > +static DEFINE_MUTEX(of_clk_lock);
> > +
> > +/**
> > + * of_clk_add_provider() - Register a clock provider for a node
> > + * @np: Device node pointer associated with clock provider
> > + * @clk_src_get: callback for decoding clock
> > + * @data: context pointer for @clk_src_get callback.
> > + */
> > +int of_clk_add_provider(struct device_node *np,
> > +			struct clk *(*clk_src_get)(struct of_phandle_args *clkspec,
> > +						   void *data),
> > +			void *data)
> > +{
> > +	struct of_clk_provider *cp;
> > +
> > +	cp = kzalloc(sizeof(struct of_clk_provider), GFP_KERNEL);
> > +	if (!cp)
> > +		return -ENOMEM;
> > +
> > +	cp->node = of_node_get(np);
> > +	cp->data = data;
> > +	cp->get = clk_src_get;
> > +
> > +	mutex_lock(&of_clk_lock);
> > +	list_add(&cp->link, &of_clk_providers);
> > +	mutex_unlock(&of_clk_lock);
> > +	pr_debug("Added clock from %s\n", np->full_name);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * of_clk_del_provider() - Remove a previously registered clock provider
> > + * @np: Device node pointer associated with clock provider
> > + */
> > +void of_clk_del_provider(struct device_node *np)
> > +{
> > +	struct of_clk_provider *cp;
> > +
> > +	mutex_lock(&of_clk_lock);
> > +	list_for_each_entry(cp, &of_clk_providers, link) {
> > +		if (cp->node == np) {
> > +			list_del(&cp->link);
> > +			of_node_put(cp->node);
> > +			kfree(cp);
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&of_clk_lock);
> > +}
> > +
> > +static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
> > +{
> > +	struct of_clk_provider *provider;
> > +	struct clk *clk = NULL;
> > +
> > +	/* Check if we have such a provider in our array */
> > +	mutex_lock(&of_clk_lock);
> > +	list_for_each_entry(provider, &of_clk_providers, link) {
> > +		if (provider->node == clkspec->np)
> > +			clk = provider->get(clkspec, provider->data);
> 
> How about:
> 
> if (provider->get)
> 	clk = provider->get(clkspec, provider->data);
> else
> 	clk = provider->data;
> 
> 
> Or a default get function can do this.
> 
> > +		if (clk)
> > +			break;
> > +	}
> > +	mutex_unlock(&of_clk_lock);
> > +
> > +	return clk;
> > +}
> > +
> > +struct clk *of_clk_get(struct device_node *np, int index)
> > +{
> > +	struct of_phandle_args clkspec;
> > +	struct clk *clk;
> > +	int rc;
> > +
> > +	if (index < 0)
> > +		return NULL;
> > +
> > +	rc = of_parse_phandle_with_args(np, "clock", "#clock-cells", index,
> > +					&clkspec);
> > +	if (rc)
> > +		return NULL;
> > +
> > +	clk = __of_clk_get_from_provider(&clkspec);
> > +	of_node_put(clkspec.np);
> > +	return clk;
> > +}
> > +
> > +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> > +{
> > +	int index = 0;
> > +
> > +	if (name)
> > +		index = of_property_match_string(np, "clock-input-names", name);
> > +	return of_clk_get(np, index);
> > +}
> > +
> > diff --git a/include/linux/of_clk.h b/include/linux/of_clk.h
> > new file mode 100644
> > index 0000000..e476a5f
> > --- /dev/null
> > +++ b/include/linux/of_clk.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Clock infrastructure for device tree platforms
> > + */
> > +#ifndef __OF_CLK_H
> > +#define __OF_CLK_H
> > +
> > +struct device;
> > +struct clk;
> > +
> > +#ifdef CONFIG_OF_CLOCK
> > +
> > +struct device_node;
> > +
> > +int of_clk_add_provider(struct device_node *np,
> > +			struct clk *(*clk_src_get)(struct of_phandle_args *args,
> > +						   void *data),
> > +			void *data);
> > +
> > +void of_clk_del_provider(struct device_node *np);
> > +
> > +struct clk *of_clk_get(struct device_node *np, int index);
> > +struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
> > +
> > +#else
> > +static struct clk *of_clk_get(struct device_node *np, int index);
> > +{
> > +	return NULL;
> > +}
> > +
> > +static struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> > +{
> > +	return NULL;
> > +}
> > +#endif
> > +
> > +#endif /* __OF_CLK_H */
> > +
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC 6/8] of: add clock providers

[Grant Likely]

On Sun, Nov 13, 2011 at 7:14 PM, Richard Zhao <richard.zhao@linaro.org> wrote:
> On Tue, Nov 08, 2011 at 08:49:42PM -0600, Rob Herring wrote:
>> > +For example:
>> > +
>> > + ? ?oscillator {
>> > + ? ? ? ?#clock-cells = <1>;
>> > + ? ? ? ?clock-output-name = "ckil", "ckih";
>> > + ? ?};
>> > +
>> > +- this node defines a device with two clock outputs, the first named
>> > + ?"ckil" and the second named "ckih". ?Consumer nodes always reference
>> > + ?clocks by index. The names should reflect the clock output signal
>> > + ?names for the device.
>> > +
>> > +==Clock consumers==
>> > +
>> > +Required properties:
>> > +clock-input: ? ? ? List of phandle and clock specifier pairs, one pair
>> > + ? ? ? ? ? for each clock input to the device.
>> > +clock-input-name: List of clock input name strings sorted in the same
>> > + ? ? ? ? ? order as the clock-input property. ?Consumers drivers
>> > + ? ? ? ? ? will use clock-input-name to match clock input names
>> > + ? ? ? ? ? with clock-input specifiers.
>> > +
>> > +For example:
>> > +
>> > + ? ?uart {
>> > + ? ? ? ?clock-input = <&osc 1> <&ref 0>;
>> > + ? ? ? ?clock-input-name = "baud", "register";
>> > + ? ?};
>>
>> This is duplicated below.

I'm okay with that. It's not entirely the same, and I think having
several examples is helpful.


>> > + ? ?/* external oscillator */
>> > + ? ?osc: oscillator {
>> > + ? ? ? ?compatible = "fixed-clock";
>> > + ? ? ? ?#clock-cells = <1>;
>> > + ? ? ? ?frequency ?= <32678>;
>>
>> The code is using "clock-frequency" here.

Fixed

>>
>> > + ? ? ? ?clock-output-name = "osc";
>> > + ? ?};
>> > +
>> > + ? ?/* phase-locked-loop device, generates a higher frequency clock
>> > + ? ? * from the external oscillator reference */
>> > + ? ?pll: pll {
>> > + ? ? ? ?compatible = "some-pll-interface"
>> > + ? ? ? ?#clock-cells = <1>;
>> > + ? ? ? ?clock-input = <&osc 0>;
>>
>> There's a mismatch in #clock-cells size and this.
>
> osc #clock-cells is 1. "0" is one cell too. Nothing wrong. Correct my understanding?

Richard is correct.  The #clock-cells value needs to be 1, which in
the clock binding means one cell for the phandle and one cell for the
attached arguments.  Exactly the same as the gpio binding.

>> > +static struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
>> > +{
>> > + ? struct of_clk_provider *provider;
>> > + ? struct clk *clk = NULL;
>> > +
>> > + ? /* Check if we have such a provider in our array */
>> > + ? mutex_lock(&of_clk_lock);
>> > + ? list_for_each_entry(provider, &of_clk_providers, link) {
>> > + ? ? ? ? ? if (provider->node == clkspec->np)
>> > + ? ? ? ? ? ? ? ? ? clk = provider->get(clkspec, provider->data);
>>
>> How about:
>>
>> if (provider->get)
>> ? ? ? clk = provider->get(clkspec, provider->data);
>> else
>> ? ? ? clk = provider->data;

I'd rather make it a requirement for every clock driver to provide the
get hook; even if it uses a stock version that simply returns ->data.

Thanks for the review.

g.

[RFC 6/8] of: add clock providers

[Sascha Hauer]

On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> of_clk_get function to allow platforms to retrieve clock data from the
> device tree.
> 
> Platform register a provider through of_clk_add_provider, which will be
> called when a device references the provider's OF node for a clock
> reference.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |  109 +++++++++++++++++
>  drivers/of/Kconfig                                 |    6 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/clock.c                                 |  129 ++++++++++++++++++++
>  include/linux/of_clk.h                             |   37 ++++++
>  5 files changed, 282 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
>  create mode 100644 drivers/of/clock.c
>  create mode 100644 include/linux/of_clk.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> new file mode 100644
> index 0000000..4770c7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -0,0 +1,109 @@
> +This binding is a work-in-progress, and are based on some experimental
> +work by benh[1].
> +
> +Sources of clock signal can be represented by any node in the device
> +tree.  Those nodes are designated as clock providers.  Clock consumer
> +nodes use a phandle and clock specifier pair to connect clock provider
> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> +specifier is an array of one more more cells identifying the clock
> +output on a device.  The length of a clock specifier is defined by the
> +value of a #clock-cells property in the clock provider node.
> +
> +[1] http://patchwork.ozlabs.org/patch/31551/
> +
> +==Clock providers==
> +
> +Required properties:
> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> +		   set to 1
> +
> +Optional properties:
> +clock-output-name: Recommended to be a list of strings of clock output signal
> +		   names indexed by the first cell in the clock specifier.
> +		   However, the meaning of clock-output-names is domain
> +		   specific to the clock provider, and is only provided to
> +		   encourage using the same meaning for the majority of clock
> +		   providers.  This format may not work for clock providers
> +		   using a complex clock specifier format.  In those cases it
> +		   is recommended to omit this property and create a binding
> +		   specific names property.

If the clock-output-name property is omitted, does this mean a clock
provider only has a single output or does it mean that it's not known
how many clock outputs a provider actually has?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 6/8] of: add clock providers

[Cousson, Benoit]

On 11/9/2011 10:13 AM, Sascha Hauer wrote:
> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:

[...]

>> +Sources of clock signal can be represented by any node in the device
>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>> +nodes use a phandle and clock specifier pair to connect clock provider
>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>> +specifier is an array of one more more cells identifying the clock
>> +output on a device.  The length of a clock specifier is defined by the
>> +value of a #clock-cells property in the clock provider node.
>> +
>> +[1] http://patchwork.ozlabs.org/patch/31551/
>> +
>> +==Clock providers==
>> +
>> +Required properties:
>> +#clock-cells:	   Number of cells in a clock specifier; typically will be
>> +		   set to 1
>> +
>> +Optional properties:
>> +clock-output-name: Recommended to be a list of strings of clock output signal
>> +		   names indexed by the first cell in the clock specifier.
>> +		   However, the meaning of clock-output-names is domain
>> +		   specific to the clock provider, and is only provided to
>> +		   encourage using the same meaning for the majority of clock
>> +		   providers.  This format may not work for clock providers
>> +		   using a complex clock specifier format.  In those cases it
>> +		   is recommended to omit this property and create a binding
>> +		   specific names property.
>
> If the clock-output-name property is omitted, does this mean a clock
> provider only has a single output or does it mean that it's not known
> how many clock outputs a provider actually has?

Allowing several outputs for a single clock node might lead to a lot of 
confusion. What will be the meaning of a clock rate if you have several 
outputs at different frequency?
I think it will be better to define a clock node as a single source of 
clock. If several outputs are needed, then we should define several 
clock nodes.
If we let a clock node be any kind of big clock blob, we will never be 
able to define some generic reusable clock node API. Everybody will 
define its own custom clock blobs.

Regards,
Benoit

[RFC 6/8] of: add clock providers

[Sascha Hauer]

On Wed, Nov 09, 2011 at 12:23:25PM +0100, Cousson, Benoit wrote:
> On 11/9/2011 10:13 AM, Sascha Hauer wrote:
> >On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> 
> [...]
> 
> >>+Sources of clock signal can be represented by any node in the device
> >>+tree.  Those nodes are designated as clock providers.  Clock consumer
> >>+nodes use a phandle and clock specifier pair to connect clock provider
> >>+outputs to clock inputs.  Similar to the gpio specifiers, a clock
> >>+specifier is an array of one more more cells identifying the clock
> >>+output on a device.  The length of a clock specifier is defined by the
> >>+value of a #clock-cells property in the clock provider node.
> >>+
> >>+[1] http://patchwork.ozlabs.org/patch/31551/
> >>+
> >>+==Clock providers==
> >>+
> >>+Required properties:
> >>+#clock-cells:	   Number of cells in a clock specifier; typically will be
> >>+		   set to 1
> >>+
> >>+Optional properties:
> >>+clock-output-name: Recommended to be a list of strings of clock output signal
> >>+		   names indexed by the first cell in the clock specifier.
> >>+		   However, the meaning of clock-output-names is domain
> >>+		   specific to the clock provider, and is only provided to
> >>+		   encourage using the same meaning for the majority of clock
> >>+		   providers.  This format may not work for clock providers
> >>+		   using a complex clock specifier format.  In those cases it
> >>+		   is recommended to omit this property and create a binding
> >>+		   specific names property.
> >
> >If the clock-output-name property is omitted, does this mean a clock
> >provider only has a single output or does it mean that it's not known
> >how many clock outputs a provider actually has?
> 
> Allowing several outputs for a single clock node might lead to a lot
> of confusion. What will be the meaning of a clock rate if you have
> several outputs at different frequency?
> I think it will be better to define a clock node as a single source
> of clock. If several outputs are needed, then we should define
> several clock nodes.
> If we let a clock node be any kind of big clock blob, we will never
> be able to define some generic reusable clock node API. Everybody
> will define its own custom clock blobs.

Generally +1. I asked myself the same question whether it's a good thing
to allow a node to have multiple clocks. For i.MX I can say that I don't
need this and that I do not intend to use this feature. Grants concerns
about this are that the clock part of the device tree might explode when
we put each and every clock into the devicetree, so he wants to allow
bigger blobs of multiple clocks.

My impression gets more and more that we either put the clock tree in the
devicetree or we do not do it, but there's not much room in between.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 6/8] of: add clock providers

[Cousson, Benoit]

On 11/9/2011 12:49 PM, Sascha Hauer wrote:
> On Wed, Nov 09, 2011 at 12:23:25PM +0100, Cousson, Benoit wrote:
>> On 11/9/2011 10:13 AM, Sascha Hauer wrote:
>>> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
>>
>> [...]
>>
>>>> +Sources of clock signal can be represented by any node in the device
>>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>>> +specifier is an array of one more more cells identifying the clock
>>>> +output on a device.  The length of a clock specifier is defined by the
>>>> +value of a #clock-cells property in the clock provider node.
>>>> +
>>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>>> +
>>>> +==Clock providers==
>>>> +
>>>> +Required properties:
>>>> +#clock-cells:	   Number of cells in a clock specifier; typically will be
>>>> +		   set to 1
>>>> +
>>>> +Optional properties:
>>>> +clock-output-name: Recommended to be a list of strings of clock output signal
>>>> +		   names indexed by the first cell in the clock specifier.
>>>> +		   However, the meaning of clock-output-names is domain
>>>> +		   specific to the clock provider, and is only provided to
>>>> +		   encourage using the same meaning for the majority of clock
>>>> +		   providers.  This format may not work for clock providers
>>>> +		   using a complex clock specifier format.  In those cases it
>>>> +		   is recommended to omit this property and create a binding
>>>> +		   specific names property.
>>>
>>> If the clock-output-name property is omitted, does this mean a clock
>>> provider only has a single output or does it mean that it's not known
>>> how many clock outputs a provider actually has?
>>
>> Allowing several outputs for a single clock node might lead to a lot
>> of confusion. What will be the meaning of a clock rate if you have
>> several outputs at different frequency?
>> I think it will be better to define a clock node as a single source
>> of clock. If several outputs are needed, then we should define
>> several clock nodes.
>> If we let a clock node be any kind of big clock blob, we will never
>> be able to define some generic reusable clock node API. Everybody
>> will define its own custom clock blobs.
>
> Generally +1. I asked myself the same question whether it's a good thing
> to allow a node to have multiple clocks. For i.MX I can say that I don't
> need this and that I do not intend to use this feature. Grants concerns
> about this are that the clock part of the device tree might explode when
> we put each and every clock into the devicetree, so he wants to allow
> bigger blobs of multiple clocks.

I can clearly understand this concern, because in the case of OMAP4, we 
will have to add ~250 nodes if we want to define the current clock tree.

> My impression gets more and more that we either put the clock tree in the
> devicetree or we do not do it, but there's not much room in between.

If we are not ready to accept 4k lines of clock data inside the 
devicetree, which is indeed huge and mostly useless, then I think we 
should just expose the leaf clocks. But then they will depend on parent 
that will not be in DT but in static files, which might be weird.

I'm not sure what the right answer is :-(

Regards,
Benoit

[RFC 6/8] of: add clock providers

[Grant Likely]

On Fri, Nov 11, 2011 at 08:57:59PM +0100, Cousson, Benoit wrote:
> On 11/9/2011 12:49 PM, Sascha Hauer wrote:
> >On Wed, Nov 09, 2011 at 12:23:25PM +0100, Cousson, Benoit wrote:
> >>On 11/9/2011 10:13 AM, Sascha Hauer wrote:
> >>>On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> >>
> >>[...]
> >>
> >>>>+Sources of clock signal can be represented by any node in the device
> >>>>+tree.  Those nodes are designated as clock providers.  Clock consumer
> >>>>+nodes use a phandle and clock specifier pair to connect clock provider
> >>>>+outputs to clock inputs.  Similar to the gpio specifiers, a clock
> >>>>+specifier is an array of one more more cells identifying the clock
> >>>>+output on a device.  The length of a clock specifier is defined by the
> >>>>+value of a #clock-cells property in the clock provider node.
> >>>>+
> >>>>+[1] http://patchwork.ozlabs.org/patch/31551/
> >>>>+
> >>>>+==Clock providers==
> >>>>+
> >>>>+Required properties:
> >>>>+#clock-cells:	   Number of cells in a clock specifier; typically will be
> >>>>+		   set to 1
> >>>>+
> >>>>+Optional properties:
> >>>>+clock-output-name: Recommended to be a list of strings of clock output signal
> >>>>+		   names indexed by the first cell in the clock specifier.
> >>>>+		   However, the meaning of clock-output-names is domain
> >>>>+		   specific to the clock provider, and is only provided to
> >>>>+		   encourage using the same meaning for the majority of clock
> >>>>+		   providers.  This format may not work for clock providers
> >>>>+		   using a complex clock specifier format.  In those cases it
> >>>>+		   is recommended to omit this property and create a binding
> >>>>+		   specific names property.
> >>>
> >>>If the clock-output-name property is omitted, does this mean a clock
> >>>provider only has a single output or does it mean that it's not known
> >>>how many clock outputs a provider actually has?

As Rob said, it only means that there are no names assigned to those
inputs.  In fact, it leaves it up to the specific clock binding as to
what it really means.  One driver may require the clock-output-name
property to be present because it uses data from there to figure out
how to configure the clock.  Another might not require it at all and
the only reason for its existence is informational.

> >>Allowing several outputs for a single clock node might lead to a lot
> >>of confusion. What will be the meaning of a clock rate if you have
> >>several outputs at different frequency?

No, clock-frequency is specific to the fixed-clock binding (which I
need to document).  That may or may not be present in other clock
provider nodes.  For instance, there is a xilinx clock divider ip
block that doesn't have any frequency of its own, but rather has
multiple clock outputs that are a scale of the input clock.  The node
for that device would never have a clock-frequency property because it
relies entirely on the upstream clock.

This proposed binding is only about one thing: attaching clock
providers to clock consumers.  It doesn't make any statements about
how the clock provider driver is organized or how it manages its
clocks.

This is by design to prevent clock consumers from digging into a clock
provider node manually without consulting its driver.  The consumer
only has a reference to a clock provider node + arguments.  It is the
job of the producer driver to interpret the arguments.

The proposed API also reflects this.  The consumer driver requests a
clock, but it is the producer driver that determines what the clock
reference actually means, and which struct clk to return.

> >>I think it will be better to define a clock node as a single source
> >>of clock. If several outputs are needed, then we should define
> >>several clock nodes.

That is entirely up to the binding for the specific clock device(s).
If it makes sense to have one node per clock, so be it.  If it makes
more sense to have a clock nexus node for other hardware, then that is
fine too.

> >>If we let a clock node be any kind of big clock blob, we will never
> >>be able to define some generic reusable clock node API. Everybody
> >>will define its own custom clock blobs.
> >
> >Generally +1. I asked myself the same question whether it's a good thing
> >to allow a node to have multiple clocks. For i.MX I can say that I don't
> >need this and that I do not intend to use this feature. Grants concerns
> >about this are that the clock part of the device tree might explode when
> >we put each and every clock into the devicetree, so he wants to allow
> >bigger blobs of multiple clocks.
> 
> I can clearly understand this concern, because in the case of OMAP4,
> we will have to add ~250 nodes if we want to define the current
> clock tree.

I'm really not convinced that it is a good idea to break out the
entire clock tree into one node per struct clk.  To begin with, that
looks to be very centric around the current 'struct clk' Linux
abstraction, which is potentially in flux.  Also, looking at Sascha's
initial RFC for describing the clock tree, I see cases where it looks
like a clock nexus node really makes sense.  For instance, the
'divider-ipg at 0x53fd4014' node which has a list of child nodes
which merely provide a register offset and shift value
(reg=0x53fd4068..0x53fd4084, shift=0x0..0xf).  It would be natural to
instead encode that as part of the clock reference, or map it directly
from the clock reference (ie, assign names to each of the clocks, and
let the clock provider driver match up the name to the reg offset &
shift values).

I had originally thought that it would be better to use names directly
for references to clocks (ie. clock = <phandle>,"name") , but after
actually playing with it and looking at the existing DT conventions,
I've reverted to cell values for the arguments and a separate set of
clock-{input,output}-name properties for attaching meaningful names,
just like we decided to do for assigning names to 'reg' properties.

> >My impression gets more and more that we either put the clock tree in the
> >devicetree or we do not do it, but there's not much room in between.

I disagree here entirely because there is a lot of room in between.
Drivers still need to understand the hardware block they are driving,
and there will still be a lot of hardware-specific knowledge encoded
into clock provider drivers (just as there is for every other kind of
driver).  The trick (and even artistry) is to determine the ideal
balance between hard coding static hardware details and
parameterization of interconnects between blocks.

> 
> If we are not ready to accept 4k lines of clock data inside the
> devicetree, which is indeed huge and mostly useless, then I think we
> should just expose the leaf clocks. But then they will depend on
> parent that will not be in DT but in static files, which might be
> weird.
> 
> I'm not sure what the right answer is :-(

I think you're right.  Expose the leaf clocks, and maybe the
interconnects between clock providers.  There is no need to expose
every single intermediary clock, especially when those clocks are
merely internal to a device IP block and are never exposed to
anything outside of itself.

I'm even okay with having a single clock driver for an entire SoC if
there isn't a very high likelyhood of any of it's internal clock
components will ever get reused somewhere else.  When compared with
board designs, internal SoC clock structure really doesn't change much
at all so there isn't the same need for breaking all the detail out to
the device tree.

Oh, and it shouldn't be weird because even clock nexus nodes that
handle several clock outputs can have clock inputs from other sources.
For instance, there could be a node for the clock provided by the
on-board crystal.

g.

[RFC 6/8] of: add clock providers

[Sascha Hauer]

On Wed, Nov 16, 2011 at 03:12:50PM -0700, Grant Likely wrote:
> On Fri, Nov 11, 2011 at 08:57:59PM +0100, Cousson, Benoit wrote:
> > On 11/9/2011 12:49 PM, Sascha Hauer wrote:
> > >On Wed, Nov 09, 2011 at 12:23:25PM +0100, Cousson, Benoit wrote:
> > >>On 11/9/2011 10:13 AM, Sascha Hauer wrote:
> > >>>On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> > >>
> > >>[...]
> > >>
> > >>>>+Sources of clock signal can be represented by any node in the device
> > >>>>+tree.  Those nodes are designated as clock providers.  Clock consumer
> > >>>>+nodes use a phandle and clock specifier pair to connect clock provider
> > >>>>+outputs to clock inputs.  Similar to the gpio specifiers, a clock
> > >>>>+specifier is an array of one more more cells identifying the clock
> > >>>>+output on a device.  The length of a clock specifier is defined by the
> > >>>>+value of a #clock-cells property in the clock provider node.
> > >>>>+
> > >>>>+[1] http://patchwork.ozlabs.org/patch/31551/
> > >>>>+
> > >>>>+==Clock providers==
> > >>>>+
> > >>>>+Required properties:
> > >>>>+#clock-cells:	   Number of cells in a clock specifier; typically will be
> > >>>>+		   set to 1
> > >>>>+
> > >>>>+Optional properties:
> > >>>>+clock-output-name: Recommended to be a list of strings of clock output signal
> > >>>>+		   names indexed by the first cell in the clock specifier.
> > >>>>+		   However, the meaning of clock-output-names is domain
> > >>>>+		   specific to the clock provider, and is only provided to
> > >>>>+		   encourage using the same meaning for the majority of clock
> > >>>>+		   providers.  This format may not work for clock providers
> > >>>>+		   using a complex clock specifier format.  In those cases it
> > >>>>+		   is recommended to omit this property and create a binding
> > >>>>+		   specific names property.
> > >>>
> > >>>If the clock-output-name property is omitted, does this mean a clock
> > >>>provider only has a single output or does it mean that it's not known
> > >>>how many clock outputs a provider actually has?
> 
> As Rob said, it only means that there are no names assigned to those
> inputs.  In fact, it leaves it up to the specific clock binding as to
> what it really means.  One driver may require the clock-output-name
> property to be present because it uses data from there to figure out
> how to configure the clock.  Another might not require it at all and
> the only reason for its existence is informational.
> 
> > >>Allowing several outputs for a single clock node might lead to a lot
> > >>of confusion. What will be the meaning of a clock rate if you have
> > >>several outputs at different frequency?
> 
> No, clock-frequency is specific to the fixed-clock binding (which I
> need to document).  That may or may not be present in other clock
> provider nodes.  For instance, there is a xilinx clock divider ip
> block that doesn't have any frequency of its own, but rather has
> multiple clock outputs that are a scale of the input clock.  The node
> for that device would never have a clock-frequency property because it
> relies entirely on the upstream clock.
> 
> This proposed binding is only about one thing: attaching clock
> providers to clock consumers.  It doesn't make any statements about
> how the clock provider driver is organized or how it manages its
> clocks.
> 
> This is by design to prevent clock consumers from digging into a clock
> provider node manually without consulting its driver.  The consumer
> only has a reference to a clock provider node + arguments.  It is the
> job of the producer driver to interpret the arguments.
> 
> The proposed API also reflects this.  The consumer driver requests a
> clock, but it is the producer driver that determines what the clock
> reference actually means, and which struct clk to return.
> 
> > >>I think it will be better to define a clock node as a single source
> > >>of clock. If several outputs are needed, then we should define
> > >>several clock nodes.
> 
> That is entirely up to the binding for the specific clock device(s).
> If it makes sense to have one node per clock, so be it.  If it makes
> more sense to have a clock nexus node for other hardware, then that is
> fine too.
> 
> > >>If we let a clock node be any kind of big clock blob, we will never
> > >>be able to define some generic reusable clock node API. Everybody
> > >>will define its own custom clock blobs.
> > >
> > >Generally +1. I asked myself the same question whether it's a good thing
> > >to allow a node to have multiple clocks. For i.MX I can say that I don't
> > >need this and that I do not intend to use this feature. Grants concerns
> > >about this are that the clock part of the device tree might explode when
> > >we put each and every clock into the devicetree, so he wants to allow
> > >bigger blobs of multiple clocks.
> > 
> > I can clearly understand this concern, because in the case of OMAP4,
> > we will have to add ~250 nodes if we want to define the current
> > clock tree.
> 
> I'm really not convinced that it is a good idea to break out the
> entire clock tree into one node per struct clk.  To begin with, that
> looks to be very centric around the current 'struct clk' Linux
> abstraction, which is potentially in flux.  Also, looking at Sascha's
> initial RFC for describing the clock tree, I see cases where it looks
> like a clock nexus node really makes sense.  For instance, the
> 'divider-ipg at 0x53fd4014' node which has a list of child nodes
> which merely provide a register offset and shift value
> (reg=0x53fd4068..0x53fd4084, shift=0x0..0xf).

Yes, these are 6 (or 7) registers which contains all the gates. But not
all of these gates are children of the ipg clock, some are derived from
other clocks. So describing this as a nexus clock would give you 6 nexus
clocks, each having 16 parents and 16 children (we have 2 bits per gate).
Many of these clock gates go to external devices, but others are also
used internally by the clock module.
With the clock gates you picked the only example in the i.MX clock
module which has a straight forward register layout. All other muxes
and dividers are spread randomly in the registers.

> It would be natural to
> instead encode that as part of the clock reference, or map it directly
> from the clock reference (ie, assign names to each of the clocks, and
> let the clock provider driver match up the name to the reg offset &
> shift values).
> 
> I had originally thought that it would be better to use names directly
> for references to clocks (ie. clock = <phandle>,"name") , but after
> actually playing with it and looking at the existing DT conventions,
> I've reverted to cell values for the arguments and a separate set of
> clock-{input,output}-name properties for attaching meaningful names,
> just like we decided to do for assigning names to 'reg' properties.
> 
> > >My impression gets more and more that we either put the clock tree in the
> > >devicetree or we do not do it, but there's not much room in between.
> 
> I disagree here entirely because there is a lot of room in between.
> Drivers still need to understand the hardware block they are driving,
> and there will still be a lot of hardware-specific knowledge encoded
> into clock provider drivers (just as there is for every other kind of
> driver).  The trick (and even artistry) is to determine the ideal
> balance between hard coding static hardware details and
> parameterization of interconnects between blocks.

That's part of the problem. When in copy-a-new-clktree-from-datasheet-to-c
mode I don't want to be an artist at all. Instead I want to be a trained
monkey. Too many people have tried to be intelligent when hacking the
clock files which explains the mess we currently have.

Ok, as you know my original goal was only to describe the building
blocks as what they are and not arbitrarily grouped together. Originally
I did not even have the device tree in mind. I am perfectly fine with
describing the clock module as a single blob with few input clocks
and many many output clocks. Then we can internally (in C code) still
describe the building blocks.

One thing I like about the full description in the device tree is
that boards would be able to adjust divider/mux settings directly in the
device tree, but we can do without.

One thing though needs a better solution I think. What if a clock module
has 100 outputs? An UART might look like this then:

uart {
	clock-input = <&ccm 23> <&ccm 97>;
	clock-input-name = "baud", "register";
};

This would be hard to verify for correctness because of the high
numbers. Also, when a maintainer gets two patches which both add
a forgotten clock to the device tree he can't just merge them but
instead has to adjust the numbers in one patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 6/8] of: add clock providers

[Rob Herring]

On 11/09/2011 05:23 AM, Cousson, Benoit wrote:
> On 11/9/2011 10:13 AM, Sascha Hauer wrote:
>> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> 
> [...]
> 
>>> +Sources of clock signal can be represented by any node in the device
>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>> +specifier is an array of one more more cells identifying the clock
>>> +output on a device.  The length of a clock specifier is defined by the
>>> +value of a #clock-cells property in the clock provider node.
>>> +
>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>> +
>>> +==Clock providers==
>>> +
>>> +Required properties:
>>> +#clock-cells:       Number of cells in a clock specifier; typically
>>> will be
>>> +           set to 1
>>> +
>>> +Optional properties:
>>> +clock-output-name: Recommended to be a list of strings of clock
>>> output signal
>>> +           names indexed by the first cell in the clock specifier.
>>> +           However, the meaning of clock-output-names is domain
>>> +           specific to the clock provider, and is only provided to
>>> +           encourage using the same meaning for the majority of clock
>>> +           providers.  This format may not work for clock providers
>>> +           using a complex clock specifier format.  In those cases it
>>> +           is recommended to omit this property and create a binding
>>> +           specific names property.
>>
>> If the clock-output-name property is omitted, does this mean a clock
>> provider only has a single output or does it mean that it's not known
>> how many clock outputs a provider actually has?
> 
> Allowing several outputs for a single clock node might lead to a lot of
> confusion. What will be the meaning of a clock rate if you have several
> outputs at different frequency?

You typically only have a frequency property for fixed clocks.

However, we should think about how to set frequency for programmable
clocks. Perhaps this is just making clock-frequency an array of
values in the same order as the outputs.

> I think it will be better to define a clock node as a single source of
> clock. If several outputs are needed, then we should define several
> clock nodes.
> If we let a clock node be any kind of big clock blob, we will never be
> able to define some generic reusable clock node API. Everybody will
> define its own custom clock blobs.
> 

Whether you decide to implement a blob or every single mux, divider, and
gate is independent from whether you use generic clock code or not. You
could simply define a clock controller node with lots of outputs yet
still use generic code to implement clock support in Linux. The reality
is you will probably have a mixture of generic and SOC-specific clocks.

What is our goal here? I'm skeptical we will ever get to the point that
we can fully describe the clock tree for a new SOC without any code
changes. Perhaps our goal is simply that clock differences across all
boards for an SOC can be described in DT.

Rob

[RFC 6/8] of: add clock providers

[Cousson, Benoit]

Hi Rob,

On 11/9/2011 2:59 PM, Rob Herring wrote:
> On 11/09/2011 05:23 AM, Cousson, Benoit wrote:
>> On 11/9/2011 10:13 AM, Sascha Hauer wrote:
>>> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
>>
>> [...]
>>
>>>> +Sources of clock signal can be represented by any node in the device
>>>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>>>> +nodes use a phandle and clock specifier pair to connect clock provider
>>>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>>>> +specifier is an array of one more more cells identifying the clock
>>>> +output on a device.  The length of a clock specifier is defined by the
>>>> +value of a #clock-cells property in the clock provider node.
>>>> +
>>>> +[1] http://patchwork.ozlabs.org/patch/31551/
>>>> +
>>>> +==Clock providers==
>>>> +
>>>> +Required properties:
>>>> +#clock-cells:       Number of cells in a clock specifier; typically
>>>> will be
>>>> +           set to 1
>>>> +
>>>> +Optional properties:
>>>> +clock-output-name: Recommended to be a list of strings of clock
>>>> output signal
>>>> +           names indexed by the first cell in the clock specifier.
>>>> +           However, the meaning of clock-output-names is domain
>>>> +           specific to the clock provider, and is only provided to
>>>> +           encourage using the same meaning for the majority of clock
>>>> +           providers.  This format may not work for clock providers
>>>> +           using a complex clock specifier format.  In those cases it
>>>> +           is recommended to omit this property and create a binding
>>>> +           specific names property.
>>>
>>> If the clock-output-name property is omitted, does this mean a clock
>>> provider only has a single output or does it mean that it's not known
>>> how many clock outputs a provider actually has?
>>
>> Allowing several outputs for a single clock node might lead to a lot of
>> confusion. What will be the meaning of a clock rate if you have several
>> outputs at different frequency?
>
> You typically only have a frequency property for fixed clocks.

Yes, for DT point of view, but every clock nodes has a rate. Variable or 
fixed.

> However, we should think about how to set frequency for programmable
> clocks. Perhaps this is just making clock-frequency an array of
> values in the same order as the outputs.

But, if we go that way the DT clock node will become more and more 
complex with less and less chance to use some common function like 
set_rate or clk_enable.
If the notion of clock blob is needed we should maybe consider another 
binding like clock-group and let the clock node be a single source of clock.

>> I think it will be better to define a clock node as a single source of
>> clock. If several outputs are needed, then we should define several
>> clock nodes.
>> If we let a clock node be any kind of big clock blob, we will never be
>> able to define some generic reusable clock node API. Everybody will
>> define its own custom clock blobs.
>>
>
> Whether you decide to implement a blob or every single mux, divider, and
> gate is independent from whether you use generic clock code or not. You
> could simply define a clock controller node with lots of outputs yet
> still use generic code to implement clock support in Linux. The reality
> is you will probably have a mixture of generic and SOC-specific clocks.

Yeah, maybe you're right, but the goal of the common clock fmwk was to 
avoid duplication everywhere. If everybody start implementing its own 
definition of the clock nodes here and there, I'm not sure that common 
clock fmwk will be that useful compared to what we have today.

For sure we should allow some custom clock implementation, but that 
should be the exception.

> What is our goal here? I'm skeptical we will ever get to the point that
> we can fully describe the clock tree for a new SOC without any code
> changes.

I don't think anybody is necessarily complaining about potential code 
change.

The goal, however, is a pretty good question. I'm not sure it is well 
defined or at least the same for everybody.

One of the main incentive for OMAP at least was to get rid of the huge 
static files inside arch/arm/mach-omap2 used so far to define the clock 
nodes.
The issue is that an OMAP4 clock file contains ~250 clock nodes in a ~4k 
lines file.
Moving that into a DT file will for sure reduce the size of the 
arch/arm/ that was one of the complain of Linus but at the price of 
creating an even bigger DT file.

One potential need for DT will be to get rid of the static device -> 
clock mapping table we have at the end of the clock definition file.

   CLK("usbhs-omap.0", "usbtll_ick", &usb_tll_hs_ick, CK_443X),
   CLK("usbhs-omap.0", "fck", &usb_tll_hs_fck, CK_443X),

This is even mandatory considering that the device name currently used 
will not exist anymore with DT devices.

That means, that at least every leaf clocks used by drivers have to be 
exposed in DT.

Should we then expose as well the whole clock tree?

> Perhaps our goal is simply that clock differences across all
> boards for an SOC can be described in DT.

We clearly should exposed the ones that are configurable at board level, 
like the main oscillator for example or the ones that are generated by 
an Audio codec.

But then, like Sascha was saying IIRC, having a mixed of DT and static 
data might be a total mess at the end, so we might end up defining 
everything in the DT file.

Regards,
Benoit

[RFC 6/8] of: add clock providers

[Rob Herring]

On 11/09/2011 03:13 AM, Sascha Hauer wrote:
> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
>> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
>> of_clk_get function to allow platforms to retrieve clock data from the
>> device tree.
>>
>> Platform register a provider through of_clk_add_provider, which will be
>> called when a device references the provider's OF node for a clock
>> reference.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>> ---
>>  .../devicetree/bindings/clock/clock-bindings.txt   |  109 +++++++++++++++++
>>  drivers/of/Kconfig                                 |    6 +
>>  drivers/of/Makefile                                |    1 +
>>  drivers/of/clock.c                                 |  129 ++++++++++++++++++++
>>  include/linux/of_clk.h                             |   37 ++++++
>>  5 files changed, 282 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
>>  create mode 100644 drivers/of/clock.c
>>  create mode 100644 include/linux/of_clk.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> new file mode 100644
>> index 0000000..4770c7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -0,0 +1,109 @@
>> +This binding is a work-in-progress, and are based on some experimental
>> +work by benh[1].
>> +
>> +Sources of clock signal can be represented by any node in the device
>> +tree.  Those nodes are designated as clock providers.  Clock consumer
>> +nodes use a phandle and clock specifier pair to connect clock provider
>> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
>> +specifier is an array of one more more cells identifying the clock
>> +output on a device.  The length of a clock specifier is defined by the
>> +value of a #clock-cells property in the clock provider node.
>> +
>> +[1] http://patchwork.ozlabs.org/patch/31551/
>> +
>> +==Clock providers==
>> +
>> +Required properties:
>> +#clock-cells:	   Number of cells in a clock specifier; typically will be
>> +		   set to 1
>> +
>> +Optional properties:
>> +clock-output-name: Recommended to be a list of strings of clock output signal
>> +		   names indexed by the first cell in the clock specifier.
>> +		   However, the meaning of clock-output-names is domain
>> +		   specific to the clock provider, and is only provided to
>> +		   encourage using the same meaning for the majority of clock
>> +		   providers.  This format may not work for clock providers
>> +		   using a complex clock specifier format.  In those cases it
>> +		   is recommended to omit this property and create a binding
>> +		   specific names property.
> 
> If the clock-output-name property is omitted, does this mean a clock
> provider only has a single output or does it mean that it's not known
> how many clock outputs a provider actually has?

Neither. It's similar to how interrupt bindings work. For a clock
consumer, you have a phandle to the parent and some number of cells to
describe the connection. Often this is just the index or local interrupt
number. Obviously, interrupt numbers make more sense than clock numbers,
so having names is more useful in the clocks case if you have many outputs.

The name strings are just additional information. Keep in mind that the
clock node name can also be used if you need to attach a name to a clock.

Rob

[RFC 6/8] of: add clock providers

[Tony Lindgren]

* Grant Likely <grant.likely@secretlab.ca> [111108 16:51]:
> +
> +    /* external oscillator */
> +    osc: oscillator {
> +        compatible = "fixed-clock";
> +        #clock-cells = <1>;
> +        frequency  = <32678>;
> +        clock-output-name = "osc";
> +    };
> +
> +    /* phase-locked-loop device, generates a higher frequency clock
> +     * from the external oscillator reference */
> +    pll: pll {
> +        compatible = "some-pll-interface"
> +        #clock-cells = <1>;
> +        clock-input = <&osc 0>;
> +        clock-input-name = "ref";
> +        reg = <0x4c000 0x1000>;
> +        clock-output-name = "pll", "pll-switched";
> +    };

I think for the clocks we need something describing which bits
in the clock register the "pll" and "pll-switched" belong to.

Otherwise we'll end up having to map all that data in the clock
driver for various SoC variants.

> +    /* UART, using the low frequency oscillator for the baud clock,
> +     * and the high frequency switched PLL output for register
> +     * clocking */
> +    uart {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +        interrupts = <33>;
> +        clock-input = <&osc 0>, <&pll 1>;
> +        clock-input-name = "baud", "register";
> +    };
> +

Here I think we need a better solution for finding the
clocks by name. Doing clock-input = <&osc 0>, <&pll 1> means
that we have redefine the uart for similar SoC many times.

How about using source + option in the name instead:

	   clock-source-name = "osc.osc", "pll.pll-switched";

This same solution would also work for the pinctrl names:

	   pinctrl-source-name = "uart1_rx.uart1_rx", "dss_data7.uart1_tx";

The reason I'm worried here is that just for omap2/3/4 we already
have 3 options for uart1 clock and 19 options for uart1_rx pin!

Using the signal names for both clocks and pinctrl would
allow defining the uart entry for for each reg address instead
of for each clock and pinctrl option.

Regards,

Tony

[RFC 6/8] of: add clock providers

[Shawn Guo]

On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> Based on work by Ben Herrenschmidt and Jeremy Kerr, this patch adds an
> of_clk_get function to allow platforms to retrieve clock data from the
> device tree.
> 
> Platform register a provider through of_clk_add_provider, which will be
> called when a device references the provider's OF node for a clock
> reference.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |  109 +++++++++++++++++
>  drivers/of/Kconfig                                 |    6 +
>  drivers/of/Makefile                                |    1 +
>  drivers/of/clock.c                                 |  129 ++++++++++++++++++++
>  include/linux/of_clk.h                             |   37 ++++++
>  5 files changed, 282 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt
>  create mode 100644 drivers/of/clock.c
>  create mode 100644 include/linux/of_clk.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> new file mode 100644
> index 0000000..4770c7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -0,0 +1,109 @@
> +This binding is a work-in-progress, and are based on some experimental
> +work by benh[1].
> +
> +Sources of clock signal can be represented by any node in the device
> +tree.  Those nodes are designated as clock providers.  Clock consumer
> +nodes use a phandle and clock specifier pair to connect clock provider
> +outputs to clock inputs.  Similar to the gpio specifiers, a clock
> +specifier is an array of one more more cells identifying the clock
> +output on a device.  The length of a clock specifier is defined by the
> +value of a #clock-cells property in the clock provider node.
> +
> +[1] http://patchwork.ozlabs.org/patch/31551/
> +
> +==Clock providers==
> +
> +Required properties:
> +#clock-cells:	   Number of cells in a clock specifier; typically will be
> +		   set to 1
> +
> +Optional properties:
> +clock-output-name: Recommended to be a list of strings of clock output signal
> +		   names indexed by the first cell in the clock specifier.
> +		   However, the meaning of clock-output-names is domain

s/clock-output-names/clock-output-name?

> +		   specific to the clock provider, and is only provided to
> +		   encourage using the same meaning for the majority of clock
> +		   providers.  This format may not work for clock providers
> +		   using a complex clock specifier format.  In those cases it
> +		   is recommended to omit this property and create a binding
> +		   specific names property.
> +
> +		   Clock consumer nodes must never directly reference
> +		   the provider's clock-output-names property.

s/clock-output-names/clock-output-name?

> +
> +For example:
> +
> +    oscillator {
> +        #clock-cells = <1>;
> +        clock-output-name = "ckil", "ckih";
> +    };
> +
> +- this node defines a device with two clock outputs, the first named
> +  "ckil" and the second named "ckih".  Consumer nodes always reference
> +  clocks by index. The names should reflect the clock output signal
> +  names for the device.
> +
> +==Clock consumers==
> +
> +Required properties:
> +clock-input:	List of phandle and clock specifier pairs, one pair
> +		for each clock input to the device.
> +clock-input-name: List of clock input name strings sorted in the same
> +		order as the clock-input property.  Consumers drivers
> +		will use clock-input-name to match clock input names
> +		with clock-input specifiers.
> +
> +For example:
> +
> +    uart {
> +        clock-input = <&osc 1> <&ref 0>;
				 ^
Comma is missed?

> +        clock-input-name = "baud", "register";
> +    };

[...]

> +struct clk *of_clk_get(struct device_node *np, int index)
> +{
> +	struct of_phandle_args clkspec;
> +	struct clk *clk;
> +	int rc;
> +
> +	if (index < 0)
> +		return NULL;
> +
> +	rc = of_parse_phandle_with_args(np, "clock", "#clock-cells", index,

According to binding document above, s/clock/clock-input?

> +					&clkspec);
> +	if (rc)
> +		return NULL;
> +
> +	clk = __of_clk_get_from_provider(&clkspec);
> +	of_node_put(clkspec.np);
> +	return clk;
> +}
> +
> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> +{
> +	int index = 0;
> +
> +	if (name)
> +		index = of_property_match_string(np, "clock-input-names", name);

If it fails to match name, we may want to force 'index' back to 0.

Also according to binding document above,
s/clock-input-names/clock-input-name?  Hope you do not want to
change binding document to get them aligned.  I have written code
per binding document :)

Regards,
Shawn

> +	return of_clk_get(np, index);
> +}

[RFC 6/8] of: add clock providers

[Grant Likely]

On Mon, Nov 21, 2011 at 8:37 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
>> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>> +{
>> + ? ? int index = 0;
>> +
>> + ? ? if (name)
>> + ? ? ? ? ? ? index = of_property_match_string(np, "clock-input-names", name);
>
> If it fails to match name, we may want to force 'index' back to 0.

Actually, if it fails to match, the function should fail.  Fixed.
Fixed all the editorial comments too.  Thanks.

>
> Also according to binding document above,
> s/clock-input-names/clock-input-name? ?Hope you do not want to
> change binding document to get them aligned. ?I have written code
> per binding document :)

I've changed to match the binding documentation.

Thanks for the review,
g.

[RFC 6/8] of: add clock providers

[Shawn Guo]

On Wed, Nov 30, 2011 at 12:38:19PM -0700, Grant Likely wrote:
> On Mon, Nov 21, 2011 at 8:37 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> > On Tue, Nov 08, 2011 at 06:19:41PM -0700, Grant Likely wrote:
> >> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> >> +{
> >> + ? ? int index = 0;
> >> +
> >> + ? ? if (name)
> >> + ? ? ? ? ? ? index = of_property_match_string(np, "clock-input-names", name);
> >
> > If it fails to match name, we may want to force 'index' back to 0.
> 
> Actually, if it fails to match, the function should fail.

Hmm, that will require the clock-input-name be same as the second
parameter of clk_get() - con_id, which may have been named to some
string not suitable for clock-input-name.  For those case, we will
probably need to change the drivers to rename the con_id?  Or just
force the clock-input-name to be that improper con_id string?

-- 
Regards,
Shawn

[RFC 8/8] dt/arm: versatile add clock parsing

[Sascha Hauer]

On Tue, Nov 08, 2011 at 06:19:43PM -0700, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  arch/arm/boot/dts/versatile-ab.dts     |   37 ++++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/versatile-pb.dts     |    2 +
>  arch/arm/mach-versatile/core.c         |   37 +------------------------------
>  arch/arm/mach-versatile/versatile_dt.c |   33 ++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts
> index 0b32925..cb8a49e 100644
> --- a/arch/arm/boot/dts/versatile-ab.dts
> +++ b/arch/arm/boot/dts/versatile-ab.dts
> @@ -19,6 +19,35 @@
>  		reg = <0x0 0x08000000>;
>  	};
>  
> +	clocks {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		ref24_clk: clock0 {
> +			compatible = "fixed-clock";
> +			#clock-cells = <1>;
> +			clock-frequency = <24000000>;
> +			clock-output-names = "ref";
> +		};
> +
> +		sp804_clk: clock1 {
> +			compatible = "fixed-clock";
> +			#clock-cells = <1>;
> +			clock-frequency = <1000000>;
> +			clock-output-names = "ref";
> +		};
> +
> +		osc4_clk: clock3 {
> +			compatible = "ics,icst307";
> +			#clock-cells = <1>;
> +			ref = <24000>;
> +			vco-max = <200000>;
> +			vd-range = <12 519>;
> +			rd-range = <3 129>;
> +			clock-output-names = "osc4";
> +		};

The registers needed to access osc4 are missing here. This probably
means that osc4 cannot be specified in the clocks node but has to
be sorted into the bus structure of the SoC.

> +
>  static void __init versatile_dt_init(void)
>  {
> +	struct device_node *node;
> +	struct clk *clk;
> +	int rc;
> +
> +	for_each_compatible_node(node, NULL, "fixed-clock") {
> +		u32 rate;
> +		if (of_property_read_u32(node, "clock-frequency", &rate))
> +			continue;
> +
> +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +		if (!clk)
> +			panic("out of memory\n");
> +		clk->rate = rate;
> +
> +		rc = of_clk_add_provider(node, versatile_dt_clk_get, clk);
> +		if (rc) {
> +			kfree(clk);
> +			pr_err("error adding fixed clk %s\n", node->name);
> +		}
> +	}
> +

Well this is obviously just a quick hack to get something working. I
just want to add the note here that Mikes current patches require to
register the clock tree top-down. This may be hard to archieve as the
clocks are somewhere in the devicetree (sorted by bus topology and not
by clock topology). So we either need more thinking in the clock
framework to overcome the top-down registration or more thinking here
in the registration of the clocks.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC 3/8] of: create of_phandle_args to simplify return of phandle parsing data

[Shawn Guo]

Hi Grant,

On Tue, Nov 08, 2011 at 06:19:38PM -0700, Grant Likely wrote:
[...]
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 4948552..566deab 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -222,6 +222,13 @@ extern const void *of_get_property(const struct device_node *node,
>  #define for_each_property(pp, properties) \
>  	for (pp = properties; pp != NULL; pp = pp->next)
>  
> +#define MAX_PHANDLE_ARGS 8
> +struct of_phandle_args {
> +	struct device_node *np;
> +	int args_count;
> +	uint32_t args[MAX_PHANDLE_ARGS];
> +};
> +

I'm seeing a bunch of warnings like the one below when compiling imx
with the series.  And moving the definition to somewhere before
'#include <asm/prom.h>' in linux/of.h removes the warnings for me.

  CC      arch/arm/kernel/devtree.o
In file included from arch/arm/plat-mxc/include/mach/irqs.h:14:0,
                 from arch/arm/include/asm/irq.h:4,
                 from arch/arm/include/asm/prom.h:17,
                 from include/linux/of.h:133,
                 from arch/arm/kernel/devtree.c:17:
include/asm-generic/gpio.h:133:24: warning: ?struct of_phandle_args? declared inside parameter list
include/asm-generic/gpio.h:133:24: warning: its scope is only this definition or declaration, which is probably not what you want

-- 
Regards,
Shawn

[RFC 3/8] of: create of_phandle_args to simplify return of phandle parsing data

[Grant Likely]

On Sun, Nov 13, 2011 at 8:59 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Hi Grant,
>
> On Tue, Nov 08, 2011 at 06:19:38PM -0700, Grant Likely wrote:
> [...]
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 4948552..566deab 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -222,6 +222,13 @@ extern const void *of_get_property(const struct device_node *node,
>> ?#define for_each_property(pp, properties) \
>> ? ? ? for (pp = properties; pp != NULL; pp = pp->next)
>>
>> +#define MAX_PHANDLE_ARGS 8
>> +struct of_phandle_args {
>> + ? ? struct device_node *np;
>> + ? ? int args_count;
>> + ? ? uint32_t args[MAX_PHANDLE_ARGS];
>> +};
>> +
>
> I'm seeing a bunch of warnings like the one below when compiling imx
> with the series. ?And moving the definition to somewhere before
> '#include <asm/prom.h>' in linux/of.h removes the warnings for me.
>
> ?CC ? ? ?arch/arm/kernel/devtree.o
> In file included from arch/arm/plat-mxc/include/mach/irqs.h:14:0,
> ? ? ? ? ? ? ? ? from arch/arm/include/asm/irq.h:4,
> ? ? ? ? ? ? ? ? from arch/arm/include/asm/prom.h:17,
> ? ? ? ? ? ? ? ? from include/linux/of.h:133,
> ? ? ? ? ? ? ? ? from arch/arm/kernel/devtree.c:17:
> include/asm-generic/gpio.h:133:24: warning: ?struct of_phandle_args? declared inside parameter list
> include/asm-generic/gpio.h:133:24: warning: its scope is only this definition or declaration, which is probably not what you want

Fixed, thanks.

g.

[RFC 3/8] of: create of_phandle_args to simplify return of phandle parsing data

[Shawn Guo]

On Tue, Nov 08, 2011 at 06:19:38PM -0700, Grant Likely wrote:
[...]
>  /**
> - * of_parse_phandles_with_args - Find a node pointed by phandle in a list
> + * of_parse_phandle_with_args - Find a node pointed by phandle in a list
>   * @np:		pointer to a device tree node containing a list
>   * @list_name:	property name that contains a list
>   * @cells_name:	property name that specifies phandles' arguments count
>   * @index:	index of a phandle to parse out
> - * @out_node:	optional pointer to device_node struct pointer (will be filled)
> - * @out_args:	optional pointer to arguments pointer (will be filled)
> + * @out_args:	optional pointer to output arguments structure (will be filled)
>   *
>   * This function is useful to parse lists of phandles and their arguments.
> - * Returns 0 on success and fills out_node and out_args, on error returns
> - * appropriate errno value.
> + * Returns 0 on success and fills out_args, on error returns appropriate
> + * errno value.
>   *
>   * Example:
>   *
> @@ -851,94 +850,105 @@ EXPORT_SYMBOL(of_parse_phandle);
>   * }
>   *
>   * To get a device_node of the `node2' node you may call this:
> - * of_parse_phandles_with_args(node3, "list", "#list-cells", 2, &node2, &args);
> + * of_parse_phandle_with_args(node3, "list", "#list-cells", 2, &args);

Not introduced by this patch, but if I read the code correctly,
the fourth parameter should be 1?

>   */
> -int of_parse_phandles_with_args(struct device_node *np, const char *list_name,
> +int of_parse_phandle_with_args(struct device_node *np, const char *list_name,
>  				const char *cells_name, int index,
> -				struct device_node **out_node,
> -				const void **out_args)
> +				struct of_phandle_args *out_args)
>  {
>  	int ret = -EINVAL;
> -	const __be32 *list;
> -	const __be32 *list_end;
> -	int size;
> -	int cur_index = 0;
> +	const __be32 *list, *list_end;
> +	int size, cur_index = 0;
> +	uint32_t count = 0;
>  	struct device_node *node = NULL;
> -	const void *args = NULL;
> +	phandle phandle;
>  
> +	/* Retrieve the phandle list property */
>  	list = of_get_property(np, list_name, &size);
> -	if (!list) {
> -		ret = -ENOENT;
> +	if (!list)
>  		goto err0;
> -	}
>  	list_end = list + size / sizeof(*list);
>  
> +	/*
> +	 * Loop over all the entries in the list, parsing the #cells property
> +	 * for each one to determine how long each argument list is
> +	 */
>  	while (list < list_end) {
> -		const __be32 *cells;
> -		phandle phandle;
> +		count = 0;
>  
> +		/*
> +		 * If phandle is null, then it is an empty entry with no args,
> +		 * skip forward to the next entry
> +		 */
>  		phandle = be32_to_cpup(list++);
> -		args = list;
> -
> -		/* one cell hole in the list = <>; */
> -		if (!phandle)
> -			goto next;
> -
> -		node = of_find_node_by_phandle(phandle);
> -		if (!node) {
> -			pr_debug("%s: could not find phandle\n",
> -				 np->full_name);
> -			goto err0;
> -		}
> +		if (phandle) {
> +			/*
> +			 * Find the provider node and parse the #*-cells
> +			 * property to determine how long the arguements are
> +			 */
> +			node = of_find_node_by_phandle(phandle);
> +			if (!node) {
> +				pr_debug("%s: could not find phandle\n",
> +					 np->full_name);
> +				ret = -EINVAL;
> +				goto err0;
> +			}
> +			if (of_property_read_u32(node, cells_name, &count)) {
> +				pr_debug("%s: could not get %s for %s\n",
> +					 np->full_name, cells_name,
> +					 node->full_name);
> +				ret = -EINVAL;
> +				goto err1;
> +			}
>  
> -		cells = of_get_property(node, cells_name, &size);
> -		if (!cells || size != sizeof(*cells)) {
> -			pr_debug("%s: could not get %s for %s\n",
> -				 np->full_name, cells_name, node->full_name);
> -			goto err1;
> +			/*
> +			 * Make sure that the arguments actually fit in the
> +			 * remaining property data length
> +			 */
> +			if (list + count > list_end) {
> +				pr_debug("%s: insufficient arguments length\n",
> +					 np->full_name);
> +				goto err1;
> +			}
>  		}
>  
> -		list += be32_to_cpup(cells);
> -		if (list > list_end) {
> -			pr_debug("%s: insufficient arguments length\n",
> -				 np->full_name);
> -			goto err1;
> -		}
> -next:
> -		if (cur_index == index)
> +		/*
> +		 * All of the error cases in the loop bail out to the err
> +		 * labels, so at this point, the parsing was successful, but
> +		 * it might be an empty entry
> +		 */
> +		if (cur_index == index) {
> +			ret = phandle ? 0 : -ENOENT;

We have some return code here ...

>  			break;
> +		}
>  
>  		of_node_put(node);
>  		node = NULL;
> -		args = NULL;
> +		list += count;
>  		cur_index++;
>  	}
>  
> -	if (!node) {
> -		/*
> -		 * args w/o node indicates that the loop above has stopped at
> -		 * the 'hole' cell. Report this differently.
> -		 */
> -		if (args)
> -			ret = -EEXIST;
> -		else
> -			ret = -ENOENT;
> +	if (!node)
>  		goto err0;
> -	}
> -
> -	if (out_node)
> -		*out_node = node;
> -	if (out_args)
> -		*out_args = args;
>  
> +	if (out_args) {
> +		int i;
> +		if (WARN_ON(count > MAX_PHANDLE_ARGS))
> +			count = MAX_PHANDLE_ARGS;
> +		out_args->np = node;
> +		out_args->args_count = count;
> +		for (i = 0; i < count; i++)
> +			out_args->args[i] = be32_to_cpup(list++);
> +	}
>  	return 0;

... should this be "return ret"?

> -err1:
> +
> + err1:
>  	of_node_put(node);
> -err0:
> + err0:
>  	pr_debug("%s failed with status %d\n", __func__, ret);
>  	return ret;
>  }
> -EXPORT_SYMBOL(of_parse_phandles_with_args);
> +EXPORT_SYMBOL(of_parse_phandle_with_args);
>  

-- 
Regards,
Shawn

[RFC 1/8] gpio/microblaze: Eliminate duplication of of_get_named_gpio_flags()

[Michal Simek]

Grant Likely wrote:
> of_reset_gpio_handle() is largely a cut-and-paste copy of
> of_get_named_gpio_flags(). There really isn't any reason for the
> split, so this patch deletes the duplicate function
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Michal Simek <monstr@monstr.eu>
> ---
>  arch/microblaze/kernel/reset.c |   43 +--------------------------------------
>  1 files changed, 2 insertions(+), 41 deletions(-)

Sorry for taking to long.

Tested and applied to microblaze tree.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

[RFC 1/8] gpio/microblaze: Eliminate duplication of of_get_named_gpio_flags()

[Grant Likely]

On Wed, Dec 21, 2011 at 11:22:34AM +0100, Michal Simek wrote:
> Grant Likely wrote:
> >of_reset_gpio_handle() is largely a cut-and-paste copy of
> >of_get_named_gpio_flags(). There really isn't any reason for the
> >split, so this patch deletes the duplicate function
> >
> >Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> >Cc: Michal Simek <monstr@monstr.eu>
> >---
> > arch/microblaze/kernel/reset.c |   43 +--------------------------------------
> > 1 files changed, 2 insertions(+), 41 deletions(-)
> 
> Sorry for taking to long.
> 
> Tested and applied to microblaze tree.

I'd rather take this one through the gpio tree if that is okay.

g.

> 
> Thanks,
> Michal
> 
> -- 
> Michal Simek, Ing. (M.Eng)
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
> Microblaze U-BOOT custodian

[RFC 1/8] gpio/microblaze: Eliminate duplication of of_get_named_gpio_flags()

[Michal Simek]

Grant Likely wrote:
> On Wed, Dec 21, 2011 at 11:22:34AM +0100, Michal Simek wrote:
>> Grant Likely wrote:
>>> of_reset_gpio_handle() is largely a cut-and-paste copy of
>>> of_get_named_gpio_flags(). There really isn't any reason for the
>>> split, so this patch deletes the duplicate function
>>>
>>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>>> Cc: Michal Simek <monstr@monstr.eu>
>>> ---
>>> arch/microblaze/kernel/reset.c |   43 +--------------------------------------
>>> 1 files changed, 2 insertions(+), 41 deletions(-)
>> Sorry for taking to long.
>>
>> Tested and applied to microblaze tree.
> 
> I'd rather take this one through the gpio tree if that is okay.

OK. Please do.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian