From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755102AbcANC0n (ORCPT ); Wed, 13 Jan 2016 21:26:43 -0500 Received: from mail-pa0-f45.google.com ([209.85.220.45]:32781 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751095AbcANC0l convert rfc822-to-8bit (ORCPT ); Wed, 13 Jan 2016 21:26:41 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: James Liao , "Matthias Brugger" , "Stephen Boyd" From: Michael Turquette In-Reply-To: <1452482936-51852-1-git-send-email-jamesjj.liao@mediatek.com> Cc: "Sascha Hauer" , "Daniel Kurtz" , srv_heupstream@mediatek.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org, "James Liao" References: <1452482936-51852-1-git-send-email-jamesjj.liao@mediatek.com> Message-ID: <20160114022637.1942.4564@quark.deferred.io> User-Agent: alot/0.3.6 Subject: Re: [PATCH v2] clk: mediatek: Allow changing PLL rate when it is off Date: Wed, 13 Jan 2016 18:26:37 -0800 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi James, Quoting James Liao (2016-01-10 19:28:56) > 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. > > 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. Must the clock signal be enabled to change the PLL rate? If so, does ARMPLL set the CLK_SET_RATE_GATE flag? > > 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 unconditionally enables the PLL whenever clk_set_rate is called, changing the previous behavior. The clk framework still tracks the intent of ARMPLLs users with the enable_count. How about something like the following: bool pll_en = clk_hw_is_enabled(&pll->hw); if (pll_en) con1 |= CON0_PCW_CHG; writel(con1, pll->base_addr + REG_CON1); if (pll->tuner_addr) writel(con1 + 1, pll->tuner_addr); if (pll_en) udelay(20); This does not rely on the hardware to tell us the intent of the user, but instead on our framework usecounting. Regards, Mike > writel(con1, pll->base_addr + REG_CON1); > if (pll->tuner_addr) > writel(con1 + 1, pll->tuner_addr); > > - if (pll_en) > - udelay(20); > + udelay(20); > } > > /* > -- > 1.9.1 >