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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxx>

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@xxxxxx>
Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
---
  .../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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux