From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7FEC5C48295 for ; Mon, 5 Feb 2024 14:05:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:CC:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AgDBCmwtLNnZdeCgiyNhxxc3jMK+KYZSvEY6QkB0X+4=; b=g0nSTp20Dpz192 VpaWWXlSAFisNfZGCIcHygOue1X5x4qL09NzSjAENij5fr91o2ZUQSVPJr8EPCYdyup61JKsaWRHI PYl1LNACm5Y1GHmdZat6OBdrf3rc8dGTee4RCyRDIukc6FKdFe6ugFftpnDOjUtANoD9wzj2rIw27 0v8XW+2agDYgbvv8jXnf9QYjhfkp9MSLJTyWIF0KdRL15jjA7RYOHy38IHK3ouXq76hoqqdyNbWWW E19NEGMaENg/mUTop9ONEFnsjYhlD+m/+Cl15HPVL0xVMuqiiyxF0/fplwV0JbmnteeOQlWhrMH2f kzj63xjdxR5DWf0QrGmA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rWzax-00000003Wnp-0WMp; Mon, 05 Feb 2024 14:05:11 +0000 Received: from fllv0015.ext.ti.com ([198.47.19.141]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rWzat-00000003Wm8-2EkZ for linux-arm-kernel@lists.infradead.org; Mon, 05 Feb 2024 14:05:09 +0000 Received: from lelv0265.itg.ti.com ([10.180.67.224]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 415E50ig023002; Mon, 5 Feb 2024 08:05:00 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1707141900; bh=tWHCZPGdnVP9n6u5fBWigVBsLLeSsPy8VL0rQblycqY=; h=Date:From:To:CC:Subject:References:In-Reply-To; b=mOezREo/iNM/aeAbvC6SOz5GAuInx3j8MW55bLKM71fM4VJ8t7Nk0HvsZgKAxwML/ Wc+xXyOGBSXbDMZxmE92/1uDO0X9fWFd2dAkEGhGc5H0GgBfAJ0Lx68tWz0/OwGrIa AwQub8cyiH8ru32LKiG/oFJ6RnenEzWRQU1lRTNE= Received: from DFLE113.ent.ti.com (dfle113.ent.ti.com [10.64.6.34]) by lelv0265.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 415E50SH032340 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 5 Feb 2024 08:05:00 -0600 Received: from DFLE103.ent.ti.com (10.64.6.24) by DFLE113.ent.ti.com (10.64.6.34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Mon, 5 Feb 2024 08:04:59 -0600 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DFLE103.ent.ti.com (10.64.6.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Mon, 5 Feb 2024 08:04:59 -0600 Received: from localhost (uda0133052.dhcp.ti.com [128.247.81.232]) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 415E4xjt050034; Mon, 5 Feb 2024 08:04:59 -0600 Date: Mon, 5 Feb 2024 08:04:59 -0600 From: Nishanth Menon To: Udit Kumar CC: , , , , , , , 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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240205044557.3340848-1-u-kumar1@ti.com> X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240205_060507_748861_80D39B43 X-CRM114-Status: GOOD ( 23.60 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > --- > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel