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

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 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   |   26 ++++++
 drivers/base/dd.c                                  |    7 ++
 drivers/clk/Makefile                               |    1 +
 drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
 drivers/clk/clk.c                                  |   10 ++-
 include/linux/clk/clk-conf.h                       |   19 +++++
 6 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/clk-conf.c
 create mode 100644 include/linux/clk/clk-conf.h

Hi Sylwester,

Thank you for the patch.

On Thursday 27 March 2014 13:16:19 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 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   |   26 ++++++
>  drivers/base/dd.c                                  |    7 ++
>  drivers/clk/Makefile                               |    1 +
>  drivers/clk/clk-conf.c                             |   87 +++++++++++++++++
>  drivers/clk/clk.c                                  |   10 ++-
>  include/linux/clk/clk-conf.h                       |   19 +++++
>  6 files changed, 149 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
> 7c52c29..b452f80 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -115,3 +115,29 @@ 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.

I'm curious, what should happen when two devices have conflicting requirements 
? If a different device required the <&clkcon 3> parent to be set to <&pll 2> 
for instance, who should win ? Shouldn't a warning be printed ?

> +For clocks which are not directly connected to any consumer device
> similarly
> +clocks, clock-parents and/or clock-rates properties should be specified in
> +assigned-clocks subnode of a clock controller DT node.

It might be that I'm not familiar enough with the clock framework, but this 
sounds unclear to me. I'm not sure what you mean exactly.

> 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 a367a98..c720e4b 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -8,6 +8,7 @@ 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
> +obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
> 
>  # 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 29dc1e7..240d776 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>
> @@ -2604,7 +2605,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;
> @@ -2619,7 +2628,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..f06b1c2
> --- /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
> +int of_clk_device_init(struct device_node *node)
> +{
> +	return 0;
> +}
> +#endif

Hi Laurent,

On 27/03/14 14:24, Laurent Pinchart wrote:
> On Thursday 27 March 2014 13:16:19 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>
>> ---
[...]
>> ---
>>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
>>  drivers/base/dd.c                                  |    7 ++
>>  drivers/clk/Makefile                               |    1 +
>>  drivers/clk/clk-conf.c                             |   87 +++++++++++++++++
>>  drivers/clk/clk.c                                  |   10 ++-
>>  include/linux/clk/clk-conf.h                       |   19 +++++
>>  6 files changed, 149 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
>> 7c52c29..b452f80 100644
>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -115,3 +115,29 @@ 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.
> 
> I'm curious, what should happen when two devices have conflicting requirements 
> ? If a different device required the <&clkcon 3> parent to be set to <&pll 2> 
> for instance, who should win ? Shouldn't a warning be printed ?

In general, the assumption is that the <&clkcon 3> clock would be used only
by the uart@a000 device. If a clock is shared I'd say it shouldn't be put
in a multiple consumer device nodes. Instead it should be put in a clock
provider node, as I was trying to explain in the sentence below.

A warning could be useful, but it could complicate the code. We would need,
for example, to store information about already configured clocks in a list and
scan it before actually altering any clock parent or rate.

>> +For clocks which are not directly connected to any consumer device
>> similarly
>> +clocks, clock-parents and/or clock-rates properties should be specified in
>> +assigned-clocks subnode of a clock controller DT node.
> 
> It might be that I'm not familiar enough with the clock framework, but this 
> sounds unclear to me. I'm not sure what you mean exactly.

Sorry about not being precise here, would something like below be more
clear ?

"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>;
        };
    };	
"

Naturally it's this just an RFC, any critics or suggestions are welcome.:)

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sylwester,

On Thursday 27 March 2014 14:57:56 Sylwester Nawrocki wrote:
> On 27/03/14 14:24, Laurent Pinchart wrote:
> > On Thursday 27 March 2014 13:16:19 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>
> >> ---
> 
> [...]
> 
> >> ---
> >> 
> >>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
> >>  drivers/base/dd.c                                  |    7 ++
> >>  drivers/clk/Makefile                               |    1 +
> >>  drivers/clk/clk-conf.c                             |   87 ++++++++++++++
> >>  drivers/clk/clk.c                                  |   10 ++-
> >>  include/linux/clk/clk-conf.h                       |   19 +++++
> >>  6 files changed, 149 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
> >> 7c52c29..b452f80 100644
> >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> @@ -115,3 +115,29 @@ 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.
> > 
> > I'm curious, what should happen when two devices have conflicting
> > requirements ? If a different device required the <&clkcon 3> parent to
> > be set to <&pll 2> for instance, who should win ? Shouldn't a warning be
> > printed ?
> 
> In general, the assumption is that the <&clkcon 3> clock would be used only
> by the uart@a000 device.

