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

[Tero Kristo]

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 at 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 at ti.com>
>> Signed-off-by: Tero Kristo <t-kristo at 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? :)

>
>> +
>> +       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.

>
> [...]
>
>> +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