On Wed, Jul 29, 2020 at 6:51 PM Nicolas Boichat [off-list ref] wrote:

On Wed, Jul 29, 2020 at 4:44 PM Weiyi Lu [off-list ref] wrote:

quoted

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.

I like this a lot better, most changes look fine, just a few nits.

quoted

Signed-off-by: Weiyi Lu <redacted>
---
 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 | 42 ++++++++++++++++++++++++++-------------
 drivers/clk/mediatek/clk-mt8183.c | 22 ++++++++++----------
 drivers/clk/mediatek/clk-pll.c    | 10 ++++++++--
 10 files changed, 122 insertions(+), 102 deletions(-)

[snip]

quoted

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index f440f2cd..3c79e1a 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c

@@ -247,8 +247,10 @@ 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);
+
+       r = readl(pll->base_addr + REG_CON0) | pll->data->en_mask;

One more question. I have the feeling that CON0_BASE_EN is what
enables the clock for good (and pll->data->en_mask is just an
additional setting/mask, since you could disable the clock by simply
clearing CON0_BASE_EN). Shouldn't you set pll->data->en_mask _first_,
then CON0_BASE_EN?

quoted

        writel(r, pll->base_addr + REG_CON0);

As a small optimization, you can do:

if (pll->data->en_mask) {
   r = readl(pll->base_addr + REG_CON0) | pll->data->en_mask;
   writel(r, pll->base_addr + REG_CON0);
}

quoted

        __mtk_pll_tuner_enable(pll);

@@ -278,6 +280,10 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
        __mtk_pll_tuner_disable(pll);

        r = readl(pll->base_addr + REG_CON0);
+       r &= ~pll->data->en_mask;

Move this to one line? (so that the code looks symmetrical, too?)

quoted

+       writel(r, pll->base_addr + REG_CON0);
+
+       r = readl(pll->base_addr + REG_CON0);
        r &= ~CON0_BASE_EN;

And ditto, ~CON0_BASE_EN then ~pll->data->en_mask?

ditto?

quoted

        writel(r, pll->base_addr + REG_CON0);

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