Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 8 Feb 2022 22:32:52 +0300
From:   Boris Lysov <arz65xx@gmail.com>
To:     Chen-Yu Tsai <wenst@chromium.org>
Cc:     Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Chun-Jie Chen <chun-jie.chen@mediatek.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Miles Chen <miles.chen@mediatek.com>,
        linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 00/31] clk: mediatek: Cleanups and Improvements -
 Part 1
Message-ID: <20220208223252.7f179c63@pc>
In-Reply-To: <20220208124034.414635-1-wenst@chromium.org>
References: <20220208124034.414635-1-wenst@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi, I couldn't find a particular patch to reply to so I'm replying cover
letter to give some input on the PLL subsystem.

On Tue,  8 Feb 2022 20:40:03 +0800
Chen-Yu Tsai <wenst@chromium.org> wrote:
>  drivers/clk/mediatek/clk-pll.c                | 100 +++++-
>  drivers/clk/mediatek/clk-pll.h                |  57 ++++

In clk-pll.c there is an mtk_clk_register_pll function which at some point
executes this:

> init.ops = &mtk_pll_ops;

In my opinion there should be a possibility to define a custom mtk_pll_ops for a
given SoC instead of using a hardcoded one because not all Mediatek SoCs share
the same PLL startup/powerdown flow. For example, the existing mtk_pll_prepare
implementation won't work for the entire Mediatek Cortex-A9 SoC family (this
includes but not limited to mt6515, mt6517, mt6575, and mt6577). 

> 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);

This code sets a bit to 1 to start a PLL but the SoCs I mentioned above would
need to have that bit cleared (set to 0) [1] [2].

Another interesting thing in mtk_pll_prepare is
> 	udelay(20);
Is 20 ms a settle time for PLL? If yes then it would also be cool to specify an
arbitrary value easily as some PLLs have longer settle time [3] [4].

Worth noting the SoCs I mentioned aren't in mainline yet, and I think there are
more modern mainline-worthy Mediatek SoCs that might also need these changes in
the future.

Thanks.

[1] MT6577 HSPA Smartphone Application Processor Datasheet, pages 1212-1227
(*_CON0 registers).
[2] MT6515 GSM/EDGE Smartphone Application Processor Datasheet, pages
1202-1216 (*_CON0 registers).
[3] pages 1303-1306 of [1]
[4] MT6589 HSPA+ Smartphone Application Processor Datasheet, page 1344
(MDPLL1 & MDPLL2)