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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8CFCFC25B08 for ; Sat, 20 Aug 2022 11:32:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344637AbiHTLcG (ORCPT ); Sat, 20 Aug 2022 07:32:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46164 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231152AbiHTLcF (ORCPT ); Sat, 20 Aug 2022 07:32:05 -0400 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C3E4A7F274 for ; Sat, 20 Aug 2022 04:32:04 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sin.source.kernel.org (Postfix) with ESMTPS id 22C32CE0945 for ; Sat, 20 Aug 2022 11:32:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 93B25C433D6; Sat, 20 Aug 2022 11:31:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1660995121; bh=x92xyb/sVL3yQBwE0vJmCTZp2Quayp28o9eTGtsJK2s=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=FYzaAgMh36cJRHQPeKTYv295Q04JBUJz0rGQSeo8HUafGM+FWjQe5JjWfx8Y2ufWw eXDI2xWuoIYya64wpmUknLB1wpkal+YEljiJ3x2CJ8ijWGy63FYkxRizX9InJloCRk x4dQnjVfR9LdutFVcYAcWXYv9Gh2dgw8HU6Lt4lZlBWfu1ry99DfqjmQMqBt7tP+45 2LzOYzyvEjBVU4WLLXkMy7G5ZEm7pY+7vBFfEtWhuyIuaFRfIPiHQBAF+IdWqq2rVV zDt0Uhupz2MnCPaOMc6wLnjigscuOvnSJcYxALY79wGCK5bwXWhUnwNYKFkO2w4ua3 tqYLp9iS2a6HQ== Date: Sat, 20 Aug 2022 12:42:35 +0100 From: Jonathan Cameron To: Lars-Peter Clausen Cc: Andy Shevchenko , Uwe =?UTF-8?B?S2xlaW5l?= =?UTF-8?B?LUvDtm5pZw==?= , Heiko Stuebner , Michael Hennerich , linux-iio , David Wu , Anand Ashok Dumbre , Vladimir Zapolskiy , Paul Cercueil , Antoniu Miclaus , Sascha Hauer , Jonathan Cameron , Nuno Sa , Michal Simek , Simon Xue Subject: Re: [PATCH 06/13] iio: adc: rockchip_saradc: Benefit from devm_clk_get_enabled() to simplify Message-ID: <20220820124235.223d4a7e@jic23-huawei> In-Reply-To: References: <20220808204740.307667-1-u.kleine-koenig@pengutronix.de> <20220808204740.307667-6-u.kleine-koenig@pengutronix.de> <20220813173142.76774c97@jic23-huawei> <20220814213058.dgoxpkoxpn6s4ojj@pengutronix.de> <20220815074149.jrkeevc3uxoo6ueb@pengutronix.de> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Tue, 16 Aug 2022 10:38:41 +0200 Lars-Peter Clausen wrote: > On 8/16/22 10:27, Lars-Peter Clausen wrote: > > On 8/16/22 10:20, Andy Shevchenko wrote: =20 > >> On Mon, Aug 15, 2022 at 10:42 AM Uwe Kleine-K=C3=B6nig > >> wrote: =20 > >>> On Mon, Aug 15, 2022 at 10:30:45AM +0300, Andy Shevchenko wrote: =20 > >>>> On Mon, Aug 15, 2022 at 12:31 AM Uwe Kleine-K=C3=B6nig > >>>> wrote: =20 > >>>>> On Sun, Aug 14, 2022 at 10:01:08PM +0300, Andy Shevchenko wrote: =20 > >>>>>> On Sat, Aug 13, 2022 at 7:21 PM Jonathan Cameron=20 > >>>>>> wrote: =20 > >>>>>>> On Mon,=C2=A0 8 Aug 2022 22:47:33 +0200 > >>>>>>> Uwe Kleine-K=C3=B6nig wrote: > >>>>>>> =20 > >>>>>>>> Make use of devm_clk_get_enabled() to replace some code that=20 > >>>>>>>> effectively > >>>>>>>> open codes this new function. > >>>>>>>> > >>>>>>>> Reviewed-by: Andy Shevchenko > >>>>>>>> Signed-off-by: Uwe Kleine-K=C3=B6nig =20 > >>>>>>> This might have side effects as it now enables the clock before=20 > >>>>>>> calling > >>>>>>> the clk_set_rate(). Also changes the clock start up ordering.=20 > >>>>>>> Neither is that > >>>>>>> scary a change, but on really fussy hardware they might cause=20 > >>>>>>> problems. > >>>>>>> > >>>>>>> Add a few rock-chips people who have sent patches in last few yea= rs > >>>>>>> to hopefully take a look or even better run a test. =20 > >>>>>> I believe you found a bug in the patch. The possible solutions are: > >>>>>> - not take the patch > >>>>>> - disable and re-enable clock around clk_set_rate() > >>>>>> > >>>>>> IIRC clk_set_rate() will spit a WARN if clock is enabled. =20 > >>>>> You mean in general? I think that's wrong. There might be some=20 > >>>>> clks that > >>>>> do that, but I'd consider them strange. If you ask me, calling > >>>>> clk_set_rate() for a *disabled* clk is the strange concept ... =20 > >>>> I think it's correct from the logic and electrical perspective. That= 's > >>>> why the preparation and enablement are separated in CCF. But please > >>>> double check, because I can't remember everything by heart. =20 > >>> In my book the separation is done because "enabling" has to sleep for > >>> some clks (e.g. PLLs) while a sleeping clk_enable() is bad for various > >>> use cases and most clks don't sleep for enabling. =20 > >> Yeah, but the idea of changing clock rate on the fly may produce > >> interesting side-effects on hardware level (with PLL latencies to lock > >> the phase and possible glitches). So, changing clock against enabled > >> hardware (not in reset / power off state) seems not a good idea. =20 > > > > The clk_set_rate() API will internally disable the clock, if the clock= =20 > > chip requires it. See `CLK_SET_RATE_GATE` flag. =20 > Sorry, I misremembered. If `CLK_SET_RATE_GATE` is set `set_rate` will=20 > return an error if the clock is enabled when `set_rate` is called. Thanks for chasing that down. In that case I definitely don't want to take this without input from those who can test it! Jonathan > > > > But I tend to agree, the better idiom is to `set_rate` we should do=20 > > that before `enable`. This will avoid any unintentional glitches on=20 > > the clock signal > > =20 >=20