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 <[email protected]>
---
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);
+
+ sci_clk->provider = provider;
+ if (ret == 0) {
+ list_add_tail(&sci_clk->node, &clks);
+ num_clks++;
+ }
}
}
--
2.34.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 <[email protected]>
> ---
> 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
>
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 <[email protected]>
>> ---
>> 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
>>