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 alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (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 81FF4C433FE for ; Sat, 5 Nov 2022 07:09:43 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 8CAD016A4; Sat, 5 Nov 2022 08:08:51 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 8CAD016A4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1667632181; bh=nbLTtZHdfpv1b/Nl7/dH3yrfjrIomM+d2Luw0dbCmCQ=; h=Date:From:Subject:To:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Zw7/cPlXLh3LZT16YoFYPcs3aZO0TM8JQvWfiSyjNTX9n8Wzf/1rqadyKSf/IB2M5 lygZRwp3iiHcm3tVSSOrxjCbRruCKa9pQTpEHrQwf/ew/HNeiUGJeAM0N2cOeARlrO U6/4S6rzTP+OOgPTt1dBKnOHr7Ml26stkSWlZqo8= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id C47B8F805E0; Sat, 5 Nov 2022 08:03:48 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 2625AF80448; Fri, 4 Nov 2022 15:31:58 +0100 (CET) Received: from aposti.net (aposti.net [89.234.176.197]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 2091DF801D5 for ; Fri, 4 Nov 2022 15:31:50 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 2091DF801D5 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=crapouillou.net header.i=@crapouillou.net header.b="CpbNQdJ/" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=crapouillou.net; s=mail; t=1667572309; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yJcwJHHN5pttniHz+NbbkUJe8GLWXEpymY5u4z+2Tls=; b=CpbNQdJ/BWVPt6aAZUIABOUDvKsIEPL9U91j803wkEpi9xnD/0w4adiBYeJcN8TJFfSWVl x/lvi9fmHRkBC7J3u5FRclEUbJUWbDpV47bpFyqI8WWI72nrAKvHhlIMavR+GX46Lp5ZKT 4vVIrBF8TIdJDdEsSYvgVdovc3N1IRg= Date: Fri, 04 Nov 2022 14:31:20 +0000 From: Paul Cercueil Subject: Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate To: Maxime Ripard Message-Id: <80VTKR.CE8RVN8M3ZYK3@crapouillou.net> In-Reply-To: <20221018-clk-range-checks-fixes-v2-56-f6736dec138e@cerno.tech> References: <20221018-clk-range-checks-fixes-v2-0-f6736dec138e@cerno.tech> <20221018-clk-range-checks-fixes-v2-56-f6736dec138e@cerno.tech> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Sat, 05 Nov 2022 08:03:30 +0100 Cc: Ulf Hansson , Prashant Gaikwad , Alexandre Belloni , Liam Girdwood , Michael Turquette , Sekhar Nori , Alexandre Torgue , dri-devel@lists.freedesktop.org, Max Filippov , Thierry Reding , linux-phy@lists.infradead.org, David Airlie , Fabio Estevam , linux-stm32@st-md-mailman.stormreply.com, Abel Vesa , Kishon Vijay Abraham I , Geert Uytterhoeven , Samuel Holland , Chunyan Zhang , Takashi Iwai , linux-tegra@vger.kernel.org, Jernej Skrabec , Jonathan Hunter , Chen-Yu Tsai , NXP Linux Team , Orson Zhai , linux-mips@vger.kernel.org, Luca Ceresoli , Linus Walleij , linux-rtc@vger.kernel.org, linux-clk@vger.kernel.org, Charles Keepax , Daniel Vetter , alsa-devel@alsa-project.org, Manivannan Sadhasivam , linux-kernel@vger.kernel.org, Sascha Hauer , linux-actions@lists.infradead.org, Richard Fitzgerald , Mark Brown , linux-mediatek@lists.infradead.org, Baolin Wang , Matthias Brugger , Pengutronix Kernel Team , linux-arm-kernel@lists.infradead.org, AngeloGioacchino Del Regno , Alessandro Zummo , linux-sunxi@lists.linux.dev, Stephen Boyd , patches@opensource.cirrus.com, Peter De Schrijver , Nicolas Ferre , Andreas =?iso-8859-1?q?F=E4rber?= , linux-renesas-soc@vger.kernel.org, Dinh Nguyen , Vinod Koul , Maxime Coquelin , David Lechner , Shawn Guo , Claudiu Beznea X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" Hi Maxime, Le ven. 4 nov. 2022 =E0 14:18:13 +0100, Maxime Ripard=20 a =E9crit : > The Ingenic CGU clocks implements a mux with a set_parent hook, but > doesn't provide a determine_rate implementation. >=20 > This is a bit odd, since set_parent() is there to, as its name=20 > implies, > change the parent of a clock. However, the most likely candidate to > trigger that parent change is a call to clk_set_rate(), with > determine_rate() figuring out which parent is the best suited for a > given rate. >=20 > The other trigger would be a call to clk_set_parent(), but it's far=20 > less > used, and it doesn't look like there's any obvious user for that=20 > clock. >=20 > So, the set_parent hook is effectively unused, possibly because of an > oversight. However, it could also be an explicit decision by the > original author to avoid any reparenting but through an explicit call=20 > to > clk_set_parent(). >=20 > The driver does implement round_rate() though, which means that we can > change the rate of the clock, but we will never get to change the > parent. >=20 > However, It's hard to tell whether it's been done on purpose or not. >=20 > Since we'll start mandating a determine_rate() implementation, let's > convert the round_rate() implementation to a determine_rate(), which > will also make the current behavior explicit. And if it was an > oversight, the clock behaviour can be adjusted later on. So it's partly on purpose, partly because I didn't know about=20 .determine_rate. There's nothing odd about having a lonely .set_parent callback; in my=20 case the clocks are parented from the device tree. Having the clocks driver trigger a parent change when requesting a rate=20 change sounds very dangerous, IMHO. My MMC controller can be parented=20 to the external 48 MHz oscillator, and if the card requests 50 MHz, it=20 could switch to one of the PLLs. That works as long as the PLLs don't=20 change rate, but if one is configured as driving the CPU clock, it=20 becomes messy. The thing is, the clocks driver has no way to know whether or not it is=20 "safe" to use a designated parent. For that reason, in practice, I never actually want to have a clock=20 re-parented - it's almost always a bad idea vs. sticking to the parent=20 clock configured in the DTS. > Signed-off-by: Maxime Ripard > --- > drivers/clk/ingenic/cgu.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) >=20 > diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c > index 1f7ba30f5a1b..0c9c8344ad11 100644 > --- a/drivers/clk/ingenic/cgu.c > +++ b/drivers/clk/ingenic/cgu.c > @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw, > return div; > } >=20 > -static long > -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate, > - unsigned long *parent_rate) > +static int ingenic_clk_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > { > struct ingenic_clk *ingenic_clk =3D to_ingenic_clk(hw); > const struct ingenic_cgu_clk_info *clk_info =3D=20 > to_clk_info(ingenic_clk); > unsigned int div =3D 1; >=20 > if (clk_info->type & CGU_CLK_DIV) > - div =3D ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate); > + div =3D ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate, > + req->rate); Sorry but I'm not sure that this works. You replace the "parent_rate" with the "best_parent_rate", and that=20 means you only check the requested rate vs. the parent with the highest=20 frequency, and not vs. the actual parent that will be used. Cheers, -Paul > else if (clk_info->type & CGU_CLK_FIXDIV) > div =3D clk_info->fixdiv.div; > else if (clk_hw_can_set_rate_parent(hw)) > - *parent_rate =3D req_rate; > + req->best_parent_rate =3D req->rate; >=20 > - return DIV_ROUND_UP(*parent_rate, div); > + req->rate =3D DIV_ROUND_UP(req->best_parent_rate, div); > + return 0; > } >=20 > static inline int ingenic_clk_check_stable(struct ingenic_cgu *cgu, > @@ -626,7 +627,7 @@ static const struct clk_ops ingenic_clk_ops =3D { > .set_parent =3D ingenic_clk_set_parent, >=20 > .recalc_rate =3D ingenic_clk_recalc_rate, > - .round_rate =3D ingenic_clk_round_rate, > + .determine_rate =3D ingenic_clk_determine_rate, > .set_rate =3D ingenic_clk_set_rate, >=20 > .enable =3D ingenic_clk_enable, >=20 > -- > b4 0.11.0-dev-99e3a