[PATCH 0/2] clk: shmobile: DIV6 clock variable parent support

[PATCH 0/2] clk: shmobile: DIV6 clock variable parent support

[Ulrich Hecht]

Hi!

Here's a new attempt at implementing variable-parent 6-bit divider (DIV6)
clocks, this time using .get_parent and .set_parent.  Also, the bindings now
actually explain how to specify the parent clocks and feature a relevant
example.

CU
Uli

Changes since v1:
- implement .get_parent, .set_parent
- improve bindings documentation
- split patch in two


Ulrich Hecht (2):
  clk: shmobile: div6: support selectable-input clocks
  clk: shmobile: document DIV6 clock parent bindings

 .../bindings/clock/renesas,cpg-div6-clocks.txt     |  28 +++--
 drivers/clk/shmobile/clk-div6.c                    | 117 ++++++++++++++++++---
 2 files changed, 127 insertions(+), 18 deletions(-)

-- 
1.8.4.5

[PATCH 0/2] clk: shmobile: DIV6 clock variable parent support

[Ulrich Hecht]

Hi!

Here's a new attempt at implementing variable-parent 6-bit divider (DIV6)
clocks, this time using .get_parent and .set_parent.  Also, the bindings now
actually explain how to specify the parent clocks and feature a relevant
example.

CU
Uli

Changes since v1:
- implement .get_parent, .set_parent
- improve bindings documentation
- split patch in two


Ulrich Hecht (2):
  clk: shmobile: div6: support selectable-input clocks
  clk: shmobile: document DIV6 clock parent bindings

 .../bindings/clock/renesas,cpg-div6-clocks.txt     |  28 +++--
 drivers/clk/shmobile/clk-div6.c                    | 117 ++++++++++++++++++---
 2 files changed, 127 insertions(+), 18 deletions(-)

-- 
1.8.4.5

[PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Ulrich Hecht]

Support for setting the parent at initialization time based on the current
hardware configuration in DIV6 clocks with selectable parents as found in
the r8a73a4, r8a7740, sh73a0, and other SoCs.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/clk/shmobile/clk-div6.c | 117 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 105 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c
index f065f69..ea44988 100644
--- a/drivers/clk/shmobile/clk-div6.c
+++ b/drivers/clk/shmobile/clk-div6.c
@@ -32,6 +32,9 @@ struct div6_clock {
 	struct clk_hw hw;
 	void __iomem *reg;
 	unsigned int div;
+	int src_shift;
+	int src_width;
+	u8 *parents;
 };
 
 #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)
@@ -39,8 +42,11 @@ struct div6_clock {
 static int cpg_div6_clock_enable(struct clk_hw *hw)
 {
 	struct div6_clock *clock = to_div6_clock(hw);
+	u32 val;
 
-	clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+	val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP))
+	    | CPG_DIV6_DIV(clock->div - 1);
+	clk_writel(val, clock->reg);
 
 	return 0;
 }
@@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw)
 	/* DIV6 clocks require the divisor field to be non-zero when stopping
 	 * the clock.
 	 */
-	clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK),
+	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
 		   clock->reg);
 }
 
@@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct div6_clock *clock = to_div6_clock(hw);
 	unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate);
+	u32 val;
 
 	clock->div = div;
 
+	val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK;
 	/* Only program the new divisor if the clock isn't stopped. */
-	if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP))
-		clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+	if (!(val & CPG_DIV6_CKSTP))
+		clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg);
+
+	return 0;
+}
+
+static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	u8 hw_index;
+	int i;
+
+	if (clock->src_width = 0)
+		return 0;
+
+	hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
+		   (BIT(clock->src_width) - 1);
+	for (i = 0; i < hw->init->num_parents; i++) {
+		if (clock->parents[i] = hw_index)
+			return i;
+	}
+
+	pr_err("%s: %s DIV6 clock set to invalid parent %d\n",
+	       __func__, hw->init->name, hw_index);
+	return 0;
+}
+
+static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	u8 hw_index;
+	u32 mask;
+
+	if (index >= hw->init->num_parents)
+		return -EINVAL;
+
+	mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
+	hw_index = clock->parents[index];
+
+	clk_writel((clk_readl(clock->reg) & mask) |
+		(hw_index << clock->src_shift), clock->reg);
 
 	return 0;
 }
