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

[PATCH RFC v4 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().

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.

I didn't add sorting of clocks depending on parentship relation when
setting the clock rates, it could be added in next iteration if it's
decided it's required.

Thank you for all the review comments.

Changes since v3:
 - improved documentation of the DT binding,
 - fixed build errors for !CONFIG_OF, the parsing helpers are only
   compiled in if CONFIG_OF is set.

Changes since v2:
 - code reordering to ensure there is no build errors, the clock
   configuration code moved to a separate file,
 - introduced an 'assigned-clocks' DT node which is supposed to contain
   clocks, clock-parents, clock-rates properties and be child node
   a clock provider node, and a code parsing it called from of_clk_init();
   It's for clocks which are not directly connected to consumer devices.
   An alternative would be to list such assigned clocks in 'clocks'
   property, along with "proper" parent clocks, but then there would
   be phandles in clocks property of a node pointing to itself and it
   would require proper handling in of_clock_init().
   I actually tried it but it looked a bit ugly and chose this time to
   use an extra subnode.

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.

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   |   42 ++++++++++
 drivers/base/dd.c                                  |    7 ++
 drivers/clk/Makefile                               |    3 +
 drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
 drivers/clk/clk.c                                  |   10 ++-
 drivers/clk/clkdev.c                               |   25 +++++-
 include/linux/clk.h                                |    7 ++
 include/linux/clk/clk-conf.h                       |   19 +++++
 8 files changed, 195 insertions(+), 5 deletions(-)
 create mode 100644 drivers/clk/clk-conf.c
 create mode 100644 include/linux/clk/clk-conf.h

--
1.7.9.5

[PATCH RFC v4 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().

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.

I didn't add sorting of clocks depending on parentship relation when
setting the clock rates, it could be added in next iteration if it's
decided it's required.

Thank you for all the review comments.

Changes since v3:
 - improved documentation of the DT binding,
 - fixed build errors for !CONFIG_OF, the parsing helpers are only
   compiled in if CONFIG_OF is set.

Changes since v2:
 - code reordering to ensure there is no build errors, the clock
   configuration code moved to a separate file,
 - introduced an 'assigned-clocks' DT node which is supposed to contain
   clocks, clock-parents, clock-rates properties and be child node
   a clock provider node, and a code parsing it called from of_clk_init();
   It's for clocks which are not directly connected to consumer devices.
   An alternative would be to list such assigned clocks in 'clocks'
   property, along with "proper" parent clocks, but then there would
   be phandles in clocks property of a node pointing to itself and it
   would require proper handling in of_clock_init().
   I actually tried it but it looked a bit ugly and chose this time to
   use an extra subnode.

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.

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   |   42 ++++++++++
 drivers/base/dd.c                                  |    7 ++
 drivers/clk/Makefile                               |    3 +
 drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
 drivers/clk/clk.c                                  |   10 ++-
 drivers/clk/clkdev.c                               |   25 +++++-
 include/linux/clk.h                                |    7 ++
 include/linux/clk/clk-conf.h                       |   19 +++++
 8 files changed, 195 insertions(+), 5 deletions(-)
 create mode 100644 drivers/clk/clk-conf.c
 create mode 100644 include/linux/clk/clk-conf.h

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

[PATCH RFC v4 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().

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.

I didn't add sorting of clocks depending on parentship relation when
setting the clock rates, it could be added in next iteration if it's
decided it's required.

Thank you for all the review comments.

Changes since v3:
 - improved documentation of the DT binding,
 - fixed build errors for !CONFIG_OF, the parsing helpers are only
   compiled in if CONFIG_OF is set.

Changes since v2:
 - code reordering to ensure there is no build errors, the clock
   configuration code moved to a separate file,
 - introduced an 'assigned-clocks' DT node which is supposed to contain
   clocks, clock-parents, clock-rates properties and be child node
   a clock provider node, and a code parsing it called from of_clk_init();
   It's for clocks which are not directly connected to consumer devices.
   An alternative would be to list such assigned clocks in 'clocks'
   property, along with "proper" parent clocks, but then there would
   be phandles in clocks property of a node pointing to itself and it
   would require proper handling in of_clock_init().
   I actually tried it but it looked a bit ugly and chose this time to
   use an extra subnode.

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.

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   |   42 ++++++++++
 drivers/base/dd.c                                  |    7 ++
 drivers/clk/Makefile                               |    3 +
 drivers/clk/clk-conf.c                             |   87 ++++++++++++++++++++
 drivers/clk/clk.c                                  |   10 ++-
 drivers/clk/clkdev.c                               |   25 +++++-
 include/linux/clk.h                                |    7 ++
 include/linux/clk/clk-conf.h                       |   19 +++++
 8 files changed, 195 insertions(+), 5 deletions(-)
 create mode 100644 drivers/clk/clk-conf.c
 create mode 100644 include/linux/clk/clk-conf.h

--
1.7.9.5

[PATCH RFC v4 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>
---
Changes since v3:
 - added missing 'static inline' to the function stub definition.

Changes since v2:
 - moved the function declaration from drivers/clk/clk.h to
   include/linux/clk.h

Changes since v1:
 - s/of_clk_get_list_entry/of_clk_get_by_property.
---
 drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
 include/linux/clk.h  |    7 +++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

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);
 
 /**
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e097..f3811d3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -397,6 +397,8 @@ struct of_phandle_args;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
 #else
@@ -409,6 +411,11 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index)
+{
+	return ERR_PTR(-ENOENT);
+}
 #endif
 
 #endif
-- 
1.7.9.5

[PATCH RFC v4 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
Changes since v3:
 - added missing 'static inline' to the function stub definition.

Changes since v2:
 - moved the function declaration from drivers/clk/clk.h to
   include/linux/clk.h

Changes since v1:
 - s/of_clk_get_list_entry/of_clk_get_by_property.
---
 drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
 include/linux/clk.h  |    7 +++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

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);
 
 /**
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e097..f3811d3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -397,6 +397,8 @@ struct of_phandle_args;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
 #else
@@ -409,6 +411,11 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index)
+{
+	return ERR_PTR(-ENOENT);
+}
 #endif
 
 #endif
-- 
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

[PATCH RFC v4 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>
---
Changes since v3:
 - added missing 'static inline' to the function stub definition.

Changes since v2:
 - moved the function declaration from drivers/clk/clk.h to
   include/linux/clk.h

Changes since v1:
 - s/of_clk_get_list_entry/of_clk_get_by_property.
---
 drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
 include/linux/clk.h  |    7 +++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

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);
 
 /**
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e097..f3811d3 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -397,6 +397,8 @@ struct of_phandle_args;
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
+struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
 #else
@@ -409,6 +411,11 @@ static inline struct clk *of_clk_get_by_name(struct device_node *np,
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline struct clk *of_clk_get_by_property(struct device_node *np,
+				   const char *list_name, int index)
+{
+	return ERR_PTR(-ENOENT);
+}
 #endif
 
 #endif
-- 
1.7.9.5

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

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

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

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

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

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

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

[Ben Dooks]

On 31/03/14 17:41, Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.

[snip]

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

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

[snip code]

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

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

[Ben Dooks]

On 31/03/14 17:41, Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.

[snip]

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

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

[snip code]

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius
--
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

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

[Ben Dooks]

On 31/03/14 17:41, Sylwester Nawrocki wrote:
> This function adds a helper function to configure clock parents and rates
> as specified in clock-parents, clock-rates DT properties for a consumer
> device and a call to it before driver is bound to a device.

[snip]

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

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

[snip code]

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

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

[Greg KH]

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


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

What makes clocks so "unique" here?

greg k-h

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

[Greg KH]

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


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

What makes clocks so "unique" here?

greg k-h
--
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

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

[Greg KH]

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


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

What makes clocks so "unique" here?

greg k-h

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

[Sascha Hauer]

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

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

Sascha

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

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

[Sascha Hauer]

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

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

Sascha

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

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

[Sascha Hauer]

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

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

Sascha

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

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester
--
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

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester

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

[Laurent Pinchart]

Hi Sylwester,

Thank you for the patch.

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

[snip]

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

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

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

[snip]

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

Aren't you missing an of_node_put() here ?

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

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sylwester,

Thank you for the patch.

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

[snip]

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

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

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

[snip]

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

Aren't you missing an of_node_put() here ?

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

-- 
Regards,

Laurent Pinchart

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

[Ben Dooks]

On 31/03/14 21:06, Greg KH wrote:
> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>> This function adds a helper function to configure clock parents and rates
>> as specified in clock-parents, clock-rates DT properties for a consumer
>> device and a call to it before driver is bound to a device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

[snip]

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

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

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

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

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

[Ben Dooks]

On 31/03/14 21:06, Greg KH wrote:
> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>> This function adds a helper function to configure clock parents and rates
>> as specified in clock-parents, clock-rates DT properties for a consumer
>> device and a call to it before driver is bound to a device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

[snip]

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

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

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

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius
--
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

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

[Ben Dooks]

On 31/03/14 21:06, Greg KH wrote:
> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote:
>> This function adds a helper function to configure clock parents and rates
>> as specified in clock-parents, clock-rates DT properties for a consumer
>> device and a call to it before driver is bound to a device.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

[snip]

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

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

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

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

--
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

--
Thanks,
Sylwester
--
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

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

--
Thanks,
Sylwester

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

[Sylwester Nawrocki]

Hi Laurent,

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

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

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

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

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

--
Regards,
Sylwester

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

[Sylwester Nawrocki]

Hi Laurent,

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

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

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

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

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

--
Regards,
Sylwester

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

[Greg KH]

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

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

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

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

thanks,

greg k-h

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

[Greg KH]

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

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

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

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

thanks,

greg k-h

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

[Greg KH]

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

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

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

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

Why not?

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

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

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

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

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

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

greg k-h

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

[Greg KH]

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

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

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

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

Why not?

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

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

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

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

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

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

greg k-h

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

[Sascha Hauer]

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

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

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

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

Sascha

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

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

[Sascha Hauer]

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

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

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

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

Sascha

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

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

[Sascha Hauer]

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

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

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

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

Sascha

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

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

[Peter De Schrijver]

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

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

Cheers,

Peter.

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

[Peter De Schrijver]

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

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

Cheers,

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

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

[Peter De Schrijver]

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

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

Cheers,

Peter.

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

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

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

OK, I'm going to put it in the bus code.

-- 
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

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

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

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

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

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

OK, I'm going to put it in the bus code.

-- 
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

It would work but I don't really like such a DT binding. Rather than
only having a long list of clocks in one node I would prefer to also
have a possibility to list clock data specific to a device in its
corresponding DT node. Similarly as it's done, e.g. with interrupts.

-- 
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

It would work but I don't really like such a DT binding. Rather than
only having a long list of clocks in one node I would prefer to also
have a possibility to list clock data specific to a device in its
corresponding DT node. Similarly as it's done, e.g. with interrupts.

-- 
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester
--
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

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

[Sylwester Nawrocki]

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

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

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

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

--
Thanks,
Sylwester