Date:   Mon, 29 Apr 2019 13:23:30 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Paul Walmsley <paul.walmsley@...ive.com>,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Paul Walmsley <paul@...an.com>,
        Wesley Terpstra <wesley@...ive.com>,
        Palmer Dabbelt <palmer@...ive.com>,
        Michael Turquette <mturquette@...libre.com>,
        Megan Wachs <megan@...ive.com>
Subject: Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library

Quoting Paul Walmsley (2019-04-11 01:27:32)
> diff --git a/drivers/clk/analogbits/Kconfig b/drivers/clk/analogbits/Kconfig
> new file mode 100644
> index 000000000000..b5fd60c7f136
> --- /dev/null
> +++ b/drivers/clk/analogbits/Kconfig
> @@ -0,0 +1,2 @@

Add SPDX for this file?

> +config CLK_ANALOGBITS_WRPLL_CLN28HPC
> +       bool
> diff --git a/drivers/clk/analogbits/Makefile b/drivers/clk/analogbits/Makefile
> new file mode 100644
> index 000000000000..bb51a3ae77a7
> --- /dev/null
> +++ b/drivers/clk/analogbits/Makefile
> @@ -0,0 +1 @@

Add SPDX for this file?

> +obj-$(CONFIG_CLK_ANALOGBITS_WRPLL_CLN28HPC)    += wrpll-cln28hpc.o
> diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c b/drivers/clk/analogbits/wrpll-cln28hpc.c
> new file mode 100644
> index 000000000000..2027872719e1
> --- /dev/null
> +++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018-2019 SiFive, Inc.
> + * Wesley Terpstra
> + * Paul Walmsley
> + *
> + * This library supports configuration parsing and reprogramming of
> + * the CLN28HPC variant of the Analog Bits Wide Range PLL.  The
> + * intention is for this library to be reusable for any device that
> + * integrates this PLL; thus the register structure and programming
> + * details are expected to be provided by a separate IP block driver.
> + *
> + * The bulk of this code is primarily useful for clock configurations
> + * that must operate at arbitrary rates, as opposed to clock configurations
> + * that are restricted by software or manufacturer guidance to a small,
> + * pre-determined set of performance points.
> + *
> + * References:
> + * - Analog Bits "Wide Range PLL Datasheet", version 2015.10.01
> + * - SiFive FU540-C000 Manual v1p0, Chapter 7 "Clocking and Reset"
> + *   https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> + */
> +
> +#include <linux/bug.h>
> +#include <linux/err.h>
> +#include <linux/log2.h>
> +#include <linux/math64.h>
> +#include <linux/clk/analogbits-wrpll-cln28hpc.h>
> +
> +/* MIN_INPUT_FREQ: minimum input clock frequency, in Hz (Fref_min) */
> +#define MIN_INPUT_FREQ                 7000000
> +
> +/* MAX_INPUT_FREQ: maximum input clock frequency, in Hz (Fref_max) */
> +#define MAX_INPUT_FREQ                 600000000
> +
> +/* MIN_POST_DIVIDE_REF_FREQ: minimum post-divider reference frequency, in Hz */
> +#define MIN_POST_DIVR_FREQ             7000000
> +
> +/* MAX_POST_DIVIDE_REF_FREQ: maximum post-divider reference frequency, in Hz */
> +#define MAX_POST_DIVR_FREQ             200000000
> +
> +/* MIN_VCO_FREQ: minimum VCO frequency, in Hz (Fvco_min) */
> +#define MIN_VCO_FREQ                   2400000000UL
> +
> +/* MAX_VCO_FREQ: maximum VCO frequency, in Hz (Fvco_max) */
> +#define MAX_VCO_FREQ                   4800000000ULL
> +
> +/* MAX_DIVQ_DIVISOR: maximum output divisor.  Selected by DIVQ = 6 */
> +#define MAX_DIVQ_DIVISOR               64
> +
> +/* MAX_DIVR_DIVISOR: maximum reference divisor.  Selected by DIVR = 63 */
> +#define MAX_DIVR_DIVISOR               64
> +
> +/* MAX_LOCK_US: maximum PLL lock time, in microseconds (tLOCK_max) */
> +#define MAX_LOCK_US                    70
> +
> +/*
> + * ROUND_SHIFT: number of bits to shift to avoid precision loss in the rounding
> + *              algorithm
> + */
> +#define ROUND_SHIFT                    20
> +
> +/*
> + * Private functions
> + */
> +
> +/**
> + * __wrpll_calc_filter_range() - determine PLL loop filter bandwidth
> + * @post_divr_freq: input clock rate after the R divider
> + *
> + * Select the value to be presented to the PLL RANGE input signals, based
> + * on the input clock frequency after the post-R-divider @post_divr_freq.
> + * This code follows the recommendations in the PLL datasheet for filter
> + * range selection.
> + *
> + * Return: The RANGE value to be presented to the PLL configuration inputs,
> + *         or -1 upon error.
> + */
> +static int __wrpll_calc_filter_range(unsigned long post_divr_freq)
> +{
> +       u8 range;
> +
> +       if (post_divr_freq < MIN_POST_DIVR_FREQ ||
> +           post_divr_freq > MAX_POST_DIVR_FREQ) {
> +               WARN(1, "%s: post-divider reference freq out of range: %lu",
> +                    __func__, post_divr_freq);
> +               return -1;
> +       }
> +
> +       if (post_divr_freq < 11000000)
> +               range = 1;
> +       else if (post_divr_freq < 18000000)
> +               range = 2;
> +       else if (post_divr_freq < 30000000)
> +               range = 3;
> +       else if (post_divr_freq < 50000000)
> +               range = 4;
> +       else if (post_divr_freq < 80000000)
> +               range = 5;
> +       else if (post_divr_freq < 130000000)
> +               range = 6;
> +       else
> +               range = 7;

Nitpick: This might be easier to read with a switch statement:

	switch (post_divr_freq) {
	case 0 ... 11000000:
		return 1;
	case 11000001 ... 18000000:
		return 2;
	case 18000001 ... 30000000:
		return 3;
	case 30000001 ... 50000000:
		return 4;
	case 50000000 ... 80000000:
		return 5;
	case 80000001 ... 130000000:
		return 6;
	}

	return 7;

> +
> +       return range;
> +}
> +
> +/**
> + * __wrpll_calc_fbdiv() - return feedback fixed divide value
> + * @c: ptr to a struct analogbits_wrpll_cfg record to read from
> + *
> + * The internal feedback path includes a fixed by-two divider; the
> + * external feedback path does not.  Return the appropriate divider
> + * value (2 or 1) depending on whether internal or external feedback
> + * is enabled.  This code doesn't test for invalid configurations
> + * (e.g. both or neither of WRPLL_FLAGS_*_FEEDBACK are set); it relies
> + * on the caller to do so.
> + *
> + * Context: Any context.  Caller must protect the memory pointed to by
> + *          @c from simultaneous modification.
> + *
> + * Return: 2 if internal feedback is enabled or 1 if external feedback
> + *         is enabled.
> + */
> +static u8 __wrpll_calc_fbdiv(struct analogbits_wrpll_cfg *c)

const c?

> +{
> +       return (c->flags & WRPLL_FLAGS_INT_FEEDBACK_MASK) ? 2 : 1;
> +}
> +
> +/**
> + * __wrpll_calc_divq() - determine DIVQ based on target PLL output clock rate
> + * @target_rate: target PLL output clock rate
> + * @vco_rate: pointer to a u64 to store the computed VCO rate into
> + *
> + * Determine a reasonable value for the PLL Q post-divider, based on the
> + * target output rate @target_rate for the PLL.  Along with returning the
> + * computed Q divider value as the return value, this function stores the
> + * desired target VCO rate into the variable pointed to by @vco_rate.
> + *
> + * Context: Any context.  Caller must protect the memory pointed to by
> + *          @vco_rate from simultaneous access or modification.
> + *
> + * Return: a positive integer DIVQ value to be programmed into the hardware
> + *         upon success, or 0 upon error (since 0 is an invalid DIVQ value)

Why are we doing that? Can't we return a normal error code and test for
it being negative and then consider the number if its greater than 0 to
be valid?

> + */
> +static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)

Why does target_rate need to be u32? Can it be unsigned long?

> +{
> +       u64 s;
> +       u8 divq = 0;
> +
> +       if (!vco_rate) {
> +               WARN_ON(1);
> +               goto wcd_out;
> +       }
> +
> +       s = div_u64(MAX_VCO_FREQ, target_rate);
> +       if (s <= 1) {
> +               divq = 1;
> +               *vco_rate = MAX_VCO_FREQ;
> +       } else if (s > MAX_DIVQ_DIVISOR) {
> +               divq = ilog2(MAX_DIVQ_DIVISOR);
> +               *vco_rate = MIN_VCO_FREQ;
> +       } else {
> +               divq = ilog2(s);
> +               *vco_rate = target_rate << divq;
> +       }
> +
> +wcd_out:
> +       return divq;
> +}
> +
> +/**
> + * __wrpll_update_parent_rate() - update PLL data when parent rate changes
> + * @c: ptr to a struct analogbits_wrpll_cfg record to write PLL data to
> + * @parent_rate: PLL input refclk rate (pre-R-divider)
> + *
> + * Pre-compute some data used by the PLL configuration algorithm when
> + * the PLL's reference clock rate changes.  The intention is to avoid
> + * computation when the parent rate remains constant - expected to be
> + * the common case.
> + *
> + * Returns: 0 upon success or -1 if the reference clock rate is out of range.
> + */
> +static int __wrpll_update_parent_rate(struct analogbits_wrpll_cfg *c,
> +                                     unsigned long parent_rate)
> +{
> +       u8 max_r_for_parent;

Why not just unsigned long or unsigned int?

> +
> +       if (parent_rate > MAX_INPUT_FREQ || parent_rate < MIN_POST_DIVR_FREQ)
> +               return -1;
> +
> +       c->parent_rate = parent_rate;
> +       max_r_for_parent = div_u64(parent_rate, MIN_POST_DIVR_FREQ);
> +       c->max_r = min_t(u8, MAX_DIVR_DIVISOR, max_r_for_parent);

Then this min_t can be min() which is simpler to reason about.

> +
> +       /* Round up */
> +       c->init_r = div_u64(parent_rate + MAX_POST_DIVR_FREQ - 1,
> +                           MAX_POST_DIVR_FREQ);

Don't we have DIV_ROUND_UP_ULL() for this?

> +
> +       return 0;
> +}
> +
> +/**
> + * analogbits_wrpll_configure() - compute PLL configuration for a target rate
> + * @c: ptr to a struct analogbits_wrpll_cfg record to write into
> + * @target_rate: target PLL output clock rate (post-Q-divider)
> + * @parent_rate: PLL input refclk rate (pre-R-divider)
> + *
> + * Given a pointer to a PLL context @c, a desired PLL target output
> + * rate @target_rate, and a reference clock input rate @parent_rate,
> + * compute the appropriate PLL signal configuration values.  PLL

I don't know if we need to repeat the arguments and their description
again in kernel-doc's first sentence. Maybe just "Compute the
appropriate PLL signal configuration values and store in PLL context
@c. PLL reprogramming is not ..."

> + * reprogramming is not glitchless, so the caller should switch any
> + * downstream logic to a different clock source or clock-gate it
> + * before presenting these values to the PLL configuration signals.
> + *
> + * The caller must pass this function a pre-initialized struct
> + * analogbits_wrpll_cfg record: either initialized to zero (with the
> + * exception of the .name and .flags fields) or read from the PLL.
> + *
> + * Context: Any context.  Caller must protect the memory pointed to by @c
> + *          from simultaneous access or modification.
> + *
> + * Return: 0 upon success; anything else upon failure.
> + */
> +int analogbits_wrpll_configure_for_rate(struct analogbits_wrpll_cfg *c,
> +                                       u32 target_rate,

Why does it need to be u32? Why not unsigned long?

> +                                       unsigned long parent_rate)
> +{
> +       unsigned long ratio;
> +       u64 target_vco_rate, delta, best_delta, f_pre_div, vco, vco_pre;
> +       u32 best_f, f, post_divr_freq;
> +       u8 fbdiv, divq, best_r, r;
> +
> +       if (c->flags == 0) {
> +               WARN(1, "%s called with uninitialized PLL config", __func__);
> +               return -1;

Please return linux error codes instead of -1. -EINVAL?

> +       }
> +
> +       /* Initialize rounding data if it hasn't been initialized already */
> +       if (parent_rate != c->parent_rate) {
> +               if (__wrpll_update_parent_rate(c, parent_rate)) {
> +                       pr_err("%s: PLL input rate is out of range\n",
> +                              __func__);
> +                       return -1;
> +               }
> +       }
> +
> +       c->flags &= ~WRPLL_FLAGS_RESET_MASK;
> +
> +       /* Put the PLL into bypass if the user requests the parent clock rate */
> +       if (target_rate == parent_rate) {
> +               c->flags |= WRPLL_FLAGS_BYPASS_MASK;
> +               return 0;
> +       }
> +       c->flags &= ~WRPLL_FLAGS_BYPASS_MASK;

Nitpick: Detach this from the above if so that we can more clearly see
the return 0 in the if statement.

> +
> +       /* Calculate the Q shift and target VCO rate */
> +       divq = __wrpll_calc_divq(target_rate, &target_vco_rate);
> +       if (divq == 0)

It's more normal style to write this as if (!divq)

> +               return -1;
> +       c->divq = divq;
> +
> +       /* Precalculate the pre-Q divider target ratio */
> +       ratio = div64_u64((target_vco_rate << ROUND_SHIFT), parent_rate);
> +
> +       fbdiv = __wrpll_calc_fbdiv(c);
> +       best_r = 0;
> +       best_f = 0;
> +       best_delta = MAX_VCO_FREQ;
> +
> +       /*
> +        * Consider all values for R which land within
> +        * [MIN_POST_DIVR_FREQ, MAX_POST_DIVR_FREQ]; prefer smaller R
> +        */
> +       for (r = c->init_r; r <= c->max_r; ++r) {
> +               /* What is the best F we can pick in this case? */

Is this a TODO?

> +               f_pre_div = ratio * r;
> +               f = (f_pre_div + (1 << ROUND_SHIFT)) >> ROUND_SHIFT;
> +               f >>= (fbdiv - 1);
> +
> +               post_divr_freq = div_u64(parent_rate, r);
> +               vco_pre = fbdiv * post_divr_freq;
> +               vco = vco_pre * f;
> +
> +               /* Ensure rounding didn't take us out of range */
> +               if (vco > target_vco_rate) {
> +                       --f;
> +                       vco = vco_pre * f;
> +               } else if (vco < MIN_VCO_FREQ) {
> +                       ++f;
> +                       vco = vco_pre * f;
> +               }
> +
> +               delta = abs(target_rate - vco);
> +               if (delta < best_delta) {
> +                       best_delta = delta;
> +                       best_r = r;
> +                       best_f = f;
> +               }
> +       }
> +
> +       c->divr = best_r - 1;
> +       c->divf = best_f - 1;
> +
> +       post_divr_freq = div_u64(parent_rate, best_r);
> +
> +       /* Pick the best PLL jitter filter */
> +       c->range = __wrpll_calc_filter_range(post_divr_freq);

This can return -1 (really should be an error code). Check the return
value and then assign?

> +
> +       return 0;
> +}
> +
> +/**
> + * analogbits_wrpll_calc_output_rate() - calculate the PLL's target output rate
> + * @c: ptr to a struct analogbits_wrpll_cfg record to read from
> + * @parent_rate: PLL refclk rate
> + *
> + * Given a pointer to the PLL's current input configuration @c and the
> + * PLL's input reference clock rate @parent_rate (before the R
> + * pre-divider), calculate the PLL's output clock rate (after the Q
> + * post-divider)
> + *
> + * Context: Any context.  Caller must protect the memory pointed to by @c
> + *          from simultaneous modification.
> + *
> + * Return: the PLL's output clock rate, in Hz.
> + */
> +unsigned long analogbits_wrpll_calc_output_rate(struct analogbits_wrpll_cfg *c,

Can c be const?

> +                                               unsigned long parent_rate)
> +{
> +       u8 fbdiv;
> +       u64 n;
> +
> +       WARN(c->flags & WRPLL_FLAGS_EXT_FEEDBACK_MASK,
> +            "external feedback mode not yet supported");

Should we return then?

> +
> +       fbdiv = __wrpll_calc_fbdiv(c);
> +       n = parent_rate * fbdiv * (c->divf + 1);
> +       n = div_u64(n, (c->divr + 1));

Drop useless parenthesis?

> +       n >>= c->divq;
> +
> +       return n;
> +}
> +
> +/**
> + * analogbits_wrpll_calc_max_lock_us() - return the time for the PLL to lock
> + * @c: ptr to a struct analogbits_wrpll_cfg record to read from
> + *
> + * Return the minimum amount of time (in microseconds) that the caller
> + * must wait after reprogramming the PLL to ensure that it is locked
> + * to the input frequency and stable.  This is likely to depend on the DIVR
> + * value; this is under discussion with the manufacturer.
> + *
> + * Return: the minimum amount of time the caller must wait for the PLL
> + *         to lock (in microseconds)
> + */
> +unsigned int analogbits_wrpll_calc_max_lock_us(struct analogbits_wrpll_cfg *c)

Can c be const?

> +{
> +       return MAX_LOCK_US;
> +}
> diff --git a/include/linux/clk/analogbits-wrpll-cln28hpc.h b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> new file mode 100644
> index 000000000000..f8dc732086fc
> --- /dev/null
> +++ b/include/linux/clk/analogbits-wrpll-cln28hpc.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 SiFive, Inc.
> + * Wesley Terpstra
> + * Paul Walmsley
> + *
> + * 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.

We don't need this boiler plate now that we have SPDX. Please remove it.

> + */
> +
> +#ifndef __LINUX_CLK_ANALOGBITS_WRPLL_CLN28HPC_H
> +#define __LINUX_CLK_ANALOGBITS_WRPLL_CLN28HPC_H
> +
> +#include <linux/types.h>
> +
> +/* DIVQ_VALUES: number of valid DIVQ values */
> +#define DIVQ_VALUES                            6
> +
> +/*
> + * Bit definitions for struct analogbits_wrpll_cfg.flags
> + *
> + * WRPLL_FLAGS_BYPASS_FLAG: if set, the PLL is either in bypass, or should be
> + *     programmed to enter bypass
> + * WRPLL_FLAGS_RESET_FLAG: if set, the PLL is in reset
> + * WRPLL_FLAGS_INT_FEEDBACK_FLAG: if set, the PLL is configured for internal
> + *     feedback mode
> + * WRPLL_FLAGS_EXT_FEEDBACK_FLAG: if set, the PLL is configured for external
> + *     feedback mode (not yet supported by this driver)
> + *
> + * The flags WRPLL_FLAGS_INT_FEEDBACK_FLAG and WRPLL_FLAGS_EXT_FEEDBACK_FLAG are

These flags aren't defined anywhere though? Instead they're shifts and
masks below?

> + * mutually exclusive.  If both bits are set, or both are zero, the struct
> + * analogbits_wrpll_cfg record is uninitialized or corrupt.
> + */
> +#define WRPLL_FLAGS_BYPASS_SHIFT               0
> +#define WRPLL_FLAGS_BYPASS_MASK                BIT(WRPLL_FLAGS_BYPASS_SHIFT)
> +#define WRPLL_FLAGS_RESET_SHIFT                1
> +#define WRPLL_FLAGS_RESET_MASK         BIT(WRPLL_FLAGS_RESET_SHIFT)
> +#define WRPLL_FLAGS_INT_FEEDBACK_SHIFT 2
> +#define WRPLL_FLAGS_INT_FEEDBACK_MASK  BIT(WRPLL_FLAGS_INT_FEEDBACK_SHIFT)
> +#define WRPLL_FLAGS_EXT_FEEDBACK_SHIFT 3
> +#define WRPLL_FLAGS_EXT_FEEDBACK_MASK  BIT(WRPLL_FLAGS_EXT_FEEDBACK_SHIFT)

Maybe you can use FIELD_GET/FIELD_SET?

> +
> +/**
> + * struct analogbits_wrpll_cfg - WRPLL configuration values
> + * @divr: reference divider value (6 bits), as presented to the PLL signals
> + * @divf: feedback divider value (9 bits), as presented to the PLL signals
> + * @divq: output divider value (3 bits), as presented to the PLL signals
> + * @flags: PLL configuration flags.  See above for more information
> + * @range: PLL loop filter range.  See below for more information
> + * @output_rate_cache: cached output rates, swept across DIVQ
> + * @parent_rate: PLL refclk rate for which values are valid
> + * @max_r: maximum possible R divider value, given @parent_rate
> + * @init_r: initial R divider value to start the search from
> + *
> + * @divr, @divq, @divq, @range represent what the PLL expects to see
> + * on its input signals.  Thus @divr and @divf are the actual divisors
> + * minus one.  @divq is a power-of-two divider; for example, 1 =
> + * divide-by-2 and 6 = divide-by-64.  0 is an invalid @divq value.
> + *
> + * When initially passing a struct analogbits_wrpll_cfg record, the
> + * record should be zero-initialized with the exception of the @flags
> + * field.  The only flag bits that need to be set are either
> + * WRPLL_FLAGS_INT_FEEDBACK or WRPLL_FLAGS_EXT_FEEDBACK.
> + *
> + * Field names beginning with an underscore should be considered
> + * private to the wrpll-cln28hpc.c code.

This sentence can be removed.

> + */
> +struct analogbits_wrpll_cfg {
> +       u8 divr;
> +       u8 divq;
> +       u8 range;
> +       u8 flags;
> +       u16 divf;
> +/* private: */
> +       u32 output_rate_cache[DIVQ_VALUES];
> +       unsigned long parent_rate;
> +       u8 max_r;
> +       u8 init_r;
> +};
> +
> +int analogbits_wrpll_configure_for_rate(struct analogbits_wrpll_cfg *c,
> +                                       u32 target_rate,
> +                                       unsigned long parent_rate);
> +
> +unsigned int analogbits_wrpll_calc_max_lock_us(struct analogbits_wrpll_cfg *c);
> +
> +unsigned long analogbits_wrpll_calc_output_rate(struct analogbits_wrpll_cfg *c,
> +                                               unsigned long parent_rate);

I wonder if it may be better to remove analogbits_ from all these
exported functions. I suspect that it wouldn't conflict if it was
prefixed with wrpll_ and it's shorter this way. Up to you.
 

Powered by blists - more mailing lists