linux-kernel - Re: [PATCH v2] clk: imx: pllv3: support fractional multiplier on vf610 PLL1/PLL2

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHQ1cqE-wF0ceMHwgRpUonkbpoCpnk8Z+8aS6rH8igsPQfxF-Q@mail.gmail.com>
Date:   Sun, 18 Dec 2016 23:07:50 -0800
From:   Andrey Smirnov <andrew.smirnov@...il.com>
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>,
        Stephen Boyd <sboyd@...eaurora.org>, linux-clk@...r.kernel.org,
        Chris Healy <cphealy@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] clk: imx: pllv3: support fractional multiplier on
 vf610 PLL1/PLL2

On Wed, Dec 14, 2016 at 12:14 AM, Nikita Yushchenko
<nikita.yoush@...entembedded.com> wrote:
> On vf610, PLL1 and PLL2 have registers to configure fractional part of
> frequency multiplier.
>
> This patch adds support for these registers.
>
> This fixes "fast system clock" issue on boards where bootloader sets
> fractional multiplier for PLL1.
>
> Suggested-by: Andrey Smirnov <andrew.smirnov@...il.com>
> CC: Chris Healy <cphealy@...il.com>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@...entembedded.com>
> ---
> This version restructures the patch:
> - introduce explicit structure to store mf (multiply factor) consisting of
>   integer part, numenator and denumenator;

Spelling. See my comments in the actual code below.

> - provides routines that:
>   - calculate rate based on parent rate and mf,
>   - read mf from hw,
>   - write mf to hw,
>   - calculate best mf for wanted rate;

I do see the benefit of having calculation routines since those get to
be reused in multiple places, read/write subroutines, OTOH, are used
only once, so I'd encourage you to drop them and move that code back
to where it was in v1.

If you are going to make changes to this patch, I also would like to
urge you to consider naming you calculation routines in a more
consistent fashion. Right now what you have is
clk_pllv3_vf610_mf_for_rate and clk_pllv3_vf610_calc_rate, those two
functions are symmetric in what they do, but, IMHO, that symmetricity
is not very obvious in their names. I'd suggest converting it as such
(and yes, the second one does return struct clk_pllv3_vf610_mf by
value):

static unsigned long clk_pllv3_vf610_mf_to_rate(unsigned long
parent_rate, const struct clk_pllv3_vf610_mf *mf)

and

struct clk_pllv3_vf610_mf clk_pllv3_vf610_rate_to_mf(unsigned long
parent_rate, unsigned long rate)


That all being said, those all are just my personal preferences, so
take them with a grain of salt.


> - implement recalc_rate() / round_rate() / set_rate() based on these.
>
>  drivers/clk/imx/clk-pllv3.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/imx/clk-vf610.c |   4 +-
>  drivers/clk/imx/clk.h       |   1 +
>  3 files changed, 113 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c
> index 7a6acc3e4a92..89ab42e7d6c0 100644
> --- a/drivers/clk/imx/clk-pllv3.c
> +++ b/drivers/clk/imx/clk-pllv3.c
> @@ -21,6 +21,9 @@
>  #define PLL_NUM_OFFSET         0x10
>  #define PLL_DENOM_OFFSET       0x20
>
> +#define PLL_VF610_NUM_OFFSET   0x20
> +#define PLL_VF610_DENOM_OFFSET 0x30
> +
>  #define BM_PLL_POWER           (0x1 << 12)
>  #define BM_PLL_LOCK            (0x1 << 31)
>  #define IMX7_ENET_PLL_POWER    (0x1 << 5)
> @@ -292,6 +295,110 @@ static const struct clk_ops clk_pllv3_av_ops = {
>         .set_rate       = clk_pllv3_av_set_rate,
>  };
>
> +struct clk_pllv3_vf610_mf {
> +       u32 mfi;        /* integer part, can be 20 or 22 */
> +       u32 mfn;        /* numinator, 30-bit value */

s/numinator/numerator/

> +       u32 mfd;        /* denomenator, 30-bit value, must be less than mfn */

s/denomenator/denominator/

Anyway, regardless of my comments above:

Teseted-by: Andrey Smirnov <andrew.smirnov@...il.com>

Regards,
Andrey Smirnov