[PATCH][RFC] clk: Use node name and index for clock name

[PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

This patch hacks the CCF core to take clock-indices into
consideration when making a default clock name in case
clock-output-names is not provided.

Without this patch of_clk_get_parent_name() does not work
for clocks with multiple indices associated with one node.

Proof of concept only. Leaks memory. Not for upstream merge.

Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Written on top of "renesas-drivers-2015-09-08-v4.2"
 Needed to propagate clock frequencies from CPG to MSTP.

 drivers/clk/clk.c                    |   46 ++++++++++++++++++++++------------
 drivers/clk/shmobile/clk-mstp.c      |    3 --
 drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
 include/linux/clk-provider.h         |    1 
 4 files changed, 35 insertions(+), 28 deletions(-)

--- 0001/drivers/clk/clk.c
+++ work/drivers/clk/clk.c	2015-09-09 13:48:21.992366518 +0900
@@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
 
-const char *of_clk_get_parent_name(struct device_node *np, int index)
+const char *of_clk_get_name(struct device_node *np, int index)
 {
-	struct of_phandle_args clkspec;
 	struct property *prop;
 	const char *clk_name;
 	const __be32 *vp;
 	u32 pv;
-	int rc;
 	int count;
+	bool has_indices = false;
 
 	if (index < 0)
 		return NULL;
 
-	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
-					&clkspec);
-	if (rc)
-		return NULL;
-
-	index = clkspec.args_count ? clkspec.args[0] : 0;
 	count = 0;
 
 	/* if there is an indices property, use it to transfer the index
 	 * specified into an array offset for the clock-output-names property.
 	 */
-	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
+	of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
+		has_indices = true;
 		if (index == pv) {
 			index = count;
 			break;
@@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
 		count++;
 	}
 
-	if (of_property_read_string_index(clkspec.np, "clock-output-names",
-					  index,
-					  &clk_name) < 0)
-		clk_name = clkspec.np->name;
+	if (of_property_read_string_index(np, "clock-output-names", index,
+					  &clk_name) < 0) {
+		if (has_indices)
+			return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);
+		else
+			return kstrdup_const(np->name, GFP_KERNEL);
+	}
 
-	of_node_put(clkspec.np);
+	return kstrdup_const(clk_name, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(of_clk_get_name);
+
+const char *of_clk_get_parent_name(struct device_node *np, int index)
+{
+	struct of_phandle_args clkspec;
+	const char *clk_name = NULL;
+	int rc;
+
+	if (index < 0)
+		return NULL;
+
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+					&clkspec);
+	if (!rc) {
+		clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ?
+					   clkspec.args[0] : 0);
+		of_node_put(clkspec.np);
+	}
 	return clk_name;
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
--- 0001/drivers/clk/shmobile/clk-mstp.c
+++ work/drivers/clk/shmobile/clk-mstp.c	2015-09-09 13:45:51.652366518 +0900
@@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init(
 
 		if (of_property_read_string_index(np, "clock-output-names",
 						  i, &name) < 0)
-			allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u",
-							   np->name, clkidx);
+			allocated_name = name = of_clk_get_name(np, clkidx);
 
 		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
 						       clkidx, group);
--- 0001/drivers/clk/shmobile/clk-rcar-gen3.c
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-09-09 13:45:51.652366518 +0900
@@ -28,15 +28,6 @@
 #define RCAR_GEN3_CLK_PLL4	5
 #define RCAR_GEN3_CLK_NR	6
 
-static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
-	[RCAR_GEN3_CLK_MAIN] = "main",
-	[RCAR_GEN3_CLK_PLL0] = "pll0",
-	[RCAR_GEN3_CLK_PLL1] = "pll1",
-	[RCAR_GEN3_CLK_PLL2] = "pll2",
-	[RCAR_GEN3_CLK_PLL3] = "pll3",
-	[RCAR_GEN3_CLK_PLL4] = "pll4",
-};
-
 struct rcar_gen3_cpg {
 	struct clk_onecell_data data;
 	void __iomem *reg;
@@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device
 			   const struct cpg_pll_config *config,
 			   unsigned int gen3_clk)
 {
-	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
+	const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN);
 	unsigned int mult = 1;
 	unsigned int div = 1;
 	u32 value;
@@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device
 		return ERR_PTR(-EINVAL);
 	}
 
-	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
+	return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk),
 					 parent_name, 0, mult, div);
 }
 
--- 0001/include/linux/clk-provider.h
+++ work/include/linux/clk-provider.h	2015-09-09 13:46:21.052366518 +0900
@@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc
 int of_clk_get_parent_count(struct device_node *np);
 int of_clk_parent_fill(struct device_node *np, const char **parents,
 		       unsigned int size);
+const char *of_clk_get_name(struct device_node *np, int index);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);

[PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

This patch hacks the CCF core to take clock-indices into
consideration when making a default clock name in case
clock-output-names is not provided.

Without this patch of_clk_get_parent_name() does not work
for clocks with multiple indices associated with one node.

Proof of concept only. Leaks memory. Not for upstream merge.

Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Written on top of "renesas-drivers-2015-09-08-v4.2"
 Needed to propagate clock frequencies from CPG to MSTP.

 drivers/clk/clk.c                    |   46 ++++++++++++++++++++++------------
 drivers/clk/shmobile/clk-mstp.c      |    3 --
 drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
 include/linux/clk-provider.h         |    1 
 4 files changed, 35 insertions(+), 28 deletions(-)

--- 0001/drivers/clk/clk.c
+++ work/drivers/clk/clk.c	2015-09-09 13:48:21.992366518 +0900
@@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
 
-const char *of_clk_get_parent_name(struct device_node *np, int index)
+const char *of_clk_get_name(struct device_node *np, int index)
 {
-	struct of_phandle_args clkspec;
 	struct property *prop;
 	const char *clk_name;
 	const __be32 *vp;
 	u32 pv;
-	int rc;
 	int count;
+	bool has_indices = false;
 
 	if (index < 0)
 		return NULL;
 
-	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
-					&clkspec);
-	if (rc)
-		return NULL;
-
-	index = clkspec.args_count ? clkspec.args[0] : 0;
 	count = 0;
 
 	/* if there is an indices property, use it to transfer the index
 	 * specified into an array offset for the clock-output-names property.
 	 */
-	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
+	of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
+		has_indices = true;
 		if (index = pv) {
 			index = count;
 			break;
@@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
 		count++;
 	}
 
-	if (of_property_read_string_index(clkspec.np, "clock-output-names",
-					  index,
-					  &clk_name) < 0)
-		clk_name = clkspec.np->name;
+	if (of_property_read_string_index(np, "clock-output-names", index,
+					  &clk_name) < 0) {
+		if (has_indices)
+			return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);
+		else
+			return kstrdup_const(np->name, GFP_KERNEL);
+	}
 
-	of_node_put(clkspec.np);
+	return kstrdup_const(clk_name, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(of_clk_get_name);
+
+const char *of_clk_get_parent_name(struct device_node *np, int index)
+{
+	struct of_phandle_args clkspec;
+	const char *clk_name = NULL;
+	int rc;
+
+	if (index < 0)
+		return NULL;
+
+	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
+					&clkspec);
+	if (!rc) {
+		clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ?
+					   clkspec.args[0] : 0);
+		of_node_put(clkspec.np);
+	}
 	return clk_name;
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
--- 0001/drivers/clk/shmobile/clk-mstp.c
+++ work/drivers/clk/shmobile/clk-mstp.c	2015-09-09 13:45:51.652366518 +0900
@@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init(
 
 		if (of_property_read_string_index(np, "clock-output-names",
 						  i, &name) < 0)
-			allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u",
-							   np->name, clkidx);
+			allocated_name = name = of_clk_get_name(np, clkidx);
 
 		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
 						       clkidx, group);
--- 0001/drivers/clk/shmobile/clk-rcar-gen3.c
+++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-09-09 13:45:51.652366518 +0900
@@ -28,15 +28,6 @@
 #define RCAR_GEN3_CLK_PLL4	5
 #define RCAR_GEN3_CLK_NR	6
 
-static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
-	[RCAR_GEN3_CLK_MAIN] = "main",
-	[RCAR_GEN3_CLK_PLL0] = "pll0",
-	[RCAR_GEN3_CLK_PLL1] = "pll1",
-	[RCAR_GEN3_CLK_PLL2] = "pll2",
-	[RCAR_GEN3_CLK_PLL3] = "pll3",
-	[RCAR_GEN3_CLK_PLL4] = "pll4",
-};
-
 struct rcar_gen3_cpg {
 	struct clk_onecell_data data;
 	void __iomem *reg;
@@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device
 			   const struct cpg_pll_config *config,
 			   unsigned int gen3_clk)
 {
-	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
+	const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN);
 	unsigned int mult = 1;
 	unsigned int div = 1;
 	u32 value;
@@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device
 		return ERR_PTR(-EINVAL);
 	}
 
-	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
+	return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk),
 					 parent_name, 0, mult, div);
 }
 
--- 0001/include/linux/clk-provider.h
+++ work/include/linux/clk-provider.h	2015-09-09 13:46:21.052366518 +0900
@@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc
 int of_clk_get_parent_count(struct device_node *np);
 int of_clk_parent_fill(struct device_node *np, const char **parents,
 		       unsigned int size);
+const char *of_clk_get_name(struct device_node *np, int index);
 const char *of_clk_get_parent_name(struct device_node *np, int index);
 
 void of_clk_init(const struct of_device_id *matches);

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> This patch hacks the CCF core to take clock-indices into
> consideration when making a default clock name in case
> clock-output-names is not provided.
> 
> Without this patch of_clk_get_parent_name() does not work
> for clocks with multiple indices associated with one node.
> 
> Proof of concept only. Leaks memory. Not for upstream merge.
> 
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Written on top of "renesas-drivers-2015-09-08-v4.2"
>  Needed to propagate clock frequencies from CPG to MSTP.
> 
>  drivers/clk/clk.c                    |   46 +++++++++++++++++++------------
>  drivers/clk/shmobile/clk-mstp.c      |    3 --
>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
>  include/linux/clk-provider.h         |    1
>  4 files changed, 35 insertions(+), 28 deletions(-)
> 
> --- 0001/drivers/clk/clk.c
> +++ work/drivers/clk/clk.c	2015-09-09 13:48:21.992366518 +0900
> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
> 
> -const char *of_clk_get_parent_name(struct device_node *np, int index)
> +const char *of_clk_get_name(struct device_node *np, int index)
>  {
> -	struct of_phandle_args clkspec;
>  	struct property *prop;
>  	const char *clk_name;
>  	const __be32 *vp;
>  	u32 pv;
> -	int rc;
>  	int count;
> +	bool has_indices = false;
> 
>  	if (index < 0)
>  		return NULL;
> 
> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> -					&clkspec);
> -	if (rc)
> -		return NULL;
> -
> -	index = clkspec.args_count ? clkspec.args[0] : 0;
>  	count = 0;
> 
>  	/* if there is an indices property, use it to transfer the index
>  	 * specified into an array offset for the clock-output-names property.
>  	 */
> -	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> +	of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
> +		has_indices = true;
>  		if (index == pv) {
>  			index = count;
>  			break;
> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
>  		count++;
>  	}
> 
> -	if (of_property_read_string_index(clkspec.np, "clock-output-names",
> -					  index,
> -					  &clk_name) < 0)
> -		clk_name = clkspec.np->name;
> +	if (of_property_read_string_index(np, "clock-output-names", index,
> +					  &clk_name) < 0) {
> +		if (has_indices)
> +			return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);

What if the clock provider has a #clock-cells larger than one ?

Another issue is that this won't guarantee that the names are unique as 
multiple DT nodes can have the same name. Instead of trying to generate unique 
names, would it be possible to handle clock registration and lookup without 
relying on names for DT-based platforms ?

> +		else
> +			return kstrdup_const(np->name, GFP_KERNEL);
> +	}
> 
> -	of_node_put(clkspec.np);
> +	return kstrdup_const(clk_name, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(of_clk_get_name);
> +
> +const char *of_clk_get_parent_name(struct device_node *np, int index)
> +{
> +	struct of_phandle_args clkspec;
> +	const char *clk_name = NULL;
> +	int rc;
> +
> +	if (index < 0)
> +		return NULL;
> +
> +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> +					&clkspec);
> +	if (!rc) {
> +		clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ?
> +					   clkspec.args[0] : 0);
> +		of_node_put(clkspec.np);
> +	}
>  	return clk_name;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
> --- 0001/drivers/clk/shmobile/clk-mstp.c
> +++ work/drivers/clk/shmobile/clk-mstp.c	2015-09-09 13:45:51.652366518 
+0900
> @@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init(
> 
>  		if (of_property_read_string_index(np, "clock-output-names",
>  						  i, &name) < 0)
> -			allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u",
> -							   np->name, clkidx);
> +			allocated_name = name = of_clk_get_name(np, clkidx);
> 
>  		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
>  						       clkidx, group);
> --- 0001/drivers/clk/shmobile/clk-rcar-gen3.c
> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-09-09 
13:45:51.652366518
> +0900 @@ -28,15 +28,6 @@
>  #define RCAR_GEN3_CLK_PLL4	5
>  #define RCAR_GEN3_CLK_NR	6
> 
> -static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
> -	[RCAR_GEN3_CLK_MAIN] = "main",
> -	[RCAR_GEN3_CLK_PLL0] = "pll0",
> -	[RCAR_GEN3_CLK_PLL1] = "pll1",
> -	[RCAR_GEN3_CLK_PLL2] = "pll2",
> -	[RCAR_GEN3_CLK_PLL3] = "pll3",
> -	[RCAR_GEN3_CLK_PLL4] = "pll4",
> -};
> -
>  struct rcar_gen3_cpg {
>  	struct clk_onecell_data data;
>  	void __iomem *reg;
> @@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device
>  			   const struct cpg_pll_config *config,
>  			   unsigned int gen3_clk)
>  {
> -	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
> +	const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN);
>  	unsigned int mult = 1;
>  	unsigned int div = 1;
>  	u32 value;
> @@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device
>  		return ERR_PTR(-EINVAL);
>  	}
> 
> -	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
> +	return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk),
>  					 parent_name, 0, mult, div);
>  }
> 
> --- 0001/include/linux/clk-provider.h
> +++ work/include/linux/clk-provider.h	2015-09-09 13:46:21.052366518 +0900
> @@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc
>  int of_clk_get_parent_count(struct device_node *np);
>  int of_clk_parent_fill(struct device_node *np, const char **parents,
>  		       unsigned int size);
> +const char *of_clk_get_name(struct device_node *np, int index);
>  const char *of_clk_get_parent_name(struct device_node *np, int index);
> 
>  void of_clk_init(const struct of_device_id *matches);

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> This patch hacks the CCF core to take clock-indices into
> consideration when making a default clock name in case
> clock-output-names is not provided.
> 
> Without this patch of_clk_get_parent_name() does not work
> for clocks with multiple indices associated with one node.
> 
> Proof of concept only. Leaks memory. Not for upstream merge.
> 
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Written on top of "renesas-drivers-2015-09-08-v4.2"
>  Needed to propagate clock frequencies from CPG to MSTP.
> 
>  drivers/clk/clk.c                    |   46 +++++++++++++++++++------------
>  drivers/clk/shmobile/clk-mstp.c      |    3 --
>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
>  include/linux/clk-provider.h         |    1
>  4 files changed, 35 insertions(+), 28 deletions(-)
> 
> --- 0001/drivers/clk/clk.c
> +++ work/drivers/clk/clk.c	2015-09-09 13:48:21.992366518 +0900
> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
> 
> -const char *of_clk_get_parent_name(struct device_node *np, int index)
> +const char *of_clk_get_name(struct device_node *np, int index)
>  {
> -	struct of_phandle_args clkspec;
>  	struct property *prop;
>  	const char *clk_name;
>  	const __be32 *vp;
>  	u32 pv;
> -	int rc;
>  	int count;
> +	bool has_indices = false;
> 
>  	if (index < 0)
>  		return NULL;
> 
> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> -					&clkspec);
> -	if (rc)
> -		return NULL;
> -
> -	index = clkspec.args_count ? clkspec.args[0] : 0;
>  	count = 0;
> 
>  	/* if there is an indices property, use it to transfer the index
>  	 * specified into an array offset for the clock-output-names property.
>  	 */
> -	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> +	of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
> +		has_indices = true;
>  		if (index = pv) {
>  			index = count;
>  			break;
> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
>  		count++;
>  	}
> 
> -	if (of_property_read_string_index(clkspec.np, "clock-output-names",
> -					  index,
> -					  &clk_name) < 0)
> -		clk_name = clkspec.np->name;
> +	if (of_property_read_string_index(np, "clock-output-names", index,
> +					  &clk_name) < 0) {
> +		if (has_indices)
> +			return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);

What if the clock provider has a #clock-cells larger than one ?

Another issue is that this won't guarantee that the names are unique as 
multiple DT nodes can have the same name. Instead of trying to generate unique 
names, would it be possible to handle clock registration and lookup without 
relying on names for DT-based platforms ?

> +		else
> +			return kstrdup_const(np->name, GFP_KERNEL);
> +	}
> 
> -	of_node_put(clkspec.np);
> +	return kstrdup_const(clk_name, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(of_clk_get_name);
> +
> +const char *of_clk_get_parent_name(struct device_node *np, int index)
> +{
> +	struct of_phandle_args clkspec;
> +	const char *clk_name = NULL;
> +	int rc;
> +
> +	if (index < 0)
> +		return NULL;
> +
> +	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> +					&clkspec);
> +	if (!rc) {
> +		clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ?
> +					   clkspec.args[0] : 0);
> +		of_node_put(clkspec.np);
> +	}
>  	return clk_name;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
> --- 0001/drivers/clk/shmobile/clk-mstp.c
> +++ work/drivers/clk/shmobile/clk-mstp.c	2015-09-09 13:45:51.652366518 
+0900
> @@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init(
> 
>  		if (of_property_read_string_index(np, "clock-output-names",
>  						  i, &name) < 0)
> -			allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u",
> -							   np->name, clkidx);
> +			allocated_name = name = of_clk_get_name(np, clkidx);
> 
>  		clks[clkidx] = cpg_mstp_clock_register(name, parent_name,
>  						       clkidx, group);
> --- 0001/drivers/clk/shmobile/clk-rcar-gen3.c
> +++ work/drivers/clk/shmobile/clk-rcar-gen3.c	2015-09-09 
13:45:51.652366518
> +0900 @@ -28,15 +28,6 @@
>  #define RCAR_GEN3_CLK_PLL4	5
>  #define RCAR_GEN3_CLK_NR	6
> 
> -static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = {
> -	[RCAR_GEN3_CLK_MAIN] = "main",
> -	[RCAR_GEN3_CLK_PLL0] = "pll0",
> -	[RCAR_GEN3_CLK_PLL1] = "pll1",
> -	[RCAR_GEN3_CLK_PLL2] = "pll2",
> -	[RCAR_GEN3_CLK_PLL3] = "pll3",
> -	[RCAR_GEN3_CLK_PLL4] = "pll4",
> -};
> -
>  struct rcar_gen3_cpg {
>  	struct clk_onecell_data data;
>  	void __iomem *reg;
> @@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device
>  			   const struct cpg_pll_config *config,
>  			   unsigned int gen3_clk)
>  {
> -	const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN];
> +	const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN);
>  	unsigned int mult = 1;
>  	unsigned int div = 1;
>  	u32 value;
> @@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device
>  		return ERR_PTR(-EINVAL);
>  	}
> 
> -	return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk],
> +	return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk),
>  					 parent_name, 0, mult, div);
>  }
> 
> --- 0001/include/linux/clk-provider.h
> +++ work/include/linux/clk-provider.h	2015-09-09 13:46:21.052366518 +0900
> @@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc
>  int of_clk_get_parent_count(struct device_node *np);
>  int of_clk_parent_fill(struct device_node *np, const char **parents,
>  		       unsigned int size);
> +const char *of_clk_get_name(struct device_node *np, int index);
>  const char *of_clk_get_parent_name(struct device_node *np, int index);
> 
>  void of_clk_init(const struct of_device_id *matches);

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

HI Laurent,

On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> This patch hacks the CCF core to take clock-indices into
>> consideration when making a default clock name in case
>> clock-output-names is not provided.
>>
>> Without this patch of_clk_get_parent_name() does not work
>> for clocks with multiple indices associated with one node.
>>
>> Proof of concept only. Leaks memory. Not for upstream merge.
>>
>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Written on top of "renesas-drivers-2015-09-08-v4.2"
>>  Needed to propagate clock frequencies from CPG to MSTP.
>>
>>  drivers/clk/clk.c                    |   46 +++++++++++++++++++------------
>>  drivers/clk/shmobile/clk-mstp.c      |    3 --
>>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
>>  include/linux/clk-provider.h         |    1
>>  4 files changed, 35 insertions(+), 28 deletions(-)
>>
>> --- 0001/drivers/clk/clk.c
>> +++ work/drivers/clk/clk.c    2015-09-09 13:48:21.992366518 +0900
>> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
>>  }
>>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
>>
>> -const char *of_clk_get_parent_name(struct device_node *np, int index)
>> +const char *of_clk_get_name(struct device_node *np, int index)
>>  {
>> -     struct of_phandle_args clkspec;
>>       struct property *prop;
>>       const char *clk_name;
>>       const __be32 *vp;
>>       u32 pv;
>> -     int rc;
>>       int count;
>> +     bool has_indices = false;
>>
>>       if (index < 0)
>>               return NULL;
>>
>> -     rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
>> -                                     &clkspec);
>> -     if (rc)
>> -             return NULL;
>> -
>> -     index = clkspec.args_count ? clkspec.args[0] : 0;
>>       count = 0;
>>
>>       /* if there is an indices property, use it to transfer the index
>>        * specified into an array offset for the clock-output-names property.
>>        */
>> -     of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
>> +     of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
>> +             has_indices = true;
>>               if (index == pv) {
>>                       index = count;
>>                       break;
>> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
>>               count++;
>>       }
>>
>> -     if (of_property_read_string_index(clkspec.np, "clock-output-names",
>> -                                       index,
>> -                                       &clk_name) < 0)
>> -             clk_name = clkspec.np->name;
>> +     if (of_property_read_string_index(np, "clock-output-names", index,
>> +                                       &clk_name) < 0) {
>> +             if (has_indices)
>> +                     return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);
>
> What if the clock provider has a #clock-cells larger than one ?

Right that is of course one issue. But according to the DT
documentation file clock-bindings.txt #clock-cells is typically set to
0 or 1.

> Another issue is that this won't guarantee that the names are unique as
> multiple DT nodes can have the same name. Instead of trying to generate unique
> names, would it be possible to handle clock registration and lookup without
> relying on names for DT-based platforms ?

It would of course make sense to do that for the long run, but at the
same time that sounds like major internal API rework since most
functions operate on string clock names today. So for short term is
the correct approach to use clock-output-names?

Cheers,

/ magnus

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

HI Laurent,

On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> This patch hacks the CCF core to take clock-indices into
>> consideration when making a default clock name in case
>> clock-output-names is not provided.
>>
>> Without this patch of_clk_get_parent_name() does not work
>> for clocks with multiple indices associated with one node.
>>
>> Proof of concept only. Leaks memory. Not for upstream merge.
>>
>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Written on top of "renesas-drivers-2015-09-08-v4.2"
>>  Needed to propagate clock frequencies from CPG to MSTP.
>>
>>  drivers/clk/clk.c                    |   46 +++++++++++++++++++------------
>>  drivers/clk/shmobile/clk-mstp.c      |    3 --
>>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
>>  include/linux/clk-provider.h         |    1
>>  4 files changed, 35 insertions(+), 28 deletions(-)
>>
>> --- 0001/drivers/clk/clk.c
>> +++ work/drivers/clk/clk.c    2015-09-09 13:48:21.992366518 +0900
>> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
>>  }
>>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
>>
>> -const char *of_clk_get_parent_name(struct device_node *np, int index)
>> +const char *of_clk_get_name(struct device_node *np, int index)
>>  {
>> -     struct of_phandle_args clkspec;
>>       struct property *prop;
>>       const char *clk_name;
>>       const __be32 *vp;
>>       u32 pv;
>> -     int rc;
>>       int count;
>> +     bool has_indices = false;
>>
>>       if (index < 0)
>>               return NULL;
>>
>> -     rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
>> -                                     &clkspec);
>> -     if (rc)
>> -             return NULL;
>> -
>> -     index = clkspec.args_count ? clkspec.args[0] : 0;
>>       count = 0;
>>
>>       /* if there is an indices property, use it to transfer the index
>>        * specified into an array offset for the clock-output-names property.
>>        */
>> -     of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
>> +     of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
>> +             has_indices = true;
>>               if (index = pv) {
>>                       index = count;
>>                       break;
>> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
>>               count++;
>>       }
>>
>> -     if (of_property_read_string_index(clkspec.np, "clock-output-names",
>> -                                       index,
>> -                                       &clk_name) < 0)
>> -             clk_name = clkspec.np->name;
>> +     if (of_property_read_string_index(np, "clock-output-names", index,
>> +                                       &clk_name) < 0) {
>> +             if (has_indices)
>> +                     return kasprintf(GFP_KERNEL, "%s.%u", np->name, index);
>
> What if the clock provider has a #clock-cells larger than one ?

Right that is of course one issue. But according to the DT
documentation file clock-bindings.txt #clock-cells is typically set to
0 or 1.

> Another issue is that this won't guarantee that the names are unique as
> multiple DT nodes can have the same name. Instead of trying to generate unique
> names, would it be possible to handle clock registration and lookup without
> relying on names for DT-based platforms ?

It would of course make sense to do that for the long run, but at the
same time that sounds like major internal API rework since most
functions operate on string clock names today. So for short term is
the correct approach to use clock-output-names?

Cheers,

/ magnus

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Laurent Pinchart]

Hi Magnus,

On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
> On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
> > On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
> >> From: Magnus Damm <damm+renesas@opensource.se>
> >> 
> >> This patch hacks the CCF core to take clock-indices into
> >> consideration when making a default clock name in case
> >> clock-output-names is not provided.
> >> 
> >> Without this patch of_clk_get_parent_name() does not work
> >> for clocks with multiple indices associated with one node.
> >> 
> >> Proof of concept only. Leaks memory. Not for upstream merge.
> >> 
> >> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >> ---
> >> 
> >>  Written on top of "renesas-drivers-2015-09-08-v4.2"
> >>  Needed to propagate clock frequencies from CPG to MSTP.
> >>  
> >>  drivers/clk/clk.c                    |   46 +++++++++++++++++-----------
> >>  drivers/clk/shmobile/clk-mstp.c      |    3 --
> >>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
> >>  include/linux/clk-provider.h         |    1
> >>  4 files changed, 35 insertions(+), 28 deletions(-)
> >> 
> >> --- 0001/drivers/clk/clk.c
> >> +++ work/drivers/clk/clk.c    2015-09-09 13:48:21.992366518 +0900
> >> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
> >>  }
> >>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
> >> 
> >> -const char *of_clk_get_parent_name(struct device_node *np, int index)
> >> +const char *of_clk_get_name(struct device_node *np, int index)
> >>  {
> >> -     struct of_phandle_args clkspec;
> >>       struct property *prop;
> >>       const char *clk_name;
> >>       const __be32 *vp;
> >>       u32 pv;
> >> -     int rc;
> >>       int count;
> >> +     bool has_indices = false;
> >> 
> >>       if (index < 0)
> >>               return NULL;
> >> 
> >> -     rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> >> index,
> >> -                                     &clkspec);
> >> -     if (rc)
> >> -             return NULL;
> >> -
> >> -     index = clkspec.args_count ? clkspec.args[0] : 0;
> >>       count = 0;
> >>       
> >>       /* if there is an indices property, use it to transfer the index
> >>        * specified into an array offset for the clock-output-names
> >>        property.
> >>        */
> >> -     of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv)
> >> {
> >> +     of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
> >> +             has_indices = true;
> >>               if (index == pv) {
> >>                       index = count;
> >>                       break;
> >> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
> >>               count++;
> >>       }
> >> 
> >> -     if (of_property_read_string_index(clkspec.np, "clock-output-names",
> >> -                                       index,
> >> -                                       &clk_name) < 0)
> >> -             clk_name = clkspec.np->name;
> >> +     if (of_property_read_string_index(np, "clock-output-names", index,
> >> +                                       &clk_name) < 0) {
> >> +             if (has_indices)
> >> +                     return kasprintf(GFP_KERNEL, "%s.%u", np->name,
> >> index);
> >
> > What if the clock provider has a #clock-cells larger than one ?
> 
> Right that is of course one issue. But according to the DT
> documentation file clock-bindings.txt #clock-cells is typically set to
> 0 or 1.

"Typically" :-)

We've discussed recently the possibility of using two cells for MSTP clocks.

> > Another issue is that this won't guarantee that the names are unique as
> > multiple DT nodes can have the same name. Instead of trying to generate
> > unique names, would it be possible to handle clock registration and
> > lookup without relying on names for DT-based platforms ?
> 
> It would of course make sense to do that for the long run, but at the
> same time that sounds like major internal API rework since most
> functions operate on string clock names today. So for short term is
> the correct approach to use clock-output-names?

I think Stephen and Mike should comment on that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Laurent Pinchart]

Hi Magnus,

On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
> On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
> > On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote:
> >> From: Magnus Damm <damm+renesas@opensource.se>
> >> 
> >> This patch hacks the CCF core to take clock-indices into
> >> consideration when making a default clock name in case
> >> clock-output-names is not provided.
> >> 
> >> Without this patch of_clk_get_parent_name() does not work
> >> for clocks with multiple indices associated with one node.
> >> 
> >> Proof of concept only. Leaks memory. Not for upstream merge.
> >> 
> >> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >> ---
> >> 
> >>  Written on top of "renesas-drivers-2015-09-08-v4.2"
> >>  Needed to propagate clock frequencies from CPG to MSTP.
> >>  
> >>  drivers/clk/clk.c                    |   46 +++++++++++++++++-----------
> >>  drivers/clk/shmobile/clk-mstp.c      |    3 --
> >>  drivers/clk/shmobile/clk-rcar-gen3.c |   13 +--------
> >>  include/linux/clk-provider.h         |    1
> >>  4 files changed, 35 insertions(+), 28 deletions(-)
> >> 
> >> --- 0001/drivers/clk/clk.c
> >> +++ work/drivers/clk/clk.c    2015-09-09 13:48:21.992366518 +0900
> >> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic
> >>  }
> >>  EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
> >> 
> >> -const char *of_clk_get_parent_name(struct device_node *np, int index)
> >> +const char *of_clk_get_name(struct device_node *np, int index)
> >>  {
> >> -     struct of_phandle_args clkspec;
> >>       struct property *prop;
> >>       const char *clk_name;
> >>       const __be32 *vp;
> >>       u32 pv;
> >> -     int rc;
> >>       int count;
> >> +     bool has_indices = false;
> >> 
> >>       if (index < 0)
> >>               return NULL;
> >> 
> >> -     rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> >> index,
> >> -                                     &clkspec);
> >> -     if (rc)
> >> -             return NULL;
> >> -
> >> -     index = clkspec.args_count ? clkspec.args[0] : 0;
> >>       count = 0;
> >>       
> >>       /* if there is an indices property, use it to transfer the index
> >>        * specified into an array offset for the clock-output-names
> >>        property.
> >>        */
> >> -     of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv)
> >> {
> >> +     of_property_for_each_u32(np, "clock-indices", prop, vp, pv) {
> >> +             has_indices = true;
> >>               if (index = pv) {
> >>                       index = count;
> >>                       break;
> >> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc
> >>               count++;
> >>       }
> >> 
> >> -     if (of_property_read_string_index(clkspec.np, "clock-output-names",
> >> -                                       index,
> >> -                                       &clk_name) < 0)
> >> -             clk_name = clkspec.np->name;
> >> +     if (of_property_read_string_index(np, "clock-output-names", index,
> >> +                                       &clk_name) < 0) {
> >> +             if (has_indices)
> >> +                     return kasprintf(GFP_KERNEL, "%s.%u", np->name,
> >> index);
> >
> > What if the clock provider has a #clock-cells larger than one ?
> 
> Right that is of course one issue. But according to the DT
> documentation file clock-bindings.txt #clock-cells is typically set to
> 0 or 1.

"Typically" :-)

We've discussed recently the possibility of using two cells for MSTP clocks.

> > Another issue is that this won't guarantee that the names are unique as
> > multiple DT nodes can have the same name. Instead of trying to generate
> > unique names, would it be possible to handle clock registration and
> > lookup without relying on names for DT-based platforms ?
> 
> It would of course make sense to do that for the long run, but at the
> same time that sounds like major internal API rework since most
> functions operate on string clock names today. So for short term is
> the correct approach to use clock-output-names?

I think Stephen and Mike should comment on that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Stephen Boyd]

On 09/09, Laurent Pinchart wrote:
> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
> > > Another issue is that this won't guarantee that the names are unique as
> > > multiple DT nodes can have the same name. Instead of trying to generate
> > > unique names, would it be possible to handle clock registration and
> > > lookup without relying on names for DT-based platforms ?
> > 
> > It would of course make sense to do that for the long run, but at the
> > same time that sounds like major internal API rework since most
> > functions operate on string clock names today. So for short term is
> > the correct approach to use clock-output-names?
> 
> I think Stephen and Mike should comment on that.
> 

We've been murmuring about moving away from string based parent
child relationship descriptions for some time now. Nothing very
concrete has come out though and I haven't thought about it in
too much detail.

Why can't we call clk_get() for the clocks that we need to find
the name of, and then call __clk_get_name() on them? Doing
clk_get() has the nice side-effect of ordering probe for
different clock controller drivers so that things like
suspend/resume are done in the correct order.

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

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Stephen Boyd]

On 09/09, Laurent Pinchart wrote:
> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
> > > Another issue is that this won't guarantee that the names are unique as
> > > multiple DT nodes can have the same name. Instead of trying to generate
> > > unique names, would it be possible to handle clock registration and
> > > lookup without relying on names for DT-based platforms ?
> > 
> > It would of course make sense to do that for the long run, but at the
> > same time that sounds like major internal API rework since most
> > functions operate on string clock names today. So for short term is
> > the correct approach to use clock-output-names?
> 
> I think Stephen and Mike should comment on that.
> 

We've been murmuring about moving away from string based parent
child relationship descriptions for some time now. Nothing very
concrete has come out though and I haven't thought about it in
too much detail.

Why can't we call clk_get() for the clocks that we need to find
the name of, and then call __clk_get_name() on them? Doing
clk_get() has the nice side-effect of ordering probe for
different clock controller drivers so that things like
suspend/resume are done in the correct order.

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

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Stephen,

On Wed, Sep 9, 2015 at 11:39 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/09, Laurent Pinchart wrote:
>> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
>> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
>> > > Another issue is that this won't guarantee that the names are unique as
>> > > multiple DT nodes can have the same name. Instead of trying to generate
>> > > unique names, would it be possible to handle clock registration and
>> > > lookup without relying on names for DT-based platforms ?
>> >
>> > It would of course make sense to do that for the long run, but at the
>> > same time that sounds like major internal API rework since most
>> > functions operate on string clock names today. So for short term is
>> > the correct approach to use clock-output-names?
>>
>> I think Stephen and Mike should comment on that.
>
> We've been murmuring about moving away from string based parent
> child relationship descriptions for some time now. Nothing very
> concrete has come out though and I haven't thought about it in
> too much detail.

If you want to get rid of the names, clocks could just be numbers, cfr.
interrupts.

Unfortunately debugging interrupt problems became more difficult with the
advent of IRQ domains and the loss of fixed interrupt numbers. as I can no
longer predict which is the right interrupt number for a specific device.

> Why can't we call clk_get() for the clocks that we need to find
> the name of, and then call __clk_get_name() on them? Doing

Who defines the name of the clock?
For e.g. clk-rcar-gen3.c that can be C code.
For e.g. clk-mstp.c we used to take them from "clock-output-names" (old),
or auto-generate them (new?).

> clk_get() has the nice side-effect of ordering probe for
> different clock controller drivers so that things like
> suspend/resume are done in the correct order.

That's true.

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][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Stephen,

On Wed, Sep 9, 2015 at 11:39 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/09, Laurent Pinchart wrote:
>> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
>> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
>> > > Another issue is that this won't guarantee that the names are unique as
>> > > multiple DT nodes can have the same name. Instead of trying to generate
>> > > unique names, would it be possible to handle clock registration and
>> > > lookup without relying on names for DT-based platforms ?
>> >
>> > It would of course make sense to do that for the long run, but at the
>> > same time that sounds like major internal API rework since most
>> > functions operate on string clock names today. So for short term is
>> > the correct approach to use clock-output-names?
>>
>> I think Stephen and Mike should comment on that.
>
> We've been murmuring about moving away from string based parent
> child relationship descriptions for some time now. Nothing very
> concrete has come out though and I haven't thought about it in
> too much detail.

If you want to get rid of the names, clocks could just be numbers, cfr.
interrupts.

Unfortunately debugging interrupt problems became more difficult with the
advent of IRQ domains and the loss of fixed interrupt numbers. as I can no
longer predict which is the right interrupt number for a specific device.

> Why can't we call clk_get() for the clocks that we need to find
> the name of, and then call __clk_get_name() on them? Doing

Who defines the name of the clock?
For e.g. clk-rcar-gen3.c that can be C code.
For e.g. clk-mstp.c we used to take them from "clock-output-names" (old),
or auto-generate them (new?).

> clk_get() has the nice side-effect of ordering probe for
> different clock controller drivers so that things like
> suspend/resume are done in the correct order.

That's true.

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][RFC] clk: Use node name and index for clock name

[Magnus Damm]

Hi Stephen,

Thanks for your feedback!

On Thu, Sep 10, 2015 at 6:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/09, Laurent Pinchart wrote:
>> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
>> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
>> > > Another issue is that this won't guarantee that the names are unique as
>> > > multiple DT nodes can have the same name. Instead of trying to generate
>> > > unique names, would it be possible to handle clock registration and
>> > > lookup without relying on names for DT-based platforms ?
>> >
>> > It would of course make sense to do that for the long run, but at the
>> > same time that sounds like major internal API rework since most
>> > functions operate on string clock names today. So for short term is
>> > the correct approach to use clock-output-names?
>>
>> I think Stephen and Mike should comment on that.
>>
>
> We've been murmuring about moving away from string based parent
> child relationship descriptions for some time now. Nothing very
> concrete has come out though and I haven't thought about it in
> too much detail.

My half-cooked attempt to fix this was posted yesterday:
[PATCH 00/05][RFC] Clock registration without parent name
http://www.spinics.net/lists/linux-clk/msg02960.html

If you have any ideas how that series can be beaten into something you
like then please let me know.

> Why can't we call clk_get() for the clocks that we need to find
> the name of, and then call __clk_get_name() on them? Doing
> clk_get() has the nice side-effect of ordering probe for
> different clock controller drivers so that things like
> suspend/resume are done in the correct order.

I think __get_clk_name() makes sense in general as long as we can
produce some sane names for the clocks while registering.

In my mind the choices we have for naming clocks for registration are:
a) Based on the DT clock-output-names property
b) Hard coded string defined in C code
c) Built on DT node name and DT clock-indices property (if present -
and no clock-output-names in DT)

Maybe there are other options? I don't mind so much in general which
one to go with, but at the same time I can see the beauty of not
making the DT clock-output-names property mandatory from a DT
perspective. So I like the approach of defaulting to c) but allow DT
to override using a). In my mind all of a) -> c) above should work
with the core clock code. Please let me know if my understanding is
wrong.

Like mentioned in earlier email, the case c) above does not work when
using of_clk_get_parent_name(). Especially the function
of_fixed_factor_clk_setup() tries to point out the parent using
of_clk_get_parent_name() which prevents it from working if the parent
clock is named following the style in c). So we cannot use fixed
factor clocks without local patches if we go with c)..

As for clk_get() vs of_clk_get() (that my patch series above is
using), this seems to be tightly related to the driver model and how
to register clocks. clk_get() takes a struct device pointer while
of_clk_get() takes a struct device_node pointer.

I've always been a fan of using the standard driver model (initially
with platform devices with platform data, now DT). Over the years
various DT-specific init methods have creeped into the kernel - I
clearly recall such ones for clocksources, irqchip and clocks, but
gpio and pinctrl may also be affected. I've never really understood
the reason behind them - shouldn't using the regular driver model with
initcall order, linking order and deferred probing be enough?

Anyway, if you have time please share your ideas about naming a) - >c)
above and what your expectation is for new drivers. Same goes for
driver model usage - do you want us to move away from of_clk_init() to
regular driver model? of_clk_init() seems to manage probe order
correctly what I can tell, not sure about suspend/resume though.

>From my side I mainly want to make our DT interface stable. So either
we use clock-output-names in DT to begin with and fix things
incrementally, or we omit clock-output-names from DT from the
beginning but if so we need to fix the clock core code so we can use
c) (or any other options).

Thanks for your help,

/ magnus

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

Hi Stephen,

Thanks for your feedback!

On Thu, Sep 10, 2015 at 6:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/09, Laurent Pinchart wrote:
>> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote:
>> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote:
>> > > Another issue is that this won't guarantee that the names are unique as
>> > > multiple DT nodes can have the same name. Instead of trying to generate
>> > > unique names, would it be possible to handle clock registration and
>> > > lookup without relying on names for DT-based platforms ?
>> >
>> > It would of course make sense to do that for the long run, but at the
>> > same time that sounds like major internal API rework since most
>> > functions operate on string clock names today. So for short term is
>> > the correct approach to use clock-output-names?
>>
>> I think Stephen and Mike should comment on that.
>>
>
> We've been murmuring about moving away from string based parent
> child relationship descriptions for some time now. Nothing very
> concrete has come out though and I haven't thought about it in
> too much detail.

My half-cooked attempt to fix this was posted yesterday:
[PATCH 00/05][RFC] Clock registration without parent name
http://www.spinics.net/lists/linux-clk/msg02960.html

If you have any ideas how that series can be beaten into something you
like then please let me know.

> Why can't we call clk_get() for the clocks that we need to find
> the name of, and then call __clk_get_name() on them? Doing
> clk_get() has the nice side-effect of ordering probe for
> different clock controller drivers so that things like
> suspend/resume are done in the correct order.

I think __get_clk_name() makes sense in general as long as we can
produce some sane names for the clocks while registering.

In my mind the choices we have for naming clocks for registration are:
a) Based on the DT clock-output-names property
b) Hard coded string defined in C code
c) Built on DT node name and DT clock-indices property (if present -
and no clock-output-names in DT)

Maybe there are other options? I don't mind so much in general which
one to go with, but at the same time I can see the beauty of not
making the DT clock-output-names property mandatory from a DT
perspective. So I like the approach of defaulting to c) but allow DT
to override using a). In my mind all of a) -> c) above should work
with the core clock code. Please let me know if my understanding is
wrong.

Like mentioned in earlier email, the case c) above does not work when
using of_clk_get_parent_name(). Especially the function
of_fixed_factor_clk_setup() tries to point out the parent using
of_clk_get_parent_name() which prevents it from working if the parent
clock is named following the style in c). So we cannot use fixed
factor clocks without local patches if we go with c)..

As for clk_get() vs of_clk_get() (that my patch series above is
using), this seems to be tightly related to the driver model and how
to register clocks. clk_get() takes a struct device pointer while
of_clk_get() takes a struct device_node pointer.

I've always been a fan of using the standard driver model (initially
with platform devices with platform data, now DT). Over the years
various DT-specific init methods have creeped into the kernel - I
clearly recall such ones for clocksources, irqchip and clocks, but
gpio and pinctrl may also be affected. I've never really understood
the reason behind them - shouldn't using the regular driver model with
initcall order, linking order and deferred probing be enough?

Anyway, if you have time please share your ideas about naming a) - >c)
above and what your expectation is for new drivers. Same goes for
driver model usage - do you want us to move away from of_clk_init() to
regular driver model? of_clk_init() seems to manage probe order
correctly what I can tell, not sure about suspend/resume though.

From my side I mainly want to make our DT interface stable. So either
we use clock-output-names in DT to begin with and fix things
incrementally, or we omit clock-output-names from DT from the
beginning but if so we need to fix the clock core code so we can use
c) (or any other options).

Thanks for your help,

/ magnus

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> This patch hacks the CCF core to take clock-indices into
> consideration when making a default clock name in case
> clock-output-names is not provided.
>
> Without this patch of_clk_get_parent_name() does not work
> for clocks with multiple indices associated with one node.
>
> Proof of concept only. Leaks memory. Not for upstream merge.
>
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

While this (probably) solves the issue, is this something we want to do?
What does we gain? E.g. we still need to allocate memory to store the clock
names, so no memory is saved by not providing clock-output-names.
But instead of useful names, the clocks now have autogenerated names, and all
look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)
Clocks are too critical to make mistakes, so having useful names matters a lot.

I'm still in favor of keeping clock-output-names for device nodes that
provide multiple clock outputs.

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][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> This patch hacks the CCF core to take clock-indices into
> consideration when making a default clock name in case
> clock-output-names is not provided.
>
> Without this patch of_clk_get_parent_name() does not work
> for clocks with multiple indices associated with one node.
>
> Proof of concept only. Leaks memory. Not for upstream merge.
>
> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

While this (probably) solves the issue, is this something we want to do?
What does we gain? E.g. we still need to allocate memory to store the clock
names, so no memory is saved by not providing clock-output-names.
But instead of useful names, the clocks now have autogenerated names, and all
look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)
Clocks are too critical to make mistakes, so having useful names matters a lot.

I'm still in favor of keeping clock-output-names for device nodes that
provide multiple clock outputs.

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][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> This patch hacks the CCF core to take clock-indices into
>> consideration when making a default clock name in case
>> clock-output-names is not provided.
>>
>> Without this patch of_clk_get_parent_name() does not work
>> for clocks with multiple indices associated with one node.
>>
>> Proof of concept only. Leaks memory. Not for upstream merge.
>>
>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>
> While this (probably) solves the issue, is this something we want to do?
> What does we gain? E.g. we still need to allocate memory to store the clock
> names, so no memory is saved by not providing clock-output-names.
> But instead of useful names, the clocks now have autogenerated names, and all
> look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)

Actually it's even worse: the autogenerated number is not the MSTP bit
number, which I can relate to the actual clock using
include/dt-bindings/clock/*.h, but the index in the sparse array of used bits,
which depends on how many MSTP clocks are defined in DT.

So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3",
aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3").

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][RFC] clk: Use node name and index for clock name

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> This patch hacks the CCF core to take clock-indices into
>> consideration when making a default clock name in case
>> clock-output-names is not provided.
>>
>> Without this patch of_clk_get_parent_name() does not work
>> for clocks with multiple indices associated with one node.
>>
>> Proof of concept only. Leaks memory. Not for upstream merge.
>>
>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>
> While this (probably) solves the issue, is this something we want to do?
> What does we gain? E.g. we still need to allocate memory to store the clock
> names, so no memory is saved by not providing clock-output-names.
> But instead of useful names, the clocks now have autogenerated names, and all
> look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)

Actually it's even worse: the autogenerated number is not the MSTP bit
number, which I can relate to the actual clock using
include/dt-bindings/clock/*.h, but the index in the sparse array of used bits,
which depends on how many MSTP clocks are defined in DT.

So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3",
aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3").

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][RFC] clk: Use node name and index for clock name

[Magnus Damm]

Hi Geert,

On Mon, Sep 14, 2015 at 9:02 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> This patch hacks the CCF core to take clock-indices into
>>> consideration when making a default clock name in case
>>> clock-output-names is not provided.
>>>
>>> Without this patch of_clk_get_parent_name() does not work
>>> for clocks with multiple indices associated with one node.
>>>
>>> Proof of concept only. Leaks memory. Not for upstream merge.
>>>
>>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>
>> While this (probably) solves the issue, is this something we want to do?
>> What does we gain? E.g. we still need to allocate memory to store the clock
>> names, so no memory is saved by not providing clock-output-names.
>> But instead of useful names, the clocks now have autogenerated names, and all
>> look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)
>
> Actually it's even worse: the autogenerated number is not the MSTP bit
> number, which I can relate to the actual clock using
> include/dt-bindings/clock/*.h, but the index in the sparse array of used bits,
> which depends on how many MSTP clocks are defined in DT.
>
> So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3",
> aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3").

Thanks for letting me know. It needs to be fixed for sure.

Cheers,

/ magnus

Re: [PATCH][RFC] clk: Use node name and index for clock name

[Magnus Damm]

Hi Geert,

On Mon, Sep 14, 2015 at 9:02 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> This patch hacks the CCF core to take clock-indices into
>>> consideration when making a default clock name in case
>>> clock-output-names is not provided.
>>>
>>> Without this patch of_clk_get_parent_name() does not work
>>> for clocks with multiple indices associated with one node.
>>>
>>> Proof of concept only. Leaks memory. Not for upstream merge.
>>>
>>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>
>> While this (probably) solves the issue, is this something we want to do?
>> What does we gain? E.g. we still need to allocate memory to store the clock
>> names, so no memory is saved by not providing clock-output-names.
>> But instead of useful names, the clocks now have autogenerated names, and all
>> look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-)
>
> Actually it's even worse: the autogenerated number is not the MSTP bit
> number, which I can relate to the actual clock using
> include/dt-bindings/clock/*.h, but the index in the sparse array of used bits,
> which depends on how many MSTP clocks are defined in DT.
>
> So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3",
> aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3").

Thanks for letting me know. It needs to be fixed for sure.

Cheers,

/ magnus