From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753207AbdFVKRg (ORCPT ); Thu, 22 Jun 2017 06:17:36 -0400 Received: from mail-ot0-f193.google.com ([74.125.82.193]:36449 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753188AbdFVKRc (ORCPT ); Thu, 22 Jun 2017 06:17:32 -0400 MIME-Version: 1.0 In-Reply-To: <20170620013710.GJ4493@codeaurora.org> References: <20170618015855.27738-1-chunyan.zhang@spreadtrum.com> <20170618015855.27738-8-chunyan.zhang@spreadtrum.com> <20170620013710.GJ4493@codeaurora.org> From: Chunyan Zhang Date: Thu, 22 Jun 2017 18:17:26 +0800 Message-ID: Subject: Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support To: Stephen Boyd Cc: Chunyan Zhang , Michael Turquette , Rob Herring , Mark Rutland , linux-clk@vger.kernel.org, "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Arnd Bergmann , Mark Brown , Xiaolong Zhang , Orson Zhai , Geng Ren , Ben Li Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20 June 2017 at 09:37, Stephen Boyd wrote: > On 06/18, Chunyan Zhang wrote: >> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile >> index 83232e5..c593a93 100644 >> --- a/drivers/clk/sprd/Makefile >> +++ b/drivers/clk/sprd/Makefile >> @@ -1,3 +1,3 @@ >> ifneq ($(CONFIG_OF),) >> -obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o >> +obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o >> endif >> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c >> new file mode 100644 >> index 0000000..6c908e4 >> --- /dev/null >> +++ b/drivers/clk/sprd/ccu_pll.c >> @@ -0,0 +1,241 @@ >> +/* >> + * Spreadtrum pll clock driver >> + * >> + * Copyright (C) 2015~2017 Spreadtrum, Inc. >> + * >> + * SPDX-License-Identifier: GPL-2.0 >> + */ >> + >> +#include >> +#include > > Is this include used? Should be clk-provider? Right, will remove it. > >> +#include >> +#include >> + >> +#include "ccu_pll.h" >> + >> +#define CCU_PLL_1M 1000000 >> +#define CCU_PLL_10M (CCU_PLL_1M * 10) >> + >> +#define pindex(pll, member) \ >> + (pll->factors[member].shift / (8 * sizeof(pll->regs[0]))) >> + >> +#define pshift(pll, member) \ >> + (pll->factors[member].shift % (8 * sizeof(pll->regs[0]))) >> + >> +#define pwidth(pll, member) \ >> + pll->factors[member].width >> + >> +#define pmask(pll, member) \ >> + ((pwidth(pll, member)) ? \ >> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \ >> + pshift(pll, member)) : 0) >> + >> +#define pinternal(pll, cfg, member) \ >> + (cfg[pindex(pll, member)] & pmask(pll, member)) >> + >> +#define pinternal_val(pll, cfg, member) \ >> + (pinternal(pll, cfg, member) >> pshift(pll, member)) >> + >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll) > > pll could be const? What this function returns is a factor used to calculate the pll rate later, I will rename this function in the next iterator. > >> +{ >> + u8 shift, index, refin_id = 3; >> + u32 mask; >> + const unsigned long refin[4] = { 2, 4, 13, 26 }; >> + >> + if (pwidth(pll, PLL_REFIN)) { >> + index = pindex(pll, PLL_REFIN); >> + shift = pshift(pll, PLL_REFIN); >> + mask = pmask(pll, PLL_REFIN); >> + refin_id = (ccu_pll_readl(pll, index) & mask) >> shift; >> + if (refin_id > 3) >> + refin_id = 3; >> + } >> + >> + return refin[refin_id]; >> +} >> + >> +static u8 pll_get_ibias(unsigned long rate, const u64 *table) >> +{ >> + u64 i; >> + u8 num = table[0]; >> + >> + for (i = 0; i < num; i++) >> + if (rate <= table[i + 1]) >> + break; >> + >> + return i == num ? num - 1 : i; >> +} >> + >> +static unsigned long ccu_pll_helper_recalc_rate(struct ccu_pll *pll, >> + unsigned long parent_rate) >> +{ >> + unsigned long rate, refin, k1, k2; >> + unsigned long kint = 0, nint; >> + u32 reg_num = pll->regs[0]; >> + u32 *cfg; >> + u32 i; >> + u32 mask; >> + >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL); >> + if (!cfg) >> + return -ENOMEM; >> + >> + for (i = 0; i < reg_num; i++) >> + cfg[i] = ccu_pll_readl(pll, i); >> + >> + refin = pll_get_refin_rate(pll); >> + >> + if (pinternal(pll, cfg, PLL_PREDIV)) >> + refin = refin * 2; >> + >> + if (pwidth(pll, PLL_POSTDIV) && >> + ((pll->fflag == 1 && pinternal(pll, cfg, PLL_POSTDIV)) || >> + (!pll->fflag && !pinternal(pll, cfg, PLL_POSTDIV)))) >> + refin = refin / 2; >> + >> + if (!pinternal(pll, cfg, PLL_DIV_S)) >> + rate = refin * pinternal_val(pll, cfg, PLL_N) * CCU_PLL_10M; >> + else { > > Please include braces on the if as well when another branch has them. Sure. > >> + nint = pinternal_val(pll, cfg, PLL_NINT); >> + if (pinternal(pll, cfg, PLL_SDM_EN)) >> + kint = pinternal_val(pll, cfg, PLL_KINT); >> + >> + mask = pmask(pll, PLL_KINT); >> +#ifdef CONFIG_64BIT >> + k1 = 1000; >> + k2 = 1000; >> + rate = DIV_ROUND_CLOSEST(refin * kint * k1, >> + ((mask >> __ffs(mask)) + 1)) * >> + k2 + refin * nint * CCU_PLL_1M; >> +#else >> + k1 = 100; >> + k2 = 10000; >> + i = pwidth(pll, PLL_KINT); >> + i = i < 21 ? 0 : i - 21; >> + rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1, >> + ((mask >> (__ffs(mask) + i)) + 1)) * >> + k2 + refin * nint * CCU_PLL_1M; >> +#endif >> + } >> + >> + return rate; >> +} >> + >> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll, >> + unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + u32 mask, shift, width, ibias_val, index, kint, nint; >> + u32 reg_num = pll->regs[0], i = 0; >> + unsigned long refin, fvco = rate; >> + struct reg_cfg *cfg; >> + >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL); >> + if (!cfg) >> + return -ENOMEM; >> + >> + refin = pll_get_refin_rate(pll); >> + >> + mask = pmask(pll, PLL_PREDIV); >> + index = pindex(pll, PLL_PREDIV); >> + width = pwidth(pll, PLL_PREDIV); >> + if (width && (ccu_pll_readl(pll, index) & mask)) >> + refin = refin * 2; >> + >> + mask = pmask(pll, PLL_POSTDIV); >> + index = pindex(pll, PLL_POSTDIV); >> + width = pwidth(pll, PLL_POSTDIV); >> + cfg[index].msk = mask; >> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) || >> + (pll->fflag == 0 && fvco > pll->fvco))) >> + cfg[index].val |= mask; >> + >> + if (width && fvco <= pll->fvco) >> + fvco = fvco * 2; >> + >> + mask = pmask(pll, PLL_DIV_S); >> + index = pindex(pll, PLL_DIV_S); >> + cfg[index].val |= mask; >> + cfg[index].msk |= mask; >> + >> + mask = pmask(pll, PLL_SDM_EN); >> + index = pindex(pll, PLL_SDM_EN); >> + cfg[index].val |= mask; >> + cfg[index].msk |= mask; >> + >> + nint = fvco/(refin * CCU_PLL_1M); >> + >> + mask = pmask(pll, PLL_NINT); >> + index = pindex(pll, PLL_NINT); >> + shift = pshift(pll, PLL_NINT); >> + cfg[index].val |= (nint << shift) & mask; >> + cfg[index].msk |= mask; >> + >> + mask = pmask(pll, PLL_KINT); >> + index = pindex(pll, PLL_KINT); >> + width = pwidth(pll, PLL_KINT); >> + shift = pshift(pll, PLL_KINT); >> +#ifndef CONFIG_64BIT >> + i = width < 21 ? 0 : i - 21; >> +#endif > > What's this? Why do we depend on CONFIG_64BIT? On 32-bit SoCs, the largest width we can support is limited due to the limitation of calculation precision. > >> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) * >> + ((mask >> (shift + i)) + 1), refin * 100) << i; >> + cfg[index].val |= (kint << shift) & mask; >> + cfg[index].msk |= mask; >> + >> + ibias_val = pll_get_ibias(fvco, pll->itable); >> + >> + mask = pmask(pll, PLL_IBIAS); >> + index = pindex(pll, PLL_IBIAS); >> + shift = pshift(pll, PLL_IBIAS); >> + cfg[index].val |= ibias_val << shift & mask; >> + cfg[index].msk |= mask; >> + >> + for (i = 0; i < reg_num; i++) { >> + if (cfg[i].msk) >> + ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk); >> + } >> + > > Are we waiting for the writel() to go through above? If so we > need a readl() of the same register to make sure the write has > completed before delaying. After writing these configuration registers, we have to wait a certain time to make sure the pll has worked as we configured. This depends on other circuit part, so we use udelay rather than reading the same register. > >> + udelay(pll->udelay); >> + >> + return 0; >> +} >> + >> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct ccu_pll *pll = hw_to_ccu_pll(hw); >> + >> + return ccu_pll_helper_recalc_rate(pll, parent_rate); >> +} >> + >> +static int ccu_pll_set_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct ccu_pll *pll = hw_to_ccu_pll(hw); >> + >> + return ccu_pll_helper_set_rate(pll, rate, parent_rate); >> +} >> + >> +static int ccu_pll_clk_prepare(struct clk_hw *hw) >> +{ >> + struct ccu_pll *pll = hw_to_ccu_pll(hw); >> + >> + udelay(pll->udelay); >> + >> + return 0; >> +} >> + >> +static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long *prate) >> +{ >> + return rate; >> +} >> + >> +const struct clk_ops ccu_pll_ops = { >> + .prepare = ccu_pll_clk_prepare, >> + .recalc_rate = ccu_pll_recalc_rate, >> + .round_rate = ccu_pll_round_rate, >> + .set_rate = ccu_pll_set_rate, >> +}; >> diff --git a/drivers/clk/sprd/ccu_pll.h b/drivers/clk/sprd/ccu_pll.h >> new file mode 100644 >> index 0000000..66fe5d1 >> --- /dev/null >> +++ b/drivers/clk/sprd/ccu_pll.h >> @@ -0,0 +1,123 @@ >> +/* >> + * Spreadtrum clock pll configurations >> + * >> + * Copyright (C) 2015~2017 Spreadtrum, Inc. >> + * >> + * SPDX-License-Identifier: GPL-2.0 >> + */ >> + >> +#ifndef _CCU_PLL_H_ >> +#define _CCU_PLL_H_ >> + >> +#include "ccu_common.h" >> + >> +struct reg_cfg { >> + u32 val; >> + u32 msk; >> +}; >> + >> +struct ccu_bit_field { >> + u8 shift; >> + u8 width; >> +}; >> + >> +enum { >> + PLL_LOCK_DONE = 0, > > Drop the = 0 please unless it's needed for something? OK. > >> + PLL_DIV_S, >> + PLL_MOD_EN, >> + PLL_SDM_EN, >> + PLL_REFIN, >> + PLL_IBIAS, >> + PLL_N, >> + PLL_NINT, >> + PLL_KINT, >> + PLL_PREDIV, >> + PLL_POSTDIV, >> + >> + PLL_FACT_MAX >> +}; >> + >> +/* >> + * struct ccu_pll - defination of adjustable pll clock > > s/defination/definition/ > >> + * >> + * @reg: registers used to set the configuration of pll clock, >> + * reg[0] shows how many registers this pll clock uses. >> + * @itable: pll ibias table, itable[0] means how many items this >> + * table includes >> + * @udelay delay time after setting rate >> + * @factors used to calculate the pll clock rate >> + * @fvco: fvco threshold rate >> + * @fflag: fvco flag >> + */ >> +struct ccu_pll { >> + const u32 *regs; >> + const u64 *itable; >> + u16 udelay; >> + const struct ccu_bit_field *factors; > > Does this change across the different PLLs? Would be nice to not Yes, different PLL has diferent bit field arragement, even on the same SoC. Thanks for your review, Chunyan > need the bit field thing. > >> + u64 fvco; >> + u16 fflag; >> + >> + struct ccu_common common; > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project