Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Quoting Paul Walmsley (2019-04-29 18:14:14)
> On Mon, 29 Apr 2019, Stephen Boyd wrote:
> >
> > Nitpick: This might be easier to read with a switch statement:
> >
> > switch (post_divr_freq) {
> > case 0 ... 1100:
> > return 1;
> > case 1101 ... 1800:
> > return 2;
> > case 1801 ... 3000:
> > return 3;
> > case 3001 ... 5000:
> > return 4;
> > case 5000 ... 8000:
> > return 5;
> > case 8001 ... 13000:
> > return 6;
> > }
> >
> > return 7;
>
> To be equivalent to the original code, we'd need to write:
>
>switch (post_divr_freq) {
>case 0 ... 1099:
>return 1;
>case 1100 ... 1799:
>return 2;
> (etc.)
>
> In any case, it's been changed to use the gcc case range operator.
Ah right, thanks!
> > > +{
> > > + 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?
>
> One motivation here is that this function returns a divisor value. So a
> zero represents a divide by zero, which is intrinsically an error for a
> function that returns a divisor. The other motivation is that the current
> return value directly maps to what the hardware expects to see.
>
> Let me know if you want me to change this anyway.
Ok, sounds fine.
>
> > > + */
> > > +static u8 __wrpll_calc_divq(u32 target_rate, u64 *vco_rate)
> >
> > Why does target_rate need to be u32?
>
> I don't think there's any specific requirement for it to be a u32.
Ok.
>
> > Can it be unsigned long?
>
> There are two basic principles motivating this:
>
> 1. Use the shortest type that fits what will be contained in the variable.
>This increases the chance that static analysis will catch any
>inadvertent overflows (for example, via gcc -Woverflow).
>
> 2. Use fixed-width types for hardware-constrained values that are
>unrelated to the CPU's native word length. This is a general design
>practice, both to avoid confusion as to whether the variable's range
>does in fact depend on the compiler's implementation, and to avoid API
>problems if the width does change. Although this last case doesn't
>apply here, the general application of this practice avoids problems
>like the longstanding API problem we've had with clk_set_rate(), which
>can't take a 64 bit clock rate argument if the kernel is built with a
>compiler that uses 32 bit longs.
>
Sure, makes sense.
>
> > > +
> > > + 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.
>
> To me they have the same meaning - min_t doesn't seem too obscure:
>
> $ fgrep -Ir min_t\( linux/ | wc -l
> 3320
> $
>
> and using it means we don't have to use a type that's needlessly large for
> the range of values that the variable will contain. However, if getting
> rid of min_t() is more important to you than that principle, it can
> of course be changed. Do you feel strongly about it?
It's not about obscurity. min_t() is an indicator that we have to coerce
type for comparison with casting. It's nice to avoid casts if possible
and use native types for the comparison. assignment. It's good that you
aren't using min() with a cast though, so this look fine to me and I'm
not going to stay worried about this.
> > > + * mutually exclusive. If both bits are set, or both are zero, the
> > > struct
> > > + * analogbits_wrpll_cfg record is uninitialized or corrupt.
> > > + */
> > > +#define
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
On Tue, 30 Apr 2019, Stephen Boyd wrote: > Quoting Paul Walmsley (2019-04-29 22:57:15) > > On Mon, 29 Apr 2019, Stephen Boyd wrote: > > > > > Quoting Paul Walmsley (2019-04-29 12:42:07) > > > > On Fri, 26 Apr 2019, Paul Walmsley wrote: > > > > > On Fri, 26 Apr 2019, Stephen Boyd wrote: > > > > > > > > > > > Quoting Paul Walmsley (2019-04-11 01:27:32) > > > > > > > Add common library code for the Analog Bits Wide-Range PLL > > > > > > > (WRPLL) IP > > > > > > > block, as implemented in TSMC CLN28HPC. > > > > > > > > > > > > I haven't deeply reviewed at all, but I already get two problems > > > > > > when > > > > > > compile testing these patches. I can fix them up if nothing else > > > > > > needs > > > > > > fixing. > > > > > > > > > > > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() > > > > > > warn: should 'target_rate << divq' be a 64 bit type? > > > > > > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in > > > > > > void function > > > > > > > > > > Hmm, that's odd. I will definitely take a look and repost. > > > > > > > > I'm not able to reproduce these problems. The configs tried here were: > > > > > > > > - 64-bit RISC-V defconfig w/ PRCI driver enabled (gcc 8.2.0 built with > > > > crosstool-NG 1.24.0) > > > > > > > > - 32-bit ARM defconfig w/ PRCI driver enabled (gcc 8.3.0 built with > > > > crosstool-NG 1.24.0) > > > > > > > > - 32-bit i386 defconfig w/ PRCI driver enabled (gcc > > > > 5.4.0-6ubuntu1~16.04.11) > > > > > > > > Could you post the toolchain and kernel config you're using? > > > > > > > > > > I'm running sparse and smatch too. > > > > OK. I was able to reproduce the __wrpll_calc_divq() warning. It's been > > resolved in the upcoming revision. > > > > But I don't see the second error with either sparse or smatch. (This is > > with sparse at commit 2b96cd804dc7 and smatch at commit f0092daff69d.) > > > > Weird! The return in void function is pretty obvious though so please > fix it regardless. Done. - Paul
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Quoting Paul Walmsley (2019-04-29 22:57:15) > On Mon, 29 Apr 2019, Stephen Boyd wrote: > > > Quoting Paul Walmsley (2019-04-29 12:42:07) > > > On Fri, 26 Apr 2019, Paul Walmsley wrote: > > > > On Fri, 26 Apr 2019, Stephen Boyd wrote: > > > > > > > > > Quoting Paul Walmsley (2019-04-11 01:27:32) > > > > > > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) > > > > > > IP > > > > > > block, as implemented in TSMC CLN28HPC. > > > > > > > > > > I haven't deeply reviewed at all, but I already get two problems when > > > > > compile testing these patches. I can fix them up if nothing else needs > > > > > fixing. > > > > > > > > > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: > > > > > should 'target_rate << divq' be a 64 bit type? > > > > > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in > > > > > void function > > > > > > > > Hmm, that's odd. I will definitely take a look and repost. > > > > > > I'm not able to reproduce these problems. The configs tried here were: > > > > > > - 64-bit RISC-V defconfig w/ PRCI driver enabled (gcc 8.2.0 built with > > > crosstool-NG 1.24.0) > > > > > > - 32-bit ARM defconfig w/ PRCI driver enabled (gcc 8.3.0 built with > > > crosstool-NG 1.24.0) > > > > > > - 32-bit i386 defconfig w/ PRCI driver enabled (gcc > > > 5.4.0-6ubuntu1~16.04.11) > > > > > > Could you post the toolchain and kernel config you're using? > > > > > > > I'm running sparse and smatch too. > > OK. I was able to reproduce the __wrpll_calc_divq() warning. It's been > resolved in the upcoming revision. > > But I don't see the second error with either sparse or smatch. (This is > with sparse at commit 2b96cd804dc7 and smatch at commit f0092daff69d.) > Weird! The return in void function is pretty obvious though so please fix it regardless.
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
On Mon, 29 Apr 2019, Stephen Boyd wrote: > Quoting Paul Walmsley (2019-04-29 12:42:07) > > On Fri, 26 Apr 2019, Paul Walmsley wrote: > > > On Fri, 26 Apr 2019, Stephen Boyd wrote: > > > > > > > Quoting Paul Walmsley (2019-04-11 01:27:32) > > > > > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP > > > > > block, as implemented in TSMC CLN28HPC. > > > > > > > > I haven't deeply reviewed at all, but I already get two problems when > > > > compile testing these patches. I can fix them up if nothing else needs > > > > fixing. > > > > > > > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: > > > > should 'target_rate << divq' be a 64 bit type? > > > > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in > > > > void function > > > > > > Hmm, that's odd. I will definitely take a look and repost. > > > > I'm not able to reproduce these problems. The configs tried here were: > > > > - 64-bit RISC-V defconfig w/ PRCI driver enabled (gcc 8.2.0 built with > > crosstool-NG 1.24.0) > > > > - 32-bit ARM defconfig w/ PRCI driver enabled (gcc 8.3.0 built with > > crosstool-NG 1.24.0) > > > > - 32-bit i386 defconfig w/ PRCI driver enabled (gcc > > 5.4.0-6ubuntu1~16.04.11) > > > > Could you post the toolchain and kernel config you're using? > > > > I'm running sparse and smatch too. OK. I was able to reproduce the __wrpll_calc_divq() warning. It's been resolved in the upcoming revision. But I don't see the second error with either sparse or smatch. (This is with sparse at commit 2b96cd804dc7 and smatch at commit f0092daff69d.) - Paul
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
On Mon, 29 Apr 2019, Stephen Boyd wrote:
> 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 ..b5fd60c7f136
> > --- /dev/null
> > +++ b/drivers/clk/analogbits/Kconfig
> > @@ -0,0 +1,2 @@
>
> Add SPDX for this file?
Done.
> > +config CLK_ANALOGBITS_WRPLL_CLN28HPC
> > + bool
> > diff --git a/drivers/clk/analogbits/Makefile
> > b/drivers/clk/analogbits/Makefile
> > new file mode 100644
> > index ..bb51a3ae77a7
> > --- /dev/null
> > +++ b/drivers/clk/analogbits/Makefile
> > @@ -0,0 +1 @@
>
> Add SPDX for this file?
Done.
> > +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 ..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
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* MIN_INPUT_FREQ: minimum input clock frequency, in Hz (Fref_min) */
> > +#define MIN_INPUT_FREQ 700
> > +
> > +/* MAX_INPUT_FREQ: maximum input clock frequency, in Hz (Fref_max) */
> > +#define MAX_INPUT_FREQ 6
> > +
> > +/* MIN_POST_DIVIDE_REF_FREQ: minimum post-divider reference frequency, in
> > Hz */
> > +#define MIN_POST_DIVR_FREQ 700
> > +
> > +/* MAX_POST_DIVIDE_REF_FREQ: maximum post-divider reference frequency, in
> > Hz */
> > +#define MAX_POST_DIVR_FREQ 2
> > +
> > +/* MIN_VCO_FREQ: minimum VCO frequency, in Hz (Fvco_min) */
> > +#define MIN_VCO_FREQ 24UL
> > +
> > +/* MAX_VCO_FREQ: maximum VCO frequency, in Hz (Fvco_max) */
> > +#define MAX_VCO_FREQ 48ULL
> > +
> > +/* 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_US70
> > +
> > +/*
> > + * ROUND_SHIFT: number of bits to shift to avoid precision loss in the
> > rounding
> > + * algorithm
> > + */
> > +#define ROUND_SHIFT20
> > +
> > +/*
> > + * 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 < 1100)
> > + range = 1;
> > + else if (post_divr_freq < 1800)
> > + range = 2;
> > + else if (post_divr_freq < 3000)
> > + range = 3;
> > + else if (post_divr_freq < 5000)
> > + range = 4;
> > + else if (post_divr_freq < 8000)
> > + range = 5;
> > + else if (post_divr_freq < 13000)
> > + range = 6;
> >
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Quoting Paul Walmsley (2019-04-29 12:42:07) > Hi Stephen, > > On Fri, 26 Apr 2019, Paul Walmsley wrote: > > > On Fri, 26 Apr 2019, Stephen Boyd wrote: > > > > > Quoting Paul Walmsley (2019-04-11 01:27:32) > > > > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP > > > > block, as implemented in TSMC CLN28HPC. > > > > > > I haven't deeply reviewed at all, but I already get two problems when > > > compile testing these patches. I can fix them up if nothing else needs > > > fixing. > > > > > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: > > > should 'target_rate << divq' be a 64 bit type? > > > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in void > > > function > > > > Hmm, that's odd. I will definitely take a look and repost. > > I'm not able to reproduce these problems. The configs tried here were: > > - 64-bit RISC-V defconfig w/ PRCI driver enabled (gcc 8.2.0 built with > crosstool-NG 1.24.0) > > - 32-bit ARM defconfig w/ PRCI driver enabled (gcc 8.3.0 built with > crosstool-NG 1.24.0) > > - 32-bit i386 defconfig w/ PRCI driver enabled (gcc > 5.4.0-6ubuntu1~16.04.11) > > Could you post the toolchain and kernel config you're using? > I'm running sparse and smatch too.
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 ..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 ..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 ..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
> +#include
> +#include
> +#include
> +#include
> +
> +/* MIN_INPUT_FREQ: minimum input clock frequency, in Hz (Fref_min) */
> +#define MIN_INPUT_FREQ 700
> +
> +/* MAX_INPUT_FREQ: maximum input clock frequency, in Hz (Fref_max) */
> +#define MAX_INPUT_FREQ 6
> +
> +/* MIN_POST_DIVIDE_REF_FREQ: minimum post-divider reference frequency, in Hz
> */
> +#define MIN_POST_DIVR_FREQ 700
> +
> +/* MAX_POST_DIVIDE_REF_FREQ: maximum post-divider reference frequency, in Hz
> */
> +#define MAX_POST_DIVR_FREQ 2
> +
> +/* MIN_VCO_FREQ: minimum VCO frequency, in Hz (Fvco_min) */
> +#define MIN_VCO_FREQ 24UL
> +
> +/* MAX_VCO_FREQ: maximum VCO frequency, in Hz (Fvco_max) */
> +#define MAX_VCO_FREQ 48ULL
> +
> +/* 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_US70
> +
> +/*
> + * ROUND_SHIFT: number of bits to shift to avoid precision loss in the
> rounding
> + * algorithm
> + */
> +#define ROUND_SHIFT20
> +
> +/*
> + * 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 < 1100)
> + range = 1;
> + else if (post_divr_freq < 1800)
> + range = 2;
> + else if (post_divr_freq < 3000)
> + range = 3;
> + else if (post_divr_freq < 5000)
> + range = 4;
> + else if (post_divr_freq < 8000)
> + range = 5;
> + else if (post_divr_freq < 13000)
> + range = 6;
> + else
> + range = 7;
Nitpick: This might be easier to read with a switch statement:
switch (post_divr_freq) {
case 0 ... 1100:
return 1;
case 1101 ... 1800:
return 2;
case 1801 ... 3000:
return 3;
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Hi Stephen, On Fri, 26 Apr 2019, Paul Walmsley wrote: > On Fri, 26 Apr 2019, Stephen Boyd wrote: > > > Quoting Paul Walmsley (2019-04-11 01:27:32) > > > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP > > > block, as implemented in TSMC CLN28HPC. > > > > I haven't deeply reviewed at all, but I already get two problems when > > compile testing these patches. I can fix them up if nothing else needs > > fixing. > > > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: > > should 'target_rate << divq' be a 64 bit type? > > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in void > > function > > Hmm, that's odd. I will definitely take a look and repost. I'm not able to reproduce these problems. The configs tried here were: - 64-bit RISC-V defconfig w/ PRCI driver enabled (gcc 8.2.0 built with crosstool-NG 1.24.0) - 32-bit ARM defconfig w/ PRCI driver enabled (gcc 8.3.0 built with crosstool-NG 1.24.0) - 32-bit i386 defconfig w/ PRCI driver enabled (gcc 5.4.0-6ubuntu1~16.04.11) Could you post the toolchain and kernel config you're using? - Paul
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
On Fri, 26 Apr 2019, Stephen Boyd wrote: > Quoting Paul Walmsley (2019-04-11 01:27:32) > > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP > > block, as implemented in TSMC CLN28HPC. > > I haven't deeply reviewed at all, but I already get two problems when > compile testing these patches. I can fix them up if nothing else needs > fixing. > > drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: should > 'target_rate << divq' be a 64 bit type? > drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in void > function Hmm, that's odd. I will definitely take a look and repost. - Paul
Re: [PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Quoting Paul Walmsley (2019-04-11 01:27:32) > Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP > block, as implemented in TSMC CLN28HPC. > > There is no bus interface or register target associated with this PLL. > This library is intended to be used by drivers for IP blocks that > expose registers connected to the PLL configuration and status > signals. > > Based on code originally written by Wesley Terpstra > : > https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60 > > This version incorporates several changes requested by Stephen > Boyd . > > Signed-off-by: Paul Walmsley > Signed-off-by: Paul Walmsley > Cc: Wesley Terpstra > Cc: Palmer Dabbelt > Cc: Michael Turquette > Cc: Stephen Boyd > Cc: Megan Wachs > Cc: linux-...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- I haven't deeply reviewed at all, but I already get two problems when compile testing these patches. I can fix them up if nothing else needs fixing. drivers/clk/analogbits/wrpll-cln28hpc.c:165 __wrpll_calc_divq() warn: should 'target_rate << divq' be a 64 bit type? drivers/clk/sifive/fu540-prci.c:214:16: error: return expression in void function
[PATCH v3 1/3] clk: analogbits: add Wide-Range PLL library
Add common library code for the Analog Bits Wide-Range PLL (WRPLL) IP block, as implemented in TSMC CLN28HPC. There is no bus interface or register target associated with this PLL. This library is intended to be used by drivers for IP blocks that expose registers connected to the PLL configuration and status signals. Based on code originally written by Wesley Terpstra : https://github.com/riscv/riscv-linux/commit/999529edf517ed75b56659d456d221b2ee56bb60 This version incorporates several changes requested by Stephen Boyd . Signed-off-by: Paul Walmsley Signed-off-by: Paul Walmsley Cc: Wesley Terpstra Cc: Palmer Dabbelt Cc: Michael Turquette Cc: Stephen Boyd Cc: Megan Wachs Cc: linux-...@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- MAINTAINERS | 6 + drivers/clk/Kconfig | 1 + drivers/clk/Makefile | 1 + drivers/clk/analogbits/Kconfig| 2 + drivers/clk/analogbits/Makefile | 1 + drivers/clk/analogbits/wrpll-cln28hpc.c | 360 ++ include/linux/clk/analogbits-wrpll-cln28hpc.h | 96 + 7 files changed, 467 insertions(+) create mode 100644 drivers/clk/analogbits/Kconfig create mode 100644 drivers/clk/analogbits/Makefile create mode 100644 drivers/clk/analogbits/wrpll-cln28hpc.c create mode 100644 include/linux/clk/analogbits-wrpll-cln28hpc.h diff --git a/MAINTAINERS b/MAINTAINERS index 2359e12e4c41..3ec37f17f90e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -960,6 +960,12 @@ F: drivers/iio/adc/ltc2497* X: drivers/iio/*/adjd* F: drivers/staging/iio/*/ad* +ANALOGBITS PLL LIBRARIES +M: Paul Walmsley +S: Supported +F: drivers/clk/analogbits/* +F: include/linux/clk/analogbits* + ANDES ARCHITECTURE M: Greentime Hu M: Vincent Chen diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index e705aab9e38b..83dc90bd5179 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -297,6 +297,7 @@ config COMMON_CLK_FIXED_MMIO Support for Memory Mapped IO Fixed clocks source "drivers/clk/actions/Kconfig" +source "drivers/clk/analogbits/Kconfig" source "drivers/clk/bcm/Kconfig" source "drivers/clk/hisilicon/Kconfig" source "drivers/clk/imgtec/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 1db133652f0c..091ee1d8af65 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_COMMON_CLK_XGENE)+= clk-xgene.o # please keep this section sorted lexicographically by directory path name obj-y += actions/ +obj-y += analogbits/ obj-$(CONFIG_COMMON_CLK_AT91) += at91/ obj-$(CONFIG_ARCH_ARTPEC) += axis/ obj-$(CONFIG_ARC_PLAT_AXS10X) += axs10x/ diff --git a/drivers/clk/analogbits/Kconfig b/drivers/clk/analogbits/Kconfig new file mode 100644 index ..b5fd60c7f136 --- /dev/null +++ b/drivers/clk/analogbits/Kconfig @@ -0,0 +1,2 @@ +config CLK_ANALOGBITS_WRPLL_CLN28HPC + bool diff --git a/drivers/clk/analogbits/Makefile b/drivers/clk/analogbits/Makefile new file mode 100644 index ..bb51a3ae77a7 --- /dev/null +++ b/drivers/clk/analogbits/Makefile @@ -0,0 +1 @@ +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 ..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 +#include +#include +#include +#include + +/* MIN_INPUT_FREQ: minimum input clock frequency, in Hz (Fref_min) */ +#define MIN_INPUT_FREQ 700 + +/* MAX_INPUT_FREQ: maximum input clock frequency, in Hz (Fref_max) */ +#define MAX_INPUT_FREQ 6 + +/* MIN_POST_DIVIDE_REF_FREQ: minimum post-divider reference frequency, in Hz */ +#define MIN_POST_DIVR_FREQ 700 +
