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

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

[Beleswar Padhi]

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;
 				list_add_tail(&sci_clk->node, &clks);
 
 				num_clks++;
-- 
2.34.1

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

[Andrew Davis]

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

Andrew

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

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

[Padhi, Beleswar]

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

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

[Andrew Davis]

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

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

[Padhi, Beleswar]

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