Re: [PATCH v5 07/24] clk: mediatek: Fix asymmetrical PLL enable and disable control

From: Ikjoon Jang <ikjn@chromium.org>
To: Weiyi Lu <weiyi.lu@mediatek.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	Rob Herring <robh@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
	Nicolas Boichat <drinkcat@chromium.org>,
	srv_heupstream@mediatek.com, linux-kernel@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	Yingjoe Chen <yingjoe.chen@mediatek.com>,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 07/24] clk: mediatek: Fix asymmetrical PLL enable and disable control
Date: Wed, 18 Nov 2020 11:55:02 +0800	[thread overview]
Message-ID: <20201118035502.GB1049148@chromium.org> (raw)
In-Reply-To: <1604887429-29445-8-git-send-email-weiyi.lu@mediatek.com>

On Mon, Nov 09, 2020 at 10:03:32AM +0800, Weiyi Lu wrote:
> In fact, the en_mask is a combination of divider enable mask
> and pll enable bit(bit0).
> Before this patch, we enabled both divider mask and bit0 in prepare(),
> but only cleared the bit0 in unprepare().
> In the future, we hope en_mask will only be used as divider enable mask.
> The enable register(CON0) will be set in 2 steps:
> first is divider mask, and then bit0 during prepare(), and vice versa.
> But considering backward compatibility, at this stage we allow en_mask
> to be a combination or a pure divider enable mask.
> And then we will make en_mask a pure divider enable mask in another
> following patch series.

I have a question on this: Are there any possible problems on controlling
divider_en and bit0 at the same time? Or is this only for cleanups?

If mtk_pll_data::en_mask is not allowed to control with bit0 together,
I guess register_pll() also needs to check en_mask::bit0 is cleared?

> 
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-pll.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index f440f2cd..11ed5d1 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -238,6 +238,7 @@ static int mtk_pll_prepare(struct clk_hw *hw)
>  {
>  	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
>  	u32 r;
> +	u32 div_en_mask;
>  
>  	r = readl(pll->pwr_addr) | CON0_PWR_ON;
>  	writel(r, pll->pwr_addr);
> @@ -247,10 +248,15 @@ static int mtk_pll_prepare(struct clk_hw *hw)
>  	writel(r, pll->pwr_addr);
>  	udelay(1);
>  
> -	r = readl(pll->base_addr + REG_CON0);
> -	r |= pll->data->en_mask;
> +	r = readl(pll->base_addr + REG_CON0) | CON0_BASE_EN;
>  	writel(r, pll->base_addr + REG_CON0);
>  
> +	div_en_mask = pll->data->en_mask & ~CON0_BASE_EN;
> +	if (div_en_mask) {
> +		r = readl(pll->base_addr + REG_CON0) | div_en_mask;
> +		writel(r, pll->base_addr + REG_CON0);
> +	}
> +
>  	__mtk_pll_tuner_enable(pll);
>  
>  	udelay(20);
> @@ -268,6 +274,7 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
>  {
>  	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
>  	u32 r;
> +	u32 div_en_mask;
>  
>  	if (pll->data->flags & HAVE_RST_BAR) {
>  		r = readl(pll->base_addr + REG_CON0);
> @@ -277,8 +284,13 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
>  
>  	__mtk_pll_tuner_disable(pll);
>  
> -	r = readl(pll->base_addr + REG_CON0);
> -	r &= ~CON0_BASE_EN;
> +	div_en_mask = pll->data->en_mask & ~CON0_BASE_EN;
> +	if (div_en_mask) {
> +		r = readl(pll->base_addr + REG_CON0) & ~div_en_mask;
> +		writel(r, pll->base_addr + REG_CON0);
> +	}
> +
> +	r = readl(pll->base_addr + REG_CON0) & ~CON0_BASE_EN;
>  	writel(r, pll->base_addr + REG_CON0);
>  
>  	r = readl(pll->pwr_addr) | CON0_ISO_EN;
> -- 
> 1.8.1.1.dirty
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel