MIME-Version: 1.0
In-Reply-To: <1423478845-2835-7-git-send-email-s.hauer@pengutronix.de>
References: <1423478845-2835-1-git-send-email-s.hauer@pengutronix.de> <1423478845-2835-7-git-send-email-s.hauer@pengutronix.de>
From: Tomasz Figa <tfiga@google.com>
Date: Fri, 13 Feb 2015 18:56:53 +0900
Message-ID: <CAAFQd5DBYU-_hvMKe86mnXau8Of5KhM1dusg9W+FmhiFuAGdpw@mail.gmail.com>
Subject: Re: [PATCH 06/13] clk: mediatek: Add basic clocks for Mediatek MT8173.
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
        James Liao <jamesjj.liao@mediatek.com>,
        Mike Turquette <mturquette@linaro.org>,
        =?UTF-8?B?WUggQ2hlbiAo6Zmz5pix6LGqKQ==?= <yh.chen@mediatek.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Henry Chen <henryc.chen@mediatek.com>,
        Rob Herring <robh+dt@kernel.org>, kernel@pengutronix.de,
        =?UTF-8?B?WWluZ2pvZSBDaGVuICjpmbPoi7HmtLIp?= 
	<Yingjoe.Chen@mediatek.com>,
        Eddie Huang <eddie.huang@mediatek.com>,
        Lee Jones <lee.jones@linaro.org>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 30777
Lines: 1009

Please find my comments inline.

On Mon, Feb 9, 2015 at 7:47 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> From: James Liao <jamesjj.liao@mediatek.com>
>
> This patch adds basic clocks for MT8173, including TOPCKGEN, PLLs,
> INFRA and PERI clocks.
>
> Signed-off-by: James Liao <jamesjj.liao@mediatek.com>
> Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/clk/mediatek/Makefile         |    1 +
>  drivers/clk/mediatek/clk-mt8173-pll.c |  807 +++++++++++++++++++++++++
>  drivers/clk/mediatek/clk-mt8173-pll.h |   14 +
>  drivers/clk/mediatek/clk-mt8173.c     | 1035 +++++++++++++++++++++++++++++++++
>  4 files changed, 1857 insertions(+)
>  create mode 100644 drivers/clk/mediatek/clk-mt8173-pll.c
>  create mode 100644 drivers/clk/mediatek/clk-mt8173-pll.h
>  create mode 100644 drivers/clk/mediatek/clk-mt8173.c
>
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index afb52e5..e030416 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -1,3 +1,4 @@
>  obj-y += clk-mtk.o clk-pll.o clk-gate.o
>  obj-$(CONFIG_RESET_CONTROLLER) += reset.o
>  obj-y += clk-mt8135.o clk-mt8135-pll.o
> +obj-y += clk-mt8173.o clk-mt8173-pll.o
> diff --git a/drivers/clk/mediatek/clk-mt8173-pll.c b/drivers/clk/mediatek/clk-mt8173-pll.c
> new file mode 100644
> index 0000000..9f6f821
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8173-pll.c
> @@ -0,0 +1,807 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <jamesjj.liao@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/clkdev.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-pll.h"
> +#include "clk-mt8173-pll.h"
> +
> +#define PLL_BASE_EN    BIT(0)
> +#define PLL_PWR_ON     BIT(0)
> +#define PLL_ISO_EN     BIT(1)
> +#define PLL_PCW_CHG    BIT(31)
> +#define RST_BAR_MASK   BIT(24)
> +#define AUDPLL_TUNER_EN        BIT(31)
> +
> +static const u32 pll_posdiv_map[8] = { 1, 2, 4, 8, 16, 16, 16, 16 };

It might be nice to have a comment what this array is for and how the
values were calculated.

> +
> +static u32 mtk_calc_pll_vco_freq(
> +               u32 fin,
> +               u32 pcw,
> +               u32 vcodivsel,
> +               u32 prediv,
> +               u32 pcwfbits)
> +{
> +       /* vco = (fin * pcw * vcodivsel / prediv) >> pcwfbits; */
> +       u64 vco = fin;
> +       u8 c = 0;
> +
> +       vco = vco * pcw * vcodivsel;

Could you use here (u64)fin directly for increased readability and
drop the initialization of vco?

> +       do_div(vco, prediv);
> +
> +       if (vco & GENMASK(pcwfbits - 1, 0))
> +               c = 1;

What is c? Could the variable has a more meaningful name?

> +
> +       vco >>= pcwfbits;
> +
> +       if (c)
> +               ++vco;
> +
> +       return (u32)vco;
> +}
> +
> +static u32 mtk_freq_limit(u32 freq)
> +{
> +       static const u64 freq_max = 3000UL * 1000 * 1000;       /* 3000 MHz */

3 GHz probably? Could you define (if not defined somewhere already) a
macro for GHZ and write this as 3 * GHZ?

> +       static const u32 freq_min = 1000 * 1000 * 1000 / 16;    /* 62.5 MHz */

Why don't you write it as 62500 * KHZ or 62 * MHZ + 500 * KHZ?

> +
> +       if (freq <= freq_min)
> +               freq = freq_min + 16;

Could you explain what's happening here? Where does the 16 come from
and why it is not defined as a macro?

> +       else if (freq > freq_max)
> +               freq = freq_max;
> +
> +       return freq;
> +}
> +
> +static int mtk_calc_pll_freq_cfg(
> +               u32 *pcw,
> +               u32 *postdiv_idx,
> +               u32 freq,
> +               u32 fin,
> +               int pcwfbits)
> +{
> +       static const u64 freq_max = 3000UL * 1000 * 1000;       /* 3000 MHz */
> +       static const u64 freq_min = 1000 * 1000 * 1000;         /* 1000 MHz */
> +       static const u64 postdiv[] = { 1, 2, 4, 8, 16 };
> +       u64 n_info;
> +       u32 idx;
> +
> +       /* search suitable postdiv */
> +       for (idx = *postdiv_idx;
> +               idx < ARRAY_SIZE(postdiv) && postdiv[idx] * freq <= freq_min;
> +               idx++)
> +               ;

Please document the arguments of this function. It is not obvious why
the value at postdiv_idx is used as starting point, even though this
pointer is also used to store the output value...

> +
> +       if (idx >= ARRAY_SIZE(postdiv))
> +               return -EINVAL; /* freq is out of range (too low) */
> +       else if (postdiv[idx] * freq > freq_max)
> +               return -EINVAL; /* freq is out of range (too high) */
> +
> +       /* n_info = freq * postdiv / 26MHz * 2^pcwfbits */
> +       n_info = (postdiv[idx] * freq) << pcwfbits;
> +       do_div(n_info, fin);
> +
> +       *postdiv_idx = idx;
> +       *pcw = (u32)n_info;
> +
> +       return 0;
> +}
> +
> +static int mtk_clk_pll_is_enabled(struct clk_hw *hw)
> +{
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +       return (readl_relaxed(pll->base_addr) & PLL_BASE_EN) != 0;
> +}
> +
> +static int mtk_clk_pll_prepare(struct clk_hw *hw)

Hmm, contents of this function don't seem to sleep. Maybe this should
be enable instead of prepare?

> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       u32 r;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       r = readl_relaxed(pll->pwr_addr) | PLL_PWR_ON;
> +       writel_relaxed(r, pll->pwr_addr);
> +       wmb();  /* sync write before delay */
> +       udelay(1);
> +
> +       r = readl_relaxed(pll->pwr_addr) & ~PLL_ISO_EN;
> +       writel_relaxed(r, pll->pwr_addr);
> +       wmb();  /* sync write before delay */
> +       udelay(1);
> +
> +       r = readl_relaxed(pll->base_addr) | pll->en_mask;
> +       writel_relaxed(r, pll->base_addr);
> +       wmb();  /* sync write before delay */
> +       udelay(20);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(pll->base_addr) | RST_BAR_MASK;
> +               writel_relaxed(r, pll->base_addr);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void mtk_clk_pll_unprepare(struct clk_hw *hw)

Similarly to prepare, maybe you should consider to implement disable
instead of unprepare.

> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       u32 r;
> +
> +       if (pll->flags & PLL_AO)
> +               return;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(pll->base_addr) & ~RST_BAR_MASK;
> +               writel_relaxed(r, pll->base_addr);
> +       }
> +
> +       r = readl_relaxed(pll->base_addr) & ~PLL_BASE_EN;
> +       writel_relaxed(r, pll->base_addr);
> +
> +       r = readl_relaxed(pll->pwr_addr) | PLL_ISO_EN;
> +       writel_relaxed(r, pll->pwr_addr);
> +
> +       r = readl_relaxed(pll->pwr_addr) & ~PLL_PWR_ON;
> +       writel_relaxed(r, pll->pwr_addr);
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +static long mtk_clk_pll_round_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long *prate)
> +{
> +       u32 pcwfbits = 14;
> +       u32 pcw = 0;
> +       u32 postdiv = 0;
> +       u32 r;
> +
> +       *prate = *prate ? *prate : 26000000;
> +       rate = mtk_freq_limit(rate);
> +       mtk_calc_pll_freq_cfg(&pcw, &postdiv, rate, *prate, pcwfbits);
> +
> +       r = mtk_calc_pll_vco_freq(*prate, pcw, 1, 1, pcwfbits);
> +       r = (r + pll_posdiv_map[postdiv] - 1) / pll_posdiv_map[postdiv];
> +
> +       return r;
> +}
> +
> +#define SDM_PLL_POSTDIV_H      6
> +#define SDM_PLL_POSTDIV_L      4
> +#define SDM_PLL_POSTDIV_MASK   GENMASK(SDM_PLL_POSTDIV_H, SDM_PLL_POSTDIV_L)
> +#define SDM_PLL_PCW_H          20
> +#define SDM_PLL_PCW_L          0
> +#define SDM_PLL_PCW_MASK       GENMASK(SDM_PLL_PCW_H, SDM_PLL_PCW_L)
> +
> +static unsigned long mtk_clk_sdm_pll_recalc_rate(
> +               struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +       u32 con0 = readl_relaxed(pll->base_addr);
> +       u32 con1 = readl_relaxed(pll->base_addr + 4);
> +
> +       u32 posdiv = (con0 & SDM_PLL_POSTDIV_MASK) >> SDM_PLL_POSTDIV_L;
> +       u32 pcw = (con1 & SDM_PLL_PCW_MASK) >> SDM_PLL_PCW_L;
> +       u32 pcwfbits = 14;
> +
> +       u32 vco_freq;
> +       unsigned long r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +
> +       vco_freq = mtk_calc_pll_vco_freq(parent_rate, pcw, 1, 1, pcwfbits);
> +       r = (vco_freq + pll_posdiv_map[posdiv] - 1) / pll_posdiv_map[posdiv];
> +
> +       return r;
> +}
> +
> +static void mtk_clk_sdm_pll_set_rate_regs(
> +               struct clk_hw *hw,
> +               u32 pcw,
> +               u32 postdiv_idx)
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con1_addr = pll->base_addr + 4;
> +       u32 con0;
> +       u32 con1;
> +       u32 pll_en;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       con0 = readl_relaxed(con0_addr);
> +       con1 = readl_relaxed(con1_addr);
> +
> +       pll_en = con0 & PLL_BASE_EN;
> +
> +       /* set postdiv */
> +       con0 &= ~SDM_PLL_POSTDIV_MASK;
> +       con0 |= postdiv_idx << SDM_PLL_POSTDIV_L;
> +       writel_relaxed(con0, con0_addr);
> +
> +       /* set pcw */
> +       con1 &= ~SDM_PLL_PCW_MASK;
> +       con1 |= pcw << SDM_PLL_PCW_L;
> +
> +       if (pll_en)
> +               con1 |= PLL_PCW_CHG;
> +
> +       writel_relaxed(con1, con1_addr);
> +
> +       if (pll_en) {
> +               wmb();  /* sync write before delay */
> +               udelay(20);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +static int mtk_clk_sdm_pll_set_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       u32 pcwfbits = 14;
> +       u32 pcw = 0;
> +       u32 postdiv_idx = 0;
> +       int r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +       r = mtk_calc_pll_freq_cfg(&pcw, &postdiv_idx, rate,
> +                       parent_rate, pcwfbits);
> +
> +       if (r == 0)
> +               mtk_clk_sdm_pll_set_rate_regs(hw, pcw, postdiv_idx);
> +
> +       return r;
> +}
> +
> +const struct clk_ops mt8173_sdm_pll_ops = {
> +       .is_enabled     = mtk_clk_pll_is_enabled,
> +       .prepare        = mtk_clk_pll_prepare,
> +       .unprepare      = mtk_clk_pll_unprepare,
> +       .recalc_rate    = mtk_clk_sdm_pll_recalc_rate,
> +       .round_rate     = mtk_clk_pll_round_rate,
> +       .set_rate       = mtk_clk_sdm_pll_set_rate,
> +};
> +
> +#define ARM_PLL_POSTDIV_H      26
> +#define ARM_PLL_POSTDIV_L      24
> +#define ARM_PLL_POSTDIV_MASK   GENMASK(ARM_PLL_POSTDIV_H, ARM_PLL_POSTDIV_L)
> +#define ARM_PLL_PCW_H          20
> +#define ARM_PLL_PCW_L          0
> +#define ARM_PLL_PCW_MASK       GENMASK(ARM_PLL_PCW_H, ARM_PLL_PCW_L)
> +
> +static unsigned long mtk_clk_arm_pll_recalc_rate(
> +               struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +       u32 con1 = readl_relaxed(pll->base_addr + 4);
> +
> +       u32 posdiv = (con1 & ARM_PLL_POSTDIV_MASK) >> ARM_PLL_POSTDIV_L;
> +       u32 pcw = (con1 & ARM_PLL_PCW_MASK) >> ARM_PLL_PCW_L;
> +       u32 pcwfbits = 14;
> +
> +       u32 vco_freq;
> +       unsigned long r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +
> +       vco_freq = mtk_calc_pll_vco_freq(parent_rate, pcw, 1, 1, pcwfbits);
> +       r = (vco_freq + pll_posdiv_map[posdiv] - 1) / pll_posdiv_map[posdiv];
> +
> +       return r;
> +}
> +
> +static void mtk_clk_arm_pll_set_rate_regs(
> +               struct clk_hw *hw,
> +               u32 pcw,
> +               u32 postdiv_idx)
> +
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con1_addr = pll->base_addr + 4;
> +       u32 con0;
> +       u32 con1;
> +       u32 pll_en;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       con0 = readl_relaxed(con0_addr);
> +       con1 = readl_relaxed(con1_addr);
> +
> +       pll_en = con0 & PLL_BASE_EN;
> +
> +       /* postdiv */
> +       con1 &= ~ARM_PLL_POSTDIV_MASK;
> +       con1 |= postdiv_idx << ARM_PLL_POSTDIV_L;
> +
> +       /* pcw */
> +       con1 &= ~ARM_PLL_PCW_MASK;
> +       con1 |= pcw << ARM_PLL_PCW_L;
> +
> +       if (pll_en)
> +               con1 |= PLL_PCW_CHG;
> +
> +       writel_relaxed(con1, con1_addr);
> +
> +       if (pll_en) {
> +               wmb();  /* sync write before delay */
> +               udelay(20);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +static int mtk_clk_arm_pll_set_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       u32 pcwfbits = 14;
> +       u32 pcw = 0;
> +       u32 postdiv_idx = 0;
> +       int r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +       r = mtk_calc_pll_freq_cfg(&pcw, &postdiv_idx, rate,
> +                       parent_rate, pcwfbits);
> +
> +       if (r == 0)
> +               mtk_clk_arm_pll_set_rate_regs(hw, pcw, postdiv_idx);
> +
> +       return r;
> +}
> +
> +const struct clk_ops mt8173_arm_pll_ops = {
> +       .is_enabled     = mtk_clk_pll_is_enabled,
> +       .prepare        = mtk_clk_pll_prepare,
> +       .unprepare      = mtk_clk_pll_unprepare,

Uhh, this is incorrect. If you provide prepare+unprepare, you also
need to provide is_prepared, not is_enabled. However, considering my
comments above, it should be possible to use enable+disable instead.

> +       .recalc_rate    = mtk_clk_arm_pll_recalc_rate,
> +       .round_rate     = mtk_clk_pll_round_rate,
> +       .set_rate       = mtk_clk_arm_pll_set_rate,
> +};
> +
> +static long mtk_clk_mm_pll_round_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long *prate)
> +{
> +       u32 pcwfbits = 14;
> +       u32 pcw = 0;
> +       u32 postdiv = 0;
> +       u32 r;
> +
> +       if (rate <= 702000000)
> +               postdiv = 2;
> +
> +       *prate = *prate ? *prate : 26000000;

I feel like it wouldn't really be a bad idea to define all the numeric
constants as macros.

> +       rate = mtk_freq_limit(rate);
> +       mtk_calc_pll_freq_cfg(&pcw, &postdiv, rate, *prate, pcwfbits);
> +
> +       r = mtk_calc_pll_vco_freq(*prate, pcw, 1, 1, pcwfbits);
> +       r = (r + pll_posdiv_map[postdiv] - 1) / pll_posdiv_map[postdiv];
> +
> +       return r;
> +}
> +
> +static int mtk_clk_mm_pll_set_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       u32 pcwfbits = 14;
> +       u32 pcw = 0;
> +       u32 postdiv_idx = 0;
> +       int r;
> +
> +       if (rate <= 702000000)
> +               postdiv_idx = 2;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +       r = mtk_calc_pll_freq_cfg(&pcw, &postdiv_idx, rate,
> +                       parent_rate, pcwfbits);
> +
> +       if (r == 0)
> +               mtk_clk_arm_pll_set_rate_regs(hw, pcw, postdiv_idx);
> +
> +       return r;
> +}
> +
> +const struct clk_ops mt8173_mm_pll_ops = {
> +       .is_enabled     = mtk_clk_pll_is_enabled,
> +       .prepare        = mtk_clk_pll_prepare,
> +       .unprepare      = mtk_clk_pll_unprepare,

Ditto.

> +       .recalc_rate    = mtk_clk_arm_pll_recalc_rate,
> +       .round_rate     = mtk_clk_mm_pll_round_rate,
> +       .set_rate       = mtk_clk_mm_pll_set_rate,
> +};
> +
> +static int mtk_clk_univ_pll_prepare(struct clk_hw *hw)
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       u32 r;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       r = readl_relaxed(pll->base_addr) | pll->en_mask;
> +       writel_relaxed(r, pll->base_addr);
> +       wmb();  /* sync write before delay */
> +       udelay(20);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(pll->base_addr) | RST_BAR_MASK;
> +               writel_relaxed(r, pll->base_addr);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void mtk_clk_univ_pll_unprepare(struct clk_hw *hw)
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       u32 r;
> +
> +       if (pll->flags & PLL_AO)
> +               return;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(pll->base_addr) & ~RST_BAR_MASK;
> +               writel_relaxed(r, pll->base_addr);
> +       }
> +
> +       r = readl_relaxed(pll->base_addr) & ~PLL_BASE_EN;
> +       writel_relaxed(r, pll->base_addr);
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +#define UNIV_PLL_POSTDIV_H     6
> +#define UNIV_PLL_POSTDIV_L     4
> +#define UNIV_PLL_POSTDIV_MASK  GENMASK(UNIV_PLL_POSTDIV_H, UNIV_PLL_POSTDIV_L)
> +#define UNIV_PLL_FBKDIV_H      20
> +#define UNIV_PLL_FBKDIV_L      14
> +#define UNIV_PLL_FBKDIV_MASK   GENMASK(UNIV_PLL_FBKDIV_H, UNIV_PLL_FBKDIV_L)
> +
> +static unsigned long mtk_clk_univ_pll_recalc_rate(
> +               struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +       u32 con0 = readl_relaxed(pll->base_addr);
> +       u32 con1 = readl_relaxed(pll->base_addr + 4);
> +
> +       u32 fbkdiv = (con1 & UNIV_PLL_FBKDIV_MASK) >> UNIV_PLL_FBKDIV_L;
> +       u32 posdiv = (con0 & UNIV_PLL_POSTDIV_MASK) >> UNIV_PLL_POSTDIV_L;
> +
> +       u32 vco_freq;
> +       unsigned long r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +
> +       vco_freq = parent_rate * fbkdiv;
> +       r = (vco_freq + pll_posdiv_map[posdiv] - 1) / pll_posdiv_map[posdiv];
> +
> +       return r;
> +}
> +
> +static void mtk_clk_univ_pll_set_rate_regs(
> +               struct clk_hw *hw,
> +               u32 pcw,
> +               u32 postdiv_idx)
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con1_addr = pll->base_addr + 4;
> +       u32 con0;
> +       u32 con1;
> +       u32 pll_en;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       con0 = readl_relaxed(con0_addr);
> +       con1 = readl_relaxed(con1_addr);
> +
> +       pll_en = con0 & PLL_BASE_EN;
> +
> +       /* postdiv */
> +       con0 &= ~UNIV_PLL_POSTDIV_MASK;
> +       con0 |= postdiv_idx << UNIV_PLL_POSTDIV_L;
> +
> +       /* fkbdiv */
> +       con1 &= ~UNIV_PLL_FBKDIV_MASK;
> +       con1 |= pcw << UNIV_PLL_FBKDIV_L;
> +
> +       writel_relaxed(con0, con0_addr);
> +       writel_relaxed(con1, con1_addr);
> +
> +       if (pll_en) {
> +               wmb();  /* sync write before delay */

The comment should say why, not what, because you can easily see that
from the code (wmb() before udelay(20) obviously can't be anything
else than "sync write before delay").

> +               udelay(20);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +static long mtk_clk_univ_pll_round_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long *prate)
> +{
> +       u32 pcwfbits = 0;
> +       u32 pcw = 0;
> +       u32 postdiv = 0;
> +       u32 r;
> +
> +       *prate = *prate ? *prate : 26000000;
> +       rate = mtk_freq_limit(rate);
> +       mtk_calc_pll_freq_cfg(&pcw, &postdiv, rate, *prate, pcwfbits);
> +
> +       r = *prate * pcw;
> +       r = (r + pll_posdiv_map[postdiv] - 1) / pll_posdiv_map[postdiv];
> +
> +       return r;
> +}
> +
> +static int mtk_clk_univ_pll_set_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       u32 pcwfbits = 0;
> +       u32 pcw = 0;
> +       u32 postdiv_idx = 0;
> +       int r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +       r = mtk_calc_pll_freq_cfg(&pcw, &postdiv_idx, rate,
> +                       parent_rate, pcwfbits);
> +
> +       if (r == 0)

I wonder if you shouldn't consider adding an error message to opposite case.

> +               mtk_clk_univ_pll_set_rate_regs(hw, pcw, postdiv_idx);
> +
> +       return r;
> +}
> +
> +const struct clk_ops mt8173_univ_pll_ops = {
> +       .is_enabled     = mtk_clk_pll_is_enabled,
> +       .prepare        = mtk_clk_univ_pll_prepare,
> +       .unprepare      = mtk_clk_univ_pll_unprepare,
> +       .recalc_rate    = mtk_clk_univ_pll_recalc_rate,
> +       .round_rate     = mtk_clk_univ_pll_round_rate,
> +       .set_rate       = mtk_clk_univ_pll_set_rate,
> +};
> +
> +static int mtk_clk_aud_pll_prepare(struct clk_hw *hw)
> +{
> +       unsigned long flags = 0;

No need to initialize.

> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con2_addr = pll->base_addr + 8;

A macro for the offset would look better.

> +       u32 r;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       r = readl_relaxed(pll->pwr_addr) | PLL_PWR_ON;
> +       writel_relaxed(r, pll->pwr_addr);
> +       wmb();  /* sync write before delay */

Why? And couldn't you use writel() instead of writel_relaxed() + wmb()?

> +       udelay(1);
> +
> +       r = readl_relaxed(pll->pwr_addr) & ~PLL_ISO_EN;
> +       writel_relaxed(r, pll->pwr_addr);
> +       wmb();  /* sync write before delay */

Ditto.

> +       udelay(1);
> +
> +       r = readl_relaxed(con0_addr) | pll->en_mask;
> +       writel_relaxed(r, con0_addr);
> +
> +       r = readl_relaxed(con2_addr) | AUDPLL_TUNER_EN;
> +       writel_relaxed(r, con2_addr);
> +       wmb();  /* sync write before delay */

Ditto.

> +       udelay(20);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(con0_addr) | RST_BAR_MASK;
> +               writel_relaxed(r, con0_addr);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +
> +       return 0;
> +}
> +
> +static void mtk_clk_aud_pll_unprepare(struct clk_hw *hw)
> +{
> +       unsigned long flags = 0;
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con2_addr = pll->base_addr + 8;
> +       u32 r;
> +
> +       if (pll->flags & PLL_AO)
> +               return;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       if (pll->flags & HAVE_RST_BAR) {
> +               r = readl_relaxed(con0_addr) & ~RST_BAR_MASK;
> +               writel_relaxed(r, con0_addr);
> +       }
> +
> +       r = readl_relaxed(con2_addr) & ~AUDPLL_TUNER_EN;
> +       writel_relaxed(r, con2_addr);
> +
> +       r = readl_relaxed(con0_addr) & ~PLL_BASE_EN;
> +       writel_relaxed(r, con0_addr);
> +
> +       r = readl_relaxed(pll->pwr_addr) | PLL_ISO_EN;
> +       writel_relaxed(r, pll->pwr_addr);
> +
> +       r = readl_relaxed(pll->pwr_addr) & ~PLL_PWR_ON;
> +       writel_relaxed(r, pll->pwr_addr);
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +#define AUD_PLL_POSTDIV_H      6
> +#define AUD_PLL_POSTDIV_L      4
> +#define AUD_PLL_POSTDIV_MASK   GENMASK(AUD_PLL_POSTDIV_H, AUD_PLL_POSTDIV_L)
> +#define AUD_PLL_PCW_H          30
> +#define AUD_PLL_PCW_L          0
> +#define AUD_PLL_PCW_MASK       GENMASK(AUD_PLL_PCW_H, AUD_PLL_PCW_L)
> +
> +static unsigned long mtk_clk_aud_pll_recalc_rate(
> +               struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +       u32 con0 = readl_relaxed(pll->base_addr);
> +       u32 con1 = readl_relaxed(pll->base_addr + 4);
> +
> +       u32 posdiv = (con0 & AUD_PLL_POSTDIV_MASK) >> AUD_PLL_POSTDIV_L;
> +       u32 pcw = (con1 & AUD_PLL_PCW_MASK) >> AUD_PLL_PCW_L;
> +       u32 pcwfbits = 24;
> +
> +       u32 vco_freq;
> +       unsigned long r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +
> +       vco_freq = mtk_calc_pll_vco_freq(parent_rate, pcw, 1, 1, pcwfbits);
> +       r = (vco_freq + pll_posdiv_map[posdiv] - 1) / pll_posdiv_map[posdiv];
> +
> +       return r;
> +}
> +
> +static void mtk_clk_aud_pll_set_rate_regs(
> +               struct clk_hw *hw,
> +               u32 pcw,
> +               u32 postdiv_idx)
> +{
> +       unsigned long flags = 0;
> +       struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +       void __iomem *con0_addr = pll->base_addr;
> +       void __iomem *con1_addr = pll->base_addr + 4;
> +       void __iomem *con2_addr = pll->base_addr + 8;
> +       u32 con0;
> +       u32 con1;
> +       u32 pll_en;
> +
> +       spin_lock_irqsave(pll->lock, flags);
> +
> +       con0 = readl_relaxed(con0_addr);
> +       con1 = readl_relaxed(con1_addr);
> +
> +       pll_en = con0 & PLL_BASE_EN;
> +
> +       /* set postdiv */
> +       con0 &= ~AUD_PLL_POSTDIV_MASK;
> +       con0 |= postdiv_idx << AUD_PLL_POSTDIV_L;
> +       writel_relaxed(con0, con0_addr);
> +
> +       /* set pcw */
> +       con1 &= ~AUD_PLL_PCW_MASK;
> +       con1 |= pcw << AUD_PLL_PCW_L;
> +
> +       if (pll_en)
> +               con1 |= PLL_PCW_CHG;
> +
> +       writel_relaxed(con1, con1_addr);
> +       writel_relaxed(con1 + 1, con2_addr);
> +
> +       if (pll_en) {
> +               wmb();  /* sync write before delay */

Same as above.

> +               udelay(20);
> +       }
> +
> +       spin_unlock_irqrestore(pll->lock, flags);
> +}
> +
> +static long mtk_clk_aud_pll_round_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long *prate)
> +{
> +       u32 pcwfbits = 24;
> +       u32 pcw = 0;
> +       u32 postdiv = 0;
> +       u32 r;
> +
> +       *prate = *prate ? *prate : 26000000;
> +       rate = mtk_freq_limit(rate);
> +       mtk_calc_pll_freq_cfg(&pcw, &postdiv, rate, *prate, pcwfbits);
> +
> +       r = mtk_calc_pll_vco_freq(*prate, pcw, 1, 1, pcwfbits);
> +       r = (r + pll_posdiv_map[postdiv] - 1) / pll_posdiv_map[postdiv];
> +
> +       return r;
> +}
> +
> +static int mtk_clk_aud_pll_set_rate(
> +               struct clk_hw *hw,
> +               unsigned long rate,
> +               unsigned long parent_rate)
> +{
> +       u32 pcwfbits = 24;
> +       u32 pcw = 0;
> +       u32 postdiv_idx = 0;
> +       int r;
> +
> +       parent_rate = parent_rate ? parent_rate : 26000000;
> +       r = mtk_calc_pll_freq_cfg(&pcw, &postdiv_idx, rate,
> +                       parent_rate, pcwfbits);
> +
> +       if (r == 0)
> +               mtk_clk_aud_pll_set_rate_regs(hw, pcw, postdiv_idx);
> +
> +       return r;
> +}
> +
> +const struct clk_ops mt8173_aud_pll_ops = {
> +       .is_enabled     = mtk_clk_pll_is_enabled,
> +       .prepare        = mtk_clk_aud_pll_prepare,
> +       .unprepare      = mtk_clk_aud_pll_unprepare,
> +       .recalc_rate    = mtk_clk_aud_pll_recalc_rate,
> +       .round_rate     = mtk_clk_aud_pll_round_rate,
> +       .set_rate       = mtk_clk_aud_pll_set_rate,
> +};
> diff --git a/drivers/clk/mediatek/clk-mt8173-pll.h b/drivers/clk/mediatek/clk-mt8173-pll.h
> new file mode 100644
> index 0000000..663ab4b
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8173-pll.h
> @@ -0,0 +1,14 @@
> +#ifndef __DRV_CLK_MT8173_PLL_H
> +#define __DRV_CLK_MT8173_PLL_H
> +
> +/*
> + * This is a private header file. DO NOT include it except clk-*.c.
> + */

I'd say this comment is redundant, because this file is already
private to mediatek clock code by how it is located in
drivers/clk/mediatek.

> +
> +extern const struct clk_ops mt8173_sdm_pll_ops;
> +extern const struct clk_ops mt8173_arm_pll_ops;
> +extern const struct clk_ops mt8173_mm_pll_ops;
> +extern const struct clk_ops mt8173_univ_pll_ops;
> +extern const struct clk_ops mt8173_aud_pll_ops;
> +
> +#endif /* __DRV_CLK_MT8173_PLL_H */
> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c
> new file mode 100644
> index 0000000..d75e591
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8173.c
> @@ -0,0 +1,1035 @@
> +/*
> + * Copyright (c) 2014 MediaTek Inc.
> + * Author: James Liao <jamesjj.liao@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-pll.h"
> +#include "clk-gate.h"
> +#include "clk-mt8173-pll.h"
> +
> +#include <dt-bindings/clock/mt8173-clk.h>
> +
> +/* ROOT */
> +#define clk_null               "clk_null"
> +#define clk26m                 "clk26m"
> +#define clk32k                 "clk32k"

Hmm, what's this? What's the purpose of defining the same string, just
without the quotation marks?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/