From mboxrd@z Thu Jan 1 00:00:00 1970 From: gregory.clement@free-electrons.com (Gregory CLEMENT) Date: Thu, 04 Apr 2013 22:45:24 +0200 Subject: [PATCH v2 1/7] clk: sunxi: Add support for AXI, AHB, APB0 and APB1 gates In-Reply-To: <20130404024658.3383.63115@quantum> References: <1363962042-29536-1-git-send-email-emilio@elopez.com.ar> <1364419243-18446-1-git-send-email-emilio@elopez.com.ar> <1364419243-18446-2-git-send-email-emilio@elopez.com.ar> <20130403214815.3383.60153@quantum> <515CD51A.2070503@elopez.com.ar> <20130404024658.3383.63115@quantum> Message-ID: <515DE664.5040505@free-electrons.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> >> 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