[PATCH RFC v1 0/3] clk: Support for DT assigned clk parent and rate

[PATCH RFC v1 0/3] clk: Support for DT assigned clk parent and rate

[Sylwester Nawrocki]

This patch set implements setting of parent clocks and clock frequencies
before a driver is bound to the device.  This is only for booting with the 
device tree case and the parent/rate configuration is supposed to be 
specified according to the DT binding previously discussed [1].

Patch 1/3 adds a variant of of_clk_get() function which accepts name of a DT 
property containing list of phandle to a clock controller node and the clock 
specifier pairs, as opposed to hard coded "clocks" property name in 
of_clk_get().  Any better names for the function are welcome.

Patch 2/3 just adds a macro definition for maximum length of a DT property 
name.  I couldn't find any already available and there are places where plain 
numbers are used.

Patch 3/3 actually adds the code parsing device nodes for related DT 
properties and performing re-parenting and/or clock frequency setting 
as required.

I wasn't sure whether it should be done in a notifier like this, or perhaps 
there should be direct calls added into driver core (similarly to pinctrl). 
So any errors can be passed up to the callers and driver probe can be retried, 
should it happen any required clocks are not yet available at probe() call 
time.

[1] http://www.spinics.net/lists/arm-kernel/msg302226.html

Sylwester Nawrocki (3):
  clk: Add function to parse an arbitrary clocks list property
  of: Add definition of maximum length of a property name
  clk: Add handling of clk parent and rate assigned from DT

 .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
 drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
 drivers/clk/clk.h                                  |    3 +
 drivers/clk/clkdev.c                               |   25 +++++-
 include/linux/of.h                                 |    3 +
 5 files changed, 143 insertions(+), 4 deletions(-)

-- 
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 v1 0/3] clk: Support for DT assigned clk parent and rate

[Sylwester Nawrocki]

This patch set implements setting of parent clocks and clock frequencies
before a driver is bound to the device.  This is only for booting with the 
device tree case and the parent/rate configuration is supposed to be 
specified according to the DT binding previously discussed [1].

Patch 1/3 adds a variant of of_clk_get() function which accepts name of a DT 
property containing list of phandle to a clock controller node and the clock 
specifier pairs, as opposed to hard coded "clocks" property name in 
of_clk_get().  Any better names for the function are welcome.

Patch 2/3 just adds a macro definition for maximum length of a DT property 
name.  I couldn't find any already available and there are places where plain 
numbers are used.

Patch 3/3 actually adds the code parsing device nodes for related DT 
properties and performing re-parenting and/or clock frequency setting 
as required.

I wasn't sure whether it should be done in a notifier like this, or perhaps 
there should be direct calls added into driver core (similarly to pinctrl). 
So any errors can be passed up to the callers and driver probe can be retried, 
should it happen any required clocks are not yet available at probe() call 
time.

[1] http://www.spinics.net/lists/arm-kernel/msg302226.html

Sylwester Nawrocki (3):
  clk: Add function to parse an arbitrary clocks list property
  of: Add definition of maximum length of a property name
  clk: Add handling of clk parent and rate assigned from DT

 .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
 drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
 drivers/clk/clk.h                                  |    3 +
 drivers/clk/clkdev.c                               |   25 +++++-
 include/linux/of.h                                 |    3 +
 5 files changed, 143 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH RFC v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Sylwester Nawrocki]

The of_clk_get_list_entry() function is like of_clk_get() except it allows
to pass name of a DT property containing list of phandles and clock specifiers.
For of_clk_get() it has been hard coded to "clocks".

Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/clk/clk.h    |    3 +++
 drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 795cc9f..dd9def1 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -10,7 +10,10 @@
  */

 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
 void of_clk_lock(void);
 void of_clk_unlock(void);
+struct clk *of_clk_get_list_entry(struct device_node *np,
+				  const char *list_name, int index);
 #endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index a360b2e..465662e 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_list_entry() - 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_list_entry(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_list_entry);
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+	return of_clk_get_list_entry(np, "clocks", index);
+}
 EXPORT_SYMBOL(of_clk_get);

 /**
--
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 v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Sylwester Nawrocki]

The of_clk_get_list_entry() function is like of_clk_get() except it allows
to pass name of a DT property containing list of phandles and clock specifiers.
For of_clk_get() it has been hard coded to "clocks".

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

diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 795cc9f..dd9def1 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -10,7 +10,10 @@
  */

 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
+
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
 void of_clk_lock(void);
 void of_clk_unlock(void);
+struct clk *of_clk_get_list_entry(struct device_node *np,
+				  const char *list_name, int index);
 #endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index a360b2e..465662e 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_list_entry() - 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_list_entry(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_list_entry);
+
+struct clk *of_clk_get(struct device_node *np, int index)
+{
+	return of_clk_get_list_entry(np, "clocks", index);
+}
 EXPORT_SYMBOL(of_clk_get);

 /**
--
1.7.9.5

[PATCH RFC v1 2/3] of: Add definition of maximum length of a property name

[Sylwester Nawrocki]

Maximum length of a property name is defined by ePAPR (2.2.4.1) as
31 characters. Add a corresponding definition, including the trailing
null space.

Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 include/linux/of.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 70c64ba..7f71221 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -30,6 +30,9 @@
 typedef u32 phandle;
 typedef u32 ihandle;
 
+/* Maximum length of name of a property, including terminating null */
+#define OF_PROP_NAME_MAXLEN	32
+
 struct property {
 	char	*name;
 	int	length;
-- 
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 v1 2/3] of: Add definition of maximum length of a property name

[Sylwester Nawrocki]

Maximum length of a property name is defined by ePAPR (2.2.4.1) as
31 characters. Add a corresponding definition, including the trailing
null space.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 include/linux/of.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 70c64ba..7f71221 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -30,6 +30,9 @@
 typedef u32 phandle;
 typedef u32 ihandle;
 
+/* Maximum length of name of a property, including terminating null */
+#define OF_PROP_NAME_MAXLEN	32
+
 struct property {
 	char	*name;
 	int	length;
-- 
1.7.9.5

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

[Sylwester Nawrocki]

This function adds a notifier callback run before a driver is bound to
its driver. It will configure parent clock and clock frequencies based
on [clk-name]-clk-parent and [clk-name]-clk-rate' DT properties.

Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
 drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 7c52c29..d618498 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -115,3 +115,27 @@ 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)
+
+==Static initial configuration of clock parent and clock frequency==
+
+Some platforms require static configuration of (parts of) the clock controller
+often determined by the board design. Such a configuration can be specified in
+a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
+properties. The former should contain phandle and clock specifier of the parent
+clock, the latter the required clock's frequency value (one cell). "clk-name"
+should be listed in the clock-names property and a phandle and a clock specifier
+pair corresponding to it should be present in the clocks property.
+
+    uart@a000 {
+        compatible = "fsl,imx-uart";
+        reg = <0xa000 0x1000>;
+	...
+        clocks = <&clkcon 0>, <&clkcon 3>;
+        clock-names = "baud", "mux";
+
+	mux-clk-parent = <&pll 1>;
+	baud-clk-rate = <460800>;
+    };
+
+In this example the pll is set as parent of "mux" clock and frequency of "baud"
+clock is specified as 460800 Hz.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 19f6f3f..9238e08 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/platform_device.h>
 #include <linux/sched.h>
 
 #include "clk.h"