OK. Removing the problem is a simple way to fix it :-) What about stating this 
explicitly in the documentation then ? Maybe by prefixing your proposed 
explanation below with something like

"Configuring a clock parent and rate through the device node that uses the 
clock is only supported for clocks that have a single user."

> If a clock is shared I'd say it shouldn't be put in a multiple consumer
> device nodes. Instead it should be put in a clock provider node, as I was
> trying to explain in the sentence below.
>
> A warning could be useful, but it could complicate the code. We would need,
> for example, to store information about already configured clocks in a list
> and scan it before actually altering any clock parent or rate.

I'm fine with implementing that later if needed, we can keep the initial 
implementation simple.

> >> +For clocks which are not directly connected to any consumer device
> >> similarly
> >> +clocks, clock-parents and/or clock-rates properties should be specified
> >> in
> >> +assigned-clocks subnode of a clock controller DT node.
> > 
> > It might be that I'm not familiar enough with the clock framework, but
> > this sounds unclear to me. I'm not sure what you mean exactly.
> 
> Sorry about not being precise here, would something like below be more
> clear ?
> 
> "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>;
>         };
>     };
> "

That's clearer indeed. Can the parents and rates depend on the board, or on 
the SoC only ? We might be getting dangerously close to specifying platform 
configuration instead of describing the hardware. A real example might be nice 
to support the discussion.

> Naturally it's this just an RFC, any critics or suggestions are welcome.:)

On Thu, Mar 27, 2014 at 03:08:10PM +0100, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Thursday 27 March 2014 14:57:56 Sylwester Nawrocki wrote:
> > On 27/03/14 14:24, Laurent Pinchart wrote:
> > > On Thursday 27 March 2014 13:16:19 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>
> > >> ---
> > 
> > [...]
> > 
> > >> ---
> > >> 
> > >>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
> > >>  drivers/base/dd.c                                  |    7 ++
> > >>  drivers/clk/Makefile                               |    1 +
> > >>  drivers/clk/clk-conf.c                             |   87 ++++++++++++++
> > >>  drivers/clk/clk.c                                  |   10 ++-
> > >>  include/linux/clk/clk-conf.h                       |   19 +++++
> > >>  6 files changed, 149 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
> > >> 7c52c29..b452f80 100644
> > >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> @@ -115,3 +115,29 @@ 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.
> > > 
> > > I'm curious, what should happen when two devices have conflicting
> > > requirements ? If a different device required the <&clkcon 3> parent to
> > > be set to <&pll 2> for instance, who should win ? Shouldn't a warning be
> > > printed ?
> > 
> > In general, the assumption is that the <&clkcon 3> clock would be used only
> > by the uart@a000 device.
> 
> OK. Removing the problem is a simple way to fix it :-) What about stating this 
> explicitly in the documentation then ? Maybe by prefixing your proposed 
> explanation below with something like
> 
> "Configuring a clock parent and rate through the device node that uses the 
> clock is only supported for clocks that have a single user."
> 
> > If a clock is shared I'd say it shouldn't be put in a multiple consumer
> > device nodes. Instead it should be put in a clock provider node, as I was
> > trying to explain in the sentence below.
> >
> > A warning could be useful, but it could complicate the code. We would need,
> > for example, to store information about already configured clocks in a list
> > and scan it before actually altering any clock parent or rate.
> 
> I'm fine with implementing that later if needed, we can keep the initial 
> implementation simple.
> 
> > >> +For clocks which are not directly connected to any consumer device
> > >> similarly
> > >> +clocks, clock-parents and/or clock-rates properties should be specified
> > >> in
> > >> +assigned-clocks subnode of a clock controller DT node.
> > > 
> > > It might be that I'm not familiar enough with the clock framework, but
> > > this sounds unclear to me. I'm not sure what you mean exactly.
> > 
> > Sorry about not being precise here, would something like below be more
> > clear ?
> > 
> > "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>;
> >         };
> >     };
> > "
> 
> That's clearer indeed. Can the parents and rates depend on the board, or on 
> the SoC only ? We might be getting dangerously close to specifying platform 
> configuration instead of describing the hardware. A real example might be nice 
> to support the discussion.

This patch comes just at the right time. This is what I do with it:

#define cko1_sel 57
#define pll4_audio_div 203
#define pll4_audio 173
#define ssi3_sel 47

&clks {
	assigned-clocks {
		clocks = <&clks cko1_sel>, <&clks ssi3_sel>, <&clks pll4_audio>;
		clock-parents = <&clks pll4_audio_div>, <&clks pll4_audio_div>, <0>;
		clock-rates = <0>, <0>, <786432000>;
	};
};

cko1_sel is a clock that can be routed out of the SoC. In my case it is
connected the sysclk of an external Audio Codec. ssi3_sel drives my SoC
internal I2S unit which I use in master mode. The above makes sure that
the I2S unit and the the external codec both get their clock from the
audio PLL. The audio PLL is configured to a rate of 786432000Hz which
is an exact multiple of the desired audio clock.

Sascha

On 27/03/14 15:08, Laurent Pinchart wrote:
> On Thursday 27 March 2014 14:57:56 Sylwester Nawrocki wrote:
>> On 27/03/14 14:24, Laurent Pinchart wrote:
>>> On Thursday 27 March 2014 13:16:19 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>
>>>> ---
>> [...]
>>>>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
>>>>  drivers/base/dd.c                                  |    7 ++
>>>>  drivers/clk/Makefile                               |    1 +
>>>>  drivers/clk/clk-conf.c                             |   87 ++++++++++++++
>>>>  drivers/clk/clk.c                                  |   10 ++-
>>>>  include/linux/clk/clk-conf.h                       |   19 +++++
>>>>  6 files changed, 149 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
>>>> 7c52c29..b452f80 100644
>>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> @@ -115,3 +115,29 @@ 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.
>>>
>>> I'm curious, what should happen when two devices have conflicting
>>> requirements ? If a different device required the <&clkcon 3> parent to
>>> be set to <&pll 2> for instance, who should win ? Shouldn't a warning be
>>> printed ?
>>
>> In general, the assumption is that the <&clkcon 3> clock would be used only
>> by the uart@a000 device.
> 
> OK. Removing the problem is a simple way to fix it :-) What about stating this 
> explicitly in the documentation then ? Maybe by prefixing your proposed 
> explanation below with something like
> 
> "Configuring a clock parent and rate through the device node that uses the 
> clock is only supported for clocks that have a single user."

Looks good, we could add it. Or perhaps something like:

"Configuring a clock parent and rate through the device node that uses the
 clock should be only done for clocks that have a single user. If a clock
 is shared and conflicting parent or rate configuration is specified in
 multiple consumer nodes a resulting configuration is undefined." ?

Not sure if it is acceptable to inject such an unpredictability to
the kernel from DT though. Might be more reasonable to go with
a clarification as you proposed.

>> If a clock is shared I'd say it shouldn't be put in a multiple consumer
>> device nodes. Instead it should be put in a clock provider node, as I was
>> trying to explain in the sentence below.
>>
>> A warning could be useful, but it could complicate the code. We would need,
>> for example, to store information about already configured clocks in a list
>> and scan it before actually altering any clock parent or rate.
> 
> I'm fine with implementing that later if needed, we can keep the initial 
> implementation simple.

OK.
>>>> +For clocks which are not directly connected to any consumer device
>>>> similarly
>>>> +clocks, clock-parents and/or clock-rates properties should be specified
>>>> in
>>>> +assigned-clocks subnode of a clock controller DT node.
>>>
>>> It might be that I'm not familiar enough with the clock framework, but
>>> this sounds unclear to me. I'm not sure what you mean exactly.
>>
>> Sorry about not being precise here, would something like below be more
>> clear ?
>>
>> "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>;
>>         };
>>     };
>> "
> 
> That's clearer indeed. Can the parents and rates depend on the board, or on 
> the SoC only ? We might be getting dangerously close to specifying platform 
> configuration instead of describing the hardware. A real example might be nice 
> to support the discussion.

The clock parent and rates could be board specific, otherwise this API would
become much less useful. The configuration often depends on what external
devices are attached to an SoC.

I don't have a real life example for the "global" configuration of shared
clocks at the moment. I added this after seeing Tero's patches [1], maybe
he could talk about some real use cases (just realized I missed to Cc him.
fixing this mistake now).

>> Naturally it's this just an RFC, any critics or suggestions are welcome.:)

--
Regards,
Sylwester

[1] http://www.spinics.net/lists/linux-omap/msg103069.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sylwester,

On Thursday, March 27, 2014 01:16:19 PM 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 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   |   26 ++++++
>  drivers/base/dd.c                                  |    7 ++
>  drivers/clk/Makefile                               |    1 +
>  drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
>  drivers/clk/clk.c                                  |   10 ++-
>  include/linux/clk/clk-conf.h                       |   19 +++++
>  6 files changed, 149 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 7c52c29..b452f80 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -115,3 +115,29 @@ 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.
> +
> +For clocks which are not directly connected to any consumer device similarly
> +clocks, clock-parents and/or clock-rates properties should be specified in
> +assigned-clocks subnode of a clock controller DT node.
> 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 a367a98..c720e4b 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -8,6 +8,7 @@ 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
> +obj-$(CONFIG_COMMON_CLK)	+= clk-conf.o
>  
>  # 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 29dc1e7..240d776 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>
> @@ -2604,7 +2605,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;
> @@ -2619,7 +2628,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..f06b1c2
> --- /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
> +int of_clk_device_init(struct device_node *node)

It causes build brake with s3c6400_defconfig.

It should be marked with 'static inline' and clk-conf.c should be build
only for CONFIG_OF=y.

> +{
> +	return 0;
> +}
> +#endif

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sascha,

On Thursday 27 March 2014 15:47:12 Sascha Hauer wrote:
> On Thu, Mar 27, 2014 at 03:08:10PM +0100, Laurent Pinchart wrote:

[snip]

> > That's clearer indeed. Can the parents and rates depend on the board, or
> > on the SoC only ? We might be getting dangerously close to specifying
> > platform configuration instead of describing the hardware. A real example
> > might be nice to support the discussion.
> 
> This patch comes just at the right time. This is what I do with it:
> 
> #define cko1_sel 57
> #define pll4_audio_div 203
> #define pll4_audio 173
> #define ssi3_sel 47
> 
> &clks {
> 	assigned-clocks {
> 		clocks = <&clks cko1_sel>, <&clks ssi3_sel>, <&clks pll4_audio>;
> 		clock-parents = <&clks pll4_audio_div>, <&clks pll4_audio_div>, <0>;
> 		clock-rates = <0>, <0>, <786432000>;
> 	};
> };
> 
> cko1_sel is a clock that can be routed out of the SoC. In my case it is
> connected the sysclk of an external Audio Codec. ssi3_sel drives my SoC
> internal I2S unit which I use in master mode. The above makes sure that
> the I2S unit and the the external codec both get their clock from the
> audio PLL. The audio PLL is configured to a rate of 786432000Hz which
> is an exact multiple of the desired audio clock.

Thank you for the example.

Are the cko1_sel and ssi3_sel used only by the external audio codec and 
internal I2S unit respectively ? If so, it might make sense to move the 
configuration of their parent to the audio codec and I2S unit DT nodes. 
However, grouping the parent configuration and the pll4 rate configuration in 
a single place makes sense as well. Guidelines are probably needed.

I get a slight feeling of uneasiness about this, probably because we're at the 
boundary between hardware description and system configuration. Encoding in DT 
that "for this particular board this particular clock must be configured this 
particular way" sounds fine to me, but we need to make sure it won't turn to 
software-driven rather than hardware-driven use case descriptions.

Hi Sylwester,

On Thursday 27 March 2014 16:02:52 Sylwester Nawrocki wrote:
> On 27/03/14 15:08, Laurent Pinchart wrote:
> > On Thursday 27 March 2014 14:57:56 Sylwester Nawrocki wrote:
> >> On 27/03/14 14:24, Laurent Pinchart wrote:
> >>> On Thursday 27 March 2014 13:16:19 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>
> >>>> ---
> >> 
> >> [...]
> >> 
> >>>>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
> >>>>  drivers/base/dd.c                                  |    7 ++
> >>>>  drivers/clk/Makefile                               |    1 +
> >>>>  drivers/clk/clk-conf.c                             |   87 ++++++++++++
> >>>>  drivers/clk/clk.c                                  |   10 ++-
> >>>>  include/linux/clk/clk-conf.h                       |   19 +++++
> >>>>  6 files changed, 149 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
> >>>> 7c52c29..b452f80 100644
> >>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>> @@ -115,3 +115,29 @@ 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.
> >>> 
> >>> I'm curious, what should happen when two devices have conflicting
> >>> requirements ? If a different device required the <&clkcon 3> parent to
> >>> be set to <&pll 2> for instance, who should win ? Shouldn't a warning be
> >>> printed ?
> >> 
> >> In general, the assumption is that the <&clkcon 3> clock would be used
> >> only by the uart@a000 device.
> > 
> > OK. Removing the problem is a simple way to fix it :-) What about stating
> > this explicitly in the documentation then ? Maybe by prefixing your
> > proposed explanation below with something like
> > 
> > "Configuring a clock parent and rate through the device node that uses the
> > clock is only supported for clocks that have a single user."
> 
> Looks good, we could add it. Or perhaps something like:
> 
> "Configuring a clock parent and rate through the device node that uses the
>  clock should be only done for clocks that have a single user. If a clock
>  is shared and conflicting parent or rate configuration is specified in
>  multiple consumer nodes a resulting configuration is undefined." ?
> 
> Not sure if it is acceptable to inject such an unpredictability to the
> kernel from DT though. Might be more reasonable to go with a clarification
> as you proposed.

I would go further and forbid it.

"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."

> >> If a clock is shared I'd say it shouldn't be put in a multiple consumer
> >> device nodes. Instead it should be put in a clock provider node, as I was
> >> trying to explain in the sentence below.
> >> 
> >> A warning could be useful, but it could complicate the code. We would
> >> need, for example, to store information about already configured clocks
> >> in a list and scan it before actually altering any clock parent or rate.
> > 
> > I'm fine with implementing that later if needed, we can keep the initial
> > implementation simple.
> 
> OK.
> 
> >>>> +For clocks which are not directly connected to any consumer device
> >>>> similarly
> >>>> +clocks, clock-parents and/or clock-rates properties should be
> >>>> specified in
> >>>> +assigned-clocks subnode of a clock controller DT node.
> >>> 
> >>> It might be that I'm not familiar enough with the clock framework, but
> >>> this sounds unclear to me. I'm not sure what you mean exactly.
> >> 
> >> Sorry about not being precise here, would something like below be more
> >> clear ?
> >> 
> >> "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>;
> >>         };
> >>     };
> >> "
> > 
> > That's clearer indeed. Can the parents and rates depend on the board, or
> > on the SoC only ? We might be getting dangerously close to specifying
> > platform configuration instead of describing the hardware. A real example
> > might be nice to support the discussion.
> 
> The clock parent and rates could be board specific, otherwise this API would
> become much less useful. The configuration often depends on what external
> devices are attached to an SoC.
> 
> I don't have a real life example for the "global" configuration of shared
> clocks at the moment. I added this after seeing Tero's patches [1], maybe
> he could talk about some real use cases (just realized I missed to Cc him.
> fixing this mistake now).

Sascha has posted a nice example of an assigned-clocks node, let's discuss 
this in replies to his e-mail.

> >> Naturally it's this just an RFC, any critics or suggestions are
> >> welcome.:)
>
> [1] http://www.spinics.net/lists/linux-omap/msg103069.html

On Fri, Mar 28, 2014 at 05:44:17PM +0100, Laurent Pinchart wrote:
> Hi Sascha,
> 
> On Thursday 27 March 2014 15:47:12 Sascha Hauer wrote:
> > On Thu, Mar 27, 2014 at 03:08:10PM +0100, Laurent Pinchart wrote:
> 
> [snip]
> 
> > > That's clearer indeed. Can the parents and rates depend on the board, or
> > > on the SoC only ? We might be getting dangerously close to specifying
> > > platform configuration instead of describing the hardware. A real example
> > > might be nice to support the discussion.
> > 
> > This patch comes just at the right time. This is what I do with it:
> > 
> > #define cko1_sel 57
> > #define pll4_audio_div 203
> > #define pll4_audio 173
> > #define ssi3_sel 47
> > 
> > &clks {
> > 	assigned-clocks {
> > 		clocks = <&clks cko1_sel>, <&clks ssi3_sel>, <&clks pll4_audio>;
> > 		clock-parents = <&clks pll4_audio_div>, <&clks pll4_audio_div>, <0>;
> > 		clock-rates = <0>, <0>, <786432000>;
> > 	};
> > };
> > 
> > cko1_sel is a clock that can be routed out of the SoC. In my case it is
> > connected the sysclk of an external Audio Codec. ssi3_sel drives my SoC
> > internal I2S unit which I use in master mode. The above makes sure that
> > the I2S unit and the the external codec both get their clock from the
> > audio PLL. The audio PLL is configured to a rate of 786432000Hz which
> > is an exact multiple of the desired audio clock.
> 
> Thank you for the example.
> 
> Are the cko1_sel and ssi3_sel used only by the external audio codec and 
> internal I2S unit respectively ? If so, it might make sense to move the 
> configuration of their parent to the audio codec and I2S unit DT nodes. 
> However, grouping the parent configuration and the pll4 rate configuration in 
> a single place makes sense as well. Guidelines are probably needed.

I didn't bother much to find the right place for the nodes. It indeed
might make sense to put them under the I2S unit and the codec. However,
the clock-rate is a shared property between the I2S unit and the codec
which probably should better be placed under the block which provides
the clocks.

> 
> I get a slight feeling of uneasiness about this, probably because we're at the 
> boundary between hardware description and system configuration. Encoding in DT 
> that "for this particular board this particular clock must be configured this 
> particular way" sounds fine to me, but we need to make sure it won't turn to 
> software-driven rather than hardware-driven use case descriptions.

I agree this is in the grey area between hardware and software
description. At least on i.MX it happens with audio and video that
totally unrelated units share a clock. Often it's next to impossible to
find an algorithm that configures the clocks correctly without the help
of hardcoded assumptions about parents and rates. I find specifying this
in the devicetree much more convenient than writing board specific code
each time.

Sascha

Hi Laurent,

On 28/03/14 17:49, Laurent Pinchart wrote:
> On Thursday 27 March 2014 16:02:52 Sylwester Nawrocki wrote:
>> > On 27/03/14 15:08, Laurent Pinchart wrote:
>>> > > On Thursday 27 March 2014 14:57:56 Sylwester Nawrocki wrote:
>>>> > >> On 27/03/14 14:24, Laurent Pinchart wrote:
>>>>> > >>> On Thursday 27 March 2014 13:16:19 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>
>>>>>> > >>>> ---
>>>> > >> 
>>>> > >> [...]
>>>> > >> 
>>>>>> > >>>>  .../devicetree/bindings/clock/clock-bindings.txt   |   26 ++++++
>>>>>> > >>>>  drivers/base/dd.c                                  |    7 ++
>>>>>> > >>>>  drivers/clk/Makefile                               |    1 +
>>>>>> > >>>>  drivers/clk/clk-conf.c                             |   87 ++++++++++++
>>>>>> > >>>>  drivers/clk/clk.c                                  |   10 ++-
>>>>>> > >>>>  include/linux/clk/clk-conf.h                       |   19 +++++
>>>>>> > >>>>  6 files changed, 149 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
>>>>>> > >>>> 7c52c29..b452f80 100644
>>>>>> > >>>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>> > >>>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>> > >>>> @@ -115,3 +115,29 @@ 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.
>>>>> > >>> 
>>>>> > >>> I'm curious, what should happen when two devices have conflicting
>>>>> > >>> requirements ? If a different device required the <&clkcon 3> parent to
>>>>> > >>> be set to <&pll 2> for instance, who should win ? Shouldn't a warning be
>>>>> > >>> printed ?
>>>> > >> 
>>>> > >> In general, the assumption is that the <&clkcon 3> clock would be used
>>>> > >> only by the uart@a000 device.
>>> > > 
>>> > > OK. Removing the problem is a simple way to fix it :-) What about stating
>>> > > this explicitly in the documentation then ? Maybe by prefixing your
>>> > > proposed explanation below with something like
>>> > > 
>>> > > "Configuring a clock parent and rate through the device node that uses the
>>> > > clock is only supported for clocks that have a single user."
>> > 
>> > Looks good, we could add it. Or perhaps something like:
>> > 
>> > "Configuring a clock parent and rate through the device node that uses the
>> >  clock should be only done for clocks that have a single user. If a clock
>> >  is shared and conflicting parent or rate configuration is specified in
>> >  multiple consumer nodes a resulting configuration is undefined." ?
>> > 
>> > Not sure if it is acceptable to inject such an unpredictability to the
>> > kernel from DT though. Might be more reasonable to go with a clarification
>> > as you proposed.
>
> I would go further and forbid it.
> 
> "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."

Sounds good to me, I'll add it in next version of the patch. Thanks.

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Sascha,

On Monday 31 March 2014 10:32:10 Sascha Hauer wrote:
> On Fri, Mar 28, 2014 at 05:44:17PM +0100, Laurent Pinchart wrote:
> > On Thursday 27 March 2014 15:47:12 Sascha Hauer wrote:
> > > On Thu, Mar 27, 2014 at 03:08:10PM +0100, Laurent Pinchart wrote:
> > [snip]
> > 
> > > > That's clearer indeed. Can the parents and rates depend on the board,
> > > > or on the SoC only ? We might be getting dangerously close to
> > > > specifying platform configuration instead of describing the hardware.
> > > > A real example might be nice to support the discussion.
> > > 
> > > This patch comes just at the right time. This is what I do with it:
> > > 
> > > #define cko1_sel 57
> > > #define pll4_audio_div 203
> > > #define pll4_audio 173
> > > #define ssi3_sel 47
> > > 
> > > &clks {
> > > 	assigned-clocks {
> > > 		clocks = <&clks cko1_sel>, <&clks ssi3_sel>, <&clks pll4_audio>;
> > > 		clock-parents = <&clks pll4_audio_div>, <&clks pll4_audio_div>, 
<0>;
> > > 		clock-rates = <0>, <0>, <786432000>;
> > > 	};
> > > };
> > > 
> > > cko1_sel is a clock that can be routed out of the SoC. In my case it is
> > > connected the sysclk of an external Audio Codec. ssi3_sel drives my SoC
> > > internal I2S unit which I use in master mode. The above makes sure that
> > > the I2S unit and the the external codec both get their clock from the
> > > audio PLL. The audio PLL is configured to a rate of 786432000Hz which
> > > is an exact multiple of the desired audio clock.
> > 
> > Thank you for the example.
> > 
> > Are the cko1_sel and ssi3_sel used only by the external audio codec and
> > internal I2S unit respectively ? If so, it might make sense to move the
> > configuration of their parent to the audio codec and I2S unit DT nodes.
> > However, grouping the parent configuration and the pll4 rate configuration
> > in a single place makes sense as well. Guidelines are probably needed.
>
> I didn't bother much to find the right place for the nodes. It indeed might
> make sense to put them under the I2S unit and the codec. However, the clock-
> rate is a shared property between the I2S unit and the codec which probably
> should better be placed under the block which provides the clocks.
> 
> > I get a slight feeling of uneasiness about this, probably because we're at
> > the boundary between hardware description and system configuration.
> > Encoding in DT that "for this particular board this particular clock must
> > be configured this particular way" sounds fine to me, but we need to make
> > sure it won't turn to software-driven rather than hardware-driven use
> > case descriptions.
>
> I agree this is in the grey area between hardware and software description.
> At least on i.MX it happens with audio and video that totally unrelated
> units share a clock. Often it's next to impossible to find an algorithm that
> configures the clocks correctly without the help of hardcoded assumptions
> about parents and rates. I find specifying this in the devicetree much more
> convenient than writing board specific code each time.

I totally agree with you. The approach makes sense, I just wanted to point out 
that this is a grey area and that we should be aware of it when designing the 
DT bindings.