| Message ID | 20240103072017.1646007-1-naman.trivedimanojbhai@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: clk: zynqmp: remove clock name dependency | expand |
On 1/3/24 08:20, Naman Trivedi Manojbhai wrote: > Currently, from zynqmp_get_parent_list() function the clock driver > references the clock by name instead of its reference from device tree. > This causes problem when the clock name in the device tree is changed. > > Remove hard dependency of clock name and update the logic to use clock > reference from device tree instead of clock name. > > Signed-off-by: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com> > --- > drivers/clk/zynqmp/clkc.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c > index a91d98e238c2..87915de083d9 100644 > --- a/drivers/clk/zynqmp/clkc.c > +++ b/drivers/clk/zynqmp/clkc.c > @@ -549,18 +549,46 @@ static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id, > u32 total_parents = clock[clk_id].num_parents; > struct clock_topology *clk_nodes; > struct clock_parent *parents; > + struct clk *clk_parent; > + char *clk_name; > > clk_nodes = clock[clk_id].node; > parents = clock[clk_id].parent; > > for (i = 0; i < total_parents; i++) { > if (!parents[i].flag) { > + ret = of_property_match_string(np, "clock-names", > + parents[i].name); > + if (ret >= 0) { > + clk_parent = of_clk_get(np, ret); > + if (clk_parent) { > + clk_name = __clk_get_name(clk_parent); > + if (clk_name) > + strcpy(parents[i].name, clk_name); > + else > + return 1; > + } else { > + return 1; > + } > + } > parent_list[i] = 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) { > strcpy(parents[i].name, "dummy_name"); > + } else { > + clk_parent = of_clk_get(np, ret); > + if (clk_parent) { > + clk_name = __clk_get_name(clk_parent); > + if (clk_name) > + strcpy(parents[i].name, clk_name); > + else > + return 1; > + } else { > + return 1; > + } > + } > parent_list[i] = parents[i].name; > } else { > strcat(parents[i].name, Not sure if this is the best way how to do it but it works as expected. Tested-by: Michal Simek <michal.simek@amd.com> Thanks, Michal
Hi Naman, kernel test robot noticed the following build errors: [auto build test ERROR on clk/clk-next] [also build test ERROR on linus/master v6.7-rc8 next-20240103] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Naman-Trivedi-Manojbhai/drivers-clk-zynqmp-remove-clock-name-dependency/20240103-152152 base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next patch link: https://lore.kernel.org/r/20240103072017.1646007-1-naman.trivedimanojbhai%40amd.com patch subject: [PATCH] drivers: clk: zynqmp: remove clock name dependency config: arm64-randconfig-004-20240103 (https://download.01.org/0day-ci/archive/20240104/202401040635.RiqRRo7w-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240104/202401040635.RiqRRo7w-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202401040635.RiqRRo7w-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/clk/zynqmp/clkc.c:565:15: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] clk_name = __clk_get_name(clk_parent); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/clk/zynqmp/clkc.c:583:15: error: assigning to 'char *' from 'const char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] clk_name = __clk_get_name(clk_parent); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. vim +565 drivers/clk/zynqmp/clkc.c 535 536 /** 537 * zynqmp_get_parent_list() - Create list of parents name 538 * @np: Device node 539 * @clk_id: Clock index 540 * @parent_list: List of parent's name 541 * @num_parents: Total number of parents 542 * 543 * Return: 0 on success else error+reason 544 */ 545 static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id, 546 const char **parent_list, u32 *num_parents) 547 { 548 int i = 0, ret; 549 u32 total_parents = clock[clk_id].num_parents; 550 struct clock_topology *clk_nodes; 551 struct clock_parent *parents; 552 struct clk *clk_parent; 553 char *clk_name; 554 555 clk_nodes = clock[clk_id].node; 556 parents = clock[clk_id].parent; 557 558 for (i = 0; i < total_parents; i++) { 559 if (!parents[i].flag) { 560 ret = of_property_match_string(np, "clock-names", 561 parents[i].name); 562 if (ret >= 0) { 563 clk_parent = of_clk_get(np, ret); 564 if (clk_parent) { > 565 clk_name = __clk_get_name(clk_parent); 566 if (clk_name) 567 strcpy(parents[i].name, clk_name); 568 else 569 return 1; 570 } else { 571 return 1; 572 } 573 } 574 parent_list[i] = parents[i].name; 575 } else if (parents[i].flag == PARENT_CLK_EXTERNAL) { 576 ret = of_property_match_string(np, "clock-names", 577 parents[i].name); 578 if (ret < 0) { 579 strcpy(parents[i].name, "dummy_name"); 580 } else { 581 clk_parent = of_clk_get(np, ret); 582 if (clk_parent) { 583 clk_name = __clk_get_name(clk_parent); 584 if (clk_name) 585 strcpy(parents[i].name, clk_name); 586 else 587 return 1; 588 } else { 589 return 1; 590 } 591 } 592 parent_list[i] = parents[i].name; 593 } else { 594 strcat(parents[i].name, 595 clk_type_postfix[clk_nodes[parents[i].flag - 1]. 596 type]); 597 parent_list[i] = parents[i].name; 598 } 599 } 600 601 *num_parents = total_parents; 602 return 0; 603 } 604
Quoting Naman Trivedi Manojbhai (2024-01-02 23:20:17) > Currently, from zynqmp_get_parent_list() function the clock driver > references the clock by name instead of its reference from device tree. > This causes problem when the clock name in the device tree is changed. > > Remove hard dependency of clock name and update the logic to use clock > reference from device tree instead of clock name. Please use struct clk_parent_data instead.
Hi Naman, kernel test robot noticed the following build warnings: [auto build test WARNING on clk/clk-next] [also build test WARNING on linus/master v6.7-rc8 next-20240103] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Naman-Trivedi-Manojbhai/drivers-clk-zynqmp-remove-clock-name-dependency/20240103-152152 base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next patch link: https://lore.kernel.org/r/20240103072017.1646007-1-naman.trivedimanojbhai%40amd.com patch subject: [PATCH] drivers: clk: zynqmp: remove clock name dependency config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240104/202401040935.LLlZNxiu-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240104/202401040935.LLlZNxiu-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202401040935.LLlZNxiu-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/clk/zynqmp/clkc.c: In function 'zynqmp_get_parent_list': >> drivers/clk/zynqmp/clkc.c:565:50: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 565 | clk_name = __clk_get_name(clk_parent); | ^ drivers/clk/zynqmp/clkc.c:583:50: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 583 | clk_name = __clk_get_name(clk_parent); | ^ vim +/const +565 drivers/clk/zynqmp/clkc.c 535 536 /** 537 * zynqmp_get_parent_list() - Create list of parents name 538 * @np: Device node 539 * @clk_id: Clock index 540 * @parent_list: List of parent's name 541 * @num_parents: Total number of parents 542 * 543 * Return: 0 on success else error+reason 544 */ 545 static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id, 546 const char **parent_list, u32 *num_parents) 547 { 548 int i = 0, ret; 549 u32 total_parents = clock[clk_id].num_parents; 550 struct clock_topology *clk_nodes; 551 struct clock_parent *parents; 552 struct clk *clk_parent; 553 char *clk_name; 554 555 clk_nodes = clock[clk_id].node; 556 parents = clock[clk_id].parent; 557 558 for (i = 0; i < total_parents; i++) { 559 if (!parents[i].flag) { 560 ret = of_property_match_string(np, "clock-names", 561 parents[i].name); 562 if (ret >= 0) { 563 clk_parent = of_clk_get(np, ret); 564 if (clk_parent) { > 565 clk_name = __clk_get_name(clk_parent); 566 if (clk_name) 567 strcpy(parents[i].name, clk_name); 568 else 569 return 1; 570 } else { 571 return 1; 572 } 573 } 574 parent_list[i] = parents[i].name; 575 } else if (parents[i].flag == PARENT_CLK_EXTERNAL) { 576 ret = of_property_match_string(np, "clock-names", 577 parents[i].name); 578 if (ret < 0) { 579 strcpy(parents[i].name, "dummy_name"); 580 } else { 581 clk_parent = of_clk_get(np, ret); 582 if (clk_parent) { 583 clk_name = __clk_get_name(clk_parent); 584 if (clk_name) 585 strcpy(parents[i].name, clk_name); 586 else 587 return 1; 588 } else { 589 return 1; 590 } 591 } 592 parent_list[i] = parents[i].name; 593 } else { 594 strcat(parents[i].name, 595 clk_type_postfix[clk_nodes[parents[i].flag - 1]. 596 type]); 597 parent_list[i] = parents[i].name; 598 } 599 } 600 601 *num_parents = total_parents; 602 return 0; 603 } 604
Hi Stephen, Thanks for review. Please find my response inline. Thanks, Naman >-----Original Message----- >From: Stephen Boyd <sboyd@kernel.org> >Sent: Thursday, January 4, 2024 6:30 AM >To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; >abel.vesa@linaro.org; angelogioacchino.delregno@collabora.com; >krzysztof.kozlowski@linaro.org; Simek, Michal <michal.simek@amd.com>; >mturquette@baylibre.com; robh@kernel.org >Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Trivedi Manojbhai, Naman ><Naman.TrivediManojbhai@amd.com> >Subject: Re: [PATCH] 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 Manojbhai (2024-01-02 23:20:17) >> Currently, from zynqmp_get_parent_list() function the clock driver >> references the clock by name instead of its reference from device tree. >> This causes problem when the clock name in the device tree is changed. >> >> Remove hard dependency of clock name and update the logic to use clock >> reference from device tree instead of clock name. > >Please use struct clk_parent_data instead. Can you please clarify how struct clk_parent_data can be used here? The zynqmp clock driver receives clock parent information from firmware and it is stored in struct clock_parent. In this patch, I added logic to get the parent clock reference from device tree and get corresponding clock name. Can you please explain how the struct clk_parent_data can be used for the same? Thanks, Naman
Hi @Stephen Boyd A gentle reminder. Please help me to understand the query I have asked below. Thanks, Naman >-----Original Message----- >From: Stephen Boyd <sboyd@kernel.org> >Sent: Thursday, January 4, 2024 6:30 AM >To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; >abel.vesa@linaro.org; angelogioacchino.delregno@collabora.com; >krzysztof.kozlowski@linaro.org; Simek, Michal <michal.simek@amd.com>; >mturquette@baylibre.com; robh@kernel.org >Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Trivedi Manojbhai, Naman ><Naman.TrivediManojbhai@amd.com> >Subject: Re: [PATCH] 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 Manojbhai (2024-01-02 23:20:17) >> Currently, from zynqmp_get_parent_list() function the clock driver >> references the clock by name instead of its reference from device tree. >> This causes problem when the clock name in the device tree is changed. >> >> Remove hard dependency of clock name and update the logic to use clock >> reference from device tree instead of clock name. > >Please use struct clk_parent_data instead.
Hi Stephen, A gentle reminder. Can you please help me to clarify the below query? Thanks, Naman >-----Original Message----- >From: Trivedi Manojbhai, Naman >Sent: Tuesday, January 9, 2024 4:55 PM >To: Stephen Boyd <sboyd@kernel.org>; abel.vesa@linaro.org; >angelogioacchino.delregno@collabora.com; Simek, Michal ><michal.simek@amd.com>; mturquette@baylibre.com; robh@kernel.org >Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org >Subject: RE: [PATCH] drivers: clk: zynqmp: remove clock name dependency > >Hi Stephen, > >Thanks for review. Please find my response inline. > >Thanks, >Naman > >>-----Original Message----- >>From: Stephen Boyd <sboyd@kernel.org> >>Sent: Thursday, January 4, 2024 6:30 AM >>To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; >>abel.vesa@linaro.org; angelogioacchino.delregno@collabora.com; >>krzysztof.kozlowski@linaro.org; Simek, Michal <michal.simek@amd.com>; >>mturquette@baylibre.com; robh@kernel.org >>Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>linux- kernel@vger.kernel.org; Trivedi Manojbhai, Naman >><Naman.TrivediManojbhai@amd.com> >>Subject: Re: [PATCH] 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 Manojbhai (2024-01-02 23:20:17) >>> Currently, from zynqmp_get_parent_list() function the clock driver >>> references the clock by name instead of its reference from device tree. >>> This causes problem when the clock name in the device tree is changed. >>> >>> Remove hard dependency of clock name and update the logic to use >>> clock reference from device tree instead of clock name. >> >>Please use struct clk_parent_data instead. >Can you please clarify how struct clk_parent_data can be used here? > >The zynqmp clock driver receives clock parent information from firmware and it >is stored in struct clock_parent. In this patch, I added logic to get the parent >clock reference from device tree and get corresponding clock name. Can you >please explain how the struct clk_parent_data can be used for the same? > >Thanks, >Naman
Hi Stephen, >-----Original Message----- >From: Stephen Boyd <sboyd@kernel.org> >Sent: Thursday, January 4, 2024 6:30 AM >To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; >abel.vesa@linaro.org; angelogioacchino.delregno@collabora.com; >krzysztof.kozlowski@linaro.org; Simek, Michal <michal.simek@amd.com>; >mturquette@baylibre.com; robh@kernel.org >Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org; Trivedi Manojbhai, Naman ><Naman.TrivediManojbhai@amd.com> >Subject: Re: [PATCH] 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 Manojbhai (2024-01-02 23:20:17) >> Currently, from zynqmp_get_parent_list() function the clock driver >> references the clock by name instead of its reference from device tree. >> This causes problem when the clock name in the device tree is changed. >> >> Remove hard dependency of clock name and update the logic to use clock >> reference from device tree instead of clock name. > >Please use struct clk_parent_data instead. Thanks for review. As per my understanding, you suggest to replace the proposed logic, and use "struct clk_parent_data" to get the clock name from device tree. I have gone through other drivers which use the "struct clk_parent_data" structure, they have hard coded clock names in the driver. In zynqmp, the driver receives clock name from firmware. Also, the "zynqmp_get_parent_list" function is called before clocks are registered. So at this point, we don't have the hw structure which has clk_parent_data. So, I did not get how to use the struct clk_parent_data in this case. Can you please provide an example which I can look at as a reference? Thanks, Naman
Hi Stephen, I am awaiting your response for the below query. Can you please help me with the same? Thanks, Naman >-----Original Message----- >From: Trivedi Manojbhai, Naman >Sent: Thursday, March 7, 2024 11:37 AM >To: Stephen Boyd <sboyd@kernel.org>; Simek, Michal ><michal.simek@amd.com>; robh@kernel.org >Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- >kernel@vger.kernel.org >Subject: RE: [PATCH] drivers: clk: zynqmp: remove clock name dependency > >Hi Stephen, > >>-----Original Message----- >>From: Stephen Boyd <sboyd@kernel.org> >>Sent: Thursday, January 4, 2024 6:30 AM >>To: Trivedi Manojbhai, Naman <Naman.TrivediManojbhai@amd.com>; >>abel.vesa@linaro.org; angelogioacchino.delregno@collabora.com; >>krzysztof.kozlowski@linaro.org; Simek, Michal <michal.simek@amd.com>; >>mturquette@baylibre.com; robh@kernel.org >>Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>linux- kernel@vger.kernel.org; Trivedi Manojbhai, Naman >><Naman.TrivediManojbhai@amd.com> >>Subject: Re: [PATCH] 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 Manojbhai (2024-01-02 23:20:17) >>> Currently, from zynqmp_get_parent_list() function the clock driver >>> references the clock by name instead of its reference from device tree. >>> This causes problem when the clock name in the device tree is changed. >>> >>> Remove hard dependency of clock name and update the logic to use >>> clock reference from device tree instead of clock name. >> >>Please use struct clk_parent_data instead. >Thanks for review. As per my understanding, you suggest to replace the >proposed logic, and use "struct clk_parent_data" to get the clock name from >device tree. > >I have gone through other drivers which use the "struct clk_parent_data" >structure, they have hard coded clock names in the driver. In zynqmp, the >driver receives clock name from firmware. > >Also, the "zynqmp_get_parent_list" function is called before clocks are >registered. So at this point, we don't have the hw structure which has >clk_parent_data. > >So, I did not get how to use the struct clk_parent_data in this case. Can you >please provide an example which I can look at as a reference? > >Thanks, >Naman
diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c index a91d98e238c2..87915de083d9 100644 --- a/drivers/clk/zynqmp/clkc.c +++ b/drivers/clk/zynqmp/clkc.c @@ -549,18 +549,46 @@ static int zynqmp_get_parent_list(struct device_node *np, u32 clk_id, u32 total_parents = clock[clk_id].num_parents; struct clock_topology *clk_nodes; struct clock_parent *parents; + struct clk *clk_parent; + char *clk_name; clk_nodes = clock[clk_id].node; parents = clock[clk_id].parent; for (i = 0; i < total_parents; i++) { if (!parents[i].flag) { + ret = of_property_match_string(np, "clock-names", + parents[i].name); + if (ret >= 0) { + clk_parent = of_clk_get(np, ret); + if (clk_parent) { + clk_name = __clk_get_name(clk_parent); + if (clk_name) + strcpy(parents[i].name, clk_name); + else + return 1; + } else { + return 1; + } + } parent_list[i] = 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) { strcpy(parents[i].name, "dummy_name"); + } else { + clk_parent = of_clk_get(np, ret); + if (clk_parent) { + clk_name = __clk_get_name(clk_parent); + if (clk_name) + strcpy(parents[i].name, clk_name); + else + return 1; + } else { + return 1; + } + } parent_list[i] = parents[i].name; } else { strcat(parents[i].name,
Currently, from zynqmp_get_parent_list() function the clock driver references the clock by name instead of its reference from device tree. This causes problem when the clock name in the device tree is changed. Remove hard dependency of clock name and update the logic to use clock reference from device tree instead of clock name. Signed-off-by: Naman Trivedi Manojbhai <naman.trivedimanojbhai@amd.com> --- drivers/clk/zynqmp/clkc.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)