@@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = {
 	.enable = cpg_div6_clock_enable,
 	.disable = cpg_div6_clock_disable,
 	.is_enabled = cpg_div6_clock_is_enabled,
+	.get_parent = cpg_div6_clock_get_parent,
+	.set_parent = cpg_div6_clock_set_parent,
 	.recalc_rate = cpg_div6_clock_recalc_rate,
 	.round_rate = cpg_div6_clock_round_rate,
 	.set_rate = cpg_div6_clock_set_rate,
@@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
 
 static void __init cpg_div6_clock_init(struct device_node *np)
 {
+	const char **parent_names;
 	struct clk_init_data init;
 	struct div6_clock *clock;
-	const char *parent_name;
 	const char *name;
 	struct clk *clk;
+	int valid_parents;
+	int num_parents;
+	u32 src_shift;
+	u32 src_width;
 	int ret;
+	int i;
 
 	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
 	if (!clock) {
@@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 		return;
 	}
 
+	num_parents = of_clk_get_parent_count(np);
+	if (num_parents < 1) {
+		pr_err("%s: no parent found for %s DIV6 clock\n",
+		       __func__, np->name);
+		return;
+	}
+
+	clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
+		GFP_KERNEL);
+	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
+				GFP_KERNEL);
+
+	if (!parent_names) {
+		pr_err("%s: failed to allocate %s parent name list\n",
+		       __func__, np->name);
+		return;
+	}
+
 	/* Remap the clock register and read the divisor. Disabling the
 	 * clock overwrites the divisor, so we need to cache its value for the
 	 * enable operation.
@@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 		goto error;
 	}
 
-	parent_name = of_clk_get_parent_name(np, 0);
-	if (parent_name = NULL) {
-		pr_err("%s: failed to get %s DIV6 clock parent name\n",
-		       __func__, np->name);
-		goto error;
+
+	for (i = 0, valid_parents = 0; i < num_parents; i++) {
+		const char *name = of_clk_get_parent_name(np, i);
+
+		if (name) {
+			parent_names[valid_parents] = name;
+			clock->parents[valid_parents] = i;
+			valid_parents++;
+		}
+	}
+
+	if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
+		if (!of_property_read_u32(np, "renesas,src-width",
+					&src_width)) {
+			clock->src_shift = src_shift;
+			clock->src_width = src_width;
+		} else {
+			pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
+			       __func__, np->name);
+			goto error;
+		}
+	} else {
+		clock->src_shift = clock->src_width = 0;
 	}
 
+
 	/* Register the clock. */
 	init.name = name;
 	init.ops = &cpg_div6_clock_ops;
 	init.flags = CLK_IS_BASIC;
-	init.parent_names = &parent_name;
-	init.num_parents = 1;
+	init.parent_names = parent_names;
+	init.num_parents = valid_parents;
 
 	clock->hw.init = &init;
 
@@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 
 	of_clk_add_provider(np, of_clk_src_simple_get, clk);
 
+	kfree(parent_names);
 	return;
 
 error:
 	if (clock->reg)
 		iounmap(clock->reg);
+	kfree(parent_names);
 	kfree(clock);
 }
 CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock", cpg_div6_clock_init);
-- 
1.8.4.5

[PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Ulrich Hecht]

Support for setting the parent at initialization time based on the current
hardware configuration in DIV6 clocks with selectable parents as found in
the r8a73a4, r8a7740, sh73a0, and other SoCs.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/clk/shmobile/clk-div6.c | 117 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 105 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c
index f065f69..ea44988 100644
--- a/drivers/clk/shmobile/clk-div6.c
+++ b/drivers/clk/shmobile/clk-div6.c
@@ -32,6 +32,9 @@ struct div6_clock {
 	struct clk_hw hw;
 	void __iomem *reg;
 	unsigned int div;
+	int src_shift;
+	int src_width;
+	u8 *parents;
 };
 
 #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)
@@ -39,8 +42,11 @@ struct div6_clock {
 static int cpg_div6_clock_enable(struct clk_hw *hw)
 {
 	struct div6_clock *clock = to_div6_clock(hw);
+	u32 val;
 
-	clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+	val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP))
+	    | CPG_DIV6_DIV(clock->div - 1);
+	clk_writel(val, clock->reg);
 
 	return 0;
 }
@@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw)
 	/* DIV6 clocks require the divisor field to be non-zero when stopping
 	 * the clock.
 	 */
-	clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK),
+	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
 		   clock->reg);
 }
 
@@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct div6_clock *clock = to_div6_clock(hw);
 	unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate);
+	u32 val;
 
 	clock->div = div;
 
+	val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK;
 	/* Only program the new divisor if the clock isn't stopped. */
-	if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP))
-		clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
+	if (!(val & CPG_DIV6_CKSTP))
+		clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg);
+
+	return 0;
+}
+
+static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	u8 hw_index;
+	int i;
+
+	if (clock->src_width == 0)
+		return 0;
+
+	hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
+		   (BIT(clock->src_width) - 1);
+	for (i = 0; i < hw->init->num_parents; i++) {
+		if (clock->parents[i] == hw_index)
+			return i;
+	}
+
+	pr_err("%s: %s DIV6 clock set to invalid parent %d\n",
+	       __func__, hw->init->name, hw_index);
+	return 0;
+}
+
+static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct div6_clock *clock = to_div6_clock(hw);
+	u8 hw_index;
+	u32 mask;
+
+	if (index >= hw->init->num_parents)
+		return -EINVAL;
+
+	mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
+	hw_index = clock->parents[index];
+
+	clk_writel((clk_readl(clock->reg) & mask) |
+		(hw_index << clock->src_shift), clock->reg);
 
 	return 0;
 }
@@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = {
 	.enable = cpg_div6_clock_enable,
 	.disable = cpg_div6_clock_disable,
 	.is_enabled = cpg_div6_clock_is_enabled,
+	.get_parent = cpg_div6_clock_get_parent,
+	.set_parent = cpg_div6_clock_set_parent,
 	.recalc_rate = cpg_div6_clock_recalc_rate,
 	.round_rate = cpg_div6_clock_round_rate,
 	.set_rate = cpg_div6_clock_set_rate,
@@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
 
 static void __init cpg_div6_clock_init(struct device_node *np)
 {
+	const char **parent_names;
 	struct clk_init_data init;
 	struct div6_clock *clock;
-	const char *parent_name;
 	const char *name;
 	struct clk *clk;
+	int valid_parents;
+	int num_parents;
+	u32 src_shift;
+	u32 src_width;
 	int ret;
+	int i;
 
 	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
 	if (!clock) {
@@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 		return;
 	}
 
+	num_parents = of_clk_get_parent_count(np);
+	if (num_parents < 1) {
+		pr_err("%s: no parent found for %s DIV6 clock\n",
+		       __func__, np->name);
+		return;
+	}
+
+	clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
+		GFP_KERNEL);
+	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
+				GFP_KERNEL);
+
+	if (!parent_names) {
+		pr_err("%s: failed to allocate %s parent name list\n",
+		       __func__, np->name);
+		return;
+	}
+
 	/* Remap the clock register and read the divisor. Disabling the
 	 * clock overwrites the divisor, so we need to cache its value for the
 	 * enable operation.
@@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 		goto error;
 	}
 
-	parent_name = of_clk_get_parent_name(np, 0);
-	if (parent_name == NULL) {
-		pr_err("%s: failed to get %s DIV6 clock parent name\n",
-		       __func__, np->name);
-		goto error;
+
+	for (i = 0, valid_parents = 0; i < num_parents; i++) {
+		const char *name = of_clk_get_parent_name(np, i);
+
+		if (name) {
+			parent_names[valid_parents] = name;
+			clock->parents[valid_parents] = i;
+			valid_parents++;
+		}
+	}
+
+	if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
+		if (!of_property_read_u32(np, "renesas,src-width",
+					&src_width)) {
+			clock->src_shift = src_shift;
+			clock->src_width = src_width;
+		} else {
+			pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
+			       __func__, np->name);
+			goto error;
+		}
+	} else {
+		clock->src_shift = clock->src_width = 0;
 	}
 
+
 	/* Register the clock. */
 	init.name = name;
 	init.ops = &cpg_div6_clock_ops;
 	init.flags = CLK_IS_BASIC;
-	init.parent_names = &parent_name;
-	init.num_parents = 1;
+	init.parent_names = parent_names;
+	init.num_parents = valid_parents;
 
 	clock->hw.init = &init;
 
@@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct device_node *np)
 
 	of_clk_add_provider(np, of_clk_src_simple_get, clk);
 
+	kfree(parent_names);
 	return;
 
 error:
 	if (clock->reg)
 		iounmap(clock->reg);
+	kfree(parent_names);
 	kfree(clock);
 }
 CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock", cpg_div6_clock_init);
-- 
1.8.4.5

[PATCH 2/2] clk: shmobile: document DIV6 clock parent bindings

[Ulrich Hecht]

Adds properties renesas,src-shift and renesas,src-width, and describes
how to specify the available parent clocks.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 .../bindings/clock/renesas,cpg-div6-clocks.txt     | 28 +++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
index 952e373..348954b 100644
--- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
@@ -7,22 +7,38 @@ to 64.
 Required Properties:
 
   - compatible: Must be one of the following
+    - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks
+    - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks
     - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
     - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks
+    - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks
     - "renesas,cpg-div6-clock" for generic DIV6 clocks
   - reg: Base address and length of the memory resource used by the DIV6 clock
-  - clocks: Reference to the parent clock
+  - clocks: Reference to the parent clock(s); if there are multiple parent
+    clocks, one must be specified for each possible parent clock setting
+    in the clock register. Invalid settings must be specified as "<0>".
+    Trailing invalid settings may be omitted.
   - #clock-cells: Must be 0
   - clock-output-names: The name of the clock as a free-form string
 
+Optional Properties:
+
+  - renesas,src-shift: Bit position of the input clock selector (default:
+    fixed input clock; requires renesas,src-width)
+  - renesas,src-width: Bit width of the input clock selector (default: fixed
+    input clock; requires renesas,src-shift)
+
 
 Example
 -------
 
-	sd2_clk: sd2_clk@e6150078 {
-		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-clock";
-		reg = <0 0xe6150078 0 4>;
-		clocks = <&pll1_div2_clk>;
+	sdhi2_clk: sdhi2_clk@e615007c {
+		compatible = "renesas,r8a73a4-div6-clock", "renesas,cpg-div6-clock";
+		reg = <0 0xe615007c 0 4>;
+		clocks = <&pll1_div2_clk>, <&cpg_clocks R8A73A4_CLK_PLL2S>,
+			 <0>, <&extal2_clk>;
+		renesas,src-shift = <6>;
+		renesas,src-width = <2>;
 		#clock-cells = <0>;
-		clock-output-names = "sd2";
+		clock-output-names = "sdhi2ck";
 	};
-- 
1.8.4.5

[PATCH 2/2] clk: shmobile: document DIV6 clock parent bindings

[Ulrich Hecht]

Adds properties renesas,src-shift and renesas,src-width, and describes
how to specify the available parent clocks.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 .../bindings/clock/renesas,cpg-div6-clocks.txt     | 28 +++++++++++++++++-----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
index 952e373..348954b 100644
--- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
@@ -7,22 +7,38 @@ to 64.
 Required Properties:
 
   - compatible: Must be one of the following
+    - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks
+    - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks
     - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
     - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks
+    - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks
     - "renesas,cpg-div6-clock" for generic DIV6 clocks
   - reg: Base address and length of the memory resource used by the DIV6 clock
-  - clocks: Reference to the parent clock
+  - clocks: Reference to the parent clock(s); if there are multiple parent
+    clocks, one must be specified for each possible parent clock setting
+    in the clock register. Invalid settings must be specified as "<0>".
+    Trailing invalid settings may be omitted.
   - #clock-cells: Must be 0
   - clock-output-names: The name of the clock as a free-form string
 
+Optional Properties:
+
+  - renesas,src-shift: Bit position of the input clock selector (default:
+    fixed input clock; requires renesas,src-width)
+  - renesas,src-width: Bit width of the input clock selector (default: fixed
+    input clock; requires renesas,src-shift)
+
 
 Example
 -------
 
-	sd2_clk: sd2_clk@e6150078 {
-		compatible = "renesas,r8a7790-div6-clock", "renesas,cpg-div6-clock";
-		reg = <0 0xe6150078 0 4>;
-		clocks = <&pll1_div2_clk>;
+	sdhi2_clk: sdhi2_clk@e615007c {
+		compatible = "renesas,r8a73a4-div6-clock", "renesas,cpg-div6-clock";
+		reg = <0 0xe615007c 0 4>;
+		clocks = <&pll1_div2_clk>, <&cpg_clocks R8A73A4_CLK_PLL2S>,
+			 <0>, <&extal2_clk>;
+		renesas,src-shift = <6>;
+		renesas,src-width = <2>;
 		#clock-cells = <0>;
-		clock-output-names = "sd2";
+		clock-output-names = "sdhi2ck";
 	};
-- 
1.8.4.5

Re: [PATCH 2/2] clk: shmobile: document DIV6 clock parent bindings

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Oct 31, 2014 at 4:01 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds properties renesas,src-shift and renesas,src-width, and describes
> how to specify the available parent clocks.

Thanks for your patch!

> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  .../bindings/clock/renesas,cpg-div6-clocks.txt     | 28 +++++++++++++++++-----
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> index 952e373..348954b 100644
> --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> @@ -7,22 +7,38 @@ to 64.
>  Required Properties:
>
>    - compatible: Must be one of the following
> +    - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks
> +    - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks
>      - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
>      - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks
> +    - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks

Missing space between "SH-Mobile" and "AG5".

>      - "renesas,cpg-div6-clock" for generic DIV6 clocks
>    - reg: Base address and length of the memory resource used by the DIV6 clock
> -  - clocks: Reference to the parent clock
> +  - clocks: Reference to the parent clock(s); if there are multiple parent
> +    clocks, one must be specified for each possible parent clock setting
> +    in the clock register. Invalid settings must be specified as "<0>".
> +    Trailing invalid settings may be omitted.

Perhaps it's good to mention "if there are multiple parent clocks, up to
1 << renesas,src-width parent clocks"?

>    - #clock-cells: Must be 0
>    - clock-output-names: The name of the clock as a free-form string
>
> +Optional Properties:
> +
> +  - renesas,src-shift: Bit position of the input clock selector (default:
> +    fixed input clock; requires renesas,src-width)
> +  - renesas,src-width: Bit width of the input clock selector (default: fixed
> +    input clock; requires renesas,src-shift)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] clk: shmobile: document DIV6 clock parent bindings

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Oct 31, 2014 at 4:01 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Adds properties renesas,src-shift and renesas,src-width, and describes
> how to specify the available parent clocks.

Thanks for your patch!

> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  .../bindings/clock/renesas,cpg-div6-clocks.txt     | 28 +++++++++++++++++-----
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> index 952e373..348954b 100644
> --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> @@ -7,22 +7,38 @@ to 64.
>  Required Properties:
>
>    - compatible: Must be one of the following
> +    - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks
> +    - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks
>      - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks
>      - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks
> +    - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks

Missing space between "SH-Mobile" and "AG5".

>      - "renesas,cpg-div6-clock" for generic DIV6 clocks
>    - reg: Base address and length of the memory resource used by the DIV6 clock
> -  - clocks: Reference to the parent clock
> +  - clocks: Reference to the parent clock(s); if there are multiple parent
> +    clocks, one must be specified for each possible parent clock setting
> +    in the clock register. Invalid settings must be specified as "<0>".
> +    Trailing invalid settings may be omitted.

Perhaps it's good to mention "if there are multiple parent clocks, up to
1 << renesas,src-width parent clocks"?

>    - #clock-cells: Must be 0
>    - clock-output-names: The name of the clock as a free-form string
>
> +Optional Properties:
> +
> +  - renesas,src-shift: Bit position of the input clock selector (default:
> +    fixed input clock; requires renesas,src-width)
> +  - renesas,src-width: Bit width of the input clock selector (default: fixed
> +    input clock; requires renesas,src-shift)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Oct 31, 2014 at 4:01 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Support for setting the parent at initialization time based on the current
> hardware configuration in DIV6 clocks with selectable parents as found in
> the r8a73a4, r8a7740, sh73a0, and other SoCs.

Thanks for your patch!

> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -32,6 +32,9 @@ struct div6_clock {
>         struct clk_hw hw;
>         void __iomem *reg;
>         unsigned int div;
> +       int src_shift;
> +       int src_width;

unsigned int or u32 (both)

> +       u8 *parents;
>  };
>
>  #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)

> +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)

Please return u8, cfr. clk_ops.get_parent()

> +{
> +       struct div6_clock *clock = to_div6_clock(hw);
> +       u8 hw_index;
> +       int i;

unsigned int

> +       if (clock->src_width = 0)
> +               return 0;
> +
> +       hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
> +                  (BIT(clock->src_width) - 1);
> +       for (i = 0; i < hw->init->num_parents; i++) {

Is hw->init still valid here?
I believe it's a pointer to local stack variable "struct clk_init_data init"
in cpg_div6_clock_init(), which was never copied?

Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux?

> +               if (clock->parents[i] = hw_index)
> +                       return i;
> +       }
> +
> +       pr_err("%s: %s DIV6 clock set to invalid parent %d\n",

%u for u8

> +              __func__, hw->init->name, hw_index);
> +       return 0;
> +}

It's a bit unfortunate that clk_ops.get_parent() cannot return an error code,
as u8 is positive. Perhaps it should return int instead?

However, as clk_mux_get_parent() does return -EINVAL if the clock
can't be found, perhaps it's best to mimic that behavior?

__clk_init_parent() calls clk_get_parent_by_index(), which does check
if the index fits within the allowed range.

__clk_init() uses the returned value to index parent_names[] without any
checking. Oops...

Mike?

> +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct div6_clock *clock = to_div6_clock(hw);
> +       u8 hw_index;
> +       u32 mask;
> +
> +       if (index >= hw->init->num_parents)

Is hw->init still valid here?
Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux?

> +               return -EINVAL;
> +
> +       mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
> +       hw_index = clock->parents[index];
> +
> +       clk_writel((clk_readl(clock->reg) & mask) |
> +               (hw_index << clock->src_shift), clock->reg);
>
>         return 0;
>  }

> @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
>
>  static void __init cpg_div6_clock_init(struct device_node *np)
>  {
> +       const char **parent_names;
>         struct clk_init_data init;
>         struct div6_clock *clock;
> -       const char *parent_name;
>         const char *name;
>         struct clk *clk;
> +       int valid_parents;
> +       int num_parents;

unsigned int (both)

> +       u32 src_shift;
> +       u32 src_width;
>         int ret;
> +       int i;

unsigned int

> @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np)
>                 return;
>         }
>
> +       num_parents = of_clk_get_parent_count(np);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found for %s DIV6 clock\n",
> +                      __func__, np->name);
> +               return;
> +       }
> +
> +       clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
> +               GFP_KERNEL);
> +       parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> +                               GFP_KERNEL);
> +

Please drop the blank line above.

> +       if (!parent_names) {
> +               pr_err("%s: failed to allocate %s parent name list\n",
> +                      __func__, np->name);

There's no need to print an error in case of allocation failures.

> +               return;
> +       }
> +
>         /* Remap the clock register and read the divisor. Disabling the
>          * clock overwrites the divisor, so we need to cache its value for the
>          * enable operation.
> @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np)
>                 goto error;
>         }
>
> -       parent_name = of_clk_get_parent_name(np, 0);
> -       if (parent_name = NULL) {
> -               pr_err("%s: failed to get %s DIV6 clock parent name\n",
> -                      __func__, np->name);
> -               goto error;
> +
> +       for (i = 0, valid_parents = 0; i < num_parents; i++) {
> +               const char *name = of_clk_get_parent_name(np, i);
> +
> +               if (name) {
> +                       parent_names[valid_parents] = name;
> +                       clock->parents[valid_parents] = i;
> +                       valid_parents++;
> +               }
> +       }
> +
> +       if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
> +               if (!of_property_read_u32(np, "renesas,src-width",
> +                                       &src_width)) {
> +                       clock->src_shift = src_shift;
> +                       clock->src_width = src_width;
> +               } else {
> +                       pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
> +                              __func__, np->name);
> +                       goto error;
> +               }
> +       } else {
> +               clock->src_shift = clock->src_width = 0;
>         }

The above construct does allow renesas,src-width without renesas,src-shift.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Oct 31, 2014 at 4:01 PM, Ulrich Hecht
<ulrich.hecht+renesas@gmail.com> wrote:
> Support for setting the parent at initialization time based on the current
> hardware configuration in DIV6 clocks with selectable parents as found in
> the r8a73a4, r8a7740, sh73a0, and other SoCs.

Thanks for your patch!

> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -32,6 +32,9 @@ struct div6_clock {
>         struct clk_hw hw;
>         void __iomem *reg;
>         unsigned int div;
> +       int src_shift;
> +       int src_width;

unsigned int or u32 (both)

> +       u8 *parents;
>  };
>
>  #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)

> +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)

Please return u8, cfr. clk_ops.get_parent()

> +{
> +       struct div6_clock *clock = to_div6_clock(hw);
> +       u8 hw_index;
> +       int i;

unsigned int

> +       if (clock->src_width == 0)
> +               return 0;
> +
> +       hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
> +                  (BIT(clock->src_width) - 1);
> +       for (i = 0; i < hw->init->num_parents; i++) {

Is hw->init still valid here?
I believe it's a pointer to local stack variable "struct clk_init_data init"
in cpg_div6_clock_init(), which was never copied?

Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux?

> +               if (clock->parents[i] == hw_index)
> +                       return i;
> +       }
> +
> +       pr_err("%s: %s DIV6 clock set to invalid parent %d\n",

%u for u8

> +              __func__, hw->init->name, hw_index);
> +       return 0;
> +}

It's a bit unfortunate that clk_ops.get_parent() cannot return an error code,
as u8 is positive. Perhaps it should return int instead?

However, as clk_mux_get_parent() does return -EINVAL if the clock
can't be found, perhaps it's best to mimic that behavior?

__clk_init_parent() calls clk_get_parent_by_index(), which does check
if the index fits within the allowed range.

__clk_init() uses the returned value to index parent_names[] without any
checking. Oops...

Mike?

> +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct div6_clock *clock = to_div6_clock(hw);
> +       u8 hw_index;
> +       u32 mask;
> +
> +       if (index >= hw->init->num_parents)

Is hw->init still valid here?
Use __clk_get_num_parents(hw->clk) instead, cfr. clk-mux?

> +               return -EINVAL;
> +
> +       mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
> +       hw_index = clock->parents[index];
> +
> +       clk_writel((clk_readl(clock->reg) & mask) |
> +               (hw_index << clock->src_shift), clock->reg);
>
>         return 0;
>  }

> @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
>
>  static void __init cpg_div6_clock_init(struct device_node *np)
>  {
> +       const char **parent_names;
>         struct clk_init_data init;
>         struct div6_clock *clock;
> -       const char *parent_name;
>         const char *name;
>         struct clk *clk;
> +       int valid_parents;
> +       int num_parents;

unsigned int (both)

> +       u32 src_shift;
> +       u32 src_width;
>         int ret;
> +       int i;

unsigned int

> @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct device_node *np)
>                 return;
>         }
>
> +       num_parents = of_clk_get_parent_count(np);
> +       if (num_parents < 1) {
> +               pr_err("%s: no parent found for %s DIV6 clock\n",
> +                      __func__, np->name);
> +               return;
> +       }
> +
> +       clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
> +               GFP_KERNEL);
> +       parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> +                               GFP_KERNEL);
> +

Please drop the blank line above.

> +       if (!parent_names) {
> +               pr_err("%s: failed to allocate %s parent name list\n",
> +                      __func__, np->name);

There's no need to print an error in case of allocation failures.

> +               return;
> +       }
> +
>         /* Remap the clock register and read the divisor. Disabling the
>          * clock overwrites the divisor, so we need to cache its value for the
>          * enable operation.
> @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct device_node *np)
>                 goto error;
>         }
>
> -       parent_name = of_clk_get_parent_name(np, 0);
> -       if (parent_name == NULL) {
> -               pr_err("%s: failed to get %s DIV6 clock parent name\n",
> -                      __func__, np->name);
> -               goto error;
> +
> +       for (i = 0, valid_parents = 0; i < num_parents; i++) {
> +               const char *name = of_clk_get_parent_name(np, i);
> +
> +               if (name) {
> +                       parent_names[valid_parents] = name;
> +                       clock->parents[valid_parents] = i;
> +                       valid_parents++;
> +               }
> +       }
> +
> +       if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
> +               if (!of_property_read_u32(np, "renesas,src-width",
> +                                       &src_width)) {
> +                       clock->src_shift = src_shift;
> +                       clock->src_width = src_width;
> +               } else {
> +                       pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
> +                              __func__, np->name);
> +                       goto error;
> +               }
> +       } else {
> +               clock->src_shift = clock->src_width = 0;
>         }

The above construct does allow renesas,src-width without renesas,src-shift.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

Mark Rutland has asked a couple of interesting questions in his review of v4, 
which I don't think you have answered. I've proposed alternative bindings that 
wouldn't require the shift and width attributes. Could you please comment on 
that ?

http://www.spinics.net/lists/linux-sh/msg34936.html

On Friday 31 October 2014 16:01:35 Ulrich Hecht wrote:
> Support for setting the parent at initialization time based on the current
> hardware configuration in DIV6 clocks with selectable parents as found in
> the r8a73a4, r8a7740, sh73a0, and other SoCs.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/clk/shmobile/clk-div6.c | 117 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 105 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/shmobile/clk-div6.c
> b/drivers/clk/shmobile/clk-div6.c index f065f69..ea44988 100644
> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -32,6 +32,9 @@ struct div6_clock {
>  	struct clk_hw hw;
>  	void __iomem *reg;
>  	unsigned int div;
> +	int src_shift;
> +	int src_width;
> +	u8 *parents;
>  };
> 
>  #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)
> @@ -39,8 +42,11 @@ struct div6_clock {
>  static int cpg_div6_clock_enable(struct clk_hw *hw)
>  {
>  	struct div6_clock *clock = to_div6_clock(hw);
> +	u32 val;
> 
> -	clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +	val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP))
> +	    | CPG_DIV6_DIV(clock->div - 1);
> +	clk_writel(val, clock->reg);
> 
>  	return 0;
>  }
> @@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw)
>  	/* DIV6 clocks require the divisor field to be non-zero when stopping
>  	 * the clock.
>  	 */
> -	clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK),
> +	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
>  		   clock->reg);
>  }
> 
> @@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw,
> unsigned long rate, {
>  	struct div6_clock *clock = to_div6_clock(hw);
>  	unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate);
> +	u32 val;
> 
>  	clock->div = div;
> 
> +	val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK;
>  	/* Only program the new divisor if the clock isn't stopped. */
> -	if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP))
> -		clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +	if (!(val & CPG_DIV6_CKSTP))
> +		clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +
> +	return 0;
> +}
> +
> +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)
> +{
> +	struct div6_clock *clock = to_div6_clock(hw);
> +	u8 hw_index;
> +	int i;
> +
> +	if (clock->src_width = 0)
> +		return 0;
> +
> +	hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
> +		   (BIT(clock->src_width) - 1);
> +	for (i = 0; i < hw->init->num_parents; i++) {
> +		if (clock->parents[i] = hw_index)
> +			return i;
> +	}
> +
> +	pr_err("%s: %s DIV6 clock set to invalid parent %d\n",
> +	       __func__, hw->init->name, hw_index);
> +	return 0;
> +}
> +
> +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct div6_clock *clock = to_div6_clock(hw);
> +	u8 hw_index;
> +	u32 mask;
> +
> +	if (index >= hw->init->num_parents)
> +		return -EINVAL;
> +
> +	mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
> +	hw_index = clock->parents[index];
> +
> +	clk_writel((clk_readl(clock->reg) & mask) |
> +		(hw_index << clock->src_shift), clock->reg);
> 
>  	return 0;
>  }
> @@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = {
>  	.enable = cpg_div6_clock_enable,
>  	.disable = cpg_div6_clock_disable,
>  	.is_enabled = cpg_div6_clock_is_enabled,
> +	.get_parent = cpg_div6_clock_get_parent,
> +	.set_parent = cpg_div6_clock_set_parent,
>  	.recalc_rate = cpg_div6_clock_recalc_rate,
>  	.round_rate = cpg_div6_clock_round_rate,
>  	.set_rate = cpg_div6_clock_set_rate,
> @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
> 
>  static void __init cpg_div6_clock_init(struct device_node *np)
>  {
> +	const char **parent_names;
>  	struct clk_init_data init;
>  	struct div6_clock *clock;
> -	const char *parent_name;
>  	const char *name;
>  	struct clk *clk;
> +	int valid_parents;
> +	int num_parents;
> +	u32 src_shift;
> +	u32 src_width;
>  	int ret;
> +	int i;
> 
>  	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
>  	if (!clock) {
> @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) return;
>  	}
> 
> +	num_parents = of_clk_get_parent_count(np);
> +	if (num_parents < 1) {
> +		pr_err("%s: no parent found for %s DIV6 clock\n",
> +		       __func__, np->name);
> +		return;
> +	}
> +
> +	clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
> +		GFP_KERNEL);
> +	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> +				GFP_KERNEL);
> +
> +	if (!parent_names) {
> +		pr_err("%s: failed to allocate %s parent name list\n",
> +		       __func__, np->name);
> +		return;
> +	}
> +
>  	/* Remap the clock register and read the divisor. Disabling the
>  	 * clock overwrites the divisor, so we need to cache its value for the
>  	 * enable operation.
> @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) goto error;
>  	}
> 
> -	parent_name = of_clk_get_parent_name(np, 0);
> -	if (parent_name = NULL) {
> -		pr_err("%s: failed to get %s DIV6 clock parent name\n",
> -		       __func__, np->name);
> -		goto error;
> +
> +	for (i = 0, valid_parents = 0; i < num_parents; i++) {
> +		const char *name = of_clk_get_parent_name(np, i);
> +
> +		if (name) {
> +			parent_names[valid_parents] = name;
> +			clock->parents[valid_parents] = i;
> +			valid_parents++;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
> +		if (!of_property_read_u32(np, "renesas,src-width",
> +					&src_width)) {
> +			clock->src_shift = src_shift;
> +			clock->src_width = src_width;
> +		} else {
> +			pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
> +			       __func__, np->name);
> +			goto error;
> +		}
> +	} else {
> +		clock->src_shift = clock->src_width = 0;
>  	}
> 
> +
>  	/* Register the clock. */
>  	init.name = name;
>  	init.ops = &cpg_div6_clock_ops;
>  	init.flags = CLK_IS_BASIC;
> -	init.parent_names = &parent_name;
> -	init.num_parents = 1;
> +	init.parent_names = parent_names;
> +	init.num_parents = valid_parents;
> 
>  	clock->hw.init = &init;
> 
> @@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct
> device_node *np)
> 
>  	of_clk_add_provider(np, of_clk_src_simple_get, clk);
> 
> +	kfree(parent_names);
>  	return;
> 
>  error:
>  	if (clock->reg)
>  		iounmap(clock->reg);
> +	kfree(parent_names);
>  	kfree(clock);
>  }
>  CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock",
> cpg_div6_clock_init);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] clk: shmobile: div6: support selectable-input clocks

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

Mark Rutland has asked a couple of interesting questions in his review of v4, 
which I don't think you have answered. I've proposed alternative bindings that 
wouldn't require the shift and width attributes. Could you please comment on 
that ?

http://www.spinics.net/lists/linux-sh/msg34936.html

On Friday 31 October 2014 16:01:35 Ulrich Hecht wrote:
> Support for setting the parent at initialization time based on the current
> hardware configuration in DIV6 clocks with selectable parents as found in
> the r8a73a4, r8a7740, sh73a0, and other SoCs.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/clk/shmobile/clk-div6.c | 117 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 105 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/shmobile/clk-div6.c
> b/drivers/clk/shmobile/clk-div6.c index f065f69..ea44988 100644
> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -32,6 +32,9 @@ struct div6_clock {
>  	struct clk_hw hw;
>  	void __iomem *reg;
>  	unsigned int div;
> +	int src_shift;
> +	int src_width;
> +	u8 *parents;
>  };
> 
>  #define to_div6_clock(_hw) container_of(_hw, struct div6_clock, hw)
> @@ -39,8 +42,11 @@ struct div6_clock {
>  static int cpg_div6_clock_enable(struct clk_hw *hw)
>  {
>  	struct div6_clock *clock = to_div6_clock(hw);
> +	u32 val;
> 
> -	clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +	val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP))
> +	    | CPG_DIV6_DIV(clock->div - 1);
> +	clk_writel(val, clock->reg);
> 
>  	return 0;
>  }
> @@ -52,7 +58,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw)
>  	/* DIV6 clocks require the divisor field to be non-zero when stopping
>  	 * the clock.
>  	 */
> -	clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK),
> +	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
>  		   clock->reg);
>  }
> 
> @@ -94,12 +100,53 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw,
> unsigned long rate, {
>  	struct div6_clock *clock = to_div6_clock(hw);
>  	unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate);
> +	u32 val;
> 
>  	clock->div = div;
> 
> +	val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK;
>  	/* Only program the new divisor if the clock isn't stopped. */
> -	if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP))
> -		clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +	if (!(val & CPG_DIV6_CKSTP))
> +		clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg);
> +
> +	return 0;
> +}
> +
> +static unsigned char cpg_div6_clock_get_parent(struct clk_hw *hw)
> +{
> +	struct div6_clock *clock = to_div6_clock(hw);
> +	u8 hw_index;
> +	int i;
> +
> +	if (clock->src_width == 0)
> +		return 0;
> +
> +	hw_index = (clk_readl(clock->reg) >> clock->src_shift) &
> +		   (BIT(clock->src_width) - 1);
> +	for (i = 0; i < hw->init->num_parents; i++) {
> +		if (clock->parents[i] == hw_index)
> +			return i;
> +	}
> +
> +	pr_err("%s: %s DIV6 clock set to invalid parent %d\n",
> +	       __func__, hw->init->name, hw_index);
> +	return 0;
> +}
> +
> +static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct div6_clock *clock = to_div6_clock(hw);
> +	u8 hw_index;
> +	u32 mask;
> +
> +	if (index >= hw->init->num_parents)
> +		return -EINVAL;
> +
> +	mask = ~((BIT(clock->src_width) - 1) << clock->src_shift);
> +	hw_index = clock->parents[index];
> +
> +	clk_writel((clk_readl(clock->reg) & mask) |
> +		(hw_index << clock->src_shift), clock->reg);
> 
>  	return 0;
>  }
> @@ -108,6 +155,8 @@ static const struct clk_ops cpg_div6_clock_ops = {
>  	.enable = cpg_div6_clock_enable,
>  	.disable = cpg_div6_clock_disable,
>  	.is_enabled = cpg_div6_clock_is_enabled,
> +	.get_parent = cpg_div6_clock_get_parent,
> +	.set_parent = cpg_div6_clock_set_parent,
>  	.recalc_rate = cpg_div6_clock_recalc_rate,
>  	.round_rate = cpg_div6_clock_round_rate,
>  	.set_rate = cpg_div6_clock_set_rate,
> @@ -115,12 +164,17 @@ static const struct clk_ops cpg_div6_clock_ops = {
> 
>  static void __init cpg_div6_clock_init(struct device_node *np)
>  {
> +	const char **parent_names;
>  	struct clk_init_data init;
>  	struct div6_clock *clock;
> -	const char *parent_name;
>  	const char *name;
>  	struct clk *clk;
> +	int valid_parents;
> +	int num_parents;
> +	u32 src_shift;
> +	u32 src_width;
>  	int ret;
> +	int i;
> 
>  	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
>  	if (!clock) {
> @@ -129,6 +183,24 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) return;
>  	}
> 
> +	num_parents = of_clk_get_parent_count(np);
> +	if (num_parents < 1) {
> +		pr_err("%s: no parent found for %s DIV6 clock\n",
> +		       __func__, np->name);
> +		return;
> +	}
> +
> +	clock->parents = kmalloc_array(num_parents, sizeof(*clock->parents),
> +		GFP_KERNEL);
> +	parent_names = kmalloc_array(num_parents, sizeof(*parent_names),
> +				GFP_KERNEL);
> +
> +	if (!parent_names) {
> +		pr_err("%s: failed to allocate %s parent name list\n",
> +		       __func__, np->name);
> +		return;
> +	}
> +
>  	/* Remap the clock register and read the divisor. Disabling the
>  	 * clock overwrites the divisor, so we need to cache its value for the
>  	 * enable operation.
> @@ -150,19 +222,38 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) goto error;
>  	}
> 
> -	parent_name = of_clk_get_parent_name(np, 0);
> -	if (parent_name == NULL) {
> -		pr_err("%s: failed to get %s DIV6 clock parent name\n",
> -		       __func__, np->name);
> -		goto error;
> +
> +	for (i = 0, valid_parents = 0; i < num_parents; i++) {
> +		const char *name = of_clk_get_parent_name(np, i);
> +
> +		if (name) {
> +			parent_names[valid_parents] = name;
> +			clock->parents[valid_parents] = i;
> +			valid_parents++;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) {
> +		if (!of_property_read_u32(np, "renesas,src-width",
> +					&src_width)) {
> +			clock->src_shift = src_shift;
> +			clock->src_width = src_width;
> +		} else {
> +			pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
> +			       __func__, np->name);
> +			goto error;
> +		}
> +	} else {
> +		clock->src_shift = clock->src_width = 0;
>  	}
> 
> +
>  	/* Register the clock. */
>  	init.name = name;
>  	init.ops = &cpg_div6_clock_ops;
>  	init.flags = CLK_IS_BASIC;
> -	init.parent_names = &parent_name;
> -	init.num_parents = 1;
> +	init.parent_names = parent_names;
> +	init.num_parents = valid_parents;
> 
>  	clock->hw.init = &init;
> 
> @@ -175,11 +266,13 @@ static void __init cpg_div6_clock_init(struct
> device_node *np)
> 
>  	of_clk_add_provider(np, of_clk_src_simple_get, clk);
> 
> +	kfree(parent_names);
>  	return;
> 
>  error:
>  	if (clock->reg)
>  		iounmap(clock->reg);
> +	kfree(parent_names);
>  	kfree(clock);
>  }
>  CLK_OF_DECLARE(cpg_div6_clk, "renesas,cpg-div6-clock",
> cpg_div6_clock_init);

-- 
Regards,

Laurent Pinchart