Re: [PATCH] clk: keystone: sci-clk: Handle missing get_num_parents operation

["Padhi, Beleswar" <b-padhi@ti.com>]

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@ti.com>
>> ---
>> 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.

>  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.

Thanks,
Beleswar

>
> Andrew
>
>> list_add_tail(&sci_clk->node, &clks);
>>                     num_clks++;
>