diff mbox series

[v2] clk: keystone: sci-clk: Adding support for non contiguous clocks

Message ID 20240206104357.3803517-1-u-kumar1@ti.com
State New
Headers
Series [v2] clk: keystone: sci-clk: Adding support for non contiguous clocks |

Commit Message

Kumar, Udit Feb. 6, 2024, 10:43 a.m. UTC 
  Most of clocks and their parents are defined in contiguous range,
But in few cases, there is gap in clock numbers[0].
Driver assumes clocks to be in contiguous range, and add their clock
ids incrementally.

New firmware started returning error while calling get_freq and is_on
API for non-available clock ids.

In this fix, driver checks and adds only valid clock ids.

Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")

[0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
Section Clocks for NAVSS0_CPTS_0 Device,
clock id 12-15 not present.

Signed-off-by: Udit Kumar <u-kumar1@ti.com>
---
Changelog

Changes in v2
- Updated commit message
- Simplified logic for valid clock id
link to v1 https://lore.kernel.org/all/20240205044557.3340848-1-u-kumar1@ti.com/


P.S
Firmawre returns total num_parents count including non available ids.
For above device id NAVSS0_CPTS_0, number of parents clocks are 16
i.e from id 2 to 17. But out of these ids few are not valid.
So driver adds only valid clock ids out ot total.

Original logs
https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
Line 2630 for error

Logs with fix v2
https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9
Line 2591


 drivers/clk/keystone/sci-clk.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Nishanth Menon Feb. 6, 2024, 1:14 p.m. UTC | #1 
On 16:13-20240206, Udit Kumar wrote:
> Most of clocks and their parents are defined in contiguous range,
> But in few cases, there is gap in clock numbers[0].
> Driver assumes clocks to be in contiguous range, and add their clock
> ids incrementally.
> 
> New firmware started returning error while calling get_freq and is_on
> API for non-available clock ids.
> 
> In this fix, driver checks and adds only valid clock ids.
> 
> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
> 
> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
> Section Clocks for NAVSS0_CPTS_0 Device,
> clock id 12-15 not present.
> 
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
> ---
> Changelog
> 
> Changes in v2
> - Updated commit message
> - Simplified logic for valid clock id
> link to v1 https://lore.kernel.org/all/20240205044557.3340848-1-u-kumar1@ti.com/
> 
> 
> P.S
> Firmawre returns total num_parents count including non available ids.
> For above device id NAVSS0_CPTS_0, number of parents clocks are 16
> i.e from id 2 to 17. But out of these ids few are not valid.
> So driver adds only valid clock ids out ot total.
> 
> Original logs
> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
> Line 2630 for error
> 
> Logs with fix v2
> https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9
> Line 2591
> 
> 
>  drivers/clk/keystone/sci-clk.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 35fe197dd303..ff249cbd54a1 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  	int num_clks = 0;
>  	int num_parents;
>  	int clk_id;
> +	u64 freq;
>  	const char * const clk_names[] = {
>  		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
>  	};
> @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  				clk_id = args.args[1] + 1;
>  
>  				while (num_parents--) {
> +					/* Check if this clock id is valid */
> +					ret = provider->ops->get_freq(provider->sci,
> +						sci_clk->dev_id, clk_id, &freq);

get_freq is a bit expensive as it has to walk the clock tree to find
the clock frequency (at least the first time?). just wondering if
there is lighter alternative here?

> +
> +					clk_id++;
> +					if (ret)
> +						continue;
> +
>  					sci_clk = devm_kzalloc(dev,
>  							       sizeof(*sci_clk),
>  							       GFP_KERNEL);
>  					if (!sci_clk)
>  						return -ENOMEM;
>  					sci_clk->dev_id = args.args[0];
> -					sci_clk->clk_id = clk_id++;
> +					sci_clk->clk_id = clk_id - 1;
>  					sci_clk->provider = provider;
>  					list_add_tail(&sci_clk->node, &clks);
> -
	Spurious change.
>  					num_clks++;
>  				}
>  			}
> -- 
> 2.34.1
>
Kamlesh Gurudasani Feb. 6, 2024, 1:54 p.m. UTC | #2 
Nishanth Menon <nm@ti.com> writes:

> On 16:13-20240206, Udit Kumar wrote:
>> Most of clocks and their parents are defined in contiguous range,
>> But in few cases, there is gap in clock numbers[0].
>> Driver assumes clocks to be in contiguous range, and add their clock
>> ids incrementally.
>> 
>> New firmware started returning error while calling get_freq and is_on
>> API for non-available clock ids.
>> 
>> In this fix, driver checks and adds only valid clock ids.
>> 
>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
>> 
>> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
>> Section Clocks for NAVSS0_CPTS_0 Device,
>> clock id 12-15 not present.
>> 
>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>>  				while (num_parents--) {
>> +					/* Check if this clock id is valid */
>> +					ret = provider->ops->get_freq(provider->sci,
>> +						sci_clk->dev_id, clk_id, &freq);
>
> get_freq is a bit expensive as it has to walk the clock tree to find
> the clock frequency (at least the first time?). just wondering if
> there is lighter alternative here?
>
How about get_clock? Doesn't read the registers at least.

Regards,
Kamlesh
Kumar, Udit Feb. 6, 2024, 2:14 p.m. UTC | #3 
On 2/6/2024 6:44 PM, Nishanth Menon wrote:
> On 16:13-20240206, Udit Kumar wrote:
>> Most of clocks and their parents are defined in contiguous range,
>> But in few cases, there is gap in clock numbers[0].
>> Driver assumes clocks to be in contiguous range, and add their clock
>> ids incrementally.
>>
>> New firmware started returning error while calling get_freq and is_on
>> API for non-available clock ids.
>>
>> In this fix, driver checks and adds only valid clock ids.
>>
>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
>>
>> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
>> Section Clocks for NAVSS0_CPTS_0 Device,
>> clock id 12-15 not present.
>>
>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>> ---
>> Changelog
>>
>> Changes in v2
>> - Updated commit message
>> - Simplified logic for valid clock id
>> link to v1 https://lore.kernel.org/all/20240205044557.3340848-1-u-kumar1@ti.com/
>>
>>
>> P.S
>> Firmawre returns total num_parents count including non available ids.
>> For above device id NAVSS0_CPTS_0, number of parents clocks are 16
>> i.e from id 2 to 17. But out of these ids few are not valid.
>> So driver adds only valid clock ids out ot total.
>>
>> Original logs
>> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
>> Line 2630 for error
>>
>> Logs with fix v2
>> https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9
>> Line 2591
>>
>>
>>   drivers/clk/keystone/sci-clk.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 35fe197dd303..ff249cbd54a1 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>>   	int num_clks = 0;
>>   	int num_parents;
>>   	int clk_id;
>> +	u64 freq;
>>   	const char * const clk_names[] = {
>>   		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
>>   	};
>> @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>>   				clk_id = args.args[1] + 1;
>>   
>>   				while (num_parents--) {
>> +					/* Check if this clock id is valid */
>> +					ret = provider->ops->get_freq(provider->sci,
>> +						sci_clk->dev_id, clk_id, &freq);
> get_freq is a bit expensive as it has to walk the clock tree to find
> the clock frequency (at least the first time?). just wondering if
> there is lighter alternative here?


Let me check , if we have some other alternative here

>> +
>> +					clk_id++;
>> +					if (ret)
>> +						continue;
>> +
>>   					sci_clk = devm_kzalloc(dev,
>>   							       sizeof(*sci_clk),
>>   							       GFP_KERNEL);
>>   					if (!sci_clk)
>>   						return -ENOMEM;
>>   					sci_clk->dev_id = args.args[0];
>> -					sci_clk->clk_id = clk_id++;
>> +					sci_clk->clk_id = clk_id - 1;
>>   					sci_clk->provider = provider;
>>   					list_add_tail(&sci_clk->node, &clks);
>> -
> 	Spurious change.

I think, you meant by deleting the line ?

If yes then will address in next version


>>   					num_clks++;
>>   				}
>>   			}
>> -- 
>> 2.34.1
>>
Kumar, Udit Feb. 6, 2024, 2:15 p.m. UTC | #4 
On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> On 16:13-20240206, Udit Kumar wrote:
>>> Most of clocks and their parents are defined in contiguous range,
>>> But in few cases, there is gap in clock numbers[0].
>>> Driver assumes clocks to be in contiguous range, and add their clock
>>> ids incrementally.
>>>
>>> New firmware started returning error while calling get_freq and is_on
>>> API for non-available clock ids.
>>>
>>> In this fix, driver checks and adds only valid clock ids.
>>>
>>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
>>>
>>> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
>>> Section Clocks for NAVSS0_CPTS_0 Device,
>>> clock id 12-15 not present.
>>>
>>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>>>   				while (num_parents--) {
>>> +					/* Check if this clock id is valid */
>>> +					ret = provider->ops->get_freq(provider->sci,
>>> +						sci_clk->dev_id, clk_id, &freq);
>> get_freq is a bit expensive as it has to walk the clock tree to find
>> the clock frequency (at least the first time?). just wondering if
>> there is lighter alternative here?
>>
> How about get_clock? Doesn't read the registers at least.

Said API needs, some flags to be passed,

Can those flag be set to zero, Chandru ?


> Regards,
> Kamlesh
Kamlesh Gurudasani Feb. 6, 2024, 2:21 p.m. UTC | #5 
Udit Kumar <u-kumar1@ti.com> writes:

> Most of clocks and their parents are defined in contiguous range,
> But in few cases, there is gap in clock numbers[0].
> Driver assumes clocks to be in contiguous range, and add their clock
> ids incrementally.
>
> New firmware started returning error while calling get_freq and is_on
> API for non-available clock ids.
>
> In this fix, driver checks and adds only valid clock ids.
>
> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
>
> [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
> Section Clocks for NAVSS0_CPTS_0 Device,
> clock id 12-15 not present.
>
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
..
> @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  				clk_id = args.args[1] + 1;
>  
>  				while (num_parents--) {
> +					/* Check if this clock id is valid */
> +					ret = provider->ops->get_freq(provider->sci,
> +						sci_clk->dev_id, clk_id, &freq);


> +
> +					clk_id++;
Why increment it here and subtract if get_freq succeeds (sci_clk->clk_id = clk_id - 1;), rather
if(ret) {
        clk_id++;
        continue;
}
> +					if (ret)
> +						continue;

> +
>  					sci_clk = devm_kzalloc(dev,
>  							       sizeof(*sci_clk),
>  							       GFP_KERNEL);
>  					if (!sci_clk)
>  						return -ENOMEM;
>  					sci_clk->dev_id = args.args[0];
> -					sci_clk->clk_id = clk_id++;
> +					sci_clk->clk_id = clk_id - 1;
and keep sci_clk->clk_id = clk_id++; intact

saves one subtraction

or even better

 - 				clk_id = args.args[1] + 1;
 + 				clk_id = args.args[1];
  				while (num_parents--) {
 +					/* Check if this clock id is valid */
 +					ret = provider->ops->get_freq(provider->sci,
 +						sci_clk->dev_id, ++clk_id, &freq);

and then no increments after, for clk_id

Regards,
Kamlesh
CHANDRU DHAVAMANI Feb. 6, 2024, 2:26 p.m. UTC | #6 
On 06/02/24 19:45, Kumar, Udit wrote:
>
> On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> On 16:13-20240206, Udit Kumar wrote:
>>>> Most of clocks and their parents are defined in contiguous range,
>>>> But in few cases, there is gap in clock numbers[0].
>>>> Driver assumes clocks to be in contiguous range, and add their clock
>>>> ids incrementally.
>>>>
>>>> New firmware started returning error while calling get_freq and is_on
>>>> API for non-available clock ids.
>>>>
>>>> In this fix, driver checks and adds only valid clock ids.
>>>>
>>>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for 
>>>> dynamically probing clocks")
>>>>
>>>> [0] 
>>>> https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html 
>>>>
>>>> Section Clocks for NAVSS0_CPTS_0 Device,
>>>> clock id 12-15 not present.
>>>>
>>>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>>>>                   while (num_parents--) {
>>>> +                    /* Check if this clock id is valid */
>>>> +                    ret = provider->ops->get_freq(provider->sci,
>>>> +                        sci_clk->dev_id, clk_id, &freq);
>>> get_freq is a bit expensive as it has to walk the clock tree to find
>>> the clock frequency (at least the first time?). just wondering if
>>> there is lighter alternative here?
>>>
>> How about get_clock? Doesn't read the registers at least.
>
> Said API needs, some flags to be passed,
>
> Can those flag be set to zero, Chandru ?


get_clock doesn't require any flags to be passed.


>
>
>> Regards,
>> Kamlesh
Kumar, Udit Feb. 6, 2024, 2:39 p.m. UTC | #7 
On 2/6/2024 7:56 PM, CHANDRU DHAVAMANI wrote:
>
> On 06/02/24 19:45, Kumar, Udit wrote:
>>
>> On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote:
>>> Nishanth Menon <nm@ti.com> writes:
>>>
>>>> On 16:13-20240206, Udit Kumar wrote:
>>>>> Most of clocks and their parents are defined in contiguous range,
>>>>> But in few cases, there is gap in clock numbers[0].
>>>>> Driver assumes clocks to be in contiguous range, and add their clock
>>>>> ids incrementally.
>>>>>
>>>>> New firmware started returning error while calling get_freq and is_on
>>>>> API for non-available clock ids.
>>>>>
>>>>> In this fix, driver checks and adds only valid clock ids.
>>>>>
>>>>> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for 
>>>>> dynamically probing clocks")
>>>>>
>>>>> [0] 
>>>>> https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html 
>>>>>
>>>>> Section Clocks for NAVSS0_CPTS_0 Device,
>>>>> clock id 12-15 not present.
>>>>>
>>>>> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
>>>>>                   while (num_parents--) {
>>>>> +                    /* Check if this clock id is valid */
>>>>> +                    ret = provider->ops->get_freq(provider->sci,
>>>>> +                        sci_clk->dev_id, clk_id, &freq);
>>>> get_freq is a bit expensive as it has to walk the clock tree to find
>>>> the clock frequency (at least the first time?). just wondering if
>>>> there is lighter alternative here?
>>>>
>>> How about get_clock? Doesn't read the registers at least.
>>
>> Said API needs, some flags to be passed,
>>
>> Can those flag be set to zero, Chandru ?
>
>
> get_clock doesn't require any flags to be passed.


May be firmware does not need it but  I was referring to

https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78



>
>
>>
>>
>>> Regards,
>>> Kamlesh
Kamlesh Gurudasani Feb. 6, 2024, 3:54 p.m. UTC | #8 
"Kumar, Udit" <u-kumar1@ti.com> writes:

>>>>> get_freq is a bit expensive as it has to walk the clock tree to find
>>>>> the clock frequency (at least the first time?). just wondering if
>>>>> there is lighter alternative here?
>>>>>
>>>> How about get_clock? Doesn't read the registers at least.
>>>
>>> Said API needs, some flags to be passed,
>>>
>>> Can those flag be set to zero, Chandru ?
>>
>>
>> get_clock doesn't require any flags to be passed.
>
>
> May be firmware does not need it but  I was referring to
>
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78
Just took a look,

I now understand the reason for confusion,

#define TI_SCI_MSG_SET_CLOCK_STATE	0x0100
#define TI_SCI_MSG_GET_CLOCK_STATE	0x0101

cops->get_clock = ti_sci_cmd_get_clock;  --> refers to
TI_SCI_MSG_SET_CLOCK_STATE
That's why we are passing the flag from linux for get_clock

Linux is using terminology of get/put.

As Chandru pointed, we don't have to pass flags, cause he is refering
to TI_SCI_MSG_GET_CLOCK_STATE

Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what
we actually want.
cops->is_auto = ti_sci_cmd_clk_is_auto;
cops->is_on = ti_sci_cmd_clk_is_on;
cops->is_off = ti_sci_cmd_clk_is_off;

Which should be safe to call, Chandru can confirm.

Regards,
Kamlesh
>
>
>
>>
>>
>>>
>>>
>>>> Regards,
>>>> Kamlesh
Kumar, Udit Feb. 7, 2024, 5:33 a.m. UTC | #9 
On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote:
> "Kumar, Udit" <u-kumar1@ti.com> writes:
>
>>>>>> get_freq is a bit expensive as it has to walk the clock tree to find
>>>>>> the clock frequency (at least the first time?). just wondering if
>>>>>> there is lighter alternative here?
>>>>>>
>>>>> How about get_clock? Doesn't read the registers at least.
>>>> Said API needs, some flags to be passed,
>>>>
>>>> Can those flag be set to zero, Chandru ?
>>>
>>> get_clock doesn't require any flags to be passed.
>>
>> May be firmware does not need it but  I was referring to
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78
> Just took a look,
>
> I now understand the reason for confusion,
>
> #define TI_SCI_MSG_SET_CLOCK_STATE	0x0100
> #define TI_SCI_MSG_GET_CLOCK_STATE	0x0101
>
> cops->get_clock = ti_sci_cmd_get_clock;  --> refers to
> TI_SCI_MSG_SET_CLOCK_STATE
> That's why we are passing the flag from linux for get_clock
>
> Linux is using terminology of get/put.
>
> As Chandru pointed, we don't have to pass flags, cause he is refering
> to TI_SCI_MSG_GET_CLOCK_STATE
>
> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what
> we actually want.
> cops->is_auto = ti_sci_cmd_clk_is_auto;
> cops->is_on = ti_sci_cmd_clk_is_on;
> cops->is_off = ti_sci_cmd_clk_is_off;


I think calling ti_sci_cmd_clk_is_auto should be good . other functions 
needs current state and requested state.

Chandru ?

>
> Which should be safe to call, Chandru can confirm.
>
> Regards,
> Kamlesh
>>
>>
>>>
>>>>
>>>>> Regards,
>>>>> Kamlesh
CHANDRU DHAVAMANI Feb. 7, 2024, 7:23 a.m. UTC | #10 
On 07/02/24 11:03, Kumar, Udit wrote:
>
> On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote:
>> "Kumar, Udit" <u-kumar1@ti.com> writes:
>>
>>>>>>> get_freq is a bit expensive as it has to walk the clock tree to 
>>>>>>> find
>>>>>>> the clock frequency (at least the first time?). just wondering if
>>>>>>> there is lighter alternative here?
>>>>>>>
>>>>>> How about get_clock? Doesn't read the registers at least.
>>>>> Said API needs, some flags to be passed,
>>>>>
>>>>> Can those flag be set to zero, Chandru ?
>>>>
>>>> get_clock doesn't require any flags to be passed.
>>>
>>> May be firmware does not need it but  I was referring to
>>>
>>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 
>>>
>> Just took a look,
>>
>> I now understand the reason for confusion,
>>
>> #define TI_SCI_MSG_SET_CLOCK_STATE    0x0100
>> #define TI_SCI_MSG_GET_CLOCK_STATE    0x0101
>>
>> cops->get_clock = ti_sci_cmd_get_clock;  --> refers to
>> TI_SCI_MSG_SET_CLOCK_STATE
>> That's why we are passing the flag from linux for get_clock
>>
>> Linux is using terminology of get/put.
>>
>> As Chandru pointed, we don't have to pass flags, cause he is refering
>> to TI_SCI_MSG_GET_CLOCK_STATE
>>
>> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what
>> we actually want.
>> cops->is_auto = ti_sci_cmd_clk_is_auto;
>> cops->is_on = ti_sci_cmd_clk_is_on;
>> cops->is_off = ti_sci_cmd_clk_is_off;
>
>
> I think calling ti_sci_cmd_clk_is_auto should be good . other 
> functions needs current state and requested state.
>
> Chandru ?
>

ti_sci_cmd_clk_is_auto is internal function to linux.
For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to the 
variables where result will be stored.


>>
>> Which should be safe to call, Chandru can confirm.
>>
>> Regards,
>> Kamlesh
>>>
>>>
>>>>
>>>>>
>>>>>> Regards,
>>>>>> Kamlesh
Kumar, Udit Feb. 7, 2024, 7:33 a.m. UTC | #11 
On 2/7/2024 12:53 PM, CHANDRU DHAVAMANI wrote:
>
> On 07/02/24 11:03, Kumar, Udit wrote:
>>
>> On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote:
>>> "Kumar, Udit" <u-kumar1@ti.com> writes:
>>>
>>>>>>>> get_freq is a bit expensive as it has to walk the clock tree to 
>>>>>>>> find
>>>>>>>> the clock frequency (at least the first time?). just wondering if
>>>>>>>> there is lighter alternative here?
>>>>>>>>
>>>>>>> How about get_clock? Doesn't read the registers at least.
>>>>>> Said API needs, some flags to be passed,
>>>>>>
>>>>>> Can those flag be set to zero, Chandru ?
>>>>>
>>>>> get_clock doesn't require any flags to be passed.
>>>>
>>>> May be firmware does not need it but  I was referring to
>>>>
>>>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 
>>>>
>>> Just took a look,
>>>
>>> I now understand the reason for confusion,
>>>
>>> #define TI_SCI_MSG_SET_CLOCK_STATE    0x0100
>>> #define TI_SCI_MSG_GET_CLOCK_STATE    0x0101
>>>
>>> cops->get_clock = ti_sci_cmd_get_clock;  --> refers to
>>> TI_SCI_MSG_SET_CLOCK_STATE
>>> That's why we are passing the flag from linux for get_clock
>>>
>>> Linux is using terminology of get/put.
>>>
>>> As Chandru pointed, we don't have to pass flags, cause he is refering
>>> to TI_SCI_MSG_GET_CLOCK_STATE
>>>
>>> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what
>>> we actually want.
>>> cops->is_auto = ti_sci_cmd_clk_is_auto;
>>> cops->is_on = ti_sci_cmd_clk_is_on;
>>> cops->is_off = ti_sci_cmd_clk_is_off;
>>
>>
>> I think calling ti_sci_cmd_clk_is_auto should be good . other 
>> functions needs current state and requested state.
>>
>> Chandru ?
>>
>
> ti_sci_cmd_clk_is_auto is internal function to linux.
> For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to 
> the variables where result will be stored.


Yes this internal function calls TI_SCI_MSG_GET_CLOCK_STATE


>
>
>>>
>>> Which should be safe to call, Chandru can confirm.
>>>
>>> Regards,
>>> Kamlesh
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> Kamlesh
CHANDRU DHAVAMANI Feb. 7, 2024, 8:06 a.m. UTC | #12 
On 07/02/24 13:03, Kumar, Udit wrote:
>
> On 2/7/2024 12:53 PM, CHANDRU DHAVAMANI wrote:
>>
>> On 07/02/24 11:03, Kumar, Udit wrote:
>>>
>>> On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote:
>>>> "Kumar, Udit" <u-kumar1@ti.com> writes:
>>>>
>>>>>>>>> get_freq is a bit expensive as it has to walk the clock tree 
>>>>>>>>> to find
>>>>>>>>> the clock frequency (at least the first time?). just wondering if
>>>>>>>>> there is lighter alternative here?
>>>>>>>>>
>>>>>>>> How about get_clock? Doesn't read the registers at least.
>>>>>>> Said API needs, some flags to be passed,
>>>>>>>
>>>>>>> Can those flag be set to zero, Chandru ?
>>>>>>
>>>>>> get_clock doesn't require any flags to be passed.
>>>>>
>>>>> May be firmware does not need it but  I was referring to
>>>>>
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 
>>>>>
>>>> Just took a look,
>>>>
>>>> I now understand the reason for confusion,
>>>>
>>>> #define TI_SCI_MSG_SET_CLOCK_STATE    0x0100
>>>> #define TI_SCI_MSG_GET_CLOCK_STATE    0x0101
>>>>
>>>> cops->get_clock = ti_sci_cmd_get_clock;  --> refers to
>>>> TI_SCI_MSG_SET_CLOCK_STATE
>>>> That's why we are passing the flag from linux for get_clock
>>>>
>>>> Linux is using terminology of get/put.
>>>>
>>>> As Chandru pointed, we don't have to pass flags, cause he is refering
>>>> to TI_SCI_MSG_GET_CLOCK_STATE
>>>>
>>>> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what
>>>> we actually want.
>>>> cops->is_auto = ti_sci_cmd_clk_is_auto;
>>>> cops->is_on = ti_sci_cmd_clk_is_on;
>>>> cops->is_off = ti_sci_cmd_clk_is_off;
>>>
>>>
>>> I think calling ti_sci_cmd_clk_is_auto should be good . other 
>>> functions needs current state and requested state.
>>>
>>> Chandru ?
>>>
>>
>> ti_sci_cmd_clk_is_auto is internal function to linux.
>> For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to 
>> the variables where result will be stored.
>
>
> Yes this internal function calls TI_SCI_MSG_GET_CLOCK_STATE
>

Okay. We can use TI_SCI_MSG_GET_CLOCK_STATE to check to if clock is 
valid or not.


>
>>
>>
>>>>
>>>> Which should be safe to call, Chandru can confirm.
>>>>
>>>> Regards,
>>>> Kamlesh
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Kamlesh
diff mbox series

Patch

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 35fe197dd303..ff249cbd54a1 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -517,6 +517,7 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 	int num_clks = 0;
 	int num_parents;
 	int clk_id;
+	u64 freq;
 	const char * const clk_names[] = {
 		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
 	};
@@ -586,16 +587,23 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 				clk_id = args.args[1] + 1;
 
 				while (num_parents--) {
+					/* Check if this clock id is valid */
+					ret = provider->ops->get_freq(provider->sci,
+						sci_clk->dev_id, clk_id, &freq);
+
+					clk_id++;
+					if (ret)
+						continue;
+
 					sci_clk = devm_kzalloc(dev,
 							       sizeof(*sci_clk),
 							       GFP_KERNEL);
 					if (!sci_clk)
 						return -ENOMEM;
 					sci_clk->dev_id = args.args[0];
-					sci_clk->clk_id = clk_id++;
+					sci_clk->clk_id = clk_id - 1;
 					sci_clk->provider = provider;
 					list_add_tail(&sci_clk->node, &clks);
-
 					num_clks++;
 				}
 			}