drivers/clk/keystone/sci-clk.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
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);
+
+ 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++;
}
}
--
2.34.1Udit 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
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
>
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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
>>
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
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
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
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
"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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.