| Message ID | 1397042790-10636-3-git-send-email-s.nawrocki@samsung.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki <s.nawrocki@samsung.com> 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 <s.nawrocki@samsung.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/04/14 18:04, Rob Herring wrote: > On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki > <s.nawrocki@samsung.com> 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 <s.nawrocki@samsung.com> >> --- [...] >> --- >> .../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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <s.nawrocki@samsung.com> > >> --- > > [...] > > >> --- > >> > >> .../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>; > >> + }; > >> + };
On 05/23/2014 04:34 AM, 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 > 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.) -Tero > > Regards, > Mike > > [1] http://www.spinics.net/lists/cpufreq/msg10071.html > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <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_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 <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_dev_init(struct device_node *node); +#else +static inline int of_clk_dev_init(struct device_node *node) +{ + return 0; +} +#endif
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 <s.nawrocki@samsung.com> --- 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