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

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

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

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++;
>>>
>