lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 13 Dec 2016 15:19:00 -0800
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Nikita Yushchenko <nikita.yoush@...entembedded.com>
Cc:     Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-clk@...r.kernel.org, Chris Healy <cphealy@...il.com>,
        linux-kernel@...r.kernel.org,
        Andrey Smirnov <andrew.smirnov@...il.com>
Subject: Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610
 PLL1/PLL2

On 12/13, Nikita Yushchenko wrote:
> >> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> >> index 19f9b622981a..24a9e914e0d5 100644
> >> --- a/drivers/clk/imx/clk-pllv3.c
> >> +++ b/drivers/clk/imx/clk-pllv3.c
> 
> >> +
> >> +	temp64 *= mfn;
> >> +	do_div(temp64, mfd);
> >> +
> >> +	return (parent_rate * div) + (u32)temp64;
> >> +}
> >> +
> >> +static long clk_pllv3_vf610_round_rate(struct clk_hw *hw, unsigned long rate,
> >> +				    unsigned long *prate)
> >> +{
> >> +	unsigned long parent_rate = *prate;
> >> +	unsigned int mfi = (rate >= 22 * parent_rate) ? 22 : 20;
> > 
> > What is the importance of 22 and 20? Hint, at the least it needs
> > a comment.
> 
> These come directly from datasheet:
> 
> Frequency multipler selection (MFI).
> 0: Fout = Fref * 20
> 1: Fout = Fref * 22
> 
> These numbers (20 / 22) are common among flavours of pllv3 hardware.
> In similar places in the same file (e.g. in clk_pllv3_recalc_rate(), in
> clk_pllv3_set_rate() ,etc) there are no comments explaining them.
> 
> Are you sure this place is special and comment is needed here?

Probably an oversight when merging the code before. Please add a
comment, or make some sort of function like

	rate_to_mfi(rate, parent_rate)

And then add a comment above the function to describe what's
special about 22 and 20.

> 
> >> +	u32 mfn, mfd = 0x3fffffff;
> >> +	u64 temp64;
> >> +
> >> +	if (rate <= parent_rate * mfi)
> >> +		mfn = 0;
> >> +	else if (rate >= parent_rate * (mfi + 1))
> >> +		mfn = mfd - 1;
> >> +	else {
> >> +		/* rate = parent_rate * (mfi + mfn/mfd) */
> >> +		temp64 = rate - parent_rate * mfi;
> >> +		temp64 *= mfd;
> >> +		do_div(temp64, parent_rate);
> >> +		mfn = temp64;
> >> +	}
> >> +
> >> +	temp64 = ((u64)mfd * mfi + mfn) * parent_rate;
> >> +	do_div(temp64, mfd);
> >> +	return (u32)temp64;
> > 
> > Do we need the cast here for some reason?
> 
> Just for readability, can remove if it hurts.

Please do.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists