diff mbox series

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

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

Commit Message

Kumar, Udit Feb. 13, 2024, 8:26 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.

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

Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
Signed-off-by: Udit Kumar <u-kumar1@ti.com>
---
Changelog
Changes in v4
- Added only incremental chanegs as per v3 discussion
- Updated commit message
- v3 was Reviewed-by, Dropping Reviewed-by as logic is changed
Link to v3
https://lore.kernel.org/all/20240207091100.4001428-1-u-kumar1@ti.com/ 

Changes in v3
- instead of get_freq, is_auto API is used to check validilty of clock
- Address comments of v2, to have preindex increment
Link to v2 https://lore.kernel.org/all/20240206104357.3803517-1-u-kumar1@ti.com/

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 v4
https://gist.github.com/uditkumarti/f25482a5e18e918010b790cffb39f572
Line 2640


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

Comments

Nishanth Menon Feb. 16, 2024, 5:09 p.m. UTC | #1 
On 13:56-20240213, 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.
> 
> [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.
> 
> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
> ---
> Changelog
> Changes in v4
> - Added only incremental chanegs as per v3 discussion
> - Updated commit message
> - v3 was Reviewed-by, Dropping Reviewed-by as logic is changed
> Link to v3
> https://lore.kernel.org/all/20240207091100.4001428-1-u-kumar1@ti.com/ 
> 
> Changes in v3
> - instead of get_freq, is_auto API is used to check validilty of clock
> - Address comments of v2, to have preindex increment
> Link to v2 https://lore.kernel.org/all/20240206104357.3803517-1-u-kumar1@ti.com/
> 
> 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 v4
> https://gist.github.com/uditkumarti/f25482a5e18e918010b790cffb39f572
> Line 2640
> 
> 
>  drivers/clk/keystone/sci-clk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 35fe197dd303..eb2ef44869b2 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -516,6 +516,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  	struct sci_clk *sci_clk, *prev;
>  	int num_clks = 0;
>  	int num_parents;
> +	bool state;
>  	int clk_id;
>  	const char * const clk_names[] = {
>  		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
> @@ -586,6 +587,15 @@ 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->is_auto(provider->sci,
> +						sci_clk->dev_id, clk_id, &state);
> +
> +					if (ret) {
> +						clk_id++;
> +						continue;
> +					}
> +


Thanks. This makes more sense.

Reviewed-by: Nishanth Menon <nm@ti.com>
>  					sci_clk = devm_kzalloc(dev,
>  							       sizeof(*sci_clk),
>  							       GFP_KERNEL);
> -- 
> 2.34.1
>
Stephen Boyd Feb. 22, 2024, 5:59 a.m. UTC | #2 
Quoting Udit Kumar (2024-02-13 00:26:40)
> 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.
> 
> [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.
> 
> Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks")
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
> ---

Applied to clk-next
Francesco Dolcini Feb. 26, 2024, 10:54 a.m. UTC | #3 
On Tue, Feb 13, 2024 at 01:56:40PM +0530, 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.

Is this the kind of errors I should expect in such situation?

ti-sci-clk 44043000.system-controller:clock-controller: recalc-rate failed for dev=13, clk=7, ret=-19

If this is the case, I feel like this patch should be back-ported to
stable kernels.

Any malfunction because of these errors or just some noise in the logs?

Francesco
Kumar, Udit Feb. 26, 2024, 1:47 p.m. UTC | #4 
On 2/26/2024 4:24 PM, Francesco Dolcini wrote:
> On Tue, Feb 13, 2024 at 01:56:40PM +0530, 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.
> Is this the kind of errors I should expect in such situation?
>
> ti-sci-clk 44043000.system-controller:clock-controller: recalc-rate failed for dev=13, clk=7, ret=-19
>
> If this is the case, I feel like this patch should be back-ported to
> stable kernels.

Sure will send to stable@vger.kernel.org


> Any malfunction because of these errors or just some noise in the logs?

Error is noise in logs, no impact on function as these reserved clocks

are not used by drivers.


>
> Francesco
>
diff mbox series

Patch

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 35fe197dd303..eb2ef44869b2 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -516,6 +516,7 @@  static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
 	struct sci_clk *sci_clk, *prev;
 	int num_clks = 0;
 	int num_parents;
+	bool state;
 	int clk_id;
 	const char * const clk_names[] = {
 		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
@@ -586,6 +587,15 @@  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->is_auto(provider->sci,
+						sci_clk->dev_id, clk_id, &state);
+
+					if (ret) {
+						clk_id++;
+						continue;
+					}
+
 					sci_clk = devm_kzalloc(dev,
 							       sizeof(*sci_clk),
 							       GFP_KERNEL);