@@ -2527,6 +2528,97 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
 
+static void __of_clk_assigned_config_set(struct clk *clk, struct clk *pclk,
+					 u32 rate)
+{
+	int rc;
+
+	if (rate) {
+		rc = clk_set_rate(clk, rate);
+		if (rc < 0)
+			pr_err("clk: couldn't set rate of clock %s (%d)\n",
+			       __clk_get_name(clk), rc);
+		else
+			pr_debug("clk: set rate of clock %s to %u\n",
+				 __clk_get_name(clk), rate);
+	}
+
+	if (!IS_ERR(pclk)) {
+		rc = clk_set_parent(clk, pclk);
+		if (rc < 0)
+			pr_err("clk: couldn't set %s as parent of %s (%d)\n",
+			       __clk_get_name(pclk), __clk_get_name(clk), rc);
+		else
+			pr_debug("clk: set %s as parent of %s\n",
+				 __clk_get_name(pclk), __clk_get_name(clk));
+	}
+}
+
+static void of_clk_assigned_config_parse(struct device_node *node)
+{
+	char prop_name[OF_PROP_NAME_MAXLEN];
+	struct property *prop;
+	const char *clk_name;
+	int rc, index = 0;
+
+	of_property_for_each_string(node, "clock-names", prop, clk_name) {
+		struct clk *clk, *pclk;
+		u32 rate = 0;
+
+		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
+					"%s-clk-parent", clk_name);
+		pclk = of_clk_get_list_entry(node, prop_name, 0);
+
+		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
+					"%s-clk-rate", clk_name);
+		rc = of_property_read_u32(node, prop_name, &rate);
+
+		if (!rc || !IS_ERR(pclk)) {
+			/*
+			 * Assuming here of_property_for_each_string() returns
+			 * consecutive values of a DT property in ascending
+			 * index order.
+			 */
+			clk = of_clk_get(node, index);
+
+			if (!IS_ERR(clk))
+				__of_clk_assigned_config_set(clk, pclk, rate);
+			else
+				pr_err("clk: couldn't get clk %s\n", clk_name);
+		}
+		index++;
+	}
+}
+
+
+static int of_clk_setup_notifier_call(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	if (!dev->of_node)
+		return NOTIFY_DONE;
+
+	switch (event) {
+	case BUS_NOTIFY_BIND_DRIVER:
+		/* Parse and configure DT assigned clock parents and rates */
+		of_clk_assigned_config_parse(dev->of_node);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block of_clk_setup_nb = {
+	.notifier_call = of_clk_setup_notifier_call,
+};
+
+int __init of_clk_setup_notifier_init(void)
+{
+	return bus_register_notifier(&platform_bus_type, &of_clk_setup_nb);
+}
+subsys_initcall(of_clk_setup_notifier_init);
+
 /**
  * of_clk_init() - Scan and init clock providers from the DT
  * @matches: array of compatible values and init functions for providers.
-- 
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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Sylwester Nawrocki]

This function adds a notifier callback run before a driver is bound to
its driver. It will configure parent clock and clock frequencies based
on [clk-name]-clk-parent and [clk-name]-clk-rate' DT properties.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
 drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 7c52c29..d618498 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -115,3 +115,27 @@ 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)
+
+==Static initial configuration of clock parent and clock frequency==
+
+Some platforms require static configuration of (parts of) the clock controller
+often determined by the board design. Such a configuration can be specified in
+a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
+properties. The former should contain phandle and clock specifier of the parent
+clock, the latter the required clock's frequency value (one cell). "clk-name"
+should be listed in the clock-names property and a phandle and a clock specifier
+pair corresponding to it should be present in the clocks property.
+
+    uart at a000 {
+        compatible = "fsl,imx-uart";
+        reg = <0xa000 0x1000>;
+	...
+        clocks = <&clkcon 0>, <&clkcon 3>;
+        clock-names = "baud", "mux";
+
+	mux-clk-parent = <&pll 1>;
+	baud-clk-rate = <460800>;
+    };
+
+In this example the pll is set as parent of "mux" clock and frequency of "baud"
+clock is specified as 460800 Hz.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 19f6f3f..9238e08 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/platform_device.h>
 #include <linux/sched.h>
 
 #include "clk.h"
@@ -2527,6 +2528,97 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
 
+static void __of_clk_assigned_config_set(struct clk *clk, struct clk *pclk,
+					 u32 rate)
+{
+	int rc;
+
+	if (rate) {
+		rc = clk_set_rate(clk, rate);
+		if (rc < 0)
+			pr_err("clk: couldn't set rate of clock %s (%d)\n",
+			       __clk_get_name(clk), rc);
+		else
+			pr_debug("clk: set rate of clock %s to %u\n",
+				 __clk_get_name(clk), rate);
+	}
+
+	if (!IS_ERR(pclk)) {
+		rc = clk_set_parent(clk, pclk);
+		if (rc < 0)
+			pr_err("clk: couldn't set %s as parent of %s (%d)\n",
+			       __clk_get_name(pclk), __clk_get_name(clk), rc);
+		else
+			pr_debug("clk: set %s as parent of %s\n",
+				 __clk_get_name(pclk), __clk_get_name(clk));
+	}
+}
+
+static void of_clk_assigned_config_parse(struct device_node *node)
+{
+	char prop_name[OF_PROP_NAME_MAXLEN];
+	struct property *prop;
+	const char *clk_name;
+	int rc, index = 0;
+
+	of_property_for_each_string(node, "clock-names", prop, clk_name) {
+		struct clk *clk, *pclk;
+		u32 rate = 0;
+
+		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
+					"%s-clk-parent", clk_name);
+		pclk = of_clk_get_list_entry(node, prop_name, 0);
+
+		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
+					"%s-clk-rate", clk_name);
+		rc = of_property_read_u32(node, prop_name, &rate);
+
+		if (!rc || !IS_ERR(pclk)) {
+			/*
+			 * Assuming here of_property_for_each_string() returns
+			 * consecutive values of a DT property in ascending
+			 * index order.
+			 */
+			clk = of_clk_get(node, index);
+
+			if (!IS_ERR(clk))
+				__of_clk_assigned_config_set(clk, pclk, rate);
+			else
+				pr_err("clk: couldn't get clk %s\n", clk_name);
+		}
+		index++;
+	}
+}
+
+
+static int of_clk_setup_notifier_call(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct device *dev = data;
+
+	if (!dev->of_node)
+		return NOTIFY_DONE;
+
+	switch (event) {
+	case BUS_NOTIFY_BIND_DRIVER:
+		/* Parse and configure DT assigned clock parents and rates */
+		of_clk_assigned_config_parse(dev->of_node);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block of_clk_setup_nb = {
+	.notifier_call = of_clk_setup_notifier_call,
+};
+
+int __init of_clk_setup_notifier_init(void)
+{
+	return bus_register_notifier(&platform_bus_type, &of_clk_setup_nb);
+}
+subsys_initcall(of_clk_setup_notifier_init);
+
 /**
  * of_clk_init() - Scan and init clock providers from the DT
  * @matches: array of compatible values and init functions for providers.
-- 
1.7.9.5

Re: [PATCH RFC v1 2/3] of: Add definition of maximum length of a property name

[Rob Herring]

On Wed, Feb 19, 2014 at 10:58 AM, Sylwester Nawrocki
<s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Maximum length of a property name is defined by ePAPR (2.2.4.1) as
> 31 characters. Add a corresponding definition, including the trailing
> null space.

Does dtc enforce this? It would be good to add if not.

> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> ---
>  include/linux/of.h |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 70c64ba..7f71221 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -30,6 +30,9 @@
>  typedef u32 phandle;
>  typedef u32 ihandle;
>
> +/* Maximum length of name of a property, including terminating null */
> +#define OF_PROP_NAME_MAXLEN    32
> +
>  struct property {
>         char    *name;
>         int     length;
> --
> 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 v1 2/3] of: Add definition of maximum length of a property name

[Rob Herring]

On Wed, Feb 19, 2014 at 10:58 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Maximum length of a property name is defined by ePAPR (2.2.4.1) as
> 31 characters. Add a corresponding definition, including the trailing
> null space.

Does dtc enforce this? It would be good to add if not.

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  include/linux/of.h |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 70c64ba..7f71221 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -30,6 +30,9 @@
>  typedef u32 phandle;
>  typedef u32 ihandle;
>
> +/* Maximum length of name of a property, including terminating null */
> +#define OF_PROP_NAME_MAXLEN    32
> +
>  struct property {
>         char    *name;
>         int     length;
> --
> 1.7.9.5
>

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

[Grant Likely]

On Wed, 19 Feb 2014 17:58:44 +0100, Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> This function adds a notifier callback run before a driver is bound to
> its driver. It will configure parent clock and clock frequencies based
> on [clk-name]-clk-parent and [clk-name]-clk-rate' DT properties.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
>  drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 7c52c29..d618498 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -115,3 +115,27 @@ 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)
> +
> +==Static initial configuration of clock parent and clock frequency==
> +
> +Some platforms require static configuration of (parts of) the clock controller
> +often determined by the board design. Such a configuration can be specified in
> +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> +properties. The former should contain phandle and clock specifier of the parent
> +clock, the latter the required clock's frequency value (one cell). "clk-name"
> +should be listed in the clock-names property and a phandle and a clock specifier
> +pair corresponding to it should be present in the clocks property.
> +
> +    uart@a000 {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +	...
> +        clocks = <&clkcon 0>, <&clkcon 3>;
> +        clock-names = "baud", "mux";
> +
> +	mux-clk-parent = <&pll 1>;
> +	baud-clk-rate = <460800>;

This mixes patterns for references to clocks. Plus it requires composing
property names which is a little painful. I'd rather see a list of
tuples to match the existing pattern already in use

	clocks = <&clkcon 0>, <&clkcon 3>;
	clock-names = "baud", "mux";
	clock-parents = <0> <&pll 1>;
	clock-rates = <0> <460800>;

g.

> +    };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 19f6f3f..9238e08 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> +#include <linux/platform_device.h>
>  #include <linux/sched.h>
>  
>  #include "clk.h"
> @@ -2527,6 +2528,97 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
>  
> +static void __of_clk_assigned_config_set(struct clk *clk, struct clk *pclk,
> +					 u32 rate)
> +{
> +	int rc;
> +
> +	if (rate) {
> +		rc = clk_set_rate(clk, rate);
> +		if (rc < 0)
> +			pr_err("clk: couldn't set rate of clock %s (%d)\n",
> +			       __clk_get_name(clk), rc);
> +		else
> +			pr_debug("clk: set rate of clock %s to %u\n",
> +				 __clk_get_name(clk), rate);
> +	}
> +
> +	if (!IS_ERR(pclk)) {
> +		rc = clk_set_parent(clk, pclk);
> +		if (rc < 0)
> +			pr_err("clk: couldn't set %s as parent of %s (%d)\n",
> +			       __clk_get_name(pclk), __clk_get_name(clk), rc);
> +		else
> +			pr_debug("clk: set %s as parent of %s\n",
> +				 __clk_get_name(pclk), __clk_get_name(clk));
> +	}
> +}
> +
> +static void of_clk_assigned_config_parse(struct device_node *node)
> +{
> +	char prop_name[OF_PROP_NAME_MAXLEN];
> +	struct property *prop;
> +	const char *clk_name;
> +	int rc, index = 0;
> +
> +	of_property_for_each_string(node, "clock-names", prop, clk_name) {
> +		struct clk *clk, *pclk;
> +		u32 rate = 0;
> +
> +		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
> +					"%s-clk-parent", clk_name);
> +		pclk = of_clk_get_list_entry(node, prop_name, 0);
> +
> +		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
> +					"%s-clk-rate", clk_name);
> +		rc = of_property_read_u32(node, prop_name, &rate);
> +
> +		if (!rc || !IS_ERR(pclk)) {
> +			/*
> +			 * Assuming here of_property_for_each_string() returns
> +			 * consecutive values of a DT property in ascending
> +			 * index order.
> +			 */
> +			clk = of_clk_get(node, index);
> +
> +			if (!IS_ERR(clk))
> +				__of_clk_assigned_config_set(clk, pclk, rate);
> +			else
> +				pr_err("clk: couldn't get clk %s\n", clk_name);
> +		}
> +		index++;
> +	}
> +}
> +
> +
> +static int of_clk_setup_notifier_call(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct device *dev = data;
> +
> +	if (!dev->of_node)
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		/* Parse and configure DT assigned clock parents and rates */
> +		of_clk_assigned_config_parse(dev->of_node);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block of_clk_setup_nb = {
> +	.notifier_call = of_clk_setup_notifier_call,
> +};
> +
> +int __init of_clk_setup_notifier_init(void)
> +{
> +	return bus_register_notifier(&platform_bus_type, &of_clk_setup_nb);
> +}
> +subsys_initcall(of_clk_setup_notifier_init);
> +
>  /**
>   * of_clk_init() - Scan and init clock providers from the DT
>   * @matches: array of compatible values and init functions for providers.
> -- 
> 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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Grant Likely]

On Wed, 19 Feb 2014 17:58:44 +0100, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> This function adds a notifier callback run before a driver is bound to
> its driver. It will configure parent clock and clock frequencies based
> on [clk-name]-clk-parent and [clk-name]-clk-rate' DT properties.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt   |   24 +++++
>  drivers/clk/clk.c                                  |   92 ++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 7c52c29..d618498 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -115,3 +115,27 @@ 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)
> +
> +==Static initial configuration of clock parent and clock frequency==
> +
> +Some platforms require static configuration of (parts of) the clock controller
> +often determined by the board design. Such a configuration can be specified in
> +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> +properties. The former should contain phandle and clock specifier of the parent
> +clock, the latter the required clock's frequency value (one cell). "clk-name"
> +should be listed in the clock-names property and a phandle and a clock specifier
> +pair corresponding to it should be present in the clocks property.
> +
> +    uart at a000 {
> +        compatible = "fsl,imx-uart";
> +        reg = <0xa000 0x1000>;
> +	...
> +        clocks = <&clkcon 0>, <&clkcon 3>;
> +        clock-names = "baud", "mux";
> +
> +	mux-clk-parent = <&pll 1>;
> +	baud-clk-rate = <460800>;

This mixes patterns for references to clocks. Plus it requires composing
property names which is a little painful. I'd rather see a list of
tuples to match the existing pattern already in use

	clocks = <&clkcon 0>, <&clkcon 3>;
	clock-names = "baud", "mux";
	clock-parents = <0> <&pll 1>;
	clock-rates = <0> <460800>;

g.

> +    };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 19f6f3f..9238e08 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> +#include <linux/platform_device.h>
>  #include <linux/sched.h>
>  
>  #include "clk.h"
> @@ -2527,6 +2528,97 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
>  
> +static void __of_clk_assigned_config_set(struct clk *clk, struct clk *pclk,
> +					 u32 rate)
> +{
> +	int rc;
> +
> +	if (rate) {
> +		rc = clk_set_rate(clk, rate);
> +		if (rc < 0)
> +			pr_err("clk: couldn't set rate of clock %s (%d)\n",
> +			       __clk_get_name(clk), rc);
> +		else
> +			pr_debug("clk: set rate of clock %s to %u\n",
> +				 __clk_get_name(clk), rate);
> +	}
> +
> +	if (!IS_ERR(pclk)) {
> +		rc = clk_set_parent(clk, pclk);
> +		if (rc < 0)
> +			pr_err("clk: couldn't set %s as parent of %s (%d)\n",
> +			       __clk_get_name(pclk), __clk_get_name(clk), rc);
> +		else
> +			pr_debug("clk: set %s as parent of %s\n",
> +				 __clk_get_name(pclk), __clk_get_name(clk));
> +	}
> +}
> +
> +static void of_clk_assigned_config_parse(struct device_node *node)
> +{
> +	char prop_name[OF_PROP_NAME_MAXLEN];
> +	struct property *prop;
> +	const char *clk_name;
> +	int rc, index = 0;
> +
> +	of_property_for_each_string(node, "clock-names", prop, clk_name) {
> +		struct clk *clk, *pclk;
> +		u32 rate = 0;
> +
> +		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
> +					"%s-clk-parent", clk_name);
> +		pclk = of_clk_get_list_entry(node, prop_name, 0);
> +
> +		snprintf(prop_name, OF_PROP_NAME_MAXLEN,
> +					"%s-clk-rate", clk_name);
> +		rc = of_property_read_u32(node, prop_name, &rate);
> +
> +		if (!rc || !IS_ERR(pclk)) {
> +			/*
> +			 * Assuming here of_property_for_each_string() returns
> +			 * consecutive values of a DT property in ascending
> +			 * index order.
> +			 */
> +			clk = of_clk_get(node, index);
> +
> +			if (!IS_ERR(clk))
> +				__of_clk_assigned_config_set(clk, pclk, rate);
> +			else
> +				pr_err("clk: couldn't get clk %s\n", clk_name);
> +		}
> +		index++;
> +	}
> +}
> +
> +
> +static int of_clk_setup_notifier_call(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct device *dev = data;
> +
> +	if (!dev->of_node)
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case BUS_NOTIFY_BIND_DRIVER:
> +		/* Parse and configure DT assigned clock parents and rates */
> +		of_clk_assigned_config_parse(dev->of_node);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block of_clk_setup_nb = {
> +	.notifier_call = of_clk_setup_notifier_call,
> +};
> +
> +int __init of_clk_setup_notifier_init(void)
> +{
> +	return bus_register_notifier(&platform_bus_type, &of_clk_setup_nb);
> +}
> +subsys_initcall(of_clk_setup_notifier_init);
> +
>  /**
>   * of_clk_init() - Scan and init clock providers from the DT
>   * @matches: array of compatible values and init functions for providers.
> -- 
> 1.7.9.5
> 

Re: [PATCH RFC v1 2/3] of: Add definition of maximum length of a property name

[Sylwester Nawrocki]

On 19/02/14 22:42, Rob Herring wrote:
> On Wed, Feb 19, 2014 at 10:58 AM, Sylwester Nawrocki
> <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> > Maximum length of a property name is defined by ePAPR (2.2.4.1) as
>> > 31 characters. Add a corresponding definition, including the trailing
>> > null space.
>
> Does dtc enforce this? It would be good to add if not.

As far as I can see there is nothing currently enforcing this in dtc.
There is no related check and too long property names are accepted
silently.
I'll try and see if I can prepare a patch for this, until someone more
familiar with dtc does it.

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

[PATCH RFC v1 2/3] of: Add definition of maximum length of a property name

[Sylwester Nawrocki]

On 19/02/14 22:42, Rob Herring wrote:
> On Wed, Feb 19, 2014 at 10:58 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> > Maximum length of a property name is defined by ePAPR (2.2.4.1) as
>> > 31 characters. Add a corresponding definition, including the trailing
>> > null space.
>
> Does dtc enforce this? It would be good to add if not.

As far as I can see there is nothing currently enforcing this in dtc.
There is no related check and too long property names are accepted
silently.
I'll try and see if I can prepare a patch for this, until someone more
familiar with dtc does it.

--
Regards,
Sylwester

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

[Sylwester Nawrocki]

On 20/02/14 15:09, Grant Likely wrote:
[...]
>> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > index 7c52c29..d618498 100644
>> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > @@ -115,3 +115,27 @@ 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)
>> > +
>> > +==Static initial configuration of clock parent and clock frequency==
>> > +
>> > +Some platforms require static configuration of (parts of) the clock controller
>> > +often determined by the board design. Such a configuration can be specified in
>> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
>> > +properties. The former should contain phandle and clock specifier of the parent
>> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
>> > +should be listed in the clock-names property and a phandle and a clock specifier
>> > +pair corresponding to it should be present in the clocks property.
>> > +
>> > +    uart@a000 {
>> > +        compatible = "fsl,imx-uart";
>> > +        reg = <0xa000 0x1000>;
>> > +	...
>> > +        clocks = <&clkcon 0>, <&clkcon 3>;
>> > +        clock-names = "baud", "mux";
>> > +
>> > +	mux-clk-parent = <&pll 1>;
>> > +	baud-clk-rate = <460800>;
>
> This mixes patterns for references to clocks. Plus it requires composing
> property names which is a little painful. I'd rather see a list of
> tuples to match the existing pattern already in use
> 
> 	clocks = <&clkcon 0>, <&clkcon 3>;
> 	clock-names = "baud", "mux";
> 	clock-parents = <0> <&pll 1>;
> 	clock-rates = <0> <460800>;

Thank you for the review. This looks much better to me. My bad, I wasn't 
aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
to be specifying exact meaning of the 'phandle' property values, except 
they be unique. Anyway, it seems to be clearly documented within the
__of_parse_phandle_with_args() function. 

I'll try this modified binding instead, presumably it would be useful to 
have a variant of __of_parse_phandle_with_args() function which would 
accept a context data containing result of previous call within an iteration, 
similarly as of_property_next_string() is written. So we don't iterate 
from beginning of the list with each __of_parse_phandle_with_args() call. 
But it's an optimization issue that could be considered separately I guess.

--
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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Sylwester Nawrocki]

On 20/02/14 15:09, Grant Likely wrote:
[...]
>> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > index 7c52c29..d618498 100644
>> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > @@ -115,3 +115,27 @@ 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)
>> > +
>> > +==Static initial configuration of clock parent and clock frequency==
>> > +
>> > +Some platforms require static configuration of (parts of) the clock controller
>> > +often determined by the board design. Such a configuration can be specified in
>> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
>> > +properties. The former should contain phandle and clock specifier of the parent
>> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
>> > +should be listed in the clock-names property and a phandle and a clock specifier
>> > +pair corresponding to it should be present in the clocks property.
>> > +
>> > +    uart at a000 {
>> > +        compatible = "fsl,imx-uart";
>> > +        reg = <0xa000 0x1000>;
>> > +	...
>> > +        clocks = <&clkcon 0>, <&clkcon 3>;
>> > +        clock-names = "baud", "mux";
>> > +
>> > +	mux-clk-parent = <&pll 1>;
>> > +	baud-clk-rate = <460800>;
>
> This mixes patterns for references to clocks. Plus it requires composing
> property names which is a little painful. I'd rather see a list of
> tuples to match the existing pattern already in use
> 
> 	clocks = <&clkcon 0>, <&clkcon 3>;
> 	clock-names = "baud", "mux";
> 	clock-parents = <0> <&pll 1>;
> 	clock-rates = <0> <460800>;

Thank you for the review. This looks much better to me. My bad, I wasn't 
aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
to be specifying exact meaning of the 'phandle' property values, except 
they be unique. Anyway, it seems to be clearly documented within the
__of_parse_phandle_with_args() function. 

I'll try this modified binding instead, presumably it would be useful to 
have a variant of __of_parse_phandle_with_args() function which would 
accept a context data containing result of previous call within an iteration, 
similarly as of_property_next_string() is written. So we don't iterate 
from beginning of the list with each __of_parse_phandle_with_args() call. 
But it's an optimization issue that could be considered separately I guess.

--
Thanks,
Sylwester

Re: [PATCH RFC v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-02-19 08:58:42)
> The of_clk_get_list_entry() function is like of_clk_get() except it allows
> to pass name of a DT property containing list of phandles and clock specifiers.
> For of_clk_get() it has been hard coded to "clocks".
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/clk/clk.h    |    3 +++
>  drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
> index 795cc9f..dd9def1 100644
> --- a/drivers/clk/clk.h
> +++ b/drivers/clk/clk.h
> @@ -10,7 +10,10 @@
>   */
> 
>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
> +
>  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>  void of_clk_lock(void);
>  void of_clk_unlock(void);
> +struct clk *of_clk_get_list_entry(struct device_node *np,
> +                                 const char *list_name, int index);
>  #endif
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index a360b2e..465662e 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_list_entry() - 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_list_entry(struct device_node *np,
> +                                 const char *list_name, int index)

Bikeshed alert: how about of_clk_get_by_property or of_clk_get_by_prop?

No strong opinion on the topic though.

Regards,
Mike

>  {
>         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_list_entry);
> +
> +struct clk *of_clk_get(struct device_node *np, int index)
> +{
> +       return of_clk_get_list_entry(np, "clocks", index);
> +}
>  EXPORT_SYMBOL(of_clk_get);
> 
>  /**
> --
> 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 v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-02-19 08:58:42)
> The of_clk_get_list_entry() function is like of_clk_get() except it allows
> to pass name of a DT property containing list of phandles and clock specifiers.
> For of_clk_get() it has been hard coded to "clocks".
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/clk/clk.h    |    3 +++
>  drivers/clk/clkdev.c |   25 +++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
> index 795cc9f..dd9def1 100644
> --- a/drivers/clk/clk.h
> +++ b/drivers/clk/clk.h
> @@ -10,7 +10,10 @@
>   */
> 
>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
> +
>  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>  void of_clk_lock(void);
>  void of_clk_unlock(void);
> +struct clk *of_clk_get_list_entry(struct device_node *np,
> +                                 const char *list_name, int index);
>  #endif
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index a360b2e..465662e 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_list_entry() - 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_list_entry(struct device_node *np,
> +                                 const char *list_name, int index)

Bikeshed alert: how about of_clk_get_by_property or of_clk_get_by_prop?

No strong opinion on the topic though.

Regards,
Mike

>  {
>         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_list_entry);
> +
> +struct clk *of_clk_get(struct device_node *np, int index)
> +{
> +       return of_clk_get_list_entry(np, "clocks", index);
> +}
>  EXPORT_SYMBOL(of_clk_get);
> 
>  /**
> --
> 1.7.9.5
> 

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

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> On 20/02/14 15:09, Grant Likely wrote:
> [...]
> >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > index 7c52c29..d618498 100644
> >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > @@ -115,3 +115,27 @@ 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)
> >> > +
> >> > +==Static initial configuration of clock parent and clock frequency==
> >> > +
> >> > +Some platforms require static configuration of (parts of) the clock controller
> >> > +often determined by the board design. Such a configuration can be specified in
> >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> >> > +properties. The former should contain phandle and clock specifier of the parent
> >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> >> > +should be listed in the clock-names property and a phandle and a clock specifier
> >> > +pair corresponding to it should be present in the clocks property.
> >> > +
> >> > +    uart@a000 {
> >> > +        compatible = "fsl,imx-uart";
> >> > +        reg = <0xa000 0x1000>;
> >> > +  ...
> >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> >> > +        clock-names = "baud", "mux";
> >> > +
> >> > +  mux-clk-parent = <&pll 1>;
> >> > +  baud-clk-rate = <460800>;
> >
> > This mixes patterns for references to clocks. Plus it requires composing
> > property names which is a little painful. I'd rather see a list of
> > tuples to match the existing pattern already in use
> > 
> >       clocks = <&clkcon 0>, <&clkcon 3>;
> >       clock-names = "baud", "mux";
> >       clock-parents = <0> <&pll 1>;
> >       clock-rates = <0> <460800>;
> 
> Thank you for the review. This looks much better to me. My bad, I wasn't 
> aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> to be specifying exact meaning of the 'phandle' property values, except 
> they be unique. Anyway, it seems to be clearly documented within the
> __of_parse_phandle_with_args() function. 
> 
> I'll try this modified binding instead, presumably it would be useful to 
> have a variant of __of_parse_phandle_with_args() function which would 
> accept a context data containing result of previous call within an iteration, 
> similarly as of_property_next_string() is written. So we don't iterate 
> from beginning of the list with each __of_parse_phandle_with_args() call. 
> But it's an optimization issue that could be considered separately I guess.

I was always partial to the regulator style of blahblah-supply but
Grant's suggestion is much cleaner with respect to the rest of the clock
binding.

I guess it will be a bit ugly if a very long array is needed with a
sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
only a single clock needs to have its parent or rate specified.

Is there a reason not to support both methods?

Regards,
Mike

> 
> --
> 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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Mike Turquette]

Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> On 20/02/14 15:09, Grant Likely wrote:
> [...]
> >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > index 7c52c29..d618498 100644
> >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > @@ -115,3 +115,27 @@ 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)
> >> > +
> >> > +==Static initial configuration of clock parent and clock frequency==
> >> > +
> >> > +Some platforms require static configuration of (parts of) the clock controller
> >> > +often determined by the board design. Such a configuration can be specified in
> >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> >> > +properties. The former should contain phandle and clock specifier of the parent
> >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> >> > +should be listed in the clock-names property and a phandle and a clock specifier
> >> > +pair corresponding to it should be present in the clocks property.
> >> > +
> >> > +    uart at a000 {
> >> > +        compatible = "fsl,imx-uart";
> >> > +        reg = <0xa000 0x1000>;
> >> > +  ...
> >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> >> > +        clock-names = "baud", "mux";
> >> > +
> >> > +  mux-clk-parent = <&pll 1>;
> >> > +  baud-clk-rate = <460800>;
> >
> > This mixes patterns for references to clocks. Plus it requires composing
> > property names which is a little painful. I'd rather see a list of
> > tuples to match the existing pattern already in use
> > 
> >       clocks = <&clkcon 0>, <&clkcon 3>;
> >       clock-names = "baud", "mux";
> >       clock-parents = <0> <&pll 1>;
> >       clock-rates = <0> <460800>;
> 
> Thank you for the review. This looks much better to me. My bad, I wasn't 
> aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> to be specifying exact meaning of the 'phandle' property values, except 
> they be unique. Anyway, it seems to be clearly documented within the
> __of_parse_phandle_with_args() function. 
> 
> I'll try this modified binding instead, presumably it would be useful to 
> have a variant of __of_parse_phandle_with_args() function which would 
> accept a context data containing result of previous call within an iteration, 
> similarly as of_property_next_string() is written. So we don't iterate 
> from beginning of the list with each __of_parse_phandle_with_args() call. 
> But it's an optimization issue that could be considered separately I guess.

I was always partial to the regulator style of blahblah-supply but
Grant's suggestion is much cleaner with respect to the rest of the clock
binding.

I guess it will be a bit ugly if a very long array is needed with a
sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
only a single clock needs to have its parent or rate specified.

Is there a reason not to support both methods?

Regards,
Mike

> 
> --
> Thanks,
> Sylwester

Re: [PATCH RFC v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Sylwester Nawrocki]

On 24/02/14 01:43, Mike Turquette wrote:
[...]
>> > +/**
>> > + * of_clk_get_list_entry() - 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_list_entry(struct device_node *np,
>> > +                                 const char *list_name, int index)
>
> Bikeshed alert: how about of_clk_get_by_property or of_clk_get_by_prop?

That sounds better to me, thanks for the suggestion.
For the next iteration I'm going to use of_clk_get_by_property() .

Regards,
Sylwester

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

[PATCH RFC v1 1/3] clk: Add function to parse an arbitrary clocks list property

[Sylwester Nawrocki]

On 24/02/14 01:43, Mike Turquette wrote:
[...]
>> > +/**
>> > + * of_clk_get_list_entry() - 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_list_entry(struct device_node *np,
>> > +                                 const char *list_name, int index)
>
> Bikeshed alert: how about of_clk_get_by_property or of_clk_get_by_prop?

That sounds better to me, thanks for the suggestion.
For the next iteration I'm going to use of_clk_get_by_property() .

Regards,
Sylwester

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

[Sylwester Nawrocki]

On 24/02/14 01:48, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
>> > On 20/02/14 15:09, Grant Likely wrote:
>> > [...]
>>>>> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > index 7c52c29..d618498 100644
>>>>> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > @@ -115,3 +115,27 @@ 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)
>>>>> > >> > +
>>>>> > >> > +==Static initial configuration of clock parent and clock frequency==
>>>>> > >> > +
>>>>> > >> > +Some platforms require static configuration of (parts of) the clock controller
>>>>> > >> > +often determined by the board design. Such a configuration can be specified in
>>>>> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
>>>>> > >> > +properties. The former should contain phandle and clock specifier of the parent
>>>>> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
>>>>> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
>>>>> > >> > +pair corresponding to it should be present in the clocks property.
>>>>> > >> > +
>>>>> > >> > +    uart@a000 {
>>>>> > >> > +        compatible = "fsl,imx-uart";
>>>>> > >> > +        reg = <0xa000 0x1000>;
>>>>> > >> > +  ...
>>>>> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
>>>>> > >> > +        clock-names = "baud", "mux";
>>>>> > >> > +
>>>>> > >> > +  mux-clk-parent = <&pll 1>;
>>>>> > >> > +  baud-clk-rate = <460800>;
>>> > >
>>> > > This mixes patterns for references to clocks. Plus it requires composing
>>> > > property names which is a little painful. I'd rather see a list of
>>> > > tuples to match the existing pattern already in use
>>> > > 
>>> > >       clocks = <&clkcon 0>, <&clkcon 3>;
>>> > >       clock-names = "baud", "mux";
>>> > >       clock-parents = <0> <&pll 1>;
>>> > >       clock-rates = <0> <460800>;
>> > 
>> > Thank you for the review. This looks much better to me. My bad, I wasn't 
>> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
>> > to be specifying exact meaning of the 'phandle' property values, except 
>> > they be unique. Anyway, it seems to be clearly documented within the
>> > __of_parse_phandle_with_args() function. 
>> > 
>> > I'll try this modified binding instead, presumably it would be useful to 
>> > have a variant of __of_parse_phandle_with_args() function which would 
>> > accept a context data containing result of previous call within an iteration, 
>> > similarly as of_property_next_string() is written. So we don't iterate 
>> > from beginning of the list with each __of_parse_phandle_with_args() call. 
>> > But it's an optimization issue that could be considered separately I guess.
>
> I was always partial to the regulator style of blahblah-supply but
> Grant's suggestion is much cleaner with respect to the rest of the clock
> binding.

I don't have a strong opinion, I'm slightly inclined towards Grant's suggestion,
which doesn't have a problem of limiting effective clk name to 21 characters.

Also DT property names with underscores coming from the clock names are a bit
incorrect and it might be not immediately obvious which part of name is a 
canonical property's name and which is clock's name. Let's consider property 
names like:

mux-clk-parent, divider-clk-rate, sclk_mmc0-clk-parent, sclk_uart_baud0-clk-parent,
etc.

> I guess it will be a bit ugly if a very long array is needed with a
> sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> only a single clock needs to have its parent or rate specified.

It was also concerning me, but this inconvenience could be mitigated by 
reordering content of clocks/clock-names properties so that the clocks for
which parents and/or rates are being assigned are first on the list.
Then number of padding zeros is minimized.

> Is there a reason not to support both methods?

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

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

[Sylwester Nawrocki]

On 24/02/14 01:48, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
>> > On 20/02/14 15:09, Grant Likely wrote:
>> > [...]
>>>>> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > index 7c52c29..d618498 100644
>>>>> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> > >> > @@ -115,3 +115,27 @@ 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)
>>>>> > >> > +
>>>>> > >> > +==Static initial configuration of clock parent and clock frequency==
>>>>> > >> > +
>>>>> > >> > +Some platforms require static configuration of (parts of) the clock controller
>>>>> > >> > +often determined by the board design. Such a configuration can be specified in
>>>>> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
>>>>> > >> > +properties. The former should contain phandle and clock specifier of the parent
>>>>> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
>>>>> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
>>>>> > >> > +pair corresponding to it should be present in the clocks property.
>>>>> > >> > +
>>>>> > >> > +    uart at a000 {
>>>>> > >> > +        compatible = "fsl,imx-uart";
>>>>> > >> > +        reg = <0xa000 0x1000>;
>>>>> > >> > +  ...
>>>>> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
>>>>> > >> > +        clock-names = "baud", "mux";
>>>>> > >> > +
>>>>> > >> > +  mux-clk-parent = <&pll 1>;
>>>>> > >> > +  baud-clk-rate = <460800>;
>>> > >
>>> > > This mixes patterns for references to clocks. Plus it requires composing
>>> > > property names which is a little painful. I'd rather see a list of
>>> > > tuples to match the existing pattern already in use
>>> > > 
>>> > >       clocks = <&clkcon 0>, <&clkcon 3>;
>>> > >       clock-names = "baud", "mux";
>>> > >       clock-parents = <0> <&pll 1>;
>>> > >       clock-rates = <0> <460800>;
>> > 
>> > Thank you for the review. This looks much better to me. My bad, I wasn't 
>> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
>> > to be specifying exact meaning of the 'phandle' property values, except 
>> > they be unique. Anyway, it seems to be clearly documented within the
>> > __of_parse_phandle_with_args() function. 
>> > 
>> > I'll try this modified binding instead, presumably it would be useful to 
>> > have a variant of __of_parse_phandle_with_args() function which would 
>> > accept a context data containing result of previous call within an iteration, 
>> > similarly as of_property_next_string() is written. So we don't iterate 
>> > from beginning of the list with each __of_parse_phandle_with_args() call. 
>> > But it's an optimization issue that could be considered separately I guess.
>
> I was always partial to the regulator style of blahblah-supply but
> Grant's suggestion is much cleaner with respect to the rest of the clock
> binding.

I don't have a strong opinion, I'm slightly inclined towards Grant's suggestion,
which doesn't have a problem of limiting effective clk name to 21 characters.

Also DT property names with underscores coming from the clock names are a bit
incorrect and it might be not immediately obvious which part of name is a 
canonical property's name and which is clock's name. Let's consider property 
names like:

mux-clk-parent, divider-clk-rate, sclk_mmc0-clk-parent, sclk_uart_baud0-clk-parent,
etc.

> I guess it will be a bit ugly if a very long array is needed with a
> sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> only a single clock needs to have its parent or rate specified.

It was also concerning me, but this inconvenience could be mitigated by 
reordering content of clocks/clock-names properties so that the clocks for
which parents and/or rates are being assigned are first on the list.
Then number of padding zeros is minimized.

> Is there a reason not to support both methods?

--
Regards,
Sylwester

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

[Grant Likely]

On Sun, 23 Feb 2014 16:48:26 -0800, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> > On 20/02/14 15:09, Grant Likely wrote:
> > [...]
> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > index 7c52c29..d618498 100644
> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > @@ -115,3 +115,27 @@ 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)
> > >> > +
> > >> > +==Static initial configuration of clock parent and clock frequency==
> > >> > +
> > >> > +Some platforms require static configuration of (parts of) the clock controller
> > >> > +often determined by the board design. Such a configuration can be specified in
> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> > >> > +properties. The former should contain phandle and clock specifier of the parent
> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
> > >> > +pair corresponding to it should be present in the clocks property.
> > >> > +
> > >> > +    uart@a000 {
> > >> > +        compatible = "fsl,imx-uart";
> > >> > +        reg = <0xa000 0x1000>;
> > >> > +  ...
> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> > >> > +        clock-names = "baud", "mux";
> > >> > +
> > >> > +  mux-clk-parent = <&pll 1>;
> > >> > +  baud-clk-rate = <460800>;
> > >
> > > This mixes patterns for references to clocks. Plus it requires composing
> > > property names which is a little painful. I'd rather see a list of
> > > tuples to match the existing pattern already in use
> > > 
> > >       clocks = <&clkcon 0>, <&clkcon 3>;
> > >       clock-names = "baud", "mux";
> > >       clock-parents = <0> <&pll 1>;
> > >       clock-rates = <0> <460800>;
> > 
> > Thank you for the review. This looks much better to me. My bad, I wasn't 
> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> > to be specifying exact meaning of the 'phandle' property values, except 
> > they be unique. Anyway, it seems to be clearly documented within the
> > __of_parse_phandle_with_args() function. 
> > 
> > I'll try this modified binding instead, presumably it would be useful to 
> > have a variant of __of_parse_phandle_with_args() function which would 
> > accept a context data containing result of previous call within an iteration, 
> > similarly as of_property_next_string() is written. So we don't iterate 
> > from beginning of the list with each __of_parse_phandle_with_args() call. 
> > But it's an optimization issue that could be considered separately I guess.
> 
> I was always partial to the regulator style of blahblah-supply but
> Grant's suggestion is much cleaner with respect to the rest of the clock
> binding.
> 
> I guess it will be a bit ugly if a very long array is needed with a
> sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> only a single clock needs to have its parent or rate specified.
> 
> Is there a reason not to support both methods?

Yes. Complexity. Having two ways to do it is far worse to maintain.

g.

--
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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Grant Likely]

On Sun, 23 Feb 2014 16:48:26 -0800, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> > On 20/02/14 15:09, Grant Likely wrote:
> > [...]
> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > index 7c52c29..d618498 100644
> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > @@ -115,3 +115,27 @@ 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)
> > >> > +
> > >> > +==Static initial configuration of clock parent and clock frequency==
> > >> > +
> > >> > +Some platforms require static configuration of (parts of) the clock controller
> > >> > +often determined by the board design. Such a configuration can be specified in
> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> > >> > +properties. The former should contain phandle and clock specifier of the parent
> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
> > >> > +pair corresponding to it should be present in the clocks property.
> > >> > +
> > >> > +    uart at a000 {
> > >> > +        compatible = "fsl,imx-uart";
> > >> > +        reg = <0xa000 0x1000>;
> > >> > +  ...
> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> > >> > +        clock-names = "baud", "mux";
> > >> > +
> > >> > +  mux-clk-parent = <&pll 1>;
> > >> > +  baud-clk-rate = <460800>;
> > >
> > > This mixes patterns for references to clocks. Plus it requires composing
> > > property names which is a little painful. I'd rather see a list of
> > > tuples to match the existing pattern already in use
> > > 
> > >       clocks = <&clkcon 0>, <&clkcon 3>;
> > >       clock-names = "baud", "mux";
> > >       clock-parents = <0> <&pll 1>;
> > >       clock-rates = <0> <460800>;
> > 
> > Thank you for the review. This looks much better to me. My bad, I wasn't 
> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> > to be specifying exact meaning of the 'phandle' property values, except 
> > they be unique. Anyway, it seems to be clearly documented within the
> > __of_parse_phandle_with_args() function. 
> > 
> > I'll try this modified binding instead, presumably it would be useful to 
> > have a variant of __of_parse_phandle_with_args() function which would 
> > accept a context data containing result of previous call within an iteration, 
> > similarly as of_property_next_string() is written. So we don't iterate 
> > from beginning of the list with each __of_parse_phandle_with_args() call. 
> > But it's an optimization issue that could be considered separately I guess.
> 
> I was always partial to the regulator style of blahblah-supply but
> Grant's suggestion is much cleaner with respect to the rest of the clock
> binding.
> 
> I guess it will be a bit ugly if a very long array is needed with a
> sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> only a single clock needs to have its parent or rate specified.
> 
> Is there a reason not to support both methods?

Yes. Complexity. Having two ways to do it is far worse to maintain.

g.

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

[Grant Likely]

On Mon, 24 Feb 2014 19:11:18 +0100, Sylwester Nawrocki <s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 24/02/14 01:48, Mike Turquette wrote:
> > Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> >> > On 20/02/14 15:09, Grant Likely wrote:
> >> > [...]
> >>>>> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > index 7c52c29..d618498 100644
> >>>>> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > @@ -115,3 +115,27 @@ 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)
> >>>>> > >> > +
> >>>>> > >> > +==Static initial configuration of clock parent and clock frequency==
> >>>>> > >> > +
> >>>>> > >> > +Some platforms require static configuration of (parts of) the clock controller
> >>>>> > >> > +often determined by the board design. Such a configuration can be specified in
> >>>>> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> >>>>> > >> > +properties. The former should contain phandle and clock specifier of the parent
> >>>>> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> >>>>> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
> >>>>> > >> > +pair corresponding to it should be present in the clocks property.
> >>>>> > >> > +
> >>>>> > >> > +    uart@a000 {
> >>>>> > >> > +        compatible = "fsl,imx-uart";
> >>>>> > >> > +        reg = <0xa000 0x1000>;
> >>>>> > >> > +  ...
> >>>>> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> >>>>> > >> > +        clock-names = "baud", "mux";
> >>>>> > >> > +
> >>>>> > >> > +  mux-clk-parent = <&pll 1>;
> >>>>> > >> > +  baud-clk-rate = <460800>;
> >>> > >
> >>> > > This mixes patterns for references to clocks. Plus it requires composing
> >>> > > property names which is a little painful. I'd rather see a list of
> >>> > > tuples to match the existing pattern already in use
> >>> > > 
> >>> > >       clocks = <&clkcon 0>, <&clkcon 3>;
> >>> > >       clock-names = "baud", "mux";
> >>> > >       clock-parents = <0> <&pll 1>;
> >>> > >       clock-rates = <0> <460800>;
> >> > 
> >> > Thank you for the review. This looks much better to me. My bad, I wasn't 
> >> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> >> > to be specifying exact meaning of the 'phandle' property values, except 
> >> > they be unique. Anyway, it seems to be clearly documented within the
> >> > __of_parse_phandle_with_args() function. 
> >> > 
> >> > I'll try this modified binding instead, presumably it would be useful to 
> >> > have a variant of __of_parse_phandle_with_args() function which would 
> >> > accept a context data containing result of previous call within an iteration, 
> >> > similarly as of_property_next_string() is written. So we don't iterate 
> >> > from beginning of the list with each __of_parse_phandle_with_args() call. 
> >> > But it's an optimization issue that could be considered separately I guess.
> >
> > I was always partial to the regulator style of blahblah-supply but
> > Grant's suggestion is much cleaner with respect to the rest of the clock
> > binding.
> 
> I don't have a strong opinion, I'm slightly inclined towards Grant's suggestion,
> which doesn't have a problem of limiting effective clk name to 21 characters.
> 
> Also DT property names with underscores coming from the clock names are a bit
> incorrect and it might be not immediately obvious which part of name is a 
> canonical property's name and which is clock's name. Let's consider property 
> names like:
> 
> mux-clk-parent, divider-clk-rate, sclk_mmc0-clk-parent, sclk_uart_baud0-clk-parent,
> etc.
> 
> > I guess it will be a bit ugly if a very long array is needed with a
> > sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> > only a single clock needs to have its parent or rate specified.
> 
> It was also concerning me, but this inconvenience could be mitigated by 
> reordering content of clocks/clock-names properties so that the clocks for
> which parents and/or rates are being assigned are first on the list.
> Then number of padding zeros is minimized.

If the array really gets that long for a device, then groups of clocks
can also be split off into sub-nodes.

g.
--
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 v1 3/3] clk: Add handling of clk parent and rate assigned from DT

[Grant Likely]

On Mon, 24 Feb 2014 19:11:18 +0100, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> On 24/02/14 01:48, Mike Turquette wrote:
> > Quoting Sylwester Nawrocki (2014-02-21 02:38:21)
> >> > On 20/02/14 15:09, Grant Likely wrote:
> >> > [...]
> >>>>> > >> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > index 7c52c29..d618498 100644
> >>>>> > >> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> > >> > @@ -115,3 +115,27 @@ 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)
> >>>>> > >> > +
> >>>>> > >> > +==Static initial configuration of clock parent and clock frequency==
> >>>>> > >> > +
> >>>>> > >> > +Some platforms require static configuration of (parts of) the clock controller
> >>>>> > >> > +often determined by the board design. Such a configuration can be specified in
> >>>>> > >> > +a clock consumer node through [clk-name]-clk-parent and [clk-name]-clk-rate DT
> >>>>> > >> > +properties. The former should contain phandle and clock specifier of the parent
> >>>>> > >> > +clock, the latter the required clock's frequency value (one cell). "clk-name"
> >>>>> > >> > +should be listed in the clock-names property and a phandle and a clock specifier
> >>>>> > >> > +pair corresponding to it should be present in the clocks property.
> >>>>> > >> > +
> >>>>> > >> > +    uart at a000 {
> >>>>> > >> > +        compatible = "fsl,imx-uart";
> >>>>> > >> > +        reg = <0xa000 0x1000>;
> >>>>> > >> > +  ...
> >>>>> > >> > +        clocks = <&clkcon 0>, <&clkcon 3>;
> >>>>> > >> > +        clock-names = "baud", "mux";
> >>>>> > >> > +
> >>>>> > >> > +  mux-clk-parent = <&pll 1>;
> >>>>> > >> > +  baud-clk-rate = <460800>;
> >>> > >
> >>> > > This mixes patterns for references to clocks. Plus it requires composing
> >>> > > property names which is a little painful. I'd rather see a list of
> >>> > > tuples to match the existing pattern already in use
> >>> > > 
> >>> > >       clocks = <&clkcon 0>, <&clkcon 3>;
> >>> > >       clock-names = "baud", "mux";
> >>> > >       clock-parents = <0> <&pll 1>;
> >>> > >       clock-rates = <0> <460800>;
> >> > 
> >> > Thank you for the review. This looks much better to me. My bad, I wasn't 
> >> > aware 0 can be used to denote an empty phandle like this. ePAPR seems not 
> >> > to be specifying exact meaning of the 'phandle' property values, except 
> >> > they be unique. Anyway, it seems to be clearly documented within the
> >> > __of_parse_phandle_with_args() function. 
> >> > 
> >> > I'll try this modified binding instead, presumably it would be useful to 
> >> > have a variant of __of_parse_phandle_with_args() function which would 
> >> > accept a context data containing result of previous call within an iteration, 
> >> > similarly as of_property_next_string() is written. So we don't iterate 
> >> > from beginning of the list with each __of_parse_phandle_with_args() call. 
> >> > But it's an optimization issue that could be considered separately I guess.
> >
> > I was always partial to the regulator style of blahblah-supply but
> > Grant's suggestion is much cleaner with respect to the rest of the clock
> > binding.
> 
> I don't have a strong opinion, I'm slightly inclined towards Grant's suggestion,
> which doesn't have a problem of limiting effective clk name to 21 characters.
> 
> Also DT property names with underscores coming from the clock names are a bit
> incorrect and it might be not immediately obvious which part of name is a 
> canonical property's name and which is clock's name. Let's consider property 
> names like:
> 
> mux-clk-parent, divider-clk-rate, sclk_mmc0-clk-parent, sclk_uart_baud0-clk-parent,
> etc.
> 
> > I guess it will be a bit ugly if a very long array is needed with a
> > sparse attribute. E.g. 20 clocks specified in the 'clocks' property and
> > only a single clock needs to have its parent or rate specified.
> 
> It was also concerning me, but this inconvenience could be mitigated by 
> reordering content of clocks/clock-names properties so that the clocks for
> which parents and/or rates are being assigned are first on the list.
> Then number of padding zeros is minimized.

If the array really gets that long for a device, then groups of clocks
can also be split off into sub-nodes.

g.