From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout4.samsung.com ([203.254.224.34]:35196 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1168560AbdDXLfV (ORCPT ); Mon, 24 Apr 2017 07:35:21 -0400 Subject: Re: [PATCH RFC 1/7] clk: samsung: Add enable/disable operation for PLL36XX clocks To: Krzysztof Kozlowski Cc: linux-samsung-soc@vger.kernel.org, jy0922.shim@samsung.com, Javier Martinez Canillas , =?UTF-8?Q?Bart=c5=82omiej_=c5=bbo=c5=82nierkiewicz?= , Seung Woo Kim , Inki Dae , Chanwoo Choi , linux-clk@vger.kernel.org From: Sylwester Nawrocki Message-id: <3232b6e5-05b6-2d91-07ef-e5473ee70889@samsung.com> Date: Mon, 24 Apr 2017 13:35:13 +0200 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=windows-1252; format=flowed References: <1492795191-31298-1-git-send-email-s.nawrocki@samsung.com> <1492795191-31298-2-git-send-email-s.nawrocki@samsung.com> <20170422152236.tbf4iuoadqrvoc2n@kozik-lap> <017b3ec8-c185-c29a-2944-12f519b461fe@samsung.com> Sender: linux-clk-owner@vger.kernel.org List-ID: (dropping unrelated addresses from Cc list) On 04/24/2017 01:18 PM, Krzysztof Kozlowski wrote: > On Mon, Apr 24, 2017 at 1:12 PM, Sylwester Nawrocki > wrote: >> On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote: >>>> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw >>>> *hw, unsigned long drate, >>>> writel_relaxed(pll_con1, pll->con_reg + 4); >>>> >>>> /* wait_lock_time */ >>>> - do { >>>> - cpu_relax(); >>>> - tmp = readl_relaxed(pll->con_reg); >>>> - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); >> >> >> I will add a comment here like: >> /* Wait until the PLL is locked if it is enabled. */ >> >>>> + if (pll_con0 & BIT(pll->enable_offs)) { >>> >>> >>> Why this additional if() is needed? >> >> >> The PLL will never transition to a locked state if it is disabled, without >> this test we would end up with an indefinite loop below. > > Hmmm... shouldn't this be split from this change? Or maybe this is > needed only because you added enable/disable for pl36xx and previously > this was not existing? Yes, it looks like previously the PLLs were always on after were enabled. I wouldn't be separating that part, as it's all logically dependent. Just the commit message might need some improvement. -- Thanks, Sylwester