Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 7/9] clk: mediatek: Fix asymmetrical PLL enable and
 disable control
To:     Weiyi Lu <weiyi.lu@mediatek.com>, Rob Herring <robh@kernel.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Nicolas Boichat <drinkcat@chromium.org>
Cc:     James Liao <jamesjj.liao@mediatek.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org,
        srv_heupstream@mediatek.com, Wendell Lin <wendell.lin@mediatek.com>
References: <1599103380-4155-1-git-send-email-weiyi.lu@mediatek.com>
 <1599103380-4155-8-git-send-email-weiyi.lu@mediatek.com>
From:   Matthias Brugger <matthias.bgg@gmail.com>
Message-ID: <516a5f5b-f5af-0362-a73e-97b3db7300aa@gmail.com>
Date:   Thu, 1 Oct 2020 16:15:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <1599103380-4155-8-git-send-email-weiyi.lu@mediatek.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 03/09/2020 05:22, Weiyi Lu wrote:
> The en_mask actually 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().
> Now, setting the enable register(CON0) in 2 steps: first divider mask,
> then bit0 during prepare(), vice versa.
> Hence, en_mask will only be used as divider enable mask.
> Meanwhile, all the SoC PLL data are updated.
> 
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
>   drivers/clk/mediatek/clk-mt2701.c | 26 +++++++++++++-------------
>   drivers/clk/mediatek/clk-mt2712.c | 30 +++++++++++++++---------------
>   drivers/clk/mediatek/clk-mt6765.c | 20 ++++++++++----------
>   drivers/clk/mediatek/clk-mt6779.c | 24 ++++++++++++------------
>   drivers/clk/mediatek/clk-mt6797.c | 20 ++++++++++----------
>   drivers/clk/mediatek/clk-mt7622.c | 18 +++++++++---------
>   drivers/clk/mediatek/clk-mt7629.c | 12 ++++++------
>   drivers/clk/mediatek/clk-mt8173.c | 28 ++++++++++++++--------------
>   drivers/clk/mediatek/clk-mt8183.c | 22 +++++++++++-----------
>   drivers/clk/mediatek/clk-pll.c    | 16 ++++++++++++----
>   10 files changed, 112 insertions(+), 104 deletions(-)
> 
[...]
>   
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index f440f2cd..e0b00bc 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -247,10 +247,14 @@ 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);
>   
> +	if (pll->data->en_mask) {
> +		r = readl(pll->base_addr + REG_CON0) | pll->data->en_mask;
> +		writel(r, pll->base_addr + REG_CON0);
> +	}
> +

I think a better approach here would be to add a flag to mtk_pll_data instead of 
changing all drivers in one big patch. This will allow you to add the driver 
that needs to write the en_mask after writing CON0_BASE_EN more easily. And it 
will later allow you to change the remaining driver one by one until all are 
using the new flag.

Regards,
Matthias