Message-ID: <CAL_JsqKDJ_yFxFYnrTtmzMPoxO2rmypiujybwbhUh35y1LS-Mw@mail.gmail.com>
Date:	Thu, 10 Apr 2014 11:04:07 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Sylwester Nawrocki <s.nawrocki@...sung.com>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Mike Turquette <mturquette@...aro.org>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Rob Herring <robh+dt@...nel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Mark Rutland <mark.rutland@....com>,
	Kumar Gala <galak@...eaurora.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Tero Kristo <t-kristo@...com>, sw0312.kim@...sung.com,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Tomasz Figa <t.figa@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v5 2/2] clk: Add handling of clk parent and rate
 assigned from DT

On Wed, Apr 9, 2014 at 6:26 AM, Sylwester Nawrocki
<s.nawrocki@...sung.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@...sung.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@...0 {
> +        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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists