Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
On Wednesday 24 April 2013 09:58 PM, Mike Turquette wrote: Quoting Nishanth Menon (2013-04-14 14:19:17) Overall strategy introduced here is simple: a clock node described in device tree blob is used to identify the exact clock provided in the SoC specific data. This is then linked back using of_clk_add_provider to the device node to be accessible by of_clk_get. Based on discussion contributions from Roger Quadros, Grygorii Strashko and others. Cc: Kevin Hilman khil...@deeprootsystems.com Cc: Mike Turquette mturque...@linaro.org Cc: Paul Walmsley p...@pwsan.com [t...@atomide.com: co-developed] Signed-off-by: Tony Lindgren t...@atomide.com Signed-off-by: Nishanth Menon n...@ti.com I can take this into clk-next after the merge window. Please refresh it for -rc1 as we discussed on irc. As an aside, will Tero's series for migrating to drivers/clk/omap[1] and Rajendra's patch for registering clocks late[2] be refreshed after the [2] is already in mainline. merge window? It would be nice to combine these efforts. Thanks, Mike [1] http://article.gmane.org/gmane.linux.ports.arm.omap/95948 [2] http://www.spinics.net/lists/arm-kernel/msg231288.html ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
Quoting Nishanth Menon (2013-04-14 14:19:17) Overall strategy introduced here is simple: a clock node described in device tree blob is used to identify the exact clock provided in the SoC specific data. This is then linked back using of_clk_add_provider to the device node to be accessible by of_clk_get. Based on discussion contributions from Roger Quadros, Grygorii Strashko and others. Cc: Kevin Hilman khil...@deeprootsystems.com Cc: Mike Turquette mturque...@linaro.org Cc: Paul Walmsley p...@pwsan.com [t...@atomide.com: co-developed] Signed-off-by: Tony Lindgren t...@atomide.com Signed-off-by: Nishanth Menon n...@ti.com I can take this into clk-next after the merge window. Please refresh it for -rc1 as we discussed on irc. As an aside, will Tero's series for migrating to drivers/clk/omap[1] and Rajendra's patch for registering clocks late[2] be refreshed after the merge window? It would be nice to combine these efforts. Thanks, Mike [1] http://article.gmane.org/gmane.linux.ports.arm.omap/95948 [2] http://www.spinics.net/lists/arm-kernel/msg231288.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
On 04/12/2013 04:54 PM, Nishanth Menon wrote: OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c. However, this presents an obstacle for using these clock nodes in Device Tree definitions. This is especially true for board specific clocks initially. The fixed clocks are currently found via clock aliases table. There are many possible approaches to this problem as discussed in the following thread: http://marc.info/?t=13637032569r=1w=2. Highlights of the options: a) device specific clk_add_alias: cons: driver handling required b) using an generic clk node and indexing to reach the clock required. This is similar in approach taken by tegra and few other platforms. Example usage: clock = clk 5; cons: potential to have mismatches in indexed table and associated dtb data. In addition, managing continued documentation in bindings as clock indexing increases. Even though readability angle could be improved by using preprocessing of DT using macros, indexed approach is inherently risky from cases like the following: clk indexes in kernel: 1 - mpu_dpll 2 - aux_clk1 3 - core_clk DT entry for peripheral X uses clk 2 to reach aux_clk1. Now, let's say kernel updates indices to: 1 - mpu_dpll 2 - per_dpll 3 - aux_clk1 4 - core_clk using the old dtb(or dts missing an update), on new kernel which has updated indices will result in per_dpll now controlled for peripheral X without warning or any potential error detection. Even though we could claim this is user error, such errors are hard to track down and fix. The error in case (b) is that you shouldn't be changing the DT bindings after they've first been created. That would avoid the problem situation. The person using the old DTB with the new kernel didn't commit user error. An alternate approach introduced here is to introduce device tree bindings corresponding to the clock nodes required in DT definition for SoC which automatically maps back to the definitions in cclockXYZ_data.c. Well, I haven't read the patches, but isn't that exactly what the 2 is in clk 2? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
On Mon, Apr 15, 2013 at 11:22 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 04/12/2013 04:54 PM, Nishanth Menon wrote: OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c. However, this presents an obstacle for using these clock nodes in Device Tree definitions. This is especially true for board specific clocks initially. The fixed clocks are currently found via clock aliases table. There are many possible approaches to this problem as discussed in the following thread: http://marc.info/?t=13637032569r=1w=2. Highlights of the options: a) device specific clk_add_alias: cons: driver handling required b) using an generic clk node and indexing to reach the clock required. This is similar in approach taken by tegra and few other platforms. Example usage: clock = clk 5; cons: potential to have mismatches in indexed table and associated dtb data. In addition, managing continued documentation in bindings as clock indexing increases. Even though readability angle could be improved by using preprocessing of DT using macros, indexed approach is inherently risky from cases like the following: clk indexes in kernel: 1 - mpu_dpll 2 - aux_clk1 3 - core_clk DT entry for peripheral X uses clk 2 to reach aux_clk1. Now, let's say kernel updates indices to: 1 - mpu_dpll 2 - per_dpll 3 - aux_clk1 4 - core_clk using the old dtb(or dts missing an update), on new kernel which has updated indices will result in per_dpll now controlled for peripheral X without warning or any potential error detection. Even though we could claim this is user error, such errors are hard to track down and fix. The error in case (b) is that you shouldn't be changing the DT bindings after they've first been created. That would avoid the problem situation. The person using the old DTB with the new kernel didn't commit user error. That is what we'd like folks to follow, but when code is in development, or productization, there is no saying the type of mistakes people end up doing. An alternate approach introduced here is to introduce device tree bindings corresponding to the clock nodes required in DT definition for SoC which automatically maps back to the definitions in cclockXYZ_data.c. Well, I haven't read the patches, but isn't that exactly what the 2 is in clk 2? partly so. yes: it indexes back to the right clock node - at that point the comparison stops. we just point back on naming as needed and introduce nodes purely for the ones that we need. Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
On 10:22-20130413, Tony Lindgren wrote:
* Nishanth Menon n...@ti.com [130412 16:43]:
Thanks for checking up. Fixed all of them below, will post part of
series again, only if I need to address further comments in other
patches..
Thanks it seems that the other ones are ready to go, just one
more comment below.
[...]
+static struct clk *omap_clk_src_get(struct of_phandle_args *clkspec, void
*data)
+{
+ struct clk *clk;
+ char clk_name[32];
+ struct device_node *np = clkspec-np;
+
+ snprintf(clk_name, 32, %s_ck, np-name);
+ clk = clk_get(NULL, clk_name);
+ if (IS_ERR(clk)) {
+ pr_err(%s: could not get clock %s(%ld)\n, __func__,
+ clk_name, PTR_ERR(clk));
+ goto out;
+ }
+ clk_put(clk);
It seems that clk_put() is actually wrong here. That's because
of_clk_get() should boild down to just the look up of the clock
and then clk_get() on it, so no double clk_get() is done in this
case. Once the consumer driver is done, it will just call clk_put()
on it.
Yep - updated version below.
From d0bf3fce235cff46feac7f5ef1a40e2fa0f2aa12 Mon Sep 17 00:00:00 2001
From: Nishanth Menon n...@ti.com
Date: Tue, 9 Apr 2013 19:26:40 -0500
Subject: [PATCH V5.1 1/6] clk: OMAP: introduce device tree binding to kernel
clock
data
OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
However, this presents an obstacle for using these clock nodes in
Device Tree definitions. This is especially true for board specific
clocks initially. The fixed clocks are currently found via clock
aliases table. There are many possible approaches to this problem as
discussed in the following thread:
http://marc.info/?t=13637032569r=1w=2.
Highlights of the options:
a) device specific clk_add_alias:
cons: driver handling required
b) using an generic clk node and indexing to reach the clock required.
This is similar in approach taken by tegra and few other platforms.
Example usage: clock = clk 5;
cons: potential to have mismatches in indexed table and associated
dtb data. In addition, managing continued documentation in bindings
as clock indexing increases. Even though readability angle could be
improved by using preprocessing of DT using macros, indexed
approach is inherently risky from cases like the following:
clk indexes in kernel:
1 - mpu_dpll
2 - aux_clk1
3 - core_clk
DT entry for peripheral X uses clk 2 to reach aux_clk1. Now, let's
say kernel updates indices to:
1 - mpu_dpll
2 - per_dpll
3 - aux_clk1
4 - core_clk
using the old dtb(or dts missing an update), on new kernel which
has updated indices will result in per_dpll now controlled for
peripheral X without warning or any potential error detection.
Even though we could claim this is user error, such errors are hard
to track down and fix.
An alternate approach introduced here is to introduce device tree
bindings corresponding to the clock nodes required in DT definition
for SoC which automatically maps back to the definitions in
cclockXYZ_data.c.
The driver introduced here to do this mapping will eventually be the
place where the clock handling will migrate to. We need to consider
this angle as well so that the solution will be an valid transition
point for moving the clock data out of kernel image (into device tree
or firmware load etc..).
Overall strategy introduced here is simple: a clock node described in
device tree blob is used to identify the exact clock provided in the
SoC specific data. This is then linked back using of_clk_add_provider
to the device node to be accessible by of_clk_get.
Based on discussion contributions from Roger Quadros, Grygorii Strashko
and others.
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Mike Turquette mturque...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
[t...@atomide.com: co-developed]
Signed-off-by: Tony Lindgren t...@atomide.com
Signed-off-by: Nishanth Menon n...@ti.com
---
.../devicetree/bindings/clock/omap-clock.txt | 40 +
drivers/clk/Makefile |1 +
drivers/clk/omap/Makefile |1 +
drivers/clk/omap/clk.c | 91
4 files changed, 133 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
create mode 100644 drivers/clk/omap/Makefile
create mode 100644 drivers/clk/omap/clk.c
diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt
b/Documentation/devicetree/bindings/clock/omap-clock.txt
new file mode 100644
index 000..047c1e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
@@ -0,0 +1,40 @@
+Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
+
+This binding is an initial minimal binding that may be enhanced as part of
+transitioning OMAP clock data out of kernel image.
+
+This binding uses the common clock binding[1].
+
+[1]
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
* Nishanth Menon n...@ti.com [130412 16:43]:
Thanks for checking up. Fixed all of them below, will post part of
series again, only if I need to address further comments in other
patches..
Thanks it seems that the other ones are ready to go, just one
more comment below.
--- /dev/null
+++ b/drivers/clk/omap/clk.c
@@ -0,0 +1,95 @@
+/*
+ * Texas Instruments OMAP Clock driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon n...@ti.com
+ * Tony Lindgren t...@atomide.com
+ *
+ * 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/clkdev.h
+#include linux/clk-provider.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/of_device.h
+#include linux/platform_device.h
+#include linux/string.h
+
+static const struct of_device_id omap_clk_of_match[] = {
+ {.compatible = ti,omap-clock,},
+ {},
+};
+
+/**
+ * omap_clk_src_get() - Get OMAP clock from node name when needed
+ * @clkspec: clkspec argument
+ * @data:unused
+ *
+ * REVISIT: We assume the following:
+ * 1. omap clock names end with _ck
+ * 2. omap clock names are under 32 characters in length
+ */
+static struct clk *omap_clk_src_get(struct of_phandle_args *clkspec, void
*data)
+{
+ struct clk *clk;
+ char clk_name[32];
+ struct device_node *np = clkspec-np;
+
+ snprintf(clk_name, 32, %s_ck, np-name);
+ clk = clk_get(NULL, clk_name);
+ if (IS_ERR(clk)) {
+ pr_err(%s: could not get clock %s(%ld)\n, __func__,
+clk_name, PTR_ERR(clk));
+ goto out;
+ }
+ clk_put(clk);
It seems that clk_put() is actually wrong here. That's because
of_clk_get() should boild down to just the look up of the clock
and then clk_get() on it, so no double clk_get() is done in this
case. Once the consumer driver is done, it will just call clk_put()
on it.
+out:
+ return clk;
+}
+
+/**
+ * omap_clk_probe() - create link from DT definition to clock data
+ * @pdev:device node
+ *
+ * NOTE: we look up the clock lazily when the consumer driver does
+ * of_clk_get() and initialize a NULL clock here.
+ */
+static int omap_clk_probe(struct platform_device *pdev)
+{
+ int res;
+ struct device_node *np = pdev-dev.of_node;
+
+ /* This allows the driver to of_clk_get() */
+ res = of_clk_add_provider(np, omap_clk_src_get, NULL);
+ if (res)
+ dev_err(pdev-dev, could not add provider(%d)\n, res);
+
+ return res;
+}
+
+static struct platform_driver omap_clk_driver = {
+ .probe = omap_clk_probe,
+ .driver = {
+.name = omap_clk,
+.of_match_table = of_match_ptr(omap_clk_of_match),
+},
+};
+
+static int __init omap_clk_init(void)
+{
+ return platform_driver_register(omap_clk_driver);
+}
+arch_initcall(omap_clk_init);
+
+MODULE_DESCRIPTION(OMAP Clock driver);
+MODULE_AUTHOR(Texas Instruments Inc.);
+MODULE_LICENSE(GPL v2);
Other than that looks OK to me.
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
* Nishanth Menon n...@ti.com [130412 15:59]:
--- /dev/null
+++ b/drivers/clk/omap/clk.c
+/**
+ * omap_clk_src_get() - Get OMAP clock from node name when needed
+ * @clkspec: clkspec argument
+ * @data:unused
+ *
+ * REVISIT: We assume the following:
+ * 1. omap clock names end with _ck
+ * 2. omap clock names are under 32 characters in length
+ */
+static struct clk *omap_clk_src_get(struct of_phandle_args *clkspec, void
*data)
+{
+ struct clk *clk;
+ char clk_name[32];
+ struct device_node *np = clkspec-np;
+
+ /* Set up things so consumer can call clk_get() with name */
I would leave out the comment above, it's a leftover from
the clk_add_alias() version that we don't need because of
of_clk_get().
+ snprintf(clk_name, 32, %s_ck, np-name);
+ clk = clk_get(NULL, clk_name);
+ if (IS_ERR(clk)) {
+ pr_err(%s: could not get clock %s(%ld)\n, __func__,
+clk_name, PTR_ERR(clk));
+ goto out;
+ }
+ clk_put(clk);
+
+out:
+ return clk;
+}
+
+/**
+ * omap_clk_probe() - create link from DT definition to clock data
+ * @pdev:device node
+ *
+ * NOTE: We assume that omap clocks are not removed.
+ */
How about drop the comment on clocks being removed above.
It no longer an issue, so maybe something like this instead:
* Note that we look up the clock lazily when the consumer
* driver does of_clk_get() and initialize a NULL clock here.
+static int omap_clk_probe(struct platform_device *pdev)
+{
+ int res;
+ struct device_node *np = pdev-dev.of_node;
+
+ /* This allows the driver to of_clk_get() */
+ res = of_clk_add_provider(np, omap_clk_src_get, NULL);
+ if (res)
+ dev_err(pdev-dev, could not add provider(%d)\n, res);
+
+ return res;
+}
+
+/* We assume here that OMAP clocks will not be removed */
Then the above comment can be removed too.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/6] clk: OMAP: introduce device tree binding to kernel clock data
On 16:31-20130412, Tony Lindgren wrote:
* Nishanth Menon n...@ti.com [130412 15:59]:
--- /dev/null
+++ b/drivers/clk/omap/clk.c
+/**
+ * omap_clk_src_get() - Get OMAP clock from node name when needed
+ * @clkspec: clkspec argument
+ * @data: unused
+ *
+ * REVISIT: We assume the following:
+ * 1. omap clock names end with _ck
+ * 2. omap clock names are under 32 characters in length
+ */
+static struct clk *omap_clk_src_get(struct of_phandle_args *clkspec, void
*data)
+{
+ struct clk *clk;
+ char clk_name[32];
+ struct device_node *np = clkspec-np;
+
+ /* Set up things so consumer can call clk_get() with name */
I would leave out the comment above, it's a leftover from
the clk_add_alias() version that we don't need because of
of_clk_get().
+ snprintf(clk_name, 32, %s_ck, np-name);
+ clk = clk_get(NULL, clk_name);
+ if (IS_ERR(clk)) {
+ pr_err(%s: could not get clock %s(%ld)\n, __func__,
+ clk_name, PTR_ERR(clk));
+ goto out;
+ }
+ clk_put(clk);
+
+out:
+ return clk;
+}
+
+/**
+ * omap_clk_probe() - create link from DT definition to clock data
+ * @pdev: device node
+ *
+ * NOTE: We assume that omap clocks are not removed.
+ */
How about drop the comment on clocks being removed above.
It no longer an issue, so maybe something like this instead:
* Note that we look up the clock lazily when the consumer
* driver does of_clk_get() and initialize a NULL clock here.
+static int omap_clk_probe(struct platform_device *pdev)
+{
+ int res;
+ struct device_node *np = pdev-dev.of_node;
+
+ /* This allows the driver to of_clk_get() */
+ res = of_clk_add_provider(np, omap_clk_src_get, NULL);
+ if (res)
+ dev_err(pdev-dev, could not add provider(%d)\n, res);
+
+ return res;
+}
+
+/* We assume here that OMAP clocks will not be removed */
Then the above comment can be removed too.
Thanks for checking up. Fixed all of them below, will post part of
series again, only if I need to address further comments in other
patches..
From f96c04860794f9bbfe240a8661641a7c90dd1640 Mon Sep 17 00:00:00 2001
From: Nishanth Menon n...@ti.com
Date: Tue, 9 Apr 2013 19:26:40 -0500
Subject: [PATCH V5 1/6] clk: OMAP: introduce device tree binding to kernel clock
data
OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
However, this presents an obstacle for using these clock nodes in
Device Tree definitions. This is especially true for board specific
clocks initially. The fixed clocks are currently found via clock
aliases table. There are many possible approaches to this problem as
discussed in the following thread:
http://marc.info/?t=13637032569r=1w=2.
Highlights of the options:
a) device specific clk_add_alias:
cons: driver handling required
b) using an generic clk node and indexing to reach the clock required.
This is similar in approach taken by tegra and few other platforms.
Example usage: clock = clk 5;
cons: potential to have mismatches in indexed table and associated
dtb data. In addition, managing continued documentation in bindings
as clock indexing increases. Even though readability angle could be
improved by using preprocessing of DT using macros, indexed
approach is inherently risky from cases like the following:
clk indexes in kernel:
1 - mpu_dpll
2 - aux_clk1
3 - core_clk
DT entry for peripheral X uses clk 2 to reach aux_clk1. Now, let's
say kernel updates indices to:
1 - mpu_dpll
2 - per_dpll
3 - aux_clk1
4 - core_clk
using the old dtb(or dts missing an update), on new kernel which
has updated indices will result in per_dpll now controlled for
peripheral X without warning or any potential error detection.
Even though we could claim this is user error, such errors are hard
to track down and fix.
An alternate approach introduced here is to introduce device tree
bindings corresponding to the clock nodes required in DT definition
for SoC which automatically maps back to the definitions in
cclockXYZ_data.c.
The driver introduced here to do this mapping will eventually be the
place where the clock handling will migrate to. We need to consider
this angle as well so that the solution will be an valid transition
point for moving the clock data out of kernel image (into device tree
or firmware load etc..).
Overall strategy introduced here is simple: a clock node described in
device tree blob is used to identify the exact clock provided in the
SoC specific data. This is then linked back using of_clk_add_provider
to the device node to be accessible by of_clk_get.
Based on discussion contributions from Roger Quadros, Grygorii Strashko
and others.
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Mike Turquette mturque...@linaro.org
Cc: Paul Walmsley p...@pwsan.com
[t...@atomide.com: co-developed]
Signed-off-by: Tony Lindgren
