[PATCH v6 1/4] clk: hi3xxx: add clock support
Mark Rutland
mark.rutland at arm.com
Fri Aug 9 11:14:10 EDT 2013
On Fri, Jul 26, 2013 at 05:32:12AM +0100, Haojian Zhuang wrote:
> Add clock support with device tree on Hisilicon SoC.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang at linaro.org>
> Cc: Mike Turquette <mturquette at linaro.org>
> ---
> .../devicetree/bindings/clock/hisilicon.txt | 66 ++++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-hi3xxx.c | 398 +++++++++++++++++++++
> 3 files changed, 465 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/hisilicon.txt
> create mode 100644 drivers/clk/clk-hi3xxx.c
>
> diff --git a/Documentation/devicetree/bindings/clock/hisilicon.txt b/Documentation/devicetree/bindings/clock/hisilicon.txt
> new file mode 100644
> index 0000000..5d7a220
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/hisilicon.txt
> @@ -0,0 +1,66 @@
> +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,hi3620-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.
> + - clkmux-reg : array of mux register offset & mask bits
Offset from what base address? There's no reg property listed, the
binding document doesn't refer to a container, and the example doesn't
show one.
Do these offsets within the parent's address space vary, or is a known
set of clocks a known offsets always going to exist?
If all the clocks are a component of a parent block of IP, is that
parent block not better described as a provider of multiple clocks?
> + - clkmux-table : array of mux select bits
> +
> +Required properties for Hi3620 gate clocks:
> + - compatible : Shall be "hisilicon,hi3620-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.
> + - hi3620-clkgate : array of enable register offset & enable bits
> + - hi3620-clkreset : array of reset register offset & enable bits
Similarly, offset from where?
> +
> +Required properties for clock divider:
> + - compatible : Shall be "hisilicon,hi3620-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.
> + - #clock-cells : from common clock binding; shall be set to 0.
> + - #clkdiv-table-cells : the number of parameters after phandle in
> + clkdiv-table property.
> + - clkdiv-table : list of value that are used to configure clock
> + divider. They're value of phandle, index & divider value.
I'm having some difficulty understanding what the properties above
actually represent.
> + - clkdiv : array of divider register offset & mask bits.
Offset from...?
> +
> +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.
> + - clkgate-inverted : bool value. True means that set-to-disable.
Is that a generic property, or only valid for hisilicon,clk-gate?
> +
> +For example:
> + timclk1: clkgate at 38 {
> + compatible = "hisilicon,clk-gate";
> + #clock-cells = <0>;
> + clocks = <&refclk_timer1>;
> + clock-output-names = "timclk1";
> + clkgate-inverted;
> + clkgate = <0 18>;
> + };
> +
> + dtable: clkdiv at 0 {
> + #clkdiv-table-cells = <2>;
> + };
> +
> + div_cfgaxi: clkdiv at 2 {
> + compatible = "hisilicon,hi3620-clk-div";
> + #clock-cells = <0>;
> + clocks = <&div_shareaxi>;
> + clock-output-names = "cfgAXI_div";
> + clkdiv-table = <&dtable 0x01 2>;
> + clkdiv = <0x100 0x60>;
> + };
[...]
> +static const struct of_device_id hi3xxx_of_match[] = {
> + { .compatible = "hisilicon,sctrl" },
> +};
This parent wasn't mentioned in the binding. Is it documented elsewhere?
> +
> +static void __iomem __init *hi3xxx_init_clocks(struct device_node *np)
> +{
> + struct device_node *parent;
> + const struct of_device_id *match;
> + void __iomem *ret = NULL;
> +
> + parent = of_get_parent(np);
> + if (!parent) {
> + pr_warn("Can't find parent node of these clocks\n");
> + goto out;
>From out you only seem to return. Why not just return?
Does this leave the of_node refcounts balanced? (as a more general
question, does most code?).
> + }
> + match = of_match_node(hi3xxx_of_match, parent);
> + if (!match) {
> + pr_warn("Can't find the right parent\n");
> + goto out;
Why not just return?
> + }
> +
> + if (!hi3xxx_clk_base) {
> + ret = of_iomap(parent, 0);
> + WARN_ON(!ret);
> + hi3xxx_clk_base = ret;
> + } else {
> + ret = hi3xxx_clk_base;
Necessarily, ret = NULL in this case.
Why not rearrange the whole block:
if (!hi3xx_clk_base)
return NULL;
/*
* do stuff assuming hi3xx_clk_base here
*/
Even better, the mapping of the registers should be centralised and done
at the start - no need to fail repeatedly for each child. You already
seemed to have a compatible string for the parent node...
[...]
> +static void __init hi3620_clkgate_setup(struct device_node *np)
> +{
> + struct hi3620_periclk *pclk;
> + struct clk_init_data *init;
> + struct clk *clk;
> + const char *clk_name, *name, **parent_names;
> + u32 rdata[2], gdata[2];
> + void __iomem *base;
> +
> + base = hi3xxx_init_clocks(np);
> + if (!base)
> + return;
> +
> + if (of_property_read_string(np, "clock-output-names", &clk_name))
> + return;
> + if (of_property_read_u32_array(np, "hi3620-clkgate",
> + &gdata[0], 2))
> + return;
> +
> + /* gate only has the fixed parent */
> + parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> + if (!parent_names)
> + return;
> + parent_names[0] = of_clk_get_parent_name(np, 0);
It seems odd to allocate a single pointer, surely there has to be a
better way? Could this single pointer not be stored in a wrapper of
clk_init_data?
Is it possible for of_clk_get_parent_name to fail?
> +
> + pclk = kzalloc(sizeof(*pclk), GFP_KERNEL);
> + if (!pclk)
> + goto err_pclk;
> +
> + init = kzalloc(sizeof(*init), GFP_KERNEL);
> + if (!init)
> + goto err_init;
> + init->name = kstrdup(clk_name, GFP_KERNEL);
You're happy to refer to the parent name embedded in the dt, but kstrdup
the child name?
> + init->ops = &hi3620_clkgate_ops;
> + init->flags = CLK_SET_RATE_PARENT;
> + init->parent_names = parent_names;
> + init->num_parents = 1;
> +
> + if (!of_property_read_u32_array(np, "hi3620-clkreset",
> + &rdata[0], 2)) {
> + pclk->reset = base + rdata[0];
> + pclk->rbits = rdata[1];
> + }
> + pclk->enable = base + gdata[0];
> + pclk->ebits = gdata[1];
> + pclk->lock = &hi3xxx_clk_lock;
> + pclk->hw.init = init;
> +
> + clk = clk_register(NULL, &pclk->hw);
> + if (IS_ERR(clk))
> + goto err_clk;
> + if (!of_property_read_string(np, "clock-names", &name))
> + clk_register_clkdev(clk, name, NULL);
> + of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + return;
> +err_clk:
> + kfree(init);
> +err_init:
> + kfree(pclk);
> +err_pclk:
> + kfree(parent_names);
> +}
> +CLK_OF_DECLARE(hi3620_gate, "hisilicon,hi3620-clk-gate", hi3620_clkgate_setup)
> +
> +static int __init hi3xxx_parse_mux(struct device_node *np,
> + u8 *num_parents,
> + u32 *table)
> +{
> + int i, cnt, ret;
> +
> + /* get the count of items in mux */
> + for (i = 0, cnt = 0; ; i++, cnt++) {
> + /* parent's #clock-cells property is always 0 */
> + if (!of_parse_phandle(np, "clocks", i))
> + break;
> + }
Remove the references to cnt in this loop. Afterwards add:
cnt = i;
> +
> + for (i = 0; i < cnt; i++) {
> + if (!of_clk_get_parent_name(np, i))
> + return -ENOENT;
> + }
> + *num_parents = cnt;
> + table = kzalloc(sizeof(u32 *) * cnt, GFP_KERNEL);
> + if (!table)
> + return -ENOMEM;
> + ret = of_property_read_u32_array(np, "clkmux-table",
> + table, cnt);
> + if (ret)
> + goto err;
> + return 0;
> +err:
> + kfree(table);
> + return ret;
The err case is only used at the end of the function. Why not:
if (!ret)
return 0;
kfree(table);
return ret;
[...]
> +static void __init hs_clkgate_setup(struct device_node *np)
> +{
> + struct clk *clk;
> + const char *clk_name, **parent_names, *name;
> + unsigned long flags = 0;
> + u32 data[2];
> + void __iomem *base;
> +
> + base = hi3xxx_init_clocks(np);
> + if (!base)
> + return;
> + if (of_property_read_string(np, "clock-output-names", &clk_name))
> + return;
> + if (of_property_read_u32_array(np, "clkgate",
> + &data[0], 2))
> + return;
> + if (of_property_read_bool(np, "clkgate-inverted"))
> + flags = CLK_GATE_SET_TO_DISABLE;
> + /* gate only has the fixed parent */
> + parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> + if (!parent_names)
> + return;
> + parent_names[0] = of_clk_get_parent_name(np, 0);
> +
> + clk = clk_register_gate(NULL, clk_name, parent_names[0], 0,
> + base + data[0], (u8)data[1], flags,
> + &hi3xxx_clk_lock);
> + if (IS_ERR(clk))
> + goto err;
> + if (!of_property_read_string(np, "clock-names", &name))
> + clk_register_clkdev(clk, name, NULL);
> + of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + return;
> +err:
> + kfree(parent_names);
Why shift the err case out of line if it's only referred to once?
> +}
> +CLK_OF_DECLARE(hs_gate, "hisilicon,clk-gate", hs_clkgate_setup)
> +
> +void __init hi3620_clkdiv_setup(struct device_node *np)
> +{
> + struct clk *clk;
> + const char *clk_name, **parent_names;
> + struct clk_div_table *table;
> + unsigned int table_num;
> + int i;
> + u32 data[2];
> + u8 shift, width;
> + const char *propname = "clkdiv-table";
> + const char *cellname = "#clkdiv-table-cells";
> + struct of_phandle_args div_table;
> + void __iomem *reg, *base;
> +
> + base = hi3xxx_init_clocks(np);
> + if (!base)
> + return;
> +
> + if (of_property_read_string(np, "clock-output-names", &clk_name))
> + return;
> + if (of_property_read_u32_array(np, "clkdiv", &data[0], 2))
> + return;
> +
> + /*process the div_table*/
> + for (i = 0; ; i++) {
> + if (of_parse_phandle_with_args(np, propname, cellname,
> + i, &div_table))
> + break;
> + }
> +
> + /*table ends with <0, 0>, so plus one to table_num*/
> + table_num = i + 1;
That certainly wasn't described in the binding (and the example doesn't
match).
> +
> + table = kzalloc(sizeof(struct clk_div_table) * table_num, GFP_KERNEL);
> + if (!table)
> + return ;
Unnecessary space.
> +
> + for (i = 0; ; i++) {
> + if (of_parse_phandle_with_args(np, propname, cellname,
> + i, &div_table))
> + break;
> +
> + table[i].val = div_table.args[0];
> + table[i].div = div_table.args[1];
> + }
> +
> + /* gate only has the fixed parent */
> + parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
> + if (!parent_names)
> + goto err_par;
> + parent_names[0] = of_clk_get_parent_name(np, 0);
Similarly to my earlier comment, I suspect there's a better way of
handling this.
Thanks,
Mark.
More information about the linux-arm-kernel
mailing list