| Message ID | 1396284116-19178-3-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 31/03/14 17:41, Sylwester Nawrocki wrote: > This function adds a helper function to configure clock parents and rates > as specified in clock-parents, clock-rates DT properties for a consumer > device and a call to it before driver is bound to a device. [snip] tree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > index 700e7aa..59fbb4e 100644 > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > @@ -132,3 +132,45 @@ clock signal, and a UART. > ("pll" and "pll-switched"). > * The UART has its baud clock connected the external oscillator and its > register clock connected to the PLL clock (the "pll-switched" signal) > + > +==Assigned clock parents and rates== > + > +Some platforms require static initial configuration of parts of the clocks > +controller. Such a configuration can be specified in a clock consumer node > +through clock-parents and clock-rates DT properties. The former should > +contain a list of parent clocks in form of phandle and clock specifier pairs, > +the latter the list of assigned clock frequency values (one cell each). > + > + uart@a000 { > + compatible = "fsl,imx-uart"; > + reg = <0xa000 0x1000>; > + ... > + clocks = <&clkcon 0>, <&clkcon 3>; > + clock-names = "baud", "mux"; > + > + clock-parents = <0>, <&pll 1>; > + clock-rates = <460800>; > + }; > + > +In this example the pll is set as parent of "mux" clock and frequency of "baud" > +clock is specified as 460800 Hz. > + > +Configuring a clock parent and rate through the device node that uses > +the clock can be done only for clocks that have a single user. Specifying > +conflicting parent or rate configuration in multiple consumer nodes for > +a shared clock is forbidden. > + > +Configuration of common clocks, which affect multiple consumer devices > +can be specified in a dedicated 'assigned-clocks' subnode of a clock > +provider node, e.g.: > + > + clkcon { > + ... > + #clock-cells = <1>; > + > + assigned-clocks { > + clocks = <&clkcon 16>, <&clkcon 17>; > + clock-parents = <0>, <&clkcon 1>; > + clock-rates = <200000>; > + }; > + }; How do you support not-setting a rate for a clock? [snip code]
On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: > This function adds a helper function to configure clock parents and rates > as specified in clock-parents, clock-rates DT properties for a consumer > device and a call to it before driver is bound to a device. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v3: > - added detailed description of the assigned-clocks subnode, > - clk-conf.c is now excluded when CONFIG_OF is not set, > - s/of_clk_device_setup/of_clk_device_init, > - added missing 'static inline' to the function stub definition. > > Changes since v2: > - edited in clock-bindings.txt, added note about 'assigned-clocks' > subnode which may be used to specify "global" clocks configuration > at a clock provider node, > - moved of_clk_device_setup() function declaration from clk-provider.h > to clk-conf.h so required function stubs are available when > CONFIG_COMMON_CLK is not enabled, > > Changes since v1: > - the helper function to parse and set assigned clock parents and > rates made public so it is available to clock providers to call > directly; > - dropped the platform bus notification and call of_clk_device_setup() > is is now called from the driver core, rather than from the > notification callback; > - s/of_clk_get_list_entry/of_clk_get_by_property. > --- > .../devicetree/bindings/clock/clock-bindings.txt | 42 ++++++++++ > drivers/base/dd.c | 7 ++ > drivers/clk/Makefile | 3 + > drivers/clk/clk-conf.c | 87 ++++++++++++++++++++ > drivers/clk/clk.c | 10 ++- > include/linux/clk/clk-conf.h | 19 +++++ > 6 files changed, 167 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/clk-conf.c > create mode 100644 include/linux/clk/clk-conf.h > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt > index 700e7aa..59fbb4e 100644 > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > @@ -132,3 +132,45 @@ clock signal, and a UART. > ("pll" and "pll-switched"). > * The UART has its baud clock connected the external oscillator and its > register clock connected to the PLL clock (the "pll-switched" signal) > + > +==Assigned clock parents and rates== > + > +Some platforms require static initial configuration of parts of the clocks > +controller. Such a configuration can be specified in a clock consumer node > +through clock-parents and clock-rates DT properties. The former should > +contain a list of parent clocks in form of phandle and clock specifier pairs, > +the latter the list of assigned clock frequency values (one cell each). > + > + uart@a000 { > + compatible = "fsl,imx-uart"; > + reg = <0xa000 0x1000>; > + ... > + clocks = <&clkcon 0>, <&clkcon 3>; > + clock-names = "baud", "mux"; > + > + clock-parents = <0>, <&pll 1>; > + clock-rates = <460800>; > + }; > + > +In this example the pll is set as parent of "mux" clock and frequency of "baud" > +clock is specified as 460800 Hz. > + > +Configuring a clock parent and rate through the device node that uses > +the clock can be done only for clocks that have a single user. Specifying > +conflicting parent or rate configuration in multiple consumer nodes for > +a shared clock is forbidden. > + > +Configuration of common clocks, which affect multiple consumer devices > +can be specified in a dedicated 'assigned-clocks' subnode of a clock > +provider node, e.g.: > + > + clkcon { > + ... > + #clock-cells = <1>; > + > + assigned-clocks { > + clocks = <&clkcon 16>, <&clkcon 17>; > + clock-parents = <0>, <&clkcon 1>; > + clock-rates = <200000>; > + }; > + }; > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 0605176..4c633e7 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -25,6 +25,7 @@ > #include <linux/async.h> > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > +#include <linux/clk/clk-conf.h> > > #include "base.h" > #include "power/power.h" > @@ -278,6 +279,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) > if (ret) > goto probe_failed; > > + if (dev->of_node) { > + ret = of_clk_device_init(dev->of_node); > + if (ret) > + goto probe_failed; > + } > + > if (driver_sysfs_add(dev)) { > printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n", > __func__, dev_name(dev)); I don't understand why you need the driver core to initialize this one type of thing? That should be in a driver, or in a class, or at worse case, the platform code. What makes clocks so "unique" here? greg k-h
On Mon, Mar 31, 2014 at 06:04:54PM +0100, Ben Dooks wrote: > On 31/03/14 17:41, Sylwester Nawrocki wrote: > >This function adds a helper function to configure clock parents and rates > >as specified in clock-parents, clock-rates DT properties for a consumer > >device and a call to it before driver is bound to a device. > > [snip] > > tree/bindings/clock/clock-bindings.txt > b/Documentation/devicetree/bindings/clock/clock-bindings.txt > >index 700e7aa..59fbb4e 100644 > >--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt > >+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt > >@@ -132,3 +132,45 @@ clock signal, and a UART. > > ("pll" and "pll-switched"). > > * The UART has its baud clock connected the external oscillator and its > > register clock connected to the PLL clock (the "pll-switched" signal) > >+ > >+==Assigned clock parents and rates== > >+ > >+Some platforms require static initial configuration of parts of the clocks > >+controller. Such a configuration can be specified in a clock consumer node > >+through clock-parents and clock-rates DT properties. The former should > >+contain a list of parent clocks in form of phandle and clock specifier pairs, > >+the latter the list of assigned clock frequency values (one cell each). > >+ > >+ uart@a000 { > >+ compatible = "fsl,imx-uart"; > >+ reg = <0xa000 0x1000>; > >+ ... > >+ clocks = <&clkcon 0>, <&clkcon 3>; > >+ clock-names = "baud", "mux"; > >+ > >+ clock-parents = <0>, <&pll 1>; > >+ clock-rates = <460800>; > >+ }; > >+ > >+In this example the pll is set as parent of "mux" clock and frequency of "baud" > >+clock is specified as 460800 Hz. > >+ > >+Configuring a clock parent and rate through the device node that uses > >+the clock can be done only for clocks that have a single user. Specifying > >+conflicting parent or rate configuration in multiple consumer nodes for > >+a shared clock is forbidden. > >+ > >+Configuration of common clocks, which affect multiple consumer devices > >+can be specified in a dedicated 'assigned-clocks' subnode of a clock > >+provider node, e.g.: > >+ > >+ clkcon { > >+ ... > >+ #clock-cells = <1>; > >+ > >+ assigned-clocks { > >+ clocks = <&clkcon 16>, <&clkcon 17>; > >+ clock-parents = <0>, <&clkcon 1>; > >+ clock-rates = <200000>; > >+ }; > >+ }; > > How do you support not-setting a rate for a clock? Not setting a rate is supported by specifying the rate to 0. That should be documented of course. Sascha
On 01/04/14 08:23, Sascha Hauer wrote: >> tree/bindings/clock/clock-bindings.txt >> > b/Documentation/devicetree/bindings/clock/clock-bindings.txt >>> > >index 700e7aa..59fbb4e 100644 >>> > >--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt >>> > >+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt >>> > >@@ -132,3 +132,45 @@ clock signal, and a UART. >>> > > ("pll" and "pll-switched"). >>> > > * The UART has its baud clock connected the external oscillator and its >>> > > register clock connected to the PLL clock (the "pll-switched" signal) >>> > >+ >>> > >+==Assigned clock parents and rates== >>> > >+ >>> > >+Some platforms require static initial configuration of parts of the clocks >>> > >+controller. Such a configuration can be specified in a clock consumer node >>> > >+through clock-parents and clock-rates DT properties. The former should >>> > >+contain a list of parent clocks in form of phandle and clock specifier pairs, >>> > >+the latter the list of assigned clock frequency values (one cell each). >>> > >+ >>> > >+ uart@a000 { >>> > >+ compatible = "fsl,imx-uart"; >>> > >+ reg = <0xa000 0x1000>; >>> > >+ ... >>> > >+ clocks = <&clkcon 0>, <&clkcon 3>; >>> > >+ clock-names = "baud", "mux"; >>> > >+ >>> > >+ clock-parents = <0>, <&pll 1>; >>> > >+ clock-rates = <460800>; >>> > >+ }; >>> > >+ >>> > >+In this example the pll is set as parent of "mux" clock and frequency of "baud" >>> > >+clock is specified as 460800 Hz. [...] >> > >> > How do you support not-setting a rate for a clock? > > Not setting a rate is supported by specifying the rate to 0. That should > be documented of course. Yes, a rate won't be set for a clock if its corresponding entry in clock-rates property is set to 0. Sorry, should have mentioned it. Would adding a sentence as below to end of the first paragraph above make it clear ? "To skip setting a rate or parent for a clock the value of a corresponding entry in the clock-rates or clock-parents property respectively should be set to 0. The trailing zeros can be omitted." -- Thanks, Sylwester
Hi Sylwester, Thank you for the patch. On Monday 31 March 2014 18:41:56 Sylwester Nawrocki wrote: > This function adds a helper function to configure clock parents and rates > as specified in clock-parents, clock-rates DT properties for a consumer > device and a call to it before driver is bound to a device. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > Changes since v3: > - added detailed description of the assigned-clocks subnode, > - clk-conf.c is now excluded when CONFIG_OF is not set, > - s/of_clk_device_setup/of_clk_device_init, > - added missing 'static inline' to the function stub definition. > > Changes since v2: > - edited in clock-bindings.txt, added note about 'assigned-clocks' > subnode which may be used to specify "global" clocks configuration > at a clock provider node, > - moved of_clk_device_setup() function declaration from clk-provider.h > to clk-conf.h so required function stubs are available when > CONFIG_COMMON_CLK is not enabled, > > Changes since v1: > - the helper function to parse and set assigned clock parents and > rates made public so it is available to clock providers to call > directly; > - dropped the platform bus notification and call of_clk_device_setup() > is is now called from the driver core, rather than from the > notification callback; > - s/of_clk_get_list_entry/of_clk_get_by_property. > --- > .../devicetree/bindings/clock/clock-bindings.txt | 42 ++++++++++ > drivers/base/dd.c | 7 ++ > drivers/clk/Makefile | 3 + > drivers/clk/clk-conf.c | 87 +++++++++++++++++ > drivers/clk/clk.c | 10 ++- > include/linux/clk/clk-conf.h | 19 +++++ > 6 files changed, 167 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/clk-conf.c > create mode 100644 include/linux/clk/clk-conf.h [snip] > diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c > new file mode 100644 > index 0000000..a2e992e > --- /dev/null > +++ b/drivers/clk/clk-conf.c > @@ -0,0 +1,87 @@ > +/* > + * Copyright (C) 2014 Samsung Electronics Co., Ltd. > + * Sylwester Nawrocki <s.nawrocki@samsung.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/of.h> > +#include <linux/printk.h> > + > +/** > + * of_clk_device_init() - parse and set clk configuration assigned to a > device > + * @node: device node to apply the configuration for > + * > + * This function parses 'clock-parents' and 'clock-rates' properties and > sets > + * any specified clock parents and rates. > + */ > +int of_clk_device_init(struct device_node *node) > +{ > + struct property *prop; > + const __be32 *cur; > + int rc, index, num_parents; > + struct clk *clk, *pclk; > + u32 rate; > + > + num_parents = of_count_phandle_with_args(node, "clock-parents", > + "#clock-cells"); > + if (num_parents == -EINVAL) > + pr_err("clk: Invalid value of clock-parents property at %s\n", > + node->full_name); This is an implementation detail, but wouldn't it simplify the code if you merged the two loops by iterating of the clocks property instead of over the clock-parents and clock-rates properties separately ? > + for (index = 0; index < num_parents; index++) { > + pclk = of_clk_get_by_property(node, "clock-parents", index); > + if (IS_ERR(pclk)) { > + /* skip empty (null) phandles */ > + if (PTR_ERR(pclk) == -ENOENT) > + continue; > + > + pr_warn("clk: couldn't get parent clock %d for %s\n", > + index, node->full_name); > + return PTR_ERR(pclk); > + } > + > + clk = of_clk_get(node, index); > + if (IS_ERR(clk)) { > + pr_warn("clk: couldn't get clock %d for %s\n", > + index, node->full_name); > + return PTR_ERR(clk); > + } > + > + rc = clk_set_parent(clk, pclk); > + if (rc < 0) > + pr_err("clk: failed to reparent %s to %s: %d\n", > + __clk_get_name(clk), __clk_get_name(pclk), rc); > + else > + pr_debug("clk: set %s as parent of %s\n", > + __clk_get_name(pclk), __clk_get_name(clk)); > + } > + > + index = 0; > + of_property_for_each_u32(node, "clock-rates", prop, cur, rate) { > + if (rate) { > + clk = of_clk_get(node, index); > + if (IS_ERR(clk)) { > + pr_warn("clk: couldn't get clock %d for %s\n", > + index, node->full_name); > + return PTR_ERR(clk); > + } > + > + rc = clk_set_rate(clk, rate); > + if (rc < 0) > + pr_err("clk: couldn't set %s clock rate: %d\n", > + __clk_get_name(clk), rc); > + else > + pr_debug("clk: set rate of clock %s to %lu\n", > + __clk_get_name(clk), clk_get_rate(clk)); > + } > + index++; > + } > + > + return 0; > +} > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index dff0373..ea6d8bd 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c [snip] > @@ -2620,7 +2621,15 @@ void __init of_clk_init(const struct of_device_id > *matches) list_for_each_entry_safe(clk_provider, next, > &clk_provider_list, node) { > if (force || parent_ready(clk_provider->np)) { > + > clk_provider->clk_init_cb(clk_provider->np); > + > + /* Set any assigned clock parents and rates */ > + np = of_get_child_by_name(clk_provider->np, > + "assigned-clocks"); > + if (np) > + of_clk_device_init(np); Aren't you missing an of_node_put() here ? > + > list_del(&clk_provider->node); > kfree(clk_provider); > is_init_done = true;
On 31/03/14 21:06, Greg KH wrote: > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: >> This function adds a helper function to configure clock parents and rates >> as specified in clock-parents, clock-rates DT properties for a consumer >> device and a call to it before driver is bound to a device. >> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> [snip] > > I don't understand why you need the driver core to initialize this one > type of thing? That should be in a driver, or in a class, or at worse > case, the platform code. > > What makes clocks so "unique" here? I suppose the issue here is that a lot of drivers currently use clocks and a number of systems have badly setup default clock trees at start time. Mark Brown and others have argued that the management of clocks which is common to all devices should not live in the driver.
On 01/04/14 15:19, Ben Dooks wrote: > On 31/03/14 21:06, Greg KH wrote: >> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: [...] >> > I don't understand why you need the driver core to initialize this one >> > type of thing? That should be in a driver, or in a class, or at worse >> > case, the platform code. >> > >> > What makes clocks so "unique" here? The reason I put it in the driver core was mainly to avoid having many drivers doing same call to this initialization function. I was considering moving it to the bus code, still there are several buses for which it would need to be repeated. Maybe really_probe() is not a best place to put this, nonetheless the requirements I could list were: 1. not involving individual drivers, 2. have such an initialization call done for all devices, irrespective of Linux bus or class type, 3. Handle errors properly, e.g. defer driver probing if a clock for a device is not yet available. One advantage I could see from making the call from within a device driver is that a device could keep using the common DT bindings and replace the common initialization function with a private one, if there is a need for some quirks handled for a device. With approach as in this patch it's difficult to override the default behaviour. However then there is a question whether we strive for the clocks management to be possibly kept away from device drivers. > I suppose the issue here is that a lot of drivers currently use > clocks and a number of systems have badly setup default clock trees > at start time. > > Mark Brown and others have argued that the management of clocks which > is common to all devices should not live in the driver. True, motivation behind this patch series was also replacing custom code in multiple drivers doing similar clock rate or parent setting by a common code, using standardized DT binding. -- Thanks, Sylwester
Hi Laurent, On 01/04/14 15:15, Laurent Pinchart wrote: [...] >> +/** >> + * of_clk_device_init() - parse and set clk configuration assigned to a >> device >> + * @node: device node to apply the configuration for >> + * >> + * This function parses 'clock-parents' and 'clock-rates' properties and >> sets >> + * any specified clock parents and rates. >> + */ >> +int of_clk_device_init(struct device_node *node) >> +{ >> + struct property *prop; >> + const __be32 *cur; >> + int rc, index, num_parents; >> + struct clk *clk, *pclk; >> + u32 rate; >> + >> + num_parents = of_count_phandle_with_args(node, "clock-parents", >> + "#clock-cells"); >> + if (num_parents == -EINVAL) >> + pr_err("clk: Invalid value of clock-parents property at %s\n", >> + node->full_name); > > This is an implementation detail, but wouldn't it simplify the code if you > merged the two loops by iterating of the clocks property instead of over the > clock-parents and clock-rates properties separately ? The issue here is that all clock parents should be set before we start setting clock rates. Otherwise the clock frequencies could be incorrect if clk_set_rate() is followed by clk_set_parent(). >> + for (index = 0; index < num_parents; index++) { >> + pclk = of_clk_get_by_property(node, "clock-parents", index); >> + if (IS_ERR(pclk)) { >> + /* skip empty (null) phandles */ >> + if (PTR_ERR(pclk) == -ENOENT) >> + continue; >> + >> + pr_warn("clk: couldn't get parent clock %d for %s\n", >> + index, node->full_name); >> + return PTR_ERR(pclk); >> + } >> + >> + clk = of_clk_get(node, index); >> + if (IS_ERR(clk)) { >> + pr_warn("clk: couldn't get clock %d for %s\n", >> + index, node->full_name); >> + return PTR_ERR(clk); >> + } >> + >> + rc = clk_set_parent(clk, pclk); >> + if (rc < 0) >> + pr_err("clk: failed to reparent %s to %s: %d\n", >> + __clk_get_name(clk), __clk_get_name(pclk), rc); >> + else >> + pr_debug("clk: set %s as parent of %s\n", >> + __clk_get_name(pclk), __clk_get_name(clk)); >> + } >> + >> + index = 0; >> + of_property_for_each_u32(node, "clock-rates", prop, cur, rate) { >> + if (rate) { >> + clk = of_clk_get(node, index); >> + if (IS_ERR(clk)) { >> + pr_warn("clk: couldn't get clock %d for %s\n", >> + index, node->full_name); >> + return PTR_ERR(clk); >> + } >> + >> + rc = clk_set_rate(clk, rate); >> + if (rc < 0) >> + pr_err("clk: couldn't set %s clock rate: %d\n", >> + __clk_get_name(clk), rc); >> + else >> + pr_debug("clk: set rate of clock %s to %lu\n", >> + __clk_get_name(clk), clk_get_rate(clk)); >> + } >> + index++; >> + } >> + >> + return 0; >> +} >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index dff0373..ea6d8bd 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c > > [snip] > >> @@ -2620,7 +2621,15 @@ void __init of_clk_init(const struct of_device_id >> *matches) list_for_each_entry_safe(clk_provider, next, >> &clk_provider_list, node) { >> if (force || parent_ready(clk_provider->np)) { >> + >> clk_provider->clk_init_cb(clk_provider->np); >> + >> + /* Set any assigned clock parents and rates */ >> + np = of_get_child_by_name(clk_provider->np, >> + "assigned-clocks"); >> + if (np) >> + of_clk_device_init(np); > > Aren't you missing an of_node_put() here ? Indeed, it's missing. Will fix that in next version, thanks for pointing out. >> list_del(&clk_provider->node); >> kfree(clk_provider); >> is_init_done = true; -- Regards, Sylwester
On Tue, Apr 01, 2014 at 02:19:40PM +0100, Ben Dooks wrote: > On 31/03/14 21:06, Greg KH wrote: > >On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: > >>This function adds a helper function to configure clock parents and rates > >>as specified in clock-parents, clock-rates DT properties for a consumer > >>device and a call to it before driver is bound to a device. > >> > >>Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > [snip] > > > > >I don't understand why you need the driver core to initialize this one > >type of thing? That should be in a driver, or in a class, or at worse > >case, the platform code. > > > >What makes clocks so "unique" here? > > I suppose the issue here is that a lot of drivers currently use > clocks and a number of systems have badly setup default clock trees > at start time. Then they should be fixed, why should _all_ Linux devices care about such broken devices/systems? > Mark Brown and others have argued that the management of clocks which > is common to all devices should not live in the driver. Then put it in the bus that initializes the devices / drivers. thanks, greg k-h
On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote: > On 01/04/14 15:19, Ben Dooks wrote: > > On 31/03/14 21:06, Greg KH wrote: > >> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: > [...] > >> > I don't understand why you need the driver core to initialize this one > >> > type of thing? That should be in a driver, or in a class, or at worse > >> > case, the platform code. > >> > > >> > What makes clocks so "unique" here? > > The reason I put it in the driver core was mainly to avoid having many > drivers doing same call to this initialization function. > I was considering moving it to the bus code, still there are several > buses for which it would need to be repeated. "several" is how many? 2? 3? 10? Please fix it "correctly" and don't put it in the driver core just because it seems easier that way. > Maybe really_probe() is not a best place to put this, nonetheless > the requirements I could list were: > > 1. not involving individual drivers, Why not? > 2. have such an initialization call done for all devices, irrespective > of Linux bus or class type, Why? Do _all_ devices that Linux supports have this issue to be resolved? > 3. Handle errors properly, e.g. defer driver probing if a clock for > a device is not yet available. Then do it in the bus that controls that device, as it knows to defer probing at that point in time. > One advantage I could see from making the call from within a device > driver is that a device could keep using the common DT bindings and > replace the common initialization function with a private one, if > there is a need for some quirks handled for a device. With approach > as in this patch it's difficult to override the default behaviour. > However then there is a question whether we strive for the clocks > management to be possibly kept away from device drivers. > > > I suppose the issue here is that a lot of drivers currently use > > clocks and a number of systems have badly setup default clock trees > > at start time. > > > > Mark Brown and others have argued that the management of clocks which > > is common to all devices should not live in the driver. > > True, motivation behind this patch series was also replacing custom > code in multiple drivers doing similar clock rate or parent setting > by a common code, using standardized DT binding. Then put it in the bus that controls these broken devices / platforms. greg k-h
On Tue, Apr 01, 2014 at 09:37:44AM -0700, Greg KH wrote: > On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote: > > On 01/04/14 15:19, Ben Dooks wrote: > > > On 31/03/14 21:06, Greg KH wrote: > > >> > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: > > [...] > > >> > I don't understand why you need the driver core to initialize this one > > >> > type of thing? That should be in a driver, or in a class, or at worse > > >> > case, the platform code. > > >> > > > >> > What makes clocks so "unique" here? > > > > The reason I put it in the driver core was mainly to avoid having many > > drivers doing same call to this initialization function. > > I was considering moving it to the bus code, still there are several > > buses for which it would need to be repeated. > > "several" is how many? 2? 3? 10? > > Please fix it "correctly" and don't put it in the driver core just > because it seems easier that way. > > > Maybe really_probe() is not a best place to put this, nonetheless > > the requirements I could list were: > > > > 1. not involving individual drivers, > > Why not? > > > 2. have such an initialization call done for all devices, irrespective > > of Linux bus or class type, > > Why? Do _all_ devices that Linux supports have this issue to be > resolved? > > > 3. Handle errors properly, e.g. defer driver probing if a clock for > > a device is not yet available. > > Then do it in the bus that controls that device, as it knows to defer > probing at that point in time. The issue this patch tries to solve is not about single devices, but about the way these devices are connected with each other. Clocks for different (sometimes completely unrelated) devices influence each other. You can't control the clock from one device without influencing other devices. Knowledge about these constraints can't be encoded in the drivers, because the constraints differ per SoC, sometimes even per board. Many SoCs share the same devices, but the clock topology they are surrounded with is always different. With this I don't mean the direct clock inputs, these are well abstracted with the current clk_* API. What I mean is situations like: "On this board use the clock controller to output this particular clock on that pin, because it happens to be the master clock of some audio codec connected externally; also make the same clock input to the internal Audio system to make sure both are in sync". These situations can be completely different on the next board or on the next SoC which has the same devices, but a different clock routing. That said, I also think the driver core doesn't have to be bothered with the clock setup. Putting the clock setup into the devicenode providing the clocks (and thus parsing it from the clock controller driver) should be sufficient. Sascha
On Tue, Apr 01, 2014 at 03:19:40PM +0200, Ben Dooks wrote: > On 31/03/14 21:06, Greg KH wrote: > > On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: > >> This function adds a helper function to configure clock parents and rates > >> as specified in clock-parents, clock-rates DT properties for a consumer > >> device and a call to it before driver is bound to a device. > >> > >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > [snip] > > > > > I don't understand why you need the driver core to initialize this one > > type of thing? That should be in a driver, or in a class, or at worse > > case, the platform code. > > > > What makes clocks so "unique" here? > > I suppose the issue here is that a lot of drivers currently use > clocks and a number of systems have badly setup default clock trees > at start time. > > Mark Brown and others have argued that the management of clocks which > is common to all devices should not live in the driver. Exactly, this data should be part of the clock provider DT nodes, not of the device nodes. Cheers, Peter.
On 01/04/14 18:37, Greg KH wrote: > On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote: >> On 01/04/14 15:19, Ben Dooks wrote: >>> On 31/03/14 21:06, Greg KH wrote: >>>>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: >> [...] >>>>> I don't understand why you need the driver core to initialize this one >>>>> type of thing? That should be in a driver, or in a class, or at worse >>>>> case, the platform code. >>>>> >>>>> What makes clocks so "unique" here? >> >> The reason I put it in the driver core was mainly to avoid having many >> drivers doing same call to this initialization function. >> I was considering moving it to the bus code, still there are several >> buses for which it would need to be repeated. > > "several" is how many? 2? 3? 10? It would be mostly the platform and amba bus, but this clock defaults could be useful also for i2c, spi. In general it's about not probable devices which may be specified in devicetree. > Please fix it "correctly" and don't put it in the driver core just > because it seems easier that way. > >> Maybe really_probe() is not a best place to put this, nonetheless >> the requirements I could list were: >> >> 1. not involving individual drivers, > > Why not? I'd say because this thing can be often considered a platform detail which would be better opaque to drivers. It's at the border of a device and its integration within a platform. >> 2. have such an initialization call done for all devices, irrespective >> of Linux bus or class type, > > Why? Do _all_ devices that Linux supports have this issue to be > resolved? Sorry, certainly not all devices. I think it's only related to the non-probable devices which can be specified in DT. >> 3. Handle errors properly, e.g. defer driver probing if a clock for >> a device is not yet available. > > Then do it in the bus that controls that device, as it knows to defer > probing at that point in time. All right, that would also work. I suspect doing it for platform and amba bus might be sufficient in the beginning. >> One advantage I could see from making the call from within a device >> driver is that a device could keep using the common DT bindings and >> replace the common initialization function with a private one, if >> there is a need for some quirks handled for a device. With approach >> as in this patch it's difficult to override the default behaviour. >> However then there is a question whether we strive for the clocks >> management to be possibly kept away from device drivers. >> >>> I suppose the issue here is that a lot of drivers currently use >>> clocks and a number of systems have badly setup default clock trees >>> at start time. >>> >>> Mark Brown and others have argued that the management of clocks which >>> is common to all devices should not live in the driver. >> >> True, motivation behind this patch series was also replacing custom >> code in multiple drivers doing similar clock rate or parent setting >> by a common code, using standardized DT binding. > > Then put it in the bus that controls these broken devices / platforms. OK, I'm going to put it in the bus code.
On 02/04/14 07:37, Sascha Hauer wrote: > On Tue, Apr 01, 2014 at 09:37:44AM -0700, Greg KH wrote: >> On Tue, Apr 01, 2014 at 04:23:12PM +0200, Sylwester Nawrocki wrote: >>> On 01/04/14 15:19, Ben Dooks wrote: >>>> On 31/03/14 21:06, Greg KH wrote: >>>>>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: >>> [...] >>>>>> I don't understand why you need the driver core to initialize this one >>>>>> type of thing? That should be in a driver, or in a class, or at worse >>>>>> case, the platform code. >>>>>> >>>>>> What makes clocks so "unique" here? >>> >>> The reason I put it in the driver core was mainly to avoid having many >>> drivers doing same call to this initialization function. >>> I was considering moving it to the bus code, still there are several >>> buses for which it would need to be repeated. >> >> "several" is how many? 2? 3? 10? >> >> Please fix it "correctly" and don't put it in the driver core just >> because it seems easier that way. >> >>> Maybe really_probe() is not a best place to put this, nonetheless >>> the requirements I could list were: >>> >>> 1. not involving individual drivers, >> >> Why not? >> >>> 2. have such an initialization call done for all devices, irrespective >>> of Linux bus or class type, >> >> Why? Do _all_ devices that Linux supports have this issue to be >> resolved? >> >>> 3. Handle errors properly, e.g. defer driver probing if a clock for >>> a device is not yet available. >> >> Then do it in the bus that controls that device, as it knows to defer >> probing at that point in time. > > The issue this patch tries to solve is not about single devices, but > about the way these devices are connected with each other. > > Clocks for different (sometimes completely unrelated) devices influence > each other. You can't control the clock from one device without > influencing other devices. Knowledge about these constraints can't be > encoded in the drivers, because the constraints differ per SoC, > sometimes even per board. Many SoCs share the same devices, but the > clock topology they are surrounded with is always different. With this I > don't mean the direct clock inputs, these are well abstracted with the > current clk_* API. What I mean is situations like: "On this board use > the clock controller to output this particular clock on that pin, > because it happens to be the master clock of some audio codec connected > externally; also make the same clock input to the internal Audio system > to make sure both are in sync". These situations can be completely > different on the next board or on the next SoC which has the same > devices, but a different clock routing. > > That said, I also think the driver core doesn't have to be bothered with > the clock setup. Putting the clock setup into the devicenode providing > the clocks (and thus parsing it from the clock controller driver) should > be sufficient. It would work but I don't really like such a DT binding. Rather than only having a long list of clocks in one node I would prefer to also have a possibility to list clock data specific to a device in its corresponding DT node. Similarly as it's done, e.g. with interrupts.
On 02/04/14 10:01, Peter De Schrijver wrote: > On Tue, Apr 01, 2014 at 03:19:40PM +0200, Ben Dooks wrote: >> On 31/03/14 21:06, Greg KH wrote: >>> On Mon, Mar 31, 2014 at 06:41:56PM +0200, Sylwester Nawrocki wrote: >>>> This function adds a helper function to configure clock parents and rates >>>> as specified in clock-parents, clock-rates DT properties for a consumer >>>> device and a call to it before driver is bound to a device. >>>> >>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >> >> [snip] >> >>> I don't understand why you need the driver core to initialize this one >>> type of thing? That should be in a driver, or in a class, or at worse >>> case, the platform code. >>> >>> What makes clocks so "unique" here? >> >> I suppose the issue here is that a lot of drivers currently use >> clocks and a number of systems have badly setup default clock trees >> at start time. >> >> Mark Brown and others have argued that the management of clocks which >> is common to all devices should not live in the driver. > > Exactly, this data should be part of the clock provider DT nodes, not > of the device nodes. Why not in the device nodes, when often this data is closely related to a device ? I'd prefer to keep the ability to also specify it at the consumer nodes. IMHO the DT binding would be more explicit and less error prone this way. E.g. clock-rates may refer directly to the consumer clocks, so why we should be specifying it for all devices in a single DT node ? Also I'm not sure if a fact that something shouldn't be handled in a device driver necessarily means that related data shouldn't be put in the device node. It would be nice to hear more opinions, it seems now there is roughly same number of proponents and opponents of each option. -- Thanks, Sylwester
diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt index 700e7aa..59fbb4e 100644 --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt @@ -132,3 +132,45 @@ clock signal, and a UART. ("pll" and "pll-switched"). * The UART has its baud clock connected the external oscillator and its register clock connected to the PLL clock (the "pll-switched" signal) + +==Assigned clock parents and rates== + +Some platforms require static initial configuration of parts of the clocks +controller. Such a configuration can be specified in a clock consumer node +through clock-parents and clock-rates DT properties. The former should +contain a list of parent clocks in form of phandle and clock specifier pairs, +the latter the list of assigned clock frequency values (one cell each). + + uart@a000 { + compatible = "fsl,imx-uart"; + reg = <0xa000 0x1000>; + ... + clocks = <&clkcon 0>, <&clkcon 3>; + clock-names = "baud", "mux"; + + clock-parents = <0>, <&pll 1>; + clock-rates = <460800>; + }; + +In this example the pll is set as parent of "mux" clock and frequency of "baud" +clock is specified as 460800 Hz. + +Configuring a clock parent and rate through the device node that uses +the clock can be done only for clocks that have a single user. Specifying +conflicting parent or rate configuration in multiple consumer nodes for +a shared clock is forbidden. + +Configuration of common clocks, which affect multiple consumer devices +can be specified in a dedicated 'assigned-clocks' subnode of a clock +provider node, e.g.: + + clkcon { + ... + #clock-cells = <1>; + + assigned-clocks { + clocks = <&clkcon 16>, <&clkcon 17>; + clock-parents = <0>, <&clkcon 1>; + clock-rates = <200000>; + }; + }; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 0605176..4c633e7 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -25,6 +25,7 @@ #include <linux/async.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> +#include <linux/clk/clk-conf.h> #include "base.h" #include "power/power.h" @@ -278,6 +279,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (ret) goto probe_failed; + if (dev->of_node) { + ret = of_clk_device_init(dev->of_node); + if (ret) + goto probe_failed; + } + if (driver_sysfs_add(dev)) { printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n", __func__, dev_name(dev)); diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 85a33e6..5ec53cb 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -8,6 +8,9 @@ obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o obj-$(CONFIG_COMMON_CLK) += clk-gate.o obj-$(CONFIG_COMMON_CLK) += clk-mux.o obj-$(CONFIG_COMMON_CLK) += clk-composite.o +ifeq ($(CONFIG_OF), y) +obj-$(CONFIG_COMMON_CLK) += clk-conf.o +endif # hardware specific clock types # please keep this section sorted lexicographically by file/directory path name diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c new file mode 100644 index 0000000..a2e992e --- /dev/null +++ b/drivers/clk/clk-conf.c @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * Sylwester Nawrocki <s.nawrocki@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/device.h> +#include <linux/of.h> +#include <linux/printk.h> + +/** + * of_clk_device_init() - parse and set clk configuration assigned to a device + * @node: device node to apply the configuration for + * + * This function parses 'clock-parents' and 'clock-rates' properties and sets + * any specified clock parents and rates. + */ +int of_clk_device_init(struct device_node *node) +{ + struct property *prop; + const __be32 *cur; + int rc, index, num_parents; + struct clk *clk, *pclk; + u32 rate; + + num_parents = of_count_phandle_with_args(node, "clock-parents", + "#clock-cells"); + if (num_parents == -EINVAL) + pr_err("clk: Invalid value of clock-parents property at %s\n", + node->full_name); + + for (index = 0; index < num_parents; index++) { + pclk = of_clk_get_by_property(node, "clock-parents", index); + if (IS_ERR(pclk)) { + /* skip empty (null) phandles */ + if (PTR_ERR(pclk) == -ENOENT) + continue; + + pr_warn("clk: couldn't get parent clock %d for %s\n", + index, node->full_name); + return PTR_ERR(pclk); + } + + clk = of_clk_get(node, index); + if (IS_ERR(clk)) { + pr_warn("clk: couldn't get clock %d for %s\n", + index, node->full_name); + return PTR_ERR(clk); + } + + rc = clk_set_parent(clk, pclk); + if (rc < 0) + pr_err("clk: failed to reparent %s to %s: %d\n", + __clk_get_name(clk), __clk_get_name(pclk), rc); + else + pr_debug("clk: set %s as parent of %s\n", + __clk_get_name(pclk), __clk_get_name(clk)); + } + + index = 0; + of_property_for_each_u32(node, "clock-rates", prop, cur, rate) { + if (rate) { + clk = of_clk_get(node, index); + if (IS_ERR(clk)) { + pr_warn("clk: couldn't get clock %d for %s\n", + index, node->full_name); + return PTR_ERR(clk); + } + + rc = clk_set_rate(clk, rate); + if (rc < 0) + pr_err("clk: couldn't set %s clock rate: %d\n", + __clk_get_name(clk), rc); + else + pr_debug("clk: set rate of clock %s to %lu\n", + __clk_get_name(clk), clk_get_rate(clk)); + } + index++; + } + + return 0; +} diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index dff0373..ea6d8bd 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -10,6 +10,7 @@ */ #include <linux/clk-private.h> +#include <linux/clk/clk-conf.h> #include <linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h> @@ -2620,7 +2621,15 @@ void __init of_clk_init(const struct of_device_id *matches) list_for_each_entry_safe(clk_provider, next, &clk_provider_list, node) { if (force || parent_ready(clk_provider->np)) { + clk_provider->clk_init_cb(clk_provider->np); + + /* Set any assigned clock parents and rates */ + np = of_get_child_by_name(clk_provider->np, + "assigned-clocks"); + if (np) + of_clk_device_init(np); + list_del(&clk_provider->node); kfree(clk_provider); is_init_done = true; @@ -2635,7 +2644,6 @@ void __init of_clk_init(const struct of_device_id *matches) */ if (!is_init_done) force = true; - } } #endif diff --git a/include/linux/clk/clk-conf.h b/include/linux/clk/clk-conf.h new file mode 100644 index 0000000..51b9c01 --- /dev/null +++ b/include/linux/clk/clk-conf.h @@ -0,0 +1,19 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * Sylwester Nawrocki <s.nawrocki@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +struct device_node; + +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) +int of_clk_device_init(struct device_node *node); +#else +static inline int of_clk_device_init(struct device_node *node) +{ + return 0; +} +#endif
This function adds a helper function to configure clock parents and rates as specified in clock-parents, clock-rates DT properties for a consumer device and a call to it before driver is bound to a device. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- Changes since v3: - added detailed description of the assigned-clocks subnode, - clk-conf.c is now excluded when CONFIG_OF is not set, - s/of_clk_device_setup/of_clk_device_init, - added missing 'static inline' to the function stub definition. Changes since v2: - edited in clock-bindings.txt, added note about 'assigned-clocks' subnode which may be used to specify "global" clocks configuration at a clock provider node, - moved of_clk_device_setup() function declaration from clk-provider.h to clk-conf.h so required function stubs are available when CONFIG_COMMON_CLK is not enabled, Changes since v1: - the helper function to parse and set assigned clock parents and rates made public so it is available to clock providers to call directly; - dropped the platform bus notification and call of_clk_device_setup() is is now called from the driver core, rather than from the notification callback; - s/of_clk_get_list_entry/of_clk_get_by_property. --- .../devicetree/bindings/clock/clock-bindings.txt | 42 ++++++++++ drivers/base/dd.c | 7 ++ drivers/clk/Makefile | 3 + drivers/clk/clk-conf.c | 87 ++++++++++++++++++++ drivers/clk/clk.c | 10 ++- include/linux/clk/clk-conf.h | 19 +++++ 6 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 drivers/clk/clk-conf.c create mode 100644 include/linux/clk/clk-conf.h -- 1.7.9.5