Re: [PATCH v10 1/7] clk: hi3xxx: add clock support

[Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Quoting Haojian Zhuang (2013-10-15 02:16:11)
> Add clock support with device tree on Hisilicon SoC.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Hello Haojian,

This patch looks mostly good to me but I have a few comments on the
binding below.

> ---
>  .../devicetree/bindings/clock/hisilicon.txt        |  99 +++++++++
>  drivers/clk/Makefile                               |   1 +
>  drivers/clk/hisilicon/Makefile                     |   2 +
>  drivers/clk/hisilicon/clk-hi3xxx.c                 | 221 +++++++++++++++++++++
>  drivers/clk/hisilicon/clk-hi3xxx.h                 |  34 ++++
>  drivers/clk/hisilicon/clkgate-seperated.c          | 129 ++++++++++++
>  6 files changed, 486 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt
>  create mode 100644 drivers/clk/hisilicon/Makefile
>  create mode 100644 drivers/clk/hisilicon/clk-hi3xxx.c
>  create mode 100644 drivers/clk/hisilicon/clk-hi3xxx.h
>  create mode 100644 drivers/clk/hisilicon/clkgate-seperated.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon.txt b/Documentation/devicetree/bindings/clock/hisilicon.txt
> new file mode 100644
> index 0000000..c29b9ef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/hisilicon.txt
> @@ -0,0 +1,99 @@
> +Device Tree Clock bindings for arch-hi3xxx
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties for mux clocks:
> + - compatible : shall be "hisilicon,clk-mux".
> + - clocks : shall be the input parent clock phandle for the clock. This should
> +       be the reference clock.
> + - clock-output-names : shall be reference name.
> + - #clock-cells : from common clock binding; shall be set to 0.
> + - reg : the mux register address. It should be the offset of the container.
> + - clkmux-mask : mask bits of the mux register.
> + - clkmux-table : array of mux select bits.
> +
> +Optional properties for mux clocks:
> + - clkmux-hiword-mask : indicates that the bit[31:16] are the hiword mask
> +       of mux selected bits (bit[15:0]). The bit[15:0] is valid only when
> +       bit[31:16] is set.

The masks are enumerated in clkmux-table. Can these masks include the
hiword-mask bits as well? This will mean that you can eliminate the
clkmux-hiword-mask property entirely. As an example:


	reg = <0xfc802000 0x4>;
	clkmux-table = <0x80000000 0x80008000>;

> +
> +
> +
> +Required properties for gate clocks:
> + - compatible : shall be "hisilicon,clk-gate".
> + - clocks : shall be the input parent clock phandle for the clock. This should
> +       be the reference clock.
> + - clock-output-names : shall be reference name.
> + - #clock-cells : from common clock binding; shall be set to 0.
> + - reg : the mux register address. It should be the offset of the container.
> + - clkgate : bit index to control the clock gate in the gate register.
> +
> +Optional properties for gate clocks:
> + - clkgate-inverted : it indicates that setting 0 could enable the clock gate
> +       and setting 1 could disable the clock gate.
> + - clkgate-seperated-reg : it indicates that there're three continuous
> +       registers (enable, disable & status) for the same clock gate.
> +
> +
> +
> +Required properties for divider clocks:
> + - compatible : shall be "hisilicon,clk-div".
> + - clocks : shall be the input parent clock phandle for the clock. This should
> +       be the reference clock.
> + - clock-output-names : shall be reference name.
> + - reg : the divider register address. It should be the offset of the
> +       container.
> + - clkdiv-mask : mask bits of the divider register.
> + - clkdiv-min : the minimum divider of the clock divider.
> + - clkdiv-max : the maximum divider of the clock divider.
> +
> +Optional properties for divider clocks:
> + - clkdiv-hiword-mask : indicates that the bit[31:16] are the hiword mask
> +       of divider selected bits (bit[15:0]). The bit[15:0] is valid only when
> +       bit[31:16] is set.
> +
> +
> +
> +For example:
> +               timer0_mux: timer0_mux@fc802000 {
> +                       compatible = "hisilicon,clk-mux";
> +                       #clock-cells = <0>;
> +                       clocks = <&osc32k &timerclk01>;
> +                       clock-output-names = "timer0_mux";
> +                       reg = <0xfc802000 0x4>;
> +                       clkmux-mask = <0x8000>;
> +                       clkmux-table = <0 0x8000>;

Can you eliminate the clkmux-mask property as well? Since you enumerate
the clkmux-table property you should be able to derive the mask from the
entries in that table.

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