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 13D58E74901 for ; Mon, 2 Oct 2023 17:58:31 +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=TCLplcqkwZhmG3iwnCLuTaKQiftT4aFiF0RgyfJVcrM=; b=KH0zqE4xrs0p8b UOlHKzHYlbriot2eFfW9KzvC4/VgpQPbcWXNileHQBbcZqZ1f6Usvget2p4lCyQT68Dh0SANT8A7d V0rHb7vCbLCEIe4RbYSTW9W3vzMOqdbnpaMScuzmsPLh5joNENOXNS/vdCq3xVliZz79ERERMUva1 jPOMYDK21Sq+QCSDLu81zXgLYzKckOYZgR+Pck2OyqLYFjb6H4ketIhpxedEyN91IepuQGrR2F+Vs cEo4yiJqf7oDIRlGe8su+CZPAh+jlJ0hgGy4UmxsrFAYeE8U38K2c/v3rRDHfqZj70FbYP6NIgdCF TR8ZgifiSjr6Ecv65jcA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qnNBC-00DBDp-2D; Mon, 02 Oct 2023 17:58:02 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qnNB8-00DBDD-2B for linux-arm-kernel@lists.infradead.org; Mon, 02 Oct 2023 17:58:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C9281C15; Mon, 2 Oct 2023 10:58:34 -0700 (PDT) Received: from pluto (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 349313F59C; Mon, 2 Oct 2023 10:57:54 -0700 (PDT) Date: Mon, 2 Oct 2023 18:57:51 +0100 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Sudeep Holla , Michael Turquette , Stephen Boyd , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, Peng Fan Subject: Re: [PATCH v3 2/2] clk: scmi: add set/get_parent support Message-ID: References: <20231001-scmi-clock-v2-v3-0-898bd92d8939@nxp.com> <20231001-scmi-clock-v2-v3-2-898bd92d8939@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231001-scmi-clock-v2-v3-2-898bd92d8939@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231002_105758_809875_6EEE1227 X-CRM114-Status: GOOD ( 29.77 ) 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 Sun, Oct 01, 2023 at 12:38:44PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > to support them. > Hi, a few notes down below. > Signed-off-by: Peng Fan > --- > drivers/clk/clk-scmi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2e1337b511eb..5aaca674830f 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -24,6 +24,7 @@ struct scmi_clk { > struct clk_hw hw; > const struct scmi_clock_info *info; > const struct scmi_protocol_handle *ph; > + struct clk_parent_data *parent_data; > }; > > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) > @@ -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > } > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > +} > + > +static u8 scmi_clk_get_parent(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + u32 parent_id; > + int ret; > + > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > + if (ret) > + return 0; > + > + return parent_id; > +} > + While testing using CLK Debugfs with CLOCK_ALLOW_WRITE_DEBUGFS 1 I noticed that I can correctly change the clk_parent and then read back the clk_possible_parents, BUT if I read clk_parent right after boot (OR after loading the clk-scmi module) I cannot get back any value from debugfs even though I can see the correct SCMI messages being exchanged from the traces. My guess was that, while scmi_clk_set_parent is invoked by the CLK core with a parent_index that has been remapped by the core to the SCMI clock domain ID, this is not done by scmi_clk_get_parent() so you are returning to the clock framework as parent_id the raw SCMI clock domain id as returned by the platform instead of the clk parent id used by the core. This does not happen after you issue at first a reparent because in that case on the following read of clk_parent the CLK framework returns the last value you have set that it had cached previously. This fixes for me the issue: ---8<---- diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 5aaca674830f..fd47232d4021 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -89,14 +89,21 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) static u8 scmi_clk_get_parent(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - u32 parent_id; + u32 parent_id, p_idx; int ret; ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); if (ret) return 0; - return parent_id; + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++) + if (clk->parent_data[p_idx].index == parent_id) + break; + + if (p_idx == clk->info->num_parents) + return 0; + + return p_idx; } ----8<----- Not sure if there is a clever way to do it. Aside from this, another inherent issue is that you cannot really return an error from .get_parent() so if the SCMI get_parent ops should fail (ex. timeout) you return 0... (and me too in the above fix) but this is due to the CLK framework callback definition itself. Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel