ARM: OMAP: clock conversion to DT

clk_get_sys / clk_get can now find clocks from device-tree. If a DT clock is found, an entry is added to the clk_lookup list also for subsequent searches.

Signed-off-by Tero Kristo <[email protected]>

Cc Russell King <[email protected]>

---
  drivers/clk/clkdev.c |   32 ++++++++++++++++++++++++++++++++
  1 file changed, 32 insertions(+)

drivers/clk/clkdev.c +19 -0

index 442a313..e39f082 100644
@@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
  EXPORT_SYMBOL(of_clk_get_by_name);
  #endif
 
+/**
+ * clkdev_add_nolock - add lookup entry for a clock
+ * @cl: pointer to new clock lookup entry
+ *
+ * Non-locking version, used internally by clk_find() to add DT based
+ * clock lookup entries.
+ */
+static void clkdev_add_nolock(struct clk_lookup *cl)
+{
+	list_add_tail(&cl->node, &clocks);
+}
+
  /*
   * Find the correct struct clk for the device and connection ID.
   * We do slightly fuzzy matching here:

@@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
  {
  	struct clk_lookup *p, *cl = NULL;
  	int match, best_found = 0, best_possible = 0;
+	struct device_node *node;
+	struct clk *clk;
+	struct of_phandle_args clkspec;
 
  	if (dev_id)
  		best_possible += 2;

@@ -133,6 +148,23 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
  				break;
  		}
  	}
+
+	if (cl)
+		return cl;
+

drivers/clk/clkdev.c +3 -0

+		clkdev_add_nolock(cl);
+	}
+
  	return cl;
  }
 

Quoted 59 lines · Tero Kristo

clk_get_sys / clk_get can now find clocks from device-tree. If a DT clock is found, an entry is added to the clk_lookup list also for subsequent searches.

Signed-off-by Tero Kristo <[email protected]>

Cc Russell King <[email protected]>

---
  drivers/clk/clkdev.c |   32 ++++++++++++++++++++++++++++++++
  1 file changed, 32 insertions(+)

drivers/clk/clkdev.c +12 -0

index 442a313..e39f082 100644
@@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node
 *np, const char *name)
  EXPORT_SYMBOL(of_clk_get_by_name);
  #endif
 
+/**
+ * clkdev_add_nolock - add lookup entry for a clock
+ * @cl: pointer to new clock lookup entry
+ *
+ * Non-locking version, used internally by clk_find() to add DT based
+ * clock lookup entries.
+ */
+static void clkdev_add_nolock(struct clk_lookup *cl)
+{
+    list_add_tail(&cl->node, &clocks);
+}
+
  /*
   * Find the correct struct clk for the device and connection ID.

* We do slightly fuzzy matching here:

drivers/clk/clkdev.c +3 -0

@@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char
 *dev_id, const char *con_id)
  {
      struct clk_lookup *p, *cl = NULL;
      int match, best_found = 0, best_possible = 0;
+    struct device_node *node;
+    struct clk *clk;
+    struct of_phandle_args clkspec;
 
      if (dev_id)

          best_possible += 2;

drivers/clk/clkdev.c +9 -0

@@ -133,6 +148,23 @@ static struct clk_lookup *clk_find(const char
 *dev_id, const char *con_id)
                  break;
          }
      }
+
+    if (cl)
+        return cl;
+
+    /* If clock was not found, attempt to look-up from DT */
+    node = of_find_node_by_name(NULL, con_id);
+
+    clkspec.np = node;
+

drivers/clk/clkdev.c static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) · line 164

Quoted 5 lines

drivers/clk/clkdev.c +5 -0

+    clk = of_clk_get_from_provider(&clkspec);
+
+    if (!IS_ERR(clk)) {
+        /* We found a clock, add node to clkdev */
+        cl = clkdev_alloc(clk, con_id, dev_id);

clkdev_alloc may return NULL depending on vclkdev_alloc in which case clkdev_add_nolock will crash trying to dereference it.

Quoted 8 lines

drivers/clk/clkdev.c +3 -0

+        clkdev_add_nolock(cl);
+    }
+
      return cl;
  }

Parses OMAP clock data from DT and registers those clocks with the clock framework. dt_omap_clk_init must be called early during boot for timer initialization so it is exported and called from the existing clock code instead of probing like a real driver. Based on initial work done by Mike Turquette.

Signed-off-by Tero Kristo <[email protected]>

Cc Mike Turquette <[email protected]>

---
  drivers/clk/Makefile      |    1 +
  drivers/clk/omap/Makefile |    1 +
  drivers/clk/omap/clk.c    |   39 +++++++++++++++++++++++++++++++++++++++

  include/linux/clk/omap.h  |   24 ++++++++++++++++++++++++
  4 files changed, 65 insertions(+)

create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c
create mode 100644 include/linux/clk/omap.h

drivers/clk/Makefile +1 -0

index 4038c2b..d3c3733 100644
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_VT8500)	+= clk-vt8500.o
  obj-$(CONFIG_ARCH_ZYNQ)		+= zynq/
  obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
  obj-$(CONFIG_PLAT_SAMSUNG)	+= samsung/
+obj-$(CONFIG_ARCH_OMAP)		+= omap/
 
  obj-$(CONFIG_X86)		+= x86/
 

drivers/clk/omap/Makefile new file +1 -0

new file mode 100644
index 0000000..8195931
@@ -0,0 +1 @@
+obj-y					+= clk.o

drivers/clk/omap/clk.c new file +0 -0

new file mode 100644
index 0000000..4bf1929

drivers/clk/omap/clk.c new file +11 -0

+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *     Tero Kristo <[email protected]>
+ *     Mike Turquette <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty

drivers/clk/omap/clk.c new file +2 -0

+	return 0;
+}

include/linux/clk/omap.h new file +0 -0

new file mode 100644
index 0000000..647f02f

include/linux/clk/omap.h new file +17 -0

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __LINUX_CLK_OMAP_H_
+#define __LINUX_CLK_OMAP_H_
+
+int __init dt_omap_clk_init(void);
+
+#endif

Quoted 6 lines · Tero Kristo

Parses OMAP clock data from DT and registers those clocks with the clock framework. dt_omap_clk_init must be called early during boot for timer initialization so it is exported and called from the existing clock code instead of probing like a real driver. Based on initial work done by Mike Turquette.

Quoted 7 lines

Signed-off-by Tero Kristo <[email protected]>

Cc Mike Turquette <[email protected]>

---
  drivers/clk/Makefile      |    1 +
  drivers/clk/omap/Makefile |    1 +
  drivers/clk/omap/clk.c    |   39

+++++++++++++++++++++++++++++++++++++++

Minor suggestion - should we just start drivers/clk/ti/ instead of clk/omap?

AM335x/DRA7 are not "strictly OMAP".. might also allow for some reuse for other TI platforms..

Quoted 28 lines

  include/linux/clk/omap.h  |   24 ++++++++++++++++++++++++
  4 files changed, 65 insertions(+)

create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c
create mode 100644 include/linux/clk/omap.h

drivers/clk/Makefile +1 -0

index 4038c2b..d3c3733 100644
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_VT8500)    += clk-vt8500.o
  obj-$(CONFIG_ARCH_ZYNQ)        += zynq/
  obj-$(CONFIG_ARCH_TEGRA)    += tegra/
  obj-$(CONFIG_PLAT_SAMSUNG)    += samsung/
+obj-$(CONFIG_ARCH_OMAP)        += omap/
 
  obj-$(CONFIG_X86)        += x86/
 

drivers/clk/omap/Makefile new file +1 -0

new file mode 100644
index 0000000..8195931
@@ -0,0 +1 @@
+obj-y                    += clk.o

drivers/clk/omap/clk.c new file +0 -0

new file mode 100644

drivers/clk/omap/clk.c line 2

Quoted 6 lines

index 0000000..4bf1929

drivers/clk/omap/clk.c new file +2 -0

@@ -0,0 +1,39 @@
+/*
+ * OMAP PRCM clock driver

maybe then prcm-clk.c ?

Quoted 16 lines

drivers/clk/omap/clk.c new file +15 -0

+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *     Tero Kristo <[email protected]>
+ *     Mike Turquette <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+

drivers/clk/omap/clk.c line 23

Quoted 6 lines

drivers/clk/omap/clk.c new file +6 -0

+#include <linux/clk-provider.h>
+#include <linux/clk/omap.h>
+#include <linux/kernel.h>
+#include <linux/of_device.h>
+
+/* FIXME - should the OMAP PRCM clock driver match generic types? */

should it? :)

drivers/clk/omap/clk.c line 25

Quoted 3 lines

drivers/clk/omap/clk.c new file +2 -0

+static const struct of_device_id clk_match[] = {
+    {.compatible = "fixed-clock", .data = of_fixed_clk_setup, },

drivers/clk/clk-fixed-rate.c seems to already do this with CLK_OF_DECLARE, or am I mistaken? and so on?

Quoted 11 lines

drivers/clk/omap/clk.c new file +9 -0

+    {.compatible = "mux-clock", .data = of_mux_clk_setup, },
+    {.compatible = "fixed-factor-clock",
+        .data = of_fixed_factor_clk_setup, },
+    {.compatible = "divider-clock", .data = of_divider_clk_setup, },
+    {.compatible = "gate-clock", .data = of_gate_clk_setup, },
+    {},
+};
+
+/* FIXME - need to initialize early; skip real driver registration &

probe */
+int __init dt_omap_clk_init(void)

Might be good to have kernel doc to explain the init requirement as documented in commit message.

drivers/clk/omap/clk.c line 37

Quoted 3 lines

drivers/clk/omap/clk.c new file +2 -0

+{
+    of_clk_init(clk_match);

just doing of_clk_init(NULL); should do the job, no? that could even be a static inline OR introduced in board_generic without additional headers?

Quoted 10 lines

drivers/clk/omap/clk.c new file +2 -0

+    return 0;
+}

include/linux/clk/omap.h new file +1 -0

new file mode 100644
index 0000000..647f02f
@@ -0,0 +1,24 @@
+/*

include/linux/clk/omap.h line 7

Quoted 6 lines

include/linux/clk/omap.h new file +6 -0

+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.

would you like to match licensing to that of the C file?

Quoted 20 lines

include/linux/clk/omap.h new file +9 -0

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA

02111-1307 USA
+ */

include/linux/clk/omap.h new file +7 -0

+
+#ifndef __LINUX_CLK_OMAP_H_
+#define __LINUX_CLK_OMAP_H_
+
+int __init dt_omap_clk_init(void);
+
+#endif

Quoted 18 lines · Nishanth Menon

Quoted 6 lines · Tero Kristo

Parses OMAP clock data from DT and registers those clocks with the clock framework. dt_omap_clk_init must be called early during boot for timer initialization so it is exported and called from the existing clock code instead of probing like a real driver. Based on initial work done by Mike Turquette.

Quoted 7 lines

Signed-off-by Tero Kristo <[email protected]>

Cc Mike Turquette <[email protected]>

---
  drivers/clk/Makefile      |    1 +
  drivers/clk/omap/Makefile |    1 +
  drivers/clk/omap/clk.c    |   39

+++++++++++++++++++++++++++++++++++++++

Minor suggestion - should we just start drivers/clk/ti/ instead of clk/omap?

Quoted 2 lines

AM335x/DRA7 are not "strictly OMAP".. might also allow for some reuse for other TI platforms..

Not sure, this idea has been bounced around a bit. samsung has its own directory under drivers/clk/ so maybe. I can change this if there is a strong wish for this.

Quoted 29 lines

Quoted 29 lines

  include/linux/clk/omap.h  |   24 ++++++++++++++++++++++++
  4 files changed, 65 insertions(+)

create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c
create mode 100644 include/linux/clk/omap.h

drivers/clk/Makefile +1 -0

index 4038c2b..d3c3733 100644
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_VT8500)    += clk-vt8500.o
  obj-$(CONFIG_ARCH_ZYNQ)        += zynq/
  obj-$(CONFIG_ARCH_TEGRA)    += tegra/
  obj-$(CONFIG_PLAT_SAMSUNG)    += samsung/
+obj-$(CONFIG_ARCH_OMAP)        += omap/
 
  obj-$(CONFIG_X86)        += x86/
 

drivers/clk/omap/Makefile new file +1 -0

new file mode 100644
index 0000000..8195931
@@ -0,0 +1 @@
+obj-y                    += clk.o

drivers/clk/omap/clk.c new file +0 -0

new file mode 100644

Quoted 8 lines

drivers/clk/omap/clk.c line 2

Quoted 6 lines

index 0000000..4bf1929

drivers/clk/omap/clk.c new file +2 -0

@@ -0,0 +1,39 @@
+/*
+ * OMAP PRCM clock driver

maybe then prcm-clk.c ?

Maybe remove the PRCM part. We have some clocks behind i2c for example, and we might want to add support for them here.

Quoted 24 lines · Tero Kristo

Quoted 22 lines · Nishanth Menon

Quoted 8 lines · Tero Kristo

Parses OMAP clock data from DT and registers those clocks with the clock
framework. dt_omap_clk_init must be called early during boot for timer initialization so it is exported and called from the existing clock code
instead of probing like a real driver. Based on initial work done by Mike Turquette.

Quoted 7 lines

Signed-off-by Tero Kristo <[email protected]>

Cc Mike Turquette <[email protected]>

---
  drivers/clk/Makefile      |    1 +
  drivers/clk/omap/Makefile |    1 +
  drivers/clk/omap/clk.c    |   39

+++++++++++++++++++++++++++++++++++++++

Minor suggestion - should we just start drivers/clk/ti/ instead of clk/omap?

AM335x/DRA7 are not "strictly OMAP".. might also allow for some reuse for other TI platforms..

Quoted 3 lines

Not sure, this idea has been bounced around a bit. samsung has its own directory under drivers/clk/ so maybe. I can change this if there is a strong wish for this.

just my 2 cents - if we use drivers/clk/ti, we can then move required stuff from arch/arm/mach-davinci or arch/arm/mach-keystone to a ti generic location allowing us to reuse things as needed.

Quoted 39 lines

Quoted 38 lines

Quoted 34 lines

  include/linux/clk/omap.h  |   24 ++++++++++++++++++++++++
  4 files changed, 65 insertions(+)

create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c
create mode 100644 include/linux/clk/omap.h

drivers/clk/Makefile +1 -0

index 4038c2b..d3c3733 100644
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_VT8500)    += clk-vt8500.o
  obj-$(CONFIG_ARCH_ZYNQ)        += zynq/
  obj-$(CONFIG_ARCH_TEGRA)    += tegra/
  obj-$(CONFIG_PLAT_SAMSUNG)    += samsung/
+obj-$(CONFIG_ARCH_OMAP)        += omap/
 
  obj-$(CONFIG_X86)        += x86/
 

drivers/clk/omap/Makefile new file +1 -0

new file mode 100644
index 0000000..8195931
@@ -0,0 +1 @@
+obj-y                    += clk.o

drivers/clk/omap/clk.c new file +0 -0

new file mode 100644
index 0000000..4bf1929

drivers/clk/omap/clk.c line 2

Quoted 2 lines

drivers/clk/omap/clk.c new file +2 -0

@@ -0,0 +1,39 @@
+/*
+ * OMAP PRCM clock driver

maybe then prcm-clk.c ?

Quoted 2 lines

Maybe remove the PRCM part. We have some clocks behind i2c for example, and we might want to add support for them here.

I think we can actually drop this file in it's entirety if we stick with of_clk_init(NULL) and depend solely of CLK_OF_DECLARE from relevant drivers.

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Signed-off-by Tero Kristo <[email protected]>

---
  drivers/clk/omap/Makefile |    2 +-
  drivers/clk/omap/clk.c    |    1 +
  drivers/clk/omap/dpll.c   |  295 +++++++++++++++++++++++++++++++++++++++++++++

  3 files changed, 297 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/omap/dpll.c

drivers/clk/omap/Makefile +1 -1

index 8195931..4cad480 100644
@@ -1 +1 @@
-obj-y					+= clk.o
+obj-y					+= clk.o dpll.o

drivers/clk/omap/clk.c +0 -0

index 4bf1929..1dafdaa 100644

drivers/clk/omap/dpll.c new file +16 -0

new file mode 100644
index 0000000..66e82be
@@ -0,0 +1,295 @@
+/*
+ * OMAP DPLL clock support
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * Tero Kristo <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */

drivers/clk/omap/dpll.c new file +18 -0

+
+static const struct clk_ops dpll_m4xen_ck_ops = {
+	.enable		= &omap3_noncore_dpll_enable,
+	.disable	= &omap3_noncore_dpll_disable,
+	.recalc_rate	= &omap4_dpll_regm4xen_recalc,
+	.round_rate	= &omap4_dpll_regm4xen_round_rate,
+	.set_rate	= &omap3_noncore_dpll_set_rate,
+	.get_parent	= &omap2_init_dpll_parent,
+};
+
+static const struct clk_ops dpll_core_ck_ops = {
+	.recalc_rate	= &omap3_dpll_recalc,
+	.get_parent	= &omap2_init_dpll_parent,
+};
+
+static const struct clk_ops dpll_ck_ops = {
+	.enable		= &omap3_noncore_dpll_enable,
+	.disable	= &omap3_noncore_dpll_disable,

drivers/clk/omap/dpll.c new file +13 -0

+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->dpll_data = dpll_data;
+	clk_hw->ops = &clkhwops_omap3_dpll;
+	clk_hw->clkdm_name = clkdm_name;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.flags = flags;

drivers/clk/omap/dpll.c new file +14 -0

+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	clk_hw->ops = &clkhwops_omap4_dpllmx;
+	clk_hw->clksel_reg = reg;
+	clk_hw->hw.init = &init;
+
+	init.name = name;
+	init.ops = ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+

drivers/clk/omap/dpll.c new file +13 -0

+ */
+static void __init of_omap_dpll_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;
+	const char *clkdm_name = NULL;
+	struct of_phandle_args clkspec;
+	u8 dpll_flags = 0;
+	struct dpll_data *dd;
+	u32 idlest_mask = 0x1;

drivers/clk/omap/dpll.c new file +6 -0

+		dd->m4xen_mask = 0x800;
+		dd->lpmode_mask = 1 << 10;
+	}
+
+	dd->mult_mask = mult_mask;
+	dd->div1_mask = div1_mask;

drivers/clk/omap/dpll.c new file +6 -0

+	kfree(dd);
+	return;
+}
+
+static void __init of_omap_dpll_x2_setup(struct device_node *node)
+{

drivers/clk/omap/dpll.c new file +9 -0

+{
+	const struct clk_ops *ops;
+
+	ops = &dpll_ck_ops;
+
+	if (of_property_read_bool(node, "ti,dpll-regm4xen"))
+		ops = &dpll_m4xen_ck_ops;
+
+	if (of_property_read_bool(node, "ti,dpll-core"))

drivers/clk/omap/dpll.c new file +2 -0

+CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup);
+#endif

This patch probably was submitted in the wrong sequence - fails build and few other issues below.

Quoted 2 lines

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Then why is $subject specific to OMAP4? is that because of of_omap4_dpll_setup?

Quoted 8 lines

Signed-off-by Tero Kristo <[email protected]>

---
  drivers/clk/omap/Makefile |    2 +-
  drivers/clk/omap/clk.c    |    1 +
  drivers/clk/omap/dpll.c   |  295

+++++++++++++++++++++++++++++++++++++++++++++

Device Tree Binding documentation?

Quoted 16 lines

  3 files changed, 297 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/omap/dpll.c

drivers/clk/omap/Makefile +1 -1

index 8195931..4cad480 100644
@@ -1 +1 @@
-obj-y                    += clk.o
+obj-y                    += clk.o dpll.o

drivers/clk/omap/clk.c +0 -0

index 4bf1929..1dafdaa 100644
@@ -28,6 +28,7 @@ static const struct of_device_id clk_match[] = {

drivers/clk/omap/clk.c static const struct of_device_id clk_match[] = { · line 33

Quoted 7 lines

drivers/clk/omap/clk.c +1 -0

          .data = of_fixed_factor_clk_setup, },
      {.compatible = "divider-clock", .data = of_divider_clk_setup, },
      {.compatible = "gate-clock", .data = of_gate_clk_setup, },
+    {.compatible = "ti,omap4-dpll-clock", .data =
 of_omap4_dpll_setup, },
      {},
  };

you would not need this if you did just of_clk_init(NULL); ?

Further, at this patch, build fails with:
drivers/clk/omap/clk.c:31:55: error: undefined identifier 'of_omap4_dpll_setup'
drivers/clk/omap/clk.c:31:48: error: ?of_omap4_dpll_setup? undeclared here (not in a function)

which makes sense since we did not export the function.

Quoted 26 lines

drivers/clk/omap/dpll.c new file +19 -0

new file mode 100644
index 0000000..66e82be
@@ -0,0 +1,295 @@
+/*
+ * OMAP DPLL clock support
+ *
+ * Copyright (C) 2013 Texas Instruments, Inc.
+ *
+ * Tero Kristo <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>

drivers/clk/omap/dpll.c line 26

Quoted 7 lines

drivers/clk/omap/dpll.c new file +7 -0

+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/string.h>
+#include <linux/log2.h>
+#include <linux/of.h>
+#include <linux/of_address.h>

after a quick check, are all these required?

drivers/clk/omap/dpll.c line 27

Quoted 2 lines

drivers/clk/omap/dpll.c new file +1 -0

+#include <linux/clk/omap.h>

why?

Quoted 25 lines

drivers/clk/omap/dpll.c new file +24 -0

+
+static const struct clk_ops dpll_m4xen_ck_ops = {
+    .enable        = &omap3_noncore_dpll_enable,
+    .disable    = &omap3_noncore_dpll_disable,
+    .recalc_rate    = &omap4_dpll_regm4xen_recalc,
+    .round_rate    = &omap4_dpll_regm4xen_round_rate,
+    .set_rate    = &omap3_noncore_dpll_set_rate,
+    .get_parent    = &omap2_init_dpll_parent,
+};
+
+static const struct clk_ops dpll_core_ck_ops = {
+    .recalc_rate    = &omap3_dpll_recalc,
+    .get_parent    = &omap2_init_dpll_parent,
+};
+
+static const struct clk_ops dpll_ck_ops = {
+    .enable        = &omap3_noncore_dpll_enable,
+    .disable    = &omap3_noncore_dpll_disable,
+    .recalc_rate    = &omap3_dpll_recalc,
+    .round_rate    = &omap2_dpll_round_rate,
+    .set_rate    = &omap3_noncore_dpll_set_rate,
+    .get_parent    = &omap2_init_dpll_parent,
+    .init        = &omap2_init_clk_clkdm,
+};

drivers/clk/omap/dpll.c line 55

Quoted 4 lines

drivers/clk/omap/dpll.c new file +4 -0

+
+static const struct clk_ops dpll_x2_ck_ops = {
+    .recalc_rate    = &omap3_clkoutx2_recalc,
+};

none of these are defined at this stage of the patch, generates a huge build error for dpll.c
http://pastebin.com/GJucv1A5

drivers/clk/omap/dpll.c line 60

Quoted 5 lines

drivers/clk/omap/dpll.c new file +5 -0

+
+struct clk *omap_clk_register_dpll(struct device *dev, const char *name,
+        const char **parent_names, int num_parents, unsigned long flags,
+        struct dpll_data *dpll_data, const char *clkdm_name,
+        const struct clk_ops *ops)

why should this be public?

drivers/clk/omap/dpll.c line 63

Quoted 4 lines

drivers/clk/omap/dpll.c new file +3 -0

+{
+    struct clk *clk;
+    struct clk_init_data init;

init = { 0 }; just to future proof?

drivers/clk/omap/dpll.c line 64

Quoted 2 lines

drivers/clk/omap/dpll.c new file +1 -0

+    struct clk_hw_omap *clk_hw;

does not exist yet in generic header?

I am assuming you do not do parameter check as you expect clk_register to do the same?

drivers/clk/omap/dpll.c line 67

Quoted 4 lines

drivers/clk/omap/dpll.c new file +3 -0

+
+    /* allocate the divider */
+    clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL);

checkpatch complained:
CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...)

or given we have dev, devm_kzalloc?

Quoted 16 lines

drivers/clk/omap/dpll.c new file +16 -0

+    if (!clk_hw) {
+        pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+        return ERR_PTR(-ENOMEM);
+    }
+
+    clk_hw->dpll_data = dpll_data;
+    clk_hw->ops = &clkhwops_omap3_dpll;
+    clk_hw->clkdm_name = clkdm_name;
+    clk_hw->hw.init = &init;
+
+    init.name = name;
+    init.ops = ops;
+    init.flags = flags;
+    init.parent_names = parent_names;
+    init.num_parents = num_parents;
+

drivers/clk/omap/dpll.c line 90

Quoted 7 lines

drivers/clk/omap/dpll.c new file +7 -0

+    /* register the clock */
+    clk = clk_register(dev, &clk_hw->hw);
+
+    if (IS_ERR(clk))
+        kfree(clk_hw);
+    else
+        omap2_init_clk_hw_omap_clocks(clk);

what if init fails? and it is in mach-omap2 at this point in time?

drivers/clk/omap/dpll.c

Quoted 9 lines

drivers/clk/omap/dpll.c new file +5 -0

+
+    return clk;
+}
+
+struct clk *omap_clk_register_dpll_x2(struct device *dev, const char

*name,

drivers/clk/omap/dpll.c new file +2 -0

+        const char *parent_name, void __iomem *reg,
+        const struct clk_ops *ops)

same question here as well

drivers/clk/omap/dpll.c line 108

Quoted 12 lines

drivers/clk/omap/dpll.c new file +11 -0

+{
+    struct clk *clk;
+    struct clk_init_data init;
+    struct clk_hw_omap *clk_hw;
+
+    if (!parent_name) {
+        pr_err("%s: dpll_x2 must have parent\n", __func__);
+        return ERR_PTR(-EINVAL);
+    }
+
+    clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL);

checkpatch complained:
CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...)

or devm_kzalloc?

Quoted 19 lines

drivers/clk/omap/dpll.c new file +18 -0

+    if (!clk_hw) {
+        pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+        return ERR_PTR(-ENOMEM);
+    }
+
+    clk_hw->ops = &clkhwops_omap4_dpllmx;
+    clk_hw->clksel_reg = reg;
+    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(dev, &clk_hw->hw);
+
+    if (IS_ERR(clk))

drivers/clk/omap/dpll.c line 93

Quoted 6 lines

drivers/clk/omap/dpll.c new file +6 -0

+        kfree(clk_hw);
+    else
+        omap2_init_clk_hw_omap_clocks(clk);
+
+    return clk;
+}

this vaguely sounds like a replica of omap_clk_register_dpll with num_parents and clk_hw->ops different. why not merge the two?

drivers/clk/omap/dpll.c

Quoted 3 lines

drivers/clk/omap/dpll.c new file +2 -0

+
+#ifdef CONFIG_OF

why not build the entire thing *iff* CONFIG_OF (Makefile/Kconfig dep)? that way, we can drop this #ifdef stuff from drivers that dont need to have dual support.

drivers/clk/omap/dpll.c line 137

Quoted 4 lines

drivers/clk/omap/dpll.c new file +3 -0

+
+/**
+ * of_omap_dpll_setup() - Setup function for OMAP DPLL clocks

node and ops not documented.

Quoted 17 lines

+ */

drivers/clk/omap/dpll.c new file +15 -0

+static void __init of_omap_dpll_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;
+    const char *clkdm_name = NULL;
+    struct of_phandle_args clkspec;
+    u8 dpll_flags = 0;
+    struct dpll_data *dd;
+    u32 idlest_mask = 0x1;
+    u32 enable_mask = 0x7;
+    u32 autoidle_mask = 0x7;
+    u32 mult_mask = 0x7ff << 8;

drivers/clk/omap/dpll.c line 160

Quoted 7 lines

drivers/clk/omap/dpll.c new file +7 -0

+    u32 div1_mask = 0x7f;
+    u32 max_multiplier = 2047;
+    u32 max_divider = 128;
+    u32 min_divider = 1;
+    int i;
+
+    dd = kzalloc(sizeof(struct dpll_data), GFP_KERNEL);

kzalloc sizeof(*dd) ?

Quoted 10 lines

drivers/clk/omap/dpll.c new file +9 -0

+    if (!dd) {
+        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) {

drivers/clk/omap/dpll.c line 171

Quoted 2 lines

drivers/clk/omap/dpll.c new file +2 -0

+        pr_err("%s: omap dpll %s must have parent(s)\n",
+            __func__, node->name);

checkpatch complained:
CHECK: Alignment should match open parenthesis
#212: FILE: drivers/clk/omap/dpll.c:171:
After applying the patch, I think you should make __func__ aligned with " and not %

Quoted 4 lines

drivers/clk/omap/dpll.c new file +3 -0

+        goto cleanup;
+    }
+

drivers/clk/omap/dpll.c line 184

Quoted 10 lines

drivers/clk/omap/dpll.c new file +10 -0

+    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);
+
+    of_property_read_u32(node, "ti,idlest-mask", &idlest_mask);
+
+    of_property_read_u32(node, "ti,enable-mask", &enable_mask);
+
+    of_property_read_u32(node, "ti,autoidle-mask", &autoidle_mask);

are these going to be different? or can we catch with compatible flag?

drivers/clk/omap/dpll.c line 190

Quoted 7 lines

drivers/clk/omap/dpll.c new file +6 -0

+
+    clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0);
+    dd->clk_ref = of_clk_get_from_provider(&clkspec);
+    if (!dd->clk_ref) {
+        pr_err("%s: ti,clk-ref for %s not found\n", __func__,
+            clk_name);

CHECK: Alignment should match open parenthesis
#231: FILE: drivers/clk/omap/dpll.c:190:
similar issue here.

drivers/clk/omap/dpll.c line 198

Quoted 8 lines

drivers/clk/omap/dpll.c new file +8 -0

+        goto cleanup;
+    }
+
+    clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0);
+    dd->clk_bypass = of_clk_get_from_provider(&clkspec);
+    if (!dd->clk_bypass) {
+        pr_err("%s: ti,clk-bypass for %s not found\n", __func__,
+            clk_name);

here as well

drivers/clk/omap/dpll.c line 207

Quoted 9 lines

drivers/clk/omap/dpll.c new file +9 -0

+        goto cleanup;
+    }
+
+    of_property_read_string(node, "ti,clkdm-name", &clkdm_name);
+
+    dd->control_reg = of_iomap(node, 0);
+    dd->idlest_reg = of_iomap(node, 1);
+    dd->autoidle_reg = of_iomap(node, 2);
+    dd->mult_div1_reg = of_iomap(node, 3);

if dts has errors, should we not verify mandatory parameters?

drivers/clk/omap/dpll.c line 213

Quoted 6 lines

drivers/clk/omap/dpll.c new file +6 -0

+
+    dd->idlest_mask = idlest_mask;
+    dd->enable_mask = enable_mask;
+    dd->autoidle_mask = autoidle_mask;
+
+    dd->modes = 0xa0;

what is 0xa0?

drivers/clk/omap/dpll.c line 223

Quoted 11 lines

drivers/clk/omap/dpll.c new file +10 -0

+
+    if (of_property_read_bool(node, "ti,dpll-j-type")) {
+        dd->sddiv_mask = 0xff000000;
+        mult_mask = 0xfff << 8;
+        div1_mask = 0xff;
+        max_multiplier = 4095;
+        max_divider = 256;
+    }
+
+    if (of_property_read_bool(node, "ti,dpll-regm4xen")) {

I think we need bindings to understand this better.

Quoted 10 lines

drivers/clk/omap/dpll.c new file +9 -0

+        dd->m4xen_mask = 0x800;
+        dd->lpmode_mask = 1 << 10;
+    }
+
+    dd->mult_mask = mult_mask;
+    dd->div1_mask = div1_mask;
+    dd->max_multiplier = max_multiplier;
+    dd->max_divider = max_divider;
+    dd->min_divider = min_divider;

drivers/clk/omap/dpll.c line 239

Quoted 7 lines

drivers/clk/omap/dpll.c new file +7 -0

+
+    clk = omap_clk_register_dpll(NULL, clk_name, parent_names,
+                num_parents, dpll_flags, dd,
+                clkdm_name, ops);
+
+    if (!IS_ERR(clk))
+        of_clk_add_provider(node, of_clk_src_simple_get, clk);

error check?

drivers/clk/omap/dpll.c line 242

Quoted 3 lines

drivers/clk/omap/dpll.c new file +3 -0

+    return;
+
+cleanup:

kfree(parent_names) ?

Quoted 11 lines

drivers/clk/omap/dpll.c new file +10 -0

+    kfree(dd);
+    return;
+}
+
+static void __init of_omap_dpll_x2_setup(struct device_node *node)
+{
+    struct clk *clk;
+    const char *clk_name = node->name;
+    void __iomem *reg;
+    const char *parent_name;

drivers/clk/omap/dpll.c line 258

Quoted 6 lines

drivers/clk/omap/dpll.c new file +6 -0

+
+    of_property_read_string(node, "clock-output-names", &clk_name);
+
+    parent_name = of_clk_get_parent_name(node, 0);
+
+    reg = of_iomap(node, 0);

if dts has errors, should we not verify mandatory parameters?

drivers/clk/omap/dpll.c line 264

Quoted 7 lines

drivers/clk/omap/dpll.c new file +6 -0

+
+    clk = omap_clk_register_dpll_x2(NULL, clk_name, parent_name,
+                reg, &dpll_x2_ck_ops);
+
+    if (!IS_ERR(clk))
+        of_clk_add_provider(node, of_clk_src_simple_get, clk);

error check?

gentle request - in this setup function we dont see a return of error value (which makes sense), but more importantly - log saying that node was not setup

drivers/clk/omap/dpll.c line 267

Quoted 4 lines

drivers/clk/omap/dpll.c new file +3 -0

+}
+
+__init void of_omap3_dpll_setup(struct device_node *node)

^^ void __init? further, you could make this static.

drivers/clk/omap/dpll.c line 271

Quoted 5 lines

drivers/clk/omap/dpll.c new file +4 -0

+{
+    /* XXX: to be done */
+}
+EXPORT_SYMBOL_GPL(of_omap3_dpll_setup);

you can drop the export if you use of_clk_init(NULL);

drivers/clk/omap/dpll.c line 274

Quoted 5 lines

+CLK_OF_DECLARE(omap3_dpll_clock, "ti,omap3-dpll-clock", of_omap3_dpll_setup);

drivers/clk/omap/dpll.c new file +2 -0

+
+__init void of_omap4_dpll_setup(struct device_node *node)

^^ void __init? further, you could make this static.

Quoted 14 lines

drivers/clk/omap/dpll.c new file +13 -0

+{
+    const struct clk_ops *ops;
+
+    ops = &dpll_ck_ops;
+
+    if (of_property_read_bool(node, "ti,dpll-regm4xen"))
+        ops = &dpll_m4xen_ck_ops;
+
+    if (of_property_read_bool(node, "ti,dpll-core"))
+        ops = &dpll_core_ck_ops;
+
+    if (of_property_read_bool(node, "ti,dpll-clk-x2")) {
+        of_omap_dpll_x2_setup(node);

drivers/clk/omap/dpll.c line 293

Quoted 6 lines

drivers/clk/omap/dpll.c new file +6 -0

+        return;
+    }
+
+    of_omap_dpll_setup(node, ops);
+}
+EXPORT_SYMBOL_GPL(of_omap4_dpll_setup);

you can drop the export if you use of_clk_init(NULL);

Quoted 3 lines

+CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup);

Quoted 2 lines

+#endif

Quoted 2 lines · Nishanth Menon

This patch probably was submitted in the wrong sequence - fails build and few other issues below.

Yeah, I'll double check the build sequence for these.

Quoted 5 lines · Tero Kristo

Quoted 2 lines

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Quoted 2 lines

Then why is $subject specific to OMAP4? is that because of of_omap4_dpll_setup?

The driver only supports omap4 dpll type at this point, omap3 dplls require some modifications on top of this, and are provided later in the series.

Quoted 10 lines

Quoted 9 lines

Signed-off-by Tero Kristo <[email protected]>

---
  drivers/clk/omap/Makefile |    2 +-
  drivers/clk/omap/clk.c    |    1 +
  drivers/clk/omap/dpll.c   |  295

+++++++++++++++++++++++++++++++++++++++++++++

Quoted 1 line

Device Tree Binding documentation?

Didn't bother writing those yet as I haven't received too much feedback whether this approach is acceptable or not.

Quoted 5 lines

Quoted 5 lines

drivers/clk/omap/clk.c +0 -0

index 4bf1929..1dafdaa 100644
@@ -28,6 +28,7 @@ static const struct of_device_id clk_match[] = {

Quoted 8 lines

drivers/clk/omap/clk.c static const struct of_device_id clk_match[] = { · line 33

Quoted 7 lines

drivers/clk/omap/clk.c +1 -0

          .data = of_fixed_factor_clk_setup, },
      {.compatible = "divider-clock", .data = of_divider_clk_setup, },
      {.compatible = "gate-clock", .data = of_gate_clk_setup, },
+    {.compatible = "ti,omap4-dpll-clock", .data =
 of_omap4_dpll_setup, },
      {},
  };

you would not need this if you did just of_clk_init(NULL); ?

Why not? And I think we still need to do this.

Quoted 5 lines

Quoted 3 lines

+#include <linux/clk/omap.h>

why?

Need dpll_data definition for example.

Quoted 9 lines

drivers/clk/omap/clk.c

Quoted 7 lines

drivers/clk/omap/clk.c +2 -0

+
+struct clk *omap_clk_register_dpll(struct device *dev, const char

*name,
+ const char **parent_names, int num_parents, unsigned long
flags,

drivers/clk/omap/clk.c +2 -0

+        struct dpll_data *dpll_data, const char *clkdm_name,
+        const struct clk_ops *ops)

why should this be public?

Currently does not need to be, but someone could in theory build up cclockXxxx_data.c file and use these calls from there. Kind of legacy support, see some of the basic clk types. I guess I can add static to this, and remove some of the params along.

Quoted 3 lines

I am assuming you do not do parameter check as you expect clk_register to do the same?

True, so I'll change the above function to static.

Quoted 4 lines

Quoted 4 lines

drivers/clk/omap/clk.c +3 -0

+
+    /* allocate the divider */
+    clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL);

Quoted 3 lines

checkpatch complained:
CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...)

Hmm, I didn't get this with checkpatch. Some special option/version you use? I still see both types of sizeof used in the kernel.

Quoted 3 lines

or given we have dev, devm_kzalloc?

Actually we don't have dev, check how this is called from below.

Quoted 17 lines

Quoted 17 lines

drivers/clk/omap/clk.c +16 -0

+    if (!clk_hw) {
+        pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+        return ERR_PTR(-ENOMEM);
+    }
+
+    clk_hw->dpll_data = dpll_data;
+    clk_hw->ops = &clkhwops_omap3_dpll;
+    clk_hw->clkdm_name = clkdm_name;
+    clk_hw->hw.init = &init;
+
+    init.name = name;
+    init.ops = ops;
+    init.flags = flags;
+    init.parent_names = parent_names;
+    init.num_parents = num_parents;
+

Quoted 8 lines

drivers/clk/omap/clk.c

Quoted 7 lines

drivers/clk/omap/clk.c +7 -0

+    /* register the clock */
+    clk = clk_register(dev, &clk_hw->hw);
+
+    if (IS_ERR(clk))
+        kfree(clk_hw);
+    else
+        omap2_init_clk_hw_omap_clocks(clk);

what if init fails? and it is in mach-omap2 at this point in time?

Yea, this just calls the autoidle init part under mach-omap2.

Quoted 6 lines

Quoted 5 lines

drivers/clk/omap/clk.c +3 -0

+
+    return clk;
+}

Quoted 12 lines

drivers/clk/omap/clk.c

Quoted 11 lines

drivers/clk/omap/clk.c +10 -0

+
+    if (of_property_read_bool(node, "ti,dpll-j-type")) {
+        dd->sddiv_mask = 0xff000000;
+        mult_mask = 0xfff << 8;
+        div1_mask = 0xff;
+        max_multiplier = 4095;
+        max_divider = 256;
+    }
+
+    if (of_property_read_bool(node, "ti,dpll-regm4xen")) {

I think we need bindings to understand this better.

Or documentation you mean?

Quoted 11 lines

Quoted 11 lines

drivers/clk/omap/clk.c +9 -0

+        dd->m4xen_mask = 0x800;
+        dd->lpmode_mask = 1 << 10;
+    }
+
+    dd->mult_mask = mult_mask;
+    dd->div1_mask = div1_mask;
+    dd->max_multiplier = max_multiplier;
+    dd->max_divider = max_divider;
+    dd->min_divider = min_divider;

Quoted 8 lines

drivers/clk/omap/clk.c

Quoted 7 lines

drivers/clk/omap/clk.c +7 -0

+
+    clk = omap_clk_register_dpll(NULL, clk_name, parent_names,
+                num_parents, dpll_flags, dd,
+                clkdm_name, ops);
+
+    if (!IS_ERR(clk))
+        of_clk_add_provider(node, of_clk_src_simple_get, clk);

error check?

This is not done with other drivers either. Would require clk_unregister use to cleanup the above register call which is currently unavailable. I could add an error trace for this though.

Quoted 11 lines

Quoted 11 lines

drivers/clk/omap/clk.c +10 -0

+    kfree(dd);
+    return;
+}
+
+static void __init of_omap_dpll_x2_setup(struct device_node *node)
+{
+    struct clk *clk;
+    const char *clk_name = node->name;
+    void __iomem *reg;
+    const char *parent_name;

Quoted 8 lines

drivers/clk/omap/clk.c

Quoted 6 lines

drivers/clk/omap/clk.c +6 -0

+
+    of_property_read_string(node, "clock-output-names", &clk_name);
+
+    parent_name = of_clk_get_parent_name(node, 0);
+
+    reg = of_iomap(node, 0);

if dts has errors, should we not verify mandatory parameters?

You mean checking the validity of this pointer? It seems of_iomap does something strange when it fails, e.g. when passed a 0x0 from DT. You can see what I do in a later patch for adding am335x support for DPLLs.

Quoted 4 lines

drivers/clk/omap/clk.c

Quoted 2 lines

drivers/clk/omap/clk.c +2 -0

+}
+EXPORT_SYMBOL_GPL(of_omap4_dpll_setup);

you can drop the export if you use of_clk_init(NULL);

Hmm no?

Actually dug this further, I think the init setup is slightly wrong at the moment, we should not do CLK_OF_DECLARE at all within the omap specific clock drivers, but instead just use the match table from clk.c. I'll change it like so.

Quoted 1 line

Quoted 2 lines

Quoted 2 lines

+CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup);

So, for example this should be removed. We don't want to support this clock type on non-omap platforms just to avoid problems.

Quoted 11 lines · Tero Kristo

Quoted 2 lines · Nishanth Menon

This patch probably was submitted in the wrong sequence - fails build and few other issues below.

Yeah, I'll double check the build sequence for these.

Quoted 5 lines · Tero Kristo

Quoted 2 lines

The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes.

Quoted 6 lines

Quoted 2 lines

Then why is $subject specific to OMAP4? is that because of of_omap4_dpll_setup?

The driver only supports omap4 dpll type at this point, omap3 dplls require some modifications on top of this, and are provided later in the series.

ok.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Signed-off-by Tero Kristo <[email protected]>

---
  drivers/clk/omap/Makefile |    2 +-
  drivers/clk/omap/clk.c    |    1 +
  drivers/clk/omap/dpll.c   |  295

+++++++++++++++++++++++++++++++++++++++++++++

Quoted 4 lines

Quoted 1 line

Device Tree Binding documentation?

Didn't bother writing those yet as I haven't received too much feedback whether this approach is acceptable or not.

Documentation helps simplify the understanding of expected usage - this is useful to review approach as well :)

Quoted 15 lines

Quoted 14 lines

drivers/clk/omap/clk.c static const struct of_device_id clk_match[] = { · line 34

Quoted 13 lines

drivers/clk/omap/clk.c +1 -0

index 4bf1929..1dafdaa 100644
@@ -28,6 +28,7 @@ static const struct of_device_id clk_match[] = {
          .data = of_fixed_factor_clk_setup, },
      {.compatible = "divider-clock", .data = of_divider_clk_setup, },
      {.compatible = "gate-clock", .data = of_gate_clk_setup, },
+    {.compatible = "ti,omap4-dpll-clock", .data =
 of_omap4_dpll_setup, },
      {},
  };

you would not need this if you did just of_clk_init(NULL); ?

Quoted 1 line

Why not? And I think we still need to do this.

CLK_OF_DECLARE will take care of having all required clks registered of_clk_init(NULL); will pick up from that list.

that will remove all extra exports, and make clk.c redundant. [...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

+#include <linux/clk/omap.h>

Quoted 3 lines

Quoted 1 line

why?

Need dpll_data definition for example.

OK - without the ordering of patches, it was not obvious. structures aside,

We should move the functions to this file instead and empty out mach-omap2 gradually, omap_dpll.h should be exported and used by mach-omap2, rather than the other way around.

Quoted 11 lines

Quoted 10 lines

drivers/clk/omap/clk.c

Quoted 8 lines

drivers/clk/omap/clk.c +2 -0

+
+struct clk *omap_clk_register_dpll(struct device *dev, const char

*name,
+ const char **parent_names, int num_parents, unsigned long
flags,

drivers/clk/omap/clk.c +2 -0

+        struct dpll_data *dpll_data, const char *clkdm_name,
+        const struct clk_ops *ops)

why should this be public?

Quoted 5 lines

Currently does not need to be, but someone could in theory build up cclockXxxx_data.c file and use these calls from there. Kind of legacy support, see some of the basic clk types. I guess I can add static to this, and remove some of the params along.

thanks. we should keep unneeded stuff in static unless a specific provable need really arises.

Quoted 10 lines

Quoted 3 lines

I am assuming you do not do parameter check as you expect clk_register to do the same?

True, so I'll change the above function to static.

Quoted 4 lines

Quoted 4 lines

drivers/clk/omap/clk.c +3 -0

+
+    /* allocate the divider */
+    clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL);

Quoted 6 lines

Quoted 3 lines

checkpatch complained:
CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...)

Hmm, I didn't get this with checkpatch. Some special option/version you use? I still see both types of sizeof used in the kernel.

use checkpatch with --strict option

Quoted 6 lines

Quoted 4 lines

or given we have dev, devm_kzalloc?

Actually we don't have dev, check how this is called from below.

ok.

Quoted 26 lines

Quoted 25 lines

Quoted 14 lines

drivers/clk/omap/clk.c +13 -0

+    if (!clk_hw) {
+        pr_err("%s: could not allocate clk_hw_omap\n", __func__);
+        return ERR_PTR(-ENOMEM);
+    }
+
+    clk_hw->dpll_data = dpll_data;
+    clk_hw->ops = &clkhwops_omap3_dpll;
+    clk_hw->clkdm_name = clkdm_name;
+    clk_hw->hw.init = &init;
+
+    init.name = name;
+    init.ops = ops;
+    init.flags = flags;

drivers/clk/omap/clk.c

Quoted 10 lines

drivers/clk/omap/clk.c +10 -0

+    init.parent_names = parent_names;
+    init.num_parents = num_parents;
+
+    /* register the clock */
+    clk = clk_register(dev, &clk_hw->hw);
+
+    if (IS_ERR(clk))
+        kfree(clk_hw);
+    else
+        omap2_init_clk_hw_omap_clocks(clk);

what if init fails? and it is in mach-omap2 at this point in time?

Quoted 1 line

Yea, this just calls the autoidle init part under mach-omap2.

we should make this independent of mach-omap2. calls should be made to here if needed from mach-omap2 instead of the other way around. OR the required code should be moved over here.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/clk/omap/clk.c +3 -0

+
+    return clk;
+}

<snip>

Quoted 13 lines

Quoted 12 lines

drivers/clk/omap/clk.c

Quoted 11 lines

drivers/clk/omap/clk.c +10 -0

+
+    if (of_property_read_bool(node, "ti,dpll-j-type")) {
+        dd->sddiv_mask = 0xff000000;
+        mult_mask = 0xfff << 8;
+        div1_mask = 0xff;
+        max_multiplier = 4095;
+        max_divider = 256;
+    }
+
+    if (of_property_read_bool(node, "ti,dpll-regm4xen")) {

I think we need bindings to understand this better.

Quoted 1 line

Or documentation you mean?

yes. Documentation/devicetree/bindings/....

Quoted 20 lines

Quoted 19 lines

Quoted 8 lines

drivers/clk/omap/clk.c +6 -0

+        dd->m4xen_mask = 0x800;
+        dd->lpmode_mask = 1 << 10;
+    }
+
+    dd->mult_mask = mult_mask;
+    dd->div1_mask = div1_mask;

drivers/clk/omap/clk.c

Quoted 10 lines

drivers/clk/omap/clk.c +10 -0

+    dd->max_multiplier = max_multiplier;
+    dd->max_divider = max_divider;
+    dd->min_divider = min_divider;
+
+    clk = omap_clk_register_dpll(NULL, clk_name, parent_names,
+                num_parents, dpll_flags, dd,
+                clkdm_name, ops);
+
+    if (!IS_ERR(clk))
+        of_clk_add_provider(node, of_clk_src_simple_get, clk);

error check?

Quoted 3 lines

This is not done with other drivers either. Would require clk_unregister use to cleanup the above register call which is currently unavailable. I could add an error trace for this though.

you can definitely ensure this driver is clean :)

Quoted 21 lines

Quoted 12 lines

Quoted 12 lines

drivers/clk/omap/clk.c +10 -0

+    kfree(dd);
+    return;
+}
+
+static void __init of_omap_dpll_x2_setup(struct device_node *node)
+{
+    struct clk *clk;
+    const char *clk_name = node->name;
+    void __iomem *reg;
+    const char *parent_name;

Quoted 8 lines

drivers/clk/omap/clk.c

Quoted 6 lines

drivers/clk/omap/clk.c +6 -0

+
+    of_property_read_string(node, "clock-output-names", &clk_name);
+
+    parent_name = of_clk_get_parent_name(node, 0);
+
+    reg = of_iomap(node, 0);

if dts has errors, should we not verify mandatory parameters?

Quoted 3 lines

You mean checking the validity of this pointer? It seems of_iomap does something strange when it fails, e.g. when passed a 0x0 from DT. You can see what I do in a later patch for adding am335x support for DPLLs.

in general, helping dts writers know of invalid/out of range parameters with a log message helps ensure those are fixed either on development or at some point - it aids debug instead of others having to scratch heads wondering what happened.

if mandatory parameters are verifable at setup and decided as bad, refusing to register is good idea especially with logs, they tend to get fixed rather faster - than an error that pops up when a specific DPLL is used at a later point in time.

just my 2 cents.
[..]

Quoted 17 lines

Quoted 5 lines

drivers/clk/omap/clk.c

Quoted 3 lines

drivers/clk/omap/clk.c +2 -0

+}
+EXPORT_SYMBOL_GPL(of_omap4_dpll_setup);

you can drop the export if you use of_clk_init(NULL);

Hmm no?

Actually dug this further, I think the init setup is slightly wrong at the moment, we should not do CLK_OF_DECLARE at all within the omap specific clock drivers, but instead just use the match table from clk.c. I'll change it like so.

Quoted 3 lines

Quoted 3 lines

+CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup);

Quoted 2 lines

So, for example this should be removed. We don't want to support this clock type on non-omap platforms just to avoid problems.

As discussed offline, doing the other way around and doing what all other platforms do (which is CLK_OF_DECLARE) is a better idea to ensure standard APIs are carried forward and not spin off OMAP as a "special platform" - at least I dont seem to see any good reasoning for it yet.

Quoted 3 lines

We should move the functions to this file instead and empty out mach-omap2 gradually, omap_dpll.h should be exported and used by mach-omap2, rather than the other way around.

Yeah, the clock stuff should evolve and move here. I just need to start from somewhere.

Tero,

drivers/clk/omap/dpll.c line 208

Quoted 5 lines · Tero Kristo

drivers/clk/omap/dpll.c new file +5 -0

+	dd->control_reg = of_iomap(node, 0);
+	dd->idlest_reg = of_iomap(node, 1);
+	dd->autoidle_reg = of_iomap(node, 2);
+	dd->mult_div1_reg = of_iomap(node, 3);
+

[]...

drivers/clk/omap/dpll.c line 258

Quoted 1 line

drivers/clk/omap/dpll.c new file +1 -0

+	reg = of_iomap(node, 0);

Doing an of_iomap() for every single clock register seems like an overkill and might have performance penalties at boot.

Quoted 11 lines · Rajendra Nayak

Tero,

drivers/clk/omap/dpll.c line 208

Quoted 5 lines · Tero Kristo

drivers/clk/omap/dpll.c new file +5 -0

+    dd->control_reg = of_iomap(node, 0);
+    dd->idlest_reg = of_iomap(node, 1);
+    dd->autoidle_reg = of_iomap(node, 2);
+    dd->mult_div1_reg = of_iomap(node, 3);
+

[]...

Quoted 1 line

+ reg = of_iomap(node, 0);

Quoted 3 lines

Doing an of_iomap() for every single clock register seems like an overkill
and might have performance penalties at boot.

the other option might be to use offset and a single allocation - but I think Tero should comment if this is possible or if registers on some SoCs are strewn all over the place

Some of the clock.h contents are needed by the new OMAP clock driver, including dpll_data and clk_hw_omap. Thus, move these to the generic omap header file which can be accessed by the driver.

Signed-off-by Tero Kristo <[email protected]>

---
  arch/arm/mach-omap2/clock.h |  151 +----------------------------------------
  include/linux/clk/omap.h    |  157 ++++++++++++++++++++++++++++++++++++++++++-
  2 files changed, 157 insertions(+), 151 deletions(-)

arch/arm/mach-omap2/clock.h +1 -127

index 7aa32cd..d1a3125 100644
@@ -21,6 +21,7 @@
 
  #include <linux/clkdev.h>
  #include <linux/clk-provider.h>
+#include <linux/clk/omap.h>
 
  struct omap_clk {
  	u16				cpu;

@@ -178,83 +179,6 @@ struct clksel {
  	const struct clksel_rate *rates;
  };
 
-/**
- * struct dpll_data - DPLL registers and integration data
- * @mult_div1_reg: register containing the DPLL M and N bitfields
- * @mult_mask: mask of the DPLL M bitfield in @mult_div1_reg
- * @div1_mask: mask of the DPLL N bitfield in @mult_div1_reg
- * @clk_bypass: struct clk pointer to the clock's bypass clock input
- * @clk_ref: struct clk pointer to the clock's reference clock input
- * @control_reg: register containing the DPLL mode bitfield
- * @enable_mask: mask of the DPLL mode bitfield in @control_reg
- * @last_rounded_rate: cache of the last rate result of omap2_dpll_round_rate()
- * @last_rounded_m: cache of the last M result of omap2_dpll_round_rate()
- * @last_rounded_m4xen: cache of the last M4X result of
- *			omap4_dpll_regm4xen_round_rate()
- * @last_rounded_lpmode: cache of the last lpmode result of
- *			 omap4_dpll_lpmode_recalc()
- * @max_multiplier: maximum valid non-bypass multiplier value (actual)
- * @last_rounded_n: cache of the last N result of omap2_dpll_round_rate()
- * @min_divider: minimum valid non-bypass divider value (actual)
- * @max_divider: maximum valid non-bypass divider value (actual)
- * @modes: possible values of @enable_mask
- * @autoidle_reg: register containing the DPLL autoidle mode bitfield
- * @idlest_reg: register containing the DPLL idle status bitfield
- * @autoidle_mask: mask of the DPLL autoidle mode bitfield in @autoidle_reg
- * @freqsel_mask: mask of the DPLL jitter correction bitfield in @control_reg
- * @idlest_mask: mask of the DPLL idle status bitfield in @idlest_reg
- * @lpmode_mask: mask of the DPLL low-power mode bitfield in @control_reg
- * @m4xen_mask: mask of the DPLL M4X multiplier bitfield in @control_reg
- * @auto_recal_bit: bitshift of the driftguard enable bit in @control_reg
- * @recal_en_bit: bitshift of the PRM_IRQENABLE_* bit for recalibration IRQs
- * @recal_st_bit: bitshift of the PRM_IRQSTATUS_* bit for recalibration IRQs
- * @flags: DPLL type/features (see below)
- *
- * Possible values for @flags:
- * DPLL_J_TYPE: "J-type DPLL" (only some 36xx, 4xxx DPLLs)
- *
- * @freqsel_mask is only used on the OMAP34xx family and AM35xx.
- *
- * XXX Some DPLLs have multiple bypass inputs, so it's not technically
- * correct to only have one @clk_bypass pointer.
- *
- * XXX The runtime-variable fields (@last_rounded_rate, @last_rounded_m,
- * @last_rounded_n) should be separated from the runtime-fixed fields
- * and placed into a different structure, so that the runtime-fixed data
- * can be placed into read-only space.
- */
-struct dpll_data {
-	void __iomem		*mult_div1_reg;
-	u32			mult_mask;
-	u32			div1_mask;
-	struct clk		*clk_bypass;
-	struct clk		*clk_ref;
-	void __iomem		*control_reg;
-	u32			enable_mask;
-	unsigned long		last_rounded_rate;
-	u16			last_rounded_m;
-	u8			last_rounded_m4xen;
-	u8			last_rounded_lpmode;
-	u16			max_multiplier;
-	u8			last_rounded_n;
-	u8			min_divider;
-	u16			max_divider;
-	u8			modes;
-	void __iomem		*autoidle_reg;
-	void __iomem		*idlest_reg;
-	u32			autoidle_mask;
-	u32			freqsel_mask;
-	u32			idlest_mask;
-	u32			dco_mask;
-	u32			sddiv_mask;
-	u32			lpmode_mask;
-	u32			m4xen_mask;
-	u8			auto_recal_bit;
-	u8			recal_en_bit;
-	u8			recal_st_bit;
-	u8			flags;
-};
-
  /*
   * struct clk.flags possibilities
   *

@@ -274,56 +198,6 @@ struct dpll_data {
  #define INVERT_ENABLE		(1 << 4)	/* 0 enables, 1 disables */
  #define CLOCK_CLKOUTX2		(1 << 5)
 
-/**
- * struct clk_hw_omap - OMAP struct clk
- * @node: list_head connecting this clock into the full clock list
- * @enable_reg: register to write to enable the clock (see @enable_bit)
- * @enable_bit: bitshift to write to enable/disable the clock (see @enable_reg)
- * @flags: see "struct clk.flags possibilities" above
- * @clksel_reg: for clksel clks, register va containing src/divisor select
- * @clksel_mask: bitmask in @clksel_reg for the src/divisor selector
- * @clksel: for clksel clks, pointer to struct clksel for this clock
- * @dpll_data: for DPLLs, pointer to struct dpll_data for this clock
- * @clkdm_name: clockdomain name that this clock is contained in
- * @clkdm: pointer to struct clockdomain, resolved from @clkdm_name at runtime
- * @rate_offset: bitshift for rate selection bitfield (OMAP1 only)
- * @src_offset: bitshift for source selection bitfield (OMAP1 only)
- *
- * XXX @rate_offset, @src_offset should probably be removed and OMAP1
- * clock code converted to use clksel.
- *
- */
-
-struct clk_hw_omap_ops;
-
-struct clk_hw_omap {
-	struct clk_hw		hw;
-	struct list_head	node;
-	unsigned long		fixed_rate;
-	u8			fixed_div;
-	void __iomem		*enable_reg;
-	u8			enable_bit;
-	u8			flags;
-	void __iomem		*clksel_reg;
-	u32			clksel_mask;
-	const struct clksel	*clksel;
-	struct dpll_data	*dpll_data;
-	const char		*clkdm_name;
-	struct clockdomain	*clkdm;
-	const struct clk_hw_omap_ops	*ops;
-};
-
-struct clk_hw_omap_ops {
-	void			(*find_idlest)(struct clk_hw_omap *oclk,
-					void __iomem **idlest_reg,
-					u8 *idlest_bit, u8 *idlest_val);
-	void			(*find_companion)(struct clk_hw_omap *oclk,
-					void __iomem **other_reg,
-					u8 *other_bit);
-	void			(*allow_idle)(struct clk_hw_omap *oclk);
-	void			(*deny_idle)(struct clk_hw_omap *oclk);
-};
-
  unsigned long omap_fixed_divisor_recalc(struct clk_hw *hw,
  					unsigned long parent_rate);
 

arch/arm/mach-omap2/clock.h +0 -3

  extern const struct clk_hw_omap_ops clkhwops_iclk;
  extern const struct clk_hw_omap_ops clkhwops_omap3430es2_ssi_wait;
  extern const struct clk_hw_omap_ops clkhwops_omap3430es2_iclk_ssi_wait;

@@ -460,8 +314,5 @@ extern const struct clksel_rate div31_1to31_rates[];
 
  extern int am33xx_clk_init(void);
 
-extern int omap2_clkops_enable_clkdm(struct clk_hw *hw);
-extern void omap2_clkops_disable_clkdm(struct clk_hw *hw);
-
 
 

  extern void omap_clocks_register(struct omap_clk *oclks, int cnt);
  #endif

include/linux/clk/omap.h +24 -1

index 647f02f..cba892a 100644
@@ -19,6 +19,161 @@
  #ifndef __LINUX_CLK_OMAP_H_
  #define __LINUX_CLK_OMAP_H_
 
-int __init dt_omap_clk_init(void);
+/**
+ * struct dpll_data - DPLL registers and integration data
+ * @mult_div1_reg: register containing the DPLL M and N bitfields
+ * @mult_mask: mask of the DPLL M bitfield in @mult_div1_reg
+ * @div1_mask: mask of the DPLL N bitfield in @mult_div1_reg
+ * @clk_bypass: struct clk pointer to the clock's bypass clock input
+ * @clk_ref: struct clk pointer to the clock's reference clock input
+ * @control_reg: register containing the DPLL mode bitfield
+ * @enable_mask: mask of the DPLL mode bitfield in @control_reg
+ * @last_rounded_rate: cache of the last rate result of omap2_dpll_round_rate()
+ * @last_rounded_m: cache of the last M result of omap2_dpll_round_rate()
+ * @last_rounded_m4xen: cache of the last M4X result of
+ *                     omap4_dpll_regm4xen_round_rate()
+ * @last_rounded_lpmode: cache of the last lpmode result of
+ *                      omap4_dpll_lpmode_recalc()
+ * @max_multiplier: maximum valid non-bypass multiplier value (actual)
+ * @last_rounded_n: cache of the last N result of omap2_dpll_round_rate()
+ * @min_divider: minimum valid non-bypass divider value (actual)
+ * @max_divider: maximum valid non-bypass divider value (actual)
+ * @modes: possible values of @enable_mask
+ * @autoidle_reg: register containing the DPLL autoidle mode bitfield
+ * @idlest_reg: register containing the DPLL idle status bitfield
+ * @autoidle_mask: mask of the DPLL autoidle mode bitfield in @autoidle_reg
+ * @freqsel_mask: mask of the DPLL jitter correction bitfield in @control_reg

include/linux/clk/omap.h +12 -0

+	void __iomem		*mult_div1_reg;
+	u32			mult_mask;
+	u32			div1_mask;
+	struct clk		*clk_bypass;
+	struct clk		*clk_ref;
+	void __iomem		*control_reg;
+	u32			enable_mask;
+	unsigned long		last_rounded_rate;
+	u16			last_rounded_m;
+	u8			last_rounded_m4xen;
+	u8			last_rounded_lpmode;
+	u16			max_multiplier;

include/linux/clk/omap.h +15 -0

+	u32			lpmode_mask;
+	u32			m4xen_mask;
+	u8			auto_recal_bit;
+	u8			recal_en_bit;
+	u8			recal_st_bit;
+	u8			flags;
+};
+
+/**
+ * struct clk_hw_omap - OMAP struct clk
+ * @node: list_head connecting this clock into the full clock list
+ * @enable_reg: register to write to enable the clock (see @enable_bit)
+ * @enable_bit: bitshift to write to enable/disable the clock (see @enable_reg)
+ * @flags: see "struct clk.flags possibilities" above
+ * @clksel_reg: for clksel clks, register va containing src/divisor select

include/linux/clk/omap.h +12 -0

+	void __iomem		*enable_reg;
+	u8			enable_bit;
+	u8			flags;
+	void __iomem		*clksel_reg;
+	u32			clksel_mask;
+	const struct clksel	*clksel;
+	struct dpll_data	*dpll_data;
+	const char		*clkdm_name;
+	struct clockdomain	*clkdm;
+	const struct clk_hw_omap_ops	*ops;
+};
+