On 1 July 2017 at 03:22, Stephen Boyd <sb...@codeaurora.org> wrote: > On 06/30, Chunyan Zhang wrote: >> Hi Stephen, >> >> On 30 June 2017 at 09:44, Stephen Boyd <sb...@codeaurora.org> wrote: >> > On 06/22, Chunyan Zhang wrote: >> >> On 20 June 2017 at 09:37, Stephen Boyd <sb...@codeaurora.org> wrote: >> >> > On 06/18, Chunyan Zhang wrote: >> >> >> + 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. >> >> >> > >> > Rename is fine, but pll can still be marked const? >> >> Oh, sorry I misunderstood :) >> You mean mark the input parameter "pll" const, right? > > Yes. > >> >> >> + >> >> >> +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. >> > >> > Does the hardware width change? Still not clear to me what's >> > going on here. >> >> I heard from my colleague, that because the calculation precision on >> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs, when the >> width of the value of PLL_KINT is larger than 21, the value is too >> large to be multiplied on 32-bit Spreadtrum's SoCs. > > It sounds like you're saying that the clk hardware is not > changing, but the sizeof(long) is different on 64-bit and 32-bit > CPUs so you've added this ifndef here for that. >
I finally figure out that this #ifndef is indeed not needed, thanks to your querying :) They told me that on Spreadtrum's 32-bit SoCs, only 20-bit of PLL_KINT is used and 3-bit is reserved in order to keep in line with the PLLs on 64-bit SoC. And we can get the same effect only if we define PLL with the specific 'width' and 'shift' for PLL_KINT. I'll remove this statement and 'i' from here and sprd_pll_helper_recalc_rate() function. >> >> i = width < 21 ? 0 : i - 20; >> >> Here ' i ' is used to adjust 'shift' rather than 'width' like below ( >> wrote the code back for convenience of understanding) >> >> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) * >> + ((mask >> (shift + i)) + 1), refin * 100) << i; >> > > Having the types for all these variables would also be helpful. > > u32 mask, shift, width, kint, nint; > unsigned long refin, fvco; > > Why don't we do 64-bit math here instead of 32-bit math? And use > DIV_ROUND_CLOSEST_ULL? Agree, we should use 64-bit math, will address this. Thanks, Chunyan > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project