This patch set adds a DT binding documentation for new 'clock-parents'
and 'clock-rates' DT properties and a helper to parse DT and set the
specified clocks configuration. The helper is now called in the platform
bus before driver probing.
Changes since v4:
- added note explaining how to skip setting parent and rate of a clock,
- added missing call to of_node_put(),
- moved of_clk_dev_init() calls to the platform bus,
- dropped debug traces.
Thanks for the reviews.
Currently clock parent/rate setting is only handled automatically for
devices on Linux platform bus; I didn't add it to other busses this
time as the original problem this patch series is trying to solve is
related to the platform devices. However, in general it might make
sense to also add it to other busses, like: amba, I2C, SPI, etc.
The other issue is that in case of deferred probe of_clk_dev_init()
could be called multiple times. To avoid this a field could be added
to struct device or struct platform_device, etc. so a device can be
marked as initialized WRT clocks and initialization could be skipped
before any subsequent deferred probe calls.
I wasn't sure if I should bother generic device data structures with
something like this so I just didn't add it now.
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 | 44 ++++++++++
drivers/base/platform.c | 5 ++
drivers/clk/Makefile | 3 +
drivers/clk/clk-conf.c | 85 ++++++++++++++++++++
drivers/clk/clk.c | 12 ++-
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
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 <[email protected]>
---
Changes since v4:
- none.
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
This patch 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 <[email protected]>
---
Changes since v4:
- added note explaining how to skip setting parent and rate
of a clock,
- moved of_clk_dev_init() calls to the platform bus,
- added missing call to of_node_put(),
- dropped debug traces.
Changes since v3:
- added detailed description of the assigned-clocks subnode,
- added missing 'static inline' to the function stub definition,
- clk-conf.c is now excluded when CONFIG_OF is not set,
- s/of_clk_device_setup/of_clk_device_init.
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 | 44 ++++++++++
drivers/base/platform.c | 5 ++
drivers/clk/Makefile | 3 +
drivers/clk/clk-conf.c | 85 ++++++++++++++++++++
drivers/clk/clk.c | 12 ++-
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..93513fc 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -132,3 +132,47 @@ 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).
+To skip setting parent or rate of a clock its corresponding entry should be
+set to 0, or can be omitted if it is not followed by any non-zero entry.
+
+ 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's 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/platform.c b/drivers/base/platform.c
index e714709..4b95322 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -22,6 +22,7 @@
#include <linux/pm_runtime.h>
#include <linux/idr.h>
#include <linux/acpi.h>
+#include <linux/clk/clk-conf.h>
#include "base.h"
#include "power/power.h"
@@ -481,6 +482,10 @@ static int platform_drv_probe(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret;
+ ret = of_clk_dev_init(_dev->of_node);
+ if (ret < 0)
+ return ret;
+
acpi_dev_pm_attach(_dev, true);
ret = drv->probe(dev);
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 5f8a287..45598f7 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..c0d4096
--- /dev/null
+++ b/drivers/clk/clk-conf.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <[email protected]>
+ *
+ * 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_dev_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_dev_init(struct device_node *node)
+{
+ struct property *prop;
+ const __be32 *cur;
+ int rc, index, num_parents;
+ struct clk *clk, *pclk;
+ u32 rate;
+
+ if (!node)
+ return 0;
+
+ num_parents = of_count_phandle_with_args(node, "clock-parents",
+ "#clock-cells");
+ if (num_parents == -EINVAL)
+ pr_err("clk: invalid value of clock-parents property at %s\n",
+ node->full_name);
+
+ for (index = 0; index < num_parents; index++) {
+ pclk = of_clk_get_by_property(node, "clock-parents", index);
+ if (IS_ERR(pclk)) {
+ /* skip empty (null) phandles */
+ if (PTR_ERR(pclk) == -ENOENT)
+ continue;
+
+ pr_warn("clk: couldn't get parent clock %d for %s\n",
+ index, node->full_name);
+
+ return PTR_ERR(pclk);
+ }
+
+ clk = of_clk_get(node, index);
+ if (IS_ERR(clk)) {
+ pr_warn("clk: couldn't get clock %d for %s\n",
+ index, node->full_name);
+ return PTR_ERR(clk);
+ }
+
+ rc = clk_set_parent(clk, pclk);
+ if (rc < 0)
+ pr_err("clk: failed to reparent %s to %s: %d\n",
+ __clk_get_name(clk), __clk_get_name(pclk), rc);
+ }
+
+ 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);
+ }
+ index++;
+ }
+
+ return 0;
+}
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dff0373..0d72a4d 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,17 @@ 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_dev_init(np);
+ of_node_put(np);
+ }
+
list_del(&clk_provider->node);
kfree(clk_provider);
is_init_done = true;
@@ -2635,7 +2646,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..afb4228
--- /dev/null
+++ b/include/linux/clk/clk-conf.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Sylwester Nawrocki <[email protected]>
+ *
+ * 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_dev_init(struct device_node *node);
+#else
+static inline int of_clk_dev_init(struct device_node *node)
+{
+ return 0;
+}
+#endif
--
1.7.9.5
On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki
<[email protected]> wrote:
> This patch 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 <[email protected]>
> ---
> Changes since v4:
> - added note explaining how to skip setting parent and rate
> of a clock,
> - moved of_clk_dev_init() calls to the platform bus,
> - added missing call to of_node_put(),
> - dropped debug traces.
>
> Changes since v3:
> - added detailed description of the assigned-clocks subnode,
> - added missing 'static inline' to the function stub definition,
> - clk-conf.c is now excluded when CONFIG_OF is not set,
> - s/of_clk_device_setup/of_clk_device_init.
>
> 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 | 44 ++++++++++
> drivers/base/platform.c | 5 ++
> drivers/clk/Makefile | 3 +
> drivers/clk/clk-conf.c | 85 ++++++++++++++++++++
> drivers/clk/clk.c | 12 ++-
> 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..93513fc 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -132,3 +132,47 @@ 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).
> +To skip setting parent or rate of a clock its corresponding entry should be
> +set to 0, or can be omitted if it is not followed by any non-zero entry.
> +
> + 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>;
Is this the input frequency or serial baud rate? Looks like a baud
rate, but the clock framework needs input (to the uart) frequency. I
would say this should be clock-frequency and specify the max baud rate
as is being done with i2c bindings. The uart driver should know how to
convert between input clock freq and baud rate.
> + };
> +
> +In this example the pll is set as parent of "mux" clock and frequency of "baud"
> +clock is specified as 460800 Hz.
I don't really like clock-parents. The parent information is part of
the clock source, not the consumer.
We've somewhat decided against having every single clock defined in DT
and rather only describe a clock controller with leaf clocks to
devices. That is not a hard rule, but for complex clock trees that is
the norm. Doing something like this will require all levels of the
clock tree to be described. You may have multiple layers of parents
that have to be configured correctly. How are you configuring the rest
of the tree?
> +
> +Configuring a clock's 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.:
This seems like a work-around due to having clock-parents in the
consumer node. If (I'm not convinced we should) we have a binding for
parent config, it needs to be a single binding that works for both
cases.
> +
> + clkcon {
> + ...
> + #clock-cells = <1>;
> +
> + assigned-clocks {
> + clocks = <&clkcon 16>, <&clkcon 17>;
> + clock-parents = <0>, <&clkcon 1>;
> + clock-rates = <200000>;
> + };
> + };
Rob
On 10/04/14 18:04, Rob Herring wrote:
> On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki
> <[email protected]> wrote:
>> This patch 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 <[email protected]>
>> ---
[...]
>> ---
>> .../devicetree/bindings/clock/clock-bindings.txt | 44 ++++++++++
>> drivers/base/platform.c | 5 ++
>> drivers/clk/Makefile | 3 +
>> drivers/clk/clk-conf.c | 85 ++++++++++++++++++++
>> drivers/clk/clk.c | 12 ++-
>> 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..93513fc 100644
>> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
>> @@ -132,3 +132,47 @@ 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).
>> +To skip setting parent or rate of a clock its corresponding entry should be
>> +set to 0, or can be omitted if it is not followed by any non-zero entry.
>> +
>> + 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>;
>
> Is this the input frequency or serial baud rate? Looks like a baud
> rate, but the clock framework needs input (to the uart) frequency. I
> would say this should be clock-frequency and specify the max baud rate
> as is being done with i2c bindings. The uart driver should know how to
> convert between input clock freq and baud rate.
This UART example is not quite representative for the issues I have been
trying to address with this patch set. There is a need to set (an initial)
input clock frequency. E.g. in case of multimedia devices there may be
a need to set clock parent and frequency of an input clock to multiple IP
blocks, so they are clocked synchronously and data is carried properly
across a whole processing chain. Thus there may not be even clock output
in an IP block, but still input clock needs to be set. IIUC there is
similar issue with audio, where it is difficult to calculate the clock
frequencies/determine parent clocks in individual drivers algorithmically.
>> + };
>> +
>> +In this example the pll is set as parent of "mux" clock and frequency
>> of "baud"
>> +clock is specified as 460800 Hz.
>
> I don't really like clock-parents. The parent information is part of
> the clock source, not the consumer.
I'm not sure we must always consider the parent information as property
of a clock source. If for example we expose a structure like below as
single clock object, supporting clock gating, parent and frequency
setting the parent setting is still accessible from within a device
driver. And clock parent selection may depend on a system configuration
not immediately obvious from within a single device driver perspective.
MUX
,-------. DIVIDER GATE
common clk source 1 -->|--. | ,--------. ,--------.
| \ | | | | |
common clk source 2 -->|- '--|-->| |-->| |--> consumer
... | | | | | |
common clk source N -->|- | '--------' '--------'
'-------'
> We've somewhat decided against having every single clock defined in DT
> and rather only describe a clock controller with leaf clocks to
> devices. That is not a hard rule, but for complex clock trees that is
> the norm. Doing something like this will require all levels of the
> clock tree to be described. You may have multiple layers of parents
> that have to be configured correctly. How are you configuring the rest
> of the tree?
I believe even clock controllers where clocks are represented as flat
array often describe the clock tree entirely by parenthood, the tree
structure is just not obvious from the DT binding.
In addition, there seems to be appearing more and more clock controller
DT bindings describing their clocks individually.
>> +Configuring a clock's 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.:
>
> This seems like a work-around due to having clock-parents in the
> consumer node. If (I'm not convinced we should) we have a binding for
> parent config, it needs to be a single binding that works for both
> cases.
When this issue was first raised during an ARM kernel summit it was
proposed to add 'assigned' prefix to DT properties for such bindings.
How about separate properties for the default clock configuration,
e.g. assigned-clocks/assigned-clock-parents/assigned-clock-rates ?
So a clock provider would look like:
clkcon {
...
#clock-cells = <1>;
assigned-clocks = <&clkcon 16>, <&clkcon 17>;
assigned-clock-parents = <0>, <&clkcon 1>;
assigned-clock-rates = <200000>;
};
And a consumer device node:
uart@a000 {
compatible = "fsl,imx-uart";
reg = <0xa000 0x1000>;
...
clocks = <&clkcon 0>;
clock-names = "baud";
assigned-clocks = <&clkcon 3>, <&clkcon 0>;
assigned-clock-parents = <&pll 1>;
assigned-clock-rates = <0>, <460800>;
};
?
>> +
>> + clkcon {
>> + ...
>> + #clock-cells = <1>;
>> +
>> + assigned-clocks {
>> + clocks = <&clkcon 16>, <&clkcon 17>;
>> + clock-parents = <0>, <&clkcon 1>;
>> + clock-rates = <200000>;
>> + };
>> + };
--
Thanks,
Sylwester
On Friday 11 April 2014 14:25:49 Sylwester Nawrocki wrote:
> On 10/04/14 18:04, Rob Herring wrote:
> > On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki wrote:
> >> This patch 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 <[email protected]>
> >> ---
>
> [...]
>
> >> ---
> >>
> >> .../devicetree/bindings/clock/clock-bindings.txt | 44 ++++++++++
> >> drivers/base/platform.c | 5 ++
> >> drivers/clk/Makefile | 3 +
> >> drivers/clk/clk-conf.c | 85 ++++++++++++++
> >> drivers/clk/clk.c | 12 ++-
> >> 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..93513fc 100644
> >> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> @@ -132,3 +132,47 @@ 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).
> >> +To skip setting parent or rate of a clock its corresponding entry should
> >> be
> >> +set to 0, or can be omitted if it is not followed by any non-zero entry.
> >> +
> >> + 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>;
> >
> > Is this the input frequency or serial baud rate? Looks like a baud
> > rate, but the clock framework needs input (to the uart) frequency. I
> > would say this should be clock-frequency and specify the max baud rate
> > as is being done with i2c bindings. The uart driver should know how to
> > convert between input clock freq and baud rate.
>
> This UART example is not quite representative for the issues I have been
> trying to address with this patch set. There is a need to set (an initial)
> input clock frequency. E.g. in case of multimedia devices there may be
> a need to set clock parent and frequency of an input clock to multiple IP
> blocks, so they are clocked synchronously and data is carried properly
> across a whole processing chain. Thus there may not be even clock output
> in an IP block, but still input clock needs to be set. IIUC there is
> similar issue with audio, where it is difficult to calculate the clock
> frequencies/determine parent clocks in individual drivers algorithmically.
Just to be used as an example, this is how the SMIA++ sensor driver computes
the PLL parameters automatically based on the input frequency, desired output
frequency and various hardware limits.
http://lxr.free-electrons.com/source/drivers/media/i2c/smiapp-pll.c
See the code complexity and keep in mind that it only handles a single device
with a single set of constraints and a single parent. If we add several
devices to the mix, as well as selectable parents, there would indeed probably
be no sane way to configure the clocks algorithmically.
> >> + };
> >> +
> >> +In this example the pll is set as parent of "mux" clock and frequency
> >> of "baud"
> >> +clock is specified as 460800 Hz.
> >
> > I don't really like clock-parents. The parent information is part of
> > the clock source, not the consumer.
>
> I'm not sure we must always consider the parent information as property
> of a clock source. If for example we expose a structure like below as
> single clock object, supporting clock gating, parent and frequency
> setting the parent setting is still accessible from within a device
> driver. And clock parent selection may depend on a system configuration
> not immediately obvious from within a single device driver perspective.
>
> MUX
> ,-------. DIVIDER GATE
> common clk source 1 -->|--. | ,--------. ,--------.
> | \ | | | | |
> common clk source 2 -->|- '--|-->| |-->| |--> consumer
> ... | | | | | |
> common clk source N -->|- | '--------' '--------'
> '-------'
>
> > We've somewhat decided against having every single clock defined in DT
> > and rather only describe a clock controller with leaf clocks to
> > devices. That is not a hard rule, but for complex clock trees that is
> > the norm. Doing something like this will require all levels of the
> > clock tree to be described. You may have multiple layers of parents
> > that have to be configured correctly. How are you configuring the rest
> > of the tree?
>
> I believe even clock controllers where clocks are represented as flat
> array often describe the clock tree entirely by parenthood, the tree
> structure is just not obvious from the DT binding.
> In addition, there seems to be appearing more and more clock controller
> DT bindings describing their clocks individually.
>
> >> +Configuring a clock's 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.:
> >
> > This seems like a work-around due to having clock-parents in the
> > consumer node. If (I'm not convinced we should) we have a binding for
> > parent config, it needs to be a single binding that works for both
> > cases.
>
> When this issue was first raised during an ARM kernel summit it was
> proposed to add 'assigned' prefix to DT properties for such bindings.
>
> How about separate properties for the default clock configuration,
> e.g. assigned-clocks/assigned-clock-parents/assigned-clock-rates ?
> So a clock provider would look like:
>
> clkcon {
> ...
> #clock-cells = <1>;
>
> assigned-clocks = <&clkcon 16>, <&clkcon 17>;
> assigned-clock-parents = <0>, <&clkcon 1>;
> assigned-clock-rates = <200000>;
> };
>
> And a consumer device node:
>
> uart@a000 {
> compatible = "fsl,imx-uart";
> reg = <0xa000 0x1000>;
> ...
> clocks = <&clkcon 0>;
> clock-names = "baud";
>
> assigned-clocks = <&clkcon 3>, <&clkcon 0>;
> assigned-clock-parents = <&pll 1>;
> assigned-clock-rates = <0>, <460800>;
> };
>
> ?
>
> >> +
> >> + clkcon {
> >> + ...
> >> + #clock-cells = <1>;
> >> +
> >> + assigned-clocks {
> >> + clocks = <&clkcon 16>, <&clkcon 17>;
> >> + clock-parents = <0>, <&clkcon 1>;
> >> + clock-rates = <200000>;
> >> + };
> >> + };
--
Regards,
Laurent Pinchart
On 23/05/14 03:34, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-04-11 05:25:49)
>>>> +==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).
>>>> +To skip setting parent or rate of a clock its corresponding entry should be
>>>> +set to 0, or can be omitted if it is not followed by any non-zero entry.
>>>> +
>>>> + 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>;
>>>
>>> Is this the input frequency or serial baud rate? Looks like a baud
>>> rate, but the clock framework needs input (to the uart) frequency. I
>>> would say this should be clock-frequency and specify the max baud rate
>>> as is being done with i2c bindings. The uart driver should know how to
>>> convert between input clock freq and baud rate.
>>
>> This UART example is not quite representative for the issues I have been
>> trying to address with this patch set. There is a need to set (an initial)
>> input clock frequency. E.g. in case of multimedia devices there may be
>> a need to set clock parent and frequency of an input clock to multiple IP
>> blocks, so they are clocked synchronously and data is carried properly
>> across a whole processing chain. Thus there may not be even clock output
>> in an IP block, but still input clock needs to be set. IIUC there is
>> similar issue with audio, where it is difficult to calculate the clock
>> frequencies/determine parent clocks in individual drivers algorithmically.
>>
>>>> + };
>>>> +
>>>> +In this example the pll is set as parent of "mux" clock and frequency
>>>> of "baud"
>>>> +clock is specified as 460800 Hz.
>>>
>>> I don't really like clock-parents. The parent information is part of
>>> the clock source, not the consumer.
>>
>> I'm not sure we must always consider the parent information as property
>> of a clock source. If for example we expose a structure like below as
>> single clock object, supporting clock gating, parent and frequency
>> setting the parent setting is still accessible from within a device
>> driver.
>
> The design of the ccf implementation certainly allows one to hide
> individually addressable/configurable clock nodes within a single struct
> clk. But should we? I have always maintained that a clock driver should
> enumerate clocks in the same way that the data sheet or technical
> reference manual states. I did make a recent exception[1], but that is
> going to be rolled back after the coordinated clock rate changes land in
> mainline.
>
>> And clock parent selection may depend on a system configuration
>> not immediately obvious from within a single device driver perspective.
>>
>> MUX
>> ,-------. DIVIDER GATE
>> common clk source 1 -->|--. | ,--------. ,--------.
>> | \ | | | | |
>> common clk source 2 -->|- '--|-->| |-->| |--> consumer
>> ... | | | | | |
>> common clk source N -->|- | '--------' '--------'
>> '-------'
>>
>>> We've somewhat decided against having every single clock defined in DT
>>> and rather only describe a clock controller with leaf clocks to
>>> devices. That is not a hard rule, but for complex clock trees that is
>>> the norm. Doing something like this will require all levels of the
>>> clock tree to be described. You may have multiple layers of parents
>>> that have to be configured correctly. How are you configuring the rest
>>> of the tree?
>>
>> I believe even clock controllers where clocks are represented as flat
>> array often describe the clock tree entirely by parenthood, the tree
>> structure is just not obvious from the DT binding.
>> In addition, there seems to be appearing more and more clock controller
>> DT bindings describing their clocks individually.
>
> I've been discouraging these per-clock node bindings in favor of the
> per-controller node style.
>
>>
>>>> +Configuring a clock's 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.:
>>>
>>> This seems like a work-around due to having clock-parents in the
>>> consumer node. If (I'm not convinced we should) we have a binding for
>>> parent config, it needs to be a single binding that works for both
>>> cases.
>>
>> When this issue was first raised during an ARM kernel summit it was
>> proposed to add 'assigned' prefix to DT properties for such bindings.
>>
>
> Yes, I like the "assigned-" prefix.
>
>> How about separate properties for the default clock configuration,
>> e.g. assigned-clocks/assigned-clock-parents/assigned-clock-rates ?
>> So a clock provider would look like:
>>
>> clkcon {
>> ...
>> #clock-cells = <1>;
>>
>> assigned-clocks = <&clkcon 16>, <&clkcon 17>;
>> assigned-clock-parents = <0>, <&clkcon 1>;
>> assigned-clock-rates = <200000>;
>> };
>>
>> And a consumer device node:
>>
>> uart@a000 {
>> compatible = "fsl,imx-uart";
>> reg = <0xa000 0x1000>;
>> ...
>> clocks = <&clkcon 0>;
>> clock-names = "baud";
>>
>> assigned-clocks = <&clkcon 3>, <&clkcon 0>;
>> assigned-clock-parents = <&pll 1>;
>> assigned-clock-rates = <0>, <460800>;
>> };
>
> It looks like this idea was dropped for v6. Can we revisit it? Take a
Yeah, we discussed this internally and some ones where not too excited
about such new properties, so I tried implementation of the bindings which
Grant initially suggested.
Having separate properties like these has an advantage though that the
clocks configuration specification is separated and not mixed with
regular clock properties describing solely the clock connections.
> look at Tero's example implementation for OMAP using this binding:
>
> http://www.spinics.net/lists/linux-omap/msg104705.html
>
> There is a bogus "default-clocks" node made solely for storing this info
> within the OMAP PRCM clock provider node. This is basically faking a
> clock consumer. I think with the proposed solution above Tero could have
> avoided that node entirely and done the following:
OK, I'm going to prepare new version of this series with the altered
DT binding.
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> index 649b5cd..e3ff1a7 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -145,6 +145,11 @@
> cm2_clocks: clocks {
> #address-cells = <1>;
> #size-cells = <0>;
> +
> + assigned-clocks = <&abe_dpll_refclk_mux_ck>,
> + <&dpll_usb_ck>, <&dpll_abe_ck>;
> + assigned-clock-parents = <&sys_32k_ck>;
> + assigned-clock-rates = <0>, <960000000>, <98304000>;
> };
>
> cm2_clockdomains: clockdomains {
>
>
> Tero, what do you think?
>
> Regards,
> Mike
>
> [1] http://www.spinics.net/lists/cpufreq/msg10071.html
--
Regards,
Sylwester
On 23/05/14 08:37, Tero Kristo wrote:
> On 05/23/2014 04:34 AM, Mike Turquette wrote:
[...]
>> It looks like this idea was dropped for v6. Can we revisit it? Take a
>> look at Tero's example implementation for OMAP using this binding:
>>
>> http://www.spinics.net/lists/linux-omap/msg104705.html
>>
>> There is a bogus "default-clocks" node made solely for storing this info
>> within the OMAP PRCM clock provider node. This is basically faking a
>> clock consumer. I think with the proposed solution above Tero could have
>> avoided that node entirely and done the following:
>>
>> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
>> index 649b5cd..e3ff1a7 100644
>> --- a/arch/arm/boot/dts/omap4.dtsi
>> +++ b/arch/arm/boot/dts/omap4.dtsi
>> @@ -145,6 +145,11 @@
>> cm2_clocks: clocks {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> +
>> + assigned-clocks = <&abe_dpll_refclk_mux_ck>,
>> + <&dpll_usb_ck>, <&dpll_abe_ck>;
>> + assigned-clock-parents = <&sys_32k_ck>;
>> + assigned-clock-rates = <0>, <960000000>, <98304000>;
>> };
>>
>> cm2_clockdomains: clockdomains {
>>
>>
>> Tero, what do you think?
>
> Yeah, if we can avoid having a dummy node someplace, it is always
> better. Only issue might be the initialization order, this was the
> reason I created the dummy node if I recall right. But I guess we can
> just scan the clock provider nodes second time at a later phase of boot
> (or just store the default info for later use.)
One issue I'm a bit concerned about with an initcall-like approach is
it may not work well for clock providers in kernel modules which can be
loaded at any time. So doing the configuration upon the clock provider
registration might have worked better. Then we could disallow (defer)
a clock provider registration if any of its dependencies (as specified
through assigned-clock* properties) are not satisfied. Do you think
that could work ?
--
Thanks,
Sylwester