Re: [PATCHv5 16/31] CLK: TI: DRA7: Add APLL support

[Keerthy <a0393675@ti.com>]

Hi Mark/Tero,

Sorry for responding late.

On Monday 19 August 2013 07:22 PM, Tero Kristo wrote:
> On 08/13/2013 02:14 PM, Mark Rutland wrote:
>> On Fri, Aug 02, 2013 at 05:25:35PM +0100, Tero Kristo wrote:
>>> From: Keerthy <j-keerthy@ti.com>
>>>
>>> The patch adds support for DRA7 PCIe APLL. The APLL
>>> sources the optional functional clocks for PCIe module.
>>>
>>> APLL stands for Analog PLL. This is different when comapred
>>> with DPLL meaning Digital PLL, the phase detection is done
>>> using an analog circuit.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   .../devicetree/bindings/clock/ti/apll.txt          |   32 +++
>>>   arch/arm/mach-omap2/clock.h                        |    1 -
>>>   drivers/clk/ti/Makefile                            |    2 +-
>>>   drivers/clk/ti/apll.c                              |  209 
>>> ++++++++++++++++++++
>>>   include/linux/clk/ti.h                             |    2 +
>>>   5 files changed, 244 insertions(+), 2 deletions(-)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/clock/ti/apll.txt
>>>   create mode 100644 drivers/clk/ti/apll.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/ti/apll.txt 
>>> b/Documentation/devicetree/bindings/clock/ti/apll.txt
>>> new file mode 100644
>>> index 0000000..f7a82e9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ti/apll.txt
>>> @@ -0,0 +1,32 @@
>>> +Binding for Texas Instruments APLL clock.
>>> +
>>> +This binding uses the common clock binding[1].  It assumes a
>>> +register-mapped APLL with usually two selectable input clocks
>>> +(reference clock and bypass clock), with analog phase locked
>>> +loop logic for multiplying the input clock to a desired output
>>> +clock. This clock also typically supports different operation
>>> +modes (locked, low power stop etc.) APLL mostly behaves like
>>> +a subtype of a DPLL [2], although a simplified one at that.
>>> +
>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> +[2] Documentation/devicetree/bindings/clock/ti/dpll.txt
>>> +
>>> +Required properties:
>>> +- compatible : shall be "ti,dra7-apll-clock"
>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>> +- clocks : link phandles of parent clocks (clk-ref and clk-bypass)
>>> +- reg : array of register base addresses for controlling the APLL:
>>> +       reg[0] = control register
>>> +       reg[1] = idle status register
>>
>> Using reg-names is likely a good idea here.
>
> I'll change these for next rev to be similar to what was discussed in 
> the DPLL part.
>
>>
>>> +- ti,clk-ref : link phandle for the reference clock
>>> +- ti,clk-bypass : link phandle for the bypass clock
>>
>> You don't need this. Use the clocks and clock-names properties.
>
> Ditto.
>
>>
>> [...]
>>
>>> +static int dra7_apll_enable(struct clk_hw *hw)
>>> +{
>>> +       struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>>> +       int r = 0, i = 0;
>>> +       struct dpll_data *ad;
>>> +       const char *clk_name;
>>> +       u8 state = 1;
>>> +       u32 v;
>>> +
>>> +       ad = clk->dpll_data;
>>> +       if (!ad)
>>> +               return -EINVAL;
>>> +
>>> +       clk_name = __clk_get_name(clk->hw.clk);
>>> +
>>> +       state <<= __ffs(ad->idlest_mask);
>>> +
>>> +       /* Check is already locked */
>>> +       if ((__raw_readl(ad->idlest_reg) & ad->idlest_mask) == state)
>>> +               return r;
>>
>> Why __raw_readl rather than raw_readl?
>
> Hmm not sure, Keerthy, do you have any comment on this as the patch 
> was originally written by you? :)

It was more taking the reference from omap2 code. raw_readl can be used.

>
>>
>>> +
>>> +       v = __raw_readl(ad->control_reg);
>>> +       v &= ~ad->enable_mask;
>>> +       v |= APLL_FORCE_LOCK << __ffs(ad->enable_mask);
>>> +       __raw_writel(v, ad->control_reg);
>>
>> Why not raw_writel? Do you not need the rmb() provided by writel, here
>> or anywhere else?
>
> Same, Keerthy? Probably just legacy copy paste from omap2 code and can 
> be updated based on your comment. Some of these might actually be some 
> old optimizations, but the APLL enable/disable should be called so 
> seldom it shouldn't matter. If no objections, I'll just change these 
> all for next rev.

Thanks Tero.

>
>>
>> [...]
>>
>>> +void __init of_dra7_apll_setup(struct device_node *node)
>>> +{
>>> +       const struct clk_ops *ops;
>>> +       struct clk *clk;
>>> +       const char *clk_name = node->name;
>>> +       int num_parents;
>>> +       const char **parent_names = NULL;
>>> +       struct of_phandle_args clkspec;
>>> +       u8 apll_flags = 0;
>>> +       struct dpll_data *ad;
>>> +       u32 idlest_mask = 0x1;
>>> +       u32 autoidle_mask = 0x3;
>>> +       int i;
>>> +
>>> +       ops = &apll_ck_ops;
>>> +       ad = kzalloc(sizeof(*ad), GFP_KERNEL);
>>> +       if (!ad) {
>>> +               pr_err("%s: could not allocate dpll_data\n", __func__);
>>> +               return;
>>> +       }
>>> +
>>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>>> +
>>> +       num_parents = of_clk_get_parent_count(node);
>>> +       if (num_parents < 1) {
>>> +               pr_err("%s: omap dpll %s must have parent(s)\n",
>>> +                      __func__, node->name);
>>> +               goto cleanup;
>>> +       }
>>> +
>>> +       parent_names = kzalloc(sizeof(char *) * num_parents, 
>>> GFP_KERNEL);
>>> +
>>> +       for (i = 0; i < num_parents; i++)
>>> +               parent_names[i] = of_clk_get_parent_name(node, i);
>>> +
>>> +       clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
>>> +       ad->clk_ref = of_clk_get_from_provider(&clkspec);
>>
>> Use clocks, clock-names, and of_clk_get_by_name().
>
> Yea, will change.
>
> -Tero

Regards,
Keerthy