From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754590AbcAHLVx (ORCPT ); Fri, 8 Jan 2016 06:21:53 -0500 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:57429 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751780AbcAHLVv (ORCPT ); Fri, 8 Jan 2016 06:21:51 -0500 Date: Fri, 8 Jan 2016 12:21:42 +0100 From: Sascha Hauer To: James Liao Cc: Matthias Brugger , Mike Turquette , Stephen Boyd , srv_heupstream@mediatek.com, linux-kernel@vger.kernel.org, Daniel Kurtz , linux-mediatek@lists.infradead.org, Sascha Hauer , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] clk: mediatek: Allow changing PLL rate when it is off Message-ID: <20160108112142.GF13058@pengutronix.de> References: <1452240997-35316-1-git-send-email-jamesjj.liao@mediatek.com> <20160108091510.GC13058@pengutronix.de> <1452246533.31362.10.camel@mtksdaap41> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452246533.31362.10.camel@mtksdaap41> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 12:20:44 up 82 days, 19:59, 159 users, load average: 0.22, 0.12, 0.14 User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 08, 2016 at 05:48:53PM +0800, James Liao wrote: > Hi Sascha, > > On Fri, 2016-01-08 at 10:15 +0100, Sascha Hauer wrote: > > On Fri, Jan 08, 2016 at 04:16:37PM +0800, James Liao wrote: > > > Some modules may need to change its clock rate before turn on it. > > > So changing PLL's rate when it is off should be allowed. > > > This patch removes PLL enabled check before set rate, so that > > > PLLs can set new frequency even if they are off. > > > > This sounds like the software refused to change the rate on disabled > > PLLs, but this is not the case. > > In fact the major change of this patch is trigger (set) CON0_PCW_CHG no > matter PLL is on or not. > > > > > > > Signed-off-by: James Liao > > > --- > > > drivers/clk/mediatek/clk-pll.c | 9 ++------- > > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c > > > index 966cab1..8e31fae 100644 > > > --- a/drivers/clk/mediatek/clk-pll.c > > > +++ b/drivers/clk/mediatek/clk-pll.c > > > @@ -91,9 +91,6 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > > > int postdiv) > > > { > > > u32 con1, val; > > > - int pll_en; > > > - > > > - pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN; > > > > > > /* set postdiv */ > > > val = readl(pll->pd_addr); > > > @@ -114,15 +111,13 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw, > > > > > > con1 = readl(pll->base_addr + REG_CON1); > > > > > > - if (pll_en) > > > - con1 |= CON0_PCW_CHG; > > > + con1 |= CON0_PCW_CHG; > > > > This bit is described as "Feedback divide ratio update". To me this > > sounds like we have to inform the hardware that the PLL registers have > > been updated. The current code only sets this bit when the PLL is > > enabled which sounds sane to me. > > > > > > > > writel(con1, pll->base_addr + REG_CON1); > > > if (pll->tuner_addr) > > > writel(con1 + 1, pll->tuner_addr); > > > > > > - if (pll_en) > > > - udelay(20); > > > + udelay(20); > > > > We seem to have to wait here until the PLL is really running at the new > > frequency. Normally we don't have to do this when the PLL is disabled. > > > > I'm sure this patch solves a real problem, from looking at it it's just > > not clear to me what the problem is. Could you clarify this a bit? > > On MT8173 for example, ARMPLL's enable bit can be controlled by other > HW. That means ARMPLL may be turned on even if we (CPU / SW) set > ARMPLL's enable bit as 0. In this case, SW may want and can still change > ARMPLL's rate by changing its pcw and postdiv settings. But without this > patch, new pcw setting will not be applied because its enable bit is 0. Ok, thanks for explaining. Could you add that to the commit message? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |