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 44026CE79AD for ; Wed, 20 Sep 2023 07:22:59 +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:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ragQ4GPvIr6dLOm0OUPBUO20B+9W5WTKdPm79Fbn1ko=; b=fUpqDOsSFr+1bk QIfcSYZsg6pFW/mLSc6U48r482jC4s7OVBFvVgdc1TAEmMI/Sb1MQqe8UFbwXLW9p8+gRk3TeIJFU /LiDfYP76GLq4qKPjIXlkgmdTotekn+UFMC1Ky4iBdzWZEVssWYyBvB5qVrgDPqCIv3tLW1xbO/zl jcEft6KcRGfI48pe435hR1Z1Veo6yutKHOoM/eWqVFIX9D6CdN1PPDAoaBGAGbPCff0AKZo99DffB w84O8OWIHuoLpmGUC9vkfDzJeBrf1f3RQrlCQnzDov0NNvvNHeBA77wwPNpHyWiJ5zayh/qRl5cGg KCsooWOM7eUg9WnZtNFQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qirXd-0027rD-0Y; Wed, 20 Sep 2023 07:22:33 +0000 Received: from mail-wm1-x32f.google.com ([2a00:1450:4864:20::32f]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qirXa-0027qE-0e for linux-arm-kernel@lists.infradead.org; Wed, 20 Sep 2023 07:22:31 +0000 Received: by mail-wm1-x32f.google.com with SMTP id 5b1f17b1804b1-404571cbb8dso68879805e9.3 for ; Wed, 20 Sep 2023 00:22:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695194544; x=1695799344; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=2bhVmf9Xb0W3PnpeUv/b1V4dB6FaBxpZCDQFmYEoS+s=; b=nrMwTnvST0wQ0rX5Xiw0qplW7fKXqUY85m9djYGJxXSAEkQx9KOmhAKuEhNUP/yEaD egDVM/wPn24snv+1d1sMK95zkc4M/S23dpWDkqvGiF1iY1sJdRrQpDfIXWHPAt3IyCeo xA10rFRb0uuxhIyK2Ppov1hQY5U6MRPwZxoHebs66//wMBiBjB/QhnQuTjV1L5WNlK5R iPFhlTtJLiivK+hU+2Lpe/07H90A9bNqtDZSO/yvwlFxgtsttCSImkcCBXETkk56/64q CPnClz+rRce7cnBhRAKcSmZrFk+0rIF9bhizUwB7LvJikbE4uFFIsrMALmEJwqBdgSor UUog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695194544; x=1695799344; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2bhVmf9Xb0W3PnpeUv/b1V4dB6FaBxpZCDQFmYEoS+s=; b=jzeA25kz8nfZRrEMTWmrRnNn5uh3QZgWhkwkOHRbQklkSyJmsBfLjiJ/Rs0xsXGEpB 42kSQHrcwsIRW3ZRWzIuOxT6jCe55dFlbd3PPfDgPGZsP/fKb4uViaC6lHZx3x9VRscz GZ24I/YVMJC7v7hX/eC21nnm+0qC2cVCk8WPtG0AZ8/yqt3+Wg2y1qX2LxDn2+e6iEj8 JuBXvMtIeVhzawhfFfmlYN6ySTKWn1fA+C25oggBUwdGV8gfbnQapycDiIRjRNCGO8Vy FW+VG47kzDSsUNtdurHemXlP6Hu29VelejxBvWc8G0+nNoDUOH7T5u2bj1mK9bs2m68v EPkg== X-Gm-Message-State: AOJu0YzBccsc4WZCUmAhxnGt6UpKPNsEEHq7TFVEVzcpKnQUmHnkAhvP v4tJy6SWlyBAqQcJVEspZj4= X-Google-Smtp-Source: AGHT+IHhx4uy7+lhAAone/xKFAY7os8e9wsVKqyqvDOdNWqQ9Nb+5vwmAcbdDNQiKjqhnufHlkQO9Q== X-Received: by 2002:a1c:f70f:0:b0:403:bb3:28c9 with SMTP id v15-20020a1cf70f000000b004030bb328c9mr1721441wmh.38.1695194543932; Wed, 20 Sep 2023 00:22:23 -0700 (PDT) Received: from PCBABN.skidata.net ([91.230.2.244]) by smtp.gmail.com with ESMTPSA id o16-20020a5d4a90000000b0031ffb51f6f9sm13119633wrq.30.2023.09.20.00.22.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 20 Sep 2023 00:22:23 -0700 (PDT) From: Benjamin Bara To: mripard@kernel.org Cc: abelvesa@kernel.org, bbara93@gmail.com, benjamin.bara@skidata.com, conor+dt@kernel.org, devicetree@vger.kernel.org, festevam@gmail.com, frank@oltmanns.dev, kernel@pengutronix.de, krzysztof.kozlowski+dt@linaro.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux@armlinux.org.uk, mturquette@baylibre.com, peng.fan@nxp.com, robh+dt@kernel.org, s.hauer@pengutronix.de, sboyd@kernel.org, shawnguo@kernel.org Subject: Re: [PATCH 06/13] clk: keep track if a clock is explicitly configured Date: Wed, 20 Sep 2023 09:22:16 +0200 Message-Id: <20230920072216.1737599-1-bbara93@gmail.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: References: MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230920_002230_242771_F11C810D X-CRM114-Status: GOOD ( 33.00 ) 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 Hi Maxime! thanks for taking the time to look through :) On Tue, 19 Sept 2023 at 09:07, Maxime Ripard wrote: > On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote: > > From: Benjamin Bara > > > > When we keep track if a clock has a given rate explicitly set by a > > consumer, we can identify unintentional clock rate changes in an easy > > way. This also helps during debugging, as one can see if a rate is set > > by accident or due to a consumer-related change. > > > > Signed-off-by: Benjamin Bara > > --- > > drivers/clk/clk.c | 25 +++++++++++++++++++++++++ > > include/linux/clk-provider.h | 1 + > > 2 files changed, 26 insertions(+) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 8f4f92547768..82c65ed432c5 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -70,6 +70,7 @@ struct clk_core { > > unsigned long rate; > > unsigned long req_rate; > > unsigned long new_rate; > > + unsigned long set_rate; > > This is pretty much what req_rate is supposed to be about. Why didn't it > work in your case? I picked this one to respond first because I think some of the implemented stuff just workarounds the current req_rate behaviour. Currently, I have two "problems" with it: 1. It's set during initialization[1]. In this phase, the *required* rate isn't known yet, so it should be 0 imo. 2. It's set during re-parenting[2,3]. Also here, just because we re-parent, the active consumer (which set the req_rate to a valid value) still requires the clock to have the same rate. That is basically the reason why we have no info if the req_rate is really "required" by a consumer or if it is just set because the parent had it at some time. It's only usage is here[4], which IMO doesn't really depends on the wrong behaviour I described above. The respective sub-tree we talk about on the imx8mp looks like this (one example for the the LVDS-only case): video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt) video_pll1_bypass (mux; 7x crtc rate) video_pll1_out (gate; 7x crtc rate) media_ldb (divider; 7x crtc rate) media_ldb_root_clk (gate; 7x crtc rate) media_disp2_pix (divider; 1x crtc rate) media_disp2_pix_root_clk (gate; 1x crtc rate) media_disp1_pix (divider; unused for now) media_disp1_pix_root_clk (gate; unused for now) The problem is that the panel driver sets media_disp1_pix_root_clk, ldb-bridge driver sets media_ldb_root_clk. All the others have a req_rate of the rate video_pll1 had when they got initialized or re-parented. My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate, IMO all clocks along the line (especially media_disp1_pix, which is "seen" as child of the PLL, and the actual divider for media_disp2_pix_root_clk) need to set their new rate as "required", because the subtree below them relies on it. This might be a wrong approach. It might be sufficient to have a req_rate only on the nodes that actually require it. However, IMHO we need to make sure that *all* required rates (especially the ones of leaves!) are respected after a change. Meaning if we e.g. request video_pll1 to change again (this time by media_ldb_root_clk), we have to ensure that media_disp2_pix_root_clk has still the rate which has been set as req_rate before. Ultimately, my trigger patch is also just a really bad workaround for a new_rate != req_rate check, so I want to re-build the idea behind it based on a differently defined req_rate. Need to take a deeper look on that. Thanks & regards Benjamin [1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L3891 [2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2726 [3] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2812 [4] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2592 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel