Re: [PATCH] clk: keystone: sci-clk: Handle missing get_num_parents operation
From: Padhi, Beleswar
Date: Thu Oct 01 2026 - 02:35:55 EST
On 10/1/2026 4:22 AM, Andrew Davis wrote:
On 9/30/26 3:58 PM, Padhi, Beleswar wrote:
On 10/1/2026 12:49 AM, Andrew Davis wrote:
On 9/30/26 2:03 PM, Beleswar Padhi wrote:
The sci-clk driver unconditionally calls the get_num_parents TI-SCI
operation while scanning clocks, both from DT and from firmware. This
works for all cases today, but with future support for some System
Controllers (e.g. PDM on TDA54), this may not be always right.
The PDM system controller on TI TDA54 SoC manages clock parents
internally and does not support the clock parent related ops. Therefore,
when scanning clocks from DT, treat the clock as having a single parent
if get_num_parents is not provided. Such a clock is registered without
parents, so the get_parent and set_parent operations are never invoked
for it.
Scanning clocks from firmware relies on get_num_parents to discover the
valid clock IDs, so return -EOPNOTSUPP in that case instead.
Signed-off-by: Beleswar Padhi <b-padhi@xxxxxx>
---
Testing Done:
- Boot test on all keystone and K3 platforms.
- Verified that the patch does not result in any warnings or errors
Logs:
https://gist.github.com/3V3RYONE/d821cea46f5dc1e70a759c97878fa487
Note:
This patch is independent and can be applied directly.
drivers/clk/keystone/sci-clk.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 9d2094bd48e3b..5bf615893a8c0 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -468,6 +468,13 @@ static int ti_sci_scan_clocks_from_fw(struct sci_clk_provider *provider)
int gap_size = 0;
struct device *dev = provider->dev;
+ /*
+ * Clocks are discovered by probing the firmware with get_num_parents,
+ * which is not available with every system firmware (e.g. ABI5.0 PDM).
+ */
+ if (!provider->ops->get_num_parents)
+ return -EOPNOTSUPP;
+
while (1) {
ret = provider->ops->get_num_parents(provider->sci, dev_id,
clk_id,
@@ -589,10 +596,18 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
sci_clk->dev_id = args.args[0];
sci_clk->clk_id = args.args[1];
sci_clk->provider = provider;
- provider->ops->get_num_parents(provider->sci,
- sci_clk->dev_id,
- sci_clk->clk_id,
- (void *)&sci_clk->num_parents);
+ /*
+ * Firmware without get_num_parents (e.g. ABI5.0
+ * PDM) manages clock parents internally, so
+ * treat the clock as having a single parent.
+ */
+ if (provider->ops->get_num_parents)
+ provider->ops->get_num_parents(provider->sci,
+ sci_clk->dev_id,
+ sci_clk->clk_id,
+ &sci_clk->num_parents);
+ else
+ sci_clk->num_parents = 1;
Why 1 and not 0?
Functionally, 1 and 0 are treated the same everywhere in the
driver. Could pick either.
Exactly what I was seeing, setting it to 0 just feels more correct.
Might be some future case that checks on the "1" parent that doesn't
exist, but "0" parents should always result in no parent check.
Sure, will do.
Also, why not keep `get_num_parents` defined, but just have it
return num_parents as 0? Haven't checked but if that works the same, but if it
does then it saves us from having to make changes here and we can isolate
firmware differences to only the firmware driver.
This works for the above branch where we are scanning clocks from device
tree. But the code path where we scan clocks from System Firmware explicitly
depends on a NAK against some clock to get out of the infinite loop. If we are
writing a get_num_parents() which returns a NAK always and sets
num_parents to 0, we are patching the protocol at this point. This does not
give the correct view IMHO.
We wouldn't return NAK if the clock exists, we return success and
set the num_parents to 0. Only for invalid clock IDs we return NAK.
This should keep the TI_SCI_CLK_PROBE_FROM_FW path happy.
That should work. Although, I am thinking if it's alright for us to implement
clock parent related ops when the ABI spec says that it's not supported. Let
me know your preference, and accordingly I can send a revision for this or
handle it in the TI-SCI ABI5.0 series whenever it's due for a revision:
https://lore.kernel.org/all/20260930194131.117129-1-b-padhi@xxxxxx
Thanks,
Beleswar
But really
that path has been disabled for many years now and I highly doubt it
even functions at all anymore, it should probably just be removed.
Andrew
Thanks,
Beleswar
Andrew
list_add_tail(&sci_clk->node, &clks);
num_clks++;