diff mbox series

[v1] clk: keystone: sci-clk: Adding support for non contiguous clocks

Message ID 20240205044557.3340848-1-u-kumar1@ti.com
State New
Headers
Series [v1] clk: keystone: sci-clk: Adding support for non contiguous clocks |

Commit Message

Kumar, Udit Feb. 5, 2024, 4:45 a.m. UTC 
  Most of clocks and their parents are defined in contiguous range,
But in few cases, there is gap in clock numbers[0].

Driver assumes clocks to be in contiguous range, and assigns
accordingly.

New firmware started returning error in case of
non-available clock id.  Therefore drivers throws error while
re-calculate and other functions.

In this fix, before assigning and adding clock in list,
driver checks if given clock is valid or not.

Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")

[0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
Section Clocks for NAVSS0_CPTS_0 Device,
clock id 12-15 and 18-19 not present

Signed-off-by: Udit Kumar <u-kumar1@ti.com>
---
Original logs
https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
Line 2630 for error

Logs with fix
https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix
Line 2594 

 drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Nishanth Menon Feb. 5, 2024, 2:04 p.m. UTC | #1 
On 10:15-20240205, Udit Kumar wrote:
> Most of clocks and their parents are defined in contiguous range,
> But in few cases, there is gap in clock numbers[0].
> 
> Driver assumes clocks to be in contiguous range, and assigns
> accordingly.
> 
> New firmware started returning error in case of
> non-available clock id.  Therefore drivers throws error while
> re-calculate and other functions.

What changed here? started returning error for what API? also please fix
up 70 char alignment -> there extra spaces in your commit message.
> 
> In this fix, before assigning and adding clock in list,
> driver checks if given clock is valid or not.
> 
> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
> 
> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
> Section Clocks for NAVSS0_CPTS_0 Device,
> clock id 12-15 and 18-19 not present
> 
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
> ---
> Original logs
> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
> Line 2630 for error
> 
> Logs with fix
> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix
> Line 2594 
> 
>  drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 35fe197dd303..d417ec018d82 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  	int num_clks = 0;
>  	int num_parents;
>  	int clk_id;
> +	int max_clk_id;
> +	u64 freq;
>  	const char * const clk_names[] = {
>  		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
>  	};
> @@ -584,6 +586,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  				}
>  
>  				clk_id = args.args[1] + 1;
> +				max_clk_id = clk_id + num_parents;
>  
>  				while (num_parents--) {
>  					sci_clk = devm_kzalloc(dev,
> @@ -592,11 +595,20 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  					if (!sci_clk)
>  						return -ENOMEM;
>  					sci_clk->dev_id = args.args[0];
> -					sci_clk->clk_id = clk_id++;
> -					sci_clk->provider = provider;
> -					list_add_tail(&sci_clk->node, &clks);
> +					/* check if given clock id is valid by calling get_freq */
> +					/* loop over max possible ids */
> +					do {
> +						sci_clk->clk_id = clk_id++;
>  
> -					num_clks++;
> +						ret = provider->ops->get_freq(provider->sci,
> +							   sci_clk->dev_id, sci_clk->clk_id, &freq);
> +					} while (ret != 0 && clk_id < max_clk_id);

take clock ids 0 1 2 3 -> Say 2 is reserved.
num_parents = 4
while(num_parents) Loop 1 ->  clk ID 0 is valid, list_add_tail
while(num_parents) Loop 2 ->  clk ID 1 is valid, list_add_tail
while(num_parents) Loop 3 ->  clk ID 2 is invalid.. so we scan forward
	to clk ID 3 -> list_add_tail
while(num_parents) Loop 4 ->  clk ID 4 is invalid.. but 5 is out of
	range, so we break off loop. sci_clk is still devm_kzalloced ->
	but since clk_id > max_clk_id, we jump off loop, and we dont add
	it to tail. so one extra allocation?
If we have multiple reserved intermediate ones, then we'd have as many
allocations that aren't linked? Could we not improve the logic a bit to
allocate just what is necessary?

> +
> +					sci_clk->provider = provider;
> +					if (ret == 0) {
> +						list_add_tail(&sci_clk->node, &clks);
> +						num_clks++;
> +					}
>  				}
>  			}
>  
> -- 
> 2.34.1
>
Kumar, Udit Feb. 5, 2024, 5:43 p.m. UTC | #2 
On 2/5/2024 7:34 PM, Nishanth Menon wrote:
> On 10:15-20240205, Udit Kumar wrote:
>> Most of clocks and their parents are defined in contiguous range,
>> But in few cases, there is gap in clock numbers[0].
>>
>> Driver assumes clocks to be in contiguous range, and assigns
>> accordingly.
>>
>> New firmware started returning error in case of
>> non-available clock id.  Therefore drivers throws error while
>> re-calculate and other functions.
> What changed here? started returning error for what API? also please fix
> up 70 char alignment -> there extra spaces in your commit message.


will address in v2

>> In this fix, before assigning and adding clock in list,
>> driver checks if given clock is valid or not.
>>
>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
>>
>> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
>> Section Clocks for NAVSS0_CPTS_0 Device,
>> clock id 12-15 and 18-19 not present
>>
>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>> ---
>> Original logs
>> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
>> Line 2630 for error
>>
>> Logs with fix
>> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix
>> Line 2594
>>
>>   drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 35fe197dd303..d417ec018d82 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>>   	int num_clks = 0;
>>   	int num_parents;
>> [..]
>> -					num_clks++;
>> +						ret = provider->ops->get_freq(provider->sci,
>> +							   sci_clk->dev_id, sci_clk->clk_id, &freq);
>> +					} while (ret != 0 && clk_id < max_clk_id);
> take clock ids 0 1 2 3 -> Say 2 is reserved.
> num_parents = 4
> while(num_parents) Loop 1 ->  clk ID 0 is valid, list_add_tail
> while(num_parents) Loop 2 ->  clk ID 1 is valid, list_add_tail
> while(num_parents) Loop 3 ->  clk ID 2 is invalid.. so we scan forward
> 	to clk ID 3 -> list_add_tail
> while(num_parents) Loop 4 ->  clk ID 4 is invalid.. but 5 is out of
> 	range, so we break off loop. sci_clk is still devm_kzalloced ->
> 	but since clk_id > max_clk_id, we jump off loop, and we dont add
> 	it to tail. so one extra allocation?

Thanks for catching this.


> If we have multiple reserved intermediate ones, then we'd have as many
> allocations that aren't linked? Could we not improve the logic a bit to
> allocate just what is necessary?

Sure, will change in v2.

to check clock validity first and then allocate, add


>> +
>> +					sci_clk->provider = provider;
>> +					if (ret == 0) {
>> +						list_add_tail(&sci_clk->node, &clks);
>> +						num_clks++;
>> +					}
>>   				}
>>   			}
>>   
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 35fe197dd303..d417ec018d82 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -517,6 +517,8 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 	int num_clks = 0;
 	int num_parents;
 	int clk_id;
+	int max_clk_id;
+	u64 freq;
 	const char * const clk_names[] = {
 		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
 	};
@@ -584,6 +586,7 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 				}
 
 				clk_id = args.args[1] + 1;
+				max_clk_id = clk_id + num_parents;
 
 				while (num_parents--) {
 					sci_clk = devm_kzalloc(dev,
@@ -592,11 +595,20 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 					if (!sci_clk)
 						return -ENOMEM;
 					sci_clk->dev_id = args.args[0];
-					sci_clk->clk_id = clk_id++;
-					sci_clk->provider = provider;
-					list_add_tail(&sci_clk->node, &clks);
+					/* check if given clock id is valid by calling get_freq */
+					/* loop over max possible ids */
+					do {
+						sci_clk->clk_id = clk_id++;
 
-					num_clks++;
+						ret = provider->ops->get_freq(provider->sci,
+							   sci_clk->dev_id, sci_clk->clk_id, &freq);
+					} while (ret != 0 && clk_id < max_clk_id);
+
+					sci_clk->provider = provider;
+					if (ret == 0) {
+						list_add_tail(&sci_clk->node, &clks);
+						num_clks++;
+					}
 				}
 			}