Received-SPF: pass (google.com: domain of linux-kernel+bounces-52779-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 5 Feb 2024 08:04:59 -0600
From: Nishanth Menon <nm@ti.com>
To: Udit Kumar <u-kumar1@ti.com>
CC: <kristo@kernel.org>, <ssantosh@kernel.org>, <vigneshr@ti.com>,
        <mturquette@baylibre.com>, <sboyd@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
        <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v1] clk: keystone: sci-clk: Adding support for non
 contiguous clocks
Message-ID: <20240205140459.orjvjqqtiugmyosc@obscurity>
References: <20240205044557.3340848-1-u-kumar1@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240205044557.3340848-1-u-kumar1@ti.com>

On 10:15-20240205, 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 assigns
> accordingly.
> 
> New firmware started returning error in case of
> non-available clock id.  Therefore drivers throws error while
> re-calculate and other functions.

What changed here? started returning error for what API? also please fix
up 70 char alignment -> there extra spaces in your commit message.
> 
> In this fix, before assigning and adding clock in list,
> driver checks if given clock is valid or not.
> 
> 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 and 18-19 not present
> 
> Signed-off-by: Udit Kumar <u-kumar1@ti.com>
> ---
> Original logs
> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs
> Line 2630 for error
> 
> Logs with fix
> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix
> Line 2594 
> 
>  drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 35fe197dd303..d417ec018d82 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  	int num_clks = 0;
>  	int num_parents;
>  	int clk_id;
> +	int max_clk_id;
> +	u64 freq;
>  	const char * const clk_names[] = {
>  		"clocks", "assigned-clocks", "assigned-clock-parents", NULL
>  	};
> @@ -584,6 +586,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  				}
>  
>  				clk_id = args.args[1] + 1;
> +				max_clk_id = clk_id + num_parents;
>  
>  				while (num_parents--) {
>  					sci_clk = devm_kzalloc(dev,
> @@ -592,11 +595,20 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
>  					if (!sci_clk)
>  						return -ENOMEM;
>  					sci_clk->dev_id = args.args[0];
> -					sci_clk->clk_id = clk_id++;
> -					sci_clk->provider = provider;
> -					list_add_tail(&sci_clk->node, &clks);
> +					/* check if given clock id is valid by calling get_freq */
> +					/* loop over max possible ids */
> +					do {
> +						sci_clk->clk_id = clk_id++;
>  
> -					num_clks++;
> +						ret = provider->ops->get_freq(provider->sci,
> +							   sci_clk->dev_id, sci_clk->clk_id, &freq);
> +					} while (ret != 0 && clk_id < max_clk_id);

take clock ids 0 1 2 3 -> Say 2 is reserved.
num_parents = 4
while(num_parents) Loop 1 ->  clk ID 0 is valid, list_add_tail
while(num_parents) Loop 2 ->  clk ID 1 is valid, list_add_tail
while(num_parents) Loop 3 ->  clk ID 2 is invalid.. so we scan forward
	to clk ID 3 -> list_add_tail
while(num_parents) Loop 4 ->  clk ID 4 is invalid.. but 5 is out of
	range, so we break off loop. sci_clk is still devm_kzalloced ->
	but since clk_id > max_clk_id, we jump off loop, and we dont add
	it to tail. so one extra allocation?
If we have multiple reserved intermediate ones, then we'd have as many
allocations that aren't linked? Could we not improve the logic a bit to
allocate just what is necessary?

> +
> +					sci_clk->provider = provider;
> +					if (ret == 0) {
> +						list_add_tail(&sci_clk->node, &clks);
> +						num_clks++;
> +					}
>  				}
>  			}
>  
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D