Re: [PATCH v2] clk: keystone: sci-clk: Adding support for non contiguous clocks
From: Kumar, Udit
Date: Tue Feb 06 2024 - 09:15:10 EST
On 2/6/2024 6:44 PM, Nishanth Menon wrote:
On 16:13-20240206, 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 add their clock
ids incrementally.
New firmware started returning error while calling get_freq and is_on
API for non-available clock ids.
In this fix, driver checks and adds only valid clock ids.
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 not present.
Signed-off-by: Udit Kumar <u-kumar1@xxxxxx>
---
Changelog
Changes in v2
- Updated commit message
- Simplified logic for valid clock id
link to v1 https://lore.kernel.org/all/20240205044557.3340848-1-u-kumar1@xxxxxx/
P.S
Firmawre returns total num_parents count including non available ids.
For above device id NAVSS0_CPTS_0, number of parents clocks are 16
i.e from id 2 to 17. But out of these ids few are not valid.
So driver adds only valid clock ids out ot total.
Original logs
https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
Line 2630 for error
Logs with fix v2
https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9
Line 2591
drivers/clk/keystone/sci-clk.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 35fe197dd303..ff249cbd54a1 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
int num_clks = 0;
int num_parents;
int clk_id;
+ u64 freq;
const char * const clk_names[] = {
"clocks", "assigned-clocks", "assigned-clock-parents", NULL
};
@@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
clk_id = args.args[1] + 1;
while (num_parents--) {
+ /* Check if this clock id is valid */
+ ret = provider->ops->get_freq(provider->sci,
+ sci_clk->dev_id, clk_id, &freq);
get_freq is a bit expensive as it has to walk the clock tree to find
the clock frequency (at least the first time?). just wondering if
there is lighter alternative here?
Let me check , if we have some other alternative here
+
+ clk_id++;
+ if (ret)
+ continue;
+
sci_clk = devm_kzalloc(dev,
sizeof(*sci_clk),
GFP_KERNEL);
if (!sci_clk)
return -ENOMEM;
sci_clk->dev_id = args.args[0];
- sci_clk->clk_id = clk_id++;
+ sci_clk->clk_id = clk_id - 1;
sci_clk->provider = provider;
list_add_tail(&sci_clk->node, &clks);
-
Spurious change.
I think, you meant by deleting the line ?
If yes then will address in next version
num_clks++;
}
}
--
2.34.1