[PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Naman Trivedi]

From: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com>

Use struct clk_parent_data to register the clock parents with the clock
framework instead of parent name.

Signed-off-by: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com>
---
V1: https://lore.kernel.org/lkml/20240103072017.1646007-1-naman.trivedimanojbhai@amd.com
V1 -> V2:
- Used struct clk_parent_data instead of parent names to register clock
  parents with the clock framework
---
 drivers/clk/zynqmp/clk-gate-zynqmp.c |  8 +--
 drivers/clk/zynqmp/clk-mux-zynqmp.c  |  9 +--
 drivers/clk/zynqmp/clk-zynqmp.h      | 26 ++++-----
 drivers/clk/zynqmp/clkc.c            | 83 +++++++++++++++++++---------
 drivers/clk/zynqmp/divider.c         |  8 +--
 drivers/clk/zynqmp/pll.c             |  9 +--
 6 files changed, 89 insertions(+), 54 deletions(-)

diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c b/drivers/clk/zynqmp/clk-gate-zynqmp.c
index b89e55737198..6bb9704ee1d3 100644
--- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
@@ -104,8 +104,8 @@ static const struct clk_ops zynqmp_clk_gate_ops = {
  *
  * Return: clock hardware of the registered clock gate
  */
-struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
-					const char * const *parents,
+struct clk_hw *zynqmp_clk_register_gate(struct device_node *np, const char *name, u32 clk_id,
+					const struct clk_parent_data *parents,
 					u8 num_parents,
 					const struct clock_topology *nodes)
 {
@@ -124,7 +124,7 @@ struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
 
 	init.flags = zynqmp_clk_map_common_ccf_flags(nodes->flag);
 
-	init.parent_names = parents;
+	init.parent_data = parents;
 	init.num_parents = 1;
 
 	/* struct clk_gate assignments */
@@ -133,7 +133,7 @@ struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
 	gate->clk_id = clk_id;
 
 	hw = &gate->hw;
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(gate);
 		hw = ERR_PTR(ret);
diff --git a/drivers/clk/zynqmp/clk-mux-zynqmp.c b/drivers/clk/zynqmp/clk-mux-zynqmp.c
index 9b5d3050b742..30daf1f77b4c 100644
--- a/drivers/clk/zynqmp/clk-mux-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-mux-zynqmp.c
@@ -128,8 +128,9 @@ static inline unsigned long zynqmp_clk_map_mux_ccf_flags(
  *
  * Return: clock hardware of the registered clock mux
  */
-struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
-				       const char * const *parents,
+struct clk_hw *zynqmp_clk_register_mux(struct device_node *np,
+				       const char *name, u32 clk_id,
+				       const struct clk_parent_data *parents,
 				       u8 num_parents,
 				       const struct clock_topology *nodes)
 {
@@ -150,14 +151,14 @@ struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
 
 	init.flags = zynqmp_clk_map_common_ccf_flags(nodes->flag);
 
-	init.parent_names = parents;
+	init.parent_data = parents;
 	init.num_parents = num_parents;
 	mux->flags = zynqmp_clk_map_mux_ccf_flags(nodes->type_flag);
 	mux->hw.init = &init;
 	mux->clk_id = clk_id;
 
 	hw = &mux->hw;
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(mux);
 		hw = ERR_PTR(ret);
diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-zynqmp.h
index 60cbc0674a9e..6343cfb57a4f 100644
--- a/drivers/clk/zynqmp/clk-zynqmp.h
+++ b/drivers/clk/zynqmp/clk-zynqmp.h
@@ -67,31 +67,31 @@ struct clock_topology {
 
 unsigned long zynqmp_clk_map_common_ccf_flags(const u32 zynqmp_flag);
 
-struct clk_hw *zynqmp_clk_register_pll(const char *name, u32 clk_id,
-				       const char * const *parents,
+struct clk_hw *zynqmp_clk_register_pll(struct device_node *np, const char *name, u32 clk_id,
+				       const struct clk_parent_data *parents,
 				       u8 num_parents,
 				       const struct clock_topology *nodes);
 
-struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
-					const char * const *parents,
+struct clk_hw *zynqmp_clk_register_gate(struct device_node *np, const char *name, u32 clk_id,
+					const struct clk_parent_data *parents,
 					u8 num_parents,
 					const struct clock_topology *nodes);
 
-struct clk_hw *zynqmp_clk_register_divider(const char *name,
+struct clk_hw *zynqmp_clk_register_divider(struct device_node *np, const char *name,
 					   u32 clk_id,
-					   const char * const *parents,
+					   const struct clk_parent_data *parents,
 					   u8 num_parents,
 					   const struct clock_topology *nodes);
 
-struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
-				       const char * const *parents,
+struct clk_hw *zynqmp_clk_register_mux(struct device_node *np, const char *name, u32 clk_id,
+				       const struct clk_parent_data *parents,
 				       u8 num_parents,
 				       const struct clock_topology *nodes);
 
-struct clk_hw *zynqmp_clk_register_fixed_factor(const char *name,
-					u32 clk_id,
-					const char * const *parents,
-					u8 num_parents,
-					const struct clock_topology *nodes);
+struct clk_hw *zynqmp_clk_register_fixed_factor(struct device_node *np, const char *name,
+						u32 clk_id,
+						const struct clk_parent_data *parents,
+						u8 num_parents,
+						const struct clock_topology *nodes);
 
 #endif
diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index a91d98e238c2..b791a459280e 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -12,6 +12,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -119,11 +120,10 @@ static const char clk_type_postfix[][10] = {
 	[TYPE_PLL] = ""
 };
 
-static struct clk_hw *(* const clk_topology[]) (const char *name, u32 clk_id,
-					const char * const *parents,
-					u8 num_parents,
-					const struct clock_topology *nodes)
-					= {
+static struct clk_hw *(* const clk_topology[]) (struct device_node *np, const char *name,
+						u32 clk_id, const struct clk_parent_data *parents,
+						u8 num_parents,
+						const struct clock_topology *nodes) = {
 	[TYPE_INVALID] = NULL,
 	[TYPE_MUX] = zynqmp_clk_register_mux,
 	[TYPE_PLL] = zynqmp_clk_register_pll,
@@ -307,14 +307,16 @@ unsigned long zynqmp_clk_map_common_ccf_flags(const u32 zynqmp_flag)
  *
  * Return: clock hardware to the registered clock
  */
-struct clk_hw *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
-					const char * const *parents,
-					u8 num_parents,
-					const struct clock_topology *nodes)
+struct clk_hw *zynqmp_clk_register_fixed_factor(struct device_node *np, const char *name,
+						u32 clk_id,
+						const struct clk_parent_data *parents,
+						u8 num_parents,
+						const struct clock_topology *nodes)
 {
 	u32 mult, div;
 	struct clk_hw *hw;
 	struct zynqmp_pm_query_data qdata = {0};
+	struct platform_device *plat_dev;
 	u32 ret_payload[PAYLOAD_ARG_CNT];
 	int ret;
 	unsigned long flag;
@@ -331,10 +333,19 @@ struct clk_hw *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
 
 	flag = zynqmp_clk_map_common_ccf_flags(nodes->flag);
 
-	hw = clk_hw_register_fixed_factor(NULL, name,
-					  parents[0],
-					  flag, mult,
-					  div);
+	plat_dev = of_find_device_by_node(np);
+	if (!plat_dev)
+		return NULL;
+
+	if (parents->name)
+		hw = clk_hw_register_fixed_factor(NULL, name,
+						  parents->name,
+						  flag, mult,
+						  div);
+	else
+		hw = devm_clk_hw_register_fixed_factor_index(&plat_dev->dev, name,
+							     parents->index,
+							     flag, mult, div);
 
 	return hw;
 }
@@ -543,7 +554,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
  * Return: 0 on success else error+reason
  */
 static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
-				  const char **parent_list, u32 *num_parents)
+				  struct clk_parent_data *parent_list, u32 *num_parents)
 {
 	int i = 0, ret;
 	u32 total_parents = clock[clk_id].num_parents;
@@ -555,18 +566,30 @@ static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
 
 	for (i = 0; i < total_parents; i++) {
 		if (!parents[i].flag) {
-			parent_list[i] = parents[i].name;
+			ret = of_property_match_string(np, "clock-names",
+						       parents[i].name);
+			if (ret >= 0) {
+				parent_list[i].index = ret;
+			} else {
+				parent_list[i].fw_name = parents[i].name;
+				parent_list[i].name = parents[i].name;
+			}
 		} else if (parents[i].flag == PARENT_CLK_EXTERNAL) {
 			ret = of_property_match_string(np, "clock-names",
 						       parents[i].name);
-			if (ret < 0)
+			if (ret >= 0) {
+				parent_list[i].index = ret;
+			} else {
 				strcpy(parents[i].name, "dummy_name");
-			parent_list[i] = parents[i].name;
+				parent_list[i].fw_name = parents[i].name;
+				parent_list[i].name = parents[i].name;
+			}
 		} else {
 			strcat(parents[i].name,
 			       clk_type_postfix[clk_nodes[parents[i].flag - 1].
 			       type]);
-			parent_list[i] = parents[i].name;
+			parent_list[i].fw_name = parents[i].name;
+			parent_list[i].name = parents[i].name;
 		}
 	}
 
@@ -583,9 +606,9 @@ static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
  *
  * Return: Returns either clock hardware or error+reason
  */
-static struct clk_hw *zynqmp_register_clk_topology(int clk_id, char *clk_name,
-						   int num_parents,
-						   const char **parent_names)
+static struct clk_hw *zynqmp_register_clk_topology(struct device_node *np, int clk_id,
+						   char *clk_name, int num_parents,
+						   struct clk_parent_data *parent_names)
 {
 	int j;
 	u32 num_nodes, clk_dev_id;
@@ -612,7 +635,7 @@ static struct clk_hw *zynqmp_register_clk_topology(int clk_id, char *clk_name,
 		if (!clk_topology[nodes[j].type])
 			continue;
 
-		hw = (*clk_topology[nodes[j].type])(clk_out[j], clk_dev_id,
+		hw = (*clk_topology[nodes[j].type])(np, clk_out[j], clk_dev_id,
 						    parent_names,
 						    num_parents,
 						    &nodes[j]);
@@ -621,7 +644,10 @@ static struct clk_hw *zynqmp_register_clk_topology(int clk_id, char *clk_name,
 				     __func__,  clk_dev_id, clk_name,
 				     PTR_ERR(hw));
 
-		parent_names[0] = clk_out[j];
+		if (parent_names->fw_name) {
+			parent_names->name = clk_out[j];
+			parent_names->fw_name = clk_out[j];
+		}
 	}
 
 	for (j = 0; j < num_nodes; j++)
@@ -640,9 +666,14 @@ static int zynqmp_register_clocks(struct device_node *np)
 {
 	int ret;
 	u32 i, total_parents = 0, type = 0;
-	const char *parent_names[MAX_PARENT];
+	struct clk_parent_data *parent_names;
+
+	parent_names = kmalloc(sizeof(*parent_names) * MAX_PARENT, GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
 
 	for (i = 0; i < clock_max_idx; i++) {
+		memset(parent_names, 0, sizeof(struct clk_parent_data) * MAX_PARENT);
 		char clk_name[MAX_NAME_LEN];
 
 		/* get clock name, continue to next clock if name not found */
@@ -665,7 +696,7 @@ static int zynqmp_register_clocks(struct device_node *np)
 		}
 
 		zynqmp_data->hws[i] =
-			zynqmp_register_clk_topology(i, clk_name,
+			zynqmp_register_clk_topology(np, i, clk_name,
 						     total_parents,
 						     parent_names);
 	}
@@ -677,6 +708,8 @@ static int zynqmp_register_clocks(struct device_node *np)
 			WARN_ON(1);
 		}
 	}
+
+	kfree(parent_names);
 	return 0;
 }
 
diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
index 5a00487ae408..7c0905be2f26 100644
--- a/drivers/clk/zynqmp/divider.c
+++ b/drivers/clk/zynqmp/divider.c
@@ -269,9 +269,9 @@ static inline unsigned long zynqmp_clk_map_divider_ccf_flags(
  *
  * Return: clock hardware to registered clock divider
  */
-struct clk_hw *zynqmp_clk_register_divider(const char *name,
+struct clk_hw *zynqmp_clk_register_divider(struct device_node *np, const char *name,
 					   u32 clk_id,
-					   const char * const *parents,
+					   const struct clk_parent_data *parents,
 					   u8 num_parents,
 					   const struct clock_topology *nodes)
 {
@@ -293,7 +293,7 @@ struct clk_hw *zynqmp_clk_register_divider(const char *name,
 
 	init.flags = zynqmp_clk_map_common_ccf_flags(nodes->flag);
 
-	init.parent_names = parents;
+	init.parent_data = parents;
 	init.num_parents = 1;
 
 	/* struct clk_divider assignments */
@@ -311,7 +311,7 @@ struct clk_hw *zynqmp_clk_register_divider(const char *name,
 	div->max_div = zynqmp_clk_get_max_divisor(clk_id, nodes->type);
 
 	hw = &div->hw;
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(div);
 		hw = ERR_PTR(ret);
diff --git a/drivers/clk/zynqmp/pll.c b/drivers/clk/zynqmp/pll.c
index 7411a7fd50ac..4bd93efed9f2 100644
--- a/drivers/clk/zynqmp/pll.c
+++ b/drivers/clk/zynqmp/pll.c
@@ -309,8 +309,9 @@ static const struct clk_ops zynqmp_pll_ops = {
  *
  * Return: clock hardware to the registered clock
  */
-struct clk_hw *zynqmp_clk_register_pll(const char *name, u32 clk_id,
-				       const char * const *parents,
+struct clk_hw *zynqmp_clk_register_pll(struct device_node *np,
+				       const char *name, u32 clk_id,
+				       const struct clk_parent_data *parents,
 				       u8 num_parents,
 				       const struct clock_topology *nodes)
 {
@@ -324,7 +325,7 @@ struct clk_hw *zynqmp_clk_register_pll(const char *name, u32 clk_id,
 
 	init.flags = zynqmp_clk_map_common_ccf_flags(nodes->flag);
 
-	init.parent_names = parents;
+	init.parent_data = parents;
 	init.num_parents = 1;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
@@ -335,7 +336,7 @@ struct clk_hw *zynqmp_clk_register_pll(const char *name, u32 clk_id,
 	pll->clk_id = clk_id;
 
 	hw = &pll->hw;
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(pll);
 		return ERR_PTR(ret);
-- 
2.25.1

Re: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Stephen Boyd]

Quoting Naman Trivedi (2024-07-22 05:19:10)
> From: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com>
> 
> Use struct clk_parent_data to register the clock parents with the clock
> framework instead of parent name.
> 
> Signed-off-by: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com>

This is great! Thanks for doing this.

> diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c b/drivers/clk/zynqmp/clk-gate-zynqmp.c
> index b89e55737198..6bb9704ee1d3 100644
> --- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
> +++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
> @@ -104,8 +104,8 @@ static const struct clk_ops zynqmp_clk_gate_ops = {
>   *
>   * Return: clock hardware of the registered clock gate
>   */
> -struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
> -                                       const char * const *parents,
> +struct clk_hw *zynqmp_clk_register_gate(struct device_node *np, const char *name, u32 clk_id,

General comment: Please use 'struct device' instead so that this driver
isn't DT specific. When you do that you can similarly use
devm_clk_hw_register() instead and introduce a lot of automatic cleanup.
If you want to do that in two steps that's fine. One patch that uses
parent_data/parent_hws and one that uses devm_ APIs and struct device to
register.

> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index a91d98e238c2..b791a459280e 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -543,7 +554,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
>   * Return: 0 on success else error+reason
>   */
>  static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
> -                                 const char **parent_list, u32 *num_parents)
> +                                 struct clk_parent_data *parent_list, u32 *num_parents)
>  {
>         int i = 0, ret;
>         u32 total_parents = clock[clk_id].num_parents;
> @@ -555,18 +566,30 @@ static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
>  
>         for (i = 0; i < total_parents; i++) {
>                 if (!parents[i].flag) {
> -                       parent_list[i] = parents[i].name;
> +                       ret = of_property_match_string(np, "clock-names",
> +                                                      parents[i].name);

You shouldn't need to match 'clock-names'. The order of that property is
fixed in the binding, which means you can simply use the index that the
name is at in the binding in 'struct parent_data'.

RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Trivedi Manojbhai, Naman]

Hi Stephen,

Thanks for review. Please find my response inline.

>-----Original Message-----
>From: Stephen Boyd <sboyd@kernel.org>
>Sent: Friday, August 9, 2024 4:18 AM
>To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; linux-
>arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Simek, Michal
><michal.simek@amd.com>; mturquette@baylibre.com; Thangaraj, Senthil
>Nathan <SenthilNathan.Thangaraj@amd.com>
>Cc: linux-kernel@vger.kernel.org; Trivedi Manojbhai, Naman
><Naman.TrivediManojbhai@amd.com>
>Subject: Re: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency
>
>Caution: This message originated from an External Source. Use proper caution
>when opening attachments, clicking links, or responding.
>
>
>Quoting Naman Trivedi (2024-07-22 05:19:10)
>> From: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com>
>>
>> Use struct clk_parent_data to register the clock parents with the
>> clock framework instead of parent name.
>>
>> Signed-off-by: Naman Trivedi Manojbhai
>> <naman.trivedimanojbhai@amd.com>
>
>This is great! Thanks for doing this.
>
>> diff --git a/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> b/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> index b89e55737198..6bb9704ee1d3 100644
>> --- a/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> +++ b/drivers/clk/zynqmp/clk-gate-zynqmp.c
>> @@ -104,8 +104,8 @@ static const struct clk_ops zynqmp_clk_gate_ops = {
>>   *
>>   * Return: clock hardware of the registered clock gate
>>   */
>> -struct clk_hw *zynqmp_clk_register_gate(const char *name, u32 clk_id,
>> -                                       const char * const *parents,
>> +struct clk_hw *zynqmp_clk_register_gate(struct device_node *np, const
>> +char *name, u32 clk_id,
>
>General comment: Please use 'struct device' instead so that this driver isn't DT
>specific. When you do that you can similarly use
>devm_clk_hw_register() instead and introduce a lot of automatic cleanup.
>If you want to do that in two steps that's fine. One patch that uses
>parent_data/parent_hws and one that uses devm_ APIs and struct device to
>register.

Thanks for suggestion. I will check and implement this.
>
>> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
>> index a91d98e238c2..b791a459280e 100644
>> --- a/drivers/clk/zynqmp/clkc.c
>> +++ b/drivers/clk/zynqmp/clkc.c
>> @@ -543,7 +554,7 @@ static int zynqmp_clock_get_parents(u32 clk_id,
>struct clock_parent *parents,
>>   * Return: 0 on success else error+reason
>>   */
>>  static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
>> -                                 const char **parent_list, u32 *num_parents)
>> +                                 struct clk_parent_data *parent_list,
>> + u32 *num_parents)
>>  {
>>         int i = 0, ret;
>>         u32 total_parents = clock[clk_id].num_parents; @@ -555,18
>> +566,30 @@ static int zynqmp_get_parent_list(struct device_node *np,
>> u32 clk_id,
>>
>>         for (i = 0; i < total_parents; i++) {
>>                 if (!parents[i].flag) {
>> -                       parent_list[i] = parents[i].name;
>> +                       ret = of_property_match_string(np, "clock-names",
>> +
>> + parents[i].name);
>
>You shouldn't need to match 'clock-names'. The order of that property is fixed
>in the binding, which means you can simply use the index that the name is at
>in the binding in 'struct parent_data'.

This driver is common across multiple device families, and each device has different set of clock names in device tree/binding.  This implementation seemed to be generic for all devices. 
To use index directly, I have to add if..else for matching compatible strings and more if..else inside each of them for matching clock names to find index. Please let me know if this is preferred approach.

RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Stephen Boyd]

Quoting Trivedi Manojbhai, Naman (2024-08-12 05:57:13)
> >> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> >> index a91d98e238c2..b791a459280e 100644
> >> --- a/drivers/clk/zynqmp/clkc.c
> >> +++ b/drivers/clk/zynqmp/clkc.c
> >> @@ -543,7 +554,7 @@ static int zynqmp_clock_get_parents(u32 clk_id,
> >struct clock_parent *parents,
> >>   * Return: 0 on success else error+reason
> >>   */
> >>  static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id,
> >> -                                 const char **parent_list, u32 *num_parents)
> >> +                                 struct clk_parent_data *parent_list,
> >> + u32 *num_parents)
> >>  {
> >>         int i = 0, ret;
> >>         u32 total_parents = clock[clk_id].num_parents; @@ -555,18
> >> +566,30 @@ static int zynqmp_get_parent_list(struct device_node *np,
> >> u32 clk_id,
> >>
> >>         for (i = 0; i < total_parents; i++) {
> >>                 if (!parents[i].flag) {
> >> -                       parent_list[i] = parents[i].name;
> >> +                       ret = of_property_match_string(np, "clock-names",
> >> +
> >> + parents[i].name);
> >
> >You shouldn't need to match 'clock-names'. The order of that property is fixed
> >in the binding, which means you can simply use the index that the name is at
> >in the binding in 'struct parent_data'.
> 
> This driver is common across multiple device families, and each device has different set of clock names in device tree/binding.  This implementation seemed to be generic for all devices. 
> To use index directly, I have to add if..else for matching compatible strings and more if..else inside each of them for matching clock names to find index. Please let me know if this is preferred approach.

It is preferred to not use clock-names and use the index directly. This
avoids a bunch of string comparisons and makes for smaller and faster
code.

RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Trivedi Manojbhai, Naman]

Hi Stephen,

Thanks for detailed explanation. I am trying to understand your suggestions and have queries for a few comments. Please find inline response.

>-----Original Message-----
>From: Stephen Boyd <sboyd@kernel.org>
>Sent: Friday, August 30, 2024 12:33 AM
>To: Simek, Michal <michal.simek@amd.com>; Thangaraj, Senthil Nathan
><SenthilNathan.Thangaraj@amd.com>; Trivedi Manojbhai, Naman
><Naman.TrivediManojbhai@amd.com>; linux-arm-kernel@lists.infradead.org;
>linux-clk@vger.kernel.org; mturquette@baylibre.com
>Cc: linux-kernel@ <"vger.kernel.org linux-kernel"@vger.kernel.org>
>Subject: RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency
>
>Caution: This message originated from an External Source. Use proper caution
>when opening attachments, clicking links, or responding.
>
>
>Quoting Trivedi Manojbhai, Naman (2024-08-28 22:54:16)
>> >> >> +566,30 @@ static int zynqmp_get_parent_list(struct device_node
>> >> >> +*np,
>> >> >> u32 clk_id,
>> >> >>
>> >> >>         for (i = 0; i < total_parents; i++) {
>> >> >>                 if (!parents[i].flag) {
>> >> >> -                       parent_list[i] = parents[i].name;
>> >> >> +                       ret = of_property_match_string(np,
>> >> >> + "clock-names",
>> >> >> +
>> >> >> + parents[i].name);
>> >> >
>> >> >You shouldn't need to match 'clock-names'. The order of that
>> >> >property is fixed in the binding, which means you can simply use
>> >> >the index that the name is at in the binding in 'struct parent_data'.
>> >>
>> >> This driver is common across multiple device families, and each
>> >> device has
>> >different set of clock names in device tree/binding.  This
>> >implementation seemed to be generic for all devices.
>> >> To use index directly, I have to add if..else for matching
>> >> compatible strings
>> >and more if..else inside each of them for matching clock names to find
>index.
>> >Please let me know if this is preferred approach.
>> >
>> >It is preferred to not use clock-names and use the index directly.
>> >This avoids a bunch of string comparisons and makes for smaller and faster
>code.
>>
>> Agreed, using the "clock-names" adds string comparisons, however, two
>reasons why I think comparing with 'clock-names' is necessary.
>>
>> First, the clock driver is common for multiple platforms. And all platforms
>have their unique DT binding.
>> So, clock name to DT index mapping is platform specific. Which means the
>driver will have to hardcode the clock name to index mapping for each
>platform.
>
>The clock name to DT index mapping must be described in the binding.
>
>>
>> Second, clock parents information is received from firmware. The parents of a
>clock may or may not be present in the DT node 'clock-controller' as many
>clocks have "intermediate" clocks as parent.
>> We don't have DT index for intermediate clocks so need to register by
>fw_name.
>
>If there isn't a DT index for those intermediate clks then they should be using a
>clk_hw pointer directly. The fw_name member matches an index in the clock-
>names property, which has a 1:1 relationship to the DT index.
>
>> For example, below debug prints show parents of "spi1_ref" clock. It doesn't
>have any parent which is in DT.
>> clkname: spi1_ref : parent 0: ppll_to_xpd
>> clkname: spi1_ref : parent 1: npll_to_xpd
>> clkname: spi1_ref : parent 2: flxpll
>> clkname: spi1_ref : parent 3: rpll
>> So, here we need to check if the parent is in the DT, and if not, register by
>fw_name.
>
>If the parent isn't in DT then it can't use fw_name. There seems to be some
>misunderstanding.
>
>>
>> The zynqmp_get_parent_list function iterates over the parent list for each
>clock, check if the parent clock is present in the DT node under 'clock-names'
>property. If so, the driver registers clock by index, else register by fw_name.
>>
>> Because of this reason I believe the comparison with "clock-names" is
>unavoidable. Please share your inputs.
>>
>
>I think what you're saying is that zynqmp_get_clock_info() is building a
>topology description for clks that the driver registers. But some of those clks
>have parents that aren't registered by this driver and are external to the device.
>The zynqmp firmware interface is string based, not number based, so you have
>to keep "clock-names" DT property.
>
>That's all OK.
>
>You don't need to parse clock-names for the DT index if you take the string
>from the zynqmp firmware and jam it into the fw_name when the parent isn't
>a clk registered by this device. When the parent _is_ a clk registered by this
>device, you should use a clk_hw pointer for that other clk to indicate the
>parent. When it's a mixture of internal and external you use struct
>clk_parent_data. When it's only internal, use struct clk_init_data::clk_hws to
>avoid even considering having to parse DT.

I am having some confusion here to understand your suggestion. In the above comment, you mentioned 

"If the parent isn't in DT then it can't use fw_name"

And here in this suggestion, you mentioned

"don't need to parse clock-names for the DT index if you take the string from the zynqmp firmware and jam it into the fw_name when the parent isn't a clk registered by this device"

These two statements seem to be conflicting to me. Are you suggesting that if the parent clock is not registered by this device, then I can use fw_name directly to register it, no matter if the parent clock is present in the DT? 

In this case I don't need to parse DT at all if I understood correctly.

>
>If you want to skip the string comparisons in the clk core, and I suggest you do
>so, you can parse clock-names once, find the index for each external clk, and
>then use that info when building the topology returned from the zynqmp
>firmware to mark the parent as "external". Then when you go to register all the
>clks you can use that info to build the parent_data or parent_hws structures.
>It's unfortunate that the zynqmp firmware interface is string based, but I
>understand it can't be changed.

Ok, if I understood correctly, I could parse clock-names once and build a table which has DT index to clock name mapping. And then use this information while registering the clocks. 
In this approach too, for each parent I need to compare parent name with the string from table. So, string comparison can't be avoided completely but DT parsing need to be done only once. 
Is this correct understanding?

Also, I presume this is alternative approach to the above suggestion.

>
>I looked at zynqmp_pm_clock_get_parents() and it looks like it's just a bunch of
>numbers for the parent descriptions. If that's true, then there aren't any
>external clks at all, and so clock-names and clocks shouldn't be in the DT
>binding and you should just use clk_hw pointers everywhere.
>Did I miss something?

There are reference clocks in the DT under 'clock-names' property and these clocks can be a parent of some other clock. The current implementation is using clk_hw pointers for all clocks, and the parents are registered by their name. That is causing problem when the clock's node name in the DT is changed.