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 <[email protected]>
---
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,
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]>
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240104/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Stephen,
Thanks for review. Please find my response inline.
Thanks,
Naman
>-----Original Message-----
>From: Stephen Boyd <[email protected]>
>Sent: Thursday, January 4, 2024 6:30 AM
>To: Trivedi Manojbhai, Naman <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Simek, Michal <[email protected]>;
>[email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; Trivedi Manojbhai, Naman
><[email protected]>
>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 <[email protected]>
>Sent: Thursday, January 4, 2024 6:30 AM
>To: Trivedi Manojbhai, Naman <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Simek, Michal <[email protected]>;
>[email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; Trivedi Manojbhai, Naman
><[email protected]>
>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 <[email protected]>; [email protected];
>[email protected]; Simek, Michal
><[email protected]>; [email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]
>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 <[email protected]>
>>Sent: Thursday, January 4, 2024 6:30 AM
>>To: Trivedi Manojbhai, Naman <[email protected]>;
>>[email protected]; [email protected];
>>[email protected]; Simek, Michal <[email protected]>;
>>[email protected]; [email protected]
>>Cc: [email protected]; [email protected];
>>linux- [email protected]; Trivedi Manojbhai, Naman
>><[email protected]>
>>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 <[email protected]>
>Sent: Thursday, January 4, 2024 6:30 AM
>To: Trivedi Manojbhai, Naman <[email protected]>;
>[email protected]; [email protected];
>[email protected]; Simek, Michal <[email protected]>;
>[email protected]; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; Trivedi Manojbhai, Naman
><[email protected]>
>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 <[email protected]>; Simek, Michal
><[email protected]>; [email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]
>Subject: RE: [PATCH] drivers: clk: zynqmp: remove clock name dependency
>
>Hi Stephen,
>
>>-----Original Message-----
>>From: Stephen Boyd <[email protected]>
>>Sent: Thursday, January 4, 2024 6:30 AM
>>To: Trivedi Manojbhai, Naman <[email protected]>;
>>[email protected]; [email protected];
>>[email protected]; Simek, Michal <[email protected]>;
>>[email protected]; [email protected]
>>Cc: [email protected]; [email protected];
>>linux- [email protected]; Trivedi Manojbhai, Naman
>><[email protected]>
>>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
Quoting Trivedi Manojbhai, Naman (2024-03-26 22:47:28)
> >>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.
No, do not get the clock name of a parent in the driver. Use struct
clk_parent_data to tell the clk framework what the parent is.
> >
> >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.
Don't use a string name. The devicetree index must be stable, so simply
use the index that the binding has as the index in struct
clk_parent_data.
struct clk_parent_data parent_map[] = {
{ .index = MY_DT_PARENT_1 }, /* parent index 0 */
{ .index = MT_DT_PARENT_2 }, /* parent index 1 */
};
> >
> >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.
I guess that's a problem with how the code is written?
> >
> >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?
> >
There are many references. A new qcom one is drivers/clk/qcom/gcc-x1e80100.c