Re: [PATCHv9 04/43] CLK: TI: Add DPLL clock support

[Nishanth Menon <nm@ti.com>]

On 10/31/2013 09:56 AM, Tero Kristo wrote:
> On 10/31/2013 04:19 PM, Nishanth Menon wrote:
>> On 10/25/2013 10:56 AM, Tero Kristo wrote:
>> [...]
>>
>>> diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt
>>> new file mode 100644
>>> index 0000000..7b87721
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt
[...]

>>
>>> +  ti,am3-* dpll types list the registers in the same order, except "autoidle"
>>> +  register is left out as this hardware does not have it, e.g.:
>>> +	reg = <0x40>, <0x50>, <0x60>;
>>> +  results in following register map:
>>> +	base + 0x40 - control
>>> +	base + 0x50 - idlest
>>> +	base + 0x60 - mult-div1
>>> +
>>> +Optional properties:
>>> +- DPLL mode setting - defining any one or more of the following overrides
>>> +  default setting.
>>> +	- ti,low-power-stop : DPLL supports low power stop mode, gating output
>>> +	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
>>> +	- ti,lock : DPLL locks in programmed rate
>>
>>> arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
>>> arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
>>> arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
>>> arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = ((1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED) |
>>> arch/arm/mach-omap2/cclock3xxx_data.c:  .modes          = (1 << DPLL_LOW_POWER_STOP) | (1 << DPLL_LOCKED),
>>> arch/arm/mach-omap2/dpll3xxx.c: _omap3_dpll_write_clken(clk, DPLL_LOCKED);
>>
>> There is no if checks for DPLL_LOCKED which is set with ti,lock,
>> should we just drop it?
> 
> Probably better to keep it in, as there might be some code in future 
> which needs this. Kind of better for readability also, as the field is 
> specified to list the valid modes for the DPLL. One could argue that 
> current kernel code is wrong for _omap3_dpll_write_clken(clk, 
> DPLL_LOCKED) as it does not check if the mode is valid for the DPLL or not.

yep, the thought did cross my mind.

> 
>>
>> [...]
>>> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
>>> new file mode 100644
>>> index 0000000..89733c9
>>> --- /dev/null
>>> +++ b/drivers/clk/ti/dpll.c
>>
>> [...]
>>
>>> +/**
>>> + * ti_clk_register_dpll() - Registers the DPLL clock
>>> + * @name:	Name of the clock node
>>> + * @parent_names: list of parent names
>>> + * @num_parents: num of parents in parent_names
>>> + * @flags:	init flags
>>> + * @dpll_data:	DPLL data
>>> + * @ops:	ops for DPLL
>>> + */
>>> +static struct clk *ti_clk_register_dpll(const char *name,
>>> +					const char **parent_names,
>>> +					int num_parents, unsigned long flags,
>>> +					struct dpll_data *dpll_data,
>>> +					const struct clk_ops *ops,
>>> +					struct regmap *regmap)
>>> +{
>>> +	struct clk *clk;
>>> +	struct clk_init_data init = { NULL };
>>> +	struct clk_hw_omap *clk_hw;
>>> +
>>> +	/* allocate the divider */
>>> +	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
>>> +	if (!clk_hw) {
>>> +		pr_err("%s: could not allocate clk_hw_omap\n", __func__);
>>
>> here and in every other driver - please dont add pr_err for kzalloc
>> not able to allocate, kzalloc will provide the warning anyways.
> 
> Oh it does? Never seen that happen as kzalloc never failed for me. :) 
> However, I can remove these as I don't think they will ever happen anyway.

There have been cleanups such as commit
38bb5253a95f2eb8cb765b7ab88aac686de6cb12 etc.. we dont want to
introduce new ones now.

>>> +	clk_hw->ops = hw_ops;
>>> +	of_property_read_u32(node, "reg", (u32 *)&clk_hw->clksel_reg);
>>> +	clk_hw->regmap = regmap;
>>> +	clk_hw->hw.init = &init;
>>> +
>>> +	init.name = name;
>>> +	init.ops = ops;
>>> +	init.parent_names = &parent_name;
>>> +	init.num_parents = 1;
>>> +
>>> +	/* register the clock */
>>> +	clk = clk_register(NULL, &clk_hw->hw);
>>> +
>>> +	if (IS_ERR(clk))
>>> +		kfree(clk_hw);
>>> +	else
>>> +		omap2_init_clk_hw_omap_clocks(clk);
>>> +
>>> +	return clk;
>>> +}
>>> +#endif
>> [...]
>>> +
>>> +#ifdef CONFIG_ARCH_OMAP3
>>
>> just a mention - I understand we are still in transition and we need
>> these #ifdefs, but eventually, we should be able to make the dpll.c
>> independent of SoC variant #ifdefs.
> 
> Eventually yes. Currently it fails to compile if you do OMAP4 / OMAP5 
> only build, as you don't have the OMAP3 support routines in.
fair enough.


-- 
Regards,
Nishanth Menon