linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND][PATCH] clk: fixed-factor: set CLK_SET_RATE_PARENT
@ 2016-06-13  9:08 Jongsung Kim
  2016-06-21  0:59 ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Jongsung Kim @ 2016-06-13  9:08 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Chanho Min, Jongsung Kim

Without CLK_SET_RATE_PARENT-flag set, clk_factor_round_rate() just
returns the current frequency. A fixed-factor-clock initialzed via
of_fixed_factor_clk_set() (ie, by device-tree) can't have the flag
set. It can be problematic when the parent of a fixed-factor-clock
is rate-controllable clk, because the rounding can't be propagated
to parent, the rounded target frequency is always the current, and
finally the frequency can't be changed.

This patch sets the flags CLK_SET_RATE_PARENT for all fixed-factor-
clocks, from clk_register_fixed_factor(), and removes checking the
flag from clk_factor_round_rate().

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
---
 drivers/clk/clk-fixed-factor.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 75cd6c7..9568306 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -38,13 +38,10 @@ static long clk_factor_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct clk_fixed_factor *fix = to_clk_fixed_factor(hw);
+	unsigned long best_parent;
 
-	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-		unsigned long best_parent;
-
-		best_parent = (rate / fix->mult) * fix->div;
-		*prate = clk_hw_round_rate(clk_hw_get_parent(hw), best_parent);
-	}
+	best_parent = (rate / fix->mult) * fix->div;
+	*prate = clk_hw_round_rate(clk_hw_get_parent(hw), best_parent);
 
 	return (*prate / fix->div) * fix->mult;
 }
@@ -88,7 +85,7 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 
 	init.name = name;
 	init.ops = &clk_fixed_factor_ops;
-	init.flags = flags | CLK_IS_BASIC;
+	init.flags = flags | CLK_IS_BASIC | CLK_SET_RATE_PARENT;
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RESEND][PATCH] clk: fixed-factor: set CLK_SET_RATE_PARENT
  2016-06-13  9:08 [RESEND][PATCH] clk: fixed-factor: set CLK_SET_RATE_PARENT Jongsung Kim
@ 2016-06-21  0:59 ` Stephen Boyd
  2016-06-24  3:49   ` [PATCH] clk: fixed-factor: add optional dt-binding clock-flags Jongsung Kim
  2016-06-24  4:12   ` [PATCH v2] " Jongsung Kim
  0 siblings, 2 replies; 13+ messages in thread
From: Stephen Boyd @ 2016-06-21  0:59 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: Michael Turquette, linux-clk, linux-kernel, Chanho Min, Maxime Ripard

On 06/13, Jongsung Kim wrote:
> Without CLK_SET_RATE_PARENT-flag set, clk_factor_round_rate() just
> returns the current frequency. A fixed-factor-clock initialzed via
> of_fixed_factor_clk_set() (ie, by device-tree) can't have the flag
> set. It can be problematic when the parent of a fixed-factor-clock
> is rate-controllable clk, because the rounding can't be propagated
> to parent, the rounded target frequency is always the current, and
> finally the frequency can't be changed.
> 
> This patch sets the flags CLK_SET_RATE_PARENT for all fixed-factor-
> clocks, from clk_register_fixed_factor(), and removes checking the
> flag from clk_factor_round_rate().
> 
> Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
> ---

Hmm there's another patch for the same problem from Maxime[1]

Mike replied there and said that it's worrisome to do this for
all users of fixed factors clks, so perhaps you can set this flag
when you register the clk instead? Or prove that all the users of
this code in mainline are ok to change behavior.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/429726.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-21  0:59 ` Stephen Boyd
@ 2016-06-24  3:49   ` Jongsung Kim
  2016-06-24  5:11     ` kbuild test robot
  2016-06-24  5:57     ` kbuild test robot
  2016-06-24  4:12   ` [PATCH v2] " Jongsung Kim
  1 sibling, 2 replies; 13+ messages in thread
From: Jongsung Kim @ 2016-06-24  3:49 UTC (permalink / raw)
  To: Maxime Ripard, Mike Turquette, Stephen Boyd
  Cc: linux-clk, devicetree, linux-kernel, Chanho Min, Jongsung Kim

There is no way to set additional flags for a DT-initialized fixed-
factor-clock, and it can be problematic i.e., when the clock rate
needs to be changed. [1][2]

This patch introduces an optional dt-binding named "clock-flags" to
be used for passing any needed flags from dts.

[1] http://www.spinics.net/lists/linux-clk/msg09040.html
[2] https://lkml.org/lkml/2016/6/20/1025

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Mike Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
---
 .../bindings/clock/fixed-factor-clock.txt          |  4 ++++
 drivers/clk/clk-fixed-factor.c                     |  4 +++-
 include/dt-bindings/clk/clk.h                      | 22 ++++++++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/clk/clk.h

diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt b/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
index 1bae8527..3e1b79e 100644
--- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
+++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
@@ -13,12 +13,16 @@ Required properties:
 
 Optional properties:
 - clock-output-names : From common clock binding.
+- clock-flags : Additional flags to be used.
 
 Example:
+	#include <dt-bindings/clk/clk.h>
+
 	clock {
 		compatible = "fixed-factor-clock";
 		clocks = <&parentclk>;
 		#clock-cells = <0>;
 		clock-div = <2>;
 		clock-mult = <1>;
+		clock-flags = <CLK_SET_RATE_PARENT>;
 	};
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 75cd6c7..da3cd9c 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -150,6 +150,7 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 	struct clk *clk;
 	const char *clk_name = node->name;
 	const char *parent_name;
+	unsigned long flags = 0;
 	u32 div, mult;
 
 	if (of_property_read_u32(node, "clock-div", &div)) {
@@ -166,8 +167,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
 	parent_name = of_clk_get_parent_name(node, 0);
+	of_property_read_u32(node, "clock-flags", &flags);
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
+	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
 					mult, div);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
diff --git a/include/dt-bindings/clk/clk.h b/include/dt-bindings/clk/clk.h
new file mode 100644
index 0000000..1834933
--- /dev/null
+++ b/include/dt-bindings/clk/clk.h
@@ -0,0 +1,22 @@
+/*
+ * See include/linux/clk-provider.h for more information.
+ */
+
+#ifndef __DT_BINDINGS_CLK_CLK_H
+#define __DT_BINDINGS_CLK_CLK_H
+
+#define BIT(nr)	(1UL << (nr))
+
+#define CLK_SET_RATE_GATE		BIT(0)
+#define CLK_SET_PARENT_GATE		BIT(1)
+#define CLK_SET_RATE_PARENT		BIT(2)
+#define CLK_IGNORE_UNUSED		BIT(3)
+#define CLK_IS_BASIC			BIT(5)
+#define CLK_GET_RATE_NOCACHE		BIT(6)
+#define CLK_SET_RATE_NO_REPARENT	BIT(7)
+#define CLK_GET_ACCURACY_NOCACHE	BIT(8)
+#define CLK_RECALC_NEW_RATES		BIT(9)
+#define CLK_SET_RATE_UNGATE		BIT(10)
+#define CLK_IS_CRITICAL			BIT(11)
+
+#endif
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-21  0:59 ` Stephen Boyd
  2016-06-24  3:49   ` [PATCH] clk: fixed-factor: add optional dt-binding clock-flags Jongsung Kim
@ 2016-06-24  4:12   ` Jongsung Kim
  2016-06-28 20:55     ` Rob Herring
  1 sibling, 1 reply; 13+ messages in thread
From: Jongsung Kim @ 2016-06-24  4:12 UTC (permalink / raw)
  To: Maxime Ripard, Mike Turquette, Stephen Boyd
  Cc: linux-clk, devicetree, linux-kernel, Chanho Min, Jongsung Kim

There is no way to set additional flags for a DT-initialized fixed-
factor-clock, and it can be problematic i.e., when the clock rate
needs to be changed. [1][2]

This patch introduces an optional dt-binding named "clock-flags" to
be used for passing any needed flags from dts.

[1] http://www.spinics.net/lists/linux-clk/msg09040.html
[2] https://lkml.org/lkml/2016/6/20/1025

Changes since v1:
 - fix possible build failure when using gcc-5 or gcc-6

Signed-off-by: Jongsung Kim <neidhard.kim@lge.com>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Mike Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
---
 .../bindings/clock/fixed-factor-clock.txt          |  4 ++++
 drivers/clk/clk-fixed-factor.c                     |  4 +++-
 include/dt-bindings/clk/clk.h                      | 22 ++++++++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/clk/clk.h

diff --git a/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt b/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
index 1bae8527..3e1b79e 100644
--- a/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
+++ b/Documentation/devicetree/bindings/clock/fixed-factor-clock.txt
@@ -13,12 +13,16 @@ Required properties:
 
 Optional properties:
 - clock-output-names : From common clock binding.
+- clock-flags : Additional flags to be used.
 
 Example:
+	#include <dt-bindings/clk/clk.h>
+
 	clock {
 		compatible = "fixed-factor-clock";
 		clocks = <&parentclk>;
 		#clock-cells = <0>;
 		clock-div = <2>;
 		clock-mult = <1>;
+		clock-flags = <CLK_SET_RATE_PARENT>;
 	};
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 75cd6c7..e626cad 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -150,6 +150,7 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 	struct clk *clk;
 	const char *clk_name = node->name;
 	const char *parent_name;
+	u32 flags = 0;
 	u32 div, mult;
 
 	if (of_property_read_u32(node, "clock-div", &div)) {
@@ -166,8 +167,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
 	parent_name = of_clk_get_parent_name(node, 0);
+	of_property_read_u32(node, "clock-flags", &flags);
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
+	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
 					mult, div);
 	if (!IS_ERR(clk))
 		of_clk_add_provider(node, of_clk_src_simple_get, clk);
diff --git a/include/dt-bindings/clk/clk.h b/include/dt-bindings/clk/clk.h
new file mode 100644
index 0000000..1834933
--- /dev/null
+++ b/include/dt-bindings/clk/clk.h
@@ -0,0 +1,22 @@
+/*
+ * See include/linux/clk-provider.h for more information.
+ */
+
+#ifndef __DT_BINDINGS_CLK_CLK_H
+#define __DT_BINDINGS_CLK_CLK_H
+
+#define BIT(nr)	(1UL << (nr))
+
+#define CLK_SET_RATE_GATE		BIT(0)
+#define CLK_SET_PARENT_GATE		BIT(1)
+#define CLK_SET_RATE_PARENT		BIT(2)
+#define CLK_IGNORE_UNUSED		BIT(3)
+#define CLK_IS_BASIC			BIT(5)
+#define CLK_GET_RATE_NOCACHE		BIT(6)
+#define CLK_SET_RATE_NO_REPARENT	BIT(7)
+#define CLK_GET_ACCURACY_NOCACHE	BIT(8)
+#define CLK_RECALC_NEW_RATES		BIT(9)
+#define CLK_SET_RATE_UNGATE		BIT(10)
+#define CLK_IS_CRITICAL			BIT(11)
+
+#endif
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-24  3:49   ` [PATCH] clk: fixed-factor: add optional dt-binding clock-flags Jongsung Kim
@ 2016-06-24  5:11     ` kbuild test robot
  2016-06-24  5:57     ` kbuild test robot
  1 sibling, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2016-06-24  5:11 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: kbuild-all, Maxime Ripard, Mike Turquette, Stephen Boyd,
	linux-clk, devicetree, linux-kernel, Chanho Min, Jongsung Kim

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

Hi,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.7-rc4 next-20160623]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jongsung-Kim/clk-fixed-factor-add-optional-dt-binding-clock-flags/20160624-115201
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: x86_64-randconfig-s4-06241247 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/clk/clk-fixed-factor.c: In function 'of_fixed_factor_clk_setup':
>> drivers/clk/clk-fixed-factor.c:170:44: error: passing argument 3 of 'of_property_read_u32' from incompatible pointer type [-Werror=incompatible-pointer-types]
     of_property_read_u32(node, "clock-flags", &flags);
                                               ^
   In file included from include/linux/clk-provider.h:15:0,
                    from drivers/clk/clk-fixed-factor.c:11:
   include/linux/of.h:916:19: note: expected 'u32 * {aka unsigned int *}' but argument is of type 'long unsigned int *'
    static inline int of_property_read_u32(const struct device_node *np,
                      ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/of_property_read_u32 +170 drivers/clk/clk-fixed-factor.c

   164				__func__, node->name);
   165			return;
   166		}
   167	
   168		of_property_read_string(node, "clock-output-names", &clk_name);
   169		parent_name = of_clk_get_parent_name(node, 0);
 > 170		of_property_read_u32(node, "clock-flags", &flags);
   171	
   172		clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
   173						mult, div);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26890 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-24  3:49   ` [PATCH] clk: fixed-factor: add optional dt-binding clock-flags Jongsung Kim
  2016-06-24  5:11     ` kbuild test robot
@ 2016-06-24  5:57     ` kbuild test robot
  1 sibling, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2016-06-24  5:57 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: kbuild-all, Maxime Ripard, Mike Turquette, Stephen Boyd,
	linux-clk, devicetree, linux-kernel, Chanho Min, Jongsung Kim

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

Hi,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.7-rc4 next-20160623]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jongsung-Kim/clk-fixed-factor-add-optional-dt-binding-clock-flags/20160624-115201
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: microblaze-mmu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=microblaze 

All warnings (new ones prefixed by >>):

   drivers/clk/clk-fixed-factor.c: In function 'of_fixed_factor_clk_setup':
>> drivers/clk/clk-fixed-factor.c:170:2: warning: passing argument 3 of 'of_property_read_u32' from incompatible pointer type
     of_property_read_u32(node, "clock-flags", &flags);
     ^
   In file included from include/linux/clk-provider.h:15:0,
                    from drivers/clk/clk-fixed-factor.c:11:
   include/linux/of.h:916:19: note: expected 'u32 *' but argument is of type 'long unsigned int *'
    static inline int of_property_read_u32(const struct device_node *np,
                      ^

vim +/of_property_read_u32 +170 drivers/clk/clk-fixed-factor.c

   154		u32 div, mult;
   155	
   156		if (of_property_read_u32(node, "clock-div", &div)) {
   157			pr_err("%s Fixed factor clock <%s> must have a clock-div property\n",
   158				__func__, node->name);
   159			return;
   160		}
   161	
   162		if (of_property_read_u32(node, "clock-mult", &mult)) {
   163			pr_err("%s Fixed factor clock <%s> must have a clock-mult property\n",
   164				__func__, node->name);
   165			return;
   166		}
   167	
   168		of_property_read_string(node, "clock-output-names", &clk_name);
   169		parent_name = of_clk_get_parent_name(node, 0);
 > 170		of_property_read_u32(node, "clock-flags", &flags);
   171	
   172		clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
   173						mult, div);
   174		if (!IS_ERR(clk))
   175			of_clk_add_provider(node, of_clk_src_simple_get, clk);
   176	}
   177	EXPORT_SYMBOL_GPL(of_fixed_factor_clk_setup);
   178	CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 12638 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-24  4:12   ` [PATCH v2] " Jongsung Kim
@ 2016-06-28 20:55     ` Rob Herring
  2016-06-28 21:18       ` Michael Turquette
  2016-06-29  5:04       ` Jongsung Kim
  0 siblings, 2 replies; 13+ messages in thread
From: Rob Herring @ 2016-06-28 20:55 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: Maxime Ripard, Mike Turquette, Stephen Boyd, linux-clk,
	devicetree, linux-kernel, Chanho Min

On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
> There is no way to set additional flags for a DT-initialized fixed-
> factor-clock, and it can be problematic i.e., when the clock rate
> needs to be changed. [1][2]
> 
> This patch introduces an optional dt-binding named "clock-flags" to
> be used for passing any needed flags from dts.

I don't think we want this in DT. If we did, the flags would need some 
documentation about what the flags mean.

Rob

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-28 20:55     ` Rob Herring
@ 2016-06-28 21:18       ` Michael Turquette
  2016-06-29  7:06         ` Jongsung Kim
  2016-06-29  5:04       ` Jongsung Kim
  1 sibling, 1 reply; 13+ messages in thread
From: Michael Turquette @ 2016-06-28 21:18 UTC (permalink / raw)
  To: Rob Herring, Jongsung Kim
  Cc: Maxime Ripard, Stephen Boyd, linux-clk, devicetree, linux-kernel,
	Chanho Min

Quoting Rob Herring (2016-06-28 13:55:18)
> On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
> > There is no way to set additional flags for a DT-initialized fixed-
> > factor-clock, and it can be problematic i.e., when the clock rate
> > needs to be changed. [1][2]
> > 
> > This patch introduces an optional dt-binding named "clock-flags" to
> > be used for passing any needed flags from dts.
> 
> I don't think we want this in DT. If we did, the flags would need some 
> documentation about what the flags mean.

Flags are specific to Linux implementation, so I agree with Rob. Better
to create a compatible string for your hardware that bakes in the flags.

Regards,
Mike

> 
> Rob

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-28 20:55     ` Rob Herring
  2016-06-28 21:18       ` Michael Turquette
@ 2016-06-29  5:04       ` Jongsung Kim
  1 sibling, 0 replies; 13+ messages in thread
From: Jongsung Kim @ 2016-06-29  5:04 UTC (permalink / raw)
  To: Rob Herring
  Cc: Maxime Ripard, Mike Turquette, Stephen Boyd, linux-clk,
	devicetree, linux-kernel, Chanho Min

On 2016년 06월 29일 05:55, Rob Herring wrote:
> On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
>> There is no way to set additional flags for a DT-initialized fixed-
>> factor-clock, and it can be problematic i.e., when the clock rate
>> needs to be changed. [1][2]
>>
>> This patch introduces an optional dt-binding named "clock-flags" to
>> be used for passing any needed flags from dts.
> I don't think we want this in DT. If we did, the flags would need some 
> documentation about what the flags mean.
Thanks for your comment. It looks not that big deal to provide a little documentation..? Some of the flags looks safe to be removed. Thinking of only fixed-factor-clock, most of them can be removed.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-28 21:18       ` Michael Turquette
@ 2016-06-29  7:06         ` Jongsung Kim
  2016-07-02  0:20           ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Jongsung Kim @ 2016-06-29  7:06 UTC (permalink / raw)
  To: Michael Turquette, Rob Herring
  Cc: Maxime Ripard, Stephen Boyd, linux-clk, devicetree, linux-kernel,
	Chanho Min

On 2016년 06월 29일 06:18, Michael Turquette wrote:
> Quoting Rob Herring (2016-06-28 13:55:18)
>> On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
>>> There is no way to set additional flags for a DT-initialized fixed-
>>> factor-clock, and it can be problematic i.e., when the clock rate
>>> needs to be changed. [1][2]
>>>
>>> This patch introduces an optional dt-binding named "clock-flags" to
>>> be used for passing any needed flags from dts.
>> I don't think we want this in DT. If we did, the flags would need some 
>> documentation about what the flags mean.
> Flags are specific to Linux implementation, so I agree with Rob. Better
> to create a compatible string for your hardware that bakes in the flags.

Thank you for your comment, Mike. This conversation starts from lacking method to set CLK_SET_RATE_PARENT from DT. I understand compatible string can be a solution. But.. if someone starts talking about lacking method to set another flag, i.e., CLK_SET_PARENT_GATE, then we'll need another compatible string list.
How do you think about defining possible required subset of the flags and using some more neutral flag-names acceptable in DT?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-06-29  7:06         ` Jongsung Kim
@ 2016-07-02  0:20           ` Stephen Boyd
  2016-07-04  1:48             ` Jongsung Kim
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2016-07-02  0:20 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: Michael Turquette, Rob Herring, Maxime Ripard, linux-clk,
	devicetree, linux-kernel, Chanho Min

On 06/29, Jongsung Kim wrote:
> On 2016년 06월 29일 06:18, Michael Turquette wrote:
> > Quoting Rob Herring (2016-06-28 13:55:18)
> >> On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
> >>> There is no way to set additional flags for a DT-initialized fixed-
> >>> factor-clock, and it can be problematic i.e., when the clock rate
> >>> needs to be changed. [1][2]
> >>>
> >>> This patch introduces an optional dt-binding named "clock-flags" to
> >>> be used for passing any needed flags from dts.
> >> I don't think we want this in DT. If we did, the flags would need some 
> >> documentation about what the flags mean.
> > Flags are specific to Linux implementation, so I agree with Rob. Better
> > to create a compatible string for your hardware that bakes in the flags.
> 
> Thank you for your comment, Mike. This conversation starts from lacking method to set CLK_SET_RATE_PARENT from DT. I understand compatible string can be a solution. But.. if someone starts talking about lacking method to set another flag, i.e., CLK_SET_PARENT_GATE, then we'll need another compatible string list.
> How do you think about defining possible required subset of the flags and using some more neutral flag-names acceptable in DT?

Do you actually have an IC on the board that is doing some fixed
factor calculation? Or is this a clk driver design where we are
listing out each piece of an SoC's clk controller in DT?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-07-02  0:20           ` Stephen Boyd
@ 2016-07-04  1:48             ` Jongsung Kim
  2016-08-25  0:52               ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Jongsung Kim @ 2016-07-04  1:48 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, Rob Herring, Maxime Ripard, linux-clk,
	devicetree, linux-kernel, Chanho Min

On 2016년 07월 02일 09:20, Stephen Boyd wrote:
> On 06/29, Jongsung Kim wrote:
>> On 2016년 06월 29일 06:18, Michael Turquette wrote:
>>> Quoting Rob Herring (2016-06-28 13:55:18)
>>>> On Fri, Jun 24, 2016 at 01:12:52PM +0900, Jongsung Kim wrote:
>>>>> There is no way to set additional flags for a DT-initialized fixed-
>>>>> factor-clock, and it can be problematic i.e., when the clock rate
>>>>> needs to be changed. [1][2]
>>>>>
>>>>> This patch introduces an optional dt-binding named "clock-flags" to
>>>>> be used for passing any needed flags from dts.
>>>> I don't think we want this in DT. If we did, the flags would need some 
>>>> documentation about what the flags mean.
>>> Flags are specific to Linux implementation, so I agree with Rob. Better
>>> to create a compatible string for your hardware that bakes in the flags.
>> Thank you for your comment, Mike. This conversation starts from lacking method to set CLK_SET_RATE_PARENT from DT. I understand compatible string can be a solution. But.. if someone starts talking about lacking method to set another flag, i.e., CLK_SET_PARENT_GATE, then we'll need another compatible string list.
>> How do you think about defining possible required subset of the flags and using some more neutral flag-names acceptable in DT?
> Do you actually have an IC on the board that is doing some fixed
> factor calculation? Or is this a clk driver design where we are
> listing out each piece of an SoC's clk controller in DT?
>
The SoC has several PLLs of identical design, and one of them is divided
to half and used for CPUs. The fixed-factor-clock represents the divider.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] clk: fixed-factor: add optional dt-binding clock-flags
  2016-07-04  1:48             ` Jongsung Kim
@ 2016-08-25  0:52               ` Stephen Boyd
  0 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2016-08-25  0:52 UTC (permalink / raw)
  To: Jongsung Kim
  Cc: Michael Turquette, Rob Herring, Maxime Ripard, linux-clk,
	devicetree, linux-kernel, Chanho Min

On 07/04, Jongsung Kim wrote:
> On 2016년 07월 02일 09:20, Stephen Boyd wrote:
> > Do you actually have an IC on the board that is doing some fixed
> > factor calculation? Or is this a clk driver design where we are
> > listing out each piece of an SoC's clk controller in DT?
> >
> The SoC has several PLLs of identical design, and one of them is divided
> to half and used for CPUs. The fixed-factor-clock represents the divider.
> 

Ok, so it sounds like we can have the driver that registers the
CPU PLL also register the fixed factor clk? I fail to see why we
need this from DT in that case.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-08-25  5:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13  9:08 [RESEND][PATCH] clk: fixed-factor: set CLK_SET_RATE_PARENT Jongsung Kim
2016-06-21  0:59 ` Stephen Boyd
2016-06-24  3:49   ` [PATCH] clk: fixed-factor: add optional dt-binding clock-flags Jongsung Kim
2016-06-24  5:11     ` kbuild test robot
2016-06-24  5:57     ` kbuild test robot
2016-06-24  4:12   ` [PATCH v2] " Jongsung Kim
2016-06-28 20:55     ` Rob Herring
2016-06-28 21:18       ` Michael Turquette
2016-06-29  7:06         ` Jongsung Kim
2016-07-02  0:20           ` Stephen Boyd
2016-07-04  1:48             ` Jongsung Kim
2016-08-25  0:52               ` Stephen Boyd
2016-06-29  5:04       ` Jongsung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).