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

[Andrew Davis <afd@ti.com>]

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.

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