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 <[email protected]>
---
Changelog
Changes in v2
- Updated commit message
- Simplified logic for valid clock id
link to v1 https://lore.kernel.org/all/[email protected]/
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.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 <[email protected]>
> ---
> Changelog
>
> Changes in v2
> - Updated commit message
> - Simplified logic for valid clock id
> link to v1 https://lore.kernel.org/all/[email protected]/
>
>
> 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
Nishanth Menon <[email protected]> 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 <[email protected]>
>> 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
Udit Kumar <[email protected]> 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 <[email protected]>
..
> @@ -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 06/02/24 19:45, Kumar, Udit wrote:
>
> On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote:
>> Nishanth Menon <[email protected]> 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 <[email protected]>
>>>> 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 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 <[email protected]>
>> ---
>> Changelog
>>
>> Changes in v2
>> - Updated commit message
>> - Simplified logic for valid clock id
>> link to v1 https://lore.kernel.org/all/[email protected]/
>>
>>
>> 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
>>
On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote:
> Nishanth Menon <[email protected]> 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 <[email protected]>
>>> 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 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 <[email protected]> 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 <[email protected]>
>>>>> 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" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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