diff mbox

[RFC,v4,2/2] clk: Add handling of clk parent and rate assigned from DT

Message ID 1396284116-19178-3-git-send-email-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

This function adds a helper function to configure clock parents and rates
as specified in clock-parents, clock-rates DT properties for a consumer
device and a call to it before driver is bound to a device.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes since v3:
 - added detailed description of the assigned-clocks subnode,
 - clk-conf.c is now excluded when CONFIG_OF is not set,
 - s/of_clk_device_setup/of_clk_device_init,
 - added missing 'static inline' to the function stub definition.

Changes since v2:
 - edited in clock-bindings.txt, added note about 'assigned-clocks'
   subnode which may be used to specify "global" clocks configuration
   at a clock provider node,
 - moved of_clk_device_setup() function declaration from clk-provider.h
   to clk-conf.h so required function stubs are available when
   CONFIG_COMMON_CLK is not enabled,

Changes since v1:
 - the helper function to parse and set assigned clock parents and
   rates made public so it is available to clock providers to call
   directly;
 - dropped the platform bus notification and call of_clk_device_setup()
   is is now called from the driver core, rather than from the
   notification callback;
 - s/of_clk_get_list_entry/of_clk_get_by_property.
---
 .../devicetree/bindings/clock/clock-bindings.txt   |   42 ++++++++++
 drivers/base/dd.c                                  |    7 ++
 drivers/clk/Makefile                               |    3 +
 drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
 drivers/clk/clk.c                                  |   10 ++-
 include/linux/clk/clk-conf.h                       |   19 +++++
 6 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/clk-conf.c
 create mode 100644 include/linux/clk/clk-conf.h

--
1.7.9.5

Comments

Ben Dooks March 31, 2014, 5:04 p.m. UTC | #1
On 31/03/14 17:41, Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.

[snip]

tree/bindings/clock/clock-bindings.txt 
b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 700e7aa..59fbb4e 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -132,3 +132,45 @@ clock signal, and a UART.
>     ("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)
> +
> +==Assigned clock parents and rates==
> +
> +Some platforms require static initial configuration of parts of the clocks
> +controller. Such a configuration can be specified in a clock consumer node
> +through clock-parents and clock-rates DT properties. The former should
> +contain a list of parent clocks in form of phandle and clock specifier pairs,
> +the latter the list of assigned clock frequency values (one cell each).
> +
> +    uart@a000 {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +        ...
> +        clocks = <&clkcon 0>, <&clkcon 3>;
> +        clock-names = "baud", "mux";
> +
> +        clock-parents = <0>, <&pll 1>;
> +        clock-rates = <460800>;
> +    };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.
> +
> +Configuring a clock parent and rate through the device node that uses
> +the clock can be done only for clocks that have a single user. Specifying
> +conflicting parent or rate configuration in multiple consumer nodes for
> +a shared clock is forbidden.
> +
> +Configuration of common clocks, which affect multiple consumer devices
> +can be specified in a dedicated 'assigned-clocks' subnode of a clock
> +provider node, e.g.:
> +
> +    clkcon {
> +        ...
> +        #clock-cells = <1>;
> +
> +        assigned-clocks {
> +            clocks = <&clkcon 16>, <&clkcon 17>;
> +            clock-parents = <0>, <&clkcon 1>;
> +            clock-rates = <200000>;
> +        };
> +    };

How do you support not-setting a rate for a clock?

[snip code]
Greg KH March 31, 2014, 8:06 p.m. UTC | #2
On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v3:
>  - added detailed description of the assigned-clocks subnode,
>  - clk-conf.c is now excluded when CONFIG_OF is not set,
>  - s/of_clk_device_setup/of_clk_device_init,
>  - added missing 'static inline' to the function stub definition.
> 
> Changes since v2:
>  - edited in clock-bindings.txt, added note about 'assigned-clocks'
>    subnode which may be used to specify "global" clocks configuration
>    at a clock provider node,
>  - moved of_clk_device_setup() function declaration from clk-provider.h
>    to clk-conf.h so required function stubs are available when
>    CONFIG_COMMON_CLK is not enabled,
> 
> Changes since v1:
>  - the helper function to parse and set assigned clock parents and
>    rates made public so it is available to clock providers to call
>    directly;
>  - dropped the platform bus notification and call of_clk_device_setup()
>    is is now called from the driver core, rather than from the
>    notification callback;
>  - s/of_clk_get_list_entry/of_clk_get_by_property.
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |   42 ++++++++++
>  drivers/base/dd.c                                  |    7 ++
>  drivers/clk/Makefile                               |    3 +
>  drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
>  drivers/clk/clk.c                                  |   10 ++-
>  include/linux/clk/clk-conf.h                       |   19 +++++
>  6 files changed, 167 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/clk-conf.c
>  create mode 100644 include/linux/clk/clk-conf.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 700e7aa..59fbb4e 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -132,3 +132,45 @@ clock signal, and a UART.
>    ("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)
> +
> +==Assigned clock parents and rates==
> +
> +Some platforms require static initial configuration of parts of the clocks
> +controller. Such a configuration can be specified in a clock consumer node
> +through clock-parents and clock-rates DT properties. The former should
> +contain a list of parent clocks in form of phandle and clock specifier pairs,
> +the latter the list of assigned clock frequency values (one cell each).
> +
> +    uart@a000 {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +        ...
> +        clocks = <&clkcon 0>, <&clkcon 3>;
> +        clock-names = "baud", "mux";
> +
> +        clock-parents = <0>, <&pll 1>;
> +        clock-rates = <460800>;
> +    };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.
> +
> +Configuring a clock parent and rate through the device node that uses
> +the clock can be done only for clocks that have a single user. Specifying
> +conflicting parent or rate configuration in multiple consumer nodes for
> +a shared clock is forbidden.
> +
> +Configuration of common clocks, which affect multiple consumer devices
> +can be specified in a dedicated 'assigned-clocks' subnode of a clock
> +provider node, e.g.:
> +
> +    clkcon {
> +        ...
> +        #clock-cells = <1>;
> +
> +        assigned-clocks {
> +            clocks = <&clkcon 16>, <&clkcon 17>;
> +            clock-parents = <0>, <&clkcon 1>;
> +            clock-rates = <200000>;
> +        };
> +    };
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0605176..4c633e7 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -25,6 +25,7 @@
>  #include <linux/async.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/devinfo.h>
> +#include <linux/clk/clk-conf.h>
> 
>  #include "base.h"
>  #include "power/power.h"
> @@ -278,6 +279,12 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		goto probe_failed;
> 
> +	if (dev->of_node) {
> +		ret = of_clk_device_init(dev->of_node);
> +		if (ret)
> +			goto probe_failed;
> +	}
> +
>  	if (driver_sysfs_add(dev)) {
>  		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
>  			__func__, dev_name(dev));


I don't understand why you need the driver core to initialize this one
type of thing?  That should be in a driver, or in a class, or at worse
case, the platform code.

What makes clocks so "unique" here?

greg k-h
Sascha Hauer April 1, 2014, 6:23 a.m. UTC | #3
On Mon, Mar 31, 2014 at 06:04:54PM +0100, Ben Dooks wrote:
> On 31/03/14 17:41, Sylwester Nawrocki wrote:
> >This function adds a helper function to configure clock parents and rates
> >as specified in clock-parents, clock-rates DT properties for a consumer
> >device and a call to it before driver is bound to a device.
> 
> [snip]
> 
> tree/bindings/clock/clock-bindings.txt
> b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >index 700e7aa..59fbb4e 100644
> >--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >@@ -132,3 +132,45 @@ clock signal, and a UART.
> >    ("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)
> >+
> >+==Assigned clock parents and rates==
> >+
> >+Some platforms require static initial configuration of parts of the clocks
> >+controller. Such a configuration can be specified in a clock consumer node
> >+through clock-parents and clock-rates DT properties. The former should
> >+contain a list of parent clocks in form of phandle and clock specifier pairs,
> >+the latter the list of assigned clock frequency values (one cell each).
> >+
> >+    uart@a000 {
> >+        compatible = "fsl,imx-uart";
> >+        reg = <0xa000 0x1000>;
> >+        ...
> >+        clocks = <&clkcon 0>, <&clkcon 3>;
> >+        clock-names = "baud", "mux";
> >+
> >+        clock-parents = <0>, <&pll 1>;
> >+        clock-rates = <460800>;
> >+    };
> >+
> >+In this example the pll is set as parent of "mux" clock and frequency of "baud"
> >+clock is specified as 460800 Hz.
> >+
> >+Configuring a clock parent and rate through the device node that uses
> >+the clock can be done only for clocks that have a single user. Specifying
> >+conflicting parent or rate configuration in multiple consumer nodes for
> >+a shared clock is forbidden.
> >+
> >+Configuration of common clocks, which affect multiple consumer devices
> >+can be specified in a dedicated 'assigned-clocks' subnode of a clock
> >+provider node, e.g.:
> >+
> >+    clkcon {
> >+        ...
> >+        #clock-cells = <1>;
> >+
> >+        assigned-clocks {
> >+            clocks = <&clkcon 16>, <&clkcon 17>;
> >+            clock-parents = <0>, <&clkcon 1>;
> >+            clock-rates = <200000>;
> >+        };
> >+    };
> 
> How do you support not-setting a rate for a clock?

Not setting a rate is supported by specifying the rate to 0. That should
be documented of course.

Sascha
On 01/04/14 08:23, Sascha Hauer wrote:
>> tree/bindings/clock/clock-bindings.txt
>> > b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > >index 700e7aa..59fbb4e 100644
>>> > >--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > >+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > >@@ -132,3 +132,45 @@ clock signal, and a UART.
>>> > >    ("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)
>>> > >+
>>> > >+==Assigned clock parents and rates==
>>> > >+
>>> > >+Some platforms require static initial configuration of parts of the clocks
>>> > >+controller. Such a configuration can be specified in a clock consumer node
>>> > >+through clock-parents and clock-rates DT properties. The former should
>>> > >+contain a list of parent clocks in form of phandle and clock specifier pairs,
>>> > >+the latter the list of assigned clock frequency values (one cell each).
>>> > >+
>>> > >+    uart@a000 {
>>> > >+        compatible = "fsl,imx-uart";
>>> > >+        reg = <0xa000 0x1000>;
>>> > >+        ...
>>> > >+        clocks = <&clkcon 0>, <&clkcon 3>;
>>> > >+        clock-names = "baud", "mux";
>>> > >+
>>> > >+        clock-parents = <0>, <&pll 1>;
>>> > >+        clock-rates = <460800>;
>>> > >+    };
>>> > >+
>>> > >+In this example the pll is set as parent of "mux" clock and frequency of "baud"
>>> > >+clock is specified as 460800 Hz.
[...]
>> > 
>> > How do you support not-setting a rate for a clock?
>
> Not setting a rate is supported by specifying the rate to 0. That should
> be documented of course.

Yes, a rate won't be set for a clock if its corresponding entry in
clock-rates property is set to 0. Sorry, should have mentioned it.

Would adding a sentence as below to end of the first paragraph above
make it clear ?

"To skip setting a rate or parent for a clock the value of a corresponding
entry in the clock-rates or clock-parents property respectively should
be set to 0. The trailing zeros can be omitted."

--
Thanks,
Sylwester
Laurent Pinchart April 1, 2014, 1:15 p.m. UTC | #5
Hi Sylwester,

Thank you for the patch.

On Monday 31 March 2014 18:41:56 Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v3:
>  - added detailed description of the assigned-clocks subnode,
>  - clk-conf.c is now excluded when CONFIG_OF is not set,
>  - s/of_clk_device_setup/of_clk_device_init,
>  - added missing 'static inline' to the function stub definition.
> 
> Changes since v2:
>  - edited in clock-bindings.txt, added note about 'assigned-clocks'
>    subnode which may be used to specify "global" clocks configuration
>    at a clock provider node,
>  - moved of_clk_device_setup() function declaration from clk-provider.h
>    to clk-conf.h so required function stubs are available when
>    CONFIG_COMMON_CLK is not enabled,
> 
> Changes since v1:
>  - the helper function to parse and set assigned clock parents and
>    rates made public so it is available to clock providers to call
>    directly;
>  - dropped the platform bus notification and call of_clk_device_setup()
>    is is now called from the driver core, rather than from the
>    notification callback;
>  - s/of_clk_get_list_entry/of_clk_get_by_property.
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |   42 ++++++++++
>  drivers/base/dd.c                                  |    7 ++
>  drivers/clk/Makefile                               |    3 +
>  drivers/clk/clk-conf.c                             |   87 +++++++++++++++++
>  drivers/clk/clk.c                                  |   10 ++-
>  include/linux/clk/clk-conf.h                       |   19 +++++
>  6 files changed, 167 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/clk-conf.c
>  create mode 100644 include/linux/clk/clk-conf.h

[snip]

> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> new file mode 100644
> index 0000000..a2e992e
> --- /dev/null
> +++ b/drivers/clk/clk-conf.c
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
> + * Sylwester Nawrocki <s.nawrocki@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +
> +/**
> + * of_clk_device_init() - parse and set clk configuration assigned to a
> device
> + * @node: device node to apply the configuration for
> + *
> + * This function parses 'clock-parents' and 'clock-rates' properties and
> sets
> + * any specified clock parents and rates.
> + */
> +int of_clk_device_init(struct device_node *node)
> +{
> +	struct property	*prop;
> +	const __be32 *cur;
> +	int rc, index, num_parents;
> +	struct clk *clk, *pclk;
> +	u32 rate;
> +
> +	num_parents = of_count_phandle_with_args(node, "clock-parents",
> +						 "#clock-cells");
> +	if (num_parents == -EINVAL)
> +		pr_err("clk: Invalid value of clock-parents property at %s\n",
> +		       node->full_name);

This is an implementation detail, but wouldn't it simplify the code if you 
merged the two loops by iterating of the 	clocks property instead of over the 
clock-parents and clock-rates properties separately ?

> +	for (index = 0; index < num_parents; index++) {
> +		pclk = of_clk_get_by_property(node, "clock-parents", index);
> +		if (IS_ERR(pclk)) {
> +			/* skip empty (null) phandles */
> +			if (PTR_ERR(pclk) == -ENOENT)
> +				continue;
> +
> +			pr_warn("clk: couldn't get parent clock %d for %s\n",
> +				index, node->full_name);
> +			return PTR_ERR(pclk);
> +		}
> +
> +		clk = of_clk_get(node, index);
> +		if (IS_ERR(clk)) {
> +			pr_warn("clk: couldn't get clock %d for %s\n",
> +				index, node->full_name);
> +			return PTR_ERR(clk);
> +		}
> +
> +		rc = clk_set_parent(clk, pclk);
> +		if (rc < 0)
> +			pr_err("clk: failed to reparent %s to %s: %d\n",
> +			       __clk_get_name(clk), __clk_get_name(pclk), rc);
> +		else
> +			pr_debug("clk: set %s as parent of %s\n",
> +				 __clk_get_name(pclk), __clk_get_name(clk));
> +	}
> +
> +	index = 0;
> +	of_property_for_each_u32(node, "clock-rates", prop, cur, rate) {
> +		if (rate) {
> +			clk = of_clk_get(node, index);
> +			if (IS_ERR(clk)) {
> +				pr_warn("clk: couldn't get clock %d for %s\n",
> +					index, node->full_name);
> +				return PTR_ERR(clk);
> +			}
> +
> +			rc = clk_set_rate(clk, rate);
> +			if (rc < 0)
> +				pr_err("clk: couldn't set %s clock rate: %d\n",
> +				       __clk_get_name(clk), rc);
> +			else
> +				pr_debug("clk: set rate of clock %s to %lu\n",
> +					 __clk_get_name(clk), clk_get_rate(clk));
> +		}
> +		index++;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index dff0373..ea6d8bd 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c

[snip]

> @@ -2620,7 +2621,15 @@ void __init of_clk_init(const struct of_device_id
> *matches) list_for_each_entry_safe(clk_provider, next,
>  					&clk_provider_list, node) {
>  			if (force || parent_ready(clk_provider->np)) {
> +
>  				clk_provider->clk_init_cb(clk_provider->np);
> +
> +				/* Set any assigned clock parents and rates */
> +				np = of_get_child_by_name(clk_provider->np,
> +							  "assigned-clocks");
> +				if (np)
> +					of_clk_device_init(np);

Aren't you missing an of_node_put() here ?

> +
>  				list_del(&clk_provider->node);
>  				kfree(clk_provider);
>  				is_init_done = true;
Ben Dooks April 1, 2014, 1:19 p.m. UTC | #6
On 31/03/14 21:06, Greg KH wrote:
> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>> This function adds a helper function to configure clock parents and rates
>> as specified in clock-parents, clock-rates DT properties for a consumer
>> device and a call to it before driver is bound to a device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

[snip]

>
> I don't understand why you need the driver core to initialize this one
> type of thing?  That should be in a driver, or in a class, or at worse
> case, the platform code.
>
> What makes clocks so "unique" here?

I suppose the issue here is that a lot of drivers currently use
clocks and a number of systems have badly setup default clock trees
at start time.

Mark Brown and others have argued that the management of clocks which
is common to all devices should not live in the driver.
On 01/04/14 15:19, Ben Dooks wrote:
> On 31/03/14 21:06, Greg KH wrote:
>> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
[...]
>> > I don't understand why you need the driver core to initialize this one
>> > type of thing?  That should be in a driver, or in a class, or at worse
>> > case, the platform code.
>> >
>> > What makes clocks so "unique" here?

The reason I put it in the driver core was mainly to avoid having many
drivers doing same call to this initialization function.
I was considering moving it to the bus code, still there are several
buses for which it would need to be repeated.

Maybe really_probe() is not a best place to put this, nonetheless
the requirements I could list were:

 1. not involving individual drivers,
 2. have such an initialization call done for all devices, irrespective
    of Linux bus or class type,
 3. Handle errors properly, e.g. defer driver probing if a clock for
   a device is not yet available.

One advantage I could see from making the call from within a device
driver is that a device could keep using the common DT bindings and
replace the common initialization function with a private one, if
there is a need for some quirks handled for a device. With approach
as in this patch it's difficult to override the default behaviour.
However then there is a question whether we strive for the clocks
management to be possibly kept away from device drivers.

> I suppose the issue here is that a lot of drivers currently use
> clocks and a number of systems have badly setup default clock trees
> at start time.
> 
> Mark Brown and others have argued that the management of clocks which
> is common to all devices should not live in the driver.

True, motivation behind this patch series was also replacing custom
code in multiple drivers doing similar clock rate or parent setting
by a common code, using standardized DT binding.

--
Thanks,
Sylwester
Hi Laurent,

On 01/04/14 15:15, Laurent Pinchart wrote:
[...]
>> +/**
>> + * of_clk_device_init() - parse and set clk configuration assigned to a
>> device
>> + * @node: device node to apply the configuration for
>> + *
>> + * This function parses 'clock-parents' and 'clock-rates' properties and
>> sets
>> + * any specified clock parents and rates.
>> + */
>> +int of_clk_device_init(struct device_node *node)
>> +{
>> +	struct property	*prop;
>> +	const __be32 *cur;
>> +	int rc, index, num_parents;
>> +	struct clk *clk, *pclk;
>> +	u32 rate;
>> +
>> +	num_parents = of_count_phandle_with_args(node, "clock-parents",
>> +						 "#clock-cells");
>> +	if (num_parents == -EINVAL)
>> +		pr_err("clk: Invalid value of clock-parents property at %s\n",
>> +		       node->full_name);
> 
> This is an implementation detail, but wouldn't it simplify the code if you 
> merged the two loops by iterating of the 	clocks property instead of over the 
> clock-parents and clock-rates properties separately ?

The issue here is that all clock parents should be set before we start setting
clock rates. Otherwise the clock frequencies could be incorrect if clk_set_rate()
is followed by clk_set_parent().

>> +	for (index = 0; index < num_parents; index++) {
>> +		pclk = of_clk_get_by_property(node, "clock-parents", index);
>> +		if (IS_ERR(pclk)) {
>> +			/* skip empty (null) phandles */
>> +			if (PTR_ERR(pclk) == -ENOENT)
>> +				continue;
>> +
>> +			pr_warn("clk: couldn't get parent clock %d for %s\n",
>> +				index, node->full_name);
>> +			return PTR_ERR(pclk);
>> +		}
>> +
>> +		clk = of_clk_get(node, index);
>> +		if (IS_ERR(clk)) {
>> +			pr_warn("clk: couldn't get clock %d for %s\n",
>> +				index, node->full_name);
>> +			return PTR_ERR(clk);
>> +		}
>> +
>> +		rc = clk_set_parent(clk, pclk);
>> +		if (rc < 0)
>> +			pr_err("clk: failed to reparent %s to %s: %d\n",
>> +			       __clk_get_name(clk), __clk_get_name(pclk), rc);
>> +		else
>> +			pr_debug("clk: set %s as parent of %s\n",
>> +				 __clk_get_name(pclk), __clk_get_name(clk));
>> +	}
>> +
>> +	index = 0;
>> +	of_property_for_each_u32(node, "clock-rates", prop, cur, rate) {
>> +		if (rate) {
>> +			clk = of_clk_get(node, index);
>> +			if (IS_ERR(clk)) {
>> +				pr_warn("clk: couldn't get clock %d for %s\n",
>> +					index, node->full_name);
>> +				return PTR_ERR(clk);
>> +			}
>> +
>> +			rc = clk_set_rate(clk, rate);
>> +			if (rc < 0)
>> +				pr_err("clk: couldn't set %s clock rate: %d\n",
>> +				       __clk_get_name(clk), rc);
>> +			else
>> +				pr_debug("clk: set rate of clock %s to %lu\n",
>> +					 __clk_get_name(clk), clk_get_rate(clk));
>> +		}
>> +		index++;
>> +	}
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index dff0373..ea6d8bd 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
> 
> [snip]
> 
>> @@ -2620,7 +2621,15 @@ void __init of_clk_init(const struct of_device_id
>> *matches) list_for_each_entry_safe(clk_provider, next,
>>  					&clk_provider_list, node) {
>>  			if (force || parent_ready(clk_provider->np)) {
>> +
>>  				clk_provider->clk_init_cb(clk_provider->np);
>> +
>> +				/* Set any assigned clock parents and rates */
>> +				np = of_get_child_by_name(clk_provider->np,
>> +							  "assigned-clocks");
>> +				if (np)
>> +					of_clk_device_init(np);
> 
> Aren't you missing an of_node_put() here ?

Indeed, it's missing. Will fix that in next version, thanks for pointing out.

>>  				list_del(&clk_provider->node);
>>  				kfree(clk_provider);
>>  				is_init_done = true;

--
Regards,
Sylwester
Greg KH April 1, 2014, 4:35 p.m. UTC | #9
On Tue, Apr 01, 2014 at 02:19:40PM +0100, Ben Dooks wrote:
> On 31/03/14 21:06, Greg KH wrote:
> >On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
> >>This function adds a helper function to configure clock parents and rates
> >>as specified in clock-parents, clock-rates DT properties for a consumer
> >>device and a call to it before driver is bound to a device.
> >>
> >>Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> [snip]
> 
> >
> >I don't understand why you need the driver core to initialize this one
> >type of thing?  That should be in a driver, or in a class, or at worse
> >case, the platform code.
> >
> >What makes clocks so "unique" here?
> 
> I suppose the issue here is that a lot of drivers currently use
> clocks and a number of systems have badly setup default clock trees
> at start time.

Then they should be fixed, why should _all_ Linux devices care about
such broken devices/systems?

> Mark Brown and others have argued that the management of clocks which
> is common to all devices should not live in the driver.

Then put it in the bus that initializes the devices / drivers.

thanks,

greg k-h
Greg KH April 1, 2014, 4:37 p.m. UTC | #10
On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote:
> On 01/04/14 15:19, Ben Dooks wrote:
> > On 31/03/14 21:06, Greg KH wrote:
> >> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
> [...]
> >> > I don't understand why you need the driver core to initialize this one
> >> > type of thing?  That should be in a driver, or in a class, or at worse
> >> > case, the platform code.
> >> >
> >> > What makes clocks so "unique" here?
> 
> The reason I put it in the driver core was mainly to avoid having many
> drivers doing same call to this initialization function.
> I was considering moving it to the bus code, still there are several
> buses for which it would need to be repeated.

"several" is how many?  2?  3?  10?

Please fix it "correctly" and don't put it in the driver core just
because it seems easier that way.

> Maybe really_probe() is not a best place to put this, nonetheless
> the requirements I could list were:
> 
>  1. not involving individual drivers,

Why not?

>  2. have such an initialization call done for all devices, irrespective
>     of Linux bus or class type,

Why?  Do _all_ devices that Linux supports have this issue to be
resolved?

>  3. Handle errors properly, e.g. defer driver probing if a clock for
>    a device is not yet available.

Then do it in the bus that controls that device, as it knows to defer
probing at that point in time.

> One advantage I could see from making the call from within a device
> driver is that a device could keep using the common DT bindings and
> replace the common initialization function with a private one, if
> there is a need for some quirks handled for a device. With approach
> as in this patch it's difficult to override the default behaviour.
> However then there is a question whether we strive for the clocks
> management to be possibly kept away from device drivers.
> 
> > I suppose the issue here is that a lot of drivers currently use
> > clocks and a number of systems have badly setup default clock trees
> > at start time.
> > 
> > Mark Brown and others have argued that the management of clocks which
> > is common to all devices should not live in the driver.
> 
> True, motivation behind this patch series was also replacing custom
> code in multiple drivers doing similar clock rate or parent setting
> by a common code, using standardized DT binding.

Then put it in the bus that controls these broken devices / platforms.

greg k-h
Sascha Hauer April 2, 2014, 5:37 a.m. UTC | #11
On Tue, Apr 01, 2014 at 09:37:44AM -0700, Greg KH wrote:
> On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote:
> > On 01/04/14 15:19, Ben Dooks wrote:
> > > On 31/03/14 21:06, Greg KH wrote:
> > >> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
> > [...]
> > >> > I don't understand why you need the driver core to initialize this one
> > >> > type of thing?  That should be in a driver, or in a class, or at worse
> > >> > case, the platform code.
> > >> >
> > >> > What makes clocks so "unique" here?
> > 
> > The reason I put it in the driver core was mainly to avoid having many
> > drivers doing same call to this initialization function.
> > I was considering moving it to the bus code, still there are several
> > buses for which it would need to be repeated.
> 
> "several" is how many?  2?  3?  10?
> 
> Please fix it "correctly" and don't put it in the driver core just
> because it seems easier that way.
> 
> > Maybe really_probe() is not a best place to put this, nonetheless
> > the requirements I could list were:
> > 
> >  1. not involving individual drivers,
> 
> Why not?
> 
> >  2. have such an initialization call done for all devices, irrespective
> >     of Linux bus or class type,
> 
> Why?  Do _all_ devices that Linux supports have this issue to be
> resolved?
> 
> >  3. Handle errors properly, e.g. defer driver probing if a clock for
> >    a device is not yet available.
> 
> Then do it in the bus that controls that device, as it knows to defer
> probing at that point in time.

The issue this patch tries to solve is not about single devices, but
about the way these devices are connected with each other.

Clocks for different (sometimes completely unrelated) devices influence
each other. You can't control the clock from one device without
influencing other devices. Knowledge about these constraints can't be
encoded in the drivers, because the constraints differ per SoC,
sometimes even per board. Many SoCs share the same devices, but the
clock topology they are surrounded with is always different. With this I
don't mean the direct clock inputs, these are well abstracted with the
current clk_* API.  What I mean is situations like: "On this board use
the clock controller to output this particular clock on that pin,
because it happens to be the master clock of some audio codec connected
externally; also make the same clock input to the internal Audio system
to make sure both are in sync". These situations can be completely
different on the next board or on the next SoC which has the same
devices, but a different clock routing.

That said, I also think the driver core doesn't have to be bothered with
the clock setup. Putting the clock setup into the devicenode providing
the clocks (and thus parsing it from the clock controller driver) should
be sufficient.

Sascha
Peter De Schrijver April 2, 2014, 8:01 a.m. UTC | #12
On Tue, Apr 01, 2014 at 03:19:40PM +0200, Ben Dooks wrote:
> On 31/03/14 21:06, Greg KH wrote:
> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
> >> This function adds a helper function to configure clock parents and rates
> >> as specified in clock-parents, clock-rates DT properties for a consumer
> >> device and a call to it before driver is bound to a device.
> >>
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> [snip]
> 
> >
> > I don't understand why you need the driver core to initialize this one
> > type of thing?  That should be in a driver, or in a class, or at worse
> > case, the platform code.
> >
> > What makes clocks so "unique" here?
> 
> I suppose the issue here is that a lot of drivers currently use
> clocks and a number of systems have badly setup default clock trees
> at start time.
> 
> Mark Brown and others have argued that the management of clocks which
> is common to all devices should not live in the driver.

Exactly, this data should be part of the clock provider DT nodes, not
of the device nodes. 

Cheers,

Peter.
On 01/04/14 18:37, Greg KH wrote:
> On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote:
>> On 01/04/14 15:19, Ben Dooks wrote:
>>> On 31/03/14 21:06, Greg KH wrote:
>>>>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>> [...]
>>>>> I don't understand why you need the driver core to initialize this one
>>>>> type of thing?  That should be in a driver, or in a class, or at worse
>>>>> case, the platform code.
>>>>>
>>>>> What makes clocks so "unique" here?
>>
>> The reason I put it in the driver core was mainly to avoid having many
>> drivers doing same call to this initialization function.
>> I was considering moving it to the bus code, still there are several
>> buses for which it would need to be repeated.
> 
> "several" is how many?  2?  3?  10?

It would be mostly the platform and amba bus, but this clock defaults
could be useful also for i2c, spi. In general it's about not probable
devices which may be specified in devicetree.

> Please fix it "correctly" and don't put it in the driver core just
> because it seems easier that way.
> 
>> Maybe really_probe() is not a best place to put this, nonetheless
>> the requirements I could list were:
>>
>>  1. not involving individual drivers,
> 
> Why not?

I'd say because this thing can be often considered a platform detail
which would be better opaque to drivers. It's at the border of a device
and its integration within a platform.

>>  2. have such an initialization call done for all devices, irrespective
>>     of Linux bus or class type,
> 
> Why?  Do _all_ devices that Linux supports have this issue to be
> resolved?

Sorry, certainly not all devices. I think it's only related to the
non-probable devices which can be specified in DT.

>>  3. Handle errors properly, e.g. defer driver probing if a clock for
>>    a device is not yet available.
> 
> Then do it in the bus that controls that device, as it knows to defer
> probing at that point in time.

All right, that would also work. I suspect doing it for platform and
amba bus might be sufficient in the beginning.

>> One advantage I could see from making the call from within a device
>> driver is that a device could keep using the common DT bindings and
>> replace the common initialization function with a private one, if
>> there is a need for some quirks handled for a device. With approach
>> as in this patch it's difficult to override the default behaviour.
>> However then there is a question whether we strive for the clocks
>> management to be possibly kept away from device drivers.
>>
>>> I suppose the issue here is that a lot of drivers currently use
>>> clocks and a number of systems have badly setup default clock trees
>>> at start time.
>>>
>>> Mark Brown and others have argued that the management of clocks which
>>> is common to all devices should not live in the driver.
>>
>> True, motivation behind this patch series was also replacing custom
>> code in multiple drivers doing similar clock rate or parent setting
>> by a common code, using standardized DT binding.
> 
> Then put it in the bus that controls these broken devices / platforms.

OK, I'm going to put it in the bus code.
On 02/04/14 07:37, Sascha Hauer wrote:
> On Tue, Apr 01, 2014 at 09:37:44AM -0700, Greg KH wrote:
>> On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote:
>>> On 01/04/14 15:19, Ben Dooks wrote:
>>>> On 31/03/14 21:06, Greg KH wrote:
>>>>>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>>> [...]
>>>>>> I don't understand why you need the driver core to initialize this one
>>>>>> type of thing?  That should be in a driver, or in a class, or at worse
>>>>>> case, the platform code.
>>>>>>
>>>>>> What makes clocks so "unique" here?
>>>
>>> The reason I put it in the driver core was mainly to avoid having many
>>> drivers doing same call to this initialization function.
>>> I was considering moving it to the bus code, still there are several
>>> buses for which it would need to be repeated.
>>
>> "several" is how many?  2?  3?  10?
>>
>> Please fix it "correctly" and don't put it in the driver core just
>> because it seems easier that way.
>>
>>> Maybe really_probe() is not a best place to put this, nonetheless
>>> the requirements I could list were:
>>>
>>>  1. not involving individual drivers,
>>
>> Why not?
>>
>>>  2. have such an initialization call done for all devices, irrespective
>>>     of Linux bus or class type,
>>
>> Why?  Do _all_ devices that Linux supports have this issue to be
>> resolved?
>>
>>>  3. Handle errors properly, e.g. defer driver probing if a clock for
>>>    a device is not yet available.
>>
>> Then do it in the bus that controls that device, as it knows to defer
>> probing at that point in time.
> 
> The issue this patch tries to solve is not about single devices, but
> about the way these devices are connected with each other.
> 
> Clocks for different (sometimes completely unrelated) devices influence
> each other. You can't control the clock from one device without
> influencing other devices. Knowledge about these constraints can't be
> encoded in the drivers, because the constraints differ per SoC,
> sometimes even per board. Many SoCs share the same devices, but the
> clock topology they are surrounded with is always different. With this I
> don't mean the direct clock inputs, these are well abstracted with the
> current clk_* API.  What I mean is situations like: "On this board use
> the clock controller to output this particular clock on that pin,
> because it happens to be the master clock of some audio codec connected
> externally; also make the same clock input to the internal Audio system
> to make sure both are in sync". These situations can be completely
> different on the next board or on the next SoC which has the same
> devices, but a different clock routing.
> 
> That said, I also think the driver core doesn't have to be bothered with
> the clock setup. Putting the clock setup into the devicenode providing
> the clocks (and thus parsing it from the clock controller driver) should
> be sufficient.

It would work but I don't really like such a DT binding. Rather than
only having a long list of clocks in one node I would prefer to also
have a possibility to list clock data specific to a device in its
corresponding DT node. Similarly as it's done, e.g. with interrupts.
On 02/04/14 10:01, Peter De Schrijver wrote:
> On Tue, Apr 01, 2014 at 03:19:40PM +0200, Ben Dooks wrote:
>> On 31/03/14 21:06, Greg KH wrote:
>>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>>>> This function adds a helper function to configure clock parents and rates
>>>> as specified in clock-parents, clock-rates DT properties for a consumer
>>>> device and a call to it before driver is bound to a device.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>
>> [snip]
>>
>>> I don't understand why you need the driver core to initialize this one
>>> type of thing?  That should be in a driver, or in a class, or at worse
>>> case, the platform code.
>>>
>>> What makes clocks so "unique" here?
>>
>> I suppose the issue here is that a lot of drivers currently use
>> clocks and a number of systems have badly setup default clock trees
>> at start time.
>>
>> Mark Brown and others have argued that the management of clocks which
>> is common to all devices should not live in the driver.
> 
> Exactly, this data should be part of the clock provider DT nodes, not
> of the device nodes. 

Why not in the device nodes, when often this data is closely related
to a device ? I'd prefer to keep the ability to also specify it at
the consumer nodes. IMHO the DT binding would be more explicit and
less error prone this way. E.g. clock-rates may refer directly to the
consumer clocks, so why we should be specifying it for all devices in
a single DT node ?

Also I'm not sure if a fact that something shouldn't be handled in
a device driver necessarily means that related data shouldn't be put
in the device node.

It would be nice to hear more opinions, it seems now there is
roughly same number of proponents and opponents of each option.

--
Thanks,
Sylwester
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 700e7aa..59fbb4e 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -132,3 +132,45 @@  clock signal, and a UART.
   ("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)
+
+==Assigned clock parents and rates==
+
+Some platforms require static initial configuration of parts of the clocks
+controller. Such a configuration can be specified in a clock consumer node
+through clock-parents and clock-rates DT properties. The former should
+contain a list of parent clocks in form of phandle and clock specifier pairs,
+the latter the list of assigned clock frequency values (one cell each).
+
+    uart@a000 {
+        compatible = "fsl,imx-uart";
+        reg = <0xa000 0x1000>;
+        ...
+        clocks = <&clkcon 0>, <&clkcon 3>;
+        clock-names = "baud", "mux";
+
+        clock-parents = <0>, <&pll 1>;
+        clock-rates = <460800>;
+    };
+
+In this example the pll is set as parent of "mux" clock and frequency of "baud"
+clock is specified as 460800 Hz.
+
+Configuring a clock parent and rate through the device node that uses
+the clock can be done only for clocks that have a single user. Specifying
+conflicting parent or rate configuration in multiple consumer nodes for
+a shared clock is forbidden.
+
+Configuration of common clocks, which affect multiple consumer devices
+can be specified in a dedicated 'assigned-clocks' subnode of a clock
+provider node, e.g.:
+
+    clkcon {
+        ...
+        #clock-cells = <1>;
+
+        assigned-clocks {
+            clocks = <&clkcon 16>, <&clkcon 17>;
+            clock-parents = <0>, <&clkcon 1>;
+            clock-rates = <200000>;
+        };
+    };
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0605176..4c633e7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,7 @@ 
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/clk/clk-conf.h>

 #include "base.h"
 #include "power/power.h"
@@ -278,6 +279,12 @@  static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		goto probe_failed;

+	if (dev->of_node) {
+		ret = of_clk_device_init(dev->of_node);
+		if (ret)
+			goto probe_failed;
+	}
+
 	if (driver_sysfs_add(dev)) {
 		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
 			__func__, dev_name(dev));
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 85a33e6..5ec53cb 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -8,6 +8,9 @@  obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-mux.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
+ifeq ($(CONFIG_OF), y)
+obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
+endif

 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
new file mode 100644
index 0000000..a2e992e
--- /dev/null
+++ b/drivers/clk/clk-conf.c
@@ -0,0 +1,87 @@ 
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/printk.h>
+
+/**
+ * of_clk_device_init() - parse and set clk configuration assigned to a device
+ * @node: device node to apply the configuration for
+ *
+ * This function parses 'clock-parents' and 'clock-rates' properties and sets
+ * any specified clock parents and rates.
+ */
+int of_clk_device_init(struct device_node *node)
+{
+	struct property	*prop;
+	const __be32 *cur;
+	int rc, index, num_parents;
+	struct clk *clk, *pclk;
+	u32 rate;
+
+	num_parents = of_count_phandle_with_args(node, "clock-parents",
+						 "#clock-cells");
+	if (num_parents == -EINVAL)
+		pr_err("clk: Invalid value of clock-parents property at %s\n",
+		       node->full_name);
+
+	for (index = 0; index < num_parents; index++) {
+		pclk = of_clk_get_by_property(node, "clock-parents", index);
+		if (IS_ERR(pclk)) {
+			/* skip empty (null) phandles */
+			if (PTR_ERR(pclk) == -ENOENT)
+				continue;
+
+			pr_warn("clk: couldn't get parent clock %d for %s\n",
+				index, node->full_name);
+			return PTR_ERR(pclk);
+		}
+
+		clk = of_clk_get(node, index);
+		if (IS_ERR(clk)) {
+			pr_warn("clk: couldn't get clock %d for %s\n",
+				index, node->full_name);
+			return PTR_ERR(clk);
+		}
+
+		rc = clk_set_parent(clk, pclk);
+		if (rc < 0)
+			pr_err("clk: failed to reparent %s to %s: %d\n",
+			       __clk_get_name(clk), __clk_get_name(pclk), rc);
+		else
+			pr_debug("clk: set %s as parent of %s\n",
+				 __clk_get_name(pclk), __clk_get_name(clk));
+	}
+
+	index = 0;
+	of_property_for_each_u32(node, "clock-rates", prop, cur, rate) {
+		if (rate) {
+			clk = of_clk_get(node, index);
+			if (IS_ERR(clk)) {
+				pr_warn("clk: couldn't get clock %d for %s\n",
+					index, node->full_name);
+				return PTR_ERR(clk);
+			}
+
+			rc = clk_set_rate(clk, rate);
+			if (rc < 0)
+				pr_err("clk: couldn't set %s clock rate: %d\n",
+				       __clk_get_name(clk), rc);
+			else
+				pr_debug("clk: set rate of clock %s to %lu\n",
+					 __clk_get_name(clk), clk_get_rate(clk));
+		}
+		index++;
+	}
+
+	return 0;
+}
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dff0373..ea6d8bd 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -10,6 +10,7 @@ 
  */

 #include <linux/clk-private.h>
+#include <linux/clk/clk-conf.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
@@ -2620,7 +2621,15 @@  void __init of_clk_init(const struct of_device_id *matches)
 		list_for_each_entry_safe(clk_provider, next,
 					&clk_provider_list, node) {
 			if (force || parent_ready(clk_provider->np)) {
+
 				clk_provider->clk_init_cb(clk_provider->np);
+
+				/* Set any assigned clock parents and rates */
+				np = of_get_child_by_name(clk_provider->np,
+							  "assigned-clocks");
+				if (np)
+					of_clk_device_init(np);
+
 				list_del(&clk_provider->node);
 				kfree(clk_provider);
 				is_init_done = true;
@@ -2635,7 +2644,6 @@  void __init of_clk_init(const struct of_device_id *matches)
 		 */
 		if (!is_init_done)
 			force = true;
-
 	}
 }
 #endif
diff --git a/include/linux/clk/clk-conf.h b/include/linux/clk/clk-conf.h
new file mode 100644
index 0000000..51b9c01
--- /dev/null
+++ b/include/linux/clk/clk-conf.h
@@ -0,0 +1,19 @@ 
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+struct device_node;
+
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+int of_clk_device_init(struct device_node *node);
+#else
+static inline int of_clk_device_init(struct device_node *node)
+{
+	return 0;
+}
+#endif