[PATCH v2 1/7] clk: sunxi: Add support for AXI, AHB, APB0 and APB1 gates
Mike Turquette
mturquette at linaro.org
Thu Apr 4 17:08:52 EDT 2013
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 at 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
More information about the linux-arm-kernel
mailing list