Patchwork [v2,1/7] clk: sunxi: Add support for AXI, AHB, APB0 and APB1 gates

login
register
mail settings
Submitter Emilio López
Date March 27, 2013, 9:20 p.m.
Message ID <1364419243-18446-2-git-send-email-emilio@elopez.com.ar>
Download mbox | patch
Permalink /patch/231834/
State New
Headers show

Comments

Emilio López - March 27, 2013, 9:20 p.m.
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(-)
Mike Turquette - April 3, 2013, 9:48 p.m.
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
Emilio López - April 4, 2013, 1:19 a.m.
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
Mike Turquette - April 4, 2013, 2:46 a.m.
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
Gregory CLEMENT - April 4, 2013, 8:45 p.m.
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
Mike Turquette - April 4, 2013, 9:08 p.m.
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

Patch

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);
 }