Re: [PATCHv9 06/43] CLK: ti: add init support for clock IP blocks

[Tero Kristo <t-kristo@ti.com>]

On 11/01/2013 09:13 PM, Nishanth Menon wrote:
> On 11/01/2013 04:12 AM, Tero Kristo wrote:
>> On 10/31/2013 05:42 PM, Nishanth Menon wrote:
>>> On 10/25/2013 10:57 AM, Tero Kristo wrote:
>>>> ti_dt_clk_init_provider() can now be used to initialize the contents of
>>>> a single clock IP block. This parses all the clocks under the IP block
>>>> and calls the corresponding init function for them.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>>    drivers/clk/ti/clk.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/clk/ti.h |    1 +
>>>>    2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
>>>> index ad58b01..7f030d7 100644
>>>> --- a/drivers/clk/ti/clk.c
>>>> +++ b/drivers/clk/ti/clk.c
>>>> @@ -19,6 +19,9 @@
>>>>    #include <linux/clkdev.h>
>>>>    #include <linux/clk/ti.h>
>>>>    #include <linux/of.h>
>>>> +#include <linux/list.h>
>>>> +
>>>> +extern struct of_device_id __clk_of_table[];
>>>>
>>>>    /**
>>>>     * ti_dt_clocks_register - register DT duplicate clocks during boot
>>>> @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
>>>>    		}
>>>>    	}
>>>>    }
>>>> +
>>>> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *);
>>>> +
>>>> +struct clk_init_item {
>>>> +	struct regmap *regmap;
>>>> +	struct device_node *np;
>>>> +	ti_of_clk_init_cb_t init_cb;
>>>> +	struct list_head node;
>>>> +};
>>>> +
>>>> +static LIST_HEAD(retry_list);
>>>> +
>>>> +void __init ti_dt_clk_init_provider(struct device_node *parent,
>>>> +				    struct regmap *regmap)
>>>> +{
>>>> +	const struct of_device_id *match;
>>>> +	struct device_node *np;
>>>> +	ti_of_clk_init_cb_t clk_init_cb;
>>>> +	struct clk_init_item *retry;
>>>> +	struct clk_init_item *tmp;
>>>> +	int ret;
>>>> +
>>>> +	for_each_child_of_node(parent, np) {
>>>> +		match = of_match_node(__clk_of_table, np);
>>>> +		if (!match)
>>>> +			continue;
>>>> +		clk_init_cb = match->data;
>>>
>>> I must admit I am confused here.
>>
>> Yea this patch is something I am not quite comfortable myself yet and
>> would like improvement ideas....
>>
>>> a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match
>>> data. The prototype of the generic of_clk_init_cb_t is typedef void
>>> (*of_clk_init_cb_t)(struct device_node *);
>>> b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes
>>> from __clk_of_table
>>
>> __clk_of_table contains the function pointers for the clock init
>> functions, not clock nodes.
>
> yes, apologies, should have stated the prototypes of functions in a
> single __clk_of_table should not be two different argument list. The
> reason is as follows: CLK_OF_DECLARE fills up that list. there can be
> generic drivers such as [1] which might be registered, OR in the case
> of MULTI_ARCH - multiple other SoC drivers would have registered
> there. There is a bunch of stuff we could mess up here.
>
> If we do not have a choice, if we would like to maintain a different
> prototype based init list, we should probably do it in a different
> structure - example __clk_of_table_regmap or something similar..
>
>
>>
>>>
>>> I assume we need ti_dt_clk_init_provider to be always called with
>>> clock list, as a result of_clk_init(NULL); will never be invoked. This
>>> is counter productive as you have have generic non SoC clock providers
>>> as well who would have been invoked with of_clk_init(NULL);
>>
>> Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock
>> nodes under respective IP blocks. Two reasons here:
>>
>> a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to
>> the clock init functions, basically the pointer to the memory map region
>> (regmap.)
>> b) of_clk_init(NULL) will initialize all the clock nodes in the system,
>> irrespective of the hierarchy considerations.
>>
>> Only thing that can be done, is to make the API introduced in this patch
>> a generic API and call it something like of_clk_init_children().
>
> you still can do that:
>
> of_prcm_init->ti_dt_clk_init_provider-> instead of matching nodes from
> __clk_of_table, you can make it __clk_of_regmap_table -> the
> CLK_OF_DECLARE will not be good enough here ofcourse, but this will
> probably belong to a generic regmap enabled clock support framework -
> i dont see much that is TI specific in drivers/clk/ti/clk.c and it
> makes sense to be part of generic clock framework handling as generic
> clock framework will receive regmap support based on previous patches.
> that allows for other platforms to use regmap based clk drivers as well.

Yea different init tables can be introduced, however I still need to 
split the support between hierarchical version and the blunt version 
that registers everything. Looking at the work from Jyri, its possible I 
need to add calls to both.

-Tero

>
>
> [1] http://marc.info/?l=linux-omap&m=138331451210184&w=2
>