Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752167AbbBSIYT (ORCPT ); Thu, 19 Feb 2015 03:24:19 -0500 Received: from metis.ext.pengutronix.de ([92.198.50.35]:60061 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751375AbbBSIYS (ORCPT ); Thu, 19 Feb 2015 03:24:18 -0500 Date: Thu, 19 Feb 2015 09:24:11 +0100 From: Sascha Hauer To: Tomasz Figa Cc: Matthias Brugger , James Liao , Mike Turquette , YH Chen =?utf-8?B?KOmZs+aYseixqik=?= , "linux-kernel@vger.kernel.org" , Henry Chen , Rob Herring , kernel@pengutronix.de, Yingjoe Chen =?utf-8?B?KOmZs+iLsea0sik=?= , Eddie Huang , Lee Jones , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH 06/13] clk: mediatek: Add basic clocks for Mediatek MT8173. Message-ID: <20150219082410.GU12209@pengutronix.de> References: <1423478845-2835-1-git-send-email-s.hauer@pengutronix.de> <1423478845-2835-7-git-send-email-s.hauer@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 09:06:56 up 126 days, 19:20, 74 users, load average: 0.06, 0.07, 0.06 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10907 Lines: 328 On Fri, Feb 13, 2015 at 06:56:53PM +0900, Tomasz Figa wrote: > Please find my comments inline. > > On Mon, Feb 9, 2015 at 7:47 PM, Sascha Hauer wrote: > > From: James Liao > > > > This patch adds basic clocks for MT8173, including TOPCKGEN, PLLs, > > INFRA and PERI clocks. > > > > Signed-off-by: James Liao > > Signed-off-by: Henry Chen > > Signed-off-by: Sascha Hauer > > --- > > 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 > > + * > > + * 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 > > +#include > > +#include > > +#include > > + > > +#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. It's the table for a power of two divider. This can be calculated, no need for a table. > > > + > > +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? yes > > > + do_div(vco, prediv); > > + > > + if (vco & GENMASK(pcwfbits - 1, 0)) > > + c = 1; > > What is c? Could the variable has a more meaningful name? I have no idea. This is not explained in the datasheet. > > > + > > + 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? Did that. > > > + 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? I don't know what's going on here. What I find suspicious is that when freq is between freq_min and freq_min + 16 it is not changed. I just dropped this. Whoever thinks he needs this can probably explain what it's good for. > > > + 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... It seems it is used by some callers to ensure a minimum divider. > > > + > > + 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? Hm, I think I rather use usleep_range instead of udelay and keep it in the prepare/unprepare path. I don't think there's need to enable/disable the PLLs in the hot pathes. > > +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. I will decide for one of both. > > > + .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. The above is unnecessary. The clk framework will never call us with prate == NULL. > > + /* 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"). I'll drop the comment. > > + 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. I'l refactor this so that mtk_calc_pll_freq_cfg() can't fail. This won't be necessary anymore. > > +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()? The original author claims this is needed. I can't prove the opposite, so I kept it. Anyway, it seems that writel() is writel_relaxed() + a wmb(), so I'll change it. > > +#include > > + > > +/* 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? I think the intention was to let the compiler detect typos when using the same strings multiple times. I don't like this either, will drop. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- 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/