[RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Sylwester Nawrocki]

This patch set adds a DT binding documentation for new 'clock-parents'
and 'clock-rates' DT properties and a helper function to parse them.
The helper is now being called from within the driver core, similarly
as it is done for the pins configuration binding to a device.

Patch 1/2 adds a variant of of_clk_get() function which accepts name of
a DT property containing list of phandle + clock specifier pairs, as
opposed to hard coded "clocks" property name in of_clk_get().
As Mike suggested I've renamed this function to of_clk_get_by_property().

Patch 2/2 actually adds the code searching for related DT properties at
device node and performing re-parenting and/or clock frequency setting
as specified.

Changes since v1:
 - updated DT binding documentation,
 - dropped the platform bus notifier, the clock setup routine is now
   being called directly from the driver core before a driver probe() call;
   this has an advantage such as all bus types are handled and any errors
   are propagated, so that, for instance a driver probe() can be deferred
   also when resources specified by clock-parents/clock-rates properties
   are not yet available; an alternative would be to let drivers call
   of_clk_device_setup() directly,
 - dropped the patch adding a macro definition for maximum DT property
   name length for now.

Open issues:
 - handling of errors from of_clk_get_by_property() could be improved,
   currently ENOENT is returned by this function not only for a null
   entry.

This series has been tested on ARM, on Exynos4412 Trats2 board.
RFC v1 can be found at:
http://www.spinics.net/lists/arm-kernel/msg309241.html

Sylwester Nawrocki (2):
  clk: Add function parsing arbitrary clock list DT property
  clk: Add handling of clk parent and rate assigned from DT

 .../devicetree/bindings/clock/clock-bindings.txt   |   23 ++++++
 drivers/base/dd.c                                  |    5 ++
 drivers/clk/clk.c                                  |   77 ++++++++++++++++++++
 drivers/clk/clk.h                                  |    3 +
 drivers/clk/clkdev.c                               |   25 ++++++-
 include/linux/clk-provider.h                       |    6 ++
 6 files changed, 135 insertions(+), 4 deletions(-)

--
1.7.9.5

[RFC PATCH v2 1/2] clk: Add function parsing arbitrary clock list DT property

[Sylwester Nawrocki]

The of_clk_get_by_property() function added by this patch is similar to
of_clk_get(), except it allows to pass name of a DT property containing
list of phandles and clock specifiers. For of_clk_get() this has been
hard coded to "clocks".

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
 - s/of_clk_get_list_entry/of_clk_get_by_property.
---
 drivers/clk/clk.h    |    3 +++
 drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 795cc9f..666bc50 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -10,7 +10,10 @@
  */
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
 void of_clk_lock(void);
 void of_clk_unlock(void);
+struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index);
 #endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index a360b2e..1d41540 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -27,17 +27,28 @@ static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-struct clk *of_clk_get(struct device_node *np, int index)
+/**
+ * of_clk_get_by_property() - Parse and lookup a clock referenced by a device node
+ * @np: pointer to clock consumer node
+ * @list_name: name of the clock list property
+ * @index: index to the clock list
+ *
+ * This function parses the @list_name property and together with @index
+ * value indicating an entry of the list uses it to look up the struct clk
+ * from the registered list of clock providers.
+ */
+struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index)
 {
 	struct of_phandle_args clkspec;
 	struct clk *clk;
 	int rc;
 
-	if (index < 0)
+	if (index < 0 || !list_name)
 		return ERR_PTR(-EINVAL);
 
-	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
-					&clkspec);
+	rc = of_parse_phandle_with_args(np, list_name, "#clock-cells",
+					index, &clkspec);
 	if (rc)
 		return ERR_PTR(rc);
 
@@ -51,6 +62,12 @@ struct clk *of_clk_get(struct device_node *np, int index)
 	of_node_put(clkspec.np);
 	return clk;
 }
+EXPORT_SYMBOL(of_clk_get_by_property);
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+	return of_clk_get_by_property(np, "clocks", index);
+}
 EXPORT_SYMBOL(of_clk_get);
 
 /**
-- 
1.7.9.5

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

[Sylwester Nawrocki]

This function adds a notifier callback run before a driver is bound
to a device. It will configure any parent clocks and clock frequencies
according to values of 'clock-parents' and 'clock-rates' DT properties
respectively.

Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
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   |   23 ++++++
 drivers/base/dd.c                                  |    5 ++
 drivers/clk/clk.c                                  |   77 ++++++++++++++++++++
 include/linux/clk-provider.h                       |    6 ++
 4 files changed, 111 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 7c52c29..eb8d547 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -115,3 +115,26 @@ 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 configuration of (parts of) the clock controller
+often determined by the board design. Such a configuration can be specified in
+a clock consumer node through clock-parents and clock-rates DT properties.
+The former should contain 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.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0605176..f0cbac1 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-provider.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -278,6 +279,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		goto probe_failed;
 
+	ret = of_clk_device_setup(dev);
+	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/clk.c b/drivers/clk/clk.c
index 19f6f3f..6fdc49b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2528,6 +2528,83 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
 
 /**
+ * of_clk_device_setup() - 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_setup(struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	struct property	*prop;
+	const __be32 *cur;
+	int rc, index, num_parents;
+	struct clk *clk, *pclk;
+	u32 rate;
+
+	if (!node)
+		return 0;
+
+	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;
+}
+
+/**
  * of_clk_init() - Scan and init clock providers from the DT
  * @matches: array of compatible values and init functions for providers.
  *
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 939533d..2cd0fbb 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -509,6 +509,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);
 
+int of_clk_device_setup(struct device *dev);
 #else /* !CONFIG_OF */
 
 static inline int of_clk_add_provider(struct device_node *np,
@@ -537,6 +538,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np,
 }
 #define of_clk_init(matches) \
 	{ while (0); }
+
+int of_clk_device_setup(struct device *dev)
+{
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 /*
-- 
1.7.9.5

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

Re: [RFC PATCH v2 2/2] clk: Add handling of clk parent and rate assigned from DT

[Sylwester Nawrocki]

On 03/03/14 19:22, Sylwester Nawrocki wrote:
> This function adds a notifier callback run before a driver is bound
> to a device. It will configure any parent clocks and clock frequencies
> according to values of 'clock-parents' and 'clock-rates' DT properties
> respectively.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> 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   |   23 ++++++
>  drivers/base/dd.c                                  |    5 ++
>  drivers/clk/clk.c                                  |   77 ++++++++++++++++++++
>  include/linux/clk-provider.h                       |    6 ++
>  4 files changed, 111 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 7c52c29..eb8d547 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -115,3 +115,26 @@ 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 configuration of (parts of) the clock controller
> +often determined by the board design. Such a configuration can be specified in
> +a clock consumer node through clock-parents and clock-rates DT properties.
> +The former should contain 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>;

I have some doubts here, i.e. the order in which clocks are being 
configured may be important in some cases. Should the binding then be
specifying that the clocks will be configured in a sequence exactly 
as listed in the clock-parents property ?

E.g. consider part of a clock controller where one of frequencies fed to
a consumer device cannot exceed given value:

                mux1
 200 MHz   0 .--------.
----->-------|--.     |
             |   \____|__                    f1 
 400 MHz   1 |        |  `-+------------------->--
----->-------|-       |    |
             '--------'	   |	  mux2
                           | 0 .---------.
                           `---|--.      |   f2
                               |   \_____|_,---->--
                 100 MHz     1 |         |   (max. 200 MHz)
                 ----->--------|         | 
                               '---------'		

In this case we want to set frequency f1 to 400 MHz and f2 to 100 MHz.
To ensure f2 doesn't exceed 200 MHz at any time, mux2 has to be switched 
to position '1' first and then mux 1 to position '1'.

> +        clock-rates = <460800>;

For clock-rates it's a bit more complicated, since it might require
setting up frequency of some clocks twice - first to a low and then 
to a higher value. Such details could likely be handled by bindings 
of individual devices. Also we could assume the clock tree 
(re)configuration is being done when any consumer clocks are masked 
at the consumer clock gates.

I'm no sure if we should sort the clocks to ensure any parents are set
before the child clocks, or should we rely on the sequence specified 
in devicetree ? I'd assume sorting it wouldn't hurt, there should not 
be relatively many clocks in a single dt node.

> +    };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.

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

Re: [RFC PATCH v2 2/2] clk: Add handling of clk parent and rate assigned from DT

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-03-25 04:19:42)
> On 03/03/14 19:22, Sylwester Nawrocki wrote:
> > This function adds a notifier callback run before a driver is bound
> > to a device. It will configure any parent clocks and clock frequencies
> > according to values of 'clock-parents' and 'clock-rates' DT properties
> > respectively.
> > 
> > Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> > 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   |   23 ++++++
> >  drivers/base/dd.c                                  |    5 ++
> >  drivers/clk/clk.c                                  |   77 ++++++++++++++++++++
> >  include/linux/clk-provider.h                       |    6 ++
> >  4 files changed, 111 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index 7c52c29..eb8d547 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -115,3 +115,26 @@ 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 configuration of (parts of) the clock controller
> > +often determined by the board design. Such a configuration can be specified in
> > +a clock consumer node through clock-parents and clock-rates DT properties.
> > +The former should contain 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>;
> 
> I have some doubts here, i.e. the order in which clocks are being 
> configured may be important in some cases. Should the binding then be
> specifying that the clocks will be configured in a sequence exactly 
> as listed in the clock-parents property ?

That's a good point, and I think we should re-examine the role of a DT
binding for this purpose. From my limited experience with DT, it seems
to be really bad at anything involving sequencing. It doesn't give us
function pointers/callbacks in the way that the old board files used to.

So I think the binding you proposed is still a good idea, but only for
the very simple case. If your platform has some detailed integration
requirements that have corner cases like you describe below, then this
DT binding might not be a good place to put the info.

A platform that provides its own pm runtime backend could neatly manage
this by having a call to pm_runtime_get deal with these integration
details such that the driver does not have to be aware of them. (caveat:
that assumes in the example below that the only thing you want to do is
set up your clocks once and then never touch them again)

Regards,
Mike

> 
> E.g. consider part of a clock controller where one of frequencies fed to
> a consumer device cannot exceed given value:
> 
>                 mux1
>  200 MHz   0 .--------.
> ----->-------|--.     |
>              |   \____|__                    f1 
>  400 MHz   1 |        |  `-+------------------->--
> ----->-------|-       |    |
>              '--------'    |      mux2
>                            | 0 .---------.
>                            `---|--.      |   f2
>                                |   \_____|_,---->--
>                  100 MHz     1 |         |   (max. 200 MHz)
>                  ----->--------|         | 
>                                '---------'              
> 
> In this case we want to set frequency f1 to 400 MHz and f2 to 100 MHz.
> To ensure f2 doesn't exceed 200 MHz at any time, mux2 has to be switched 
> to position '1' first and then mux 1 to position '1'.
> 
> > +        clock-rates = <460800>;
> 
> For clock-rates it's a bit more complicated, since it might require
> setting up frequency of some clocks twice - first to a low and then 
> to a higher value. Such details could likely be handled by bindings 
> of individual devices. Also we could assume the clock tree 
> (re)configuration is being done when any consumer clocks are masked 
> at the consumer clock gates.
> 
> I'm no sure if we should sort the clocks to ensure any parents are set
> before the child clocks, or should we rely on the sequence specified 
> in devicetree ? I'd assume sorting it wouldn't hurt, there should not 
> be relatively many clocks in a single dt node.
> 
> > +    };
> > +
> > +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> > +clock is specified as 460800 Hz.
> 
> --
> Thanks,
> Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 2/2] clk: Add handling of clk parent and rate assigned from DT

[Sylwester Nawrocki]

Hi Mike,

On 25/03/14 23:54, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-03-25 04:19:42)
>> > On 03/03/14 19:22, Sylwester Nawrocki wrote:
[...]
>>> > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt 
>>> > > b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > > index 7c52c29..eb8d547 100644
>>> > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> > > @@ -115,3 +115,26 @@ 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 configuration of (parts of) the clock controller
>>> > > +often determined by the board design. Such a configuration can be specified in
>>> > > +a clock consumer node through clock-parents and clock-rates DT properties.
>>> > > +The former should contain 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>;
>> > 
>> > I have some doubts here, i.e. the order in which clocks are being 
>> > configured may be important in some cases. Should the binding then be
>> > specifying that the clocks will be configured in a sequence exactly 
>> > as listed in the clock-parents property ?
>
> That's a good point, and I think we should re-examine the role of a DT
> binding for this purpose. From my limited experience with DT, it seems
> to be really bad at anything involving sequencing. It doesn't give us
> function pointers/callbacks in the way that the old board files used to.
>
> So I think the binding you proposed is still a good idea, but only for
> the very simple case. If your platform has some detailed integration
> requirements that have corner cases like you describe below, then this
> DT binding might not be a good place to put the info.
> 
> A platform that provides its own pm runtime backend could neatly manage
> this by having a call to pm_runtime_get deal with these integration
> details such that the driver does not have to be aware of them. (caveat:
> that assumes in the example below that the only thing you want to do is
> set up your clocks once and then never touch them again)

This sounds reasonable, my impression was also that devicetree might be 
not the best place to put such a sequencing information.

We would just provide a one-time configuration data, without an indication 
of how the configuration should be performed when there are multiple 
clocks listed. As an usual DT practice the order of configuration wouldn't
be defined by the binding.

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

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Maxime Coquelin]

Hi Sylwester,

	I like the principle of your implementation, but I have two questions:
	1 - How can we manage PM with this solution, as the parent/rate will be 
set only once at probe time?
	2 - How to set the parent of a parent clock (which can be shared with 
other devices)? Same question about the parent rates.


Thanks,
Maxime

On 03/03/2014 07:15 PM, Sylwester Nawrocki wrote:
> This patch set adds a DT binding documentation for new 'clock-parents'
> and 'clock-rates' DT properties and a helper function to parse them.
> The helper is now being called from within the driver core, similarly
> as it is done for the pins configuration binding to a device.
>
> Patch 1/2 adds a variant of of_clk_get() function which accepts name of
> a DT property containing list of phandle + clock specifier pairs, as
> opposed to hard coded "clocks" property name in of_clk_get().
> As Mike suggested I've renamed this function to of_clk_get_by_property().
>
> Patch 2/2 actually adds the code searching for related DT properties at
> device node and performing re-parenting and/or clock frequency setting
> as specified.
>
> Changes since v1:
>   - updated DT binding documentation,
>   - dropped the platform bus notifier, the clock setup routine is now
>     being called directly from the driver core before a driver probe() call;
>     this has an advantage such as all bus types are handled and any errors
>     are propagated, so that, for instance a driver probe() can be deferred
>     also when resources specified by clock-parents/clock-rates properties
>     are not yet available; an alternative would be to let drivers call
>     of_clk_device_setup() directly,
>   - dropped the patch adding a macro definition for maximum DT property
>     name length for now.
>
> Open issues:
>   - handling of errors from of_clk_get_by_property() could be improved,
>     currently ENOENT is returned by this function not only for a null
>     entry.
>
> This series has been tested on ARM, on Exynos4412 Trats2 board.
> RFC v1 can be found at:
> http://www.spinics.net/lists/arm-kernel/msg309241.html
>
> Sylwester Nawrocki (2):
>    clk: Add function parsing arbitrary clock list DT property
>    clk: Add handling of clk parent and rate assigned from DT
>
>   .../devicetree/bindings/clock/clock-bindings.txt   |   23 ++++++
>   drivers/base/dd.c                                  |    5 ++
>   drivers/clk/clk.c                                  |   77 ++++++++++++++++++++
>   drivers/clk/clk.h                                  |    3 +
>   drivers/clk/clkdev.c                               |   25 ++++++-
>   include/linux/clk-provider.h                       |    6 ++
>   6 files changed, 135 insertions(+), 4 deletions(-)
>
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Sylwester Nawrocki]

Hi Maxime,

On 06/03/14 14:45, Maxime Coquelin wrote:
> Hi Sylwester,
> 
> 	I like the principle of your implementation, but I have two questions:
> 	1 - How can we manage PM with this solution, as the parent/rate will be 
> set only once at probe time?
> 	2 - How to set the parent of a parent clock (which can be shared with 
> other devices)? Same question about the parent rates.

Thanks for your feedback and apologies for late reply.

IIUC your first concern is about a situation when clocks need to be
reconfigured upon each resume from system sleep or runtime PM resume ?
As I mentioned in v1 of the RFC I was considering having individual
drivers calling explicitly the clocks set up routine. Presumably this
would allow to resolve the power management related issue.
One example I'm aware the approach as in this RFC wouldn't work is
when a device in a SoC belongs to a power domain, which needs to be
first switched on before we can start setting up and the clocks'
configuration get lost after the power domain switch off.

OTOH I suspect devices for which one-time clocks setup is sufficient
will be quite common. And for these there would need to be a single
call to the setup routine in probe() I guess, since it wouldn't be
possible to figure out just from the DT data when the actual call
should be made.

For a global clocks configuration, I thought about specifying that
in the clocks controller node, and then to have the setup routine
called e.g. from of_clk_init(). I think that could work well enough,
together with the patch [1], adding clock dependencies handling.
But then the clock frequency set up function would need to be
modified to respect the clock parent relationships, similarly as
in patch series [2]. A just noticed [2] recently, after posting
this RFC (adding Tero at Cc).

--
Regards,
Sylwester

[1] http://www.spinics.net/lists/arm-kernel/msg310507.html
[2] 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-03-20 05:42:33)
> Hi Maxime,
> 
> On 06/03/14 14:45, Maxime Coquelin wrote:
> > Hi Sylwester,
> > 
> >       I like the principle of your implementation, but I have two questions:
> >       1 - How can we manage PM with this solution, as the parent/rate will be 
> > set only once at probe time?
> >       2 - How to set the parent of a parent clock (which can be shared with 
> > other devices)? Same question about the parent rates.
> 
> Thanks for your feedback and apologies for late reply.
> 
> IIUC your first concern is about a situation when clocks need to be
> reconfigured upon each resume from system sleep or runtime PM resume ?
> As I mentioned in v1 of the RFC I was considering having individual
> drivers calling explicitly the clocks set up routine. Presumably this
> would allow to resolve the power management related issue.
> One example I'm aware the approach as in this RFC wouldn't work is
> when a device in a SoC belongs to a power domain, which needs to be
> first switched on before we can start setting up and the clocks'
> configuration get lost after the power domain switch off.

I like Sylwester's approach of handling this one-time setup in the
driver core.

Any kind of fine grained power management should not be hidden by DT,
and by definition that logic belongs in the device driver. It can still
be nicely abstracted away by runtime pm[1].

Regards,
Mike

[1] Message-ID: <20140320114238.GQ7528-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>

> 
> OTOH I suspect devices for which one-time clocks setup is sufficient
> will be quite common. And for these there would need to be a single
> call to the setup routine in probe() I guess, since it wouldn't be
> possible to figure out just from the DT data when the actual call
> should be made.
> 
> For a global clocks configuration, I thought about specifying that
> in the clocks controller node, and then to have the setup routine
> called e.g. from of_clk_init(). I think that could work well enough,
> together with the patch [1], adding clock dependencies handling.
> But then the clock frequency set up function would need to be
> modified to respect the clock parent relationships, similarly as
> in patch series [2]. A just noticed [2] recently, after posting
> this RFC (adding Tero at Cc).
> 
> --
> Regards,
> Sylwester
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html
> [2] 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Maxime Coquelin]

Hi Mike,

On 03/21/2014 02:45 AM, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-03-20 05:42:33)
>> Hi Maxime,
>>
>> On 06/03/14 14:45, Maxime Coquelin wrote:
>>> Hi Sylwester,
>>>
>>>        I like the principle of your implementation, but I have two questions:
>>>        1 - How can we manage PM with this solution, as the parent/rate will be
>>> set only once at probe time?
>>>        2 - How to set the parent of a parent clock (which can be shared with
>>> other devices)? Same question about the parent rates.
>>
>> Thanks for your feedback and apologies for late reply.
>>
>> IIUC your first concern is about a situation when clocks need to be
>> reconfigured upon each resume from system sleep or runtime PM resume ?
>> As I mentioned in v1 of the RFC I was considering having individual
>> drivers calling explicitly the clocks set up routine. Presumably this
>> would allow to resolve the power management related issue.
>> One example I'm aware the approach as in this RFC wouldn't work is
>> when a device in a SoC belongs to a power domain, which needs to be
>> first switched on before we can start setting up and the clocks'
>> configuration get lost after the power domain switch off.
>
> I like Sylwester's approach of handling this one-time setup in the
> driver core.
>
> Any kind of fine grained power management should not be hidden by DT,
> and by definition that logic belongs in the device driver. It can still
> be nicely abstracted away by runtime pm[1].
>
> Regards,
> Mike
>
> [1] Message-ID: <20140320114238.GQ7528@n2100.arm.linux.org.uk>
How can I access this reference?

Thanks,
Maxime

>
>>
>> OTOH I suspect devices for which one-time clocks setup is sufficient
>> will be quite common. And for these there would need to be a single
>> call to the setup routine in probe() I guess, since it wouldn't be
>> possible to figure out just from the DT data when the actual call
>> should be made.
>>
>> For a global clocks configuration, I thought about specifying that
>> in the clocks controller node, and then to have the setup routine
>> called e.g. from of_clk_init(). I think that could work well enough,
>> together with the patch [1], adding clock dependencies handling.
>> But then the clock frequency set up function would need to be
>> modified to respect the clock parent relationships, similarly as
>> in patch series [2]. A just noticed [2] recently, after posting
>> this RFC (adding Tero at Cc).
>>
>> --
>> Regards,
>> Sylwester
>>
>> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html
>> [2] http://www.spinics.net/lists/linux-omap/msg103069.html

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Mike Turquette]

Quoting Maxime Coquelin (2014-03-21 07:09:26)
> Hi Mike,
> 
> On 03/21/2014 02:45 AM, Mike Turquette wrote:
> > Quoting Sylwester Nawrocki (2014-03-20 05:42:33)
> >> Hi Maxime,
> >>
> >> On 06/03/14 14:45, Maxime Coquelin wrote:
> >>> Hi Sylwester,
> >>>
> >>>        I like the principle of your implementation, but I have two questions:
> >>>        1 - How can we manage PM with this solution, as the parent/rate will be
> >>> set only once at probe time?
> >>>        2 - How to set the parent of a parent clock (which can be shared with
> >>> other devices)? Same question about the parent rates.
> >>
> >> Thanks for your feedback and apologies for late reply.
> >>
> >> IIUC your first concern is about a situation when clocks need to be
> >> reconfigured upon each resume from system sleep or runtime PM resume ?
> >> As I mentioned in v1 of the RFC I was considering having individual
> >> drivers calling explicitly the clocks set up routine. Presumably this
> >> would allow to resolve the power management related issue.
> >> One example I'm aware the approach as in this RFC wouldn't work is
> >> when a device in a SoC belongs to a power domain, which needs to be
> >> first switched on before we can start setting up and the clocks'
> >> configuration get lost after the power domain switch off.
> >
> > I like Sylwester's approach of handling this one-time setup in the
> > driver core.
> >
> > Any kind of fine grained power management should not be hidden by DT,
> > and by definition that logic belongs in the device driver. It can still
> > be nicely abstracted away by runtime pm[1].
> >
> > Regards,
> > Mike
> >
> > [1] Message-ID: <20140320114238.GQ7528-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
> How can I access this reference?

I was trying to provide a reference to this thread which is tangentially
related to this topic:

http://www.spinics.net/lists/arm-kernel/msg317016.html

But it hadn't hit any of the mail archives at the time of my writing so
I just copied Russell's Message-ID from his email. If you have a copy of
that email then you can search on Message-ID's for that and you will
find it. :-)

Regards,
Mike

> 
> Thanks,
> Maxime
> 
> >
> >>
> >> OTOH I suspect devices for which one-time clocks setup is sufficient
> >> will be quite common. And for these there would need to be a single
> >> call to the setup routine in probe() I guess, since it wouldn't be
> >> possible to figure out just from the DT data when the actual call
> >> should be made.
> >>
> >> For a global clocks configuration, I thought about specifying that
> >> in the clocks controller node, and then to have the setup routine
> >> called e.g. from of_clk_init(). I think that could work well enough,
> >> together with the patch [1], adding clock dependencies handling.
> >> But then the clock frequency set up function would need to be
> >> modified to respect the clock parent relationships, similarly as
> >> in patch series [2]. A just noticed [2] recently, after posting
> >> this RFC (adding Tero at Cc).
> >>
> >> --
> >> Regards,
> >> Sylwester
> >>
> >> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html
> >> [2] 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Eric Boxer]

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

I've added this to my to-do list. On March 24, 2014 at 4:57:29 PM CDT, Mike Turquette <mturquette@linaro.org> wrote:Quoting Maxime Coquelin (2014-03-21 07:09:26)> Hi Mike,> > On 03/21/2014 02:45 AM, Mike Turquette wrote:> > Quoting Sylwester Nawrocki (2014-03-20 05:42:33)> >> Hi Maxime,> >>> >> On 06/03/14 14:45, Maxime Coquelin wrote:> >>> Hi Sylwester,> >>>> >>> I like the principle of your implementation, but I have two questions:> >>> 1 - How can we manage PM with this solution, as the parent/rate will be> >>> set only once at probe time?> >>> 2 - How to set the parent of a parent clock (which can be shared with> >>> other devices)? Same question about the parent rates.> >>> >> Thanks for your feedback and apologies for late reply.> >>> >> IIUC your first concern is about a situation when clocks need to be> >> reconfigured upon each resume from system sleep or runtime PM resume ?> >> As I mentioned in v1 of the RFC I was considering having individual> >> drivers calling explicitly the clocks set up routine. Presumably this> >> would allow to resolve the power management related issue.> >> One example I'm aware the approach as in this RFC wouldn't work is> >> when a device in a SoC belongs to a power domain, which needs to be> >> first switched on before we can start setting up and the clocks'> >> configuration get lost after the power domain switch off.> >> > I like Sylwester's approach of handling this one-time setup in the> > driver core.> >> > Any kind of fine grained power management should not be hidden by DT,> > and by definition that logic belongs in the device driver. It can still> > be nicely abstracted away by runtime pm[1].> >> > Regards,> > Mike> >> > [1] Message-ID: > How can I access this reference?I was trying to provide a reference to this thread which is tangentiallyrelated to this topic:http://www.spinics.net/lists/arm-kernel/msg317016.htmlBut it hadn't hit any of the mail archives at the time of my writing soI just copied Russell's Message-ID from his email. If you have a copy ofthat email then you can search on Message-ID's for that and you willfind it. :-)Regards,Mike> > Thanks,> Maxime> > >> >>> >> OTOH I suspect devices for which one-time clocks setup is sufficient> >> will be quite common. And for these there would need to be a single> >> call to the setup routine in probe() I guess, since it wouldn't be> >> possible to figure out just from the DT data when the actual call> >> should be made.> >>> >> For a global clocks configuration, I thought about specifying that> >> in the clocks controller node, and then to have the setup routine> >> called e.g. from of_clk_init(). I think that could work well enough,> >> together with the patch [1], adding clock dependencies handling.> >> But then the clock frequency set up function would need to be> >> modified to respect the clock parent relationships, similarly as> >> in patch series [2]. A just noticed [2] recently, after posting> >> this RFC (adding Tero at Cc).> >>> >> --> >> Regards,> >> Sylwester> >>> >> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html> >> [2] http://www.spinics.net/lists/linux-omap/msg103069.html--To unsubscribe from this list: send the line "unsubscribe linux-kernel" inthe body of a message to majordomo@vger.kernel.orgMore majordomo info at http://vger.kernel.org/majordomo-info.htmlPlease read the FAQ at http://www.tux.org/lkml/     

[-- Attachment #2: Type: text/html, Size: 4192 bytes --]

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Maxime Coquelin]

On 03/20/2014 01:42 PM, Sylwester Nawrocki wrote:
> Hi Maxime,
>
> On 06/03/14 14:45, Maxime Coquelin wrote:
>> Hi Sylwester,
>>
>> 	I like the principle of your implementation, but I have two questions:
>> 	1 - How can we manage PM with this solution, as the parent/rate will be
>> set only once at probe time?
>> 	2 - How to set the parent of a parent clock (which can be shared with
>> other devices)? Same question about the parent rates.
>
> Thanks for your feedback and apologies for late reply.
No problem! Apologies accepted ;)

>
> IIUC your first concern is about a situation when clocks need to be
> reconfigured upon each resume from system sleep or runtime PM resume ?

Yes. This is the case of the STi SoCs.
When resuming from system sleep, the clocks-related registers are 
restored at their boot state.

> As I mentioned in v1 of the RFC I was considering having individual
> drivers calling explicitly the clocks set up routine. Presumably this
> would allow to resolve the power management related issue.
 From a functional point of view, that would indeed resolve the PM 
related issue.
But I'm not sure that on a performance point of view, parsing the DT at 
each driver's resume call is an efficient way.

> One example I'm aware the approach as in this RFC wouldn't work is
> when a device in a SoC belongs to a power domain, which needs to be
> first switched on before we can start setting up and the clocks'
> configuration get lost after the power domain switch off.
Yes, that's another case to handle.
I don't know which platforms are in that case, but not STi SoCs for your 
information.

>
> OTOH I suspect devices for which one-time clocks setup is sufficient
> will be quite common. And for these there would need to be a single
> call to the setup routine in probe() I guess, since it wouldn't be
> possible to figure out just from the DT data when the actual call
> should be made.
>
> For a global clocks configuration, I thought about specifying that
> in the clocks controller node, and then to have the setup routine
> called e.g. from of_clk_init(). I think that could work well enough,
> together with the patch [1], adding clock dependencies handling.
> But then the clock frequency set up function would need to be
> modified to respect the clock parent relationships, similarly as
> in patch series [2]. A just noticed [2] recently, after posting
> this RFC (adding Tero at Cc).
OK, I agree with the approach.
There is still the PM issue remaining with these clocks, but I think 
that is not related to your series as we already have the issue currently.

Thanks,
Maxime

>
> --
> Regards,
> Sylwester
>
> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html
> [2] http://www.spinics.net/lists/linux-omap/msg103069.html
>

Re: [RFC PATCH v2 0/2] clk: Support for DT assigned clock parents and rates

[Sylwester Nawrocki]

On 21/03/14 12:20, Maxime Coquelin wrote:
> On 03/20/2014 01:42 PM, Sylwester Nawrocki wrote:
>> On 06/03/14 14:45, Maxime Coquelin wrote:
>>> Hi Sylwester,
>>>
>>> 	I like the principle of your implementation, but I have two questions:
>>> 	1 - How can we manage PM with this solution, as the parent/rate will be
>>> set only once at probe time?
>>> 	2 - How to set the parent of a parent clock (which can be shared with
>>> other devices)? Same question about the parent rates.
>>
>> Thanks for your feedback and apologies for late reply.
>
> No problem! Apologies accepted ;)

:)
>> IIUC your first concern is about a situation when clocks need to be
>> reconfigured upon each resume from system sleep or runtime PM resume ?
> 
> Yes. This is the case of the STi SoCs.
> When resuming from system sleep, the clocks-related registers are 
> restored at their boot state.

Couldn't this be handled at the clocks controller driver suspend/resume
operations ?

>> As I mentioned in v1 of the RFC I was considering having individual
>> drivers calling explicitly the clocks set up routine. Presumably this
>> would allow to resolve the power management related issue.
>
>  From a functional point of view, that would indeed resolve the PM 
> related issue.
> But I'm not sure that on a performance point of view, parsing the DT at 
> each driver's resume call is an efficient way.

Right, it might not indeed be a good idea to do the parsing repeatedly.

>> One example I'm aware the approach as in this RFC wouldn't work is
>> when a device in a SoC belongs to a power domain, which needs to be
>> first switched on before we can start setting up and the clocks'
>> configuration get lost after the power domain switch off.
>
> Yes, that's another case to handle.
> I don't know which platforms are in that case, but not STi SoCs for your 
> information.

OK, this could still hopefully be resolved in a clock controller driver,
making it aware it belongs to same power domain as the clock consumer
devices.

>> OTOH I suspect devices for which one-time clocks setup is sufficient
>> will be quite common. And for these there would need to be a single
>> call to the setup routine in probe() I guess, since it wouldn't be
>> possible to figure out just from the DT data when the actual call
>> should be made.
>>
>> For a global clocks configuration, I thought about specifying that
>> in the clocks controller node, and then to have the setup routine
>> called e.g. from of_clk_init(). I think that could work well enough,
>> together with the patch [1], adding clock dependencies handling.
>> But then the clock frequency set up function would need to be
>> modified to respect the clock parent relationships, similarly as
>> in patch series [2]. A just noticed [2] recently, after posting
>> this RFC (adding Tero at Cc).
>
> OK, I agree with the approach.
> There is still the PM issue remaining with these clocks, but I think 
> that is not related to your series as we already have the issue currently.

>> [1] http://www.spinics.net/lists/arm-kernel/msg310507.html
>> [2] http://www.spinics.net/lists/linux-omap/msg103069.html

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