| Message ID | 1364419243-18446-2-git-send-email-emilio@elopez.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Emilio López (2013-03-27 14:20:37) > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > index d528a24..244de90 100644 > --- a/drivers/clk/sunxi/clk-sunxi.c > +++ b/drivers/clk/sunxi/clk-sunxi.c > @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, > } > > > + A lot of white space between these functions. > +/** > + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks > + */ > + > +#define SUNXI_GATES_MAX_SIZE 64 > + > +struct gates_data { > + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); > +}; > + > +static const __initconst struct gates_data axi_gates_data = { > + .mask = {1}, > +}; > + > +static const __initconst struct gates_data ahb_gates_data = { > + .mask = {0x7F77FFF, 0x14FB3F}, > +}; > + > +static const __initconst struct gates_data apb0_gates_data = { > + .mask = {0x4EF}, > +}; > + > +static const __initconst struct gates_data apb1_gates_data = { > + .mask = {0xFF00F7}, > +}; > + > +static void __init sunxi_gates_clk_setup(struct device_node *node, > + struct gates_data *data) > +{ > + struct clk_onecell_data *clk_data; > + const char *clk_parent; > + const char *clk_name; > + void *reg; > + int qty; > + int i = 0; > + int j = 0; > + int ignore; > + > + reg = of_iomap(node, 0); > + > + clk_parent = of_clk_get_parent_name(node, 0); > + > + /* Worst-case size approximation and memory allocation */ > + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); > + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); > + if (!clk_data) > + return; > + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); > + if (!clk_data->clks) { > + kfree(clk_data); > + return; > + } > + > + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { > + of_property_read_string_index(node, "clock-output-names", > + j, &clk_name); > + > + /* No driver claims this clock, but it should remain gated */ Should the comment read, "ungated" instead of "gated"? > + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; > + > + clk_data->clks[i] = clk_register_gate(NULL, clk_name, > + clk_parent, ignore, > + reg + 4 * (i/32), i % 32, > + 0, &clk_lock); > + WARN_ON(IS_ERR(clk_data->clks[i])); > + > + j++; > + } > + > + /* Adjust to the real max */ > + clk_data->clk_num = i; > + > + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); > +} > + > /* Matches for of_clk_init */ > static const __initconst struct of_device_id clk_match[] = { > {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, > @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { > {} > }; > > +/* Matches for gate clocks */ > +static const __initconst struct of_device_id clk_gates_match[] = { > + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, > + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, > + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, > + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, > + {} > +}; > + > static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, > void *function) > { > @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) > > /* Register mux clocks */ > of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); > + > + /* Register gate clocks */ > + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); I'm still a device tree noob, so this may be a dumb question. Can the above be converted to of_clk_init? Regards, Mike > } > -- > 1.8.2
Hi Mike, El 03/04/13 18:48, Mike Turquette escribió: > Quoting Emilio López (2013-03-27 14:20:37) >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index d528a24..244de90 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, >> } >> >> >> + > > A lot of white space between these functions. All of the function blocks are separated with three spaces on the file; I thought it looked more readable that way, but I don't have any strong opinion on separation. Is there any standard for this used on the kernel? In any case, and to keep consistency, can we handle this on a follow-up patch? >> +/** >> + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks >> + */ >> + >> +#define SUNXI_GATES_MAX_SIZE 64 >> + >> +struct gates_data { >> + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); >> +}; >> + >> +static const __initconst struct gates_data axi_gates_data = { >> + .mask = {1}, >> +}; >> + >> +static const __initconst struct gates_data ahb_gates_data = { >> + .mask = {0x7F77FFF, 0x14FB3F}, >> +}; >> + >> +static const __initconst struct gates_data apb0_gates_data = { >> + .mask = {0x4EF}, >> +}; >> + >> +static const __initconst struct gates_data apb1_gates_data = { >> + .mask = {0xFF00F7}, >> +}; >> + >> +static void __init sunxi_gates_clk_setup(struct device_node *node, >> + struct gates_data *data) >> +{ >> + struct clk_onecell_data *clk_data; >> + const char *clk_parent; >> + const char *clk_name; >> + void *reg; >> + int qty; >> + int i = 0; >> + int j = 0; >> + int ignore; >> + >> + reg = of_iomap(node, 0); >> + >> + clk_parent = of_clk_get_parent_name(node, 0); >> + >> + /* Worst-case size approximation and memory allocation */ >> + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); >> + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); >> + if (!clk_data) >> + return; >> + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); >> + if (!clk_data->clks) { >> + kfree(clk_data); >> + return; >> + } >> + >> + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { >> + of_property_read_string_index(node, "clock-output-names", >> + j, &clk_name); >> + >> + /* No driver claims this clock, but it should remain gated */ > > Should the comment read, "ungated" instead of "gated"? Indeed, good catch. Do you want me to resend the series, or can you amend this when picking the patches? >> + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; >> + >> + clk_data->clks[i] = clk_register_gate(NULL, clk_name, >> + clk_parent, ignore, >> + reg + 4 * (i/32), i % 32, >> + 0, &clk_lock); >> + WARN_ON(IS_ERR(clk_data->clks[i])); >> + >> + j++; >> + } >> + >> + /* Adjust to the real max */ >> + clk_data->clk_num = i; >> + >> + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); >> +} >> + >> /* Matches for of_clk_init */ >> static const __initconst struct of_device_id clk_match[] = { >> {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, >> @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { >> {} >> }; >> >> +/* Matches for gate clocks */ >> +static const __initconst struct of_device_id clk_gates_match[] = { >> + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, >> + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, >> + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, >> + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, >> + {} >> +}; >> + >> static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, >> void *function) >> { >> @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) >> >> /* Register mux clocks */ >> of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); >> + >> + /* Register gate clocks */ >> + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); > > I'm still a device tree noob, so this may be a dumb question. Can the > above be converted to of_clk_init? As far as I know, you can't, because of_clk_init doesn't allow for custom data to be passed to the functions. If we were to use of_clk_init we would need one function per clock, and it would be mostly duplicated code / wrappers. I've added Gregory on Cc, please correct me if this is not the case. Thanks for the review, Emilio
Quoting Emilio López (2013-04-03 18:19:22) > Hi Mike, > > El 03/04/13 18:48, Mike Turquette escribió: > > Quoting Emilio López (2013-03-27 14:20:37) > >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > >> index d528a24..244de90 100644 > >> --- a/drivers/clk/sunxi/clk-sunxi.c > >> +++ b/drivers/clk/sunxi/clk-sunxi.c > >> @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, > >> } > >> > >> > >> + > > > > A lot of white space between these functions. > > All of the function blocks are separated with three spaces on the file; > I thought it looked more readable that way, but I don't have any strong > opinion on separation. Is there any standard for this used on the kernel? > > In any case, and to keep consistency, can we handle this on a follow-up > patch? > If it's consistent throughout the file then go ahead and keep it. > >> +/** > >> + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks > >> + */ > >> + > >> +#define SUNXI_GATES_MAX_SIZE 64 > >> + > >> +struct gates_data { > >> + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); > >> +}; > >> + > >> +static const __initconst struct gates_data axi_gates_data = { > >> + .mask = {1}, > >> +}; > >> + > >> +static const __initconst struct gates_data ahb_gates_data = { > >> + .mask = {0x7F77FFF, 0x14FB3F}, > >> +}; > >> + > >> +static const __initconst struct gates_data apb0_gates_data = { > >> + .mask = {0x4EF}, > >> +}; > >> + > >> +static const __initconst struct gates_data apb1_gates_data = { > >> + .mask = {0xFF00F7}, > >> +}; > >> + > >> +static void __init sunxi_gates_clk_setup(struct device_node *node, > >> + struct gates_data *data) > >> +{ > >> + struct clk_onecell_data *clk_data; > >> + const char *clk_parent; > >> + const char *clk_name; > >> + void *reg; > >> + int qty; > >> + int i = 0; > >> + int j = 0; > >> + int ignore; > >> + > >> + reg = of_iomap(node, 0); > >> + > >> + clk_parent = of_clk_get_parent_name(node, 0); > >> + > >> + /* Worst-case size approximation and memory allocation */ > >> + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); > >> + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); > >> + if (!clk_data) > >> + return; > >> + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); > >> + if (!clk_data->clks) { > >> + kfree(clk_data); > >> + return; > >> + } > >> + > >> + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { > >> + of_property_read_string_index(node, "clock-output-names", > >> + j, &clk_name); > >> + > >> + /* No driver claims this clock, but it should remain gated */ > > > > Should the comment read, "ungated" instead of "gated"? > > Indeed, good catch. Do you want me to resend the series, or can you > amend this when picking the patches? > I can amend it, but I don't like to get into the habit of doing that too often. I'll wait on Gregory's response on the of_clk_init stuff before I do so. Regards, Mike > >> + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; > >> + > >> + clk_data->clks[i] = clk_register_gate(NULL, clk_name, > >> + clk_parent, ignore, > >> + reg + 4 * (i/32), i % 32, > >> + 0, &clk_lock); > >> + WARN_ON(IS_ERR(clk_data->clks[i])); > >> + > >> + j++; > >> + } > >> + > >> + /* Adjust to the real max */ > >> + clk_data->clk_num = i; > >> + > >> + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); > >> +} > >> + > >> /* Matches for of_clk_init */ > >> static const __initconst struct of_device_id clk_match[] = { > >> {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, > >> @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { > >> {} > >> }; > >> > >> +/* Matches for gate clocks */ > >> +static const __initconst struct of_device_id clk_gates_match[] = { > >> + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, > >> + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, > >> + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, > >> + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, > >> + {} > >> +}; > >> + > >> static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, > >> void *function) > >> { > >> @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) > >> > >> /* Register mux clocks */ > >> of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); > >> + > >> + /* Register gate clocks */ > >> + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); > > > > I'm still a device tree noob, so this may be a dumb question. Can the > > above be converted to of_clk_init? > > As far as I know, you can't, because of_clk_init doesn't allow for > custom data to be passed to the functions. If we were to use of_clk_init > we would need one function per clock, and it would be mostly duplicated > code / wrappers. I've added Gregory on Cc, please correct me if this is > not the case. > > Thanks for the review, > > Emilio
On 04/04/2013 04:46 AM, Mike Turquette wrote: > Quoting Emilio López (2013-04-03 18:19:22) >> Hi Mike, >> >> El 03/04/13 18:48, Mike Turquette escribió: >>> Quoting Emilio López (2013-03-27 14:20:37) >>>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >>>> index d528a24..244de90 100644 >>>> --- a/drivers/clk/sunxi/clk-sunxi.c >>>> +++ b/drivers/clk/sunxi/clk-sunxi.c >>>> @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, >>>> } >>>> >>>> >>>> + >>> >>> A lot of white space between these functions. >> >> All of the function blocks are separated with three spaces on the file; >> I thought it looked more readable that way, but I don't have any strong >> opinion on separation. Is there any standard for this used on the kernel? >> >> In any case, and to keep consistency, can we handle this on a follow-up >> patch? >> > > If it's consistent throughout the file then go ahead and keep it. > >>>> +/** >>>> + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks >>>> + */ >>>> + >>>> +#define SUNXI_GATES_MAX_SIZE 64 >>>> + >>>> +struct gates_data { >>>> + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); >>>> +}; >>>> + >>>> +static const __initconst struct gates_data axi_gates_data = { >>>> + .mask = {1}, >>>> +}; >>>> + >>>> +static const __initconst struct gates_data ahb_gates_data = { >>>> + .mask = {0x7F77FFF, 0x14FB3F}, >>>> +}; >>>> + >>>> +static const __initconst struct gates_data apb0_gates_data = { >>>> + .mask = {0x4EF}, >>>> +}; >>>> + >>>> +static const __initconst struct gates_data apb1_gates_data = { >>>> + .mask = {0xFF00F7}, >>>> +}; >>>> + >>>> +static void __init sunxi_gates_clk_setup(struct device_node *node, >>>> + struct gates_data *data) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + const char *clk_parent; >>>> + const char *clk_name; >>>> + void *reg; >>>> + int qty; >>>> + int i = 0; >>>> + int j = 0; >>>> + int ignore; >>>> + >>>> + reg = of_iomap(node, 0); >>>> + >>>> + clk_parent = of_clk_get_parent_name(node, 0); >>>> + >>>> + /* Worst-case size approximation and memory allocation */ >>>> + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); >>>> + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); >>>> + if (!clk_data) >>>> + return; >>>> + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); >>>> + if (!clk_data->clks) { >>>> + kfree(clk_data); >>>> + return; >>>> + } >>>> + >>>> + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { >>>> + of_property_read_string_index(node, "clock-output-names", >>>> + j, &clk_name); >>>> + >>>> + /* No driver claims this clock, but it should remain gated */ >>> >>> Should the comment read, "ungated" instead of "gated"? >> >> Indeed, good catch. Do you want me to resend the series, or can you >> amend this when picking the patches? >> > > I can amend it, but I don't like to get into the habit of doing that too > often. > > I'll wait on Gregory's response on the of_clk_init stuff before I do so. > > Regards, > Mike > >>>> + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; >>>> + >>>> + clk_data->clks[i] = clk_register_gate(NULL, clk_name, >>>> + clk_parent, ignore, >>>> + reg + 4 * (i/32), i % 32, >>>> + 0, &clk_lock); >>>> + WARN_ON(IS_ERR(clk_data->clks[i])); >>>> + >>>> + j++; >>>> + } >>>> + >>>> + /* Adjust to the real max */ >>>> + clk_data->clk_num = i; >>>> + >>>> + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); >>>> +} >>>> + >>>> /* Matches for of_clk_init */ >>>> static const __initconst struct of_device_id clk_match[] = { >>>> {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, >>>> @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { >>>> {} >>>> }; >>>> >>>> +/* Matches for gate clocks */ >>>> +static const __initconst struct of_device_id clk_gates_match[] = { >>>> + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, >>>> + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, >>>> + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, >>>> + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, >>>> + {} >>>> +}; >>>> + >>>> static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, >>>> void *function) >>>> { >>>> @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) >>>> >>>> /* Register mux clocks */ >>>> of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); >>>> + >>>> + /* Register gate clocks */ >>>> + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); >>> >>> I'm still a device tree noob, so this may be a dumb question. Can the >>> above be converted to of_clk_init? >> >> As far as I know, you can't, because of_clk_init doesn't allow for >> custom data to be passed to the functions. If we were to use of_clk_init >> we would need one function per clock, and it would be mostly duplicated >> code / wrappers. I've added Gregory on Cc, please correct me if this is >> not the case. I confirm that the current implementation of of_clk_init only take setup functions. That was also the reason why we didn't use it in mvebu/clk-core.c for example. Maybe it should be a good improvement to allow of_clk_init to receive a function _and_ data for a given node. Something like that; typedef void (*of_clk_init_cb_t)(struct device_node *, void *data); struct clk_of_setup { of_clk_init_cb_t clk_init_cb; void* data; } void __init of_clk_init(const struct of_device_id *matches) { struct device_node *np; if (!matches) matches = __clk_of_table; for_each_matching_node(np, matches) { const struct of_device_id *match = of_match_node(matches, np); match->clk_init_cb(np, match->data); } } I have just writen this code in this email I didn't even try to compile this code. This was just a rough idea which could be use as a base for future patch for 3.11. With a good coccinelle script it should be not too complicated. But about this current patch, I am fine with it, and you can add my Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> >> Thanks for the review, >> >> Emilio
Quoting Gregory CLEMENT (2013-04-04 13:45:24) > On 04/04/2013 04:46 AM, Mike Turquette wrote: > > Quoting Emilio López (2013-04-03 18:19:22) > >> Hi Mike, > >> > >> El 03/04/13 18:48, Mike Turquette escribió: > >>> Quoting Emilio López (2013-03-27 14:20:37) > >>>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > >>>> index d528a24..244de90 100644 > >>>> --- a/drivers/clk/sunxi/clk-sunxi.c > >>>> +++ b/drivers/clk/sunxi/clk-sunxi.c > >>>> @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, > >>>> } > >>>> > >>>> > >>>> + > >>> > >>> A lot of white space between these functions. > >> > >> All of the function blocks are separated with three spaces on the file; > >> I thought it looked more readable that way, but I don't have any strong > >> opinion on separation. Is there any standard for this used on the kernel? > >> > >> In any case, and to keep consistency, can we handle this on a follow-up > >> patch? > >> > > > > If it's consistent throughout the file then go ahead and keep it. > > > >>>> +/** > >>>> + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks > >>>> + */ > >>>> + > >>>> +#define SUNXI_GATES_MAX_SIZE 64 > >>>> + > >>>> +struct gates_data { > >>>> + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); > >>>> +}; > >>>> + > >>>> +static const __initconst struct gates_data axi_gates_data = { > >>>> + .mask = {1}, > >>>> +}; > >>>> + > >>>> +static const __initconst struct gates_data ahb_gates_data = { > >>>> + .mask = {0x7F77FFF, 0x14FB3F}, > >>>> +}; > >>>> + > >>>> +static const __initconst struct gates_data apb0_gates_data = { > >>>> + .mask = {0x4EF}, > >>>> +}; > >>>> + > >>>> +static const __initconst struct gates_data apb1_gates_data = { > >>>> + .mask = {0xFF00F7}, > >>>> +}; > >>>> + > >>>> +static void __init sunxi_gates_clk_setup(struct device_node *node, > >>>> + struct gates_data *data) > >>>> +{ > >>>> + struct clk_onecell_data *clk_data; > >>>> + const char *clk_parent; > >>>> + const char *clk_name; > >>>> + void *reg; > >>>> + int qty; > >>>> + int i = 0; > >>>> + int j = 0; > >>>> + int ignore; > >>>> + > >>>> + reg = of_iomap(node, 0); > >>>> + > >>>> + clk_parent = of_clk_get_parent_name(node, 0); > >>>> + > >>>> + /* Worst-case size approximation and memory allocation */ > >>>> + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); > >>>> + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); > >>>> + if (!clk_data) > >>>> + return; > >>>> + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); > >>>> + if (!clk_data->clks) { > >>>> + kfree(clk_data); > >>>> + return; > >>>> + } > >>>> + > >>>> + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { > >>>> + of_property_read_string_index(node, "clock-output-names", > >>>> + j, &clk_name); > >>>> + > >>>> + /* No driver claims this clock, but it should remain gated */ > >>> > >>> Should the comment read, "ungated" instead of "gated"? > >> > >> Indeed, good catch. Do you want me to resend the series, or can you > >> amend this when picking the patches? > >> > > > > I can amend it, but I don't like to get into the habit of doing that too > > often. > > > > I'll wait on Gregory's response on the of_clk_init stuff before I do so. > > > > Regards, > > Mike > > > >>>> + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; > >>>> + > >>>> + clk_data->clks[i] = clk_register_gate(NULL, clk_name, > >>>> + clk_parent, ignore, > >>>> + reg + 4 * (i/32), i % 32, > >>>> + 0, &clk_lock); > >>>> + WARN_ON(IS_ERR(clk_data->clks[i])); > >>>> + > >>>> + j++; > >>>> + } > >>>> + > >>>> + /* Adjust to the real max */ > >>>> + clk_data->clk_num = i; > >>>> + > >>>> + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); > >>>> +} > >>>> + > >>>> /* Matches for of_clk_init */ > >>>> static const __initconst struct of_device_id clk_match[] = { > >>>> {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, > >>>> @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { > >>>> {} > >>>> }; > >>>> > >>>> +/* Matches for gate clocks */ > >>>> +static const __initconst struct of_device_id clk_gates_match[] = { > >>>> + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, > >>>> + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, > >>>> + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, > >>>> + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, > >>>> + {} > >>>> +}; > >>>> + > >>>> static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, > >>>> void *function) > >>>> { > >>>> @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) > >>>> > >>>> /* Register mux clocks */ > >>>> of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); > >>>> + > >>>> + /* Register gate clocks */ > >>>> + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); > >>> > >>> I'm still a device tree noob, so this may be a dumb question. Can the > >>> above be converted to of_clk_init? > >> > >> As far as I know, you can't, because of_clk_init doesn't allow for > >> custom data to be passed to the functions. If we were to use of_clk_init > >> we would need one function per clock, and it would be mostly duplicated > >> code / wrappers. I've added Gregory on Cc, please correct me if this is > >> not the case. > > I confirm that the current implementation of of_clk_init only take setup > functions. That was also the reason why we didn't use it in mvebu/clk-core.c > for example. > > Maybe it should be a good improvement to allow of_clk_init to receive > a function _and_ data for a given node. Something like that; > > typedef void (*of_clk_init_cb_t)(struct device_node *, void *data); > struct clk_of_setup { > of_clk_init_cb_t clk_init_cb; > void* data; > } > > void __init of_clk_init(const struct of_device_id *matches) > { > struct device_node *np; > > if (!matches) > matches = __clk_of_table; > > for_each_matching_node(np, matches) { > const struct of_device_id *match = of_match_node(matches, np); > match->clk_init_cb(np, match->data); > } > } > > I have just writen this code in this email I didn't even try to compile this code. > This was just a rough idea which could be use as a base for future patch for 3.11. > With a good coccinelle script it should be not too complicated. > Sure, maybe some solution can be found for 3.11. > > But about this current patch, I am fine with it, and you can add my > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > Thanks. The three clock patches have been pushed to clk-next. Regards, Mike > > >> > >> Thanks for the review, > >> > >> Emilio > > > -- > Gregory Clement, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com
diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt index 20b8479..729f524 100644 --- a/Documentation/devicetree/bindings/clock/sunxi.txt +++ b/Documentation/devicetree/bindings/clock/sunxi.txt @@ -10,15 +10,23 @@ Required properties: "allwinner,sun4i-pll1-clk" - for the main PLL clock "allwinner,sun4i-cpu-clk" - for the CPU multiplexer clock "allwinner,sun4i-axi-clk" - for the AXI clock + "allwinner,sun4i-axi-gates-clk" - for the AXI gates "allwinner,sun4i-ahb-clk" - for the AHB clock + "allwinner,sun4i-ahb-gates-clk" - for the AHB gates "allwinner,sun4i-apb0-clk" - for the APB0 clock + "allwinner,sun4i-apb0-gates-clk" - for the APB0 gates "allwinner,sun4i-apb1-clk" - for the APB1 clock "allwinner,sun4i-apb1-mux-clk" - for the APB1 clock muxing + "allwinner,sun4i-apb1-gates-clk" - for the APB1 gates Required properties for all clocks: - reg : shall be the control register address for the clock. - clocks : shall be the input parent clock(s) phandle for the clock -- #clock-cells : from common clock binding; shall be set to 0. +- #clock-cells : from common clock binding; shall be set to 0 except for + "allwinner,sun4i-*-gates-clk" where it shall be set to 1 + +Additionally, "allwinner,sun4i-*-gates-clk" clocks require: +- clock-output-names : the corresponding gate names that the clock controls For example: @@ -42,3 +50,102 @@ cpu: cpu@01c20054 { reg = <0x01c20054 0x4>; clocks = <&osc32k>, <&osc24M>, <&pll1>; }; + + + +Gate clock outputs + +The "allwinner,sun4i-*-gates-clk" clocks provide several gatable outputs; +their corresponding offsets as present on sun4i are listed below. Note that +some of these gates are not present on sun5i. + + * AXI gates ("allwinner,sun4i-axi-gates-clk") + + DRAM 0 + + * AHB gates ("allwinner,sun4i-ahb-gates-clk") + + USB0 0 + EHCI0 1 + OHCI0 2* + EHCI1 3 + OHCI1 4* + SS 5 + DMA 6 + BIST 7 + MMC0 8 + MMC1 9 + MMC2 10 + MMC3 11 + MS 12** + NAND 13 + SDRAM 14 + + ACE 16 + EMAC 17 + TS 18 + + SPI0 20 + SPI1 21 + SPI2 22 + SPI3 23 + PATA 24 + SATA 25** + GPS 26* + + VE 32 + TVD 33 + TVE0 34 + TVE1 35 + LCD0 36 + LCD1 37 + + CSI0 40 + CSI1 41 + + HDMI 43 + DE_BE0 44 + DE_BE1 45 + DE_FE0 46 + DE_FE1 47 + + MP 50 + + MALI400 52 + + * APB0 gates ("allwinner,sun4i-apb0-gates-clk") + + CODEC 0 + SPDIF 1* + AC97 2 + IIS 3 + + PIO 5 + IR0 6 + IR1 7 + + KEYPAD 10 + + * APB1 gates ("allwinner,sun4i-apb1-gates-clk") + + I2C0 0 + I2C1 1 + I2C2 2 + + CAN 4 + SCR 5 + PS20 6 + PS21 7 + + UART0 16 + UART1 17 + UART2 18 + UART3 19 + UART4 20 + UART5 21 + UART6 22 + UART7 23 + +Notation: + [*]: The datasheet didn't mention these, but they are present on AW code + [**]: The datasheet had this marked as "NC" but they are used on AW code diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c index d528a24..244de90 100644 --- a/drivers/clk/sunxi/clk-sunxi.c +++ b/drivers/clk/sunxi/clk-sunxi.c @@ -302,6 +302,82 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, } + +/** + * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks + */ + +#define SUNXI_GATES_MAX_SIZE 64 + +struct gates_data { + DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); +}; + +static const __initconst struct gates_data axi_gates_data = { + .mask = {1}, +}; + +static const __initconst struct gates_data ahb_gates_data = { + .mask = {0x7F77FFF, 0x14FB3F}, +}; + +static const __initconst struct gates_data apb0_gates_data = { + .mask = {0x4EF}, +}; + +static const __initconst struct gates_data apb1_gates_data = { + .mask = {0xFF00F7}, +}; + +static void __init sunxi_gates_clk_setup(struct device_node *node, + struct gates_data *data) +{ + struct clk_onecell_data *clk_data; + const char *clk_parent; + const char *clk_name; + void *reg; + int qty; + int i = 0; + int j = 0; + int ignore; + + reg = of_iomap(node, 0); + + clk_parent = of_clk_get_parent_name(node, 0); + + /* Worst-case size approximation and memory allocation */ + qty = find_last_bit(data->mask, SUNXI_GATES_MAX_SIZE); + clk_data = kmalloc(sizeof(struct clk_onecell_data), GFP_KERNEL); + if (!clk_data) + return; + clk_data->clks = kzalloc((qty+1) * sizeof(struct clk *), GFP_KERNEL); + if (!clk_data->clks) { + kfree(clk_data); + return; + } + + for_each_set_bit(i, data->mask, SUNXI_GATES_MAX_SIZE) { + of_property_read_string_index(node, "clock-output-names", + j, &clk_name); + + /* No driver claims this clock, but it should remain gated */ + ignore = !strcmp("ahb_sdram", clk_name) ? CLK_IGNORE_UNUSED : 0; + + clk_data->clks[i] = clk_register_gate(NULL, clk_name, + clk_parent, ignore, + reg + 4 * (i/32), i % 32, + 0, &clk_lock); + WARN_ON(IS_ERR(clk_data->clks[i])); + + j++; + } + + /* Adjust to the real max */ + clk_data->clk_num = i; + + of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); +} + /* Matches for of_clk_init */ static const __initconst struct of_device_id clk_match[] = { {.compatible = "fixed-clock", .data = of_fixed_clk_setup,}, @@ -331,6 +407,15 @@ static const __initconst struct of_device_id clk_mux_match[] = { {} }; +/* Matches for gate clocks */ +static const __initconst struct of_device_id clk_gates_match[] = { + {.compatible = "allwinner,sun4i-axi-gates-clk", .data = &axi_gates_data,}, + {.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &ahb_gates_data,}, + {.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &apb0_gates_data,}, + {.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &apb1_gates_data,}, + {} +}; + static void __init of_sunxi_table_clock_setup(const struct of_device_id *clk_match, void *function) { @@ -359,4 +444,7 @@ void __init sunxi_init_clocks(void) /* Register mux clocks */ of_sunxi_table_clock_setup(clk_mux_match, sunxi_mux_clk_setup); + + /* Register gate clocks */ + of_sunxi_table_clock_setup(clk_gates_match, sunxi_gates_clk_setup); }
This patchset adds DT support for all the AXI, AHB, APB0 and APB1 gates present on sunxi SoCs. Signed-off-by: Emilio López <emilio@elopez.com.ar> --- Changes from v1: - renamed compatibles to sun4i - renamed TWI -> I2C - added note about gate availability on sun5i Documentation/devicetree/bindings/clock/sunxi.txt | 109 +++++++++++++++++++++- drivers/clk/sunxi/clk-sunxi.c | 88 +++++++++++++++++ 2 files changed, 196 insertions(+), 1 deletion(-)